[PATCH v5 1/1] staging: iio: adc: ad7280a: use devm_* APIs

[PATCH v5 1/1] staging: iio: adc: ad7280a: use devm_* APIs

[Slawomir Stepien]

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(-)

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);
-- 
2.19.1

Re: [PATCH v5 1/1] staging: iio: adc: ad7280a: use devm_* APIs

[Himanshu Jha]

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`.


-- 
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology

Re: [PATCH v5 1/1] staging: iio: adc: ad7280a: use devm_* APIs

[Jonathan Cameron]

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`.
> 
> 

Re: [PATCH v5 1/1] staging: iio: adc: ad7280a: use devm_* APIs

[Slawomir Stepien]

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!

-- 
Slawomir Stepien

Re: [PATCH v5 1/1] staging: iio: adc: ad7280a: use devm_* APIs

[Slawomir Stepien]

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?

-- 
Slawomir Stepien

Re: [PATCH v5 1/1] staging: iio: adc: ad7280a: use devm_* APIs

[Jonathan Cameron]

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?
> 

Re: [PATCH v5 1/1] staging: iio: adc: ad7280a: use devm_* APIs

[Slawomir Stepien]

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.

-- 
Slawomir Stepien