| Message ID | 20221016163409.320197-14-jic23@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | IIO: More devm_regulator[_bulk]_get_enable() users | expand |
On 10/16/22 19:34, Jonathan Cameron wrote: > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > This driver only turns the power on at probe and off via a custom > devm_add_action_or_reset() callback. I think this one did not use devm_add_action_or_reset() > The new devm_regulator_get_enable() > replaces this boilerplate code. Some additional refactoring to drop > now unnecessary unwinding after this change. > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Cc: Tomasz Duszynski <tduszyns@gmail.com> > --- > drivers/iio/pressure/ms5611.h | 3 --- > drivers/iio/pressure/ms5611_core.c | 32 +++++------------------------- > 2 files changed, 5 insertions(+), 30 deletions(-) > // snip > @@ -477,7 +456,6 @@ void ms5611_remove(struct iio_dev *indio_dev) > { > iio_device_unregister(indio_dev); > iio_triggered_buffer_cleanup(indio_dev); > - ms5611_fini(indio_dev); > } > EXPORT_SYMBOL_NS(ms5611_remove, IIO_MS5611); Just a thought but maybe the whole remove() could be done using devm()? (As far as I can say the current flow works. AFAICS the devm unwinding is done after the remove() has been ran. But perhaps it would be cleaner if the remove() would not need to be exported at all.) With, or without the remove() removed, if commit description is fixed: Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>
On 10/16/22 19:34, Jonathan Cameron wrote: > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > This driver only turns the power on at probe and off via a custom > devm_add_action_or_reset() callback. I think this one did not use devm_add_action_or_reset() > The new devm_regulator_get_enable() > replaces this boilerplate code. Some additional refactoring to drop > now unnecessary unwinding after this change. > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Cc: Tomasz Duszynski <tduszyns@gmail.com> > --- > drivers/iio/pressure/ms5611.h | 3 --- > drivers/iio/pressure/ms5611_core.c | 32 +++++------------------------- > 2 files changed, 5 insertions(+), 30 deletions(-) > // snip > @@ -477,7 +456,6 @@ void ms5611_remove(struct iio_dev *indio_dev) > { > iio_device_unregister(indio_dev); > iio_triggered_buffer_cleanup(indio_dev); > - ms5611_fini(indio_dev); > } > EXPORT_SYMBOL_NS(ms5611_remove, IIO_MS5611); Just a thought but maybe the whole remove() could be done using devm()? (As far as I can say the current flow works. AFAICS the devm unwinding is done after the remove() has been ran. But perhaps it would be cleaner if the remove() would not need to be exported at all.) With, or without the remove() removed, if commit description is fixed: Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>
On Mon, 17 Oct 2022 09:26:15 +0300 Matti Vaittinen <mazziesaccount@gmail.com> wrote: > On 10/16/22 19:34, Jonathan Cameron wrote: > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > This driver only turns the power on at probe and off via a custom > > devm_add_action_or_reset() callback. > > I think this one did not use devm_add_action_or_reset() The other changes hitting this driver are now upstream so applied to the togreg branch of iio.git with a bit of fuzz. Thanks, Jonathan > > > The new devm_regulator_get_enable() > > replaces this boilerplate code. Some additional refactoring to drop > > now unnecessary unwinding after this change. > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Cc: Tomasz Duszynski <tduszyns@gmail.com> > > --- > > drivers/iio/pressure/ms5611.h | 3 --- > > drivers/iio/pressure/ms5611_core.c | 32 +++++------------------------- > > 2 files changed, 5 insertions(+), 30 deletions(-) > > > > // snip > > > @@ -477,7 +456,6 @@ void ms5611_remove(struct iio_dev *indio_dev) > > { > > iio_device_unregister(indio_dev); > > iio_triggered_buffer_cleanup(indio_dev); > > - ms5611_fini(indio_dev); > > } > > EXPORT_SYMBOL_NS(ms5611_remove, IIO_MS5611); > > Just a thought but maybe the whole remove() could be done using devm()? > (As far as I can say the current flow works. AFAICS the devm unwinding > is done after the remove() has been ran. But perhaps it would be cleaner > if the remove() would not need to be exported at all.) > > With, or without the remove() removed, if commit description is fixed: > Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com> >
diff --git a/drivers/iio/pressure/ms5611.h b/drivers/iio/pressure/ms5611.h index cbc9349c342a..816e83befd23 100644 --- a/drivers/iio/pressure/ms5611.h +++ b/drivers/iio/pressure/ms5611.h @@ -13,8 +13,6 @@ #include <linux/iio/iio.h> #include <linux/mutex.h> -struct regulator; - #define MS5611_RESET 0x1e #define MS5611_READ_ADC 0x00 #define MS5611_READ_PROM_WORD 0xA0 @@ -56,7 +54,6 @@ struct ms5611_state { s32 *temp, s32 *pressure); struct ms5611_chip_info *chip_info; - struct regulator *vdd; }; int ms5611_probe(struct iio_dev *indio_dev, struct device *dev, diff --git a/drivers/iio/pressure/ms5611_core.c b/drivers/iio/pressure/ms5611_core.c index 717521de66c4..b95ee6034548 100644 --- a/drivers/iio/pressure/ms5611_core.c +++ b/drivers/iio/pressure/ms5611_core.c @@ -390,40 +390,21 @@ static const struct iio_info ms5611_info = { static int ms5611_init(struct iio_dev *indio_dev) { int ret; - struct ms5611_state *st = iio_priv(indio_dev); /* Enable attached regulator if any. */ - st->vdd = devm_regulator_get(indio_dev->dev.parent, "vdd"); - if (IS_ERR(st->vdd)) - return PTR_ERR(st->vdd); - - ret = regulator_enable(st->vdd); - if (ret) { - dev_err(indio_dev->dev.parent, - "failed to enable Vdd supply: %d\n", ret); + ret = devm_regulator_get_enable(indio_dev->dev.parent, "vdd"); + if (ret) return ret; - } ret = ms5611_reset(indio_dev); if (ret < 0) - goto err_regulator_disable; + return ret; ret = ms5611_read_prom(indio_dev); if (ret < 0) - goto err_regulator_disable; + return ret; return 0; - -err_regulator_disable: - regulator_disable(st->vdd); - return ret; -} - -static void ms5611_fini(const struct iio_dev *indio_dev) -{ - const struct ms5611_state *st = iio_priv(indio_dev); - - regulator_disable(st->vdd); } int ms5611_probe(struct iio_dev *indio_dev, struct device *dev, @@ -454,7 +435,7 @@ int ms5611_probe(struct iio_dev *indio_dev, struct device *dev, ms5611_trigger_handler, NULL); if (ret < 0) { dev_err(dev, "iio triggered buffer setup failed\n"); - goto err_fini; + return ret; } ret = iio_device_register(indio_dev); @@ -467,8 +448,6 @@ int ms5611_probe(struct iio_dev *indio_dev, struct device *dev, err_buffer_cleanup: iio_triggered_buffer_cleanup(indio_dev); -err_fini: - ms5611_fini(indio_dev); return ret; } EXPORT_SYMBOL_NS(ms5611_probe, IIO_MS5611); @@ -477,7 +456,6 @@ void ms5611_remove(struct iio_dev *indio_dev) { iio_device_unregister(indio_dev); iio_triggered_buffer_cleanup(indio_dev); - ms5611_fini(indio_dev); } EXPORT_SYMBOL_NS(ms5611_remove, IIO_MS5611);