On Mon, 2 Nov 2020 15:22:28 +0100 Bartosz Golaszewski <[email protected]> wrote:
> From: Bartosz Golaszewski <[email protected]> > > In order to simplify resource management and error paths in probe() and > entirely drop the remove() callback - use devres helpers wherever > possible. Define devm actions for cancelling the delayed work and > disabling the clock. > > Signed-off-by: Bartosz Golaszewski <[email protected]> One addition inline. thanks, Jonathan > --- > drivers/iio/adc/xilinx-xadc-core.c | 129 +++++++++++++---------------- > 1 file changed, 56 insertions(+), 73 deletions(-) > > diff --git a/drivers/iio/adc/xilinx-xadc-core.c > b/drivers/iio/adc/xilinx-xadc-core.c > index 7792aa4cf9cb..6ce1745b4995 100644 > --- a/drivers/iio/adc/xilinx-xadc-core.c > +++ b/drivers/iio/adc/xilinx-xadc-core.c > @@ -709,11 +709,12 @@ static const struct iio_trigger_ops xadc_trigger_ops = { > static struct iio_trigger *xadc_alloc_trigger(struct iio_dev *indio_dev, > const char *name) > { > + struct device *dev = indio_dev->dev.parent; > struct iio_trigger *trig; > int ret; > > - trig = iio_trigger_alloc("%s%d-%s", indio_dev->name, > - indio_dev->id, name); > + trig = devm_iio_trigger_alloc(dev, "%s%d-%s", indio_dev->name, > + indio_dev->id, name); > if (trig == NULL) > return ERR_PTR(-ENOMEM); > > @@ -721,15 +722,11 @@ static struct iio_trigger *xadc_alloc_trigger(struct > iio_dev *indio_dev, > trig->ops = &xadc_trigger_ops; > iio_trigger_set_drvdata(trig, iio_priv(indio_dev)); > > - ret = iio_trigger_register(trig); > + ret = devm_iio_trigger_register(dev, trig); > if (ret) > - goto error_free_trig; > + return ERR_PTR(ret); > > return trig; > - > -error_free_trig: > - iio_trigger_free(trig); > - return ERR_PTR(ret); > } > > static int xadc_power_adc_b(struct xadc *xadc, unsigned int seq_mode) > @@ -1188,6 +1185,20 @@ static int xadc_parse_dt(struct iio_dev *indio_dev, > struct device_node *np, > return 0; > } > > +static void xadc_clk_disable_unprepare(void *data) > +{ > + struct clk *clk = data; > + > + clk_disable_unprepare(clk); > +} > + > +static void xadc_cancel_delayed_work(void *data) > +{ > + struct delayed_work *work = data; > + > + cancel_delayed_work_sync(work); > +} > + > static int xadc_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -1236,34 +1247,35 @@ static int xadc_probe(struct platform_device *pdev) > return ret; > > if (xadc->ops->flags & XADC_FLAGS_BUFFERED) { > - ret = iio_triggered_buffer_setup(indio_dev, > - &iio_pollfunc_store_time, &xadc_trigger_handler, > - &xadc_buffer_ops); > + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, > + &iio_pollfunc_store_time, > + &xadc_trigger_handler, > + &xadc_buffer_ops); > if (ret) > return ret; > > xadc->convst_trigger = xadc_alloc_trigger(indio_dev, "convst"); > - if (IS_ERR(xadc->convst_trigger)) { > - ret = PTR_ERR(xadc->convst_trigger); > - goto err_triggered_buffer_cleanup; > - } > + if (IS_ERR(xadc->convst_trigger)) > + return PTR_ERR(xadc->convst_trigger); > + > xadc->samplerate_trigger = xadc_alloc_trigger(indio_dev, > "samplerate"); > - if (IS_ERR(xadc->samplerate_trigger)) { > - ret = PTR_ERR(xadc->samplerate_trigger); > - goto err_free_convst_trigger; > - } > + if (IS_ERR(xadc->samplerate_trigger)) > + return PTR_ERR(xadc->samplerate_trigger); > } > > xadc->clk = devm_clk_get(dev, NULL); > - if (IS_ERR(xadc->clk)) { > - ret = PTR_ERR(xadc->clk); > - goto err_free_samplerate_trigger; > - } > + if (IS_ERR(xadc->clk)) > + return PTR_ERR(xadc->clk); > > ret = clk_prepare_enable(xadc->clk); > if (ret) > - goto err_free_samplerate_trigger; > + return ret; > + > + ret = devm_add_action_or_reset(dev, > + xadc_clk_disable_unprepare, xadc->clk); > + if (ret) > + return ret; > > /* > * Make sure not to exceed the maximum samplerate since otherwise the > @@ -1272,22 +1284,28 @@ static int xadc_probe(struct platform_device *pdev) > if (xadc->ops->flags & XADC_FLAGS_BUFFERED) { > ret = xadc_read_samplerate(xadc); > if (ret < 0) > - goto err_free_samplerate_trigger; > + return ret; > + > if (ret > XADC_MAX_SAMPLERATE) { > ret = xadc_write_samplerate(xadc, XADC_MAX_SAMPLERATE); > if (ret < 0) > - goto err_free_samplerate_trigger; > + return ret; > } > } > > - ret = request_irq(xadc->irq, xadc->ops->interrupt_handler, 0, > - dev_name(dev), indio_dev); > + ret = devm_request_irq(dev, xadc->irq, xadc->ops->interrupt_handler, 0, > + dev_name(dev), indio_dev); > + if (ret) > + return ret; > + > + ret = devm_add_action_or_reset(dev, xadc_cancel_delayed_work, > + &xadc->zynq_unmask_work); > if (ret) > - goto err_clk_disable_unprepare; > + return ret; > > ret = xadc->ops->setup(pdev, indio_dev, xadc->irq); > if (ret) > - goto err_free_irq; > + return ret; > > for (i = 0; i < 16; i++) > xadc_read_adc_reg(xadc, XADC_REG_THRESHOLD(i), > @@ -1295,7 +1313,7 @@ static int xadc_probe(struct platform_device *pdev) > > ret = xadc_write_adc_reg(xadc, XADC_REG_CONF0, conf0); > if (ret) > - goto err_free_irq; > + return ret; > > bipolar_mask = 0; > for (i = 0; i < indio_dev->num_channels; i++) { > @@ -1305,17 +1323,18 @@ static int xadc_probe(struct platform_device *pdev) > > ret = xadc_write_adc_reg(xadc, XADC_REG_INPUT_MODE(0), bipolar_mask); > if (ret) > - goto err_free_irq; > + return ret; > + > ret = xadc_write_adc_reg(xadc, XADC_REG_INPUT_MODE(1), > bipolar_mask >> 16); > if (ret) > - goto err_free_irq; > + return ret; > > /* Disable all alarms */ > ret = xadc_update_adc_reg(xadc, XADC_REG_CONF1, XADC_CONF1_ALARM_MASK, > XADC_CONF1_ALARM_MASK); > if (ret) > - goto err_free_irq; > + return ret; > > /* Set thresholds to min/max */ > for (i = 0; i < 16; i++) { > @@ -1330,59 +1349,23 @@ static int xadc_probe(struct platform_device *pdev) > ret = xadc_write_adc_reg(xadc, XADC_REG_THRESHOLD(i), > xadc->threshold[i]); > if (ret) > - goto err_free_irq; > + return ret; > } > > /* Go to non-buffered mode */ > xadc_postdisable(indio_dev); > > - ret = iio_device_register(indio_dev); > + ret = devm_iio_device_register(dev, indio_dev); > if (ret) > - goto err_free_irq; > + return ret; > > platform_set_drvdata(pdev, indio_dev); I haven't checked carefully, but it looks like you don't need this any more as there are no remaining calls to platform_get_drvdata(). I can clean that up whilst applying as long as you confirm I'm right! Thanks, Jonathan > > - return 0; > - > -err_free_irq: > - free_irq(xadc->irq, indio_dev); > - cancel_delayed_work_sync(&xadc->zynq_unmask_work); > -err_clk_disable_unprepare: > - clk_disable_unprepare(xadc->clk); > -err_free_samplerate_trigger: > - if (xadc->ops->flags & XADC_FLAGS_BUFFERED) > - iio_trigger_free(xadc->samplerate_trigger); > -err_free_convst_trigger: > - if (xadc->ops->flags & XADC_FLAGS_BUFFERED) > - iio_trigger_free(xadc->convst_trigger); > -err_triggered_buffer_cleanup: > - if (xadc->ops->flags & XADC_FLAGS_BUFFERED) > - iio_triggered_buffer_cleanup(indio_dev); > - > - return ret; > -} > - > -static int xadc_remove(struct platform_device *pdev) > -{ > - struct iio_dev *indio_dev = platform_get_drvdata(pdev); > - struct xadc *xadc = iio_priv(indio_dev); > - > - iio_device_unregister(indio_dev); > - if (xadc->ops->flags & XADC_FLAGS_BUFFERED) { > - iio_trigger_free(xadc->samplerate_trigger); > - iio_trigger_free(xadc->convst_trigger); > - iio_triggered_buffer_cleanup(indio_dev); > - } > - free_irq(xadc->irq, indio_dev); > - cancel_delayed_work_sync(&xadc->zynq_unmask_work); > - clk_disable_unprepare(xadc->clk); > - > return 0; > } > > static struct platform_driver xadc_driver = { > .probe = xadc_probe, > - .remove = xadc_remove, > .driver = { > .name = "xadc", > .of_match_table = xadc_of_match_table,
