[RFC] I2C: Remove the i2c_client id field

[RFC] I2C: Remove the i2c_client id field

[Jean Delvare]

Hi Greg, hi all,

While porting various hardware monitoring drivers to Linux 2.6 and
otherwise working on i2c drivers in 2.6, I found that the i2c_client
structure has an "id" field (of type int) which is mostly unused. I am
not exactly sure why it was introduced in the first place, and since the
i2c subsystem code was significantly reworked since, it might not
actually matter.

Most hardware monitoring drivers allocate a unique (per driver) id
through an incremented static global variable, and never use it. Some
(lm85 and most notably adm1026) use the value in debug messages. I saw
various video drivers appending the id value to the client name between
square brackets, while others would set the id field to -1 and then
leave it alone. The i2c core itself doesn't use this field.

Using this field to identify a client doesn't make much sense to me, for
the following reasons:

1* A client is already uniquely identified by the combination of the
number of the bus it sits on and the address it is located at on this
bus.

2* With the implementation described above, the id will possibly change
depending on which i2c bus drivers are loaded and the order they were
loaded in. As a consequence, you can't rely on its value from
user-space, and its usability in kernel-space isn't obvious either.

3* As a matter of fact, no driver in the kernel tree uses this field
except for debugging (and even then with no obvious benefit), with only
a few exceptions where I could easily change the code so it wouldn't
need this field anymore.

Thus, I propose that we simply get rid of this field, so as to save some
memory space and kill some useless code. If anyone really ever needs to
carry some sort of id attached to an i2c_client structure, this is
private data and can be added to whatever structure the data field is
pointing to for this particular driver.

Unless someone objects with valid reasons, I am going to send patches to
kill the i2c_client id field. I have everything ready, but don't know
exactly how I should send them. The difficulty comes from the relatively
large number of affected drivers (50) and the fact that they spread over
very different subsystems (the big ones are i2c and media/video, plus
half a dozen drivers in acorn/char, macintoch and sound).

Greg, can you tell me if you would take such a patch, and how I would
have to split it for you to accept it?

Thanks,
-- 
Jean Delvare
http://khali.linux-fr.org/

Re: [RFC] I2C: Remove the i2c_client id field

[Philip Pokorny]

So only the drives I wrote use the ID in a meaningful way?

What do you propose to replace the ID value in the debug messages with? 
 Ideally it would be the things you mention that uniquely identify the 
chip in question (bus number and address)

How hard are those values to get at?  Do we have to chase possibly NULL 
pointers?

:v)

Jean Delvare wrote:

>Hi Greg, hi all,
>
>While porting various hardware monitoring drivers to Linux 2.6 and
>otherwise working on i2c drivers in 2.6, I found that the i2c_client
>structure has an "id" field (of type int) which is mostly unused. I am
>not exactly sure why it was introduced in the first place, and since the
>i2c subsystem code was significantly reworked since, it might not
>actually matter.
>
>Most hardware monitoring drivers allocate a unique (per driver) id
>through an incremented static global variable, and never use it. Some
>(lm85 and most notably adm1026) use the value in debug messages. I saw
>various video drivers appending the id value to the client name between
>square brackets, while others would set the id field to -1 and then
>leave it alone. The i2c core itself doesn't use this field.
>
>Using this field to identify a client doesn't make much sense to me, for
>the following reasons:
>
>1* A client is already uniquely identified by the combination of the
>number of the bus it sits on and the address it is located at on this
>bus.
>
>2* With the implementation described above, the id will possibly change
>depending on which i2c bus drivers are loaded and the order they were
>loaded in. As a consequence, you can't rely on its value from
>user-space, and its usability in kernel-space isn't obvious either.
>
>3* As a matter of fact, no driver in the kernel tree uses this field
>except for debugging (and even then with no obvious benefit), with only
>a few exceptions where I could easily change the code so it wouldn't
>need this field anymore.
>
>Thus, I propose that we simply get rid of this field, so as to save some
>memory space and kill some useless code. If anyone really ever needs to
>carry some sort of id attached to an i2c_client structure, this is
>private data and can be added to whatever structure the data field is
>pointing to for this particular driver.
>
>Unless someone objects with valid reasons, I am going to send patches to
>kill the i2c_client id field. I have everything ready, but don't know
>exactly how I should send them. The difficulty comes from the relatively
>large number of affected drivers (50) and the fact that they spread over
>very different subsystems (the big ones are i2c and media/video, plus
>half a dozen drivers in acorn/char, macintoch and sound).
>
>Greg, can you tell me if you would take such a patch, and how I would
>have to split it for you to accept it?
>
>Thanks,
>  
>

