[PATCH] misc/eeprom/at24: init dynamic bin_attribute structures

[PATCH] misc/eeprom/at24: init dynamic bin_attribute structures

[Wolfram Sang]

Commit 6992f5334995af474c2b58d010d08bc597f0f2fe introduced this requirement.

Reported-by: Albrecht Dreß <albrecht.dress-KvP5wT2u2U0@public.gmane.org>
Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Cc: Eric W. Biederman <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
Cc: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
---
 drivers/misc/eeprom/at24.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

Depends on:

http://thread.gmane.org/gmane.linux.drivers.i2c/5496

([PATCH V2] at24: Fall back to byte or word reads if needed)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 88c20da..d2deea4 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -541,6 +541,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	at24->bin.attr.mode = chip.flags & AT24_FLAG_IRUGO ? S_IRUGO : S_IRUSR;
 	at24->bin.read = at24_bin_read;
 	at24->bin.size = chip.byte_len;
+	sysfs_bin_attr_init(&at24->bin);
 
 	at24->macc.read = at24_macc_read;
 
-- 
1.7.0

Re: [PATCH] misc/eeprom/at24: init dynamic bin_attribute structures

[Jean Delvare]

Hi Wolfram,

On Thu, 11 Mar 2010 01:55:32 +0100, Wolfram Sang wrote:
> Commit 6992f5334995af474c2b58d010d08bc597f0f2fe introduced this requirement.
> 
> Reported-by: Albrecht Dreß <albrecht.dress-KvP5wT2u2U0@public.gmane.org>
> Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Cc: Eric W. Biederman <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
> Cc: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> ---
>  drivers/misc/eeprom/at24.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> Depends on:
> 
> http://thread.gmane.org/gmane.linux.drivers.i2c/5496
> 
> ([PATCH V2] at24: Fall back to byte or word reads if needed)
> 
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index 88c20da..d2deea4 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -541,6 +541,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  	at24->bin.attr.mode = chip.flags & AT24_FLAG_IRUGO ? S_IRUGO : S_IRUSR;
>  	at24->bin.read = at24_bin_read;
>  	at24->bin.size = chip.byte_len;
> +	sysfs_bin_attr_init(&at24->bin);

I think it would make more sense to move the initialization _before_
manually setting other struct members. You don't know what
sysfs_bin_attr_init does or will do in the future.

>  
>  	at24->macc.read = at24_macc_read;
>  

I presume the at25 driver needs the same fix?

-- 
Jean Delvare

Re: [PATCH] misc/eeprom/at24: init dynamic bin_attribute structures

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1046 bytes --]

Hi Jean,

> > diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> > index 88c20da..d2deea4 100644
> > --- a/drivers/misc/eeprom/at24.c
> > +++ b/drivers/misc/eeprom/at24.c
> > @@ -541,6 +541,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
> >  	at24->bin.attr.mode = chip.flags & AT24_FLAG_IRUGO ? S_IRUGO : S_IRUSR;
> >  	at24->bin.read = at24_bin_read;
> >  	at24->bin.size = chip.byte_len;
> > +	sysfs_bin_attr_init(&at24->bin);
> 
> I think it would make more sense to move the initialization _before_
> manually setting other struct members. You don't know what
> sysfs_bin_attr_init does or will do in the future.

Okay.

> I presume the at25 driver needs the same fix?

Yes, sent it to the SPI-list sperately. I wasn't sure if a patch series would
ease or complicate things.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] misc/eeprom/at24: init dynamic bin_attribute structures

[Jean Delvare]

On Thu, 11 Mar 2010 09:03:47 +0100, Wolfram Sang wrote:
> Hi Jean,
> 
> > > diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> > > index 88c20da..d2deea4 100644
> > > --- a/drivers/misc/eeprom/at24.c
> > > +++ b/drivers/misc/eeprom/at24.c
> > > @@ -541,6 +541,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
> > >  	at24->bin.attr.mode = chip.flags & AT24_FLAG_IRUGO ? S_IRUGO : S_IRUSR;
> > >  	at24->bin.read = at24_bin_read;
> > >  	at24->bin.size = chip.byte_len;
> > > +	sysfs_bin_attr_init(&at24->bin);
> > 
> > I think it would make more sense to move the initialization _before_
> > manually setting other struct members. You don't know what
> > sysfs_bin_attr_init does or will do in the future.
> 
> Okay.

