[PATCH 1/2] misc: at24: parse OF-data, too

[PATCH 1/2] misc: at24: parse OF-data, too

[Wolfram Sang]

Information about the pagesize and read-only-status may also come from
the devicetree. Parse this data, too, and act accordingly. While we are
here, change the initialization printout a bit. write_max is useful to
know to detect performance bottlenecks, the rest is superfluous.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---

Grant: As mentioned at ELCE10, I could pretty much respin this old approach I
tried roughly a year ago (just with archdata then). If the approach and docs
are good, I am fine with the patches entering via one of your trees.

 Documentation/powerpc/dts-bindings/eeprom.txt |   28 ++++++++++++++++++++++
 drivers/misc/eeprom/at24.c                    |   33 ++++++++++++++++++++-----
 2 files changed, 53 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/powerpc/dts-bindings/eeprom.txt

diff --git a/Documentation/powerpc/dts-bindings/eeprom.txt b/Documentation/powerpc/dts-bindings/eeprom.txt
new file mode 100644
index 0000000..4342c10
--- /dev/null
+++ b/Documentation/powerpc/dts-bindings/eeprom.txt
@@ -0,0 +1,28 @@
+EEPROMs (I2C)
+
+Required properties:
+
+  - compatible : should be "<manufacturer>,<type>"
+		 If there is no specific driver for <manufacturer>, a generic
+		 driver based on <type> is selected. Possible types are:
+		 24c00, 24c01, 24c02, 24c04, 24c08, 24c16, 24c32, 24c64,
+		 24c128, 24c256, 24c512, 24c1024, spd
+
+  - reg : the I2C address of the EEPROM
+
+Optional properties:
+
+  - pagesize : the length of the pagesize for writing. Please consult the
+               manual of your device, that value varies a lot. A wrong value
+	       may result in data loss! If not specified, a safety value of
+	       '1' is used which will be very slow.
+
+  - read-only: this parameterless property disables writes to the eeprom
+
+Example:
+
+eeprom@52 {
+	compatible = "atmel,24c32";
+	reg = <0x52>;
+	pagesize = <32>;
+};
diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 559b0b3..aaf16cb 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -20,6 +20,7 @@
 #include <linux/log2.h>
 #include <linux/bitops.h>
 #include <linux/jiffies.h>
+#include <linux/of.h>
 #include <linux/i2c.h>
 #include <linux/i2c/at24.h>
 
@@ -457,6 +458,27 @@ static ssize_t at24_macc_write(struct memory_accessor *macc, const char *buf,
 
 /*-------------------------------------------------------------------------*/
 
+#ifdef CONFIG_OF
+static void at24_get_ofdata(struct i2c_client *client,
+		struct at24_platform_data *chip)
+{
+	const u32 *val;
+	struct device_node *node = client->dev.of_node;
+
+	if (node) {
+		if (of_get_property(node, "read-only", NULL))
+			chip->flags |= AT24_FLAG_READONLY;
+		val = of_get_property(node, "pagesize", NULL);
+		if (val)
+			chip->page_size = *val;
+	}
+}
+#else
+static void at24_get_ofdata(struct i2c_client *client,
+		struct at24_platform_data *chip)
+{ }
+#endif /* CONFIG_OF */
+
 static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
 	struct at24_platform_data chip;
@@ -485,6 +507,9 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		 */
 		chip.page_size = 1;
 
+		/* update chipdata if OF is present */
+		at24_get_ofdata(client, &chip);
+
 		chip.setup = NULL;
 		chip.context = NULL;
 	}
@@ -597,19 +622,15 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 
 	i2c_set_clientdata(client, at24);
 
-	dev_info(&client->dev, "%zu byte %s EEPROM %s\n",
+	dev_info(&client->dev, "%zu byte %s EEPROM, %s, %u bytes/write\n",
 		at24->bin.size, client->name,
-		writable ? "(writable)" : "(read-only)");
+		writable ? "writable" : "read-only", at24->write_max);
 	if (use_smbus == I2C_SMBUS_WORD_DATA ||
 	    use_smbus == I2C_SMBUS_BYTE_DATA) {
 		dev_notice(&client->dev, "Falling back to %s reads, "
 			   "performance will suffer\n", use_smbus ==
 			   I2C_SMBUS_WORD_DATA ? "word" : "byte");
 	}
