| Message ID | 20181104104900.13997-1-sst@poczta.fm (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5,1/1] staging: iio: adc: ad7280a: use devm_* APIs | expand |
On Sun, Nov 04, 2018 at 11:49:00AM +0100, Slawomir Stepien wrote: > devm_* APIs are device managed and make code simpler. > > Signed-off-by: Slawomir Stepien <sst@poczta.fm> > --- [] > Since v4: > * on devm_add_action fail, call the action on error handling For such a use case, `devm_add_action_or_reset()` seems like a better candidate. > + ret = devm_add_action(&spi->dev, ad7280_sw_power_down, st); > + if (ret) { > + ad7280_sw_power_down(st); > + return ret; > + } Also, you can suppress the `1/1` in sunject [PATCH v5 1/1] by passing `-N` flag to `git format-patch`.
On Sun, 4 Nov 2018 20:24:56 +0530 Himanshu Jha <himanshujha199640@gmail.com> wrote: > On Sun, Nov 04, 2018 at 11:49:00AM +0100, Slawomir Stepien wrote: > > devm_* APIs are device managed and make code simpler. > > > > Signed-off-by: Slawomir Stepien <sst@poczta.fm> > > --- > > [] > > > Since v4: > > * on devm_add_action fail, call the action on error handling > > For such a use case, `devm_add_action_or_reset()` seems like a better > candidate. Cool. Somehow I'd missed the existence of that. Always thought such a function would be handy but never actually checked if there was one ;) Doh and thanks for the pointer. Easy task for anyone who wants it is to see where else this is directly applicable in IIO drivers. The odd bit here is that I'm not entirely sure what 'power up' action this power down is undoing, so not sure where exactly it should be. It may just be a catch all for the device being left powered up after a read sometime earlier. If that's the case I would suggest a comment making that clear and do it only just before the devm_iio_device_register (as we don't power up anywhere in probe that I can see and this is the point at which a power up 'might' occur as the interfaces are exposed. Jonathan Jonathan > > > + ret = devm_add_action(&spi->dev, ad7280_sw_power_down, st); > > + if (ret) { > > + ad7280_sw_power_down(st); > > + return ret; > > + } > > Also, you can suppress the `1/1` in sunject [PATCH v5 1/1] by passing > `-N` flag to `git format-patch`. > >
On lis 04, 2018 20:24, Himanshu Jha wrote: > On Sun, Nov 04, 2018 at 11:49:00AM +0100, Slawomir Stepien wrote: > > devm_* APIs are device managed and make code simpler. > > > > Signed-off-by: Slawomir Stepien <sst@poczta.fm> > > --- > > [] > > > Since v4: > > * on devm_add_action fail, call the action on error handling > > For such a use case, `devm_add_action_or_reset()` seems like a better > candidate. Thank you for that. > > + ret = devm_add_action(&spi->dev, ad7280_sw_power_down, st); > > + if (ret) { > > + ad7280_sw_power_down(st); > > + return ret; > > + } > > Also, you can suppress the `1/1` in sunject [PATCH v5 1/1] by passing > `-N` flag to `git format-patch`. I haven't even notice that 1/1. I will use -N when sending only one patch. Thanks!
On lis 04, 2018 16:33, Jonathan Cameron wrote: > The odd bit here is that I'm not entirely sure what 'power up' action > this power down is undoing, so not sure where exactly it should be. > > It may just be a catch all for the device being left powered up after > a read sometime earlier. If that's the case I would suggest a comment > making that clear and do it only just before the devm_iio_device_register > (as we don't power up anywhere in probe that I can see and this is the > point at which a power up 'might' occur as the interfaces are exposed. Inside the ad7280_chain_setup(), the first write to the device(s) is with AD7280A_CTRL_LB_SWRST bit. The next write is de-asserting this bit. Based on datasheet, such two writes will reset the upper part of the control register to its default state, that means, the device will left the software power down state. So inside ad7280_chain_setup() we have the power up you are talking about. That is why I think that action that will put the device into software power down should be after spi_setup(), but before ad7280_chain_setup() - and this is the current form (of course I will use the ...or_reset() variant as Jha pointed out in next patch's version). What do you think?
On Fri, 9 Nov 2018 17:59:04 +0100 Slawomir Stepien <sst@poczta.fm> wrote: > On lis 04, 2018 16:33, Jonathan Cameron wrote: > > The odd bit here is that I'm not entirely sure what 'power up' action > > this power down is undoing, so not sure where exactly it should be. > > > > It may just be a catch all for the device being left powered up after > > a read sometime earlier. If that's the case I would suggest a comment > > making that clear and do it only just before the devm_iio_device_register > > (as we don't power up anywhere in probe that I can see and this is the > > point at which a power up 'might' occur as the interfaces are exposed. > > Inside the ad7280_chain_setup(), the first write to the device(s) is with > AD7280A_CTRL_LB_SWRST bit. The next write is de-asserting this bit. Based on > datasheet, such two writes will reset the upper part of the control register to > its default state, that means, the device will left the software power down > state. > > So inside ad7280_chain_setup() we have the power up you are talking about. > That is why I think that action that will put the device into software power > down should be after spi_setup(), but before ad7280_chain_setup() - and this is > the current form (of course I will use the ...or_reset() variant as Jha pointed > out in next patch's version). Good explanation. I would just add a brief version of this to the code so it's easy to follow for anyone looking at it in the future. Thanks, Jonathan > > What do you think? >
On lis 11, 2018 15:01, Jonathan Cameron wrote: > On Fri, 9 Nov 2018 17:59:04 +0100 > Slawomir Stepien <sst@poczta.fm> wrote: > > > On lis 04, 2018 16:33, Jonathan Cameron wrote: > > > The odd bit here is that I'm not entirely sure what 'power up' action > > > this power down is undoing, so not sure where exactly it should be. > > > > > > It may just be a catch all for the device being left powered up after > > > a read sometime earlier. If that's the case I would suggest a comment > > > making that clear and do it only just before the devm_iio_device_register > > > (as we don't power up anywhere in probe that I can see and this is the > > > point at which a power up 'might' occur as the interfaces are exposed. > > > > Inside the ad7280_chain_setup(), the first write to the device(s) is with > > AD7280A_CTRL_LB_SWRST bit. The next write is de-asserting this bit. Based on > > datasheet, such two writes will reset the upper part of the control register to > > its default state, that means, the device will left the software power down > > state. > > > > So inside ad7280_chain_setup() we have the power up you are talking about. > > That is why I think that action that will put the device into software power > > down should be after spi_setup(), but before ad7280_chain_setup() - and this is > > the current form (of course I will use the ...or_reset() variant as Jha pointed > > out in next patch's version). > Good explanation. I would just add a brief version of this to the code > so it's easy to follow for anyone looking at it in the future. Sending v6 (now in the form of two patches), so we can have a discussion there. Thank you.
diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c index 58420dcb406d..0d82be30a5de 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,12 @@ static int ad7280_probe(struct spi_device *spi) st->spi->mode = SPI_MODE_1; spi_setup(st->spi); + ret = devm_add_action(&spi->dev, ad7280_sw_power_down, st); + if (ret) { + ad7280_sw_power_down(st); + 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 ? @@ -909,65 +923,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); - - 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; } @@ -982,7 +968,6 @@ static struct spi_driver ad7280_driver = { .name = "ad7280", }, .probe = ad7280_probe, - .remove = ad7280_remove, .id_table = ad7280_id, }; module_spi_driver(ad7280_driver);
devm_* APIs are device managed and make code simpler. Signed-off-by: Slawomir Stepien <sst@poczta.fm> --- 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 --- drivers/staging/iio/adc/ad7280a.c | 97 +++++++++++++------------------ 1 file changed, 41 insertions(+), 56 deletions(-)