Re: [RFC] I2C: Remove the i2c_client id field

[Jean Delvare]

Hi Philip,

> So only the drives I wrote use the ID in a meaningful way?

True, providing we limit our consideration to the hardware monitoring
drivers. Even in your drivers, the meaningfulness is discussable.

The lm85 driver simply displays the assigned id once (at the time it
assigns it). Since the id is then never used, I would consider the lm85
driver similar to the other hardware monitoring drivers.

The adm1026 driver, OTOH, does use the id value in all debug messages,
and it also only reconfigures the GPIO pins for the first client only
(id == 0). Although this is a real use of the id, it only matters if you
use the module parameters for GPIO pins reconfiguration and actually
have more than one ADM1026 chip (a quite rare chip if you remember). You
don't necessarily know which ADM1026 will get id 0 anyway (if the chips
are on different busses it depends on the order the bus drivers were
loaded in), and I am not sure why one would want to reprogram only the
first chip. Unless someone comes with such a specific hardware setup so
that we can examine what is really needed, I think we can get rid of the
"id == 0" test and reconfigure "all" ADM1026 chips (which really is only
one for the two known boards using an ADM1026).

BTW, does anyone really use the GPIO pins reconfiguration parameters?

> What do you propose to replace the ID value in the debug messages
> with?
> Ideally it would be the things you mention that uniquely identify
> the chip in question (bus number and address)
> How hard are those values to get at?  Do we have to chase possibly
> NULL pointers?

Everything is already done. The adm1026 driver (like all other drivers
of the directory) uses dev_dbg to print its debug messages, and dev_dbg
prepends the bus number and chip address to any line it prints. This
means that the "client id" is redundant and can be removed losslessly.

Thanks,
-- 
Jean Delvare
http://khali.linux-fr.org/

Re: [RFC] I2C: Remove the i2c_client id field

[Philip Pokorny]

Jean Delvare wrote:

>Hi Philip,
>
>  
>
>>So only the drives I wrote use the ID in a meaningful way?
>>    
>>
>
>True, providing we limit our consideration to the hardware monitoring
>drivers. Even in your drivers, the meaningfulness is discussable.
>
>The lm85 driver simply displays the assigned id once (at the time it
>assigns it). Since the id is then never used, I would consider the lm85
>driver similar to the other hardware monitoring drivers.
>  
>

Oops.  I probably should have used the ID in the debug messages...  I 
guess it doesn't matter now.

>The adm1026 driver, OTOH, does use the id value in all debug messages,
>and it also only reconfigures the GPIO pins for the first client only
>(id == 0). Although this is a real use of the id, it only matters if you
>use the module parameters for GPIO pins reconfiguration and actually
>have more than one ADM1026 chip (a quite rare chip if you remember).
>
Not for me.  We ship hundreds of systems each month with a motherboard 
with that chip on it.  I think it's actually on two different 
motherboards we sell.

> You
>don't necessarily know which ADM1026 will get id 0 anyway (if the chips
>are on different busses it depends on the order the bus drivers were
>loaded in),
>
Wouldn't a force_xxx parameter cause a specific bus/id to be probed and 
assigned first?

> and I am not sure why one would want to reprogram only the
>first chip. Unless someone comes with such a specific hardware setup so
>that we can examine what is really needed,
>
Well, that's exactly the problem that I had.  The motherboard vendor's 
BIOS didn't set the chip up and I had to program it myself.  I got the 
schematics from the vendor for the part of the motherboard attached to 
the chip so that I could program it correctly.

> I think we can get rid of the
>"id == 0" test and reconfigure "all" ADM1026 chips (which really is only
>one for the two known boards using an ADM1026).
>  
>
I think that would be a bad idea.  Reprogramming any chip is generally a 
bad idea (as we can see from the recent removal of all the init code) 
and forcing any specified config to apply to all chips found in the 
system would be an even worse idea.

