Skip to contentSkip to conversation

eeprom/at24: use device_property_*() functions instead of of_get_property()

PATCH Superseded Ben Gardner 4 participants 16 messages Feb 8, 2017 – Feb 10, 2017

Subsystems (2)
Char and misc drivers I2C subsystem host drivers
lore
Share & tools
lore

Ben Gardner

author
#0 lore raw

PATCH 1/1 eeprom/at24: use device_property_*() functions instead of of_get_property()

Allow the at24 driver to get configuration information from both OF and ACPI by using the more generic device_property functions. This change was inspired by the at25.c driver.

I have a custom board with a ST M24C02 EEPROM attached to an I2C bus. With the following ACPI construct, this patch instantiates a working instance of the driver.

Device (EEP0) {
Name (_HID, "PRP0001")
Name (_DSD, Package () {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), Package () {
Package () {"compatible", Package () {"st,24c02"}}, Package () {"size", 256},
Package () {"pagesize", 16},
},
})
Name (_CRS, ResourceTemplate () {
I2cSerialBus (
0x0057, ControllerInitiated, 400000, AddressingMode7Bit, "\\_SB.PCI0.I2C3", 0x00, ResourceConsumer,,)
})
}

Note: Matching the driver to the I2C device requires another patch.
http://www.spinics.net/lists/linux-acpi/msg71914.html

Signed-off-by Ben Gardner <[email protected]>
---
 drivers/misc/eeprom/at24.c | 28 +++++++++-------------------
 1 file changed, 9 insertions(+), 19 deletions(-)
drivers/misc/eeprom/at24.c +9 -19
index 051b147..9cb8904 100644
@@ -19,7 +19,6 @@
 #include <linux/log2.h>
 #include <linux/bitops.h>
 #include <linux/jiffies.h>
-#include <linux/of.h>
 #include <linux/acpi.h>
 #include <linux/i2c.h>
 #include <linux/nvmem-provider.h>

@@ -562,26 +561,17 @@ static int at24_write(void *priv, unsigned int off, void *val, size_t count)
 	return 0;
 }
 
-#ifdef CONFIG_OF
-static void at24_get_ofdata(struct i2c_client *client,
+static void at24_fw_to_chip(struct device *dev,
 			    struct at24_platform_data *chip)
 {
-	const __be32 *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 = be32_to_cpup(val);
-	}
+	u32 val;
+
+	if (device_property_present(dev, "read-only"))
+		chip->flags |= AT24_FLAG_READONLY;
+
+	if (device_property_read_u32(dev, "pagesize", &val) == 0)
+		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)
 {

@@ -621,7 +611,7 @@ 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);
+		at24_fw_to_chip(&client->dev, &chip);
 
 		chip.setup = NULL;
 		chip.context = NULL;
Signature · 1 line
2.7.4
Reviewed-by on v1 1/1
#1 lore raw
Quoted 26 lines · Ben Gardner

Allow the at24 driver to get configuration information from both OF and ACPI by using the more generic device_property functions. This change was inspired by the at25.c driver.

I have a custom board with a ST M24C02 EEPROM attached to an I2C bus. With the following ACPI construct, this patch instantiates a working instance of the driver.

Device (EEP0) {
Name (_HID, "PRP0001")
Name (_DSD, Package () {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),

  Package () {
   Package () {"compatible", Package () {"st,24c02"}},
   Package () {"size", 256},
   Package () {"pagesize", 16},

},
})
Name (_CRS, ResourceTemplate () {
I2cSerialBus (
0x0057, ControllerInitiated, 400000,
AddressingMode7Bit, "\\_SB.PCI0.I2C3", 0x00,
ResourceConsumer,,)
})
}

Quoted 2 lines

Note: Matching the driver to the I2C device requires another patch.
http://www.spinics.net/lists/linux-acpi/msg71914.html

Nice!

Few comments, after addressing them

FWIW:

Reviewed-by Andy Shevchenko <[email protected]>
Quoted 19 lines
Signed-off-by Ben Gardner <[email protected]>
---
 drivers/misc/eeprom/at24.c | 28 +++++++++-------------------
 1 file changed, 9 insertions(+), 19 deletions(-)
drivers/misc/eeprom/at24.c +0 -1
index 051b147..9cb8904 100644
@@ -19,7 +19,6 @@
 #include <linux/log2.h>
 #include <linux/bitops.h>
 #include <linux/jiffies.h>
-#include <linux/of.h>
 #include <linux/acpi.h>
 #include <linux/i2c.h>
 #include <linux/nvmem-provider.h>

drivers/misc/eeprom/at24.c static int at24_write(void *priv, unsigned int off, void *val, size_t count) · line 565

