[PATCH 1/1] at24: add i2c_driver struct member for at24_driver, and reduce the priority of at24_init

[PATCH 1/1] at24: add i2c_driver struct member for at24_driver, and reduce the priority of at24_init

[jgq516-Re5JQEeQqe8AvxtiuMwx3w]

From: Xiao Jiang <jgq516-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

The following changes are ensure at24 driver can be probed successfully,
which are based on Linux 2.6.36-rc5.
1. Add class, detect and address_list member in at24_driver because they
   will be check in i2c_detect().
2. Since the at24 driver are tightly coupled with i2c bus driver, make
   sure it's init priority is lower than i2c bus.

Signed-off-by: Xiao Jiang <jgq516-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/misc/eeprom/at24.c |   16 +++++++++++++++-
 1 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 559b0b3..a40f74e 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -129,6 +129,9 @@ static const struct i2c_device_id at24_ids[] = {
 };
 MODULE_DEVICE_TABLE(i2c, at24_ids);
 
+/* Addresses to scan */
+static const unsigned short normal_i2c[] = { 0x50, 0x51, 0x52, 0x53, 0x54,
+					0x55, 0x56, 0x57, I2C_CLIENT_END };
 /*-------------------------------------------------------------------------*/
 
 /*
@@ -646,6 +649,13 @@ static int __devexit at24_remove(struct i2c_client *client)
 	return 0;
 }
 
+static int at24_detect(struct i2c_client *client, struct i2c_board_info *info)
+{
+	strlcpy(info->type, "at24", I2C_NAME_SIZE);
+
+	return 0;
+}
+
 /*-------------------------------------------------------------------------*/
 
 static struct i2c_driver at24_driver = {
@@ -656,6 +666,10 @@ static struct i2c_driver at24_driver = {
 	.probe = at24_probe,
 	.remove = __devexit_p(at24_remove),
 	.id_table = at24_ids,
+
+	.class = I2C_CLASS_SPD,
+	.detect = at24_detect,
+	.address_list = normal_i2c,
 };
 
 static int __init at24_init(void)
@@ -663,7 +677,7 @@ static int __init at24_init(void)
 	io_limit = rounddown_pow_of_two(io_limit);
 	return i2c_add_driver(&at24_driver);
 }
