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

[Ben Gardner <gardner.ben@gmail.com>]

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