Quoted 7 lines
drivers/misc/eeprom/at24.c +1 -2
@@ -562,26 +561,17 @@ static int at24_write(void *priv, unsigned int off, void *val, size_t count)
        return 0;
 }
 
-#ifdef CONFIG_OF
-static void at24_get_ofdata(struct i2c_client *client,
+static void at24_fw_to_chip(struct device *dev,
                            struct at24_platform_data *chip)

"fw" here is ambiguous a bit.
Would at24_get_pdata() work for you?

Quoted 8 lines
drivers/misc/eeprom/at24.c +0 -7
 {
-       const __be32 *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);

drivers/misc/eeprom/at24.c static int at24_write(void *priv, unsigned int off, void *val, size_t count)

Quoted 10 lines
drivers/misc/eeprom/at24.c +6 -3
-               if (val)
-                       chip->page_size = be32_to_cpup(val);
-       }
+       u32 val;
+
+       if (device_property_present(dev, "read-only"))
+               chip->flags |= AT24_FLAG_READONLY;
+
 
+       if (device_property_read_u32(dev, "pagesize", &val) == 0)

I would use default from probe here.

int ret;

...

ret = ..._u32(..., &val);
if (ret) {
/* ...long comment from ->probe()... */
chip->page_size = 1;
} else
chip->page_size = val;

Quoted 11 lines
drivers/misc/eeprom/at24.c +1 -5
+               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)

{

drivers/misc/eeprom/at24.c static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) · line 614

Quoted 4 lines
drivers/misc/eeprom/at24.c +0 -0
@@ -621,7 +611,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
                chip.page_size = 1;
 
 
                /* update chipdata if OF is present */

This is now redundant.

Quoted 8 lines
drivers/misc/eeprom/at24.c +1 -1
-               at24_get_ofdata(client, &chip);
+               at24_fw_to_chip(&client->dev, &chip);
 
                chip.setup = NULL;

                chip.context = NULL;

--
2.7.4

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author
#2 lore raw

Hi Andy,

Thanks for taking the time to look at this.

Quoted 6 lines · Andy Shevchenko
Quoted 4 lines · Ben Gardner

Allow the at24 driver to get configuration information from both OF and ACPI by using the more generic device_property functions. This change was inspired by the at25.c driver.

(snip)

Quoted 5 lines

Few comments, after addressing them

FWIW:

Reviewed-by Andy Shevchenko <[email protected]>

(snip)

Quoted 8 lines

drivers/misc/eeprom/at24.c static int at24_write(void *priv, unsigned int off, void *val, size_t count) · line 565

Quoted 5 lines
drivers/misc/eeprom/at24.c +1 -2
-#ifdef CONFIG_OF
-static void at24_get_ofdata(struct i2c_client *client,
+static void at24_fw_to_chip(struct device *dev,
                            struct at24_platform_data *chip)

"fw" here is ambiguous a bit.
Would at24_get_pdata() work for you?

That name also works for me.

Quoted 23 lines
Quoted 8 lines

{

drivers/misc/eeprom/at24.c +0 -7
-       const __be32 *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);

drivers/misc/eeprom/at24.c static int at24_write(void *priv, unsigned int off, void *val, size_t count)

Quoted 10 lines
drivers/misc/eeprom/at24.c +6 -3
-               if (val)
-                       chip->page_size = be32_to_cpup(val);
-       }
+       u32 val;
+
+       if (device_property_present(dev, "read-only"))
+               chip->flags |= AT24_FLAG_READONLY;
+
 
+       if (device_property_read_u32(dev, "pagesize", &val) == 0)

I would use default from probe here.

int ret;

Quoted 8 lines

...

ret = ..._u32(..., &val);
if (ret) {
/* ...long comment from ->probe()... */
chip->page_size = 1;
} else
chip->page_size = val;

That sounds reasonable.

Quoted 14 lines
Quoted 13 lines
drivers/misc/eeprom/at24.c +1 -5
+               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)
 {

@@ -621,7 +611,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
                chip.page_size = 1;
Quoted 3 lines

drivers/misc/eeprom/at24.c static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) · line 613

Quoted 1 line
drivers/misc/eeprom/at24.c +0 -0
                /* update chipdata if OF is present */

This is now redundant.

Yeah, that comment doesn't seem all that useful.

Quoted 11 lines
Quoted 8 lines
drivers/misc/eeprom/at24.c +1 -2
-               at24_get_ofdata(client, &chip);
+               at24_fw_to_chip(&client->dev, &chip);
 
                chip.setup = NULL;
                chip.context = NULL;
--

2.7.4

Quoted 3 lines

--
With Best Regards,
Andy Shevchenko

I'll send an update.

