Re: [PATCH 04/11] staging: iio: adc: ad7606: Use devm functions in probe
On Thu, 13 Dec 2018 14:46:16 +0200 Stefan Popa wrote: > Switch to devm version of request_irq, iio_triggered_buffer_setup, > iio_device_register. To avoid potential ordering issues in probe, > devm_add_action_or_reset() is used for the regulator_disable(). This > simplifies the code and decreases the chance of bugs. > > Signed-off-by: Stefan Popa Very nice. Applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to play with it. Thanks, Jonathan > --- > drivers/staging/iio/adc/ad7606.c | 59 > +--- > drivers/staging/iio/adc/ad7606.h | 1 - > drivers/staging/iio/adc/ad7606_par.c | 6 > drivers/staging/iio/adc/ad7606_spi.c | 6 > 4 files changed, 21 insertions(+), 51 deletions(-) > > diff --git a/drivers/staging/iio/adc/ad7606.c > b/drivers/staging/iio/adc/ad7606.c > index 4b1bc20..7191d51 100644 > --- a/drivers/staging/iio/adc/ad7606.c > +++ b/drivers/staging/iio/adc/ad7606.c > @@ -417,6 +417,13 @@ static const struct iio_info ad7606_info_range = { > .attrs = _attribute_group_range, > }; > > +static void ad7606_regulator_disable(void *data) > +{ > + struct ad7606_state *st = data; > + > + regulator_disable(st->reg); > +} > + > int ad7606_probe(struct device *dev, int irq, void __iomem *base_address, >const char *name, unsigned int id, >const struct ad7606_bus_ops *bops) > @@ -430,6 +437,7 @@ int ad7606_probe(struct device *dev, int irq, void > __iomem *base_address, > return -ENOMEM; > > st = iio_priv(indio_dev); > + dev_set_drvdata(dev, indio_dev); > > st->dev = dev; > mutex_init(>lock); > @@ -450,11 +458,15 @@ int ad7606_probe(struct device *dev, int irq, void > __iomem *base_address, > return ret; > } > > + ret = devm_add_action_or_reset(dev, ad7606_regulator_disable, st); > + if (ret) > + return ret; > + > st->chip_info = _chip_info_tbl[id]; > > ret = ad7606_request_gpios(st); > if (ret) > - goto error_disable_reg; > + return ret; > > indio_dev->dev.parent = dev; > if (st->gpio_os) { > @@ -479,50 +491,21 @@ int ad7606_probe(struct device *dev, int irq, void > __iomem *base_address, > if (ret) > dev_warn(st->dev, "failed to RESET: no RESET GPIO specified\n"); > > - ret = request_irq(irq, ad7606_interrupt, IRQF_TRIGGER_FALLING, name, > - indio_dev); > - if (ret) > - goto error_disable_reg; > - > - ret = iio_triggered_buffer_setup(indio_dev, _trigger_handler, > - NULL, NULL); > + ret = devm_request_irq(dev, irq, ad7606_interrupt, IRQF_TRIGGER_FALLING, > +name, indio_dev); > if (ret) > - goto error_free_irq; > + return ret; > > - ret = iio_device_register(indio_dev); > + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, > + _trigger_handler, > + NULL, NULL); > if (ret) > - goto error_unregister_ring; > - > - dev_set_drvdata(dev, indio_dev); > - > - return 0; > -error_unregister_ring: > - iio_triggered_buffer_cleanup(indio_dev); > - > -error_free_irq: > - free_irq(irq, indio_dev); > + return ret; > > -error_disable_reg: > - regulator_disable(st->reg); > - return ret; > + return devm_iio_device_register(dev, indio_dev); > } > EXPORT_SYMBOL_GPL(ad7606_probe); > > -int ad7606_remove(struct device *dev, int irq) > -{ > - struct iio_dev *indio_dev = dev_get_drvdata(dev); > - struct ad7606_state *st = iio_priv(indio_dev); > - > - iio_device_unregister(indio_dev); > - iio_triggered_buffer_cleanup(indio_dev); > - > - free_irq(irq, indio_dev); > - regulator_disable(st->reg); > - > - return 0; > -} > -EXPORT_SYMBOL_GPL(ad7606_remove); > - > #ifdef CONFIG_PM_SLEEP > > static int ad7606_suspend(struct device *dev) > diff --git a/drivers/staging/iio/adc/ad7606.h > b/drivers/staging/iio/adc/ad7606.h > index cf20ca2..70486ef 100644 > --- a/drivers/staging/iio/adc/ad7606.h > +++ b/drivers/staging/iio/adc/ad7606.h > @@ -84,7 +84,6 @@ struct ad7606_bus_ops { > int ad7606_probe(struct device *dev, int irq, void __iomem *base_address, >const char *name, unsigned int id, >const struct ad7606_bus_ops *bops); > -int ad7606_remove(struct device *dev, int irq); > > enum ad7606_supported_device_ids { > ID_AD7605_4, > diff --git a/drivers/staging/iio/adc/ad7606_par.c > b/drivers/staging/iio/adc/ad7606_par.c > index db2fede46..6269ee7 100644 > --- a/drivers/staging/iio/adc/ad7606_par.c > +++ b/drivers/staging/iio/adc/ad7606_par.c > @@ -71,11 +71,6 @@ static int ad7606_par_probe(struct platform_device *pdev) > _par8_bops); > } >
[PATCH 04/11] staging: iio: adc: ad7606: Use devm functions in probe
Switch to devm version of request_irq, iio_triggered_buffer_setup, iio_device_register. To avoid potential ordering issues in probe, devm_add_action_or_reset() is used for the regulator_disable(). This simplifies the code and decreases the chance of bugs. Signed-off-by: Stefan Popa --- drivers/staging/iio/adc/ad7606.c | 59 +--- drivers/staging/iio/adc/ad7606.h | 1 - drivers/staging/iio/adc/ad7606_par.c | 6 drivers/staging/iio/adc/ad7606_spi.c | 6 4 files changed, 21 insertions(+), 51 deletions(-) diff --git a/drivers/staging/iio/adc/ad7606.c b/drivers/staging/iio/adc/ad7606.c index 4b1bc20..7191d51 100644 --- a/drivers/staging/iio/adc/ad7606.c +++ b/drivers/staging/iio/adc/ad7606.c @@ -417,6 +417,13 @@ static const struct iio_info ad7606_info_range = { .attrs = _attribute_group_range, }; +static void ad7606_regulator_disable(void *data) +{ + struct ad7606_state *st = data; + + regulator_disable(st->reg); +} + int ad7606_probe(struct device *dev, int irq, void __iomem *base_address, const char *name, unsigned int id, const struct ad7606_bus_ops *bops) @@ -430,6 +437,7 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address, return -ENOMEM; st = iio_priv(indio_dev); + dev_set_drvdata(dev, indio_dev); st->dev = dev; mutex_init(>lock); @@ -450,11 +458,15 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address, return ret; } + ret = devm_add_action_or_reset(dev, ad7606_regulator_disable, st); + if (ret) + return ret; + st->chip_info = _chip_info_tbl[id]; ret = ad7606_request_gpios(st); if (ret) - goto error_disable_reg; + return ret; indio_dev->dev.parent = dev; if (st->gpio_os) { @@ -479,50 +491,21 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address, if (ret) dev_warn(st->dev, "failed to RESET: no RESET GPIO specified\n"); - ret = request_irq(irq, ad7606_interrupt, IRQF_TRIGGER_FALLING, name, - indio_dev); - if (ret) - goto error_disable_reg; - - ret = iio_triggered_buffer_setup(indio_dev, _trigger_handler, -NULL, NULL); + ret = devm_request_irq(dev, irq, ad7606_interrupt, IRQF_TRIGGER_FALLING, + name, indio_dev); if (ret) - goto error_free_irq; + return ret; - ret = iio_device_register(indio_dev); + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, + _trigger_handler, + NULL, NULL); if (ret) - goto error_unregister_ring; - - dev_set_drvdata(dev, indio_dev); - - return 0; -error_unregister_ring: - iio_triggered_buffer_cleanup(indio_dev); - -error_free_irq: - free_irq(irq, indio_dev); + return ret; -error_disable_reg: - regulator_disable(st->reg); - return ret; + return devm_iio_device_register(dev, indio_dev); } EXPORT_SYMBOL_GPL(ad7606_probe); -int ad7606_remove(struct device *dev, int irq) -{ - struct iio_dev *indio_dev = dev_get_drvdata(dev); - struct ad7606_state *st = iio_priv(indio_dev); - - iio_device_unregister(indio_dev); - iio_triggered_buffer_cleanup(indio_dev); - - free_irq(irq, indio_dev); - regulator_disable(st->reg); - - return 0; -} -EXPORT_SYMBOL_GPL(ad7606_remove); - #ifdef CONFIG_PM_SLEEP static int ad7606_suspend(struct device *dev) diff --git a/drivers/staging/iio/adc/ad7606.h b/drivers/staging/iio/adc/ad7606.h index cf20ca2..70486ef 100644 --- a/drivers/staging/iio/adc/ad7606.h +++ b/drivers/staging/iio/adc/ad7606.h @@ -84,7 +84,6 @@ struct ad7606_bus_ops { int ad7606_probe(struct device *dev, int irq, void __iomem *base_address, const char *name, unsigned int id, const struct ad7606_bus_ops *bops); -int ad7606_remove(struct device *dev, int irq); enum ad7606_supported_device_ids { ID_AD7605_4, diff --git a/drivers/staging/iio/adc/ad7606_par.c b/drivers/staging/iio/adc/ad7606_par.c index db2fede46..6269ee7 100644 --- a/drivers/staging/iio/adc/ad7606_par.c +++ b/drivers/staging/iio/adc/ad7606_par.c @@ -71,11 +71,6 @@ static int ad7606_par_probe(struct platform_device *pdev) _par8_bops); } -static int ad7606_par_remove(struct platform_device *pdev) -{ - return ad7606_remove(>dev, platform_get_irq(pdev, 0)); -} - static const struct platform_device_id ad7606_driver_ids[] = { { .name = "ad7605-4", @@ -97,7 +92,6 @@ MODULE_DEVICE_TABLE(platform, ad7606_driver_ids); static struct platform_driver