-	dev_dbg(&client->dev,
-		"page_size %d, num_addresses %d, write_max %d, use_smbus %d\n",
-		chip.page_size, num_addresses,
-		at24->write_max, use_smbus);
 
 	/* export data to kernel code */
 	if (chip.setup)
-- 
1.7.2.3

[PATCH 2/2] powerpc: pcm030/032: add pagesize to dts

[Wolfram Sang]

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---
 arch/powerpc/boot/dts/pcm030.dts |    1 +
 arch/powerpc/boot/dts/pcm032.dts |    3 ++-
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/boot/dts/pcm030.dts b/arch/powerpc/boot/dts/pcm030.dts
index 8a4ec30..e7c36bc 100644
--- a/arch/powerpc/boot/dts/pcm030.dts
+++ b/arch/powerpc/boot/dts/pcm030.dts
@@ -259,6 +259,7 @@
 			eeprom@52 {
 				compatible = "catalyst,24c32";
 				reg = <0x52>;
+				pagesize = <32>;
 			};
 		};
 
diff --git a/arch/powerpc/boot/dts/pcm032.dts b/arch/powerpc/boot/dts/pcm032.dts
index 85d857a..e175e2c 100644
--- a/arch/powerpc/boot/dts/pcm032.dts
+++ b/arch/powerpc/boot/dts/pcm032.dts
@@ -257,8 +257,9 @@
 				reg = <0x51>;
 			};
 			eeprom@52 {
-				compatible = "at24,24c32";
+				compatible = "catalyst,24c32";
 				reg = <0x52>;
+				pagesize = <32>;
 			};
 		};
 
-- 
1.7.2.3

Re: [PATCH 2/2] powerpc: pcm030/032: add pagesize to dts

[Anton Vorontsov]

On Mon, Nov 15, 2010 at 06:25:16PM +0100, Wolfram Sang wrote:
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> ---
>  arch/powerpc/boot/dts/pcm030.dts |    1 +
>  arch/powerpc/boot/dts/pcm032.dts |    3 ++-
>  2 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/powerpc/boot/dts/pcm030.dts b/arch/powerpc/boot/dts/pcm030.dts
> index 8a4ec30..e7c36bc 100644
> --- a/arch/powerpc/boot/dts/pcm030.dts
> +++ b/arch/powerpc/boot/dts/pcm030.dts
> @@ -259,6 +259,7 @@
>  			eeprom@52 {
>  				compatible = "catalyst,24c32";
>  				reg = <0x52>;
> +				pagesize = <32>;

I think you'd better drop the pagesize property altogether, and
instead make the compatible string more specific (if needed at
all. are there any 'catalyst,24c32' chips with pagesize != 32?)

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH 2/2] powerpc: pcm030/032: add pagesize to dts

[Mitch Bradley]

