* [PATCH v6 0/2] staging: iio: adc: ad7280a: use devm API when applicable
@ 2018-11-11 15:59 Slawomir Stepien
2018-11-11 15:59 ` [PATCH v6 1/2] staging: iio: adc: ad7280a: power down the device on error in probe Slawomir Stepien
2018-11-11 15:59 ` [PATCH v6 2/2] staging: iio: adc: ad7280a: use devm_* APIs Slawomir Stepien
0 siblings, 2 replies; 6+ messages in thread
From: Slawomir Stepien @ 2018-11-11 15:59 UTC (permalink / raw)
To: lars, Michael.Hennerich, jic23, knaack.h, pmeerw; +Cc: linux-iio, gregkh
This small series of patches will enable devm API in the driver when applicable.
The patch contains two parts: 1st part enables powering down the device in
probe function, 2nd part changes the API to devm when applicable.
The first commit has been added for better visualization of what will happen
when the devm API is used - the whole remove function is not needed, but we
still need to power down the device from somewhere.
History:
Since v5:
* split the patch into two commits: (1) power down the device in probe, (2) use
devm API when applicable
* devm_add_action -> devm_add_action_or_reset
Since v4:
* on devm_add_action fail, call the action on error handling
* move the devm_add_action just after spi_setup - this will call the action on
more error paths in probe (fail in: ad7280_chain_setup, ad7280_channel_init,
ad7280_attr_init)
Since v3:
* use devm_add_action with software power down
* the whole remove call back is not needed anymore
Since v2:
* iio_device_register -> devm_iio_device_register
Since v1:
* request_threaded_irq -> devm_request_threaded_irq
Slawomir Stepien (2):
staging: iio: adc: ad7280a: power down the device on error in probe
staging: iio: adc: ad7280a: use devm_* APIs
drivers/staging/iio/adc/ad7280a.c | 95 +++++++++++++------------------
1 file changed, 39 insertions(+), 56 deletions(-)
--
2.19.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v6 1/2] staging: iio: adc: ad7280a: power down the device on error in probe
2018-11-11 15:59 [PATCH v6 0/2] staging: iio: adc: ad7280a: use devm API when applicable Slawomir Stepien
@ 2018-11-11 15:59 ` Slawomir Stepien
2018-11-11 17:00 ` Jonathan Cameron
2018-11-11 15:59 ` [PATCH v6 2/2] staging: iio: adc: ad7280a: use devm_* APIs Slawomir Stepien
1 sibling, 1 reply; 6+ messages in thread
From: Slawomir Stepien @ 2018-11-11 15:59 UTC (permalink / raw)
To: lars, Michael.Hennerich, jic23, knaack.h, pmeerw; +Cc: linux-iio, gregkh
Power down the device if anything goes wrong after the SPI has been
setup correctly in the probe function.
Existing code that toggles the AD7280A_CTRL_LB_SWRST bit inside
ad7280_chain_setup function is responsible for powering up the device.
Signed-off-by: Slawomir Stepien <sst@poczta.fm>
---
drivers/staging/iio/adc/ad7280a.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c
index 58420dcb406d..3ab06fd87675 100644
--- a/drivers/staging/iio/adc/ad7280a.c
+++ b/drivers/staging/iio/adc/ad7280a.c
@@ -870,7 +870,7 @@ static int ad7280_probe(struct spi_device *spi)
ret = ad7280_chain_setup(st);
if (ret < 0)
- return ret;
+ goto error_power_down;
st->slave_num = ret;
st->scan_cnt = (st->slave_num + 1) * AD7280A_NUM_CH;
@@ -901,7 +901,7 @@ static int ad7280_probe(struct spi_device *spi)
ret = ad7280_channel_init(st);
if (ret < 0)
- return ret;
+ goto error_power_down;
indio_dev->num_channels = ret;
indio_dev->channels = st->channels;
@@ -950,6 +950,9 @@ static int ad7280_probe(struct spi_device *spi)
error_free_channels:
kfree(st->channels);
+error_power_down:
+ ad7280_write(st, AD7280A_DEVADDR_MASTER, AD7280A_CONTROL_HB, 1,
+ AD7280A_CTRL_HB_PWRDN_SW | st->ctrl_hb);
return ret;
}
--
2.19.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v6 2/2] staging: iio: adc: ad7280a: use devm_* APIs
2018-11-11 15:59 [PATCH v6 0/2] staging: iio: adc: ad7280a: use devm API when applicable Slawomir Stepien
2018-11-11 15:59 ` [PATCH v6 1/2] staging: iio: adc: ad7280a: power down the device on error in probe Slawomir Stepien
@ 2018-11-11 15:59 ` Slawomir Stepien
2018-11-11 17:04 ` Jonathan Cameron
1 sibling, 1 reply; 6+ messages in thread
From: Slawomir Stepien @ 2018-11-11 15:59 UTC (permalink / raw)
To: lars, Michael.Hennerich, jic23, knaack.h, pmeerw; +Cc: linux-iio, gregkh
devm_* APIs are device managed and make code simpler.
Signed-off-by: Slawomir Stepien <sst@poczta.fm>
---
drivers/staging/iio/adc/ad7280a.c | 102 ++++++++++++------------------
1 file changed, 41 insertions(+), 61 deletions(-)
diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c
index 3ab06fd87675..327d3c96e83f 100644
--- a/drivers/staging/iio/adc/ad7280a.c
+++ b/drivers/staging/iio/adc/ad7280a.c
@@ -342,6 +342,14 @@ static int ad7280_read_all_channels(struct ad7280_state *st, unsigned int cnt,
return sum;
}
+static void ad7280_sw_power_down(void *data)
+{
+ struct ad7280_state *st = data;
+
+ ad7280_write(st, AD7280A_DEVADDR_MASTER, AD7280A_CONTROL_HB, 1,
+ AD7280A_CTRL_HB_PWRDN_SW | st->ctrl_hb);
+}
+
static int ad7280_chain_setup(struct ad7280_state *st)
{
unsigned int val, n;
@@ -492,8 +500,8 @@ static int ad7280_channel_init(struct ad7280_state *st)
{
int dev, ch, cnt;
- st->channels = kcalloc((st->slave_num + 1) * 12 + 2,
- sizeof(*st->channels), GFP_KERNEL);
+ st->channels = devm_kcalloc(&st->spi->dev, (st->slave_num + 1) * 12 + 2,
+ sizeof(*st->channels), GFP_KERNEL);
if (!st->channels)
return -ENOMEM;
@@ -552,16 +560,18 @@ static int ad7280_channel_init(struct ad7280_state *st)
static int ad7280_attr_init(struct ad7280_state *st)
{
int dev, ch, cnt;
+ unsigned int index;
- st->iio_attr = kcalloc(2, sizeof(*st->iio_attr) *
- (st->slave_num + 1) * AD7280A_CELLS_PER_DEV,
- GFP_KERNEL);
+ st->iio_attr = devm_kcalloc(&st->spi->dev, 2, sizeof(*st->iio_attr) *
+ (st->slave_num + 1) * AD7280A_CELLS_PER_DEV,
+ GFP_KERNEL);
if (!st->iio_attr)
return -ENOMEM;
for (dev = 0, cnt = 0; dev <= st->slave_num; dev++)
for (ch = AD7280A_CELL_VOLTAGE_1; ch <= AD7280A_CELL_VOLTAGE_6;
ch++, cnt++) {
+ index = dev * AD7280A_CELLS_PER_DEV + ch;
st->iio_attr[cnt].address =
ad7280a_devaddr(dev) << 8 | ch;
st->iio_attr[cnt].dev_attr.attr.mode =
@@ -571,10 +581,9 @@ static int ad7280_attr_init(struct ad7280_state *st)
st->iio_attr[cnt].dev_attr.store =
ad7280_store_balance_sw;
st->iio_attr[cnt].dev_attr.attr.name =
- kasprintf(GFP_KERNEL,
- "in%d-in%d_balance_switch_en",
- dev * AD7280A_CELLS_PER_DEV + ch,
- dev * AD7280A_CELLS_PER_DEV + ch + 1);
+ devm_kasprintf(&st->spi->dev, GFP_KERNEL,
+ "in%d-in%d_balance_switch_en",
+ index, index + 1);
ad7280_attributes[cnt] =
&st->iio_attr[cnt].dev_attr.attr;
cnt++;
@@ -588,10 +597,9 @@ static int ad7280_attr_init(struct ad7280_state *st)
st->iio_attr[cnt].dev_attr.store =
ad7280_store_balance_timer;
st->iio_attr[cnt].dev_attr.attr.name =
- kasprintf(GFP_KERNEL,
- "in%d-in%d_balance_timer",
- dev * AD7280A_CELLS_PER_DEV + ch,
- dev * AD7280A_CELLS_PER_DEV + ch + 1);
+ devm_kasprintf(&st->spi->dev, GFP_KERNEL,
+ "in%d-in%d_balance_timer",
+ index, index + 1);
ad7280_attributes[cnt] =
&st->iio_attr[cnt].dev_attr.attr;
}
@@ -863,6 +871,10 @@ static int ad7280_probe(struct spi_device *spi)
st->spi->mode = SPI_MODE_1;
spi_setup(st->spi);
+ ret = devm_add_action_or_reset(&spi->dev, ad7280_sw_power_down, st);
+ if (ret)
+ return ret;
+
st->ctrl_lb = AD7280A_CTRL_LB_ACQ_TIME(pdata->acquisition_time & 0x3);
st->ctrl_hb = AD7280A_CTRL_HB_CONV_AVG(pdata->conversion_averaging
& 0x3) | (pdata->thermistor_term_en ?
@@ -870,7 +882,7 @@ static int ad7280_probe(struct spi_device *spi)
ret = ad7280_chain_setup(st);
if (ret < 0)
- goto error_power_down;
+ return ret;
st->slave_num = ret;
st->scan_cnt = (st->slave_num + 1) * AD7280A_NUM_CH;
@@ -901,7 +913,7 @@ static int ad7280_probe(struct spi_device *spi)
ret = ad7280_channel_init(st);
if (ret < 0)
- goto error_power_down;
+ return ret;
indio_dev->num_channels = ret;
indio_dev->channels = st->channels;
@@ -909,68 +921,37 @@ static int ad7280_probe(struct spi_device *spi)
ret = ad7280_attr_init(st);
if (ret < 0)
- goto error_free_channels;
+ return ret;
- ret = iio_device_register(indio_dev);
+ ret = devm_iio_device_register(&spi->dev, indio_dev);
if (ret)
- goto error_free_attr;
+ return ret;
if (spi->irq > 0) {
ret = ad7280_write(st, AD7280A_DEVADDR_MASTER,
AD7280A_ALERT, 1,
AD7280A_ALERT_RELAY_SIG_CHAIN_DOWN);
if (ret)
- goto error_unregister;
+ return ret;
ret = ad7280_write(st, ad7280a_devaddr(st->slave_num),
AD7280A_ALERT, 0,
AD7280A_ALERT_GEN_STATIC_HIGH |
(pdata->chain_last_alert_ignore & 0xF));
if (ret)
- goto error_unregister;
-
- ret = request_threaded_irq(spi->irq,
- NULL,
- ad7280_event_handler,
- IRQF_TRIGGER_FALLING |
- IRQF_ONESHOT,
- indio_dev->name,
- indio_dev);
+ return ret;
+
+ ret = devm_request_threaded_irq(&spi->dev, spi->irq,
+ NULL,
+ ad7280_event_handler,
+ IRQF_TRIGGER_FALLING |
+ IRQF_ONESHOT,
+ indio_dev->name,
+ indio_dev);
if (ret)
- goto error_unregister;
+ return ret;
}
- return 0;
-error_unregister:
- iio_device_unregister(indio_dev);
-
-error_free_attr:
- kfree(st->iio_attr);
-
-error_free_channels:
- kfree(st->channels);
-
-error_power_down:
- ad7280_write(st, AD7280A_DEVADDR_MASTER, AD7280A_CONTROL_HB, 1,
- AD7280A_CTRL_HB_PWRDN_SW | st->ctrl_hb);
- return ret;
-}
-
-static int ad7280_remove(struct spi_device *spi)
-{
- struct iio_dev *indio_dev = spi_get_drvdata(spi);
- struct ad7280_state *st = iio_priv(indio_dev);
-
- if (spi->irq > 0)
- free_irq(spi->irq, indio_dev);
- iio_device_unregister(indio_dev);
-
- ad7280_write(st, AD7280A_DEVADDR_MASTER, AD7280A_CONTROL_HB, 1,
- AD7280A_CTRL_HB_PWRDN_SW | st->ctrl_hb);
-
- kfree(st->channels);
- kfree(st->iio_attr);
-
return 0;
}
@@ -985,7 +966,6 @@ static struct spi_driver ad7280_driver = {
.name = "ad7280",
},
.probe = ad7280_probe,
- .remove = ad7280_remove,
.id_table = ad7280_id,
};
module_spi_driver(ad7280_driver);
--
2.19.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v6 1/2] staging: iio: adc: ad7280a: power down the device on error in probe
2018-11-11 15:59 ` [PATCH v6 1/2] staging: iio: adc: ad7280a: power down the device on error in probe Slawomir Stepien
@ 2018-11-11 17:00 ` Jonathan Cameron
0 siblings, 0 replies; 6+ messages in thread
From: Jonathan Cameron @ 2018-11-11 17:00 UTC (permalink / raw)
To: Slawomir Stepien
Cc: lars, Michael.Hennerich, knaack.h, pmeerw, linux-iio, gregkh
On Sun, 11 Nov 2018 16:59:10 +0100
Slawomir Stepien <sst@poczta.fm> wrote:
> Power down the device if anything goes wrong after the SPI has been
> setup correctly in the probe function.
>
> Existing code that toggles the AD7280A_CTRL_LB_SWRST bit inside
> ad7280_chain_setup function is responsible for powering up the device.
>
> Signed-off-by: Slawomir Stepien <sst@poczta.fm>
Hi Slawomir,
Nearly perfect... But see inline...
> ---
> drivers/staging/iio/adc/ad7280a.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c
> index 58420dcb406d..3ab06fd87675 100644
> --- a/drivers/staging/iio/adc/ad7280a.c
> +++ b/drivers/staging/iio/adc/ad7280a.c
> @@ -870,7 +870,7 @@ static int ad7280_probe(struct spi_device *spi)
>
> ret = ad7280_chain_setup(st);
> if (ret < 0)
> - return ret;
> + goto error_power_down;
The ideal is always for a function to unwind on error to the point where
it leaves no visible side effects. As such we should power down 'inside'
chain_setup on error (unfortunately there are a lot exit paths in that
function so the resulting patch will be larger than this :(
This may seem a pain to do this way, but it makes the code flow
more obvious generally making things cleaner.
Rather than go around again for such a trivial thing, I've
made the change and pushed out the updated patch to the togreg
branch of iio.git as testing for the autobuilders to see if
I messed it up!
Note this will move the resulting devm call as well. I'll do that for
patch 2.
Jonathan
>
> st->slave_num = ret;
> st->scan_cnt = (st->slave_num + 1) * AD7280A_NUM_CH;
> @@ -901,7 +901,7 @@ static int ad7280_probe(struct spi_device *spi)
>
> ret = ad7280_channel_init(st);
> if (ret < 0)
> - return ret;
> + goto error_power_down;
>
> indio_dev->num_channels = ret;
> indio_dev->channels = st->channels;
> @@ -950,6 +950,9 @@ static int ad7280_probe(struct spi_device *spi)
> error_free_channels:
> kfree(st->channels);
>
> +error_power_down:
> + ad7280_write(st, AD7280A_DEVADDR_MASTER, AD7280A_CONTROL_HB, 1,
> + AD7280A_CTRL_HB_PWRDN_SW | st->ctrl_hb);
> return ret;
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v6 2/2] staging: iio: adc: ad7280a: use devm_* APIs
2018-11-11 15:59 ` [PATCH v6 2/2] staging: iio: adc: ad7280a: use devm_* APIs Slawomir Stepien
@ 2018-11-11 17:04 ` Jonathan Cameron
2018-11-11 20:23 ` Slawomir Stepien
0 siblings, 1 reply; 6+ messages in thread
From: Jonathan Cameron @ 2018-11-11 17:04 UTC (permalink / raw)
To: Slawomir Stepien
Cc: lars, Michael.Hennerich, knaack.h, pmeerw, linux-iio, gregkh
On Sun, 11 Nov 2018 16:59:11 +0100
Slawomir Stepien <sst@poczta.fm> wrote:
> devm_* APIs are device managed and make code simpler.
>
> Signed-off-by: Slawomir Stepien <sst@poczta.fm>
Hi Slawomir,
I made the minor mod below to align this with the change I made in patch 1.
It does close a very small window where you might unnecessary power down
if the first command in the chain_setup function resulted in an error
so the reset may never have been issued. Of course you can't actually
tell where the error occurred in that spi write. We'll just take
the view it's such a minor risk that we can assume the spi write has
no side effects on failure.
Thanks for persisting with this. It looks like a good result to me.
Thanks,
Jonathan
> ---
> drivers/staging/iio/adc/ad7280a.c | 102 ++++++++++++------------------
> 1 file changed, 41 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c
> index 3ab06fd87675..327d3c96e83f 100644
> --- a/drivers/staging/iio/adc/ad7280a.c
> +++ b/drivers/staging/iio/adc/ad7280a.c
> @@ -342,6 +342,14 @@ static int ad7280_read_all_channels(struct ad7280_state *st, unsigned int cnt,
> return sum;
> }
>
> +static void ad7280_sw_power_down(void *data)
> +{
> + struct ad7280_state *st = data;
> +
> + ad7280_write(st, AD7280A_DEVADDR_MASTER, AD7280A_CONTROL_HB, 1,
> + AD7280A_CTRL_HB_PWRDN_SW | st->ctrl_hb);
> +}
> +
> static int ad7280_chain_setup(struct ad7280_state *st)
> {
> unsigned int val, n;
> @@ -492,8 +500,8 @@ static int ad7280_channel_init(struct ad7280_state *st)
> {
> int dev, ch, cnt;
>
> - st->channels = kcalloc((st->slave_num + 1) * 12 + 2,
> - sizeof(*st->channels), GFP_KERNEL);
> + st->channels = devm_kcalloc(&st->spi->dev, (st->slave_num + 1) * 12 + 2,
> + sizeof(*st->channels), GFP_KERNEL);
> if (!st->channels)
> return -ENOMEM;
>
> @@ -552,16 +560,18 @@ static int ad7280_channel_init(struct ad7280_state *st)
> static int ad7280_attr_init(struct ad7280_state *st)
> {
> int dev, ch, cnt;
> + unsigned int index;
>
> - st->iio_attr = kcalloc(2, sizeof(*st->iio_attr) *
> - (st->slave_num + 1) * AD7280A_CELLS_PER_DEV,
> - GFP_KERNEL);
> + st->iio_attr = devm_kcalloc(&st->spi->dev, 2, sizeof(*st->iio_attr) *
> + (st->slave_num + 1) * AD7280A_CELLS_PER_DEV,
> + GFP_KERNEL);
> if (!st->iio_attr)
> return -ENOMEM;
>
> for (dev = 0, cnt = 0; dev <= st->slave_num; dev++)
> for (ch = AD7280A_CELL_VOLTAGE_1; ch <= AD7280A_CELL_VOLTAGE_6;
> ch++, cnt++) {
> + index = dev * AD7280A_CELLS_PER_DEV + ch;
> st->iio_attr[cnt].address =
> ad7280a_devaddr(dev) << 8 | ch;
> st->iio_attr[cnt].dev_attr.attr.mode =
> @@ -571,10 +581,9 @@ static int ad7280_attr_init(struct ad7280_state *st)
> st->iio_attr[cnt].dev_attr.store =
> ad7280_store_balance_sw;
> st->iio_attr[cnt].dev_attr.attr.name =
> - kasprintf(GFP_KERNEL,
> - "in%d-in%d_balance_switch_en",
> - dev * AD7280A_CELLS_PER_DEV + ch,
> - dev * AD7280A_CELLS_PER_DEV + ch + 1);
> + devm_kasprintf(&st->spi->dev, GFP_KERNEL,
> + "in%d-in%d_balance_switch_en",
> + index, index + 1);
> ad7280_attributes[cnt] =
> &st->iio_attr[cnt].dev_attr.attr;
> cnt++;
> @@ -588,10 +597,9 @@ static int ad7280_attr_init(struct ad7280_state *st)
> st->iio_attr[cnt].dev_attr.store =
> ad7280_store_balance_timer;
> st->iio_attr[cnt].dev_attr.attr.name =
> - kasprintf(GFP_KERNEL,
> - "in%d-in%d_balance_timer",
> - dev * AD7280A_CELLS_PER_DEV + ch,
> - dev * AD7280A_CELLS_PER_DEV + ch + 1);
> + devm_kasprintf(&st->spi->dev, GFP_KERNEL,
> + "in%d-in%d_balance_timer",
> + index, index + 1);
> ad7280_attributes[cnt] =
> &st->iio_attr[cnt].dev_attr.attr;
> }
> @@ -863,6 +871,10 @@ static int ad7280_probe(struct spi_device *spi)
> st->spi->mode = SPI_MODE_1;
> spi_setup(st->spi);
>
> + ret = devm_add_action_or_reset(&spi->dev, ad7280_sw_power_down, st);
> + if (ret)
> + return ret;
> +
> st->ctrl_lb = AD7280A_CTRL_LB_ACQ_TIME(pdata->acquisition_time & 0x3);
> st->ctrl_hb = AD7280A_CTRL_HB_CONV_AVG(pdata->conversion_averaging
> & 0x3) | (pdata->thermistor_term_en ?
> @@ -870,7 +882,7 @@ static int ad7280_probe(struct spi_device *spi)
>
> ret = ad7280_chain_setup(st);
> if (ret < 0)
> - goto error_power_down;
> + return ret;
>
> st->slave_num = ret;
> st->scan_cnt = (st->slave_num + 1) * AD7280A_NUM_CH;
> @@ -901,7 +913,7 @@ static int ad7280_probe(struct spi_device *spi)
>
> ret = ad7280_channel_init(st);
> if (ret < 0)
> - goto error_power_down;
> + return ret;
>
> indio_dev->num_channels = ret;
> indio_dev->channels = st->channels;
> @@ -909,68 +921,37 @@ static int ad7280_probe(struct spi_device *spi)
>
> ret = ad7280_attr_init(st);
> if (ret < 0)
> - goto error_free_channels;
> + return ret;
>
> - ret = iio_device_register(indio_dev);
> + ret = devm_iio_device_register(&spi->dev, indio_dev);
> if (ret)
> - goto error_free_attr;
> + return ret;
>
> if (spi->irq > 0) {
> ret = ad7280_write(st, AD7280A_DEVADDR_MASTER,
> AD7280A_ALERT, 1,
> AD7280A_ALERT_RELAY_SIG_CHAIN_DOWN);
> if (ret)
> - goto error_unregister;
> + return ret;
>
> ret = ad7280_write(st, ad7280a_devaddr(st->slave_num),
> AD7280A_ALERT, 0,
> AD7280A_ALERT_GEN_STATIC_HIGH |
> (pdata->chain_last_alert_ignore & 0xF));
> if (ret)
> - goto error_unregister;
> -
> - ret = request_threaded_irq(spi->irq,
> - NULL,
> - ad7280_event_handler,
> - IRQF_TRIGGER_FALLING |
> - IRQF_ONESHOT,
> - indio_dev->name,
> - indio_dev);
> + return ret;
> +
> + ret = devm_request_threaded_irq(&spi->dev, spi->irq,
> + NULL,
> + ad7280_event_handler,
> + IRQF_TRIGGER_FALLING |
> + IRQF_ONESHOT,
> + indio_dev->name,
> + indio_dev);
> if (ret)
> - goto error_unregister;
> + return ret;
> }
>
> - return 0;
> -error_unregister:
> - iio_device_unregister(indio_dev);
> -
> -error_free_attr:
> - kfree(st->iio_attr);
> -
> -error_free_channels:
> - kfree(st->channels);
> -
> -error_power_down:
> - ad7280_write(st, AD7280A_DEVADDR_MASTER, AD7280A_CONTROL_HB, 1,
> - AD7280A_CTRL_HB_PWRDN_SW | st->ctrl_hb);
> - return ret;
> -}
> -
> -static int ad7280_remove(struct spi_device *spi)
> -{
> - struct iio_dev *indio_dev = spi_get_drvdata(spi);
> - struct ad7280_state *st = iio_priv(indio_dev);
> -
> - if (spi->irq > 0)
> - free_irq(spi->irq, indio_dev);
> - iio_device_unregister(indio_dev);
> -
> - ad7280_write(st, AD7280A_DEVADDR_MASTER, AD7280A_CONTROL_HB, 1,
> - AD7280A_CTRL_HB_PWRDN_SW | st->ctrl_hb);
> -
> - kfree(st->channels);
> - kfree(st->iio_attr);
> -
> return 0;
> }
>
> @@ -985,7 +966,6 @@ static struct spi_driver ad7280_driver = {
> .name = "ad7280",
> },
> .probe = ad7280_probe,
> - .remove = ad7280_remove,
> .id_table = ad7280_id,
> };
> module_spi_driver(ad7280_driver);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v6 2/2] staging: iio: adc: ad7280a: use devm_* APIs
2018-11-11 17:04 ` Jonathan Cameron
@ 2018-11-11 20:23 ` Slawomir Stepien
0 siblings, 0 replies; 6+ messages in thread
From: Slawomir Stepien @ 2018-11-11 20:23 UTC (permalink / raw)
To: Jonathan Cameron
Cc: lars, Michael.Hennerich, knaack.h, pmeerw, linux-iio, gregkh
On lis 11, 2018 17:04, Jonathan Cameron wrote:
> On Sun, 11 Nov 2018 16:59:11 +0100
> Slawomir Stepien <sst@poczta.fm> wrote:
>
> > devm_* APIs are device managed and make code simpler.
> >
> > Signed-off-by: Slawomir Stepien <sst@poczta.fm>
> Hi Slawomir,
>
> I made the minor mod below to align this with the change I made in patch 1.
> It does close a very small window where you might unnecessary power down
> if the first command in the chain_setup function resulted in an error
> so the reset may never have been issued. Of course you can't actually
> tell where the error occurred in that spi write. We'll just take
> the view it's such a minor risk that we can assume the spi write has
> no side effects on failure.
Got your point here. Checked the final change and it looks OK.
Thank you for changing that and picking it.
> Thanks for persisting with this. It looks like a good result to me.
Thank you for the persisting reviews ;)
--
Slawomir Stepien
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-11-12 6:12 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-11-11 15:59 [PATCH v6 0/2] staging: iio: adc: ad7280a: use devm API when applicable Slawomir Stepien
2018-11-11 15:59 ` [PATCH v6 1/2] staging: iio: adc: ad7280a: power down the device on error in probe Slawomir Stepien
2018-11-11 17:00 ` Jonathan Cameron
2018-11-11 15:59 ` [PATCH v6 2/2] staging: iio: adc: ad7280a: use devm_* APIs Slawomir Stepien
2018-11-11 17:04 ` Jonathan Cameron
2018-11-11 20:23 ` Slawomir Stepien
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).