[PATCH v2 0/3] staging: iio: ad7150: improve driver readability

[PATCH v2 0/3] staging: iio: ad7150: improve driver readability

[Melissa Wen]

This patchset solves readability issues in AD7150 code: use of FIELD_GET
to fashion improvement, make operation more succint and remove useless
comments.

Changes in v2:
- Remove noisy patch that reorganized registers definitions
- Remove else to improve i2c return operation.

Melissa Wen (3):
  staging: iio: ad7150: use FIELD_GET and GENMASK
  staging: iio: ad7150: simplify i2c SMBus return treatment
  staging: iio: ad7150: clean up of comments

 drivers/staging/iio/cdc/ad7150.c | 26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

-- 
2.20.1

[PATCH v2 1/3] staging: iio: ad7150: use FIELD_GET and GENMASK

[Melissa Wen]

Use the bitfield macro FIELD_GET, and GENMASK to do the shift and mask in
one go. This makes the code more readable than explicit masking followed
by a shift.

Signed-off-by: Melissa Wen <melissa.srw@gmail.com>
---
 drivers/staging/iio/cdc/ad7150.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
index 8234da4b8c65..091aa33589d7 100644
--- a/drivers/staging/iio/cdc/ad7150.c
+++ b/drivers/staging/iio/cdc/ad7150.c
@@ -5,6 +5,7 @@
  * Copyright 2010-2011 Analog Devices Inc.
  */
 
+#include <linux/bitfield.h>
 #include <linux/interrupt.h>
 #include <linux/device.h>
 #include <linux/kernel.h>
@@ -45,6 +46,9 @@
 #define AD7150_SN0                 22
 #define AD7150_ID                  23
 