On 11/15/2010 7:32 AM, Anton Vorontsov wrote:
> On Mon, Nov 15, 2010 at 06:25:16PM +0100, Wolfram Sang wrote:
>> Signed-off-by: Wolfram Sang<w.sang@pengutronix.de>
>> ---
>>   arch/powerpc/boot/dts/pcm030.dts |    1 +
>>   arch/powerpc/boot/dts/pcm032.dts |    3 ++-
>>   2 files changed, 3 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/powerpc/boot/dts/pcm030.dts b/arch/powerpc/boot/dts/pcm030.dts
>> index 8a4ec30..e7c36bc 100644
>> --- a/arch/powerpc/boot/dts/pcm030.dts
>> +++ b/arch/powerpc/boot/dts/pcm030.dts
>> @@ -259,6 +259,7 @@
>>   			eeprom@52 {
>>   				compatible = "catalyst,24c32";
>>   				reg =<0x52>;
>> +				pagesize =<32>;
>
> I think you'd better drop the pagesize property altogether, and
> instead make the compatible string more specific (if needed at
> all. are there any 'catalyst,24c32' chips with pagesize != 32?)


Microchip makes a 24c32 part that looks pretty similar to the catalyst 
part, but Microchip's has a 64-byte page size compared to Catalyst's 32.

It would probably be feasible to have a generic I2C EEPROM driver that 
could handle many different parts, parameterized by total size, block 
size, and page size.


>
> Thanks,
>

Re: [PATCH 2/2] powerpc: pcm030/032: add pagesize to dts

[Anton Vorontsov]

On Mon, Nov 15, 2010 at 11:06:44AM -1000, Mitch Bradley wrote:
[...]
> >>  			eeprom@52 {
> >>  				compatible = "catalyst,24c32";
> >>  				reg =<0x52>;
> >>+				pagesize =<32>;
> >
> >I think you'd better drop the pagesize property altogether, and
> >instead make the compatible string more specific (if needed at
> >all. are there any 'catalyst,24c32' chips with pagesize != 32?)
> 
> Microchip makes a 24c32 part that looks pretty similar to the
> catalyst part, but Microchip's has a 64-byte page size compared to
> Catalyst's 32.

Well, when using microchip part, the compatible string would be
"microchip,24c32", correct? Then we have all the information
already, no need for the pagesize.

> It would probably be feasible to have a generic I2C EEPROM driver
> that could handle many different parts, parameterized by total size,
> block size, and page size.

I guess it can do this already via I2C ID table. The problem
is that I2C core is only using part ID (no vendor ID matching).

So, the current driver may just implement quirks like this:

if (of_device_is_compatible(np, "catalyst,24c32"))
	pagesize = 32;

Or, if it's some generic pattern, something like

if (of_device_is_compatible_vendor(np, "catalyst"))
	pagesize /= 2;

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH 1/2] misc: at24: parse OF-data, too

[Grant Likely]

On Mon, Nov 15, 2010 at 10:25 AM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> Information about the pagesize and read-only-status may also come from
> the devicetree. Parse this data, too, and act accordingly. While we are
> here, change the initialization printout a bit. write_max is useful to
> know to detect performance bottlenecks, the rest is superfluous.
>
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> ---
>
> Grant: As mentioned at ELCE10, I could pretty much respin this old approach I
> tried roughly a year ago (just with archdata then). If the approach and docs
> are good, I am fine with the patches entering via one of your trees.
>
>  Documentation/powerpc/dts-bindings/eeprom.txt |   28 ++++++++++++++++++++++
>  drivers/misc/eeprom/at24.c                    |   33 ++++++++++++++++++++-----
>  2 files changed, 53 insertions(+), 6 deletions(-)
>  create mode 100644 Documentation/powerpc/dts-bindings/eeprom.txt
>
> diff --git a/Documentation/powerpc/dts-bindings/eeprom.txt b/Documentation/powerpc/dts-bindings/eeprom.txt
> new file mode 100644
> index 0000000..4342c10
> --- /dev/null
> +++ b/Documentation/powerpc/dts-bindings/eeprom.txt
> @@ -0,0 +1,28 @@
> +EEPROMs (I2C)
> +
> +Required properties:
> +
> +  - compatible : should be "<manufacturer>,<type>"
> +                If there is no specific driver for <manufacturer>, a generic
> +                driver based on <type> is selected. Possible types are:
> +                24c00, 24c01, 24c02, 24c04, 24c08, 24c16, 24c32, 24c64,
> +                24c128, 24c256, 24c512, 24c1024, spd
> +
> +  - reg : the I2C address of the EEPROM
> +
> +Optional properties:
> +
> +  - pagesize : the length of the pagesize for writing. Please consult the
> +               manual of your device, that value varies a lot. A wrong value
> +              may result in data loss! If not specified, a safety value of
> +              '1' is used which will be very slow.

As Anton mentions, the compatible value should be specific enough that
a pagesize property isn't needed (assuming there aren't at24 devices
with differing page sizes), but that leaves the question of where to
put the page size data in the driver.  I think the obvious answer is
to put it in the at24_ids table.  This would also be a good thing for
the non-OF use-cases too.  Unfortunately the driver_data field is
already in use and requires refactoring to turn driver_data into a
pointer to a structure, and so requires more work (sorry).

A refactored at24_ids table might look something like (there may be a
cleaner way to go about it though, I used the macro to solve the
problem of constructing an at24_platform_data structure for each entry
and storing a pointer to it in the unsigned long driver_data field):

