[PATCH 0/7] staging:iio:ade7854: Cleanup on I2C/SPI code

[PATCH 0/7] staging:iio:ade7854: Cleanup on I2C/SPI code

[Rodrigo Siqueira]

This patchset reworks the I2C/SPI code from meter module. The set of
patches try to reduce the code duplication and make the communication
reliable. The current version of the module had many code duplications,
which make the code more error-prone and hard to read. Jonh Syne
identified some wrong error handling and fixed it in his patches; in
this series of patches I analyzed Jonh's fixes, and use it in the new
code.

It is important to highlight that meter module is under observation, due
to the lack of hardware and the old design of the chip. However, John
has the hardware for testing and interest to help to update the code
[1]. As a result, this patchset represents the first work effort to
update the meter module in the staging.

1 - https://marc.info/?l=linux-iio&m=152046885922153&w=2 

Rodrigo Siqueira (7):
  staging:iio:ade7854: Rework I2C write function
  staging:iio:ade7854: Rework SPI write function
  staging:iio:ade7854: Replace many functions for one function
  staging:iio:ade7854: Rework I2C read function
  staging:iio:ade7854: Rework SPI read function
  staging:iio:ade7854: Remove read_reg_* duplications
  staging:iio:ade7854: Add proper error handling condition.

 drivers/staging/iio/meter/ade7854-i2c.c | 236 ++++++++--------------------
 drivers/staging/iio/meter/ade7854-spi.c | 268 +++++++-------------------------
 drivers/staging/iio/meter/ade7854.c     |  44 +++---
 drivers/staging/iio/meter/ade7854.h     |  23 +--
 4 files changed, 160 insertions(+), 411 deletions(-)

-- 
2.16.2

[PATCH 1/7] staging:iio:ade7854: Rework I2C write function

[Rodrigo Siqueira]

The write operation using I2C has many code duplications and four
different interfaces per data size. This patch introduces a single
function that centralizes the main tasks.

The central function inserted by this patch can easily replace all the
four functions related to the data size. However, this patch does not
remove any code signature for keeping the meter module work and make
easier to review this patch.

Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
---
 drivers/staging/iio/meter/ade7854-i2c.c | 89 +++++++++++++++++++--------------
 drivers/staging/iio/meter/ade7854.h     |  7 +++
 2 files changed, 58 insertions(+), 38 deletions(-)

diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
index 317e4f0d8176..03133a05eae4 100644
--- a/drivers/staging/iio/meter/ade7854-i2c.c
+++ b/drivers/staging/iio/meter/ade7854-i2c.c
@@ -15,41 +15,74 @@
 #include <linux/iio/iio.h>
 #include "ade7854.h"
 
-static int ade7854_i2c_write_reg_8(struct device *dev,
-				   u16 reg_address,
-				   u8 val)
+static int ade7854_i2c_write_reg(struct device *dev,
+				 u16 reg_address,
+				 u32 val,
+				 enum data_size type)
 {
 	int ret;
+	int count;
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct ade7854_state *st = iio_priv(indio_dev);
 
 	mutex_lock(&st->buf_lock);
 	st->tx[0] = (reg_address >> 8) & 0xFF;
 	st->tx[1] = reg_address & 0xFF;
-	st->tx[2] = val;
 
-	ret = i2c_master_send(st->i2c, st->tx, 3);
+	switch (type) {
+	case DATA_SIZE_8_BITS:
+		st->tx[2] = val & 0xFF;
+		count = 3;
+		break;
+	case DATA_SIZE_16_BITS:
+		st->tx[2] = (val >> 8) & 0xFF;
+		st->tx[3] = val & 0xFF;
+		count = 4;
+		break;
+	case DATA_SIZE_24_BITS:
+		st->tx[2] = (val >> 16) & 0xFF;
+		st->tx[3] = (val >> 8) & 0xFF;
+		st->tx[4] = val & 0xFF;
+		count = 5;
+		break;
+	case DATA_SIZE_32_BITS:
+		st->tx[2] = (val >> 24) & 0xFF;
+		st->tx[3] = (val >> 16) & 0xFF;
+		st->tx[4] = (val >> 8) & 0xFF;
+		st->tx[5] = val & 0xFF;
+		count = 6;
+		break;
+	default:
+		ret = -EINVAL;
+		goto error_i2c_write_unlock;
+	}
+
+	ret = i2c_master_send(st->i2c, st->tx, count);
+
+error_i2c_write_unlock:
 	mutex_unlock(&st->buf_lock);
 
 	return ret;
 }
 
+static int ade7854_i2c_write_reg_8(struct device *dev,
+				   u16 reg_address,
+				   u8 val)
+{
+	int ret;
+
+	ret = ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_8_BITS);
+
+	return ret;
+}
+
 static int ade7854_i2c_write_reg_16(struct device *dev,
 				    u16 reg_address,
 				    u16 val)
 {
 	int ret;
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct ade7854_state *st = iio_priv(indio_dev);
 
-	mutex_lock(&st->buf_lock);
-	st->tx[0] = (reg_address >> 8) & 0xFF;
-	st->tx[1] = reg_address & 0xFF;
-	st->tx[2] = (val >> 8) & 0xFF;
-	st->tx[3] = val & 0xFF;
-
-	ret = i2c_master_send(st->i2c, st->tx, 4);
-	mutex_unlock(&st->buf_lock);
+	ret = ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_16_BITS);
 
 	return ret;
 }
@@ -59,18 +92,8 @@ static int ade7854_i2c_write_reg_24(struct device *dev,
 				    u32 val)
 {
 	int ret;
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct ade7854_state *st = iio_priv(indio_dev);
 
-	mutex_lock(&st->buf_lock);
-	st->tx[0] = (reg_address >> 8) & 0xFF;
-	st->tx[1] = reg_address & 0xFF;
-	st->tx[2] = (val >> 16) & 0xFF;
-	st->tx[3] = (val >> 8) & 0xFF;
-	st->tx[4] = val & 0xFF;
-
-	ret = i2c_master_send(st->i2c, st->tx, 5);
-	mutex_unlock(&st->buf_lock);
+	ret = ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_24_BITS);
 
 	return ret;
 }
@@ -80,23 +103,13 @@ static int ade7854_i2c_write_reg_32(struct device *dev,
 				    u32 val)
 {
 	int ret;
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct ade7854_state *st = iio_priv(indio_dev);
-
-	mutex_lock(&st->buf_lock);
-	st->tx[0] = (reg_address >> 8) & 0xFF;
-	st->tx[1] = reg_address & 0xFF;
-	st->tx[2] = (val >> 24) & 0xFF;
-	st->tx[3] = (val >> 16) & 0xFF;
-	st->tx[4] = (val >> 8) & 0xFF;
-	st->tx[5] = val & 0xFF;
 
-	ret = i2c_master_send(st->i2c, st->tx, 6);
-	mutex_unlock(&st->buf_lock);
+	ret = ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_32_BITS);
 
 	return ret;
 }
 
+
 static int ade7854_i2c_read_reg_8(struct device *dev,
 				  u16 reg_address,
 				  u8 *val)
diff --git a/drivers/staging/iio/meter/ade7854.h b/drivers/staging/iio/meter/ade7854.h
index a82d38224cbd..71bdd638f348 100644
--- a/drivers/staging/iio/meter/ade7854.h
+++ b/drivers/staging/iio/meter/ade7854.h
@@ -143,6 +143,13 @@
 #define ADE7854_SPI_BURST	(u32)(1000 * 1000)
 #define ADE7854_SPI_FAST	(u32)(2000 * 1000)
 