I think a better idea that addresses your concerns about bus ordering 
would be to add an additional parameter that is a bus/chip number pair 
which is the chip to initialize.  Something like:

static int gpio_target[2] = { -1, -1 }
MODULE_PARM(gpio_target, "2i");
MODULE_PARM_DESC(gpio_target,"Address of chip to whose GPIO is to be 
programmed");

This would be similar to the bus/address pairs used in the 
force_subclient parameters to the w83781d driver.

>BTW, does anyone really use the GPIO pins reconfiguration parameters?
>  
>

Not anymore, but I did as I mention above.

:v)

Re: [RFC] I2C: Remove the i2c_client id field

[Jean Delvare]

Hi Philip,

> > Although this is a real use of the id, it only
> > matters if you use the module parameters for GPIO pins
> > reconfiguration and actually have more than one ADM1026 chip (a
> > quite rare chip if you remember).
>
> Not for me.  We ship hundreds of systems each month with a
> motherboard with that chip on it.  I think it's actually on two
> different motherboards we sell.

The two motherboards with ADM1026 I know of are (actually thanks to the
MBM site):
Iwill DK8S2
Accelertech HDAMA

I think I remember you ship HDAMA boards, right? Well, there must be a
reason why you (Penguin Computing) wrote the driver in the first place
and recently ported it to 2.6.

What I want to insist on is that, contrary to other chips which are used
on a wide range of different boards, the ADM1026 isn't so we can
concentrate of the specific use it has on these boards instead of
writing a full-featured driver.

> Wouldn't a force_xxx parameter cause a specific bus/id to be probed
> and  assigned first?

Ah, good question. Checking...

Hm, no it doesn't. The module parameters change the decision taken by
i2c_detect on each adapter/address combination, but doesn't affect the
order in which the chips are detected.

> > and I am not sure why one would want to reprogram only the
> > first chip. Unless someone comes with such a specific hardware setup
> > so that we can examine what is really needed,
> >

> Well, that's exactly the problem that I had.  The motherboard vendor's
> BIOS didn't set the chip up and I had to program it myself.  I got the
> schematics from the vendor for the part of the motherboard attached to
> the chip so that I could program it correctly.

Side note, it would probably be better to insist that the vendor should
fix the BIOS instead. Especially if you ship hundreds of these chips
each months, I guess you have some weight when asking them something.

Does you board really have 2 ADM1026 chips and only one needed
reprogramming?

> > I think we can get rid of the
> >"id == 0" test and reconfigure "all" ADM1026 chips (which really is
> > only one for the two known boards using an ADM1026).
>
> I think that would be a bad idea.  Reprogramming any chip is generally
> a  bad idea (as we can see from the recent removal of all the init
> code) and forcing any specified config to apply to all chips found in
> the system would be an even worse idea.

Agreed, but...

> I think a better idea that addresses your concerns about bus ordering 
> would be to add an additional parameter that is a bus/chip number
> pair which is the chip to initialize.  Something like:
> 
> static int gpio_target[2] = { -1, -1 }
> MODULE_PARM(gpio_target, "2i");
> MODULE_PARM_DESC(gpio_target,"Address of chip to whose GPIO is to be 
> programmed");
> 
> This would be similar to the bus/address pairs used in the 
> force_subclient parameters to the w83781d driver.

True. But again, do you have boards with *2* ADM1026 chips? Until
someone points out one boards which does, I see no benefit in
implementing this module parameter.

One question BTW, why don't you simply reprogram the chip(s) with i2cset
before loading the driver? That would save some kernel memory, and
solves the problem altogether.

Thanks,
-- 
Jean Delvare
http://khali.linux-fr.org/

Re: [RFC] I2C: Remove the i2c_client id field

[Greg KH]

On Mon, Dec 27, 2004 at 11:04:02PM +0100, Jean Delvare wrote:
> Hi Greg, hi all,
> 
> While porting various hardware monitoring drivers to Linux 2.6 and
> otherwise working on i2c drivers in 2.6, I found that the i2c_client
> structure has an "id" field (of type int) which is mostly unused. I am
> not exactly sure why it was introduced in the first place, and since the
> i2c subsystem code was significantly reworked since, it might not
> actually matter.

It's fine with me if it's dropped.

thanks,

greg k-h