Thanks,
Ben

author
#3 lore raw

PATCH v2 1/1 eeprom/at24: use device_property_*() functions instead of of_get_property()

In reply to Ben Gardner

Allow the at24 driver to get configuration information from both OF and ACPI by using the more generic device_property functions. This change was inspired by the at25.c driver.

I have a custom board with a ST M24C02 EEPROM attached to an I2C bus. With the following ACPI construct, this patch instantiates a working instance of the driver.

Device (EEP0) {
Name (_HID, "PRP0001")
Name (_DSD, Package () {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), Package () {
Package () {"compatible", Package () {"st,24c02"}}, Package () {"size", 256},
Package () {"pagesize", 16},
},
})
Name (_CRS, ResourceTemplate () {
I2cSerialBus (
0x0057, ControllerInitiated, 400000, AddressingMode7Bit, "\\_SB.PCI0.I2C3", 0x00, ResourceConsumer,,)
})
}

Note: Matching the driver to the I2C device requires another patch.
http://www.spinics.net/lists/linux-acpi/msg71914.html

Signed-off-by Ben Gardner <[email protected]>
---
 drivers/misc/eeprom/at24.c | 43 +++++++++++++++++--------------------------
 1 file changed, 17 insertions(+), 26 deletions(-)
drivers/misc/eeprom/at24.c +17 -26
index 051b147..81099f3 100644
@@ -19,7 +19,6 @@
 #include <linux/log2.h>
 #include <linux/bitops.h>
 #include <linux/jiffies.h>
-#include <linux/of.h>
 #include <linux/acpi.h>
 #include <linux/i2c.h>
 #include <linux/nvmem-provider.h>

@@ -562,26 +561,25 @@ static int at24_write(void *priv, unsigned int off, void *val, size_t count)
 	return 0;
 }
 
-#ifdef CONFIG_OF
-static void at24_get_ofdata(struct i2c_client *client,
-			    struct at24_platform_data *chip)
+static void at24_get_pdata(struct device *dev,
+			   struct at24_platform_data *chip)
 {
-	const __be32 *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 = be32_to_cpup(val);
+	u32 val;
+
+	if (device_property_present(dev, "read-only"))
+		chip->flags |= AT24_FLAG_READONLY;
+
+	if (device_property_read_u32(dev, "pagesize", &val) == 0) {
+		chip->page_size = val;
+	} else {
+		/*
+		 * This is slow, but we can't know all eeproms, so we better
+		 * play safe. Specifying custom eeprom-types via platform_data
+		 * is recommended anyhow.
+		 */
+		chip.page_size = 1;
 	}
 }
-#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)
 {

@@ -613,15 +611,8 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		chip.byte_len = BIT(magic & AT24_BITMASK(AT24_SIZE_BYTELEN));
 		magic >>= AT24_SIZE_BYTELEN;
 		chip.flags = magic & AT24_BITMASK(AT24_SIZE_FLAGS);
-		/*
-		 * This is slow, but we can't know all eeproms, so we better
-		 * play safe. Specifying custom eeprom-types via platform_data
-		 * is recommended anyhow.
-		 */
-		chip.page_size = 1;
 
-		/* update chipdata if OF is present */
-		at24_get_ofdata(client, &chip);
+		at24_get_pdata(&client->dev, &chip);
 
 		chip.setup = NULL;
 		chip.context = NULL;
Signature · 1 line
2.7.4
#4 lore raw
Quoted 26 lines · Ben Gardner

Allow the at24 driver to get configuration information from both OF and ACPI by using the more generic device_property functions. This change was inspired by the at25.c driver.

I have a custom board with a ST M24C02 EEPROM attached to an I2C bus. With the following ACPI construct, this patch instantiates a working instance of the driver.

Device (EEP0) {
Name (_HID, "PRP0001")
Name (_DSD, Package () {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),

  Package () {
   Package () {"compatible", Package () {"st,24c02"}},
   Package () {"size", 256},
   Package () {"pagesize", 16},

},
})
Name (_CRS, ResourceTemplate () {
I2cSerialBus (
0x0057, ControllerInitiated, 400000,
AddressingMode7Bit, "\\_SB.PCI0.I2C3", 0x00,
ResourceConsumer,,)
})
}

Quoted 3 lines

Note: Matching the driver to the I2C device requires another patch.
http://www.spinics.net/lists/linux-acpi/msg71914.html

Still couple of comments.

drivers/misc/eeprom/at24.c line 23

Quoted 6 lines
drivers/misc/eeprom/at24.c +0 -1
@@ -19,7 +19,6 @@
 #include <linux/log2.h>
 #include <linux/bitops.h>
 #include <linux/jiffies.h>
 