+enum data_size {
+	DATA_SIZE_8_BITS = 1,
+	DATA_SIZE_16_BITS,
+	DATA_SIZE_24_BITS,
+	DATA_SIZE_32_BITS,
+};
+
 /**
  * struct ade7854_state - device instance specific data
  * @spi:			actual spi_device
-- 
2.16.2

Re: [PATCH 1/7] staging:iio:ade7854: Rework I2C write function

[Dan Carpenter]

On Wed, Mar 14, 2018 at 03:10:18PM -0300, Rodrigo Siqueira wrote:
> The write operation using I2C has many code duplications and four
> different interfaces per data size. This patch introduces a single
> function that centralizes the main tasks.
> 
> The central function inserted by this patch can easily replace all the
> four functions related to the data size. However, this patch does not
> remove any code signature for keeping the meter module work and make
> easier to review this patch.
> 
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> ---
>  drivers/staging/iio/meter/ade7854-i2c.c | 89 +++++++++++++++++++--------------
>  drivers/staging/iio/meter/ade7854.h     |  7 +++
>  2 files changed, 58 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
> index 317e4f0d8176..03133a05eae4 100644
> --- a/drivers/staging/iio/meter/ade7854-i2c.c
> +++ b/drivers/staging/iio/meter/ade7854-i2c.c
> @@ -15,41 +15,74 @@
>  #include <linux/iio/iio.h>
>  #include "ade7854.h"
>  
> -static int ade7854_i2c_write_reg_8(struct device *dev,
> -				   u16 reg_address,
> -				   u8 val)
> +static int ade7854_i2c_write_reg(struct device *dev,
> +				 u16 reg_address,
> +				 u32 val,
> +				 enum data_size type)


The data size should just be the number of bytes and not an enum.
1 means 1 byte / 8 bits.
2 means 2 bytes / 16 bits.
3 means 3 bytes / 24 bits.
etc.

>  {
>  	int ret;
> +	int count;
>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>  	struct ade7854_state *st = iio_priv(indio_dev);
>  
>  	mutex_lock(&st->buf_lock);
>  	st->tx[0] = (reg_address >> 8) & 0xFF;
>  	st->tx[1] = reg_address & 0xFF;
> -	st->tx[2] = val;
>  
> -	ret = i2c_master_send(st->i2c, st->tx, 3);
> +	switch (type) {
> +	case DATA_SIZE_8_BITS:
> +		st->tx[2] = val & 0xFF;
> +		count = 3;
> +		break;
> +	case DATA_SIZE_16_BITS:
> +		st->tx[2] = (val >> 8) & 0xFF;
> +		st->tx[3] = val & 0xFF;
> +		count = 4;
> +		break;
> +	case DATA_SIZE_24_BITS:
> +		st->tx[2] = (val >> 16) & 0xFF;
> +		st->tx[3] = (val >> 8) & 0xFF;
> +		st->tx[4] = val & 0xFF;
> +		count = 5;
> +		break;
> +	case DATA_SIZE_32_BITS:
> +		st->tx[2] = (val >> 24) & 0xFF;
> +		st->tx[3] = (val >> 16) & 0xFF;
> +		st->tx[4] = (val >> 8) & 0xFF;
> +		st->tx[5] = val & 0xFF;
> +		count = 6;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		goto error_i2c_write_unlock;
> +	}
> +
> +	ret = i2c_master_send(st->i2c, st->tx, count);
> +
> +error_i2c_write_unlock:

These labels are sort of long.  And what does the "i2c_write" really
mean?  It should be obvious that we're not jumping to a different
function.

Just "unlock:" is OK as a label name.

>  	mutex_unlock(&st->buf_lock);
>  
>  	return ret;
>  }
>  
> +static int ade7854_i2c_write_reg_8(struct device *dev,
> +				   u16 reg_address,
> +				   u8 val)
> +{
> +	int ret;
> +
> +	ret = ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_8_BITS);
> +
> +	return ret;
> +}

Just do it like this:

static int ade7854_i2c_write_reg_8(struct device *dev, u16 reg_address, u8 val)
{
	return ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_8_BITS);
}



> +
>  static int ade7854_i2c_write_reg_16(struct device *dev,
>  				    u16 reg_address,
>  				    u16 val)
>  {
>  	int ret;
> -	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> -	struct ade7854_state *st = iio_priv(indio_dev);
>  
> -	mutex_lock(&st->buf_lock);
> -	st->tx[0] = (reg_address >> 8) & 0xFF;
> -	st->tx[1] = reg_address & 0xFF;
> -	st->tx[2] = (val >> 8) & 0xFF;
> -	st->tx[3] = val & 0xFF;
> -
> -	ret = i2c_master_send(st->i2c, st->tx, 4);
> -	mutex_unlock(&st->buf_lock);
> +	ret = ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_16_BITS);
>  
>  	return ret;

Again:

	return ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_16_BITS);



>  }
> @@ -59,18 +92,8 @@ static int ade7854_i2c_write_reg_24(struct device *dev,
>  				    u32 val)
>  {
>  	int ret;
> -	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> -	struct ade7854_state *st = iio_priv(indio_dev);
>  
> -	mutex_lock(&st->buf_lock);
> -	st->tx[0] = (reg_address >> 8) & 0xFF;
> -	st->tx[1] = reg_address & 0xFF;
> -	st->tx[2] = (val >> 16) & 0xFF;
> -	st->tx[3] = (val >> 8) & 0xFF;
> -	st->tx[4] = val & 0xFF;
> -
> -	ret = i2c_master_send(st->i2c, st->tx, 5);
> -	mutex_unlock(&st->buf_lock);
> +	ret = ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_24_BITS);
>  
>  	return ret;

Same.

>  }
> @@ -80,23 +103,13 @@ static int ade7854_i2c_write_reg_32(struct device *dev,
>  				    u32 val)
>  {
>  	int ret;
> -	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> -	struct ade7854_state *st = iio_priv(indio_dev);
> -
> -	mutex_lock(&st->buf_lock);
> -	st->tx[0] = (reg_address >> 8) & 0xFF;
> -	st->tx[1] = reg_address & 0xFF;
> -	st->tx[2] = (val >> 24) & 0xFF;
> -	st->tx[3] = (val >> 16) & 0xFF;
> -	st->tx[4] = (val >> 8) & 0xFF;
> -	st->tx[5] = val & 0xFF;
>  
> -	ret = i2c_master_send(st->i2c, st->tx, 6);
> -	mutex_unlock(&st->buf_lock);
> +	ret = ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_32_BITS);
>  
>  	return ret;

Same.

>  }
>  
> +

Checkpatch.pl will complain about this second blank line.

regards,
dan carpenter

Re: [PATCH 1/7] staging:iio:ade7854: Rework I2C write function

[Rodrigo Siqueira]

Hi,

I will fixes all of these things here and in the other patches and send a
v2.

Thanks for the review.

On 03/15, Dan Carpenter wrote:
> On Wed, Mar 14, 2018 at 03:10:18PM -0300, Rodrigo Siqueira wrote:
> > The write operation using I2C has many code duplications and four
> > different interfaces per data size. This patch introduces a single
> > function that centralizes the main tasks.
> > 
> > The central function inserted by this patch can easily replace all the
> > four functions related to the data size. However, this patch does not
> > remove any code signature for keeping the meter module work and make
> > easier to review this patch.
> > 
> > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > ---
> >  drivers/staging/iio/meter/ade7854-i2c.c | 89 +++++++++++++++++++--------------
> >  drivers/staging/iio/meter/ade7854.h     |  7 +++
> >  2 files changed, 58 insertions(+), 38 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
> > index 317e4f0d8176..03133a05eae4 100644
> > --- a/drivers/staging/iio/meter/ade7854-i2c.c
> > +++ b/drivers/staging/iio/meter/ade7854-i2c.c
> > @@ -15,41 +15,74 @@
> >  #include <linux/iio/iio.h>
> >  #include "ade7854.h"
> >  
> > -static int ade7854_i2c_write_reg_8(struct device *dev,
> > -				   u16 reg_address,
> > -				   u8 val)
> > +static int ade7854_i2c_write_reg(struct device *dev,
> > +				 u16 reg_address,
> > +				 u32 val,
> > +				 enum data_size type)
> 
> 
> The data size should just be the number of bytes and not an enum.
> 1 means 1 byte / 8 bits.
> 2 means 2 bytes / 16 bits.
> 3 means 3 bytes / 24 bits.
> etc.
> 
> >  {
> >  	int ret;
> > +	int count;
> >  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> >  	struct ade7854_state *st = iio_priv(indio_dev);
> >  
> >  	mutex_lock(&st->buf_lock);
> >  	st->tx[0] = (reg_address >> 8) & 0xFF;
> >  	st->tx[1] = reg_address & 0xFF;
> > -	st->tx[2] = val;
> >  
> > -	ret = i2c_master_send(st->i2c, st->tx, 3);
> > +	switch (type) {
> > +	case DATA_SIZE_8_BITS:
> > +		st->tx[2] = val & 0xFF;
> > +		count = 3;
> > +		break;
> > +	case DATA_SIZE_16_BITS:
> > +		st->tx[2] = (val >> 8) & 0xFF;
> > +		st->tx[3] = val & 0xFF;
> > +		count = 4;
> > +		break;
> > +	case DATA_SIZE_24_BITS:
> > +		st->tx[2] = (val >> 16) & 0xFF;
> > +		st->tx[3] = (val >> 8) & 0xFF;
> > +		st->tx[4] = val & 0xFF;
> > +		count = 5;
> > +		break;
> > +	case DATA_SIZE_32_BITS:
> > +		st->tx[2] = (val >> 24) & 0xFF;
> > +		st->tx[3] = (val >> 16) & 0xFF;
> > +		st->tx[4] = (val >> 8) & 0xFF;
> > +		st->tx[5] = val & 0xFF;
> > +		count = 6;
> > +		break;
> > +	default:
> > +		ret = -EINVAL;
> > +		goto error_i2c_write_unlock;
> > +	}
> > +
> > +	ret = i2c_master_send(st->i2c, st->tx, count);
> > +
> > +error_i2c_write_unlock:
> 
> These labels are sort of long.  And what does the "i2c_write" really
> mean?  It should be obvious that we're not jumping to a different
> function.
> 
> Just "unlock:" is OK as a label name.
> 
> >  	mutex_unlock(&st->buf_lock);
> >  
> >  	return ret;
> >  }
> >  
> > +static int ade7854_i2c_write_reg_8(struct device *dev,
> > +				   u16 reg_address,
> > +				   u8 val)
> > +{
> > +	int ret;
> > +
> > +	ret = ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_8_BITS);
> > +
> > +	return ret;
> > +}
> 
> Just do it like this:
> 
> static int ade7854_i2c_write_reg_8(struct device *dev, u16 reg_address, u8 val)
> {
> 	return ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_8_BITS);
> }
> 
> 
> 
> > +
> >  static int ade7854_i2c_write_reg_16(struct device *dev,
> >  				    u16 reg_address,
> >  				    u16 val)
> >  {
> >  	int ret;
> > -	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > -	struct ade7854_state *st = iio_priv(indio_dev);
> >  
> > -	mutex_lock(&st->buf_lock);
> > -	st->tx[0] = (reg_address >> 8) & 0xFF;
> > -	st->tx[1] = reg_address & 0xFF;
> > -	st->tx[2] = (val >> 8) & 0xFF;
> > -	st->tx[3] = val & 0xFF;
> > -
> > -	ret = i2c_master_send(st->i2c, st->tx, 4);
> > -	mutex_unlock(&st->buf_lock);
> > +	ret = ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_16_BITS);
> >  
> >  	return ret;
> 
> Again:
> 
> 	return ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_16_BITS);
> 
> 
> 
> >  }
> > @@ -59,18 +92,8 @@ static int ade7854_i2c_write_reg_24(struct device *dev,
> >  				    u32 val)
> >  {
> >  	int ret;
> > -	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > -	struct ade7854_state *st = iio_priv(indio_dev);
> >  
> > -	mutex_lock(&st->buf_lock);
> > -	st->tx[0] = (reg_address >> 8) & 0xFF;
> > -	st->tx[1] = reg_address & 0xFF;
> > -	st->tx[2] = (val >> 16) & 0xFF;
> > -	st->tx[3] = (val >> 8) & 0xFF;
> > -	st->tx[4] = val & 0xFF;
> > -
> > -	ret = i2c_master_send(st->i2c, st->tx, 5);
> > -	mutex_unlock(&st->buf_lock);
> > +	ret = ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_24_BITS);
> >  
> >  	return ret;
> 
> Same.
> 
> >  }
> > @@ -80,23 +103,13 @@ static int ade7854_i2c_write_reg_32(struct device *dev,
> >  				    u32 val)
> >  {
> >  	int ret;
> > -	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > -	struct ade7854_state *st = iio_priv(indio_dev);
> > -
> > -	mutex_lock(&st->buf_lock);
> > -	st->tx[0] = (reg_address >> 8) & 0xFF;
> > -	st->tx[1] = reg_address & 0xFF;
> > -	st->tx[2] = (val >> 24) & 0xFF;
> > -	st->tx[3] = (val >> 16) & 0xFF;
> > -	st->tx[4] = (val >> 8) & 0xFF;
> > -	st->tx[5] = val & 0xFF;
> >  
> > -	ret = i2c_master_send(st->i2c, st->tx, 6);
> > -	mutex_unlock(&st->buf_lock);
> > +	ret = ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_32_BITS);
> >  
> >  	return ret;
> 
> Same.
> 
> >  }
> >  
> > +
> 
> Checkpatch.pl will complain about this second blank line.
> 
> regards,
> dan carpenter
> 

[PATCH 2/7] staging:iio:ade7854: Rework SPI write function

[Rodrigo Siqueira]

The write operation using SPI has a many code duplications (similar to
I2C) and four different interfaces per data size. This patch introduces
a single function that centralizes the main task related to SPI.

Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
---
 drivers/staging/iio/meter/ade7854-spi.c | 100 ++++++++++++++------------------
 1 file changed, 45 insertions(+), 55 deletions(-)

diff --git a/drivers/staging/iio/meter/ade7854-spi.c b/drivers/staging/iio/meter/ade7854-spi.c
index 4419b8f06197..0dae118428cf 100644
--- a/drivers/staging/iio/meter/ade7854-spi.c
+++ b/drivers/staging/iio/meter/ade7854-spi.c
@@ -15,9 +15,10 @@
 #include <linux/iio/iio.h>
 #include "ade7854.h"
 
-static int ade7854_spi_write_reg_8(struct device *dev,
-				   u16 reg_address,
-				   u8 val)
+static int ade7854_spi_write_reg(struct device *dev,
+				 u16 reg_address,
+				 u32 val,
+				 enum data_size type)
 {
 	int ret;
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
@@ -32,36 +33,58 @@ static int ade7854_spi_write_reg_8(struct device *dev,
 	st->tx[0] = ADE7854_WRITE_REG;
 	st->tx[1] = (reg_address >> 8) & 0xFF;
 	st->tx[2] = reg_address & 0xFF;
-	st->tx[3] = val & 0xFF;
+	switch (type) {
+	case DATA_SIZE_8_BITS:
+		st->tx[3] = val & 0xFF;
+		break;
+	case DATA_SIZE_16_BITS:
+		xfer.len = 5;
+		st->tx[3] = (val >> 8) & 0xFF;
+		st->tx[4] = val & 0xFF;
+		break;
+	case DATA_SIZE_24_BITS:
+		xfer.len = 6;
+		st->tx[3] = (val >> 16) & 0xFF;
+		st->tx[4] = (val >> 8) & 0xFF;
+		st->tx[5] = val & 0xFF;
+		break;
+	case DATA_SIZE_32_BITS:
+		xfer.len = 7;
+		st->tx[3] = (val >> 24) & 0xFF;
+		st->tx[4] = (val >> 16) & 0xFF;
+		st->tx[5] = (val >> 8) & 0xFF;
+		st->tx[6] = val & 0xFF;
+		break;
+	default:
+		ret = -EINVAL;
+		goto error_spi_mutex_unlock;
+	}
 
 	ret = spi_sync_transfer(st->spi, &xfer, 1);
+error_spi_mutex_unlock:
 	mutex_unlock(&st->buf_lock);
 
 	return ret;
 }
 
+static int ade7854_spi_write_reg_8(struct device *dev,
+				   u16 reg_address,
+				   u8 val)
+{
+	int ret;
+
+	ret = ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_8_BITS);
+
+	return ret;
+}
+
 static int ade7854_spi_write_reg_16(struct device *dev,
 				    u16 reg_address,
 				    u16 val)
 {
 	int ret;
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct ade7854_state *st = iio_priv(indio_dev);
-	struct spi_transfer xfer = {
-		.tx_buf = st->tx,
-		.bits_per_word = 8,
-		.len = 5,
-	};
 
-	mutex_lock(&st->buf_lock);
-	st->tx[0] = ADE7854_WRITE_REG;
-	st->tx[1] = (reg_address >> 8) & 0xFF;
-	st->tx[2] = reg_address & 0xFF;
-	st->tx[3] = (val >> 8) & 0xFF;
-	st->tx[4] = val & 0xFF;
-
-	ret = spi_sync_transfer(st->spi, &xfer, 1);
-	mutex_unlock(&st->buf_lock);
+	ret = ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_16_BITS);
 
 	return ret;
 }
@@ -71,24 +94,8 @@ static int ade7854_spi_write_reg_24(struct device *dev,
 				    u32 val)
 {
 	int ret;
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct ade7854_state *st = iio_priv(indio_dev);
-	struct spi_transfer xfer = {
-		.tx_buf = st->tx,
-		.bits_per_word = 8,
-		.len = 6,
-	};
 
-	mutex_lock(&st->buf_lock);
-	st->tx[0] = ADE7854_WRITE_REG;
-	st->tx[1] = (reg_address >> 8) & 0xFF;
-	st->tx[2] = reg_address & 0xFF;
-	st->tx[3] = (val >> 16) & 0xFF;
-	st->tx[4] = (val >> 8) & 0xFF;
-	st->tx[5] = val & 0xFF;
-
-	ret = spi_sync_transfer(st->spi, &xfer, 1);
-	mutex_unlock(&st->buf_lock);
+	ret = ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_24_BITS);
 
 	return ret;
 }
@@ -98,25 +105,8 @@ static int ade7854_spi_write_reg_32(struct device *dev,
 				    u32 val)
 {
 	int ret;
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct ade7854_state *st = iio_priv(indio_dev);
-	struct spi_transfer xfer = {
-		.tx_buf = st->tx,
-		.bits_per_word = 8,
-		.len = 7,
-	};
 
-	mutex_lock(&st->buf_lock);
-	st->tx[0] = ADE7854_WRITE_REG;
-	st->tx[1] = (reg_address >> 8) & 0xFF;
-	st->tx[2] = reg_address & 0xFF;
-	st->tx[3] = (val >> 24) & 0xFF;
-	st->tx[4] = (val >> 16) & 0xFF;
-	st->tx[5] = (val >> 8) & 0xFF;
-	st->tx[6] = val & 0xFF;
-
-	ret = spi_sync_transfer(st->spi, &xfer, 1);
-	mutex_unlock(&st->buf_lock);
+	ret = ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_32_BITS);
 
 	return ret;
 }
-- 
2.16.2

[PATCH 3/7] staging:iio:ade7854: Replace many functions for one function

[Rodrigo Siqueira]

This patch removes code duplications related to the write_reg_*
functions and centralizes them in a single function. Also, it eliminates
the legacy functions and replaces them by a unique signature that is
used by SPI and I2C.

Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
---
 drivers/staging/iio/meter/ade7854-i2c.c | 50 +--------------------------------
 drivers/staging/iio/meter/ade7854-spi.c | 49 +-------------------------------
 drivers/staging/iio/meter/ade7854.c     | 12 ++++----
 drivers/staging/iio/meter/ade7854.h     |  9 +++---
 4 files changed, 12 insertions(+), 108 deletions(-)

diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
index 03133a05eae4..f302359d2265 100644
--- a/drivers/staging/iio/meter/ade7854-i2c.c
+++ b/drivers/staging/iio/meter/ade7854-i2c.c
@@ -65,51 +65,6 @@ static int ade7854_i2c_write_reg(struct device *dev,
 	return ret;
 }
 
-static int ade7854_i2c_write_reg_8(struct device *dev,
-				   u16 reg_address,
-				   u8 val)
-{
-	int ret;
-
-	ret = ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_8_BITS);
-
-	return ret;
-}
-
-static int ade7854_i2c_write_reg_16(struct device *dev,
-				    u16 reg_address,
-				    u16 val)
-{
-	int ret;
-
-	ret = ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_16_BITS);
-
-	return ret;
-}
-
-static int ade7854_i2c_write_reg_24(struct device *dev,
-				    u16 reg_address,
-				    u32 val)
-{
-	int ret;
-
-	ret = ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_24_BITS);
-
-	return ret;
-}
-
-static int ade7854_i2c_write_reg_32(struct device *dev,
-				    u16 reg_address,
-				    u32 val)
-{
-	int ret;
-
-	ret = ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_32_BITS);
-
-	return ret;
-}
-
-
 static int ade7854_i2c_read_reg_8(struct device *dev,
 				  u16 reg_address,
 				  u8 *val)
@@ -230,10 +185,7 @@ static int ade7854_i2c_probe(struct i2c_client *client,
 	st->read_reg_16 = ade7854_i2c_read_reg_16;
 	st->read_reg_24 = ade7854_i2c_read_reg_24;
 	st->read_reg_32 = ade7854_i2c_read_reg_32;
-	st->write_reg_8 = ade7854_i2c_write_reg_8;
-	st->write_reg_16 = ade7854_i2c_write_reg_16;
-	st->write_reg_24 = ade7854_i2c_write_reg_24;
-	st->write_reg_32 = ade7854_i2c_write_reg_32;
+	st->write_reg = ade7854_i2c_write_reg;
 	st->i2c = client;
 	st->irq = client->irq;
 
diff --git a/drivers/staging/iio/meter/ade7854-spi.c b/drivers/staging/iio/meter/ade7854-spi.c
index 0dae118428cf..df3df85f9440 100644
--- a/drivers/staging/iio/meter/ade7854-spi.c
+++ b/drivers/staging/iio/meter/ade7854-spi.c
@@ -67,50 +67,6 @@ static int ade7854_spi_write_reg(struct device *dev,
 	return ret;
 }
 
-static int ade7854_spi_write_reg_8(struct device *dev,
-				   u16 reg_address,
-				   u8 val)
-{
-	int ret;
-
-	ret = ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_8_BITS);
-
-	return ret;
-}
-
-static int ade7854_spi_write_reg_16(struct device *dev,
-				    u16 reg_address,
-				    u16 val)
-{
-	int ret;
-
-	ret = ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_16_BITS);
-
-	return ret;
-}
-
-static int ade7854_spi_write_reg_24(struct device *dev,
-				    u16 reg_address,
-				    u32 val)
-{
-	int ret;
-
-	ret = ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_24_BITS);
-
-	return ret;
-}
-
-static int ade7854_spi_write_reg_32(struct device *dev,
-				    u16 reg_address,
-				    u32 val)
-{
-	int ret;
-
-	ret = ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_32_BITS);
-
-	return ret;
-}
-
 static int ade7854_spi_read_reg_8(struct device *dev,
 				  u16 reg_address,
 				  u8 *val)
@@ -276,10 +232,7 @@ static int ade7854_spi_probe(struct spi_device *spi)
 	st->read_reg_16 = ade7854_spi_read_reg_16;
 	st->read_reg_24 = ade7854_spi_read_reg_24;
 	st->read_reg_32 = ade7854_spi_read_reg_32;
-	st->write_reg_8 = ade7854_spi_write_reg_8;
-	st->write_reg_16 = ade7854_spi_write_reg_16;
-	st->write_reg_24 = ade7854_spi_write_reg_24;
-	st->write_reg_32 = ade7854_spi_write_reg_32;
+	st->write_reg = ade7854_spi_write_reg;
 	st->irq = spi->irq;
 	st->spi = spi;
 
diff --git a/drivers/staging/iio/meter/ade7854.c b/drivers/staging/iio/meter/ade7854.c
index 90d07cdca4b8..f9a977dcd670 100644
--- a/drivers/staging/iio/meter/ade7854.c
+++ b/drivers/staging/iio/meter/ade7854.c
@@ -105,7 +105,7 @@ static ssize_t ade7854_write_8bit(struct device *dev,
 	ret = kstrtou8(buf, 10, &val);
 	if (ret)
 		goto error_ret;
-	ret = st->write_reg_8(dev, this_attr->address, val);
+	ret = st->write_reg(dev, this_attr->address, val, DATA_SIZE_8_BITS);
 
 error_ret:
 	return ret ? ret : len;
@@ -126,7 +126,7 @@ static ssize_t ade7854_write_16bit(struct device *dev,
 	ret = kstrtou16(buf, 10, &val);
 	if (ret)
 		goto error_ret;
-	ret = st->write_reg_16(dev, this_attr->address, val);
+	ret = st->write_reg(dev, this_attr->address, val, DATA_SIZE_16_BITS);
 
 error_ret:
 	return ret ? ret : len;
@@ -147,7 +147,7 @@ static ssize_t ade7854_write_24bit(struct device *dev,
 	ret = kstrtou32(buf, 10, &val);
 	if (ret)
 		goto error_ret;
-	ret = st->write_reg_24(dev, this_attr->address, val);
+	ret = st->write_reg(dev, this_attr->address, val, DATA_SIZE_24_BITS);
 
 error_ret:
 	return ret ? ret : len;
@@ -168,7 +168,7 @@ static ssize_t ade7854_write_32bit(struct device *dev,
 	ret = kstrtou32(buf, 10, &val);
 	if (ret)
 		goto error_ret;
-	ret = st->write_reg_32(dev, this_attr->address, val);
+	ret = st->write_reg(dev, this_attr->address, val, DATA_SIZE_32_BITS);
 
 error_ret:
 	return ret ? ret : len;
@@ -183,7 +183,7 @@ static int ade7854_reset(struct device *dev)
 	st->read_reg_16(dev, ADE7854_CONFIG, &val);
 	val |= BIT(7); /* Software Chip Reset */
 
