diff mbox series

[v6,2/2] staging: iio: adc: ad7280a: use devm_* APIs

Message ID 20181111155911.3604-3-sst@poczta.fm (mailing list archive)
State New, archived
Headers show
Series staging: iio: adc: ad7280a: use devm API when applicable | expand

Commit Message

Slawomir Stepien Nov. 11, 2018, 3:59 p.m. UTC 
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(-)

Comments

Jonathan Cameron Nov. 11, 2018, 5:04 p.m. UTC | #1 
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);
Slawomir Stepien Nov. 11, 2018, 8:23 p.m. UTC | #2 
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 ;)
diff mbox series

Patch

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