diff mbox

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

Message ID 1486654433-20148-1-git-send-email-gardner.ben@gmail.com
State Superseded
Headers show

Commit Message

Ben Gardner Feb. 9, 2017, 3:33 p.m. UTC 
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 <gardner.ben@gmail.com>
---
 drivers/misc/eeprom/at24.c | 43 +++++++++++++++++--------------------------
 1 file changed, 17 insertions(+), 26 deletions(-)

Comments

Andy Shevchenko Feb. 9, 2017, 4:20 p.m. UTC | #1 
On Thu, Feb 9, 2017 at 5:33 PM, Ben Gardner <gardner.ben@gmail.com> wrote:
> 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
>

Still couple of comments.

> @@ -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>
?

> +static void at24_get_pdata(struct device *dev,
> +                          struct at24_platform_data *chip)

Now it fits one line.

> +       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.

> +               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;
Ben Gardner Feb. 9, 2017, 5:03 p.m. UTC | #2 
> Still couple of comments.
>
>> @@ -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>
> ?

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.

>> +static void at24_get_pdata(struct device *dev,
>> +                          struct at24_platform_data *chip)
>
> Now it fits one line.
Sure does. Exactly 80 columns.

>> +       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.

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.

>> +               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
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
kernel test robot Feb. 9, 2017, 8:35 p.m. UTC | #3 
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
Andy Shevchenko Feb. 9, 2017, 9:30 p.m. UTC | #4 
On Thu, Feb 9, 2017 at 10:35 PM, kbuild test robot <lkp@intel.com> wrote:
> 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.

>
> 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
kernel test robot Feb. 9, 2017, 11:52 p.m. UTC | #5 
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
diff mbox

Patch

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 051b147..81099f3 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -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;