-#include <linux/of.h>

I suppose
+ #include <linux/property.h>
?

drivers/misc/eeprom/at24.c static int at24_write(void *priv, unsigned int off, void *val, size_t count) · line 565

Quoted 2 lines
drivers/misc/eeprom/at24.c +2 -0
+static void at24_get_pdata(struct device *dev,
+                          struct at24_platform_data *chip)

Now it fits one line.

drivers/misc/eeprom/at24.c static int at24_write(void *priv, unsigned int off, void *val, size_t count) · line 572

Quoted 6 lines
drivers/misc/eeprom/at24.c +6 -0
+       u32 val;
+
+       if (device_property_present(dev, "read-only"))
+               chip->flags |= AT24_FLAG_READONLY;
+
+       if (device_property_read_u32(dev, "pagesize", &val) == 0) {

' == 0' looks awkward.

And I think

ret = x();
if (ret)
...
else
...

pattern would look slightly better.

Quoted 8 lines
drivers/misc/eeprom/at24.c +8 -0
+               chip->page_size = val;
+       } else {
+               /*
+                * This is slow, but we can't know all eeproms, so we better
+                * play safe. Specifying custom eeprom-types via platform_data
+                * is recommended anyhow.
+                */
+               chip.page_size = 1;
Signature · 2 lines
With Best Regards,
Andy Shevchenko
author
#5 lore raw
Quoted 7 lines

Still couple of comments.

Quoted 4 lines
drivers/misc/eeprom/at24.c +0 -0
@@ -19,7 +19,6 @@
 #include <linux/log2.h>
 #include <linux/bitops.h>
 #include <linux/jiffies.h>
Quoted 5 lines

drivers/misc/eeprom/at24.c line 22

Quoted 1 line
drivers/misc/eeprom/at24.c +0 -1
-#include <linux/of.h>

I suppose
+ #include <linux/property.h>
?

It looks like <linux/property.h> is included via <linux/acpi.h> outside of any "#ifdef CONFIG_ACPI" check.
But Documentation/process/submit-checklist.rst indicates:

1) If you use a facility then #include the file that defines/declares that facility. Don't depend on other header files pulling in ones that you use.

So, I'll add it.
As an aside, a quick scan of the drivers/ folder shows that fewer than half of the files that use functions from that header actually include it.

Quoted 4 lines

drivers/misc/eeprom/at24.c static int at24_write(void *priv, unsigned int off, void *val, size_t count) · line 565

Quoted 2 lines
drivers/misc/eeprom/at24.c +2 -0
+static void at24_get_pdata(struct device *dev,
+                          struct at24_platform_data *chip)

Now it fits one line.

Sure does. Exactly 80 columns.

Quoted 11 lines

drivers/misc/eeprom/at24.c static int at24_write(void *priv, unsigned int off, void *val, size_t count) · line 572

Quoted 6 lines
drivers/misc/eeprom/at24.c +6 -0
+       u32 val;
+
+       if (device_property_present(dev, "read-only"))
+               chip->flags |= AT24_FLAG_READONLY;
+
+       if (device_property_read_u32(dev, "pagesize", &val) == 0) {

' == 0' looks awkward.

And I think

Quoted 7 lines

ret = x();
if (ret)
...
else
...

pattern would look slightly better.

There are 51 C files that use a temporary variable as you suggest with "device_property_read" functions.
There are 7 C files that use the "if (!device_property_read" style. It looks like at25.c was the only one that uses the '== 0' construct. Guess I picked the wrong one to copy.
So, sure, I'll change that.

Quoted 12 lines

drivers/misc/eeprom/at24.c static int at24_write(void *priv, unsigned int off, void *val, size_t count) · line 580

Quoted 8 lines
drivers/misc/eeprom/at24.c +8 -0
+               chip->page_size = val;
+       } else {
+               /*
+                * This is slow, but we can't know all eeproms, so we better
+                * play safe. Specifying custom eeprom-types via platform_data
+                * is recommended anyhow.
+                */
+               chip.page_size = 1;

--
With Best Regards,
Andy Shevchenko

author
#6 lore raw

PATCH v3 1/1 eeprom/at24: use device_property_*() functions instead of of_get_property()

Allow the at24 driver to get configuration information from both OF and ACPI by using the more generic device_property functions. This change was inspired by the at25.c driver.

I have a custom board with a ST M24C02 EEPROM attached to an I2C bus. With the following ACPI construct, this patch instantiates a working instance of the driver.

Device (EEP0) {
Name (_HID, "PRP0001")
Name (_DSD, Package () {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), Package () {
Package () {"compatible", Package () {"st,24c02"}}, Package () {"pagesize", 16},
},
})
Name (_CRS, ResourceTemplate () {
I2cSerialBus (
0x0057, ControllerInitiated, 400000, AddressingMode7Bit, "\\_SB.PCI0.I2C3", 0x00, ResourceConsumer,,)
})
}