#define AT24_DEVDATA(_len, _page_size, _flags)   \
	((unsigned long)((struct at24_platform_data[]) \
		{{.byte_len = len, .page_size = _page_size, .flags =
		_flags}}))

static const struct i2c_device_id at24_ids[] = {
	/* needs 8 addresses as A0-A2 are ignored */
	{ "24c00", AT24_DEVDATA(128 / 8, 16, AT24_FLAG_TAKE8ADDR) },
	/* old variants can't be handled with this generic entry! */
	{ "24c01", AT24_DEVDATA(1024 / 8, 16, 0) },
	{ "24c02", AT24_DEVDATA(2048 / 8, 16, 0) },
	...
};

This will also make the probe code simpler.

However, if I am wrong and similarly named at24 devices have different
page sizes, then encoding it in a property is the right thing to do.

Also, the read-only property is fine.

g.


> +
> +  - read-only: this parameterless property disables writes to the eeprom
> +
> +Example:
> +
> +eeprom@52 {
> +       compatible = "atmel,24c32";
> +       reg = <0x52>;
> +       pagesize = <32>;
> +};
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index 559b0b3..aaf16cb 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -20,6 +20,7 @@
>  #include <linux/log2.h>
>  #include <linux/bitops.h>
>  #include <linux/jiffies.h>
> +#include <linux/of.h>
>  #include <linux/i2c.h>
>  #include <linux/i2c/at24.h>
>
> @@ -457,6 +458,27 @@ static ssize_t at24_macc_write(struct memory_accessor *macc, const char *buf,
>
>  /*-------------------------------------------------------------------------*/
>
> +#ifdef CONFIG_OF
> +static void at24_get_ofdata(struct i2c_client *client,
> +               struct at24_platform_data *chip)
> +{
> +       const u32 *val;
> +       struct device_node *node = client->dev.of_node;
> +
> +       if (node) {
> +               if (of_get_property(node, "read-only", NULL))
> +                       chip->flags |= AT24_FLAG_READONLY;
> +               val = of_get_property(node, "pagesize", NULL);
> +               if (val)
> +                       chip->page_size = *val;
> +       }
> +}
> +#else
> +static void at24_get_ofdata(struct i2c_client *client,
> +               struct at24_platform_data *chip)
> +{ }
> +#endif /* CONFIG_OF */
> +
>  static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  {
>        struct at24_platform_data chip;
> @@ -485,6 +507,9 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
>                 */
>                chip.page_size = 1;
>
> +               /* update chipdata if OF is present */
> +               at24_get_ofdata(client, &chip);
> +
>                chip.setup = NULL;
>                chip.context = NULL;
>        }
> @@ -597,19 +622,15 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
>
>        i2c_set_clientdata(client, at24);
>
> -       dev_info(&client->dev, "%zu byte %s EEPROM %s\n",
> +       dev_info(&client->dev, "%zu byte %s EEPROM, %s, %u bytes/write\n",
>                at24->bin.size, client->name,
> -               writable ? "(writable)" : "(read-only)");
> +               writable ? "writable" : "read-only", at24->write_max);
>        if (use_smbus == I2C_SMBUS_WORD_DATA ||
>            use_smbus == I2C_SMBUS_BYTE_DATA) {
>                dev_notice(&client->dev, "Falling back to %s reads, "
>                           "performance will suffer\n", use_smbus ==
>                           I2C_SMBUS_WORD_DATA ? "word" : "byte");
>        }
> -       dev_dbg(&client->dev,
> -               "page_size %d, num_addresses %d, write_max %d, use_smbus %d\n",
> -               chip.page_size, num_addresses,
> -               at24->write_max, use_smbus);
>
>        /* export data to kernel code */
>        if (chip.setup)
> --
> 1.7.2.3
>
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 1/2] misc: at24: parse OF-data, too

[Grant Likely]