-	return st->write_reg_16(dev, ADE7854_CONFIG, val);
+	return st->write_reg(dev, ADE7854_CONFIG, val, DATA_SIZE_16_BITS);
 }
 
 static IIO_DEV_ATTR_AIGAIN(0644,
@@ -426,7 +426,7 @@ static int ade7854_set_irq(struct device *dev, bool enable)
 	else
 		irqen &= ~BIT(17);
 
-	return st->write_reg_32(dev, ADE7854_MASK0, irqen);
+	return st->write_reg(dev, ADE7854_MASK0, irqen, DATA_SIZE_32_BITS);
 }
 
 static int ade7854_initial_setup(struct iio_dev *indio_dev)
diff --git a/drivers/staging/iio/meter/ade7854.h b/drivers/staging/iio/meter/ade7854.h
index 71bdd638f348..3830607de7c5 100644
--- a/drivers/staging/iio/meter/ade7854.h
+++ b/drivers/staging/iio/meter/ade7854.h
@@ -152,7 +152,8 @@ enum data_size {
 
 /**
  * struct ade7854_state - device instance specific data
- * @spi:			actual spi_device
+ * @spi:		actual spi_device
+ * @write_reg		Wrapper function for I2C and SPI write
  * @indio_dev:		industrial I/O device structure
  * @buf_lock:		mutex to protect tx and rx
  * @tx:			transmit buffer
@@ -165,10 +166,8 @@ struct ade7854_state {
 	int (*read_reg_16)(struct device *dev, u16 reg_address, u16 *val);
 	int (*read_reg_24)(struct device *dev, u16 reg_address, u32 *val);
 	int (*read_reg_32)(struct device *dev, u16 reg_address, u32 *val);
-	int (*write_reg_8)(struct device *dev, u16 reg_address, u8 val);
-	int (*write_reg_16)(struct device *dev, u16 reg_address, u16 val);
-	int (*write_reg_24)(struct device *dev, u16 reg_address, u32 val);
-	int (*write_reg_32)(struct device *dev, u16 reg_address, u32 val);
+	int (*write_reg)(struct device *dev, u16 reg_address, u32 val,
+			 enum data_size type);
 	int irq;
 	struct mutex buf_lock;
 	u8 tx[ADE7854_MAX_TX] ____cacheline_aligned;
-- 
2.16.2

[PATCH 4/7] staging:iio:ade7854: Rework I2C read function

[Rodrigo Siqueira]

The read operation for the I2C function has many duplications that can
be generalized into a single function. This patch reworks the read
operation for I2C to centralizes all similar code in a single function.
Part of the rework includes a proper error handling and a fix on the
i2c_master_recv for 32 bits as pointed by John Syne patches.

It is possible to remove all the old interface to use the new one,
however, for keeping the things simple and working this patch maintain
legacy interface.

Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
Signed-off-by: John Syne <john3909@gmail.com>
---
 drivers/staging/iio/meter/ade7854-i2c.c | 106 ++++++++++++++------------------
 1 file changed, 47 insertions(+), 59 deletions(-)

diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
index f302359d2265..845f8c348945 100644
--- a/drivers/staging/iio/meter/ade7854-i2c.c
+++ b/drivers/staging/iio/meter/ade7854-i2c.c
@@ -65,9 +65,10 @@ static int ade7854_i2c_write_reg(struct device *dev,
 	return ret;
 }
 
-static int ade7854_i2c_read_reg_8(struct device *dev,
-				  u16 reg_address,
-				  u8 *val)
+static int ade7854_i2c_read_reg(struct device *dev,
+				u16 reg_address,
+				u32 *val,
+				enum data_size type)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct ade7854_state *st = iio_priv(indio_dev);
@@ -78,42 +79,58 @@ static int ade7854_i2c_read_reg_8(struct device *dev,
 	st->tx[1] = reg_address & 0xFF;
 
 	ret = i2c_master_send(st->i2c, st->tx, 2);
-	if (ret)
-		goto out;
+	if (ret < 0)
+		goto error_i2c_read_unlock;
 
-	ret = i2c_master_recv(st->i2c, st->rx, 1);
-	if (ret)
-		goto out;
+	ret = i2c_master_recv(st->i2c, st->rx, type);
+	if (ret < 0)
+		goto error_i2c_read_unlock;
 
-	*val = st->rx[0];
-out:
+	switch (type) {
+	case DATA_SIZE_8_BITS:
+		*val = st->rx[0];
+		break;
+	case DATA_SIZE_16_BITS:
+		*val = (st->rx[0] << 8) | st->rx[1];
+		break;
+	case DATA_SIZE_24_BITS:
+		*val = (st->rx[0] << 16) | (st->rx[1] << 8) | st->rx[2];
+		break;
+	case DATA_SIZE_32_BITS:
+		*val = (st->rx[0] << 24) | (st->rx[1] << 16) |
+			(st->rx[2] << 8) | st->rx[3];
+		break;
+	default:
+		ret = -EINVAL;
+		goto error_i2c_read_unlock;
+	}
+
+error_i2c_read_unlock:
 	mutex_unlock(&st->buf_lock);
 	return ret;
 }
 
+static int ade7854_i2c_read_reg_8(struct device *dev,
+				  u16 reg_address,
+				  u8 *val)
+{
+	int ret;
+
+	ret = ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
+				   DATA_SIZE_8_BITS);
+
+	return ret;
+}
+
 static int ade7854_i2c_read_reg_16(struct device *dev,
 				   u16 reg_address,
 				   u16 *val)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct ade7854_state *st = iio_priv(indio_dev);
 	int ret;
 
-	mutex_lock(&st->buf_lock);
-	st->tx[0] = (reg_address >> 8) & 0xFF;
-	st->tx[1] = reg_address & 0xFF;
-
-	ret = i2c_master_send(st->i2c, st->tx, 2);
-	if (ret)
-		goto out;
-
-	ret = i2c_master_recv(st->i2c, st->rx, 2);
-	if (ret)
-		goto out;
+	ret = ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
+				   DATA_SIZE_16_BITS);
 
-	*val = (st->rx[0] << 8) | st->rx[1];
-out:
-	mutex_unlock(&st->buf_lock);
 	return ret;
 }
 
@@ -121,25 +138,11 @@ static int ade7854_i2c_read_reg_24(struct device *dev,
 				   u16 reg_address,
 				   u32 *val)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct ade7854_state *st = iio_priv(indio_dev);
 	int ret;
 
-	mutex_lock(&st->buf_lock);
-	st->tx[0] = (reg_address >> 8) & 0xFF;
-	st->tx[1] = reg_address & 0xFF;
-
-	ret = i2c_master_send(st->i2c, st->tx, 2);
-	if (ret)
-		goto out;
-
-	ret = i2c_master_recv(st->i2c, st->rx, 3);
-	if (ret)
-		goto out;
+	ret = ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
+				   DATA_SIZE_24_BITS);
 
-	*val = (st->rx[0] << 16) | (st->rx[1] << 8) | st->rx[2];
-out:
-	mutex_unlock(&st->buf_lock);
 	return ret;
 }
 
@@ -147,26 +150,11 @@ static int ade7854_i2c_read_reg_32(struct device *dev,
 				   u16 reg_address,
 				   u32 *val)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct ade7854_state *st = iio_priv(indio_dev);
 	int ret;
 
-	mutex_lock(&st->buf_lock);
-	st->tx[0] = (reg_address >> 8) & 0xFF;
-	st->tx[1] = reg_address & 0xFF;
-
-	ret = i2c_master_send(st->i2c, st->tx, 2);
-	if (ret)
-		goto out;
-
-	ret = i2c_master_recv(st->i2c, st->rx, 3);
-	if (ret)
-		goto out;
+	ret = ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
+				   DATA_SIZE_32_BITS);
 
-	*val = (st->rx[0] << 24) | (st->rx[1] << 16) |
-		(st->rx[2] << 8) | st->rx[3];
-out:
-	mutex_unlock(&st->buf_lock);
 	return ret;
 }
 
-- 
2.16.2

[PATCH 5/7] staging:iio:ade7854: Rework SPI read function

[Rodrigo Siqueira]

Rework read SPI function to reduce the code duplication and centralizes
all the task in a single function.

Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
---
 drivers/staging/iio/meter/ade7854-spi.c | 132 ++++++++++----------------------
 1 file changed, 41 insertions(+), 91 deletions(-)

diff --git a/drivers/staging/iio/meter/ade7854-spi.c b/drivers/staging/iio/meter/ade7854-spi.c
index df3df85f9440..0feef24aa888 100644
--- a/drivers/staging/iio/meter/ade7854-spi.c
+++ b/drivers/staging/iio/meter/ade7854-spi.c
@@ -67,9 +67,10 @@ static int ade7854_spi_write_reg(struct device *dev,
 	return ret;
 }
 
-static int ade7854_spi_read_reg_8(struct device *dev,
-				  u16 reg_address,
-				  u8 *val)
+static int ade7854_spi_read_reg(struct device *dev,
+				u16 reg_address,
+				u32 *val,
+				enum data_size type)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct ade7854_state *st = iio_priv(indio_dev);
@@ -82,7 +83,7 @@ static int ade7854_spi_read_reg_8(struct device *dev,
 		}, {
 			.rx_buf = st->rx,
 			.bits_per_word = 8,
-			.len = 1,
+			.len = type,
 		}
 	};
 
@@ -94,51 +95,52 @@ static int ade7854_spi_read_reg_8(struct device *dev,
 
 	ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
 	if (ret) {
-		dev_err(&st->spi->dev, "problem when reading 8 bit register 0x%02X",
+		dev_err(&st->spi->dev, "problem when reading register 0x%02X",
 			reg_address);
-		goto error_ret;
+		goto error_spi_read_unlock;
+	}
+
+	switch (type) {
+	case DATA_SIZE_8_BITS:
+		*val = st->rx[0];
+		break;
+	case DATA_SIZE_16_BITS:
+		*val = be16_to_cpup((const __be16 *)st->rx);
+		break;
+	case DATA_SIZE_24_BITS:
+		*val = (st->rx[0] << 16) | (st->rx[1] << 8) | st->rx[2];
+		break;
+	case DATA_SIZE_32_BITS:
+		*val = be32_to_cpup((const __be32 *)st->rx);
+		break;
 	}
-	*val = st->rx[0];
 
-error_ret:
+error_spi_read_unlock:
 	mutex_unlock(&st->buf_lock);
 	return ret;
 }
 
+static int ade7854_spi_read_reg_8(struct device *dev,
+				  u16 reg_address,
+				  u8 *val)
+{
+	int ret;
+
+	ret = ade7854_spi_read_reg(dev, reg_address, (u32 *)val,
+				   DATA_SIZE_8_BITS);
+
+	return ret;
+}
+
 static int ade7854_spi_read_reg_16(struct device *dev,
 				   u16 reg_address,
 				   u16 *val)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct ade7854_state *st = iio_priv(indio_dev);
 	int ret;
-	struct spi_transfer xfers[] = {
-		{
-			.tx_buf = st->tx,
-			.bits_per_word = 8,
-			.len = 3,
-		}, {
-			.rx_buf = st->rx,
-			.bits_per_word = 8,
-			.len = 2,
-		}
-	};
 
-	mutex_lock(&st->buf_lock);
-	st->tx[0] = ADE7854_READ_REG;
-	st->tx[1] = (reg_address >> 8) & 0xFF;
-	st->tx[2] = reg_address & 0xFF;
+	ret = ade7854_spi_read_reg(dev, reg_address, (u32 *)val,
+				   DATA_SIZE_16_BITS);
 
-	ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
-	if (ret) {
-		dev_err(&st->spi->dev, "problem when reading 16 bit register 0x%02X",
-			reg_address);
-		goto error_ret;
-	}
-	*val = be16_to_cpup((const __be16 *)st->rx);
-
-error_ret:
-	mutex_unlock(&st->buf_lock);
 	return ret;
 }
 
@@ -146,37 +148,11 @@ static int ade7854_spi_read_reg_24(struct device *dev,
 				   u16 reg_address,
 				   u32 *val)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct ade7854_state *st = iio_priv(indio_dev);
 	int ret;
-	struct spi_transfer xfers[] = {
-		{
-			.tx_buf = st->tx,
-			.bits_per_word = 8,
-			.len = 3,
-		}, {
-			.rx_buf = st->rx,
-			.bits_per_word = 8,
-			.len = 3,
-		}
-	};
 
-	mutex_lock(&st->buf_lock);
-
-	st->tx[0] = ADE7854_READ_REG;
-	st->tx[1] = (reg_address >> 8) & 0xFF;
-	st->tx[2] = reg_address & 0xFF;
+	ret = ade7854_spi_read_reg(dev, reg_address, (u32 *)val,
+				   DATA_SIZE_24_BITS);
 
-	ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
-	if (ret) {
-		dev_err(&st->spi->dev, "problem when reading 24 bit register 0x%02X",
-			reg_address);
-		goto error_ret;
-	}
-	*val = (st->rx[0] << 16) | (st->rx[1] << 8) | st->rx[2];
-
-error_ret:
-	mutex_unlock(&st->buf_lock);
 	return ret;
 }
 
@@ -184,37 +160,11 @@ static int ade7854_spi_read_reg_32(struct device *dev,
 				   u16 reg_address,
 				   u32 *val)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct ade7854_state *st = iio_priv(indio_dev);
 	int ret;
-	struct spi_transfer xfers[] = {
-		{
-			.tx_buf = st->tx,
-			.bits_per_word = 8,
-			.len = 3,
-		}, {
-			.rx_buf = st->rx,
-			.bits_per_word = 8,
-			.len = 4,
-		}
-	};
 
-	mutex_lock(&st->buf_lock);
-
-	st->tx[0] = ADE7854_READ_REG;
-	st->tx[1] = (reg_address >> 8) & 0xFF;
-	st->tx[2] = reg_address & 0xFF;
+	ret = ade7854_spi_read_reg(dev, reg_address, (u32 *)val,
+				   DATA_SIZE_32_BITS);
 
-	ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
-	if (ret) {
-		dev_err(&st->spi->dev, "problem when reading 32 bit register 0x%02X",
-			reg_address);
-		goto error_ret;
-	}
-	*val = be32_to_cpup((const __be32 *)st->rx);
-
-error_ret:
-	mutex_unlock(&st->buf_lock);
 	return ret;
 }
 
-- 
2.16.2

[PATCH 6/7] staging:iio:ade7854: Remove read_reg_* duplications

[Rodrigo Siqueira]

The original code had a read function per data size; after updates, all
read functions tasks were centralized in a single function, but the old
signature was kept to maintain the module working without problems. This
patch removes a set of duplications associated with read_reg_*, and
update the areas that calling the old interface by the new one. During
the update for use a single function, some errors handlings were updated
based on the John Syne patches.

Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
Signed-off-by: John Syne <john3909@gmail.com>
---
 drivers/staging/iio/meter/ade7854-i2c.c | 53 +------------------------------
 drivers/staging/iio/meter/ade7854-spi.c | 55 ++-------------------------------
 drivers/staging/iio/meter/ade7854.c     | 28 ++++++++---------
 drivers/staging/iio/meter/ade7854.h     |  7 ++---
 4 files changed, 20 insertions(+), 123 deletions(-)

diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
index 845f8c348945..fc1ff08dc2d3 100644
--- a/drivers/staging/iio/meter/ade7854-i2c.c
+++ b/drivers/staging/iio/meter/ade7854-i2c.c
@@ -110,54 +110,6 @@ static int ade7854_i2c_read_reg(struct device *dev,
 	return ret;
 }
 
-static int ade7854_i2c_read_reg_8(struct device *dev,
-				  u16 reg_address,
-				  u8 *val)
-{
-	int ret;
-
-	ret = ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
-				   DATA_SIZE_8_BITS);
-
-	return ret;
-}
-
-static int ade7854_i2c_read_reg_16(struct device *dev,
-				   u16 reg_address,
-				   u16 *val)
-{
-	int ret;
-
-	ret = ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
-				   DATA_SIZE_16_BITS);
-
-	return ret;
-}
-
-static int ade7854_i2c_read_reg_24(struct device *dev,
-				   u16 reg_address,
-				   u32 *val)
-{
-	int ret;
-
-	ret = ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
-				   DATA_SIZE_24_BITS);
-
-	return ret;
-}
-
-static int ade7854_i2c_read_reg_32(struct device *dev,
-				   u16 reg_address,
-				   u32 *val)
-{
-	int ret;
-
-	ret = ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
-				   DATA_SIZE_32_BITS);
-
-	return ret;
-}
-
 static int ade7854_i2c_probe(struct i2c_client *client,
 			     const struct i2c_device_id *id)
 {
@@ -169,10 +121,7 @@ static int ade7854_i2c_probe(struct i2c_client *client,
 		return -ENOMEM;
 	st = iio_priv(indio_dev);
 	i2c_set_clientdata(client, indio_dev);
-	st->read_reg_8 = ade7854_i2c_read_reg_8;
-	st->read_reg_16 = ade7854_i2c_read_reg_16;
-	st->read_reg_24 = ade7854_i2c_read_reg_24;
-	st->read_reg_32 = ade7854_i2c_read_reg_32;
+	st->read_reg = ade7854_i2c_read_reg;
 	st->write_reg = ade7854_i2c_write_reg;
 	st->i2c = client;
 	st->irq = client->irq;
diff --git a/drivers/staging/iio/meter/ade7854-spi.c b/drivers/staging/iio/meter/ade7854-spi.c
index 0feef24aa888..5711f9ff822e 100644
--- a/drivers/staging/iio/meter/ade7854-spi.c
+++ b/drivers/staging/iio/meter/ade7854-spi.c
@@ -94,7 +94,7 @@ static int ade7854_spi_read_reg(struct device *dev,
 	st->tx[2] = reg_address & 0xFF;
 
 	ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
-	if (ret) {
+	if (ret < 0) {
 		dev_err(&st->spi->dev, "problem when reading register 0x%02X",
 			reg_address);
 		goto error_spi_read_unlock;
@@ -120,54 +120,6 @@ static int ade7854_spi_read_reg(struct device *dev,
 	return ret;
 }
 
-static int ade7854_spi_read_reg_8(struct device *dev,
-				  u16 reg_address,
-				  u8 *val)
-{
-	int ret;
-
-	ret = ade7854_spi_read_reg(dev, reg_address, (u32 *)val,
-				   DATA_SIZE_8_BITS);
-
-	return ret;
-}
-
-static int ade7854_spi_read_reg_16(struct device *dev,
-				   u16 reg_address,
-				   u16 *val)
-{
-	int ret;
-
-	ret = ade7854_spi_read_reg(dev, reg_address, (u32 *)val,
-				   DATA_SIZE_16_BITS);
-
-	return ret;
-}
-
-static int ade7854_spi_read_reg_24(struct device *dev,
-				   u16 reg_address,
-				   u32 *val)
-{
-	int ret;
-
-	ret = ade7854_spi_read_reg(dev, reg_address, (u32 *)val,
-				   DATA_SIZE_24_BITS);
-
-	return ret;
-}
-
-static int ade7854_spi_read_reg_32(struct device *dev,
-				   u16 reg_address,
-				   u32 *val)
-{
-	int ret;
-
-	ret = ade7854_spi_read_reg(dev, reg_address, (u32 *)val,
-				   DATA_SIZE_32_BITS);
-
-	return ret;
-}
-
 static int ade7854_spi_probe(struct spi_device *spi)
 {
 	struct ade7854_state *st;
@@ -178,10 +130,7 @@ static int ade7854_spi_probe(struct spi_device *spi)
 		return -ENOMEM;
 	st = iio_priv(indio_dev);
 	spi_set_drvdata(spi, indio_dev);
-	st->read_reg_8 = ade7854_spi_read_reg_8;
-	st->read_reg_16 = ade7854_spi_read_reg_16;
-	st->read_reg_24 = ade7854_spi_read_reg_24;
-	st->read_reg_32 = ade7854_spi_read_reg_32;
+	st->read_reg = ade7854_spi_read_reg;
 	st->write_reg = ade7854_spi_write_reg;
 	st->irq = spi->irq;
 	st->spi = spi;
diff --git a/drivers/staging/iio/meter/ade7854.c b/drivers/staging/iio/meter/ade7854.c
index f9a977dcd670..09fd8c067738 100644
--- a/drivers/staging/iio/meter/ade7854.c
+++ b/drivers/staging/iio/meter/ade7854.c
@@ -27,13 +27,13 @@ static ssize_t ade7854_read_8bit(struct device *dev,
 				 char *buf)
 {
 	int ret;
-	u8 val = 0;
+	u32 val = 0;
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct ade7854_state *st = iio_priv(indio_dev);
 	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
 
-	ret = st->read_reg_8(dev, this_attr->address, &val);
-	if (ret)
+	ret = st->read_reg(dev, this_attr->address, &val, DATA_SIZE_8_BITS);
+	if (ret < 0)
 		return ret;
 
 	return sprintf(buf, "%u\n", val);
@@ -44,13 +44,13 @@ static ssize_t ade7854_read_16bit(struct device *dev,
 				  char *buf)
 {
 	int ret;
-	u16 val = 0;
+	u32 val = 0;
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct ade7854_state *st = iio_priv(indio_dev);
 	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
 
-	ret = st->read_reg_16(dev, this_attr->address, &val);
-	if (ret)
+	ret = st->read_reg(dev, this_attr->address, &val, DATA_SIZE_16_BITS);
+	if (ret < 0)
 		return ret;
 
 	return sprintf(buf, "%u\n", val);
@@ -66,8 +66,8 @@ static ssize_t ade7854_read_24bit(struct device *dev,
 	struct ade7854_state *st = iio_priv(indio_dev);
 	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
 
-	ret = st->read_reg_24(dev, this_attr->address, &val);
-	if (ret)
+	ret = st->read_reg(dev, this_attr->address, &val, DATA_SIZE_24_BITS);
+	if (ret < 0)
 		return ret;
 
 	return sprintf(buf, "%u\n", val);
@@ -83,8 +83,8 @@ static ssize_t ade7854_read_32bit(struct device *dev,
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct ade7854_state *st = iio_priv(indio_dev);
 
-	ret = st->read_reg_32(dev, this_attr->address, &val);
-	if (ret)
+	ret = st->read_reg(dev, this_attr->address, &val, DATA_SIZE_32_BITS);
+	if (ret < 0)
 		return ret;
 
 	return sprintf(buf, "%u\n", val);
@@ -178,9 +178,9 @@ static int ade7854_reset(struct device *dev)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct ade7854_state *st = iio_priv(indio_dev);
-	u16 val;
+	u32 val;
 
-	st->read_reg_16(dev, ADE7854_CONFIG, &val);
+	st->read_reg(dev, ADE7854_CONFIG, &val, DATA_SIZE_16_BITS);
 	val |= BIT(7); /* Software Chip Reset */
 
 	return st->write_reg(dev, ADE7854_CONFIG, val, DATA_SIZE_16_BITS);
@@ -415,8 +415,8 @@ static int ade7854_set_irq(struct device *dev, bool enable)
 	int ret;
 	u32 irqen;
 
-	ret = st->read_reg_32(dev, ADE7854_MASK0, &irqen);
-	if (ret)
+	ret = st->read_reg(dev, ADE7854_MASK0, &irqen, DATA_SIZE_32_BITS);
+	if (ret < 0)
 		return ret;
 
 	if (enable)
diff --git a/drivers/staging/iio/meter/ade7854.h b/drivers/staging/iio/meter/ade7854.h
index 3830607de7c5..3d762e74a7be 100644
--- a/drivers/staging/iio/meter/ade7854.h
+++ b/drivers/staging/iio/meter/ade7854.h
@@ -153,6 +153,7 @@ enum data_size {
 /**
  * struct ade7854_state - device instance specific data
  * @spi:		actual spi_device
+ * @read_reg		Wrapper function for I2C and SPI read
  * @write_reg		Wrapper function for I2C and SPI write
  * @indio_dev:		industrial I/O device structure
  * @buf_lock:		mutex to protect tx and rx
@@ -162,10 +163,8 @@ enum data_size {
 struct ade7854_state {
 	struct spi_device *spi;
 	struct i2c_client *i2c;
-	int (*read_reg_8)(struct device *dev, u16 reg_address, u8 *val);
-	int (*read_reg_16)(struct device *dev, u16 reg_address, u16 *val);
-	int (*read_reg_24)(struct device *dev, u16 reg_address, u32 *val);
-	int (*read_reg_32)(struct device *dev, u16 reg_address, u32 *val);
+	int (*read_reg)(struct device *dev, u16 reg_address, u32 *val,
+			enum data_size type);
 	int (*write_reg)(struct device *dev, u16 reg_address, u32 val,
 			 enum data_size type);
 	int irq;
-- 
2.16.2

[PATCH 7/7] staging:iio:ade7854: Add proper error handling condition

[Rodrigo Siqueira]

There is some improper error handling for IRQ and device register.  This
patch adds a proper verification. The IRQ correction was extracted from
John Syne patches.

Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
Signed-off-by: John Syne <john3909@gmail.com>
---
 drivers/staging/iio/meter/ade7854.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/iio/meter/ade7854.c b/drivers/staging/iio/meter/ade7854.c
index 09fd8c067738..49cbe365e43d 100644
--- a/drivers/staging/iio/meter/ade7854.c
+++ b/drivers/staging/iio/meter/ade7854.c
@@ -436,7 +436,7 @@ static int ade7854_initial_setup(struct iio_dev *indio_dev)
 
 	/* Disable IRQ */
 	ret = ade7854_set_irq(dev, false);
-	if (ret) {
+	if (ret < 0) {
 		dev_err(dev, "disable irq failed");
 		goto err_ret;
 	}
@@ -544,7 +544,7 @@ int ade7854_probe(struct iio_dev *indio_dev, struct device *dev)
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
 	ret = devm_iio_device_register(dev, indio_dev);
-	if (ret)
+	if (ret < 0)
 		return ret;
 
 	/* Get the device into a sane initial state */
-- 
2.16.2

Re: [PATCH 7/7] staging:iio:ade7854: Add proper error handling condition

[Dan Carpenter]

On Wed, Mar 14, 2018 at 03:12:18PM -0300, Rodrigo Siqueira wrote:
> There is some improper error handling for IRQ and device register.  This
> patch adds a proper verification. The IRQ correction was extracted from
> John Syne patches.
> 
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> Signed-off-by: John Syne <john3909@gmail.com>
> ---
>  drivers/staging/iio/meter/ade7854.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/iio/meter/ade7854.c b/drivers/staging/iio/meter/ade7854.c
> index 09fd8c067738..49cbe365e43d 100644
> --- a/drivers/staging/iio/meter/ade7854.c
> +++ b/drivers/staging/iio/meter/ade7854.c
> @@ -436,7 +436,7 @@ static int ade7854_initial_setup(struct iio_dev *indio_dev)
>  
>  	/* Disable IRQ */
>  	ret = ade7854_set_irq(dev, false);
> -	if (ret) {
> +	if (ret < 0) {
>  		dev_err(dev, "disable irq failed");
>  		goto err_ret;
>  	}

Why is the original wrong?  It seems fine.

regards,
dan carpenter

Re: [PATCH 7/7] staging:iio:ade7854: Add proper error handling condition

[Rodrigo Siqueira]

On 03/15, Dan Carpenter wrote:
> On Wed, Mar 14, 2018 at 03:12:18PM -0300, Rodrigo Siqueira wrote:
> > There is some improper error handling for IRQ and device register.  This
> > patch adds a proper verification. The IRQ correction was extracted from
> > John Syne patches.
> > 
> > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > Signed-off-by: John Syne <john3909@gmail.com>
> > ---
> >  drivers/staging/iio/meter/ade7854.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/meter/ade7854.c b/drivers/staging/iio/meter/ade7854.c
> > index 09fd8c067738..49cbe365e43d 100644
> > --- a/drivers/staging/iio/meter/ade7854.c
> > +++ b/drivers/staging/iio/meter/ade7854.c
> > @@ -436,7 +436,7 @@ static int ade7854_initial_setup(struct iio_dev *indio_dev)
> >  
> >  	/* Disable IRQ */
> >  	ret = ade7854_set_irq(dev, false);
> > -	if (ret) {
> > +	if (ret < 0) {
> >  		dev_err(dev, "disable irq failed");
> >  		goto err_ret;
> >  	}
> 
> Why is the original wrong?  It seems fine.

Hi,

If you look at "drivers/staging/iio/meter/ade7854-(i2c|spi).c", you will
find a line like this: " st->write_reg = ade7854_(i2c|spi)_write_reg;".
Than, if you go through the "ade7854_i2c_write_reg" (focus only on i2c
for while) you will see "i2c_master_send(st->i2c, st->tx, count)",
which can returns a positive number that does not necessary means an
error. If you find the same for the spi case, you will end up in the
function "spi_sync_transfer" that only return 0 for success or negative
for failure. So, handling the negative case better fit the i2c and spi case.

Thanks

> regards,
> dan carpenter
> 
> 

Re: [PATCH 7/7] staging:iio:ade7854: Add proper error handling condition

[Dan Carpenter]

You're right that there is a bug but this is not the right fix.

The ade7854_i2c_write_reg_32() function returns 6 on success which makes
no sense.  It should be zero or negative error codes.  All the write_reg
functions in drivers/staging/iio/meter/ade7854-i2c.c have the same bug.

Please, fix that instead and leave ade7854_initial_setup() alone.

regards,
dan carpenter

Re: [PATCH 7/7] staging:iio:ade7854: Add proper error handling condition

[Rodrigo Siqueira]

On 03/16, Dan Carpenter wrote:
> You're right that there is a bug but this is not the right fix.
> 
> The ade7854_i2c_write_reg_32() function returns 6 on success which makes
> no sense.  It should be zero or negative error codes.  All the write_reg
> functions in drivers/staging/iio/meter/ade7854-i2c.c have the same bug.
> 
> Please, fix that instead and leave ade7854_initial_setup() alone.

I see. However, I think the following steps could be better:

1) Update ade7854_i2c_write_reg() in order to return 0

The primary objective of this patchset it removes the duplications
related to write_reg_* and read_reg_*. As a result, in the first patch
create the function ade7854_i2c_write_reg(). I will add the following
code to fix the problem that we are discussing:

ade7854_i2c_write_reg() {
...
ret = i2c_master_send(st->i2c, st->tx, count);
...
return ret < 0 ? ret : 0;
}

With this, I do not need to touch any of the
ade7854_i2c_write_reg_(8|16|24|32) functions. 

2) Drop out the last patch from the patchset

Is that ok? If so, I will send the V2 today.
 
> regards,
> dan carpenter
> 

Re: [PATCH 7/7] staging:iio:ade7854: Add proper error handling condition

[Dan Carpenter]

Generally, it's better to fix the bug in the existing code, and then
do the cleanup later.  That way the fixes can be backported to stable
kernels more easily.

I don't know this subsystem very well.  Perhaps Jonathan doesn't care
for one reason or another (like maybe he's not going to back port the
fix).  So it might not matter how you do it.

regards,
dan carpenter

Re: [PATCH 7/7] staging:iio:ade7854: Add proper error handling condition

[Rodrigo Siqueira]

On 03/16, Dan Carpenter wrote:
> Generally, it's better to fix the bug in the existing code, and then
> do the cleanup later.  That way the fixes can be backported to stable
> kernels more easily.

Hummm... the backport argue make a lot of sense for me. I will generate
a v2 with the bug fixes in the beginning.

Thanks 
> I don't know this subsystem very well.  Perhaps Jonathan doesn't care
> for one reason or another (like maybe he's not going to back port the
> fix).  So it might not matter how you do it.
> 
> regards,
> dan carpenter
> 
>