From: Bartosz Golaszewski <[email protected]>
This is a follow-up to commit 750628c79bb1 ("iio: adc: xilinx-xadc: use
devm_krealloc()"). I noticed we can use even more devres helpers and entirely
drop the remove() callback.
Bartosz Golaszewski (5):
iio: adc: xilinx: use helper variable for &pdev->dev
iio: adc: xilinx: use devm_krealloc() instead of kfree() + kcalloc()
iio: adc: xilinx: use a devres action to disable and unprepare the
clock
iio: adc: xilinx: use devres for irq handling
iio: adc: xilinx: use iio devres helpers
drivers/iio/adc/xilinx-xadc-core.c | 146 +++++++++++++----------------
1 file changed, 65 insertions(+), 81 deletions(-)
--
2.29.1
From: Bartosz Golaszewski <[email protected]>
Further simplify the remove() callback and error paths in probe() by
using the managed variant of request_irq() as well as using a devm action
for cancelling the delayed work at driver detach.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/iio/adc/xilinx-xadc-core.c | 35 ++++++++++++++++++------------
1 file changed, 21 insertions(+), 14 deletions(-)
diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
index e0da6258092c..4440b7a9bd36 100644
--- a/drivers/iio/adc/xilinx-xadc-core.c
+++ b/drivers/iio/adc/xilinx-xadc-core.c
@@ -1192,6 +1192,13 @@ static void xadc_clk_disable_unprepare(void *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;
@@ -1289,14 +1296,19 @@ static int xadc_probe(struct platform_device *pdev)
}
}
- 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)
+ goto err_free_samplerate_trigger;
+
+ ret = devm_add_action_or_reset(dev, xadc_cancel_delayed_work,
+ &xadc->zynq_unmask_work);
if (ret)
goto err_free_samplerate_trigger;
ret = xadc->ops->setup(pdev, indio_dev, xadc->irq);
if (ret)
- goto err_free_irq;
+ goto err_free_samplerate_trigger;
for (i = 0; i < 16; i++)
xadc_read_adc_reg(xadc, XADC_REG_THRESHOLD(i),
@@ -1304,7 +1316,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;
+ goto err_free_samplerate_trigger;
bipolar_mask = 0;
for (i = 0; i < indio_dev->num_channels; i++) {
@@ -1314,17 +1326,17 @@ 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;
+ goto err_free_samplerate_trigger;
ret = xadc_write_adc_reg(xadc, XADC_REG_INPUT_MODE(1),
bipolar_mask >> 16);
if (ret)
- goto err_free_irq;
+ goto err_free_samplerate_trigger;
/* 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;
+ goto err_free_samplerate_trigger;
/* Set thresholds to min/max */
for (i = 0; i < 16; i++) {
@@ -1339,7 +1351,7 @@ 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;
+ goto err_free_samplerate_trigger;
}
/* Go to non-buffered mode */
@@ -1347,15 +1359,12 @@ static int xadc_probe(struct platform_device *pdev)
ret = iio_device_register(indio_dev);
if (ret)
- goto err_free_irq;
+ goto err_free_samplerate_trigger;
platform_set_drvdata(pdev, indio_dev);
return 0;
-err_free_irq:
- free_irq(xadc->irq, indio_dev);
- cancel_delayed_work_sync(&xadc->zynq_unmask_work);
err_free_samplerate_trigger:
if (xadc->ops->flags & XADC_FLAGS_BUFFERED)
iio_trigger_free(xadc->samplerate_trigger);
@@ -1380,8 +1389,6 @@ static int xadc_remove(struct platform_device *pdev)
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);
return 0;
}
--
2.29.1
From: Bartosz Golaszewski <[email protected]>
Shrink the code by using devres variants of all iio routines. This allows
us to entirely drop the remove() callback as well as significantly
simplifies error paths in probe().
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/iio/adc/xilinx-xadc-core.c | 97 ++++++++++--------------------
1 file changed, 32 insertions(+), 65 deletions(-)
diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
index 4440b7a9bd36..c17705205456 100644
--- a/drivers/iio/adc/xilinx-xadc-core.c
+++ b/drivers/iio/adc/xilinx-xadc-core.c
@@ -706,11 +706,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);
@@ -718,15 +719,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)
@@ -1247,39 +1244,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)
- goto err_free_samplerate_trigger;
+ return ret;
/*
* Make sure not to exceed the maximum samplerate since otherwise the
@@ -1288,27 +1281,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 = devm_request_irq(dev, xadc->irq, xadc->ops->interrupt_handler, 0,
dev_name(dev), indio_dev);
if (ret)
- goto err_free_samplerate_trigger;
+ return ret;
ret = devm_add_action_or_reset(dev, xadc_cancel_delayed_work,
&xadc->zynq_unmask_work);
if (ret)
- goto err_free_samplerate_trigger;
+ return ret;
ret = xadc->ops->setup(pdev, indio_dev, xadc->irq);
if (ret)
- goto err_free_samplerate_trigger;
+ return ret;
for (i = 0; i < 16; i++)
xadc_read_adc_reg(xadc, XADC_REG_THRESHOLD(i),
@@ -1316,7 +1310,7 @@ static int xadc_probe(struct platform_device *pdev)
ret = xadc_write_adc_reg(xadc, XADC_REG_CONF0, conf0);
if (ret)
- goto err_free_samplerate_trigger;
+ return ret;
bipolar_mask = 0;
for (i = 0; i < indio_dev->num_channels; i++) {
@@ -1326,17 +1320,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_samplerate_trigger;
+ return ret;
+
ret = xadc_write_adc_reg(xadc, XADC_REG_INPUT_MODE(1),
bipolar_mask >> 16);
if (ret)
- goto err_free_samplerate_trigger;
+ 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_samplerate_trigger;
+ return ret;
/* Set thresholds to min/max */
for (i = 0; i < 16; i++) {
@@ -1351,51 +1346,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_samplerate_trigger;
+ 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_samplerate_trigger;
+ return ret;
platform_set_drvdata(pdev, indio_dev);
- return 0;
-
-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);
- }
-
return 0;
}
static struct platform_driver xadc_driver = {
.probe = xadc_probe,
- .remove = xadc_remove,
.driver = {
.name = "xadc",
.of_match_table = xadc_of_match_table,
--
2.29.1
On Mon, 26 Oct 2020 14:36:08 +0100
Bartosz Golaszewski <[email protected]> wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> Further simplify the remove() callback and error paths in probe() by
> using the managed variant of request_irq() as well as using a devm action
> for cancelling the delayed work at driver detach.
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
Again, this is potentially fine but I'd rather you cleaned up the ordering first
rather than doing things in this order.
The end result of the whole series looks like it will be correct, but that isn't
so obvious for the intermediate patches on their own.
Also, you end up with a lot of noise renaming gotos that then go away at the
end.
Jonathan
> ---
> drivers/iio/adc/xilinx-xadc-core.c | 35 ++++++++++++++++++------------
> 1 file changed, 21 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
> index e0da6258092c..4440b7a9bd36 100644
> --- a/drivers/iio/adc/xilinx-xadc-core.c
> +++ b/drivers/iio/adc/xilinx-xadc-core.c
> @@ -1192,6 +1192,13 @@ static void xadc_clk_disable_unprepare(void *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;
> @@ -1289,14 +1296,19 @@ static int xadc_probe(struct platform_device *pdev)
> }
> }
>
> - 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)
> + goto err_free_samplerate_trigger;
> +
> + ret = devm_add_action_or_reset(dev, xadc_cancel_delayed_work,
> + &xadc->zynq_unmask_work);
> if (ret)
> goto err_free_samplerate_trigger;
>
> ret = xadc->ops->setup(pdev, indio_dev, xadc->irq);
> if (ret)
> - goto err_free_irq;
> + goto err_free_samplerate_trigger;
>
> for (i = 0; i < 16; i++)
> xadc_read_adc_reg(xadc, XADC_REG_THRESHOLD(i),
> @@ -1304,7 +1316,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;
> + goto err_free_samplerate_trigger;
>
> bipolar_mask = 0;
> for (i = 0; i < indio_dev->num_channels; i++) {
> @@ -1314,17 +1326,17 @@ 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;
> + goto err_free_samplerate_trigger;
> ret = xadc_write_adc_reg(xadc, XADC_REG_INPUT_MODE(1),
> bipolar_mask >> 16);
> if (ret)
> - goto err_free_irq;
> + goto err_free_samplerate_trigger;
>
> /* 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;
> + goto err_free_samplerate_trigger;
>
> /* Set thresholds to min/max */
> for (i = 0; i < 16; i++) {
> @@ -1339,7 +1351,7 @@ 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;
> + goto err_free_samplerate_trigger;
> }
>
> /* Go to non-buffered mode */
> @@ -1347,15 +1359,12 @@ static int xadc_probe(struct platform_device *pdev)
>
> ret = iio_device_register(indio_dev);
> if (ret)
> - goto err_free_irq;
> + goto err_free_samplerate_trigger;
>
> platform_set_drvdata(pdev, indio_dev);
>
> return 0;
>
> -err_free_irq:
> - free_irq(xadc->irq, indio_dev);
> - cancel_delayed_work_sync(&xadc->zynq_unmask_work);
> err_free_samplerate_trigger:
> if (xadc->ops->flags & XADC_FLAGS_BUFFERED)
> iio_trigger_free(xadc->samplerate_trigger);
> @@ -1380,8 +1389,6 @@ static int xadc_remove(struct platform_device *pdev)
> 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);
>
> return 0;
> }
On Thu, Oct 29, 2020 at 4:41 PM Jonathan Cameron <[email protected]> wrote:
>
> On Mon, 26 Oct 2020 14:36:08 +0100
> Bartosz Golaszewski <[email protected]> wrote:
>
> > From: Bartosz Golaszewski <[email protected]>
> >
> > Further simplify the remove() callback and error paths in probe() by
> > using the managed variant of request_irq() as well as using a devm action
> > for cancelling the delayed work at driver detach.
> >
> > Signed-off-by: Bartosz Golaszewski <[email protected]>
>
> Again, this is potentially fine but I'd rather you cleaned up the ordering first
> rather than doing things in this order.
>
> The end result of the whole series looks like it will be correct, but that isn't
> so obvious for the intermediate patches on their own.
>
> Also, you end up with a lot of noise renaming gotos that then go away at the
> end.
>
> Jonathan
>
Hi Jonathan,
My two priorities for the ordering of this series were: correct
end-result and not breaking anything on the way. The latter
unfortunately gets in the way of cleaner looking intermediate patches.
I tried to not alter the ordering in which the resources are freed at
any step. As devres release callbacks are called *after* remove() and
in a reverse order to how they were registered, I needed to start from
the bottom of the remove() callback and convert the last operation,
then go upwards from there.
If I tried to do it from the top - I probably could remove labels
earlier and in a cleaner manner but it wouldn't guarantee
bisectability.
Bartosz
On Fri, 30 Oct 2020 11:52:00 +0100
Bartosz Golaszewski <[email protected]> wrote:
> On Thu, Oct 29, 2020 at 4:41 PM Jonathan Cameron <[email protected]> wrote:
> >
> > On Mon, 26 Oct 2020 14:36:08 +0100
> > Bartosz Golaszewski <[email protected]> wrote:
> >
> > > From: Bartosz Golaszewski <[email protected]>
> > >
> > > Further simplify the remove() callback and error paths in probe() by
> > > using the managed variant of request_irq() as well as using a devm action
> > > for cancelling the delayed work at driver detach.
> > >
> > > Signed-off-by: Bartosz Golaszewski <[email protected]>
> >
> > Again, this is potentially fine but I'd rather you cleaned up the ordering first
> > rather than doing things in this order.
> >
> > The end result of the whole series looks like it will be correct, but that isn't
> > so obvious for the intermediate patches on their own.
> >
> > Also, you end up with a lot of noise renaming gotos that then go away at the
> > end.
> >
> > Jonathan
> >
>
> Hi Jonathan,
>
> My two priorities for the ordering of this series were: correct
> end-result and not breaking anything on the way. The latter
> unfortunately gets in the way of cleaner looking intermediate patches.
>
> I tried to not alter the ordering in which the resources are freed at
> any step. As devres release callbacks are called *after* remove() and
> in a reverse order to how they were registered, I needed to start from
> the bottom of the remove() callback and convert the last operation,
> then go upwards from there.
>
> If I tried to do it from the top - I probably could remove labels
> earlier and in a cleaner manner but it wouldn't guarantee
> bisectability.
>
Maybe best plan is to squash last 3 patches into one?
I suspect that's going to be easier to review.
Jonathan
> Bartosz
On Sat, Oct 31, 2020 at 12:10 PM Jonathan Cameron <[email protected]> wrote:
>
[snip]
> >
> > Hi Jonathan,
> >
> > My two priorities for the ordering of this series were: correct
> > end-result and not breaking anything on the way. The latter
> > unfortunately gets in the way of cleaner looking intermediate patches.
> >
> > I tried to not alter the ordering in which the resources are freed at
> > any step. As devres release callbacks are called *after* remove() and
> > in a reverse order to how they were registered, I needed to start from
> > the bottom of the remove() callback and convert the last operation,
> > then go upwards from there.
> >
> > If I tried to do it from the top - I probably could remove labels
> > earlier and in a cleaner manner but it wouldn't guarantee
> > bisectability.
> >
>
> Maybe best plan is to squash last 3 patches into one?
>
> I suspect that's going to be easier to review.
>
> Jonathan
>
Sure I can do this.
Bartosz