-module_init(at24_init);
+device_initcall_sync(at24_init);
 
 static void __exit at24_exit(void)
 {
-- 
1.6.3.3

Re: [PATCH 1/1] at24: add i2c_driver struct member for at24_driver, and reduce the priority of at24_init

[Wolfram Sang]

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

Hi,

On Tue, Sep 21, 2010 at 02:07:23PM +0800, jgq516-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> From: Xiao Jiang <jgq516-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> The following changes are ensure at24 driver can be probed successfully,
> which are based on Linux 2.6.36-rc5.
> 1. Add class, detect and address_list member in at24_driver because they
>    will be check in i2c_detect().

NAK. You would be attaching the driver to every chip in that address
list. Most EEPROMs are simply not probable, that's why it is not done in
the driver.

> 2. Since the at24 driver are tightly coupled with i2c bus driver, make
>    sure it's init priority is lower than i2c bus.

Can you describe a scenario where the current method fails?

Regards,

   Wolfram

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

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

Re: [PATCH 1/1] at24: add i2c_driver struct member for at24_driver, and reduce the priority of at24_init

[Jean Delvare]

Hi Xiao,

On Tue, 21 Sep 2010 14:07:23 +0800, jgq516-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> From: Xiao Jiang <jgq516-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> The following changes are ensure at24 driver can be probed successfully,
> which are based on Linux 2.6.36-rc5.
> 1. Add class, detect and address_list member in at24_driver because they
>    will be check in i2c_detect().
> 2. Since the at24 driver are tightly coupled with i2c bus driver, make
>    sure it's init priority is lower than i2c bus.
> 
> Signed-off-by: Xiao Jiang <jgq516-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/misc/eeprom/at24.c |   16 +++++++++++++++-
>  1 files changed, 15 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index 559b0b3..a40f74e 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -129,6 +129,9 @@ static const struct i2c_device_id at24_ids[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, at24_ids);
>  
> +/* Addresses to scan */
> +static const unsigned short normal_i2c[] = { 0x50, 0x51, 0x52, 0x53, 0x54,
> +					0x55, 0x56, 0x57, I2C_CLIENT_END };
>  /*-------------------------------------------------------------------------*/
>  
>  /*
> @@ -646,6 +649,13 @@ static int __devexit at24_remove(struct i2c_client *client)
>  	return 0;
>  }
>  
> +static int at24_detect(struct i2c_client *client, struct i2c_board_info *info)
> +{
> +	strlcpy(info->type, "at24", I2C_NAME_SIZE);
> +
> +	return 0;
> +}
> +
>  /*-------------------------------------------------------------------------*/
>  
>  static struct i2c_driver at24_driver = {
> @@ -656,6 +666,10 @@ static struct i2c_driver at24_driver = {
>  	.probe = at24_probe,
>  	.remove = __devexit_p(at24_remove),
>  	.id_table = at24_ids,
> +
> +	.class = I2C_CLASS_SPD,
> +	.detect = at24_detect,
> +	.address_list = normal_i2c,
>  };
>  
>  static int __init at24_init(void)
> @@ -663,7 +677,7 @@ static int __init at24_init(void)
>  	io_limit = rounddown_pow_of_two(io_limit);
>  	return i2c_add_driver(&at24_driver);
>  }
> -module_init(at24_init);
> +device_initcall_sync(at24_init);
>  
>  static void __exit at24_exit(void)
>  {

Nack. We don't want a driver to unconditionally attach to I2C
addresses. It will inevitably grab devices which it doesn't actually
support. There's also the problem that larger EEPROMs reply to more
than one address, and there's no way to differentiate between one large
and many smaller EEPROMs (for example one AT24C04 at 0x50 vs. two
AT24C02 at 0x50 and 0x51.)

Worse, the above gives _write_ access to the EEPROM from user-space.
This means users are free to make their expensive memory modules
unusable by accidentally writing to the SPD EEPROM.

The bottom line is that EEPROM devices must be declared properly, they
can't be autodetected.

The only case where autodetection makes sense is for read-only EEPROMs
with well-specified contents, so we can check the contents for
autodetection. SPD EEPROMs fall into this category, as well as the Sony
proprietary EEPROMs in Vaio laptops. EDID EEPROMs could qualify as
well, however I think I'd rather leave graphics adapter drivers deal
with them and not interfere.

Don't get me wrong, I'd be happy to get rid of the legacy "eeprom"
driver, and in order to do this, extending the "at24" driver to
autodetect SPD EEPROMs is needed. But it needs to be done with great
care and thoughts.

-- 
Jean Delvare

Re: [PATCH 1/1] at24: add i2c_driver struct member for at24_driver, and reduce the priority of at24_init

[Wolfram Sang]

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


>    Yes, we can't cover all the eeprom types, but I think people need
>    one example with all the function can be used. If somebody need use
>    at24.c for another eeprom, he only need to do the change
>    "s/at24/eeprom type", which maybe helpful to others. Anyway,
>    current code aren't work because of the lack of these members.

The instantiation of devices is described here:

Documentation/i2c/instantiating-devices

I'd think that would do to help people?

>      > 2. Since the at24 driver are tightly coupled with i2c bus
>      > driver, make � �sure it's init priority is lower than i2c bus.
> 
>      Can you describe a scenario where the current method fails?
> 
>    In my board, at24_init() is called before i2c-bus driver, because
>    the bus driver is initialized with module_init. But some i2c bus
>    used subsys_initcall to replace module_init, those i2c bus should
>    be ok without the change. I think only do one modification is
>    better than change all the i2c bus driver.

But they should still match, even if at24 comes first, no? Otherwise
there is a bug somewhere. (BTW changing i2c-bus-drivers to
subsys_initcall() looks better to me, too, because if there is something
like a PMIC or GPIOs connected to the bus, you usually want them early).

Regards,

   Wolfram

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

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

Re: [PATCH 1/1] at24: add i2c_driver struct member for at24_driver, and reduce the priority of at24_init

[Jean Delvare]

Hi Xiao,

Please always reply to the list, so that your reply gets archived, and
also so that others can participate in the discussion.

On Tue, 21 Sep 2010 15:29:29 +0800, xiaojiang wrote:
> 2010/9/21 Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> > On Tue, 21 Sep 2010 14:07:23 +0800, jgq516-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> > > From: Xiao Jiang <jgq516-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > >
> > > The following changes are ensure at24 driver can be probed successfully,
> > > which are based on Linux 2.6.36-rc5.
> > > 1. Add class, detect and address_list member in at24_driver because they
> > >    will be check in i2c_detect().
> > (...)
> > Nack. We don't want a driver to unconditionally attach to I2C
> > addresses. It will inevitably grab devices which it doesn't actually
> > support. There's also the problem that larger EEPROMs reply to more
> > than one address, and there's no way to differentiate between one large
> > and many smaller EEPROMs (for example one AT24C04 at 0x50 vs. two
> > AT24C02 at 0x50 and 0x51.)
>
> Sorry, I don't  consider this sceneo.
> 
> > Worse, the above gives _write_ access to the EEPROM from user-space.
> > This means users are free to make their expensive memory modules
> > unusable by accidentally writing to the SPD EEPROM.
>
> Why user-space can't do write operation to eeprom? if so, people can't
> update the eeprom's data, but update eeprom is necessay for my board.

Whether or not this is desirable, depends on what each EEPROM is being
used for on a given system. Of course we want write support in some
cases, but making it the default for autodetected devices seems way too
dangerous. If people really need write access to their EEPROMs, they
have to properly declare them and not rely on autodetection.

> > The bottom line is that EEPROM devices must be declared properly, they
> > can't be autodetected.
> >
> > The only case where autodetection makes sense is for read-only EEPROMs
> > with well-specified contents, so we can check the contents for
> > autodetection. SPD EEPROMs fall into this category, as well as the Sony
> > proprietary EEPROMs in Vaio laptops. EDID EEPROMs could qualify as
> > well, however I think I'd rather leave graphics adapter drivers deal
> > with them and not interfere.
> >
> > Don't get me wrong, I'd be happy to get rid of the legacy "eeprom"
> > driver, and in order to do this, extending the "at24" driver to
> > autodetect SPD EEPROMs is needed. But it needs to be done with great
> > care and thoughts.
>
>  I will consider more about it, thanks:)

-- 
Jean Delvare