Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1577165 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] eeprom/at24: use device_property_*() functions instead of of_get_property() Ben Gardner <gardner.ben@gmail.com> - 2017-02-08 23:50 +0100
    Re: [PATCH] eeprom/at24: use device_property_*() functions instead of of_get_property() Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-02-09 01:10 +0100
      Re: [PATCH] eeprom/at24: use device_property_*() functions instead of of_get_property() Ben Gardner <gardner.ben@gmail.com> - 2017-02-09 16:40 +0100
    [PATCH v2] eeprom/at24: use device_property_*() functions instead of of_get_property() Ben Gardner <gardner.ben@gmail.com> - 2017-02-09 17:10 +0100
      Re: [PATCH v2] eeprom/at24: use device_property_*() functions instead  of of_get_property() Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-02-09 17:30 +0100
        Re: [PATCH v2] eeprom/at24: use device_property_*() functions instead  of of_get_property() Ben Gardner <gardner.ben@gmail.com> - 2017-02-09 19:50 +0100
      Re: [PATCH v2] eeprom/at24: use device_property_*() functions instead  of of_get_property() Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-02-09 22:40 +0100
      Re: [PATCH v2] eeprom/at24: use device_property_*() functions  instead of of_get_property() kbuild test robot <lkp@intel.com> - 2017-02-10 01:00 +0100

#1577165 — [PATCH] 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 <gardner.ben@gmail.com>
---
 drivers/misc/eeprom/at24.c | 28 +++++++++-------------------
 1 file changed, 9 insertions(+), 19 deletions(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 051b147..9cb8904 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,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;
-- 
2.7.4

[toc] | [next] | [standalone]

#1577213

On Wed, Feb 8, 2017 at 9:53 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

Nice!

Few comments, after addressing them

FWIW:
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>


>
> Signed-off-by: Ben Gardner <gardner.ben@gmail.com>
> ---
>  drivers/misc/eeprom/at24.c | 28 +++++++++-------------------
>  1 file changed, 9 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index 051b147..9cb8904 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,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?

>  {
> -       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)

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;


> +               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 */

This is now redundant.

> -               at24_get_ofdata(client, &chip);
> +               at24_fw_to_chip(&client->dev, &chip);
>
>                 chip.setup = NULL;
>                 chip.context = NULL;
> --
> 2.7.4
>



-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1577710

Hi Andy,

Thanks for taking the time to look at this.

On Wed, Feb 8, 2017 at 6:07 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Wed, Feb 8, 2017 at 9:53 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.
> >

(snip)

> Few comments, after addressing them
>
> FWIW:
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>

(snip)

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

> >  {
> > -       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)
>
> 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;

That sounds reasonable.

> > +               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 */
>
> This is now redundant.

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

> > -               at24_get_ofdata(client, &chip);
> > +               at24_fw_to_chip(&client->dev, &chip);
> >
> >                 chip.setup = NULL;
> >                 chip.context = NULL;
> > --
> > 2.7.4
> >
>
>
>
> --
> With Best Regards,
> Andy Shevchenko

I'll send an update.

Thanks,
Ben

[toc] | [prev] | [next] | [standalone]

#1577727 — [PATCH v2] 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 <gardner.ben@gmail.com>
---
 drivers/misc/eeprom/at24.c | 43 +++++++++++++++++--------------------------
 1 file changed, 17 insertions(+), 26 deletions(-)

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;
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1577743 — Re: [PATCH v2] eeprom/at24: use device_property_*() functions instead of of_get_property()

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;

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1577863 — Re: [PATCH v2] eeprom/at24: use device_property_*() functions instead of of_get_property()

> 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

[toc] | [prev] | [next] | [standalone]

#1577978 — Re: [PATCH v2] eeprom/at24: use device_property_*() functions instead of of_get_property()

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



-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1578064 — Re: [PATCH v2] eeprom/at24: use device_property_*() functions instead of of_get_property()

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web