On Mon, Nov 15, 2010 at 10:25 AM, Wolfram Sang <w.sang@pengutronix.de> wrot=
e:
> Information about the pagesize and read-only-status may also come from
> the devicetree. Parse this data, too, and act accordingly. While we are
> here, change the initialization printout a bit. write_max is useful to
> know to detect performance bottlenecks, the rest is superfluous.
>
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> ---
>
> Grant: As mentioned at ELCE10, I could pretty much respin this old approa=
ch I
> tried roughly a year ago (just with archdata then). If the approach and d=
ocs
> are good, I am fine with the patches entering via one of your trees.
>
> =A0Documentation/powerpc/dts-bindings/eeprom.txt | =A0 28 +++++++++++++++=
+++++++
> =A0drivers/misc/eeprom/at24.c =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0| =
=A0 33 ++++++++++++++++++++-----
> =A02 files changed, 53 insertions(+), 6 deletions(-)
> =A0create mode 100644 Documentation/powerpc/dts-bindings/eeprom.txt
>
> diff --git a/Documentation/powerpc/dts-bindings/eeprom.txt b/Documentatio=
n/powerpc/dts-bindings/eeprom.txt
> new file mode 100644
> index 0000000..4342c10
> --- /dev/null
> +++ b/Documentation/powerpc/dts-bindings/eeprom.txt
> @@ -0,0 +1,28 @@
> +EEPROMs (I2C)
> +
> +Required properties:
> +
> + =A0- compatible : should be "<manufacturer>,<type>"
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0If there is no specific driver for <manu=
facturer>, a generic
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0driver based on <type> is selected. Poss=
ible types are:
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A024c00, 24c01, 24c02, 24c04, 24c08, 24c16=
, 24c32, 24c64,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A024c128, 24c256, 24c512, 24c1024, spd
> +
> + =A0- reg : the I2C address of the EEPROM
> +
> +Optional properties:
> +
> + =A0- pagesize : the length of the pagesize for writing. Please consult =
the
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 manual of your device, that value varies a =
lot. A wrong value
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0may result in data loss! If not specified, a=
 safety value of
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0'1' is used which will be very slow.

As Anton mentions, the compatible value should be specific enough that
a pagesize property isn't needed (assuming there aren't at24 devices
with differing page sizes), but that leaves the question of where to
put the page size data in the driver.  I think the obvious answer is
to put it in the at24_ids table.  This would also be a good thing for
the non-OF use-cases too.  Unfortunately the driver_data field is
already in use and requires refactoring to turn driver_data into a
pointer to a structure, and so requires more work (sorry).

A refactored at24_ids table might look something like (there may be a
cleaner way to go about it though, I used the macro to solve the
problem of constructing an at24_platform_data structure for each entry
and storing a pointer to it in the unsigned long driver_data field):

#define AT24_DEVDATA(_len, _page_size, _flags)   \
	((unsigned long)((struct at24_platform_data[]) \
		{{.byte_len =3D len, .page_size =3D _page_size, .flags =3D
		_flags}}))

static const struct i2c_device_id at24_ids[] =3D {
	/* needs 8 addresses as A0-A2 are ignored */
	{ "24c00", AT24_DEVDATA(128 / 8, 16, AT24_FLAG_TAKE8ADDR) },
	/* old variants can't be handled with this generic entry! */
	{ "24c01", AT24_DEVDATA(1024 / 8, 16, 0) },
	{ "24c02", AT24_DEVDATA(2048 / 8, 16, 0) },
	...
};

This will also make the probe code simpler.

However, if I am wrong and similarly named at24 devices have different
page sizes, then encoding it in a property is the right thing to do.

Also, the read-only property is fine.

g.


> +
> + =A0- read-only: this parameterless property disables writes to the eepr=
om
> +
> +Example:
> +
> +eeprom@52 {
> + =A0 =A0 =A0 compatible =3D "atmel,24c32";
> + =A0 =A0 =A0 reg =3D <0x52>;
> + =A0 =A0 =A0 pagesize =3D <32>;
> +};
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index 559b0b3..aaf16cb 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -20,6 +20,7 @@
> =A0#include <linux/log2.h>
> =A0#include <linux/bitops.h>
> =A0#include <linux/jiffies.h>
> +#include <linux/of.h>
> =A0#include <linux/i2c.h>
> =A0#include <linux/i2c/at24.h>
>
> @@ -457,6 +458,27 @@ static ssize_t at24_macc_write(struct memory_accesso=
r *macc, const char *buf,
>
> =A0/*--------------------------------------------------------------------=
-----*/
>
> +#ifdef CONFIG_OF
> +static void at24_get_ofdata(struct i2c_client *client,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 struct at24_platform_data *chip)
> +{
> + =A0 =A0 =A0 const u32 *val;
> + =A0 =A0 =A0 struct device_node *node =3D client->dev.of_node;
> +
> + =A0 =A0 =A0 if (node) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (of_get_property(node, "read-only", NULL=
))
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 chip->flags |=3D AT24_FLAG_=
READONLY;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 val =3D of_get_property(node, "pagesize", N=
ULL);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (val)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 chip->page_size =3D *val;
> + =A0 =A0 =A0 }
> +}
> +#else
> +static void at24_get_ofdata(struct i2c_client *client,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 struct at24_platform_data *chip)
> +{ }
> +#endif /* CONFIG_OF */
> +
> =A0static int at24_probe(struct i2c_client *client, const struct i2c_devi=
ce_id *id)
> =A0{
> =A0 =A0 =A0 =A0struct at24_platform_data chip;
> @@ -485,6 +507,9 @@ static int at24_probe(struct i2c_client *client, cons=
t struct i2c_device_id *id)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 */
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0chip.page_size =3D 1;
>
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* update chipdata if OF is present */
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 at24_get_ofdata(client, &chip);
> +
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0chip.setup =3D NULL;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0chip.context =3D NULL;
> =A0 =A0 =A0 =A0}
> @@ -597,19 +622,15 @@ static int at24_probe(struct i2c_client *client, co=
nst struct i2c_device_id *id)
>
> =A0 =A0 =A0 =A0i2c_set_clientdata(client, at24);
>
> - =A0 =A0 =A0 dev_info(&client->dev, "%zu byte %s EEPROM %s\n",
> + =A0 =A0 =A0 dev_info(&client->dev, "%zu byte %s EEPROM, %s, %u bytes/wr=
ite\n",
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0at24->bin.size, client->name,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 writable ? "(writable)" : "(read-only)");
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 writable ? "writable" : "read-only", at24->=
write_max);
> =A0 =A0 =A0 =A0if (use_smbus =3D=3D I2C_SMBUS_WORD_DATA ||
> =A0 =A0 =A0 =A0 =A0 =A0use_smbus =3D=3D I2C_SMBUS_BYTE_DATA) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0dev_notice(&client->dev, "Falling back to =
%s reads, "
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 "performance will suf=
fer\n", use_smbus =3D=3D
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 I2C_SMBUS_WORD_DATA ?=
 "word" : "byte");
> =A0 =A0 =A0 =A0}
> - =A0 =A0 =A0 dev_dbg(&client->dev,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 "page_size %d, num_addresses %d, write_max =
%d, use_smbus %d\n",
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 chip.page_size, num_addresses,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 at24->write_max, use_smbus);
>
> =A0 =A0 =A0 =A0/* export data to kernel code */
> =A0 =A0 =A0 =A0if (chip.setup)
> --
> 1.7.2.3
>
>



--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 2/2] powerpc: pcm030/032: add pagesize to dts

[Grant Likely]