Note: Matching the driver to the I2C device requires another patch.
http://www.spinics.net/lists/linux-acpi/msg71914.html

Signed-off-by Ben Gardner <[email protected]>
---
 drivers/misc/eeprom/at24.c | 45 +++++++++++++++++++--------------------------
 1 file changed, 19 insertions(+), 26 deletions(-)
drivers/misc/eeprom/at24.c +19 -26
index 051b147..c2d9969 100644
@@ -19,7 +19,7 @@
 #include <linux/log2.h>
 #include <linux/bitops.h>
 #include <linux/jiffies.h>
-#include <linux/of.h>
+#include <linux/property.h>
 #include <linux/acpi.h>
 #include <linux/i2c.h>
 #include <linux/nvmem-provider.h>

@@ -562,26 +562,26 @@ static int at24_write(void *priv, unsigned int off, void *val, size_t count)
 	return 0;
 }
 
-#ifdef CONFIG_OF
-static void at24_get_ofdata(struct i2c_client *client,
-			    struct at24_platform_data *chip)
+static void at24_get_pdata(struct device *dev, struct at24_platform_data *chip)
 {
-	const __be32 *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 = be32_to_cpup(val);
+	int err;
+	u32 val;
+
+	if (device_property_present(dev, "read-only"))
+		chip->flags |= AT24_FLAG_READONLY;
+
+	err = device_property_read_u32(dev, "pagesize", &val);
+	if (!err) {
+		chip->page_size = val;
+	} else {
+		/*
+		 * This is slow, but we can't know all eeproms, so we better
+		 * play safe. Specifying custom eeprom-types via platform_data
+		 * is recommended anyhow.
+		 */
+		chip.page_size = 1;
 	}
 }
-#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)
 {

@@ -613,15 +613,8 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		chip.byte_len = BIT(magic & AT24_BITMASK(AT24_SIZE_BYTELEN));
 		magic >>= AT24_SIZE_BYTELEN;
 		chip.flags = magic & AT24_BITMASK(AT24_SIZE_FLAGS);
-		/*
-		 * This is slow, but we can't know all eeproms, so we better
-		 * play safe. Specifying custom eeprom-types via platform_data
-		 * is recommended anyhow.
-		 */
-		chip.page_size = 1;
 
-		/* update chipdata if OF is present */
-		at24_get_ofdata(client, &chip);
+		at24_get_pdata(&client->dev, &chip);
 
 		chip.setup = NULL;
 		chip.context = NULL;
Signature · 1 line
2.7.4
author
#7 lore raw

Ignore this last patch. I should remember to test even the simplest of cut-and-paste ops.

Quoted 110 lines · Ben Gardner

Allow the at24 driver to get configuration information from both OF and ACPI by using the more generic device_property functions. This change was inspired by the at25.c driver.

I have a custom board with a ST M24C02 EEPROM attached to an I2C bus. With the following ACPI construct, this patch instantiates a working instance of the driver.

Device (EEP0) {
Name (_HID, "PRP0001")
Name (_DSD, Package () {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),

  Package () {
   Package () {"compatible", Package () {"st,24c02"}},
   Package () {"pagesize", 16},

},
})
Name (_CRS, ResourceTemplate () {
I2cSerialBus (
0x0057, ControllerInitiated, 400000,
AddressingMode7Bit, "\\_SB.PCI0.I2C3", 0x00,
ResourceConsumer,,)
})
}

Note: Matching the driver to the I2C device requires another patch.
http://www.spinics.net/lists/linux-acpi/msg71914.html

Signed-off-by Ben Gardner <[email protected]>
---
 drivers/misc/eeprom/at24.c | 45 +++++++++++++++++++--------------------------
 1 file changed, 19 insertions(+), 26 deletions(-)
drivers/misc/eeprom/at24.c +19 -27
index 051b147..c2d9969 100644
@@ -19,7 +19,7 @@
 #include <linux/log2.h>
 #include <linux/bitops.h>
 #include <linux/jiffies.h>
-#include <linux/of.h>
+#include <linux/property.h>
 #include <linux/acpi.h>
 #include <linux/i2c.h>
 #include <linux/nvmem-provider.h>

@@ -562,26 +562,26 @@ static int at24_write(void *priv, unsigned int off, void *val, size_t count)
        return 0;
 }
 
