Hello Andy,
Thanks for the review!
On Sun, Mar 22, 2020 at 01:46:21AM +0200, Andy Shevchenko wrote:
> On Tue, Mar 17, 2020 at 4:53 PM Michael Auchter <michael.auchter@xxxxxx> wrote:
> >
> > There are no in-tree users of the platform data for this driver, so
> > remove it and convert the driver to use device tree instead.
>
> ...
>
> > + chip->reg = devm_regulator_get_optional(&client->dev, "vref");
> > + if (!IS_ERR(chip->reg)) {
>
> Why not to go with usual positive conditional?
I took this pattern from ad7266.c which Lars pointed me to. I agree that
a positive conditional here would probably be more natural. I'll change
that if you'd prefer.
> > + ret = regulator_enable(chip->reg);
> > + if (ret)
> > + return ret;
> > +
> > chip->command |= AD7291_EXT_REF;
> > + } else {
> > + if (PTR_ERR(chip->reg) != -ENODEV)
> > + return PTR_ERR(chip->reg);
> > +
> > + chip->reg = NULL;
> > + }
>
> ...
>
> > +static const struct of_device_id ad7291_of_match[] = {
> > + { .compatible = "adi,ad7291", },
>
> > + {},
>
> No need for comma.
Indeed, I'll drop it.
>
> > +};
>
> ...
>
> > + .of_match_table = of_match_ptr(ad7291_of_match),
>
> No need to use of_match_ptr(). Haven't you got a compiler warning in !OF case?
Hm, no warning as far as I can see with !OF... but agreed that this
doesn't make much sense as-is.
Is dropping of_match_ptr() the preferred route here? The driver doesn't
depend on OF, so it seems like keeping of_match_ptr and instead guarding
the ad7291_of_match table with #ifdef CONFIG_OF would be preferred. Of
course, maybe that's not worth it for saving some bytes from the final
image.
Let me know which route would be preferred.
Thanks again,
Michael
