| Message ID | 20240531-iio-adc-ref-supply-refactor-v1-2-4b313c0615ad@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: use devm_regulator_get_enable_read_voltage round 1 | expand |
On Fri, 2024-05-31 at 16:19 -0500, David Lechner wrote: > This makes use of the new devm_regulator_get_enable_read_voltage() > function to reduce boilerplate code. > > Signed-off-by: David Lechner <dlechner@baylibre.com> > --- > drivers/iio/adc/ad7266.c | 37 ++++++++++--------------------------- > 1 file changed, 10 insertions(+), 27 deletions(-) > > diff --git a/drivers/iio/adc/ad7266.c b/drivers/iio/adc/ad7266.c > index 353a97f9c086..026db1bedc0a 100644 > --- a/drivers/iio/adc/ad7266.c > +++ b/drivers/iio/adc/ad7266.c > @@ -25,7 +25,6 @@ > > struct ad7266_state { > struct spi_device *spi; > - struct regulator *reg; > unsigned long vref_mv; > > struct spi_transfer single_xfer[3]; > @@ -377,11 +376,6 @@ static const char * const ad7266_gpio_labels[] = { > "ad0", "ad1", "ad2", > }; > > -static void ad7266_reg_disable(void *reg) > -{ > - regulator_disable(reg); > -} > - > static int ad7266_probe(struct spi_device *spi) > { > struct ad7266_platform_data *pdata = spi->dev.platform_data; > @@ -396,28 +390,17 @@ static int ad7266_probe(struct spi_device *spi) > > st = iio_priv(indio_dev); > > - st->reg = devm_regulator_get_optional(&spi->dev, "vref"); > - if (!IS_ERR(st->reg)) { > - ret = regulator_enable(st->reg); > - if (ret) > - return ret; > - > - ret = devm_add_action_or_reset(&spi->dev, ad7266_reg_disable, st- > >reg); > - if (ret) > - return ret; > - > - ret = regulator_get_voltage(st->reg); > - if (ret < 0) > - return ret; > - > - st->vref_mv = ret / 1000; > - } else { > - /* Any other error indicates that the regulator does exist */ > - if (PTR_ERR(st->reg) != -ENODEV) > - return PTR_ERR(st->reg); > - /* Use internal reference */ > + /* > + * Use external reference from vref if present, otherwise use 2.5V > + * internal reference. > + */ Not sure the comment brings any added value. The code is fairly self explanatory IMO... > + ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vref"); > + if (ret == -ENODEV) > st->vref_mv = 2500; > - } > + else if (ret < 0) > + return ret; > + else I think it would be better (as that is the typical pattern) to first check for errors. Also the 'return' in the middle of the else if () is a bit weird to me... Maybe something like this? if (ret < 0 && ret != -ENODEV) return ret; if (ret == -ENODEV) st->vref_mv = 2500; else st->vref_mv = ret / 1000; or even replacing the if() else by st->vref_mv = ret == -ENODEV ? 2500 : ret / 1000; - Nuno Sá
On 6/4/24 6:19 AM, Nuno Sá wrote: > On Fri, 2024-05-31 at 16:19 -0500, David Lechner wrote: >> This makes use of the new devm_regulator_get_enable_read_voltage() >> function to reduce boilerplate code. >> >> Signed-off-by: David Lechner <dlechner@baylibre.com> >> --- >> drivers/iio/adc/ad7266.c | 37 ++++++++++--------------------------- >> 1 file changed, 10 insertions(+), 27 deletions(-) >> >> diff --git a/drivers/iio/adc/ad7266.c b/drivers/iio/adc/ad7266.c >> index 353a97f9c086..026db1bedc0a 100644 >> --- a/drivers/iio/adc/ad7266.c >> +++ b/drivers/iio/adc/ad7266.c >> @@ -25,7 +25,6 @@ >> >> struct ad7266_state { >> struct spi_device *spi; >> - struct regulator *reg; >> unsigned long vref_mv; >> >> struct spi_transfer single_xfer[3]; >> @@ -377,11 +376,6 @@ static const char * const ad7266_gpio_labels[] = { >> "ad0", "ad1", "ad2", >> }; >> >> -static void ad7266_reg_disable(void *reg) >> -{ >> - regulator_disable(reg); >> -} >> - >> static int ad7266_probe(struct spi_device *spi) >> { >> struct ad7266_platform_data *pdata = spi->dev.platform_data; >> @@ -396,28 +390,17 @@ static int ad7266_probe(struct spi_device *spi) >> >> st = iio_priv(indio_dev); >> >> - st->reg = devm_regulator_get_optional(&spi->dev, "vref"); >> - if (!IS_ERR(st->reg)) { >> - ret = regulator_enable(st->reg); >> - if (ret) >> - return ret; >> - >> - ret = devm_add_action_or_reset(&spi->dev, ad7266_reg_disable, st- >>> reg); >> - if (ret) >> - return ret; >> - >> - ret = regulator_get_voltage(st->reg); >> - if (ret < 0) >> - return ret; >> - >> - st->vref_mv = ret / 1000; >> - } else { >> - /* Any other error indicates that the regulator does exist */ >> - if (PTR_ERR(st->reg) != -ENODEV) >> - return PTR_ERR(st->reg); >> - /* Use internal reference */ >> + /* >> + * Use external reference from vref if present, otherwise use 2.5V >> + * internal reference. >> + */ > > Not sure the comment brings any added value. The code is fairly self explanatory > IMO... Well, you do this every day. :-) For someone who never wrote an IIO driver, it could be helpful. > >> + ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vref"); >> + if (ret == -ENODEV) >> st->vref_mv = 2500; >> - } >> + else if (ret < 0) >> + return ret; >> + else > > I think it would be better (as that is the typical pattern) to first check for > errors. Also the 'return' in the middle of the else if () is a bit weird to me... > Maybe something like this? > > if (ret < 0 && ret != -ENODEV) > return ret; > if (ret == -ENODEV) > st->vref_mv = 2500; > else > st->vref_mv = ret / 1000; > > or even replacing the if() else by > st->vref_mv = ret == -ENODEV ? 2500 : ret / 1000; > > - Nuno Sá > I think I like that better too.
On Tue, 2024-06-04 at 09:10 -0500, David Lechner wrote: > On 6/4/24 6:19 AM, Nuno Sá wrote: > > On Fri, 2024-05-31 at 16:19 -0500, David Lechner wrote: > > > This makes use of the new devm_regulator_get_enable_read_voltage() > > > function to reduce boilerplate code. > > > > > > Signed-off-by: David Lechner <dlechner@baylibre.com> > > > --- > > > drivers/iio/adc/ad7266.c | 37 ++++++++++--------------------------- > > > 1 file changed, 10 insertions(+), 27 deletions(-) > > > > > > diff --git a/drivers/iio/adc/ad7266.c b/drivers/iio/adc/ad7266.c > > > index 353a97f9c086..026db1bedc0a 100644 > > > --- a/drivers/iio/adc/ad7266.c > > > +++ b/drivers/iio/adc/ad7266.c > > > @@ -25,7 +25,6 @@ > > > > > > struct ad7266_state { > > > struct spi_device *spi; > > > - struct regulator *reg; > > > unsigned long vref_mv; > > > > > > struct spi_transfer single_xfer[3]; > > > @@ -377,11 +376,6 @@ static const char * const ad7266_gpio_labels[] = { > > > "ad0", "ad1", "ad2", > > > }; > > > > > > -static void ad7266_reg_disable(void *reg) > > > -{ > > > - regulator_disable(reg); > > > -} > > > - > > > static int ad7266_probe(struct spi_device *spi) > > > { > > > struct ad7266_platform_data *pdata = spi->dev.platform_data; > > > @@ -396,28 +390,17 @@ static int ad7266_probe(struct spi_device *spi) > > > > > > st = iio_priv(indio_dev); > > > > > > - st->reg = devm_regulator_get_optional(&spi->dev, "vref"); > > > - if (!IS_ERR(st->reg)) { > > > - ret = regulator_enable(st->reg); > > > - if (ret) > > > - return ret; > > > - > > > - ret = devm_add_action_or_reset(&spi->dev, > > > ad7266_reg_disable, st- > > > > reg); > > > - if (ret) > > > - return ret; > > > - > > > - ret = regulator_get_voltage(st->reg); > > > - if (ret < 0) > > > - return ret; > > > - > > > - st->vref_mv = ret / 1000; > > > - } else { > > > - /* Any other error indicates that the regulator does > > > exist */ > > > - if (PTR_ERR(st->reg) != -ENODEV) > > > - return PTR_ERR(st->reg); > > > - /* Use internal reference */ > > > + /* > > > + * Use external reference from vref if present, otherwise use > > > 2.5V > > > + * internal reference. > > > + */ > > > > Not sure the comment brings any added value. The code is fairly self > > explanatory > > IMO... > > Well, you do this every day. :-) I guess... still not an excuse :) > > For someone who never wrote an IIO driver, it could be helpful. Still dunno but no strong feelings anyways... - Nuno Sá
diff --git a/drivers/iio/adc/ad7266.c b/drivers/iio/adc/ad7266.c index 353a97f9c086..026db1bedc0a 100644 --- a/drivers/iio/adc/ad7266.c +++ b/drivers/iio/adc/ad7266.c @@ -25,7 +25,6 @@ struct ad7266_state { struct spi_device *spi; - struct regulator *reg; unsigned long vref_mv; struct spi_transfer single_xfer[3]; @@ -377,11 +376,6 @@ static const char * const ad7266_gpio_labels[] = { "ad0", "ad1", "ad2", }; -static void ad7266_reg_disable(void *reg) -{ - regulator_disable(reg); -} - static int ad7266_probe(struct spi_device *spi) { struct ad7266_platform_data *pdata = spi->dev.platform_data; @@ -396,28 +390,17 @@ static int ad7266_probe(struct spi_device *spi) st = iio_priv(indio_dev); - st->reg = devm_regulator_get_optional(&spi->dev, "vref"); - if (!IS_ERR(st->reg)) { - ret = regulator_enable(st->reg); - if (ret) - return ret; - - ret = devm_add_action_or_reset(&spi->dev, ad7266_reg_disable, st->reg); - if (ret) - return ret; - - ret = regulator_get_voltage(st->reg); - if (ret < 0) - return ret; - - st->vref_mv = ret / 1000; - } else { - /* Any other error indicates that the regulator does exist */ - if (PTR_ERR(st->reg) != -ENODEV) - return PTR_ERR(st->reg); - /* Use internal reference */ + /* + * Use external reference from vref if present, otherwise use 2.5V + * internal reference. + */ + ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vref"); + if (ret == -ENODEV) st->vref_mv = 2500; - } + else if (ret < 0) + return ret; + else + st->vref_mv = ret / 1000; if (pdata) { st->fixed_addr = pdata->fixed_addr;
This makes use of the new devm_regulator_get_enable_read_voltage() function to reduce boilerplate code. Signed-off-by: David Lechner <dlechner@baylibre.com> --- drivers/iio/adc/ad7266.c | 37 ++++++++++--------------------------- 1 file changed, 10 insertions(+), 27 deletions(-)