-#ifdef CONFIG_OF
-static void at24_get_ofdata(struct i2c_client *client,
-                           struct at24_platform_data *chip)
+static void at24_get_pdata(struct device *dev, struct at24_platform_data *chip)
 {
-       const __be32 *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 = be32_to_cpup(val);
+       int err;
+       u32 val;
+
+       if (device_property_present(dev, "read-only"))
+               chip->flags |= AT24_FLAG_READONLY;
+
+       err = device_property_read_u32(dev, "pagesize", &val);
+       if (!err) {
+               chip->page_size = val;
+       } else {
+               /*
+                * This is slow, but we can't know all eeproms, so we better
+                * play safe. Specifying custom eeprom-types via platform_data
+                * is recommended anyhow.
+                */
+               chip.page_size = 1;
        }
 }
-#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)
 {

@@ -613,15 +613,8 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
                chip.byte_len = BIT(magic & AT24_BITMASK(AT24_SIZE_BYTELEN));
                magic >>= AT24_SIZE_BYTELEN;
                chip.flags = magic & AT24_BITMASK(AT24_SIZE_FLAGS);
-               /*
-                * This is slow, but we can't know all eeproms, so we better
-                * play safe. Specifying custom eeprom-types via platform_data
-                * is recommended anyhow.
-                */
-               chip.page_size = 1;
 
-               /* update chipdata if OF is present */
-               at24_get_ofdata(client, &chip);
+               at24_get_pdata(&client->dev, &chip);
 
                chip.setup = NULL;
                chip.context = NULL;
--

2.7.4

author
#8 lore raw

PATCH v4 1/1 eeprom/at24: use device_property_*() functions instead of of_get_property()

Allow the at24 driver to get configuration information from both OF and ACPI by using the more generic device_property functions. This change was inspired by the at25.c driver.

I have a custom board with a ST M24C02 EEPROM attached to an I2C bus. With the following ACPI construct, this patch instantiates a working instance of the driver.

Device (EEP0) {
Name (_HID, "PRP0001")
Name (_DSD, Package () {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), Package () {
Package () {"compatible", Package () {"st,24c02"}}, Package () {"pagesize", 16},
},
})
Name (_CRS, ResourceTemplate () {
I2cSerialBus (
0x0057, ControllerInitiated, 400000, AddressingMode7Bit, "\\_SB.PCI0.I2C3", 0x00, ResourceConsumer,,)
})
}

Note: Matching the driver to the I2C device requires another patch.
http://www.spinics.net/lists/linux-acpi/msg71914.html

Signed-off-by Ben Gardner <[email protected]>
---
 drivers/misc/eeprom/at24.c | 45 +++++++++++++++++++--------------------------
 1 file changed, 19 insertions(+), 26 deletions(-)
drivers/misc/eeprom/at24.c +19 -26
index 051b147..764ff5df 100644
@@ -19,7 +19,7 @@
 #include <linux/log2.h>
 #include <linux/bitops.h>
 #include <linux/jiffies.h>
-#include <linux/of.h>
+#include <linux/property.h>
 #include <linux/acpi.h>
 #include <linux/i2c.h>
 #include <linux/nvmem-provider.h>

@@ -562,26 +562,26 @@ static int at24_write(void *priv, unsigned int off, void *val, size_t count)
 	return 0;
 }
 
-#ifdef CONFIG_OF
-static void at24_get_ofdata(struct i2c_client *client,
-			    struct at24_platform_data *chip)
+static void at24_get_pdata(struct device *dev, struct at24_platform_data *chip)
 {
-	const __be32 *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 = be32_to_cpup(val);
+	int err;
+	u32 val;
+
+	if (device_property_present(dev, "read-only"))
+		chip->flags |= AT24_FLAG_READONLY;
+
+	err = device_property_read_u32(dev, "pagesize", &val);
+	if (!err) {
+		chip->page_size = val;
+	} else {
+		/*
+		 * This is slow, but we can't know all eeproms, so we better
+		 * play safe. Specifying custom eeprom-types via platform_data
+		 * is recommended anyhow.
+		 */
+		chip->page_size = 1;
 	}
 }
