Messages in this thread | | | Date | Mon, 30 Mar 2020 15:12:43 -0500 | From | Michael Auchter <> | Subject | Re: 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
| |