lkml.org 
[lkml]   [2020]   [Mar]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: Re: [PATCH v2 1/2] iio: adc: ad7291: convert to device tree
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@ni.com> 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

\
 
 \ /
  Last update: 2020-03-30 23:01    [W:0.803 / U:7.048 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site