-#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)
 {

@@ -613,15 +613,8 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		chip.byte_len = BIT(magic & AT24_BITMASK(AT24_SIZE_BYTELEN));
 		magic >>= AT24_SIZE_BYTELEN;
 		chip.flags = magic & AT24_BITMASK(AT24_SIZE_FLAGS);
-		/*
-		 * This is slow, but we can't know all eeproms, so we better
-		 * play safe. Specifying custom eeprom-types via platform_data
-		 * is recommended anyhow.
-		 */
-		chip.page_size = 1;
 
-		/* update chipdata if OF is present */
-		at24_get_ofdata(client, &chip);
+		at24_get_pdata(&client->dev, &chip);
 
 		chip.setup = NULL;
 		chip.context = NULL;
Signature · 1 line
2.7.4
Reviewed-by on v4 1/1
#9 lore raw
Quoted 25 lines · Ben Gardner

Allow the at24 driver to get configuration information from both OF and ACPI by using the more generic device_property functions. This change was inspired by the at25.c driver.

I have a custom board with a ST M24C02 EEPROM attached to an I2C bus. With the following ACPI construct, this patch instantiates a working instance of the driver.

Device (EEP0) {
Name (_HID, "PRP0001")
Name (_DSD, Package () {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),

  Package () {
   Package () {"compatible", Package () {"st,24c02"}},
   Package () {"pagesize", 16},

},
})
Name (_CRS, ResourceTemplate () {
I2cSerialBus (
0x0057, ControllerInitiated, 400000,
AddressingMode7Bit, "\\_SB.PCI0.I2C3", 0x00,
ResourceConsumer,,)
})
}

Quoted 2 lines

Note: Matching the driver to the I2C device requires another patch.
http://www.spinics.net/lists/linux-acpi/msg71914.html

Reviewed-by Andy Shevchenko <[email protected]>
Quoted 82 lines
Signed-off-by Ben Gardner <[email protected]>
---
 drivers/misc/eeprom/at24.c | 45 +++++++++++++++++++--------------------------
 1 file changed, 19 insertions(+), 26 deletions(-)
drivers/misc/eeprom/at24.c +19 -27
index 051b147..764ff5df 100644
@@ -19,7 +19,7 @@
 #include <linux/log2.h>
 #include <linux/bitops.h>
 #include <linux/jiffies.h>
-#include <linux/of.h>
+#include <linux/property.h>
 #include <linux/acpi.h>
 #include <linux/i2c.h>
 #include <linux/nvmem-provider.h>

@@ -562,26 +562,26 @@ static int at24_write(void *priv, unsigned int off, void *val, size_t count)
        return 0;
 }
 
-#ifdef CONFIG_OF
-static void at24_get_ofdata(struct i2c_client *client,
-                           struct at24_platform_data *chip)
+static void at24_get_pdata(struct device *dev, struct at24_platform_data *chip)
 {
-       const __be32 *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 = be32_to_cpup(val);
+       int err;
+       u32 val;
+
+       if (device_property_present(dev, "read-only"))
+               chip->flags |= AT24_FLAG_READONLY;
+
+       err = device_property_read_u32(dev, "pagesize", &val);
+       if (!err) {
+               chip->page_size = val;
+       } else {
+               /*
+                * This is slow, but we can't know all eeproms, so we better
+                * play safe. Specifying custom eeprom-types via platform_data
+                * is recommended anyhow.
+                */
+               chip->page_size = 1;
        }
 }
-#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)
 {

@@ -613,15 +613,8 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
                chip.byte_len = BIT(magic & AT24_BITMASK(AT24_SIZE_BYTELEN));
                magic >>= AT24_SIZE_BYTELEN;
                chip.flags = magic & AT24_BITMASK(AT24_SIZE_FLAGS);
-               /*
-                * This is slow, but we can't know all eeproms, so we better
-                * play safe. Specifying custom eeprom-types via platform_data
-                * is recommended anyhow.
-                */
-               chip.page_size = 1;
 
-               /* update chipdata if OF is present */
-               at24_get_ofdata(client, &chip);
+               at24_get_pdata(&client->dev, &chip);
 
                chip.setup = NULL;
                chip.context = NULL;
--

2.7.4

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author
#11 lore raw
Quoted 7 lines · Wolfram Sang
Quoted 3 lines

Note: Matching the driver to the I2C device requires another patch.
http://www.spinics.net/lists/linux-acpi/msg71914.html

Do I get it right? With the patch applied we won't have a regression but the new feature will only be available when the above series is upstream? So, it is a "weak" dependency?

That is correct.
This patch shouldn't cause any regressions, but isn't useful without the other patch series.

The other patch series was picked up today by Rafael J. Wysocki.
http://marc.info/?l=linux-acpi&m=148664872023023&w=2

Ben

Applied
#12 lore raw
In reply to Ben Gardner
Quoted 25 lines · Ben Gardner

Allow the at24 driver to get configuration information from both OF and ACPI by using the more generic device_property functions. This change was inspired by the at25.c driver.

I have a custom board with a ST M24C02 EEPROM attached to an I2C bus. With the following ACPI construct, this patch instantiates a working instance of the driver.

Device (EEP0) {
Name (_HID, "PRP0001")
Name (_DSD, Package () {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),

  Package () {
   Package () {"compatible", Package () {"st,24c02"}},
   Package () {"pagesize", 16},

},
})
Name (_CRS, ResourceTemplate () {
I2cSerialBus (
0x0057, ControllerInitiated, 400000,
AddressingMode7Bit, "\\_SB.PCI0.I2C3", 0x00,
ResourceConsumer,,)
})
}

