Re: Re: [PATCH v2 1/2] iio: adc: ad7291: convert to device tree

[Michael Auchter]

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