Linux-IIO Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v3 1/2] iio: tsl2772: Use device-managed API
@ 2019-07-31  3:04 Chuhong Yuan
  2019-07-31  9:28 ` Brian Masney
  2019-08-05 15:04 ` Jonathan Cameron
  0 siblings, 2 replies; 3+ messages in thread
From: Chuhong Yuan @ 2019-07-31  3:04 UTC (permalink / raw)
  To: unlisted-recipients:; (no To-header on input)
  Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Brian Masney, linux-iio, linux-kernel,
	Chuhong Yuan

Use devm_() APIs to simplify the code.

Signed-off-by: Chuhong Yuan <hslester96@gmail.com>
---
Changes in v3:
  - Split v2 into two patches.

 drivers/iio/light/tsl2772.c | 36 +++++++++++++++---------------------
 1 file changed, 15 insertions(+), 21 deletions(-)

diff --git a/drivers/iio/light/tsl2772.c b/drivers/iio/light/tsl2772.c
index 83cece921843..f1134f183be7 100644
--- a/drivers/iio/light/tsl2772.c
+++ b/drivers/iio/light/tsl2772.c
@@ -860,6 +860,13 @@ static int tsl2772_chip_off(struct iio_dev *indio_dev)
 	return tsl2772_write_control_reg(chip, 0x00);
 }
 
