[i2c] [patch 2.6.20-rc1 3/6] other i2c code stops using i2c_adapter.dev

[David Brownell]

On Monday 18 December 2006 11:02 am, Jean Delvare wrote:

> > @@ -284,7 +287,8 @@ int i2c_del_adapter(struct i2c_adapter *
> >  	/* free dynamically allocated bus id */
> >  	idr_remove(&i2c_adapter_idr, adap->nr);
> >  
> > -	dev_dbg(&adap->dev, "adapter [%s] unregistered\n", adap->name);
> > +	dev_dbg(dev, "adapter %s [%s] unregistered\n",
> > +			adap->class_dev.class_id, adap->name);
> 
> You modify the format slightly here, and I am fine with that change,
> but you should do the same at registration time for consistency.

OK ... moving that change from a later patch to this one.

Note that this is preserving content, since previously "adap->dev" was
a synthetic name mostly equivalent to the class name, but "dev" uses
the real hardware name.  So this fixes a diagnostic bug too:   previous
text made it hard to associate the hardware and its bus ID.


> > @@ -426,9 +433,9 @@ int i2c_attach_client(struct i2c_client 
> >  
> >  	snprintf(&client->dev.bus_id[0], sizeof(client->dev.bus_id),
> >  		"%d-%04x", i2c_adapter_id(adapter), client->addr);
> > -	dev_dbg(&adapter->dev, "client [%s] registered with bus id %s\n",
> > -		client->name, client->dev.bus_id);
> >  	res = device_register(&client->dev);
> > +	dev_dbg(&client->dev, "attach client [%s] --> %d\n",
> > +		client->name, res);
> 
> This change doesn't belong here. I agree that the debugging messages in
> i2c-core are far from optimal, but we'd rather improve them after
> fixing the i2c core rather before.

I was going for "as part of"; at some level, fixing core messages
is just fixing core messages.  I'll revert that, but that makes it
unlikely the problems with these messages will get fixed any time
soon.  (Notice how someone debugging a problem in this area doesn't
get enough information to sort out what's gone wrong when faults
show up later in the sequence... and how the original messages
reported the core successfully "registered" before even making the
call to do so!!)


> > @@ -452,7 +459,7 @@ out_unregister:
> >  	wait_for_completion(&client->released);
> >  out_list:
> >  	list_del(&client->list);
> > -	dev_err(&adapter->dev, "Failed to attach i2c client %s at 0x%02x "
> > +	dev_err(&client->dev, "Failed to attach i2c client %s at 0x%02x "
> >  		"(%d)\n", client->name, client->addr, res);
> >  out_unlock:
> >  	mutex_unlock(&adapter->clist_lock);
> 
> What's the rationale for changing the adapter-based messages above to
> client-based ones?

They have to change anyway, and these are fundamentally messages about the
client being registered, not the adapter.  Plus as I implied above:  in all
code, diagnostics get bugfixed so infrequently that I've learned that second
passes virtually never happen.

I'll change those back.



> > @@ -1511,13 +1511,14 @@ static int isp1301_probe(struct i2c_adap
> >  	if (kind < 0) {
> >  		status = isp1301_get_u16(isp, ISP1301_VENDOR_ID);
> >  		if (status != I2C_VENDOR_ID_PHILIPS) {
> > -			dev_dbg(&bus->dev, "addr %d not philips id: %d\n",
> > +			dev_dbg(bus->class_dev.dev,
> > +				"addr %d not philips id: %d\n",
> >  				address, status);
> >  			goto fail1;
> >  		}
> >  		status = isp1301_get_u16(isp, ISP1301_PRODUCT_ID);
> >  		if (status != I2C_PRODUCT_ID_PHILIPS_1301) {
> > -			dev_dbg(&bus->dev, "%d not isp1301, %d\n",
> > +			dev_dbg(bus->class_dev, "%d not isp1301, %d\n",
> >  				address, status);
> 
> Looks like a bug to me, should be bus->class_dev.dev?

Yes.  I must not have build-tested this driver.

It's kind of awkward, since the boards that need it can't really work
right with mainline kernels until _all_ the driver model updates get
merged, and then they convert to "new style" binding ... plus, most
such boards have slightly newer silicon, which changed the state machine
changed in some subtly incompatible ways that are a real PITA to test.
Sigh.