+/* AD7150 masks */
+#define AD7150_THRESHTYPE_MSK			GENMASK(6, 5)
+
 /**
  * struct ad7150_chip_info - instance specific chip data
  * @client: i2c client for this device
@@ -137,7 +141,7 @@ static int ad7150_read_event_config(struct iio_dev *indio_dev,
 	if (ret < 0)
 		return ret;
 
-	threshtype = (ret >> 5) & 0x03;
+	threshtype = FIELD_GET(AD7150_THRESHTYPE_MSK, ret);
 	adaptive = !!(ret & 0x80);
 
 	switch (type) {
-- 
2.20.1

Re: [PATCH v2 1/3] staging: iio: ad7150: use FIELD_GET and GENMASK

[Jonathan Cameron]

On Fri, 14 Jun 2019 13:32:21 -0300
Melissa Wen <melissa.srw@gmail.com> wrote:

> Use the bitfield macro FIELD_GET, and GENMASK to do the shift and mask in
> one go. This makes the code more readable than explicit masking followed
> by a shift.
> 
> Signed-off-by: Melissa Wen <melissa.srw@gmail.com>
Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to paly with it.

Thanks,

Jonathan

> ---
>  drivers/staging/iio/cdc/ad7150.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
> index 8234da4b8c65..091aa33589d7 100644
> --- a/drivers/staging/iio/cdc/ad7150.c
> +++ b/drivers/staging/iio/cdc/ad7150.c
> @@ -5,6 +5,7 @@
>   * Copyright 2010-2011 Analog Devices Inc.
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/interrupt.h>
>  #include <linux/device.h>
>  #include <linux/kernel.h>
> @@ -45,6 +46,9 @@
>  #define AD7150_SN0                 22
>  #define AD7150_ID                  23
>  
> +/* AD7150 masks */
> +#define AD7150_THRESHTYPE_MSK			GENMASK(6, 5)
> +
>  /**
>   * struct ad7150_chip_info - instance specific chip data
>   * @client: i2c client for this device
> @@ -137,7 +141,7 @@ static int ad7150_read_event_config(struct iio_dev *indio_dev,
>  	if (ret < 0)
>  		return ret;
>  
> -	threshtype = (ret >> 5) & 0x03;
> +	threshtype = FIELD_GET(AD7150_THRESHTYPE_MSK, ret);
>  	adaptive = !!(ret & 0x80);
>  
>  	switch (type) {

[PATCH v2 2/3] staging: iio: ad7150: simplify i2c SMBus return treatment

[Melissa Wen]

Since i2c_smbus_write_byte_data returns no-positive value, this commit
making the treatment of its return value less verbose.

Signed-off-by: Melissa Wen <melissa.srw@gmail.com>
---
 drivers/staging/iio/cdc/ad7150.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
index 091aa33589d7..7d56f10a19ed 100644
--- a/drivers/staging/iio/cdc/ad7150.c
+++ b/drivers/staging/iio/cdc/ad7150.c
@@ -202,16 +202,11 @@ static int ad7150_write_event_params(struct iio_dev *indio_dev,
 	ret = i2c_smbus_write_byte_data(chip->client,
 					ad7150_addresses[chan][4],
 					sens);
-	if (ret < 0)
+	if (ret)
 		return ret;
-
-	ret = i2c_smbus_write_byte_data(chip->client,
+	return i2c_smbus_write_byte_data(chip->client,
 					ad7150_addresses[chan][5],
 					timeout);
-	if (ret < 0)
-		return ret;
-
-	return 0;
 }
 
 static int ad7150_write_event_config(struct iio_dev *indio_dev,
-- 
2.20.1

Re: [PATCH v2 2/3] staging: iio: ad7150: simplify i2c SMBus return treatment

[Jonathan Cameron]

On Fri, 14 Jun 2019 13:32:54 -0300
Melissa Wen <melissa.srw@gmail.com> wrote:

> Since i2c_smbus_write_byte_data returns no-positive value, this commit
> making the treatment of its return value less verbose.
> 
> Signed-off-by: Melissa Wen <melissa.srw@gmail.com>
Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to play with it.

Thanks,

Jonathan

> ---
>  drivers/staging/iio/cdc/ad7150.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
> index 091aa33589d7..7d56f10a19ed 100644
> --- a/drivers/staging/iio/cdc/ad7150.c
> +++ b/drivers/staging/iio/cdc/ad7150.c
> @@ -202,16 +202,11 @@ static int ad7150_write_event_params(struct iio_dev *indio_dev,
>  	ret = i2c_smbus_write_byte_data(chip->client,
>  					ad7150_addresses[chan][4],
>  					sens);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
> -
> -	ret = i2c_smbus_write_byte_data(chip->client,
> +	return i2c_smbus_write_byte_data(chip->client,
>  					ad7150_addresses[chan][5],
>  					timeout);
> -	if (ret < 0)
> -		return ret;
> -
> -	return 0;
>  }
>  
>  static int ad7150_write_event_config(struct iio_dev *indio_dev,

[PATCH v2 3/3] staging: iio: ad7150: clean up of comments

[Melissa Wen]

General cleaning of comments to remove useless information or improve
description.

Signed-off-by: Melissa Wen <melissa.srw@gmail.com>
---
 drivers/staging/iio/cdc/ad7150.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
index 7d56f10a19ed..51d6b52bce8b 100644
--- a/drivers/staging/iio/cdc/ad7150.c
+++ b/drivers/staging/iio/cdc/ad7150.c
@@ -163,7 +163,8 @@ static int ad7150_read_event_config(struct iio_dev *indio_dev,
 	return -EINVAL;
 }
 
-/* lock should be held */
+/* state_lock should be held to ensure consistent state*/
+
 static int ad7150_write_event_params(struct iio_dev *indio_dev,
 				     unsigned int chan,
 				     enum iio_event_type type,
@@ -479,10 +480,6 @@ static const struct iio_chan_spec ad7150_channels[] = {
 	AD7150_CAPACITANCE_CHAN(1)
 };
 
-/*
- * threshold events
- */
-
 static irqreturn_t ad7150_event_handler(int irq, void *private)
 {
 	struct iio_dev *indio_dev = private;
@@ -571,10 +568,6 @@ static const struct iio_info ad7150_info = {
 	.write_event_value = &ad7150_write_event_value,
 };
 
-/*
- * device probe and remove
- */
-
 static int ad7150_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
-- 
2.20.1

Re: [PATCH v2 3/3] staging: iio: ad7150: clean up of comments

[Jonathan Cameron]

On Fri, 14 Jun 2019 13:33:19 -0300
Melissa Wen <melissa.srw@gmail.com> wrote:

> General cleaning of comments to remove useless information or improve
> description.
> 
> Signed-off-by: Melissa Wen <melissa.srw@gmail.com>
Applied,

Thanks,

Jonathan

> ---
>  drivers/staging/iio/cdc/ad7150.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
> index 7d56f10a19ed..51d6b52bce8b 100644
> --- a/drivers/staging/iio/cdc/ad7150.c
> +++ b/drivers/staging/iio/cdc/ad7150.c
> @@ -163,7 +163,8 @@ static int ad7150_read_event_config(struct iio_dev *indio_dev,
>  	return -EINVAL;
>  }
>  
> -/* lock should be held */
> +/* state_lock should be held to ensure consistent state*/
> +
>  static int ad7150_write_event_params(struct iio_dev *indio_dev,
>  				     unsigned int chan,
>  				     enum iio_event_type type,
> @@ -479,10 +480,6 @@ static const struct iio_chan_spec ad7150_channels[] = {
>  	AD7150_CAPACITANCE_CHAN(1)
>  };
>  
> -/*
> - * threshold events
> - */
> -
>  static irqreturn_t ad7150_event_handler(int irq, void *private)
>  {
>  	struct iio_dev *indio_dev = private;
> @@ -571,10 +568,6 @@ static const struct iio_info ad7150_info = {
>  	.write_event_value = &ad7150_write_event_value,
>  };
>  
> -/*
> - * device probe and remove
> - */
> -
>  static int ad7150_probe(struct i2c_client *client,
>  			const struct i2c_device_id *id)
>  {