Hi Andy,
Thanks for taking the time to look at this.
On Wed, Feb 8, 2017 at 6:07 PM, Andy Shevchenko
<andy.shevchenko@xxxxxxxxx> wrote:
>
> On Wed, Feb 8, 2017 at 9:53 PM, Ben Gardner <gardner.ben@xxxxxxxxx> 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@xxxxxxxxx>
>
(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
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