+static void tsl2772_chip_off_action(void *data)
+{
+	struct iio_dev *indio_dev = data;
+
+	tsl2772_chip_off(indio_dev);
+}
+
 /**
  * tsl2772_invoke_change - power cycle the device to implement the user
  *                         parameters
@@ -1807,10 +1814,10 @@ static int tsl2772_probe(struct i2c_client *clientp,
 		return PTR_ERR(chip->vdd_supply);
 	}
 
-	ret = devm_add_action(&clientp->dev, tsl2772_disable_regulators_action,
+	ret = devm_add_action_or_reset(&clientp->dev,
+				tsl2772_disable_regulators_action,
 			      chip);
 	if (ret < 0) {
-		tsl2772_disable_regulators_action(chip);
 		dev_err(&clientp->dev, "Failed to setup regulator cleanup action %d\n",
 			ret);
 		return ret;
@@ -1877,15 +1884,14 @@ static int tsl2772_probe(struct i2c_client *clientp,
 	if (ret < 0)
 		return ret;
 
-	ret = iio_device_register(indio_dev);
-	if (ret) {
-		tsl2772_chip_off(indio_dev);
-		dev_err(&clientp->dev,
-			"%s: iio registration failed\n", __func__);
+	ret = devm_add_action_or_reset(&clientp->dev,
+					tsl2772_chip_off_action,
+					indio_dev);
+
+	if (ret < 0)
 		return ret;
-	}
 
-	return 0;
+	return devm_iio_device_register(&clientp->dev, indio_dev);
 }
 
 static int tsl2772_suspend(struct device *dev)
@@ -1922,17 +1928,6 @@ static int tsl2772_resume(struct device *dev)
 	return tsl2772_chip_on(indio_dev);
 }
 
-static int tsl2772_remove(struct i2c_client *client)
-{
-	struct iio_dev *indio_dev = i2c_get_clientdata(client);
-
-	tsl2772_chip_off(indio_dev);
-
-	iio_device_unregister(indio_dev);
-
-	return 0;
-}
-
 static const struct i2c_device_id tsl2772_idtable[] = {
 	{ "tsl2571", tsl2571 },
 	{ "tsl2671", tsl2671 },
@@ -1979,7 +1974,6 @@ static struct i2c_driver tsl2772_driver = {
 	},
 	.id_table = tsl2772_idtable,
 	.probe = tsl2772_probe,
-	.remove = tsl2772_remove,
 };
 
 module_i2c_driver(tsl2772_driver);
-- 
2.20.1


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v3 1/2] iio: tsl2772: Use device-managed API
  2019-07-31  3:04 [PATCH v3 1/2] iio: tsl2772: Use device-managed API Chuhong Yuan
@ 2019-07-31  9:28 ` Brian Masney
  2019-08-05 15:04 ` Jonathan Cameron
  1 sibling, 0 replies; 3+ messages in thread
From: Brian Masney @ 2019-07-31  9:28 UTC (permalink / raw)
  To: Chuhong Yuan
  Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, linux-iio, linux-kernel

Hi Chuhong,

On Wed, Jul 31, 2019 at 11:04:15AM +0800, Chuhong Yuan wrote:
> Use devm_() APIs to simplify the code.
> 
> Signed-off-by: Chuhong Yuan <hslester96@gmail.com>

There needs to be more of a changelog associated with this patch since
this doesn't describe what all is done below. I see the following
distinct three things that are done with this patch:

- Use devm_add_action_or_reset to remove the call to
  tsl2772_disable_regulators_action to simplify the error path.

- Use devm_add_action_or_reset to call tsl2772_chip_off
  when the device is removed. The changelog should also mention that
  this fixes an issue where the chip is turned off before the device is
  unregistered from the IIO subsystem. There should also be a Fixes tag
  and you can reference my patch that moved the driver out of staging as
  the commit that was fixed to make it easier for the folks that
  maintain the stable kernels. This issue most likely has been present
  since the driver was first introduced into mainline in 2012, however
  there were a large number commits that were needed in order to get this
  out of staging.

- Use devm_iio_device_register, which removes the tsl2772_remove
  function.

I would break this up further into three patches that are described
above. The patches will be small but it makes it much easier on the
maintainers who see a large number of patches come through. It helps
to keep the git history in the kernel clean, which is very helpful to
other developers.

Brian


> ---
> Changes in v3:
>   - Split v2 into two patches.
> 
>  drivers/iio/light/tsl2772.c | 36 +++++++++++++++---------------------
>  1 file changed, 15 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/iio/light/tsl2772.c b/drivers/iio/light/tsl2772.c
> index 83cece921843..f1134f183be7 100644
> --- a/drivers/iio/light/tsl2772.c
> +++ b/drivers/iio/light/tsl2772.c
> @@ -860,6 +860,13 @@ static int tsl2772_chip_off(struct iio_dev *indio_dev)
>  	return tsl2772_write_control_reg(chip, 0x00);
>  }
>  
> +static void tsl2772_chip_off_action(void *data)
> +{
> +	struct iio_dev *indio_dev = data;
> +
> +	tsl2772_chip_off(indio_dev);
> +}
> +
>  /**
>   * tsl2772_invoke_change - power cycle the device to implement the user
>   *                         parameters
> @@ -1807,10 +1814,10 @@ static int tsl2772_probe(struct i2c_client *clientp,
>  		return PTR_ERR(chip->vdd_supply);
>  	}
>  
> -	ret = devm_add_action(&clientp->dev, tsl2772_disable_regulators_action,
> +	ret = devm_add_action_or_reset(&clientp->dev,
> +				tsl2772_disable_regulators_action,
>  			      chip);
>  	if (ret < 0) {
> -		tsl2772_disable_regulators_action(chip);
>  		dev_err(&clientp->dev, "Failed to setup regulator cleanup action %d\n",
>  			ret);
>  		return ret;
> @@ -1877,15 +1884,14 @@ static int tsl2772_probe(struct i2c_client *clientp,
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = iio_device_register(indio_dev);
> -	if (ret) {
> -		tsl2772_chip_off(indio_dev);
> -		dev_err(&clientp->dev,
> -			"%s: iio registration failed\n", __func__);
> +	ret = devm_add_action_or_reset(&clientp->dev,
> +					tsl2772_chip_off_action,
> +					indio_dev);
> +
> +	if (ret < 0)
>  		return ret;
> -	}
>  
> -	return 0;
> +	return devm_iio_device_register(&clientp->dev, indio_dev);
>  }
>  
>  static int tsl2772_suspend(struct device *dev)
> @@ -1922,17 +1928,6 @@ static int tsl2772_resume(struct device *dev)
>  	return tsl2772_chip_on(indio_dev);
>  }
>  
> -static int tsl2772_remove(struct i2c_client *client)
> -{
> -	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> -
> -	tsl2772_chip_off(indio_dev);
> -
> -	iio_device_unregister(indio_dev);
> -
> -	return 0;
> -}
> -
>  static const struct i2c_device_id tsl2772_idtable[] = {
>  	{ "tsl2571", tsl2571 },
>  	{ "tsl2671", tsl2671 },
> @@ -1979,7 +1974,6 @@ static struct i2c_driver tsl2772_driver = {
>  	},
>  	.id_table = tsl2772_idtable,
>  	.probe = tsl2772_probe,
> -	.remove = tsl2772_remove,
>  };
>  
>  module_i2c_driver(tsl2772_driver);
> -- 
> 2.20.1

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v3 1/2] iio: tsl2772: Use device-managed API
  2019-07-31  3:04 [PATCH v3 1/2] iio: tsl2772: Use device-managed API Chuhong Yuan
  2019-07-31  9:28 ` Brian Masney
@ 2019-08-05 15:04 ` Jonathan Cameron
  1 sibling, 0 replies; 3+ messages in thread
From: Jonathan Cameron @ 2019-08-05 15:04 UTC (permalink / raw)
  To: Chuhong Yuan
  Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	Brian Masney, linux-iio, linux-kernel

On Wed, 31 Jul 2019 11:04:15 +0800
Chuhong Yuan <hslester96@gmail.com> wrote:

> Use devm_() APIs to simplify the code.
> 
> Signed-off-by: Chuhong Yuan <hslester96@gmail.com>
> ---
> Changes in v3:
>   - Split v2 into two patches.
> 
>  drivers/iio/light/tsl2772.c | 36 +++++++++++++++---------------------
>  1 file changed, 15 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/iio/light/tsl2772.c b/drivers/iio/light/tsl2772.c
> index 83cece921843..f1134f183be7 100644
> --- a/drivers/iio/light/tsl2772.c
> +++ b/drivers/iio/light/tsl2772.c
> @@ -860,6 +860,13 @@ static int tsl2772_chip_off(struct iio_dev *indio_dev)
>  	return tsl2772_write_control_reg(chip, 0x00);
>  }
>  
> +static void tsl2772_chip_off_action(void *data)
> +{
> +	struct iio_dev *indio_dev = data;
> +
> +	tsl2772_chip_off(indio_dev);
> +}
> +
>  /**
>   * tsl2772_invoke_change - power cycle the device to implement the user
>   *                         parameters
> @@ -1807,10 +1814,10 @@ static int tsl2772_probe(struct i2c_client *clientp,
>  		return PTR_ERR(chip->vdd_supply);
>  	}
>  
> -	ret = devm_add_action(&clientp->dev, tsl2772_disable_regulators_action,
> +	ret = devm_add_action_or_reset(&clientp->dev,
> +				tsl2772_disable_regulators_action,

Please fix that alignment in here.

Thanks,

Jonathan

>  			      chip);
>  	if (ret < 0) {
> -		tsl2772_disable_regulators_action(chip);
>  		dev_err(&clientp->dev, "Failed to setup regulator cleanup action %d\n",
>  			ret);
>  		return ret;
> @@ -1877,15 +1884,14 @@ static int tsl2772_probe(struct i2c_client *clientp,
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = iio_device_register(indio_dev);
> -	if (ret) {
> -		tsl2772_chip_off(indio_dev);
> -		dev_err(&clientp->dev,
> -			"%s: iio registration failed\n", __func__);
> +	ret = devm_add_action_or_reset(&clientp->dev,
> +					tsl2772_chip_off_action,
> +					indio_dev);
> +
> +	if (ret < 0)
>  		return ret;
> -	}
>  
> -	return 0;
> +	return devm_iio_device_register(&clientp->dev, indio_dev);
>  }
>  
>  static int tsl2772_suspend(struct device *dev)
> @@ -1922,17 +1928,6 @@ static int tsl2772_resume(struct device *dev)
>  	return tsl2772_chip_on(indio_dev);
>  }
>  
> -static int tsl2772_remove(struct i2c_client *client)
> -{
> -	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> -
> -	tsl2772_chip_off(indio_dev);
> -
> -	iio_device_unregister(indio_dev);
> -
> -	return 0;
> -}
> -
>  static const struct i2c_device_id tsl2772_idtable[] = {
>  	{ "tsl2571", tsl2571 },
>  	{ "tsl2671", tsl2671 },
> @@ -1979,7 +1974,6 @@ static struct i2c_driver tsl2772_driver = {
>  	},
>  	.id_table = tsl2772_idtable,
>  	.probe = tsl2772_probe,
> -	.remove = tsl2772_remove,
>  };
>  
>  module_i2c_driver(tsl2772_driver);


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-31  3:04 [PATCH v3 1/2] iio: tsl2772: Use device-managed API Chuhong Yuan
2019-07-31  9:28 ` Brian Masney
2019-08-05 15:04 ` Jonathan Cameron

Linux-IIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iio/0 linux-iio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iio linux-iio/ https://lore.kernel.org/linux-iio \
		linux-iio@vger.kernel.org linux-iio@archiver.kernel.org
	public-inbox-index linux-iio


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-iio


AGPL code for this site: git clone https://public-inbox.org/ public-inbox