Quoted 2 lines

Note: Matching the driver to the I2C device requires another patch.
http://www.spinics.net/lists/linux-acpi/msg71914.html

This should have been below the "---" because dependencies are not relevant in the git history.

Quoted 2 lines
Signed-off-by Ben Gardner <[email protected]>

Please send new patches not in-reply-to old patches, seperate threads are easier to find IMHO.

Other than that:

Applied to for-next, thanks!

build bot
#13 lore raw
In reply to Ben Gardner
url: https://github.com/0day-ci/linux/commits/Ben-Gardner/eeprom-at24-use-device_property_-functions-instead-of-of_get_property/20170210-025938 config: x86_64-rhel (attached as .config) compiler…

Hi Ben,

[auto build test ERROR on char-misc/char-misc-testing]
[also build test ERROR on v4.10-rc7 next-20170209]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Ben-Gardner/eeprom-at24-use-device_property_-functions-instead-of-of_get_property/20170210-025938
config: x86_64-rhel (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901 reproduce:

        # save the attached .config to linux build tree
        make ARCH=x86_64

All errors (new ones prefixed by >>):

drivers/misc/eeprom/at24.c: In function 'at24_get_pdata':

>> drivers/misc/eeprom/at24.c:580:7: error: 'chip' is a pointer; did you mean to use '->'?
      chip.page_size = 1;
          ^
          ->

vim +580 drivers/misc/eeprom/at24.c

574 } else {
575 /*
576 * This is slow, but we can't know all eeproms, so we better
577 * play safe. Specifying custom eeprom-types via platform_data
578 * is recommended anyhow.
579 */

 > 580			chip.page_size = 1;

581 }
582 }
583

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

#14 lore raw
Quoted 5 lines · kbuild test robot

Hi Ben,

[auto build test ERROR on char-misc/char-misc-testing]
[also build test ERROR on v4.10-rc7 next-20170209]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

Optimistic way of thinking: kbuild bot was fast enough.
Pessimistic: it wasn't too slow enough.

P.S. I think it against older version of the patch, thus, we may safely ignore this.

Quoted 32 lines

url: https://github.com/0day-ci/linux/commits/Ben-Gardner/eeprom-at24-use-device_property_-functions-instead-of-of_get_property/20170210-025938
config: x86_64-rhel (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901 reproduce:

        # save the attached .config to linux build tree
        make ARCH=x86_64

All errors (new ones prefixed by >>):

drivers/misc/eeprom/at24.c: In function 'at24_get_pdata':

Quoted 1 line
Quoted 1 line

drivers/misc/eeprom/at24.c:580:7: error: 'chip' is a pointer; did you mean to use '->'?

      chip.page_size = 1;
          ^
          ->

vim +580 drivers/misc/eeprom/at24.c

574 } else {
575 /*

   576                   * This is slow, but we can't know all eeproms, so we better
   577                   * play safe. Specifying custom eeprom-types via platform_data
   578                   * is recommended anyhow.
   579                   */
Quoted 1 line

580 chip.page_size = 1;

581 }
582 }
583

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

Signature · 2 lines
With Best Regards,
Andy Shevchenko
build bot
#15 lore raw
In reply to Ben Gardner
url: https://github.com/0day-ci/linux/commits/Ben-Gardner/eeprom-at24-use-device_property_-functions-instead-of-of_get_property/20170210-025938 config: x86_64-randconfig-it0-02100353 (attached as…

Hi Ben,

[auto build test ERROR on char-misc/char-misc-testing]
[also build test ERROR on v4.10-rc7]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Ben-Gardner/eeprom-at24-use-device_property_-functions-instead-of-of_get_property/20170210-025938
config: x86_64-randconfig-it0-02100353 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4 reproduce:

        # save the attached .config to linux build tree
        make ARCH=x86_64

All errors (new ones prefixed by >>):

drivers/misc/eeprom/at24.c: In function 'at24_get_pdata':

>> drivers/misc/eeprom/at24.c:580:7: error: request for member 'page_size' in something not a structure or union
      chip.page_size = 1;
          ^

vim +/page_size +580 drivers/misc/eeprom/at24.c

574 } else {
575 /*
576 * This is slow, but we can't know all eeproms, so we better
577 * play safe. Specifying custom eeprom-types via platform_data
578 * is recommended anyhow.
579 */

 > 580			chip.page_size = 1;

581 }
582 }
583

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation