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

login
register
mail settings
Submitter Wolfram Sang
Date Nov. 15, 2010, 5:25 p.m.
Message ID <1289841916-3825-1-git-send-email-w.sang@pengutronix.de>
Download mbox | patch
Permalink /patch/71255/
State Not Applicable
Headers show

Comments

Wolfram Sang - Nov. 15, 2010, 5:25 p.m.
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
Grant Likely - Nov. 15, 2010, 9:41 p.m.
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
>
>

Patch

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)