On Mon, Nov 15, 2010 at 2:24 PM, Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Mon, Nov 15, 2010 at 11:06:44AM -1000, Mitch Bradley wrote:
> [...]
>> >>                    eeprom@52 {
>> >>                            compatible = "catalyst,24c32";
>> >>                            reg =<0x52>;
>> >>+                           pagesize =<32>;
>> >
>> >I think you'd better drop the pagesize property altogether, and
>> >instead make the compatible string more specific (if needed at
>> >all. are there any 'catalyst,24c32' chips with pagesize != 32?)
>>
>> Microchip makes a 24c32 part that looks pretty similar to the
>> catalyst part, but Microchip's has a 64-byte page size compared to
>> Catalyst's 32.
>
> Well, when using microchip part, the compatible string would be
> "microchip,24c32", correct? Then we have all the information
> already, no need for the pagesize.
>
>> It would probably be feasible to have a generic I2C EEPROM driver
>> that could handle many different parts, parameterized by total size,
>> block size, and page size.

The current at24.c driver is already parameterized; but part of the
parameter data is statically linked into the board support code.

>
> I guess it can do this already via I2C ID table. The problem
> is that I2C core is only using part ID (no vendor ID matching).

This could potentially be changed for at24 devices since the i2c
subsystem already matches by name.  It would mean adding the vendor
prefix to all instantiations of the device in the kernel, although it
would mess up the current heuristic used by of_modalias_node() (not a
show-stopper).

>
> So, the current driver may just implement quirks like this:
>
> if (of_device_is_compatible(np, "catalyst,24c32"))
>        pagesize = 32;
>
> Or, if it's some generic pattern, something like
>
> if (of_device_is_compatible_vendor(np, "catalyst"))
>        pagesize /= 2;

This would get ugly in a hurry.  It would be better to make it data
driven by searching for a better match in an of_device_id table so
that the workarounds don't grow over time and eventually achieve
sentience.

g.

Re: [PATCH 2/2] powerpc: pcm030/032: add pagesize to dts

[Grant Likely]

On Mon, Nov 15, 2010 at 2:24 PM, Anton Vorontsov <cbouatmailru@gmail.com> w=
rote:
> On Mon, Nov 15, 2010 at 11:06:44AM -1000, Mitch Bradley wrote:
> [...]
>> >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0eeprom@52 {
>> >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0compatible =3D=
 "catalyst,24c32";
>> >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0reg =3D<0x52>;
>> >>+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 pagesize =3D<32>=
;
>> >
>> >I think you'd better drop the pagesize property altogether, and
>> >instead make the compatible string more specific (if needed at
>> >all. are there any 'catalyst,24c32' chips with pagesize !=3D 32?)
>>
>> Microchip makes a 24c32 part that looks pretty similar to the
>> catalyst part, but Microchip's has a 64-byte page size compared to
>> Catalyst's 32.
>
> Well, when using microchip part, the compatible string would be
> "microchip,24c32", correct? Then we have all the information
> already, no need for the pagesize.
>
>> It would probably be feasible to have a generic I2C EEPROM driver
>> that could handle many different parts, parameterized by total size,
>> block size, and page size.

The current at24.c driver is already parameterized; but part of the
parameter data is statically linked into the board support code.

>
> I guess it can do this already via I2C ID table. The problem
> is that I2C core is only using part ID (no vendor ID matching).

This could potentially be changed for at24 devices since the i2c
subsystem already matches by name.  It would mean adding the vendor
prefix to all instantiations of the device in the kernel, although it
would mess up the current heuristic used by of_modalias_node() (not a
show-stopper).

>
> So, the current driver may just implement quirks like this:
>
> if (of_device_is_compatible(np, "catalyst,24c32"))
> =A0 =A0 =A0 =A0pagesize =3D 32;
>
> Or, if it's some generic pattern, something like
>
> if (of_device_is_compatible_vendor(np, "catalyst"))
> =A0 =A0 =A0 =A0pagesize /=3D 2;

This would get ugly in a hurry.  It would be better to make it data
driven by searching for a better match in an of_device_id table so
that the workarounds don't grow over time and eventually achieve
sentience.

g.

Re: [PATCH 2/2] powerpc: pcm030/032: add pagesize to dts

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 1352 bytes --]

> > >I think you'd better drop the pagesize property altogether, and
> > >instead make the compatible string more specific (if needed at
> > >all. are there any 'catalyst,24c32' chips with pagesize != 32?)
> > 
> > Microchip makes a 24c32 part that looks pretty similar to the
> > catalyst part, but Microchip's has a 64-byte page size compared to
> > Catalyst's 32.
> 
> Well, when using microchip part, the compatible string would be
> "microchip,24c32", correct? Then we have all the information
> already, no need for the pagesize.

Hmm, there are myriads of I2C eeproms out there, this table would be enourmous.
Even worse, I seem to recall that I had once seen a manufacturer increasing the
page-size from one charge to the next without changing the part-number, so I
got this feeling "you can't map pagesize to manufacturer/type" which I still
have. Sadly, this was long ago, so I can't proof it right now. Will try to dig
up some datasheets when in the office tomorrow. In general, I2C EEPROMs are
really a mess, the basic access method is the same, but except that everything
else is possible :) Thus, this approach. Thus, this approach.

Regards,

   Wolfram

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

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

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 2/2] powerpc: pcm030/032: add pagesize to dts

[Wolfram Sang]

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