OK, patch applied with this change, thanks.

> > I presume the at25 driver needs the same fix?
> 
> Yes, sent it to the SPI-list sperately. I wasn't sure if a patch series would
> ease or complicate things.

Fine with me, as long as it get fixed.

-- 
Jean Delvare

Re: [PATCH] misc/eeprom/at24: init dynamic bin_attribute structures

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 487 bytes --]


> > > I presume the at25 driver needs the same fix?
> > 
> > Yes, sent it to the SPI-list sperately. I wasn't sure if a patch series would
> > ease or complicate things.
> 
> Fine with me, as long as it get fixed.

I am currently using my newly acquired coccinelle-fu to find further drivers in
need of such a fix :)

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] misc/eeprom/at24: init dynamic bin_attribute structures

[Jean Delvare]

On Thu, 11 Mar 2010 12:23:21 +0100, Wolfram Sang wrote:
> 
> > > > I presume the at25 driver needs the same fix?
> > > 
> > > Yes, sent it to the SPI-list sperately. I wasn't sure if a patch series would
> > > ease or complicate things.
> > 
> > Fine with me, as long as it get fixed.
> 
> I am currently using my newly acquired coccinelle-fu to find further drivers in
> need of such a fix :)

A quick grep suggests that only 3 drivers need care:
arch/mips/txx9/generic/setup.c
drivers/misc/eeprom/at24.c
drivers/misc/eeprom/at25.c

I'm curious if coccinelle will find more...

-- 
Jean Delvare

Re: [PATCH] misc/eeprom/at24: init dynamic bin_attribute structures

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 547 bytes --]

> A quick grep suggests that only 3 drivers need care:
> arch/mips/txx9/generic/setup.c
> drivers/misc/eeprom/at24.c
> drivers/misc/eeprom/at25.c
> 
> I'm curious if coccinelle will find more...

See kernel-janitors:

drivers/base/firmware_class.c
drivers/rtc/rtc-ds1742.c

needed a fix, too. At least those, I am not 100% sure I found all of them.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] misc/eeprom/at24: init dynamic bin_attribute structures

[Albrecht Dreß]

[-- Attachment #1: Type: text/plain, Size: 1637 bytes --]

Hi Wolfram:

your patch completely fixes the issue on my 5200B board.

Just an other dumb question:  as I mentioned, the driver is loaded even if no chip is attached to the i2c bus.  Wouldn't it make sense to check if the chip is there (e.g. by reading the first byte), and eject with ENODEV if it isn't?

Thanks, Albrecht.

Tested-By: Albrecht Dreß <albrecht.dress-KvP5wT2u2U0@public.gmane.org>

Am 11.03.10 01:55 schrieb(en) Wolfram Sang:
> Commit 6992f5334995af474c2b58d010d08bc597f0f2fe introduced this requirement.
> 
> Reported-by: Albrecht Dreß <albrecht.dress-KvP5wT2u2U0@public.gmane.org>
> Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Cc: Eric W. Biederman <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
> Cc: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> ---
>  drivers/misc/eeprom/at24.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> Depends on:
> 
> http://thread.gmane.org/gmane.linux.drivers.i2c/5496
> 
> ([PATCH V2] at24: Fall back to byte or word reads if needed)
> 
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index 88c20da..d2deea4 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -541,6 +541,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  	at24->bin.attr.mode = chip.flags & AT24_FLAG_IRUGO ? S_IRUGO : S_IRUSR;
>  	at24->bin.read = at24_bin_read;
>  	at24->bin.size = chip.byte_len;
> +	sysfs_bin_attr_init(&at24->bin);
> 
>  	at24->macc.read = at24_macc_read;
> 
> --
> 1.7.0
> 
> 
> 
> 


[-- Attachment #2: Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH] misc/eeprom/at24: init dynamic bin_attribute structures

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 660 bytes --]

> Just an other dumb question:  as I mentioned, the driver is loaded even if no
> chip is attached to the i2c bus.  Wouldn't it make sense to check if the chip
> is there (e.g. by reading the first byte), and eject with ENODEV if it isn't?

That would be part of a detect-callback. In probe() the device is already
considered to be present. But you can instantiate devices at runtime if you
want to add the eeprom dynamically (Documentation/i2c/instantiating-devices)

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]