| Message ID | 20200317145113.12413-1-michael.auchter@ni.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] iio: adc: ad7291: convert to device tree | expand |
On 3/17/20 3:51 PM, Michael Auchter 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. > > Signed-off-by: Michael Auchter <michael.auchter@ni.com> Reviewed-by: Lars-Peter Clausen <lars@metafoo.de>
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? > + 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. > +}; ... > + .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?
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
On Mon, Mar 30, 2020 at 11:12 PM Michael Auchter <michael.auchter@ni.com> wrote: > 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: ... > > > + 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. Yes, please do. ... > > > + .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... Have you used `make W=1 ...`? With it you should get a warning that table defined but not used. > 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. You need either both (of_match_ptr() _and_ ugly ifdeffery, and note you will need of.h for that) or none (mod_devicetable.h maybe needed, though). > Let me know which route would be preferred. If we would like to use this in non-DT environment, then drop all that OF-specific stuff.
On Tue, 2020-03-17 at 09:51 -0500, Michael Auchter wrote: > [External] > > 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. > A few comments inline for this. Sorry if this is a bit late, but since there will be a V3 based on the DT bindings patch, it's still not too late. > Signed-off-by: Michael Auchter <michael.auchter@ni.com> > --- > > Changes since v1: > - Fix regulator handling as suggested by Lars > > drivers/iio/adc/ad7291.c | 33 ++++++++++++++++------------ > include/linux/platform_data/ad7291.h | 13 ----------- > 2 files changed, 19 insertions(+), 27 deletions(-) > delete mode 100644 include/linux/platform_data/ad7291.h > > diff --git a/drivers/iio/adc/ad7291.c b/drivers/iio/adc/ad7291.c > index b2b137fed246..43a201fb4f34 100644 > --- a/drivers/iio/adc/ad7291.c > +++ b/drivers/iio/adc/ad7291.c > @@ -20,8 +20,6 @@ > #include <linux/iio/sysfs.h> > #include <linux/iio/events.h> > > -#include <linux/platform_data/ad7291.h> > - > /* > * Simplified handling > * > @@ -465,7 +463,6 @@ static const struct iio_info ad7291_info = { > static int ad7291_probe(struct i2c_client *client, > const struct i2c_device_id *id) > { > - struct ad7291_platform_data *pdata = client->dev.platform_data; > struct ad7291_chip_info *chip; > struct iio_dev *indio_dev; > int ret; > @@ -475,16 +472,6 @@ static int ad7291_probe(struct i2c_client *client, > return -ENOMEM; > chip = iio_priv(indio_dev); > > - if (pdata && pdata->use_external_ref) { > - chip->reg = devm_regulator_get(&client->dev, "vref"); > - if (IS_ERR(chip->reg)) > - return PTR_ERR(chip->reg); > - > - ret = regulator_enable(chip->reg); > - if (ret) > - return ret; > - } > - > mutex_init(&chip->state_lock); > /* this is only used for device removal purposes */ > i2c_set_clientdata(client, indio_dev); > @@ -495,8 +482,19 @@ static int ad7291_probe(struct i2c_client *client, > AD7291_T_SENSE_MASK | /* Tsense always enabled */ > AD7291_ALERT_POLARITY; /* set irq polarity low level */ > > - if (pdata && pdata->use_external_ref) > + chip->reg = devm_regulator_get_optional(&client->dev, "vref"); > + if (!IS_ERR(chip->reg)) { > + 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; > + } > > indio_dev->name = id->name; > indio_dev->channels = ad7291_channels; > @@ -569,9 +567,16 @@ static const struct i2c_device_id ad7291_id[] = { > > MODULE_DEVICE_TABLE(i2c, ad7291_id); > > +static const struct of_device_id ad7291_of_match[] = { > + { .compatible = "adi,ad7291", }, > + {}, [2] if updating [1], maybe remove the trailing comma for the null-terminator; so, {}, -> {} not a biggy, but there are chances that someone else might complain about it before Jonathan gets a chance to look over this; > +}; > +MODULE_DEVICE_TABLE(of, ad7291_of_match); > + > static struct i2c_driver ad7291_driver = { > .driver = { > .name = KBUILD_MODNAME, > + .of_match_table = of_match_ptr(ad7291_of_match), [1] not sure if there was a comment about this, but I'd remove the of_match_ptr() and just assign this directly; i.e. .of_match_table = ad7297_of_match, there is some code that can also handle this OF-table for ACPI as well; I don't know if this is sufficient for ACPI [with this patch], but it's a good idea to prepare for that; > }, > .probe = ad7291_probe, > .remove = ad7291_remove, > diff --git a/include/linux/platform_data/ad7291.h > b/include/linux/platform_data/ad7291.h > deleted file mode 100644 > index b1fd1530c9a5..000000000000 > --- a/include/linux/platform_data/ad7291.h > +++ /dev/null > @@ -1,13 +0,0 @@ > -/* SPDX-License-Identifier: GPL-2.0 */ > -#ifndef __IIO_AD7291_H__ > -#define __IIO_AD7291_H__ > - > -/** > - * struct ad7291_platform_data - AD7291 platform data > - * @use_external_ref: Whether to use an external or internal reference > voltage > - */ > -struct ad7291_platform_data { > - bool use_external_ref; > -}; > - > -#endif
diff --git a/drivers/iio/adc/ad7291.c b/drivers/iio/adc/ad7291.c index b2b137fed246..43a201fb4f34 100644 --- a/drivers/iio/adc/ad7291.c +++ b/drivers/iio/adc/ad7291.c @@ -20,8 +20,6 @@ #include <linux/iio/sysfs.h> #include <linux/iio/events.h> -#include <linux/platform_data/ad7291.h> - /* * Simplified handling * @@ -465,7 +463,6 @@ static const struct iio_info ad7291_info = { static int ad7291_probe(struct i2c_client *client, const struct i2c_device_id *id) { - struct ad7291_platform_data *pdata = client->dev.platform_data; struct ad7291_chip_info *chip; struct iio_dev *indio_dev; int ret; @@ -475,16 +472,6 @@ static int ad7291_probe(struct i2c_client *client, return -ENOMEM; chip = iio_priv(indio_dev); - if (pdata && pdata->use_external_ref) { - chip->reg = devm_regulator_get(&client->dev, "vref"); - if (IS_ERR(chip->reg)) - return PTR_ERR(chip->reg); - - ret = regulator_enable(chip->reg); - if (ret) - return ret; - } - mutex_init(&chip->state_lock); /* this is only used for device removal purposes */ i2c_set_clientdata(client, indio_dev); @@ -495,8 +482,19 @@ static int ad7291_probe(struct i2c_client *client, AD7291_T_SENSE_MASK | /* Tsense always enabled */ AD7291_ALERT_POLARITY; /* set irq polarity low level */ - if (pdata && pdata->use_external_ref) + chip->reg = devm_regulator_get_optional(&client->dev, "vref"); + if (!IS_ERR(chip->reg)) { + 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; + } indio_dev->name = id->name; indio_dev->channels = ad7291_channels; @@ -569,9 +567,16 @@ static const struct i2c_device_id ad7291_id[] = { MODULE_DEVICE_TABLE(i2c, ad7291_id); +static const struct of_device_id ad7291_of_match[] = { + { .compatible = "adi,ad7291", }, + {}, +}; +MODULE_DEVICE_TABLE(of, ad7291_of_match); + static struct i2c_driver ad7291_driver = { .driver = { .name = KBUILD_MODNAME, + .of_match_table = of_match_ptr(ad7291_of_match), }, .probe = ad7291_probe, .remove = ad7291_remove, diff --git a/include/linux/platform_data/ad7291.h b/include/linux/platform_data/ad7291.h deleted file mode 100644 index b1fd1530c9a5..000000000000 --- a/include/linux/platform_data/ad7291.h +++ /dev/null @@ -1,13 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0 */ -#ifndef __IIO_AD7291_H__ -#define __IIO_AD7291_H__ - -/** - * struct ad7291_platform_data - AD7291 platform data - * @use_external_ref: Whether to use an external or internal reference voltage - */ -struct ad7291_platform_data { - bool use_external_ref; -}; - -#endif
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. Signed-off-by: Michael Auchter <michael.auchter@ni.com> --- Changes since v1: - Fix regulator handling as suggested by Lars drivers/iio/adc/ad7291.c | 33 ++++++++++++++++------------ include/linux/platform_data/ad7291.h | 13 ----------- 2 files changed, 19 insertions(+), 27 deletions(-) delete mode 100644 include/linux/platform_data/ad7291.h