> > >I think you'd better drop the pagesize property altogether, and
> > >instead make the compatible string more specific (if needed at
> > >all. are there any 'catalyst,24c32' chips with pagesize != 32?)
> > 
> > Microchip makes a 24c32 part that looks pretty similar to the
> > catalyst part, but Microchip's has a 64-byte page size compared to
> > Catalyst's 32.
> 
> Well, when using microchip part, the compatible string would be
> "microchip,24c32", correct? Then we have all the information
> already, no need for the pagesize.

Hmm, there are myriads of I2C eeproms out there, this table would be enourmous.
Even worse, I seem to recall that I had once seen a manufacturer increasing the
page-size from one charge to the next without changing the part-number, so I
got this feeling "you can't map pagesize to manufacturer/type" which I still
have. Sadly, this was long ago, so I can't proof it right now. Will try to dig
up some datasheets when in the office tomorrow. In general, I2C EEPROMs are
really a mess, the basic access method is the same, but except that everything
else is possible :) Thus, this approach. Thus, this approach.

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 2/2] powerpc: pcm030/032: add pagesize to dts

[Mitch Bradley]

In general I think it's better to report parameter values directly, 
instead of inferring them from manufacturer and part numbers.  That way 
you at least have a fighting chance of avoiding a kernel upgrade when a 
part changes.

Of course, that only works when the device tree is exported from the 
boot firmware instead of having to carry the device tree inside the kernel.

Re: [PATCH 2/2] powerpc: pcm030/032: add pagesize to dts

[David Gibson]

On Mon, Nov 15, 2010 at 12:17:51PM -1000, Mitch Bradley wrote:
> In general I think it's better to report parameter values directly,
> instead of inferring them from manufacturer and part numbers.  That
> way you at least have a fighting chance of avoiding a kernel upgrade
> when a part changes.

I tend to agree.  Although a fallback to deducing from the
manufacturer / part id if the pagesize property is missing would be
sensible.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH 2/2] powerpc: pcm030/032: add pagesize to dts

[David Gibson]

On Mon, Nov 15, 2010 at 12:17:51PM -1000, Mitch Bradley wrote:
> In general I think it's better to report parameter values directly,
> instead of inferring them from manufacturer and part numbers.  That
> way you at least have a fighting chance of avoiding a kernel upgrade
> when a part changes.

I tend to agree.  Although a fallback to deducing from the
manufacturer / part id if the pagesize property is missing would be
sensible.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH 2/2] powerpc: pcm030/032: add pagesize to dts

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 613 bytes --]


> Even worse, I seem to recall that I had once seen a manufacturer increasing the
> page-size from one charge to the next without changing the part-number, so I
> got this feeling "you can't map pagesize to manufacturer/type" which I still
> have. Sadly, this was long ago, so I can't proof it right now. Will try to dig
> up some datasheets when in the office tomorrow.

Had a look, couldn't find anything :( And now?

Regards,

   Wolfram

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

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

[-- Attachment #2: Type: text/plain, Size: 150 bytes --]

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 2/2] powerpc: pcm030/032: add pagesize to dts

[Wolfram Sang]

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


> Even worse, I seem to recall that I had once seen a manufacturer increasing the
> page-size from one charge to the next without changing the part-number, so I
> got this feeling "you can't map pagesize to manufacturer/type" which I still
> have. Sadly, this was long ago, so I can't proof it right now. Will try to dig
> up some datasheets when in the office tomorrow.

Had a look, couldn't find anything :( And now?

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 2/2] powerpc: pcm030/032: add pagesize to dts

[Anton Vorontsov]

On Tue, Nov 16, 2010 at 10:45:37PM +0100, Wolfram Sang wrote:
> 
> > Even worse, I seem to recall that I had once seen a manufacturer increasing the
> > page-size from one charge to the next without changing the part-number, so I
> > got this feeling "you can't map pagesize to manufacturer/type" which I still
> > have. Sadly, this was long ago, so I can't proof it right now. Will try to dig
> > up some datasheets when in the office tomorrow.
> 
> Had a look, couldn't find anything :( And now?

Well, it seems that there are enough people in "pagesize" camp
anyway, I'd say just go ahead with the current approach. :-)

It's an additional information, so won't do any harm anyway...

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2