| Message ID | 20200428111430.71723-3-alexandru.ardelean@analog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] iio: adc: ti_am335x_adc: alloc channels via devm_kcalloc() | expand |
On Tue, 28 Apr 2020 14:14:30 +0300 Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > This change converts the rest of the probe to use devm_ functions. > Consequently this allows us to remove the remove hook. > > It tries to preserve the initial order or probe & remove. > The devm_add_action() call hooks the cleanup routine (what's needed still > for the remove part). > If that doesn't work the DMA channel is cleaned up manually inside the > probe hook. This done (like this) because the remove hook has a peculiar > cleanup that tries to restore a step-mask, and that only seems to happen on > the remove hook, and not in any probe error-cleanup paths. > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> First two patches are fine, but this last one is (as you've noted) more complex. I'd like to cleanup the complexity rather than papering over it. So the real question is why we need to restore the step-mask on exit, but not in other paths in the code. From what I recall (and it's been quite a lot of years) the step mask is controlling the 'scan' so that we capture the set of enabled channels and no others (there is a mux that is being controlled). The current optimization is to not bother resetting that to empty when we read individual channels, or come out of buffered mode because we will set it anyway when moving to some new mode. What I can't understand is why we need to set it in the exit path? This is a complex corner given the involvement of the touchscreen driver and mfd. My first inclination is we may be better off leaving it alone unless we have a test setup to make sure we fully understand what is going on. Given your stated reason for tidying this up was to deal with the buffer stuff and this has no impact on that, I'll take patches 1 and 2 for now and leave this one out. However, I'd like to leave more time for comments on those two as well (though they seem 'obviously' correct to me). Thanks, Jonathan > --- > drivers/iio/adc/ti_am335x_adc.c | 63 +++++++++++++++++---------------- > 1 file changed, 32 insertions(+), 31 deletions(-) > > diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c > index 03b2ab649cc3..9fac83e036c1 100644 > --- a/drivers/iio/adc/ti_am335x_adc.c > +++ b/drivers/iio/adc/ti_am335x_adc.c > @@ -562,6 +562,18 @@ static int tiadc_request_dma(struct platform_device *pdev, > return -ENOMEM; > } > > +static void tiadc_cleanup_dma(struct tiadc_device *adc_dev) > +{ > + struct tiadc_dma *dma = &adc_dev->dma; > + > + if (!dma->chan) > + return; > + > + dma_free_coherent(dma->chan->device->dev, DMA_BUFFER_SIZE, > + dma->buf, dma->addr); > + dma_release_channel(dma->chan); > +} > + > static int tiadc_parse_dt(struct platform_device *pdev, > struct tiadc_device *adc_dev) > { > @@ -593,6 +605,17 @@ static int tiadc_parse_dt(struct platform_device *pdev, > return 0; > } > > +static void tiadc_cleanup(void *data) > +{ > + struct tiadc_device *adc_dev = data; > + u32 step_en; > + > + tiadc_cleanup_dma(adc_dev); > + > + step_en = get_adc_step_mask(adc_dev); > + am335x_tsc_se_clr(adc_dev->mfd_tscadc, step_en); > +} > + > static int tiadc_probe(struct platform_device *pdev) > { > struct iio_dev *indio_dev; > @@ -635,48 +658,27 @@ static int tiadc_probe(struct platform_device *pdev) > IRQF_SHARED, > &tiadc_buffer_setup_ops); > > + err = devm_iio_device_register(&pdev->dev, indio_dev); > if (err) > - goto err_free_channels; > - > - err = iio_device_register(indio_dev); > - if (err) > - goto err_buffer_unregister; > + return err; > > platform_set_drvdata(pdev, indio_dev); > > err = tiadc_request_dma(pdev, adc_dev); > if (err && err == -EPROBE_DEFER) > - goto err_dma; > + return err; > + > + err = devm_add_action(&pdev->dev, tiadc_cleanup, adc_dev); > + if (err) > + goto err_free_dma; > > return 0; > > -err_dma: > - iio_device_unregister(indio_dev); > -err_buffer_unregister: > -err_free_channels: > +err_free_dma: > + tiadc_cleanup_dma(adc_dev); > return err; > } > > -static int tiadc_remove(struct platform_device *pdev) > -{ > - struct iio_dev *indio_dev = platform_get_drvdata(pdev); > - struct tiadc_device *adc_dev = iio_priv(indio_dev); > - struct tiadc_dma *dma = &adc_dev->dma; > - u32 step_en; > - > - if (dma->chan) { > - dma_free_coherent(dma->chan->device->dev, DMA_BUFFER_SIZE, > - dma->buf, dma->addr); > - dma_release_channel(dma->chan); > - } > - iio_device_unregister(indio_dev); > - > - step_en = get_adc_step_mask(adc_dev); > - am335x_tsc_se_clr(adc_dev->mfd_tscadc, step_en); > - > - return 0; > -} > - > static int __maybe_unused tiadc_suspend(struct device *dev) > { > struct iio_dev *indio_dev = dev_get_drvdata(dev); > @@ -723,7 +725,6 @@ static struct platform_driver tiadc_driver = { > .of_match_table = ti_adc_dt_ids, > }, > .probe = tiadc_probe, > - .remove = tiadc_remove, > }; > module_platform_driver(tiadc_driver); >
diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c index 03b2ab649cc3..9fac83e036c1 100644 --- a/drivers/iio/adc/ti_am335x_adc.c +++ b/drivers/iio/adc/ti_am335x_adc.c @@ -562,6 +562,18 @@ static int tiadc_request_dma(struct platform_device *pdev, return -ENOMEM; } +static void tiadc_cleanup_dma(struct tiadc_device *adc_dev) +{ + struct tiadc_dma *dma = &adc_dev->dma; + + if (!dma->chan) + return; + + dma_free_coherent(dma->chan->device->dev, DMA_BUFFER_SIZE, + dma->buf, dma->addr); + dma_release_channel(dma->chan); +} + static int tiadc_parse_dt(struct platform_device *pdev, struct tiadc_device *adc_dev) { @@ -593,6 +605,17 @@ static int tiadc_parse_dt(struct platform_device *pdev, return 0; } +static void tiadc_cleanup(void *data) +{ + struct tiadc_device *adc_dev = data; + u32 step_en; + + tiadc_cleanup_dma(adc_dev); + + step_en = get_adc_step_mask(adc_dev); + am335x_tsc_se_clr(adc_dev->mfd_tscadc, step_en); +} + static int tiadc_probe(struct platform_device *pdev) { struct iio_dev *indio_dev; @@ -635,48 +658,27 @@ static int tiadc_probe(struct platform_device *pdev) IRQF_SHARED, &tiadc_buffer_setup_ops); + err = devm_iio_device_register(&pdev->dev, indio_dev); if (err) - goto err_free_channels; - - err = iio_device_register(indio_dev); - if (err) - goto err_buffer_unregister; + return err; platform_set_drvdata(pdev, indio_dev); err = tiadc_request_dma(pdev, adc_dev); if (err && err == -EPROBE_DEFER) - goto err_dma; + return err; + + err = devm_add_action(&pdev->dev, tiadc_cleanup, adc_dev); + if (err) + goto err_free_dma; return 0; -err_dma: - iio_device_unregister(indio_dev); -err_buffer_unregister: -err_free_channels: +err_free_dma: + tiadc_cleanup_dma(adc_dev); return err; } -static int tiadc_remove(struct platform_device *pdev) -{ - struct iio_dev *indio_dev = platform_get_drvdata(pdev); - struct tiadc_device *adc_dev = iio_priv(indio_dev); - struct tiadc_dma *dma = &adc_dev->dma; - u32 step_en; - - if (dma->chan) { - dma_free_coherent(dma->chan->device->dev, DMA_BUFFER_SIZE, - dma->buf, dma->addr); - dma_release_channel(dma->chan); - } - iio_device_unregister(indio_dev); - - step_en = get_adc_step_mask(adc_dev); - am335x_tsc_se_clr(adc_dev->mfd_tscadc, step_en); - - return 0; -} - static int __maybe_unused tiadc_suspend(struct device *dev) { struct iio_dev *indio_dev = dev_get_drvdata(dev); @@ -723,7 +725,6 @@ static struct platform_driver tiadc_driver = { .of_match_table = ti_adc_dt_ids, }, .probe = tiadc_probe, - .remove = tiadc_remove, }; module_platform_driver(tiadc_driver);
This change converts the rest of the probe to use devm_ functions. Consequently this allows us to remove the remove hook. It tries to preserve the initial order or probe & remove. The devm_add_action() call hooks the cleanup routine (what's needed still for the remove part). If that doesn't work the DMA channel is cleaned up manually inside the probe hook. This done (like this) because the remove hook has a peculiar cleanup that tries to restore a step-mask, and that only seems to happen on the remove hook, and not in any probe error-cleanup paths. Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> --- drivers/iio/adc/ti_am335x_adc.c | 63 +++++++++++++++++---------------- 1 file changed, 32 insertions(+), 31 deletions(-)