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

[PATCH v2 0/8] 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

Changes in V2:
 - Reorganize the patchset to make easier to backport fixes.
 - Adds two commits at the beginning of the patchset. First, fixes bugs
   related to wrong verification in read/write I2C operations. Second,
   adjust the incorrect amount of data read.
 - Removes unnecessary code in read/write functions for SPI and I2C
   during the rework.

Rodrigo Siqueira (8):
  staging:iio:ade7854: Fix error handling on read/write
  staging:iio:ade7854: Fix the wrong number of bits to read
  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

 drivers/staging/iio/meter/ade7854-i2c.c | 238 +++++++++-------------------
 drivers/staging/iio/meter/ade7854-spi.c | 268 +++++++-------------------------
 drivers/staging/iio/meter/ade7854.c     |  40 ++---
 drivers/staging/iio/meter/ade7854.h     |  23 +--
 4 files changed, 159 insertions(+), 410 deletions(-)

-- 
2.16.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 0/8] 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

Changes in V2:
 - Reorganize the patchset to make easier to backport fixes.
 - Adds two commits at the beginning of the patchset. First, fixes bugs
   related to wrong verification in read/write I2C operations. Second,
   adjust the incorrect amount of data read.
 - Removes unnecessary code in read/write functions for SPI and I2C
   during the rework.

Rodrigo Siqueira (8):
  staging:iio:ade7854: Fix error handling on read/write
  staging:iio:ade7854: Fix the wrong number of bits to read
  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

 drivers/staging/iio/meter/ade7854-i2c.c | 238 +++++++++-------------------
 drivers/staging/iio/meter/ade7854-spi.c | 268 +++++++-------------------------
 drivers/staging/iio/meter/ade7854.c     |  40 ++---
 drivers/staging/iio/meter/ade7854.h     |  23 +--
 4 files changed, 159 insertions(+), 410 deletions(-)

-- 
2.16.2

[PATCH v2 1/8] staging:iio:ade7854: Fix error handling on read/write

[Rodrigo Siqueira]

The original code does not correctly handle the error related to I2C
read and write. This patch fixes the error handling related to all
read/write functions for I2C. This patch is an adaptation of 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 | 24 ++++++++++++------------
 drivers/staging/iio/meter/ade7854.c     | 10 +++++-----
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
index 317e4f0d8176..4437f1e33261 100644
--- a/drivers/staging/iio/meter/ade7854-i2c.c
+++ b/drivers/staging/iio/meter/ade7854-i2c.c
@@ -31,7 +31,7 @@ static int ade7854_i2c_write_reg_8(struct device *dev,
 	ret = i2c_master_send(st->i2c, st->tx, 3);
 	mutex_unlock(&st->buf_lock);
 
-	return ret;
+	return ret < 0 ? ret : 0;
 }
 
 static int ade7854_i2c_write_reg_16(struct device *dev,
@@ -51,7 +51,7 @@ static int ade7854_i2c_write_reg_16(struct device *dev,
 	ret = i2c_master_send(st->i2c, st->tx, 4);
 	mutex_unlock(&st->buf_lock);
 
-	return ret;
+	return ret < 0 ? ret : 0;
 }
 
 static int ade7854_i2c_write_reg_24(struct device *dev,
@@ -72,7 +72,7 @@ static int ade7854_i2c_write_reg_24(struct device *dev,
 	ret = i2c_master_send(st->i2c, st->tx, 5);
 	mutex_unlock(&st->buf_lock);
 
-	return ret;
+	return ret < 0 ? ret : 0;
 }
 
 static int ade7854_i2c_write_reg_32(struct device *dev,
@@ -94,7 +94,7 @@ static int ade7854_i2c_write_reg_32(struct device *dev,
 	ret = i2c_master_send(st->i2c, st->tx, 6);
 	mutex_unlock(&st->buf_lock);
 
-	return ret;
+	return ret < 0 ? ret : 0;
 }
 
 static int ade7854_i2c_read_reg_8(struct device *dev,
@@ -110,11 +110,11 @@ 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)
+	if (ret < 0)
 		goto out;
 
 	ret = i2c_master_recv(st->i2c, st->rx, 1);
-	if (ret)
+	if (ret < 0)
 		goto out;
 
 	*val = st->rx[0];
@@ -136,11 +136,11 @@ static int ade7854_i2c_read_reg_16(struct device *dev,
 	st->tx[1] = reg_address & 0xFF;
 
 	ret = i2c_master_send(st->i2c, st->tx, 2);
-	if (ret)
+	if (ret < 0)
 		goto out;
 
 	ret = i2c_master_recv(st->i2c, st->rx, 2);
-	if (ret)
+	if (ret < 0)
 		goto out;
 
 	*val = (st->rx[0] << 8) | st->rx[1];
@@ -162,11 +162,11 @@ static int ade7854_i2c_read_reg_24(struct device *dev,
 	st->tx[1] = reg_address & 0xFF;
 
 	ret = i2c_master_send(st->i2c, st->tx, 2);
-	if (ret)
+	if (ret < 0)
 		goto out;
 
 	ret = i2c_master_recv(st->i2c, st->rx, 3);
-	if (ret)
+	if (ret < 0)
 		goto out;
 
 	*val = (st->rx[0] << 16) | (st->rx[1] << 8) | st->rx[2];
@@ -188,11 +188,11 @@ static int ade7854_i2c_read_reg_32(struct device *dev,
 	st->tx[1] = reg_address & 0xFF;
 
 	ret = i2c_master_send(st->i2c, st->tx, 2);
-	if (ret)
+	if (ret < 0)
 		goto out;
 
 	ret = i2c_master_recv(st->i2c, st->rx, 3);
-	if (ret)
+	if (ret < 0)
 		goto out;
 
 	*val = (st->rx[0] << 24) | (st->rx[1] << 16) |
diff --git a/drivers/staging/iio/meter/ade7854.c b/drivers/staging/iio/meter/ade7854.c
index 90d07cdca4b8..0193ae3aae29 100644
--- a/drivers/staging/iio/meter/ade7854.c
+++ b/drivers/staging/iio/meter/ade7854.c
@@ -33,7 +33,7 @@ static ssize_t ade7854_read_8bit(struct device *dev,
 	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
 
 	ret = st->read_reg_8(dev, this_attr->address, &val);
-	if (ret)
+	if (ret < 0)
 		return ret;
 
 	return sprintf(buf, "%u\n", val);
@@ -50,7 +50,7 @@ static ssize_t ade7854_read_16bit(struct device *dev,
 	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
 
 	ret = st->read_reg_16(dev, this_attr->address, &val);
-	if (ret)
+	if (ret < 0)
 		return ret;
 
 	return sprintf(buf, "%u\n", val);
@@ -67,7 +67,7 @@ static ssize_t ade7854_read_24bit(struct device *dev,
 	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
 
 	ret = st->read_reg_24(dev, this_attr->address, &val);
-	if (ret)
+	if (ret < 0)
 		return ret;
 
 	return sprintf(buf, "%u\n", val);
@@ -84,7 +84,7 @@ static ssize_t ade7854_read_32bit(struct device *dev,
 	struct ade7854_state *st = iio_priv(indio_dev);
 
 	ret = st->read_reg_32(dev, this_attr->address, &val);
-	if (ret)
+	if (ret < 0)
 		return ret;
 
 	return sprintf(buf, "%u\n", val);
@@ -416,7 +416,7 @@ static int ade7854_set_irq(struct device *dev, bool enable)
 	u32 irqen;
 
 	ret = st->read_reg_32(dev, ADE7854_MASK0, &irqen);
-	if (ret)
+	if (ret < 0)
 		return ret;
 
 	if (enable)
-- 
2.16.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 1/8] staging:iio:ade7854: Fix error handling on read/write

[Rodrigo Siqueira]

The original code does not correctly handle the error related to I2C
read and write. This patch fixes the error handling related to all
read/write functions for I2C. This patch is an adaptation of 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 | 24 ++++++++++++------------
 drivers/staging/iio/meter/ade7854.c     | 10 +++++-----
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
index 317e4f0d8176..4437f1e33261 100644
--- a/drivers/staging/iio/meter/ade7854-i2c.c
+++ b/drivers/staging/iio/meter/ade7854-i2c.c
@@ -31,7 +31,7 @@ static int ade7854_i2c_write_reg_8(struct device *dev,
 	ret = i2c_master_send(st->i2c, st->tx, 3);
 	mutex_unlock(&st->buf_lock);
 
-	return ret;
+	return ret < 0 ? ret : 0;
 }
 
 static int ade7854_i2c_write_reg_16(struct device *dev,
@@ -51,7 +51,7 @@ static int ade7854_i2c_write_reg_16(struct device *dev,
 	ret = i2c_master_send(st->i2c, st->tx, 4);
 	mutex_unlock(&st->buf_lock);
 
-	return ret;
+	return ret < 0 ? ret : 0;
 }
 
 static int ade7854_i2c_write_reg_24(struct device *dev,
@@ -72,7 +72,7 @@ static int ade7854_i2c_write_reg_24(struct device *dev,
 	ret = i2c_master_send(st->i2c, st->tx, 5);
 	mutex_unlock(&st->buf_lock);
 
-	return ret;
+	return ret < 0 ? ret : 0;
 }
 
 static int ade7854_i2c_write_reg_32(struct device *dev,
@@ -94,7 +94,7 @@ static int ade7854_i2c_write_reg_32(struct device *dev,
 	ret = i2c_master_send(st->i2c, st->tx, 6);
 	mutex_unlock(&st->buf_lock);
 
-	return ret;
+	return ret < 0 ? ret : 0;
 }
 
 static int ade7854_i2c_read_reg_8(struct device *dev,
@@ -110,11 +110,11 @@ 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)
+	if (ret < 0)
 		goto out;
 
 	ret = i2c_master_recv(st->i2c, st->rx, 1);
-	if (ret)
+	if (ret < 0)
 		goto out;
 
 	*val = st->rx[0];
@@ -136,11 +136,11 @@ static int ade7854_i2c_read_reg_16(struct device *dev,
 	st->tx[1] = reg_address & 0xFF;
 
 	ret = i2c_master_send(st->i2c, st->tx, 2);
-	if (ret)
+	if (ret < 0)
 		goto out;
 
 	ret = i2c_master_recv(st->i2c, st->rx, 2);
-	if (ret)
+	if (ret < 0)
 		goto out;
 
 	*val = (st->rx[0] << 8) | st->rx[1];
@@ -162,11 +162,11 @@ static int ade7854_i2c_read_reg_24(struct device *dev,
 	st->tx[1] = reg_address & 0xFF;
 
 	ret = i2c_master_send(st->i2c, st->tx, 2);
-	if (ret)
+	if (ret < 0)
 		goto out;
 
 	ret = i2c_master_recv(st->i2c, st->rx, 3);
-	if (ret)
+	if (ret < 0)
 		goto out;
 
 	*val = (st->rx[0] << 16) | (st->rx[1] << 8) | st->rx[2];
@@ -188,11 +188,11 @@ static int ade7854_i2c_read_reg_32(struct device *dev,
 	st->tx[1] = reg_address & 0xFF;
 
 	ret = i2c_master_send(st->i2c, st->tx, 2);
-	if (ret)
+	if (ret < 0)
 		goto out;
 
 	ret = i2c_master_recv(st->i2c, st->rx, 3);
-	if (ret)
+	if (ret < 0)
 		goto out;
 
 	*val = (st->rx[0] << 24) | (st->rx[1] << 16) |
diff --git a/drivers/staging/iio/meter/ade7854.c b/drivers/staging/iio/meter/ade7854.c
index 90d07cdca4b8..0193ae3aae29 100644
--- a/drivers/staging/iio/meter/ade7854.c
+++ b/drivers/staging/iio/meter/ade7854.c
@@ -33,7 +33,7 @@ static ssize_t ade7854_read_8bit(struct device *dev,
 	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
 
 	ret = st->read_reg_8(dev, this_attr->address, &val);
-	if (ret)
+	if (ret < 0)
 		return ret;
 
 	return sprintf(buf, "%u\n", val);
@@ -50,7 +50,7 @@ static ssize_t ade7854_read_16bit(struct device *dev,
 	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
 
 	ret = st->read_reg_16(dev, this_attr->address, &val);
-	if (ret)
+	if (ret < 0)
 		return ret;
 
 	return sprintf(buf, "%u\n", val);
@@ -67,7 +67,7 @@ static ssize_t ade7854_read_24bit(struct device *dev,
 	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
 
 	ret = st->read_reg_24(dev, this_attr->address, &val);
-	if (ret)
+	if (ret < 0)
 		return ret;
 
 	return sprintf(buf, "%u\n", val);
@@ -84,7 +84,7 @@ static ssize_t ade7854_read_32bit(struct device *dev,
 	struct ade7854_state *st = iio_priv(indio_dev);
 
 	ret = st->read_reg_32(dev, this_attr->address, &val);
-	if (ret)
+	if (ret < 0)
 		return ret;
 
 	return sprintf(buf, "%u\n", val);
@@ -416,7 +416,7 @@ static int ade7854_set_irq(struct device *dev, bool enable)
 	u32 irqen;
 
 	ret = st->read_reg_32(dev, ADE7854_MASK0, &irqen);
-	if (ret)
+	if (ret < 0)
 		return ret;
 
 	if (enable)
-- 
2.16.2

Re: [PATCH v2 1/8] staging:iio:ade7854: Fix error handling on read/write

[Jonathan Cameron]

On Fri, 16 Mar 2018 19:48:33 -0300
Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> wrote:

> The original code does not correctly handle the error related to I2C
> read and write. This patch fixes the error handling related to all
> read/write functions for I2C. This patch is an adaptation of the John
> Syne patches.
> 
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> Signed-off-by: John Syne <john3909@gmail.com>
Hi Rodrigo,

I'm not sure what the chain of authorship was here.  If this is fundamentally
John's original patch he should still be the author and his sign off should be
first.  You then sign off afterwards to indicate that you 'handled' the patch
and believe the work to be John's (you are trusting his sign off).  This
is 'fun' legal stuff - read the docs on developers certificate of origin.

If the patch has changed 'enough' (where that is a fuzzy definition)
then you should as you have here take the authorship, but John's sign off is
no longer true (it's a different patch).  If John has reviewed the code
it is fine to have a reviewed-by or acked-by from John there to reflect
that.

Anyhow, please clarify the situation as I shouldn't take a patch where
I'm applying my sign-off without knowing the origins etc.

> ---
>  drivers/staging/iio/meter/ade7854-i2c.c | 24 ++++++++++++------------
>  drivers/staging/iio/meter/ade7854.c     | 10 +++++-----
>  2 files changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
> index 317e4f0d8176..4437f1e33261 100644
> --- a/drivers/staging/iio/meter/ade7854-i2c.c
> +++ b/drivers/staging/iio/meter/ade7854-i2c.c
> @@ -31,7 +31,7 @@ static int ade7854_i2c_write_reg_8(struct device *dev,
>  	ret = i2c_master_send(st->i2c, st->tx, 3);
>  	mutex_unlock(&st->buf_lock);
>  
> -	return ret;
> +	return ret < 0 ? ret : 0;
>  }
>  
>  static int ade7854_i2c_write_reg_16(struct device *dev,
> @@ -51,7 +51,7 @@ static int ade7854_i2c_write_reg_16(struct device *dev,
>  	ret = i2c_master_send(st->i2c, st->tx, 4);
>  	mutex_unlock(&st->buf_lock);
>  
> -	return ret;
> +	return ret < 0 ? ret : 0;
>  }
>  
>  static int ade7854_i2c_write_reg_24(struct device *dev,
> @@ -72,7 +72,7 @@ static int ade7854_i2c_write_reg_24(struct device *dev,
>  	ret = i2c_master_send(st->i2c, st->tx, 5);
>  	mutex_unlock(&st->buf_lock);
>  
> -	return ret;
> +	return ret < 0 ? ret : 0;
>  }
>  
>  static int ade7854_i2c_write_reg_32(struct device *dev,
> @@ -94,7 +94,7 @@ static int ade7854_i2c_write_reg_32(struct device *dev,
>  	ret = i2c_master_send(st->i2c, st->tx, 6);
>  	mutex_unlock(&st->buf_lock);
>  
> -	return ret;
> +	return ret < 0 ? ret : 0;
>  }
So for write cases you are flattening to 0 for good and < 0 for bad.
good.
>  
>  static int ade7854_i2c_read_reg_8(struct device *dev,
> @@ -110,11 +110,11 @@ 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)
> +	if (ret < 0)
>  		goto out;
>  
>  	ret = i2c_master_recv(st->i2c, st->rx, 1);
> -	if (ret)
> +	if (ret < 0)
>  		goto out;
>  
>  	*val = st->rx[0];
But in read cases you are returning the number of bytes read...
Given these functions can know the 'right' answer to that why not check
it here and do the same as for writes in return 0 for good and < 0 for
bad?
> @@ -136,11 +136,11 @@ static int ade7854_i2c_read_reg_16(struct device *dev,
>  	st->tx[1] = reg_address & 0xFF;
>  
>  	ret = i2c_master_send(st->i2c, st->tx, 2);
> -	if (ret)
> +	if (ret < 0)
>  		goto out;
>  
>  	ret = i2c_master_recv(st->i2c, st->rx, 2);
> -	if (ret)
> +	if (ret < 0)
>  		goto out;
>  
>  	*val = (st->rx[0] << 8) | st->rx[1];
> @@ -162,11 +162,11 @@ static int ade7854_i2c_read_reg_24(struct device *dev,
>  	st->tx[1] = reg_address & 0xFF;
>  
>  	ret = i2c_master_send(st->i2c, st->tx, 2);
> -	if (ret)
> +	if (ret < 0)
>  		goto out;
>  
>  	ret = i2c_master_recv(st->i2c, st->rx, 3);
> -	if (ret)
> +	if (ret < 0)
>  		goto out;
>  
>  	*val = (st->rx[0] << 16) | (st->rx[1] << 8) | st->rx[2];
> @@ -188,11 +188,11 @@ static int ade7854_i2c_read_reg_32(struct device *dev,
>  	st->tx[1] = reg_address & 0xFF;
>  
>  	ret = i2c_master_send(st->i2c, st->tx, 2);
> -	if (ret)
> +	if (ret < 0)
>  		goto out;
>  
>  	ret = i2c_master_recv(st->i2c, st->rx, 3);
> -	if (ret)
> +	if (ret < 0)
>  		goto out;
>  
>  	*val = (st->rx[0] << 24) | (st->rx[1] << 16) |
> diff --git a/drivers/staging/iio/meter/ade7854.c b/drivers/staging/iio/meter/ade7854.c
> index 90d07cdca4b8..0193ae3aae29 100644
> --- a/drivers/staging/iio/meter/ade7854.c
> +++ b/drivers/staging/iio/meter/ade7854.c
> @@ -33,7 +33,7 @@ static ssize_t ade7854_read_8bit(struct device *dev,
>  	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>  
>  	ret = st->read_reg_8(dev, this_attr->address, &val);
> -	if (ret)
> +	if (ret < 0)
If you did as discussed above with the reads then this change would not
be needed and all the changes would be confined to the i2c code.

Thanks,

Jonathan


>  		return ret;
>  
>  	return sprintf(buf, "%u\n", val);
> @@ -50,7 +50,7 @@ static ssize_t ade7854_read_16bit(struct device *dev,
>  	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>  
>  	ret = st->read_reg_16(dev, this_attr->address, &val);
> -	if (ret)
> +	if (ret < 0)
>  		return ret;
>  
>  	return sprintf(buf, "%u\n", val);
> @@ -67,7 +67,7 @@ static ssize_t ade7854_read_24bit(struct device *dev,
>  	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>  
>  	ret = st->read_reg_24(dev, this_attr->address, &val);
> -	if (ret)
> +	if (ret < 0)
>  		return ret;
>  
>  	return sprintf(buf, "%u\n", val);
> @@ -84,7 +84,7 @@ static ssize_t ade7854_read_32bit(struct device *dev,
>  	struct ade7854_state *st = iio_priv(indio_dev);
>  
>  	ret = st->read_reg_32(dev, this_attr->address, &val);
> -	if (ret)
> +	if (ret < 0)
>  		return ret;
>  
>  	return sprintf(buf, "%u\n", val);
> @@ -416,7 +416,7 @@ static int ade7854_set_irq(struct device *dev, bool enable)
>  	u32 irqen;
>  
>  	ret = st->read_reg_32(dev, ADE7854_MASK0, &irqen);
> -	if (ret)
> +	if (ret < 0)
>  		return ret;
>  
>  	if (enable)

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 1/8] staging:iio:ade7854: Fix error handling on read/write

[Jonathan Cameron]

On Fri, 16 Mar 2018 19:48:33 -0300
Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> wrote:

> The original code does not correctly handle the error related to I2C
> read and write. This patch fixes the error handling related to all
> read/write functions for I2C. This patch is an adaptation of the John
> Syne patches.
> 
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> Signed-off-by: John Syne <john3909@gmail.com>
Hi Rodrigo,

I'm not sure what the chain of authorship was here.  If this is fundamentally
John's original patch he should still be the author and his sign off should be
first.  You then sign off afterwards to indicate that you 'handled' the patch
and believe the work to be John's (you are trusting his sign off).  This
is 'fun' legal stuff - read the docs on developers certificate of origin.

If the patch has changed 'enough' (where that is a fuzzy definition)
then you should as you have here take the authorship, but John's sign off is
no longer true (it's a different patch).  If John has reviewed the code
it is fine to have a reviewed-by or acked-by from John there to reflect
that.

Anyhow, please clarify the situation as I shouldn't take a patch where
I'm applying my sign-off without knowing the origins etc.

> ---
>  drivers/staging/iio/meter/ade7854-i2c.c | 24 ++++++++++++------------
>  drivers/staging/iio/meter/ade7854.c     | 10 +++++-----
>  2 files changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
> index 317e4f0d8176..4437f1e33261 100644
> --- a/drivers/staging/iio/meter/ade7854-i2c.c
> +++ b/drivers/staging/iio/meter/ade7854-i2c.c
> @@ -31,7 +31,7 @@ static int ade7854_i2c_write_reg_8(struct device *dev,
>  	ret = i2c_master_send(st->i2c, st->tx, 3);
>  	mutex_unlock(&st->buf_lock);
>  
> -	return ret;
> +	return ret < 0 ? ret : 0;
>  }
>  
>  static int ade7854_i2c_write_reg_16(struct device *dev,
> @@ -51,7 +51,7 @@ static int ade7854_i2c_write_reg_16(struct device *dev,
>  	ret = i2c_master_send(st->i2c, st->tx, 4);
>  	mutex_unlock(&st->buf_lock);
>  
> -	return ret;
> +	return ret < 0 ? ret : 0;
>  }
>  
>  static int ade7854_i2c_write_reg_24(struct device *dev,
> @@ -72,7 +72,7 @@ static int ade7854_i2c_write_reg_24(struct device *dev,
>  	ret = i2c_master_send(st->i2c, st->tx, 5);
>  	mutex_unlock(&st->buf_lock);
>  
> -	return ret;
> +	return ret < 0 ? ret : 0;
>  }
>  
>  static int ade7854_i2c_write_reg_32(struct device *dev,
> @@ -94,7 +94,7 @@ static int ade7854_i2c_write_reg_32(struct device *dev,
>  	ret = i2c_master_send(st->i2c, st->tx, 6);
>  	mutex_unlock(&st->buf_lock);
>  
> -	return ret;
> +	return ret < 0 ? ret : 0;
>  }
So for write cases you are flattening to 0 for good and < 0 for bad.
good.
>  
>  static int ade7854_i2c_read_reg_8(struct device *dev,
> @@ -110,11 +110,11 @@ 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)
> +	if (ret < 0)
>  		goto out;
>  
>  	ret = i2c_master_recv(st->i2c, st->rx, 1);
> -	if (ret)
> +	if (ret < 0)
>  		goto out;
>  
>  	*val = st->rx[0];
But in read cases you are returning the number of bytes read...
Given these functions can know the 'right' answer to that why not check
it here and do the same as for writes in return 0 for good and < 0 for
bad?
> @@ -136,11 +136,11 @@ static int ade7854_i2c_read_reg_16(struct device *dev,
>  	st->tx[1] = reg_address & 0xFF;
>  
>  	ret = i2c_master_send(st->i2c, st->tx, 2);
> -	if (ret)
> +	if (ret < 0)
>  		goto out;
>  
>  	ret = i2c_master_recv(st->i2c, st->rx, 2);
> -	if (ret)
> +	if (ret < 0)
>  		goto out;
>  
>  	*val = (st->rx[0] << 8) | st->rx[1];
> @@ -162,11 +162,11 @@ static int ade7854_i2c_read_reg_24(struct device *dev,
>  	st->tx[1] = reg_address & 0xFF;
>  
>  	ret = i2c_master_send(st->i2c, st->tx, 2);
> -	if (ret)
> +	if (ret < 0)
>  		goto out;
>  
>  	ret = i2c_master_recv(st->i2c, st->rx, 3);
> -	if (ret)
> +	if (ret < 0)
>  		goto out;
>  
>  	*val = (st->rx[0] << 16) | (st->rx[1] << 8) | st->rx[2];
> @@ -188,11 +188,11 @@ static int ade7854_i2c_read_reg_32(struct device *dev,
>  	st->tx[1] = reg_address & 0xFF;
>  
>  	ret = i2c_master_send(st->i2c, st->tx, 2);
> -	if (ret)
> +	if (ret < 0)
>  		goto out;
>  
>  	ret = i2c_master_recv(st->i2c, st->rx, 3);
> -	if (ret)
> +	if (ret < 0)
>  		goto out;
>  
>  	*val = (st->rx[0] << 24) | (st->rx[1] << 16) |
> diff --git a/drivers/staging/iio/meter/ade7854.c b/drivers/staging/iio/meter/ade7854.c
> index 90d07cdca4b8..0193ae3aae29 100644
> --- a/drivers/staging/iio/meter/ade7854.c
> +++ b/drivers/staging/iio/meter/ade7854.c
> @@ -33,7 +33,7 @@ static ssize_t ade7854_read_8bit(struct device *dev,
>  	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>  
>  	ret = st->read_reg_8(dev, this_attr->address, &val);
> -	if (ret)
> +	if (ret < 0)
If you did as discussed above with the reads then this change would not
be needed and all the changes would be confined to the i2c code.

Thanks,

Jonathan


>  		return ret;
>  
>  	return sprintf(buf, "%u\n", val);
> @@ -50,7 +50,7 @@ static ssize_t ade7854_read_16bit(struct device *dev,
>  	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>  
>  	ret = st->read_reg_16(dev, this_attr->address, &val);
> -	if (ret)
> +	if (ret < 0)
>  		return ret;
>  
>  	return sprintf(buf, "%u\n", val);
> @@ -67,7 +67,7 @@ static ssize_t ade7854_read_24bit(struct device *dev,
>  	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>  
>  	ret = st->read_reg_24(dev, this_attr->address, &val);
> -	if (ret)
> +	if (ret < 0)
>  		return ret;
>  
>  	return sprintf(buf, "%u\n", val);
> @@ -84,7 +84,7 @@ static ssize_t ade7854_read_32bit(struct device *dev,
>  	struct ade7854_state *st = iio_priv(indio_dev);
>  
>  	ret = st->read_reg_32(dev, this_attr->address, &val);
> -	if (ret)
> +	if (ret < 0)
>  		return ret;
>  
>  	return sprintf(buf, "%u\n", val);
> @@ -416,7 +416,7 @@ static int ade7854_set_irq(struct device *dev, bool enable)
>  	u32 irqen;
>  
>  	ret = st->read_reg_32(dev, ADE7854_MASK0, &irqen);
> -	if (ret)
> +	if (ret < 0)
>  		return ret;
>  
>  	if (enable)

Re: [PATCH v2 1/8] staging:iio:ade7854: Fix error handling on read/write

[Rodrigo Siqueira]

On 03/18, Jonathan Cameron wrote:
> On Fri, 16 Mar 2018 19:48:33 -0300
> Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> wrote:
> 
> > The original code does not correctly handle the error related to I2C
> > read and write. This patch fixes the error handling related to all
> > read/write functions for I2C. This patch is an adaptation of the John
> > Syne patches.
> > 
> > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > Signed-off-by: John Syne <john3909@gmail.com>
> Hi Rodrigo,
> 
> I'm not sure what the chain of authorship was here.  If this is fundamentally
> John's original patch he should still be the author and his sign off should be
> first.  You then sign off afterwards to indicate that you 'handled' the patch
> and believe the work to be John's (you are trusting his sign off).  This
> is 'fun' legal stuff - read the docs on developers certificate of origin.
> 
> If the patch has changed 'enough' (where that is a fuzzy definition)
> then you should as you have here take the authorship, but John's sign off is
> no longer true (it's a different patch).  If John has reviewed the code
> it is fine to have a reviewed-by or acked-by from John there to reflect
> that.
> 
> Anyhow, please clarify the situation as I shouldn't take a patch where
> I'm applying my sign-off without knowing the origins etc.

Hi Jonathan,

Just for clarification, this is fundamentally John's original patch with
some changes on the way that write_reg operation returns the error. I
should ask for someone else, how to correctly handle this situation
since I did not have experience with this situation.

Actually, when I worked on this patch, I was confused about using
different authorship from the email. I got confused because of the
following statement:

"Make sure that the email you specify here is the same email you used to
set up sending mail. The Linux kernel developers will not accept a patch
where the "From" email differs from the "Signed-off-by" line, which is
what will happen if these two emails do not match." [1]

Anyway, I think this is not a newbie issue, and I should asked first.
Thanks for the great explanation, I will not make this kind of mistake
again.

Thanks

[1] - https://kernelnewbies.org/FirstKernelPatch
 
> > ---
> >  drivers/staging/iio/meter/ade7854-i2c.c | 24 ++++++++++++------------
> >  drivers/staging/iio/meter/ade7854.c     | 10 +++++-----
> >  2 files changed, 17 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
> > index 317e4f0d8176..4437f1e33261 100644
> > --- a/drivers/staging/iio/meter/ade7854-i2c.c
> > +++ b/drivers/staging/iio/meter/ade7854-i2c.c
> > @@ -31,7 +31,7 @@ static int ade7854_i2c_write_reg_8(struct device *dev,
> >  	ret = i2c_master_send(st->i2c, st->tx, 3);
> >  	mutex_unlock(&st->buf_lock);
> >  
> > -	return ret;
> > +	return ret < 0 ? ret : 0;
> >  }
> >  
> >  static int ade7854_i2c_write_reg_16(struct device *dev,
> > @@ -51,7 +51,7 @@ static int ade7854_i2c_write_reg_16(struct device *dev,
> >  	ret = i2c_master_send(st->i2c, st->tx, 4);
> >  	mutex_unlock(&st->buf_lock);
> >  
> > -	return ret;
> > +	return ret < 0 ? ret : 0;
> >  }
> >  
> >  static int ade7854_i2c_write_reg_24(struct device *dev,
> > @@ -72,7 +72,7 @@ static int ade7854_i2c_write_reg_24(struct device *dev,
> >  	ret = i2c_master_send(st->i2c, st->tx, 5);
> >  	mutex_unlock(&st->buf_lock);
> >  
> > -	return ret;
> > +	return ret < 0 ? ret : 0;
> >  }
> >  
> >  static int ade7854_i2c_write_reg_32(struct device *dev,
> > @@ -94,7 +94,7 @@ static int ade7854_i2c_write_reg_32(struct device *dev,
> >  	ret = i2c_master_send(st->i2c, st->tx, 6);
> >  	mutex_unlock(&st->buf_lock);
> >  
> > -	return ret;
> > +	return ret < 0 ? ret : 0;
> >  }
> So for write cases you are flattening to 0 for good and < 0 for bad.
> good.
> >  
> >  static int ade7854_i2c_read_reg_8(struct device *dev,
> > @@ -110,11 +110,11 @@ 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)
> > +	if (ret < 0)
> >  		goto out;
> >  
> >  	ret = i2c_master_recv(st->i2c, st->rx, 1);
> > -	if (ret)
> > +	if (ret < 0)
> >  		goto out;
> >  
> >  	*val = st->rx[0];
> But in read cases you are returning the number of bytes read...
> Given these functions can know the 'right' answer to that why not check
> it here and do the same as for writes in return 0 for good and < 0 for
> bad?
> > @@ -136,11 +136,11 @@ static int ade7854_i2c_read_reg_16(struct device *dev,
> >  	st->tx[1] = reg_address & 0xFF;
> >  
> >  	ret = i2c_master_send(st->i2c, st->tx, 2);
> > -	if (ret)
> > +	if (ret < 0)
> >  		goto out;
> >  
> >  	ret = i2c_master_recv(st->i2c, st->rx, 2);
> > -	if (ret)
> > +	if (ret < 0)
> >  		goto out;
> >  
> >  	*val = (st->rx[0] << 8) | st->rx[1];
> > @@ -162,11 +162,11 @@ static int ade7854_i2c_read_reg_24(struct device *dev,
> >  	st->tx[1] = reg_address & 0xFF;
> >  
> >  	ret = i2c_master_send(st->i2c, st->tx, 2);
> > -	if (ret)
> > +	if (ret < 0)
> >  		goto out;
> >  
> >  	ret = i2c_master_recv(st->i2c, st->rx, 3);
> > -	if (ret)
> > +	if (ret < 0)
> >  		goto out;
> >  
> >  	*val = (st->rx[0] << 16) | (st->rx[1] << 8) | st->rx[2];
> > @@ -188,11 +188,11 @@ static int ade7854_i2c_read_reg_32(struct device *dev,
> >  	st->tx[1] = reg_address & 0xFF;
> >  
> >  	ret = i2c_master_send(st->i2c, st->tx, 2);
> > -	if (ret)
> > +	if (ret < 0)
> >  		goto out;
> >  
> >  	ret = i2c_master_recv(st->i2c, st->rx, 3);
> > -	if (ret)
> > +	if (ret < 0)
> >  		goto out;
> >  
> >  	*val = (st->rx[0] << 24) | (st->rx[1] << 16) |
> > diff --git a/drivers/staging/iio/meter/ade7854.c b/drivers/staging/iio/meter/ade7854.c
> > index 90d07cdca4b8..0193ae3aae29 100644
> > --- a/drivers/staging/iio/meter/ade7854.c
> > +++ b/drivers/staging/iio/meter/ade7854.c
> > @@ -33,7 +33,7 @@ static ssize_t ade7854_read_8bit(struct device *dev,
> >  	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> >  
> >  	ret = st->read_reg_8(dev, this_attr->address, &val);
> > -	if (ret)
> > +	if (ret < 0)
> If you did as discussed above with the reads then this change would not
> be needed and all the changes would be confined to the i2c code.
> 
> Thanks,
> 
> Jonathan
> 
> 
> >  		return ret;
> >  
> >  	return sprintf(buf, "%u\n", val);
> > @@ -50,7 +50,7 @@ static ssize_t ade7854_read_16bit(struct device *dev,
> >  	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> >  
> >  	ret = st->read_reg_16(dev, this_attr->address, &val);
> > -	if (ret)
> > +	if (ret < 0)
> >  		return ret;
> >  
> >  	return sprintf(buf, "%u\n", val);
> > @@ -67,7 +67,7 @@ static ssize_t ade7854_read_24bit(struct device *dev,
> >  	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> >  
> >  	ret = st->read_reg_24(dev, this_attr->address, &val);
> > -	if (ret)
> > +	if (ret < 0)
> >  		return ret;
> >  
> >  	return sprintf(buf, "%u\n", val);
> > @@ -84,7 +84,7 @@ static ssize_t ade7854_read_32bit(struct device *dev,
> >  	struct ade7854_state *st = iio_priv(indio_dev);
> >  
> >  	ret = st->read_reg_32(dev, this_attr->address, &val);
> > -	if (ret)
> > +	if (ret < 0)
> >  		return ret;
> >  
> >  	return sprintf(buf, "%u\n", val);
> > @@ -416,7 +416,7 @@ static int ade7854_set_irq(struct device *dev, bool enable)
> >  	u32 irqen;
> >  
> >  	ret = st->read_reg_32(dev, ADE7854_MASK0, &irqen);
> > -	if (ret)
> > +	if (ret < 0)
> >  		return ret;
> >  
> >  	if (enable)
> 
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 1/8] staging:iio:ade7854: Fix error handling on read/write

[Rodrigo Siqueira]

On 03/18, Jonathan Cameron wrote:
> On Fri, 16 Mar 2018 19:48:33 -0300
> Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> wrote:
> 
> > The original code does not correctly handle the error related to I2C
> > read and write. This patch fixes the error handling related to all
> > read/write functions for I2C. This patch is an adaptation of the John
> > Syne patches.
> > 
> > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > Signed-off-by: John Syne <john3909@gmail.com>
> Hi Rodrigo,
> 
> I'm not sure what the chain of authorship was here.  If this is fundamentally
> John's original patch he should still be the author and his sign off should be
> first.  You then sign off afterwards to indicate that you 'handled' the patch
> and believe the work to be John's (you are trusting his sign off).  This
> is 'fun' legal stuff - read the docs on developers certificate of origin.
> 
> If the patch has changed 'enough' (where that is a fuzzy definition)
> then you should as you have here take the authorship, but John's sign off is
> no longer true (it's a different patch).  If John has reviewed the code
> it is fine to have a reviewed-by or acked-by from John there to reflect
> that.
> 
> Anyhow, please clarify the situation as I shouldn't take a patch where
> I'm applying my sign-off without knowing the origins etc.

Hi Jonathan,

Just for clarification, this is fundamentally John's original patch with
some changes on the way that write_reg operation returns the error. I
should ask for someone else, how to correctly handle this situation
since I did not have experience with this situation.

Actually, when I worked on this patch, I was confused about using
different authorship from the email. I got confused because of the
following statement:

"Make sure that the email you specify here is the same email you used to
set up sending mail. The Linux kernel developers will not accept a patch
where the "From" email differs from the "Signed-off-by" line, which is
what will happen if these two emails do not match." [1]

Anyway, I think this is not a newbie issue, and I should asked first.
Thanks for the great explanation, I will not make this kind of mistake
again.

Thanks

[1] - https://kernelnewbies.org/FirstKernelPatch
 
> > ---
> >  drivers/staging/iio/meter/ade7854-i2c.c | 24 ++++++++++++------------
> >  drivers/staging/iio/meter/ade7854.c     | 10 +++++-----
> >  2 files changed, 17 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
> > index 317e4f0d8176..4437f1e33261 100644
> > --- a/drivers/staging/iio/meter/ade7854-i2c.c
> > +++ b/drivers/staging/iio/meter/ade7854-i2c.c
> > @@ -31,7 +31,7 @@ static int ade7854_i2c_write_reg_8(struct device *dev,
> >  	ret = i2c_master_send(st->i2c, st->tx, 3);
> >  	mutex_unlock(&st->buf_lock);
> >  
> > -	return ret;
> > +	return ret < 0 ? ret : 0;
> >  }
> >  
> >  static int ade7854_i2c_write_reg_16(struct device *dev,
> > @@ -51,7 +51,7 @@ static int ade7854_i2c_write_reg_16(struct device *dev,
> >  	ret = i2c_master_send(st->i2c, st->tx, 4);
> >  	mutex_unlock(&st->buf_lock);
> >  
> > -	return ret;
> > +	return ret < 0 ? ret : 0;
> >  }
> >  
> >  static int ade7854_i2c_write_reg_24(struct device *dev,
> > @@ -72,7 +72,7 @@ static int ade7854_i2c_write_reg_24(struct device *dev,
> >  	ret = i2c_master_send(st->i2c, st->tx, 5);
> >  	mutex_unlock(&st->buf_lock);
> >  
> > -	return ret;
> > +	return ret < 0 ? ret : 0;
> >  }
> >  
> >  static int ade7854_i2c_write_reg_32(struct device *dev,
> > @@ -94,7 +94,7 @@ static int ade7854_i2c_write_reg_32(struct device *dev,
> >  	ret = i2c_master_send(st->i2c, st->tx, 6);
> >  	mutex_unlock(&st->buf_lock);
> >  
> > -	return ret;
> > +	return ret < 0 ? ret : 0;
> >  }
> So for write cases you are flattening to 0 for good and < 0 for bad.
> good.
> >  
> >  static int ade7854_i2c_read_reg_8(struct device *dev,
> > @@ -110,11 +110,11 @@ 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)
> > +	if (ret < 0)
> >  		goto out;
> >  
> >  	ret = i2c_master_recv(st->i2c, st->rx, 1);
> > -	if (ret)
> > +	if (ret < 0)
> >  		goto out;
> >  
> >  	*val = st->rx[0];
> But in read cases you are returning the number of bytes read...
> Given these functions can know the 'right' answer to that why not check
> it here and do the same as for writes in return 0 for good and < 0 for
> bad?
> > @@ -136,11 +136,11 @@ static int ade7854_i2c_read_reg_16(struct device *dev,
> >  	st->tx[1] = reg_address & 0xFF;
> >  
> >  	ret = i2c_master_send(st->i2c, st->tx, 2);
> > -	if (ret)
> > +	if (ret < 0)
> >  		goto out;
> >  
> >  	ret = i2c_master_recv(st->i2c, st->rx, 2);
> > -	if (ret)
> > +	if (ret < 0)
> >  		goto out;
> >  
> >  	*val = (st->rx[0] << 8) | st->rx[1];
> > @@ -162,11 +162,11 @@ static int ade7854_i2c_read_reg_24(struct device *dev,
> >  	st->tx[1] = reg_address & 0xFF;
> >  
> >  	ret = i2c_master_send(st->i2c, st->tx, 2);
> > -	if (ret)
> > +	if (ret < 0)
> >  		goto out;
> >  
> >  	ret = i2c_master_recv(st->i2c, st->rx, 3);
> > -	if (ret)
> > +	if (ret < 0)
> >  		goto out;
> >  
> >  	*val = (st->rx[0] << 16) | (st->rx[1] << 8) | st->rx[2];
> > @@ -188,11 +188,11 @@ static int ade7854_i2c_read_reg_32(struct device *dev,
> >  	st->tx[1] = reg_address & 0xFF;
> >  
> >  	ret = i2c_master_send(st->i2c, st->tx, 2);
> > -	if (ret)
> > +	if (ret < 0)
> >  		goto out;
> >  
> >  	ret = i2c_master_recv(st->i2c, st->rx, 3);
> > -	if (ret)
> > +	if (ret < 0)
> >  		goto out;
> >  
> >  	*val = (st->rx[0] << 24) | (st->rx[1] << 16) |
> > diff --git a/drivers/staging/iio/meter/ade7854.c b/drivers/staging/iio/meter/ade7854.c
> > index 90d07cdca4b8..0193ae3aae29 100644
> > --- a/drivers/staging/iio/meter/ade7854.c
> > +++ b/drivers/staging/iio/meter/ade7854.c
> > @@ -33,7 +33,7 @@ static ssize_t ade7854_read_8bit(struct device *dev,
> >  	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> >  
> >  	ret = st->read_reg_8(dev, this_attr->address, &val);
> > -	if (ret)
> > +	if (ret < 0)
> If you did as discussed above with the reads then this change would not
> be needed and all the changes would be confined to the i2c code.
> 
> Thanks,
> 
> Jonathan
> 
> 
> >  		return ret;
> >  
> >  	return sprintf(buf, "%u\n", val);
> > @@ -50,7 +50,7 @@ static ssize_t ade7854_read_16bit(struct device *dev,
> >  	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> >  
> >  	ret = st->read_reg_16(dev, this_attr->address, &val);
> > -	if (ret)
> > +	if (ret < 0)
> >  		return ret;
> >  
> >  	return sprintf(buf, "%u\n", val);
> > @@ -67,7 +67,7 @@ static ssize_t ade7854_read_24bit(struct device *dev,
> >  	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> >  
> >  	ret = st->read_reg_24(dev, this_attr->address, &val);
> > -	if (ret)
> > +	if (ret < 0)
> >  		return ret;
> >  
> >  	return sprintf(buf, "%u\n", val);
> > @@ -84,7 +84,7 @@ static ssize_t ade7854_read_32bit(struct device *dev,
> >  	struct ade7854_state *st = iio_priv(indio_dev);
> >  
> >  	ret = st->read_reg_32(dev, this_attr->address, &val);
> > -	if (ret)
> > +	if (ret < 0)
> >  		return ret;
> >  
> >  	return sprintf(buf, "%u\n", val);
> > @@ -416,7 +416,7 @@ static int ade7854_set_irq(struct device *dev, bool enable)
> >  	u32 irqen;
> >  
> >  	ret = st->read_reg_32(dev, ADE7854_MASK0, &irqen);
> > -	if (ret)
> > +	if (ret < 0)
> >  		return ret;
> >  
> >  	if (enable)
> 

[PATCH v2 2/8] staging:iio:ade7854: Fix the wrong number of bits to read

[Rodrigo Siqueira]

The function ade7854_i2c_read_reg_32() have to invoke the
i2c_master_recv() for read 32 bits values, however, the counter is set
to 3 which means 24 bits. This patch fixes the wrong size of 24 bits, to
32 bits. Finally, this patch is based on 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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
index 4437f1e33261..37c957482493 100644
--- a/drivers/staging/iio/meter/ade7854-i2c.c
+++ b/drivers/staging/iio/meter/ade7854-i2c.c
@@ -191,7 +191,7 @@ static int ade7854_i2c_read_reg_32(struct device *dev,
 	if (ret < 0)
 		goto out;
 
-	ret = i2c_master_recv(st->i2c, st->rx, 3);
+	ret = i2c_master_recv(st->i2c, st->rx, 4);
 	if (ret < 0)
 		goto out;
 
-- 
2.16.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 2/8] staging:iio:ade7854: Fix the wrong number of bits to read

[Rodrigo Siqueira]

The function ade7854_i2c_read_reg_32() have to invoke the
i2c_master_recv() for read 32 bits values, however, the counter is set
to 3 which means 24 bits. This patch fixes the wrong size of 24 bits, to
32 bits. Finally, this patch is based on 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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
index 4437f1e33261..37c957482493 100644
--- a/drivers/staging/iio/meter/ade7854-i2c.c
+++ b/drivers/staging/iio/meter/ade7854-i2c.c
@@ -191,7 +191,7 @@ static int ade7854_i2c_read_reg_32(struct device *dev,
 	if (ret < 0)
 		goto out;
 
-	ret = i2c_master_recv(st->i2c, st->rx, 3);
+	ret = i2c_master_recv(st->i2c, st->rx, 4);
 	if (ret < 0)
 		goto out;
 
-- 
2.16.2

Re: [PATCH v2 2/8] staging:iio:ade7854: Fix the wrong number of bits to read

[Jonathan Cameron]

On Fri, 16 Mar 2018 19:48:51 -0300
Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> wrote:

> The function ade7854_i2c_read_reg_32() have to invoke the
> i2c_master_recv() for read 32 bits values, however, the counter is set
> to 3 which means 24 bits. This patch fixes the wrong size of 24 bits, to
> 32 bits. Finally, this patch is based on John Syne patches.
> 
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> Signed-off-by: John Syne <john3909@gmail.com>
Same issue with clarifying the authorship.  Also for these fixes
ideally include a 'fixes' tag to the original commit. It makes
life easy for the scripts the stable maintainers use to work
out when things should apply.

Here I would imagine it is the original commit that added the driver,
though we may have had enough changes over the years to make these
hard to apply.

Jonathan

> ---
>  drivers/staging/iio/meter/ade7854-i2c.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
> index 4437f1e33261..37c957482493 100644
> --- a/drivers/staging/iio/meter/ade7854-i2c.c
> +++ b/drivers/staging/iio/meter/ade7854-i2c.c
> @@ -191,7 +191,7 @@ static int ade7854_i2c_read_reg_32(struct device *dev,
>  	if (ret < 0)
>  		goto out;
>  
> -	ret = i2c_master_recv(st->i2c, st->rx, 3);
> +	ret = i2c_master_recv(st->i2c, st->rx, 4);
>  	if (ret < 0)
>  		goto out;
>  

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 2/8] staging:iio:ade7854: Fix the wrong number of bits to read

[Jonathan Cameron]

On Fri, 16 Mar 2018 19:48:51 -0300
Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> wrote:

> The function ade7854_i2c_read_reg_32() have to invoke the
> i2c_master_recv() for read 32 bits values, however, the counter is set
> to 3 which means 24 bits. This patch fixes the wrong size of 24 bits, to
> 32 bits. Finally, this patch is based on John Syne patches.
> 
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> Signed-off-by: John Syne <john3909@gmail.com>
Same issue with clarifying the authorship.  Also for these fixes
ideally include a 'fixes' tag to the original commit. It makes
life easy for the scripts the stable maintainers use to work
out when things should apply.

Here I would imagine it is the original commit that added the driver,
though we may have had enough changes over the years to make these
hard to apply.

Jonathan

> ---
>  drivers/staging/iio/meter/ade7854-i2c.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
> index 4437f1e33261..37c957482493 100644
> --- a/drivers/staging/iio/meter/ade7854-i2c.c
> +++ b/drivers/staging/iio/meter/ade7854-i2c.c
> @@ -191,7 +191,7 @@ static int ade7854_i2c_read_reg_32(struct device *dev,
>  	if (ret < 0)
>  		goto out;
>  
> -	ret = i2c_master_recv(st->i2c, st->rx, 3);
> +	ret = i2c_master_recv(st->i2c, st->rx, 4);
>  	if (ret < 0)
>  		goto out;
>  

[PATCH v2 3/8] 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 | 96 ++++++++++++++++-----------------
 drivers/staging/iio/meter/ade7854.h     |  7 +++
 2 files changed, 53 insertions(+), 50 deletions(-)

diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
index 37c957482493..1daad42b1e92 100644
--- a/drivers/staging/iio/meter/ade7854-i2c.c
+++ b/drivers/staging/iio/meter/ade7854-i2c.c
@@ -15,86 +15,82 @@
 #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,
+				 int bytes)
 {
 	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 (bytes) {
+	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 unlock;
+	}
+
+	ret = i2c_master_send(st->i2c, st->tx, count);
+
+unlock:
 	mutex_unlock(&st->buf_lock);
 
 	return ret < 0 ? ret : 0;
 }
 
+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);
-
-	return ret < 0 ? ret : 0;
+	return ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_16_BITS);
 }
 
 static int ade7854_i2c_write_reg_24(struct device *dev,
 				    u16 reg_address,
 				    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);
-
-	return ret < 0 ? ret : 0;
+	return ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_24_BITS);
 }
 
 static int ade7854_i2c_write_reg_32(struct device *dev,
 				    u16 reg_address,
 				    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);
-
-	return ret < 0 ? ret : 0;
+	return ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_32_BITS);
 }
 
 static int ade7854_i2c_read_reg_8(struct device *dev,
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

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 3/8] 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 | 96 ++++++++++++++++-----------------
 drivers/staging/iio/meter/ade7854.h     |  7 +++
 2 files changed, 53 insertions(+), 50 deletions(-)

diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
index 37c957482493..1daad42b1e92 100644
--- a/drivers/staging/iio/meter/ade7854-i2c.c
+++ b/drivers/staging/iio/meter/ade7854-i2c.c
@@ -15,86 +15,82 @@
 #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,
+				 int bytes)
 {
 	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 (bytes) {
+	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 unlock;
+	}
+
+	ret = i2c_master_send(st->i2c, st->tx, count);
+
+unlock:
 	mutex_unlock(&st->buf_lock);
 
 	return ret < 0 ? ret : 0;
 }
 
+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);
-
-	return ret < 0 ? ret : 0;
+	return ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_16_BITS);
 }
 
 static int ade7854_i2c_write_reg_24(struct device *dev,
 				    u16 reg_address,
 				    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);
-
-	return ret < 0 ? ret : 0;
+	return ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_24_BITS);
 }
 
 static int ade7854_i2c_write_reg_32(struct device *dev,
 				    u16 reg_address,
 				    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);
-
-	return ret < 0 ? ret : 0;
+	return ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_32_BITS);
 }
 
 static int ade7854_i2c_read_reg_8(struct device *dev,
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 v2 3/8] staging:iio:ade7854: Rework I2C write function

[Jonathan Cameron]

On Fri, 16 Mar 2018 19:49:08 -0300
Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> 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 | 96 ++++++++++++++++-----------------
>  drivers/staging/iio/meter/ade7854.h     |  7 +++
>  2 files changed, 53 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
> index 37c957482493..1daad42b1e92 100644
> --- a/drivers/staging/iio/meter/ade7854-i2c.c
> +++ b/drivers/staging/iio/meter/ade7854-i2c.c
> @@ -15,86 +15,82 @@
>  #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,
> +				 int bytes)
>  {
>  	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;
There are a lot of endian conversions in here, but as some aren't
standard sizes (24 bits) and others are unaligned which makes things
messy, perhaps we are better doing it 'long hand' like this.

> -	st->tx[2] = val;
>  
> -	ret = i2c_master_send(st->i2c, st->tx, 3);
> +	switch (bytes) {
> +	case DATA_SIZE_8_BITS:

The defines really don't help here.  These are real numbers
so have them as such.

> +		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 unlock;
> +	}
> +
> +	ret = i2c_master_send(st->i2c, st->tx, count);
> +
> +unlock:
>  	mutex_unlock(&st->buf_lock);
>  
>  	return ret < 0 ? ret : 0;
>  }
>  
> +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);
> -
> -	return ret < 0 ? ret : 0;
> +	return ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_16_BITS);
>  }
>  
>  static int ade7854_i2c_write_reg_24(struct device *dev,
>  				    u16 reg_address,
>  				    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);
> -
> -	return ret < 0 ? ret : 0;
> +	return ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_24_BITS);
>  }
>  
>  static int ade7854_i2c_write_reg_32(struct device *dev,
>  				    u16 reg_address,
>  				    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);
> -
> -	return ret < 0 ? ret : 0;
> +	return ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_32_BITS);
>  }
>  
>  static int ade7854_i2c_read_reg_8(struct device *dev,
> 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,
> +};
Don't introduce this - these aren't magic numbers so they don't need
'names' to say what they are!

Jonathan
> +
>  /**
>   * struct ade7854_state - device instance specific data
>   * @spi:			actual spi_device

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 3/8] staging:iio:ade7854: Rework I2C write function

[Jonathan Cameron]

On Fri, 16 Mar 2018 19:49:08 -0300
Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> 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 | 96 ++++++++++++++++-----------------
>  drivers/staging/iio/meter/ade7854.h     |  7 +++
>  2 files changed, 53 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
> index 37c957482493..1daad42b1e92 100644
> --- a/drivers/staging/iio/meter/ade7854-i2c.c
> +++ b/drivers/staging/iio/meter/ade7854-i2c.c
> @@ -15,86 +15,82 @@
>  #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,
> +				 int bytes)
>  {
>  	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;
There are a lot of endian conversions in here, but as some aren't
standard sizes (24 bits) and others are unaligned which makes things
messy, perhaps we are better doing it 'long hand' like this.

> -	st->tx[2] = val;
>  
> -	ret = i2c_master_send(st->i2c, st->tx, 3);
> +	switch (bytes) {
> +	case DATA_SIZE_8_BITS:

The defines really don't help here.  These are real numbers
so have them as such.

> +		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 unlock;
> +	}
> +
> +	ret = i2c_master_send(st->i2c, st->tx, count);
> +
> +unlock:
>  	mutex_unlock(&st->buf_lock);
>  
>  	return ret < 0 ? ret : 0;
>  }
>  
> +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);
> -
> -	return ret < 0 ? ret : 0;
> +	return ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_16_BITS);
>  }
>  
>  static int ade7854_i2c_write_reg_24(struct device *dev,
>  				    u16 reg_address,
>  				    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);
> -
> -	return ret < 0 ? ret : 0;
> +	return ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_24_BITS);
>  }
>  
>  static int ade7854_i2c_write_reg_32(struct device *dev,
>  				    u16 reg_address,
>  				    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);
> -
> -	return ret < 0 ? ret : 0;
> +	return ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_32_BITS);
>  }
>  
>  static int ade7854_i2c_read_reg_8(struct device *dev,
> 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,
> +};
Don't introduce this - these aren't magic numbers so they don't need
'names' to say what they are!

Jonathan
> +
>  /**
>   * struct ade7854_state - device instance specific data
>   * @spi:			actual spi_device

[PATCH v2 4/8] 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 | 108 ++++++++++++--------------------
 1 file changed, 41 insertions(+), 67 deletions(-)

diff --git a/drivers/staging/iio/meter/ade7854-spi.c b/drivers/staging/iio/meter/ade7854-spi.c
index 4419b8f06197..f21dc24194fb 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,
+				 int bytes)
 {
 	int ret;
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
@@ -32,93 +33,66 @@ 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 (bytes) {
+	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 unlock;
+	}
 
 	ret = spi_sync_transfer(st->spi, &xfer, 1);
+unlock:
 	mutex_unlock(&st->buf_lock);
 
 	return ret;
 }
 
+static int ade7854_spi_write_reg_8(struct device *dev,
+				   u16 reg_address,
+				   u8 val)
+{
+	return ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_8_BITS);
+}
+
 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);
-
-	return ret;
+	return ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_16_BITS);
 }
 
 static int ade7854_spi_write_reg_24(struct device *dev,
 				    u16 reg_address,
 				    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);
-
-	return ret;
+	return ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_24_BITS);
 }
 
 static int ade7854_spi_write_reg_32(struct device *dev,
 				    u16 reg_address,
 				    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);
-
-	return ret;
+	return ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_32_BITS);
 }
 
 static int ade7854_spi_read_reg_8(struct device *dev,
-- 
2.16.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 4/8] 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 | 108 ++++++++++++--------------------
 1 file changed, 41 insertions(+), 67 deletions(-)

diff --git a/drivers/staging/iio/meter/ade7854-spi.c b/drivers/staging/iio/meter/ade7854-spi.c
index 4419b8f06197..f21dc24194fb 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,
+				 int bytes)
 {
 	int ret;
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
@@ -32,93 +33,66 @@ 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 (bytes) {
+	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 unlock;
+	}
 
 	ret = spi_sync_transfer(st->spi, &xfer, 1);
+unlock:
 	mutex_unlock(&st->buf_lock);
 
 	return ret;
 }
 
+static int ade7854_spi_write_reg_8(struct device *dev,
+				   u16 reg_address,
+				   u8 val)
+{
+	return ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_8_BITS);
+}
+
 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);
-
-	return ret;
+	return ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_16_BITS);
 }
 
 static int ade7854_spi_write_reg_24(struct device *dev,
 				    u16 reg_address,
 				    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);
-
-	return ret;
+	return ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_24_BITS);
 }
 
 static int ade7854_spi_write_reg_32(struct device *dev,
 				    u16 reg_address,
 				    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);
-
-	return ret;
+	return ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_32_BITS);
 }
 
 static int ade7854_spi_read_reg_8(struct device *dev,
-- 
2.16.2

Re: [PATCH v2 4/8] staging:iio:ade7854: Rework SPI write function

[Jonathan Cameron]

On Fri, 16 Mar 2018 19:49:24 -0300
Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> wrote:

> 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>
A good change, but same comment on the defines as for the i2c case.

Thanks,

Jonathan

> ---
>  drivers/staging/iio/meter/ade7854-spi.c | 108 ++++++++++++--------------------
>  1 file changed, 41 insertions(+), 67 deletions(-)
> 
> diff --git a/drivers/staging/iio/meter/ade7854-spi.c b/drivers/staging/iio/meter/ade7854-spi.c
> index 4419b8f06197..f21dc24194fb 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,
> +				 int bytes)
>  {
>  	int ret;
>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> @@ -32,93 +33,66 @@ 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 (bytes) {
> +	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 unlock;
> +	}
>  
>  	ret = spi_sync_transfer(st->spi, &xfer, 1);
> +unlock:
>  	mutex_unlock(&st->buf_lock);
>  
>  	return ret;
>  }
>  
> +static int ade7854_spi_write_reg_8(struct device *dev,
> +				   u16 reg_address,
> +				   u8 val)
> +{
> +	return ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_8_BITS);
> +}
> +
>  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);
> -
> -	return ret;
> +	return ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_16_BITS);
>  }
>  
>  static int ade7854_spi_write_reg_24(struct device *dev,
>  				    u16 reg_address,
>  				    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);
> -
> -	return ret;
> +	return ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_24_BITS);
>  }
>  
>  static int ade7854_spi_write_reg_32(struct device *dev,
>  				    u16 reg_address,
>  				    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);
> -
> -	return ret;
> +	return ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_32_BITS);
>  }
>  
>  static int ade7854_spi_read_reg_8(struct device *dev,

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 4/8] staging:iio:ade7854: Rework SPI write function

[Jonathan Cameron]

On Fri, 16 Mar 2018 19:49:24 -0300
Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> wrote:

> 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>
A good change, but same comment on the defines as for the i2c case.

Thanks,

Jonathan

> ---
>  drivers/staging/iio/meter/ade7854-spi.c | 108 ++++++++++++--------------------
>  1 file changed, 41 insertions(+), 67 deletions(-)
> 
> diff --git a/drivers/staging/iio/meter/ade7854-spi.c b/drivers/staging/iio/meter/ade7854-spi.c
> index 4419b8f06197..f21dc24194fb 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,
> +				 int bytes)
>  {
>  	int ret;
>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> @@ -32,93 +33,66 @@ 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 (bytes) {
> +	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 unlock;
> +	}
>  
>  	ret = spi_sync_transfer(st->spi, &xfer, 1);
> +unlock:
>  	mutex_unlock(&st->buf_lock);
>  
>  	return ret;
>  }
>  
> +static int ade7854_spi_write_reg_8(struct device *dev,
> +				   u16 reg_address,
> +				   u8 val)
> +{
> +	return ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_8_BITS);
> +}
> +
>  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);
> -
> -	return ret;
> +	return ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_16_BITS);
>  }
>  
>  static int ade7854_spi_write_reg_24(struct device *dev,
>  				    u16 reg_address,
>  				    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);
> -
> -	return ret;
> +	return ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_24_BITS);
>  }
>  
>  static int ade7854_spi_write_reg_32(struct device *dev,
>  				    u16 reg_address,
>  				    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);
> -
> -	return ret;
> +	return ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_32_BITS);
>  }
>  
>  static int ade7854_spi_read_reg_8(struct device *dev,

[PATCH v2 5/8] 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 | 33 +--------------------------------
 drivers/staging/iio/meter/ade7854-spi.c | 33 +--------------------------------
 drivers/staging/iio/meter/ade7854.c     | 12 ++++++------
 drivers/staging/iio/meter/ade7854.h     |  9 ++++-----
 4 files changed, 12 insertions(+), 75 deletions(-)

diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
index 1daad42b1e92..e95147a1bac1 100644
--- a/drivers/staging/iio/meter/ade7854-i2c.c
+++ b/drivers/staging/iio/meter/ade7854-i2c.c
@@ -65,34 +65,6 @@ static int ade7854_i2c_write_reg(struct device *dev,
 	return ret < 0 ? ret : 0;
 }
 
-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)
-{
-	return ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_16_BITS);
-}
-
-static int ade7854_i2c_write_reg_24(struct device *dev,
-				    u16 reg_address,
-				    u32 val)
-{
-	return ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_24_BITS);
-}
-
-static int ade7854_i2c_write_reg_32(struct device *dev,
-				    u16 reg_address,
-				    u32 val)
-{
-	return ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_32_BITS);
-}
-
 static int ade7854_i2c_read_reg_8(struct device *dev,
 				  u16 reg_address,
 				  u8 *val)
@@ -213,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 f21dc24194fb..1eba3044b86f 100644
--- a/drivers/staging/iio/meter/ade7854-spi.c
+++ b/drivers/staging/iio/meter/ade7854-spi.c
@@ -67,34 +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)
-{
-	return ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_8_BITS);
-}
-
-static int ade7854_spi_write_reg_16(struct device *dev,
-				    u16 reg_address,
-				    u16 val)
-{
-	return ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_16_BITS);
-}
-
-static int ade7854_spi_write_reg_24(struct device *dev,
-				    u16 reg_address,
-				    u32 val)
-{
-	return ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_24_BITS);
-}
-
-static int ade7854_spi_write_reg_32(struct device *dev,
-				    u16 reg_address,
-				    u32 val)
-{
-	return ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_32_BITS);
-}
-
 static int ade7854_spi_read_reg_8(struct device *dev,
 				  u16 reg_address,
 				  u8 *val)
@@ -260,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 0193ae3aae29..4aa7c6ce0016 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..aa29faf7c91f 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,
+			 int type);
 	int irq;
 	struct mutex buf_lock;
 	u8 tx[ADE7854_MAX_TX] ____cacheline_aligned;
-- 
2.16.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 5/8] 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 | 33 +--------------------------------
 drivers/staging/iio/meter/ade7854-spi.c | 33 +--------------------------------
 drivers/staging/iio/meter/ade7854.c     | 12 ++++++------
 drivers/staging/iio/meter/ade7854.h     |  9 ++++-----
 4 files changed, 12 insertions(+), 75 deletions(-)

diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
index 1daad42b1e92..e95147a1bac1 100644
--- a/drivers/staging/iio/meter/ade7854-i2c.c
+++ b/drivers/staging/iio/meter/ade7854-i2c.c
@@ -65,34 +65,6 @@ static int ade7854_i2c_write_reg(struct device *dev,
 	return ret < 0 ? ret : 0;
 }
 
-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)
-{
-	return ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_16_BITS);
-}
-
-static int ade7854_i2c_write_reg_24(struct device *dev,
-				    u16 reg_address,
-				    u32 val)
-{
-	return ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_24_BITS);
-}
-
-static int ade7854_i2c_write_reg_32(struct device *dev,
-				    u16 reg_address,
-				    u32 val)
-{
-	return ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_32_BITS);
-}
-
 static int ade7854_i2c_read_reg_8(struct device *dev,
 				  u16 reg_address,
 				  u8 *val)
@@ -213,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 f21dc24194fb..1eba3044b86f 100644
--- a/drivers/staging/iio/meter/ade7854-spi.c
+++ b/drivers/staging/iio/meter/ade7854-spi.c
@@ -67,34 +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)
-{
-	return ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_8_BITS);
-}
-
-static int ade7854_spi_write_reg_16(struct device *dev,
-				    u16 reg_address,
-				    u16 val)
-{
-	return ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_16_BITS);
-}
-
-static int ade7854_spi_write_reg_24(struct device *dev,
-				    u16 reg_address,
-				    u32 val)
-{
-	return ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_24_BITS);
-}
-
-static int ade7854_spi_write_reg_32(struct device *dev,
-				    u16 reg_address,
-				    u32 val)
-{
-	return ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_32_BITS);
-}
-
 static int ade7854_spi_read_reg_8(struct device *dev,
 				  u16 reg_address,
 				  u8 *val)
@@ -260,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 0193ae3aae29..4aa7c6ce0016 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..aa29faf7c91f 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,
+			 int type);
 	int irq;
 	struct mutex buf_lock;
 	u8 tx[ADE7854_MAX_TX] ____cacheline_aligned;
-- 
2.16.2

Re: [PATCH v2 5/8] staging:iio:ade7854: Replace many functions for one function

[Jonathan Cameron]

On Fri, 16 Mar 2018 19:49:40 -0300
Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> wrote:

> 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>
Makes sense and a nice reduction in lines of code.

Will pick up once the precursors are ready.

Thanks,

Jonathan
> ---
>  drivers/staging/iio/meter/ade7854-i2c.c | 33 +--------------------------------
>  drivers/staging/iio/meter/ade7854-spi.c | 33 +--------------------------------
>  drivers/staging/iio/meter/ade7854.c     | 12 ++++++------
>  drivers/staging/iio/meter/ade7854.h     |  9 ++++-----
>  4 files changed, 12 insertions(+), 75 deletions(-)
> 
> diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
> index 1daad42b1e92..e95147a1bac1 100644
> --- a/drivers/staging/iio/meter/ade7854-i2c.c
> +++ b/drivers/staging/iio/meter/ade7854-i2c.c
> @@ -65,34 +65,6 @@ static int ade7854_i2c_write_reg(struct device *dev,
>  	return ret < 0 ? ret : 0;
>  }
>  
> -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)
> -{
> -	return ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_16_BITS);
> -}
> -
> -static int ade7854_i2c_write_reg_24(struct device *dev,
> -				    u16 reg_address,
> -				    u32 val)
> -{
> -	return ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_24_BITS);
> -}
> -
> -static int ade7854_i2c_write_reg_32(struct device *dev,
> -				    u16 reg_address,
> -				    u32 val)
> -{
> -	return ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_32_BITS);
> -}
> -
>  static int ade7854_i2c_read_reg_8(struct device *dev,
>  				  u16 reg_address,
>  				  u8 *val)
> @@ -213,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 f21dc24194fb..1eba3044b86f 100644
> --- a/drivers/staging/iio/meter/ade7854-spi.c
> +++ b/drivers/staging/iio/meter/ade7854-spi.c
> @@ -67,34 +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)
> -{
> -	return ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_8_BITS);
> -}
> -
> -static int ade7854_spi_write_reg_16(struct device *dev,
> -				    u16 reg_address,
> -				    u16 val)
> -{
> -	return ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_16_BITS);
> -}
> -
> -static int ade7854_spi_write_reg_24(struct device *dev,
> -				    u16 reg_address,
> -				    u32 val)
> -{
> -	return ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_24_BITS);
> -}
> -
> -static int ade7854_spi_write_reg_32(struct device *dev,
> -				    u16 reg_address,
> -				    u32 val)
> -{
> -	return ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_32_BITS);
> -}
> -
>  static int ade7854_spi_read_reg_8(struct device *dev,
>  				  u16 reg_address,
>  				  u8 *val)
> @@ -260,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 0193ae3aae29..4aa7c6ce0016 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..aa29faf7c91f 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,
> +			 int type);
>  	int irq;
>  	struct mutex buf_lock;
>  	u8 tx[ADE7854_MAX_TX] ____cacheline_aligned;

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 5/8] staging:iio:ade7854: Replace many functions for one function

[Jonathan Cameron]

On Fri, 16 Mar 2018 19:49:40 -0300
Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> wrote:

> 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>
Makes sense and a nice reduction in lines of code.

Will pick up once the precursors are ready.

Thanks,

Jonathan
> ---
>  drivers/staging/iio/meter/ade7854-i2c.c | 33 +--------------------------------
>  drivers/staging/iio/meter/ade7854-spi.c | 33 +--------------------------------
>  drivers/staging/iio/meter/ade7854.c     | 12 ++++++------
>  drivers/staging/iio/meter/ade7854.h     |  9 ++++-----
>  4 files changed, 12 insertions(+), 75 deletions(-)
> 
> diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
> index 1daad42b1e92..e95147a1bac1 100644
> --- a/drivers/staging/iio/meter/ade7854-i2c.c
> +++ b/drivers/staging/iio/meter/ade7854-i2c.c
> @@ -65,34 +65,6 @@ static int ade7854_i2c_write_reg(struct device *dev,
>  	return ret < 0 ? ret : 0;
>  }
>  
> -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)
> -{
> -	return ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_16_BITS);
> -}
> -
> -static int ade7854_i2c_write_reg_24(struct device *dev,
> -				    u16 reg_address,
> -				    u32 val)
> -{
> -	return ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_24_BITS);
> -}
> -
> -static int ade7854_i2c_write_reg_32(struct device *dev,
> -				    u16 reg_address,
> -				    u32 val)
> -{
> -	return ade7854_i2c_write_reg(dev, reg_address, val, DATA_SIZE_32_BITS);
> -}
> -
>  static int ade7854_i2c_read_reg_8(struct device *dev,
>  				  u16 reg_address,
>  				  u8 *val)
> @@ -213,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 f21dc24194fb..1eba3044b86f 100644
> --- a/drivers/staging/iio/meter/ade7854-spi.c
> +++ b/drivers/staging/iio/meter/ade7854-spi.c
> @@ -67,34 +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)
> -{
> -	return ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_8_BITS);
> -}
> -
> -static int ade7854_spi_write_reg_16(struct device *dev,
> -				    u16 reg_address,
> -				    u16 val)
> -{
> -	return ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_16_BITS);
> -}
> -
> -static int ade7854_spi_write_reg_24(struct device *dev,
> -				    u16 reg_address,
> -				    u32 val)
> -{
> -	return ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_24_BITS);
> -}
> -
> -static int ade7854_spi_write_reg_32(struct device *dev,
> -				    u16 reg_address,
> -				    u32 val)
> -{
> -	return ade7854_spi_write_reg(dev, reg_address, val, DATA_SIZE_32_BITS);
> -}
> -
>  static int ade7854_spi_read_reg_8(struct device *dev,
>  				  u16 reg_address,
>  				  u8 *val)
> @@ -260,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 0193ae3aae29..4aa7c6ce0016 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..aa29faf7c91f 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,
> +			 int type);
>  	int irq;
>  	struct mutex buf_lock;
>  	u8 tx[ADE7854_MAX_TX] ____cacheline_aligned;

[PATCH v2 6/8] 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 | 110 ++++++++++++--------------------
 1 file changed, 41 insertions(+), 69 deletions(-)

diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
index e95147a1bac1..20db8eedb84a 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 < 0 ? ret : 0;
 }
 
-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,
+				int bytes)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct ade7854_state *st = iio_priv(indio_dev);
@@ -79,95 +80,66 @@ static int ade7854_i2c_read_reg_8(struct device *dev,
 
 	ret = i2c_master_send(st->i2c, st->tx, 2);
 	if (ret < 0)
-		goto out;
+		goto unlock;
 
-	ret = i2c_master_recv(st->i2c, st->rx, 1);
+	ret = i2c_master_recv(st->i2c, st->rx, bytes);
 	if (ret < 0)
-		goto out;
+		goto unlock;
+
+	switch (bytes) {
+	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 unlock;
+	}
 
-	*val = st->rx[0];
-out:
+unlock:
 	mutex_unlock(&st->buf_lock);
 	return ret;
 }
 
+static int ade7854_i2c_read_reg_8(struct device *dev,
+				  u16 reg_address,
+				  u8 *val)
+{
+	return ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
+				    DATA_SIZE_8_BITS);
+}
+
 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 < 0)
-		goto out;
-
-	ret = i2c_master_recv(st->i2c, st->rx, 2);
-	if (ret < 0)
-		goto out;
-
-	*val = (st->rx[0] << 8) | st->rx[1];
-out:
-	mutex_unlock(&st->buf_lock);
-	return ret;
+	return ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
+				    DATA_SIZE_16_BITS);
 }
 
 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 < 0)
-		goto out;
-
-	ret = i2c_master_recv(st->i2c, st->rx, 3);
-	if (ret < 0)
-		goto out;
-
-	*val = (st->rx[0] << 16) | (st->rx[1] << 8) | st->rx[2];
-out:
-	mutex_unlock(&st->buf_lock);
-	return ret;
+	return ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
+				    DATA_SIZE_24_BITS);
 }
 
 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 < 0)
-		goto out;
-
-	ret = i2c_master_recv(st->i2c, st->rx, 4);
-	if (ret < 0)
-		goto out;
-
-	*val = (st->rx[0] << 24) | (st->rx[1] << 16) |
-		(st->rx[2] << 8) | st->rx[3];
-out:
-	mutex_unlock(&st->buf_lock);
-	return ret;
+	return ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
+				    DATA_SIZE_32_BITS);
 }
 
 static int ade7854_i2c_probe(struct i2c_client *client,
-- 
2.16.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 6/8] 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 | 110 ++++++++++++--------------------
 1 file changed, 41 insertions(+), 69 deletions(-)

diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
index e95147a1bac1..20db8eedb84a 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 < 0 ? ret : 0;
 }
 
-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,
+				int bytes)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct ade7854_state *st = iio_priv(indio_dev);
@@ -79,95 +80,66 @@ static int ade7854_i2c_read_reg_8(struct device *dev,
 
 	ret = i2c_master_send(st->i2c, st->tx, 2);
 	if (ret < 0)
-		goto out;
+		goto unlock;
 
-	ret = i2c_master_recv(st->i2c, st->rx, 1);
+	ret = i2c_master_recv(st->i2c, st->rx, bytes);
 	if (ret < 0)
-		goto out;
+		goto unlock;
+
+	switch (bytes) {
+	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 unlock;
+	}
 
-	*val = st->rx[0];
-out:
+unlock:
 	mutex_unlock(&st->buf_lock);
 	return ret;
 }
 
+static int ade7854_i2c_read_reg_8(struct device *dev,
+				  u16 reg_address,
+				  u8 *val)
+{
+	return ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
+				    DATA_SIZE_8_BITS);
+}
+
 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 < 0)
-		goto out;
-
-	ret = i2c_master_recv(st->i2c, st->rx, 2);
-	if (ret < 0)
-		goto out;
-
-	*val = (st->rx[0] << 8) | st->rx[1];
-out:
-	mutex_unlock(&st->buf_lock);
-	return ret;
+	return ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
+				    DATA_SIZE_16_BITS);
 }
 
 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 < 0)
-		goto out;
-
-	ret = i2c_master_recv(st->i2c, st->rx, 3);
-	if (ret < 0)
-		goto out;
-
-	*val = (st->rx[0] << 16) | (st->rx[1] << 8) | st->rx[2];
-out:
-	mutex_unlock(&st->buf_lock);
-	return ret;
+	return ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
+				    DATA_SIZE_24_BITS);
 }
 
 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 < 0)
-		goto out;
-
-	ret = i2c_master_recv(st->i2c, st->rx, 4);
-	if (ret < 0)
-		goto out;
-
-	*val = (st->rx[0] << 24) | (st->rx[1] << 16) |
-		(st->rx[2] << 8) | st->rx[3];
-out:
-	mutex_unlock(&st->buf_lock);
-	return ret;
+	return ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
+				    DATA_SIZE_32_BITS);
 }
 
 static int ade7854_i2c_probe(struct i2c_client *client,
-- 
2.16.2

Re: [PATCH v2 6/8] staging:iio:ade7854: Rework I2C read function

[Jonathan Cameron]

On Fri, 16 Mar 2018 19:49:59 -0300
Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> wrote:

> 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.
This seems likely to have been covered by the earlier patch. Please update
the description.

Otherwise same define comment but beyond that looks good.

Jonathan

> 
> 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 | 110 ++++++++++++--------------------
>  1 file changed, 41 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
> index e95147a1bac1..20db8eedb84a 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 < 0 ? ret : 0;
>  }
>  
> -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,
> +				int bytes)
>  {
>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>  	struct ade7854_state *st = iio_priv(indio_dev);
> @@ -79,95 +80,66 @@ static int ade7854_i2c_read_reg_8(struct device *dev,
>  
>  	ret = i2c_master_send(st->i2c, st->tx, 2);
>  	if (ret < 0)
> -		goto out;
> +		goto unlock;
>  
> -	ret = i2c_master_recv(st->i2c, st->rx, 1);
> +	ret = i2c_master_recv(st->i2c, st->rx, bytes);
>  	if (ret < 0)
> -		goto out;
> +		goto unlock;
> +
> +	switch (bytes) {
> +	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 unlock;
> +	}
>  
> -	*val = st->rx[0];
> -out:
> +unlock:
>  	mutex_unlock(&st->buf_lock);
>  	return ret;
>  }
>  
> +static int ade7854_i2c_read_reg_8(struct device *dev,
> +				  u16 reg_address,
> +				  u8 *val)
> +{
> +	return ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
> +				    DATA_SIZE_8_BITS);
> +}
> +
>  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 < 0)
> -		goto out;
> -
> -	ret = i2c_master_recv(st->i2c, st->rx, 2);
> -	if (ret < 0)
> -		goto out;
> -
> -	*val = (st->rx[0] << 8) | st->rx[1];
> -out:
> -	mutex_unlock(&st->buf_lock);
> -	return ret;
> +	return ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
> +				    DATA_SIZE_16_BITS);
>  }
>  
>  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 < 0)
> -		goto out;
> -
> -	ret = i2c_master_recv(st->i2c, st->rx, 3);
> -	if (ret < 0)
> -		goto out;
> -
> -	*val = (st->rx[0] << 16) | (st->rx[1] << 8) | st->rx[2];
> -out:
> -	mutex_unlock(&st->buf_lock);
> -	return ret;
> +	return ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
> +				    DATA_SIZE_24_BITS);
>  }
>  
>  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 < 0)
> -		goto out;
> -
> -	ret = i2c_master_recv(st->i2c, st->rx, 4);
> -	if (ret < 0)
> -		goto out;
> -
> -	*val = (st->rx[0] << 24) | (st->rx[1] << 16) |
> -		(st->rx[2] << 8) | st->rx[3];
> -out:
> -	mutex_unlock(&st->buf_lock);
> -	return ret;
> +	return ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
> +				    DATA_SIZE_32_BITS);
>  }
>  
>  static int ade7854_i2c_probe(struct i2c_client *client,

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 6/8] staging:iio:ade7854: Rework I2C read function

[Jonathan Cameron]

On Fri, 16 Mar 2018 19:49:59 -0300
Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> wrote:

> 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.
This seems likely to have been covered by the earlier patch. Please update
the description.

Otherwise same define comment but beyond that looks good.

Jonathan

> 
> 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 | 110 ++++++++++++--------------------
>  1 file changed, 41 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
> index e95147a1bac1..20db8eedb84a 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 < 0 ? ret : 0;
>  }
>  
> -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,
> +				int bytes)
>  {
>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>  	struct ade7854_state *st = iio_priv(indio_dev);
> @@ -79,95 +80,66 @@ static int ade7854_i2c_read_reg_8(struct device *dev,
>  
>  	ret = i2c_master_send(st->i2c, st->tx, 2);
>  	if (ret < 0)
> -		goto out;
> +		goto unlock;
>  
> -	ret = i2c_master_recv(st->i2c, st->rx, 1);
> +	ret = i2c_master_recv(st->i2c, st->rx, bytes);
>  	if (ret < 0)
> -		goto out;
> +		goto unlock;
> +
> +	switch (bytes) {
> +	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 unlock;
> +	}
>  
> -	*val = st->rx[0];
> -out:
> +unlock:
>  	mutex_unlock(&st->buf_lock);
>  	return ret;
>  }
>  
> +static int ade7854_i2c_read_reg_8(struct device *dev,
> +				  u16 reg_address,
> +				  u8 *val)
> +{
> +	return ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
> +				    DATA_SIZE_8_BITS);
> +}
> +
>  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 < 0)
> -		goto out;
> -
> -	ret = i2c_master_recv(st->i2c, st->rx, 2);
> -	if (ret < 0)
> -		goto out;
> -
> -	*val = (st->rx[0] << 8) | st->rx[1];
> -out:
> -	mutex_unlock(&st->buf_lock);
> -	return ret;
> +	return ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
> +				    DATA_SIZE_16_BITS);
>  }
>  
>  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 < 0)
> -		goto out;
> -
> -	ret = i2c_master_recv(st->i2c, st->rx, 3);
> -	if (ret < 0)
> -		goto out;
> -
> -	*val = (st->rx[0] << 16) | (st->rx[1] << 8) | st->rx[2];
> -out:
> -	mutex_unlock(&st->buf_lock);
> -	return ret;
> +	return ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
> +				    DATA_SIZE_24_BITS);
>  }
>  
>  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 < 0)
> -		goto out;
> -
> -	ret = i2c_master_recv(st->i2c, st->rx, 4);
> -	if (ret < 0)
> -		goto out;
> -
> -	*val = (st->rx[0] << 24) | (st->rx[1] << 16) |
> -		(st->rx[2] << 8) | st->rx[3];
> -out:
> -	mutex_unlock(&st->buf_lock);
> -	return ret;
> +	return ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
> +				    DATA_SIZE_32_BITS);
>  }
>  
>  static int ade7854_i2c_probe(struct i2c_client *client,

[PATCH v2 7/8] 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 | 140 +++++++++-----------------------
 1 file changed, 37 insertions(+), 103 deletions(-)

diff --git a/drivers/staging/iio/meter/ade7854-spi.c b/drivers/staging/iio/meter/ade7854-spi.c
index 1eba3044b86f..964d6c6e76d1 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,
+				int bytes)
 {
 	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 = bytes,
 		}
 	};
 
@@ -94,128 +95,61 @@ 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 unlock;
+	}
+
+	switch (bytes) {
+	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:
+unlock:
 	mutex_unlock(&st->buf_lock);
 	return ret;
 }
 
+static int ade7854_spi_read_reg_8(struct device *dev,
+				  u16 reg_address,
+				  u8 *val)
+{
+	return ade7854_spi_read_reg(dev, reg_address, (u32 *)val,
+				    DATA_SIZE_8_BITS);
+}
+
 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 = 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;
+	return ade7854_spi_read_reg(dev, reg_address, (u32 *)val,
+				    DATA_SIZE_16_BITS);
 }
 
 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 = 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;
+	return ade7854_spi_read_reg(dev, reg_address, (u32 *)val,
+				    DATA_SIZE_24_BITS);
 }
 
 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 = 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;
+	return ade7854_spi_read_reg(dev, reg_address, (u32 *)val,
+				    DATA_SIZE_32_BITS);
 }
 
 static int ade7854_spi_probe(struct spi_device *spi)
-- 
2.16.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 7/8] 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 | 140 +++++++++-----------------------
 1 file changed, 37 insertions(+), 103 deletions(-)

diff --git a/drivers/staging/iio/meter/ade7854-spi.c b/drivers/staging/iio/meter/ade7854-spi.c
index 1eba3044b86f..964d6c6e76d1 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,
+				int bytes)
 {
 	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 = bytes,
 		}
 	};
 
@@ -94,128 +95,61 @@ 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 unlock;
+	}
+
+	switch (bytes) {
+	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:
+unlock:
 	mutex_unlock(&st->buf_lock);
 	return ret;
 }
 
+static int ade7854_spi_read_reg_8(struct device *dev,
+				  u16 reg_address,
+				  u8 *val)
+{
+	return ade7854_spi_read_reg(dev, reg_address, (u32 *)val,
+				    DATA_SIZE_8_BITS);
+}
+
 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 = 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;
+	return ade7854_spi_read_reg(dev, reg_address, (u32 *)val,
+				    DATA_SIZE_16_BITS);
 }
 
 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 = 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;
+	return ade7854_spi_read_reg(dev, reg_address, (u32 *)val,
+				    DATA_SIZE_24_BITS);
 }
 
 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 = 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;
+	return ade7854_spi_read_reg(dev, reg_address, (u32 *)val,
+				    DATA_SIZE_32_BITS);
 }
 
 static int ade7854_spi_probe(struct spi_device *spi)
-- 
2.16.2

[PATCH v2 8/8] 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 | 37 +------------------------------
 drivers/staging/iio/meter/ade7854-spi.c | 39 ++-------------------------------
 drivers/staging/iio/meter/ade7854.c     | 18 +++++++--------
 drivers/staging/iio/meter/ade7854.h     |  7 +++---
 4 files changed, 15 insertions(+), 86 deletions(-)

diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
index 20db8eedb84a..162c171a8851 100644
--- a/drivers/staging/iio/meter/ade7854-i2c.c
+++ b/drivers/staging/iio/meter/ade7854-i2c.c
@@ -110,38 +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)
-{
-	return ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
-				    DATA_SIZE_8_BITS);
-}
-
-static int ade7854_i2c_read_reg_16(struct device *dev,
-				   u16 reg_address,
-				   u16 *val)
-{
-	return ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
-				    DATA_SIZE_16_BITS);
-}
-
-static int ade7854_i2c_read_reg_24(struct device *dev,
-				   u16 reg_address,
-				   u32 *val)
-{
-	return ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
-				    DATA_SIZE_24_BITS);
-}
-
-static int ade7854_i2c_read_reg_32(struct device *dev,
-				   u16 reg_address,
-				   u32 *val)
-{
-	return ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
-				    DATA_SIZE_32_BITS);
-}
-
 static int ade7854_i2c_probe(struct i2c_client *client,
 			     const struct i2c_device_id *id)
 {
@@ -153,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 964d6c6e76d1..66b8a8767a26 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 unlock;
@@ -120,38 +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)
-{
-	return ade7854_spi_read_reg(dev, reg_address, (u32 *)val,
-				    DATA_SIZE_8_BITS);
-}
-
-static int ade7854_spi_read_reg_16(struct device *dev,
-				   u16 reg_address,
-				   u16 *val)
-{
-	return ade7854_spi_read_reg(dev, reg_address, (u32 *)val,
-				    DATA_SIZE_16_BITS);
-}
-
-static int ade7854_spi_read_reg_24(struct device *dev,
-				   u16 reg_address,
-				   u32 *val)
-{
-	return ade7854_spi_read_reg(dev, reg_address, (u32 *)val,
-				    DATA_SIZE_24_BITS);
-}
-
-static int ade7854_spi_read_reg_32(struct device *dev,
-				   u16 reg_address,
-				   u32 *val)
-{
-	return ade7854_spi_read_reg(dev, reg_address, (u32 *)val,
-				    DATA_SIZE_32_BITS);
-}
-
 static int ade7854_spi_probe(struct spi_device *spi)
 {
 	struct ade7854_state *st;
@@ -162,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 4aa7c6ce0016..09fd8c067738 100644
--- a/drivers/staging/iio/meter/ade7854.c
+++ b/drivers/staging/iio/meter/ade7854.c
@@ -27,12 +27,12 @@ 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);
+	ret = st->read_reg(dev, this_attr->address, &val, DATA_SIZE_8_BITS);
 	if (ret < 0)
 		return ret;
 
@@ -44,12 +44,12 @@ 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);
+	ret = st->read_reg(dev, this_attr->address, &val, DATA_SIZE_16_BITS);
 	if (ret < 0)
 		return ret;
 
@@ -66,7 +66,7 @@ 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);
+	ret = st->read_reg(dev, this_attr->address, &val, DATA_SIZE_24_BITS);
 	if (ret < 0)
 		return ret;
 
@@ -83,7 +83,7 @@ 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);
+	ret = st->read_reg(dev, this_attr->address, &val, DATA_SIZE_32_BITS);
 	if (ret < 0)
 		return ret;
 
@@ -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,7 +415,7 @@ static int ade7854_set_irq(struct device *dev, bool enable)
 	int ret;
 	u32 irqen;
 
-	ret = st->read_reg_32(dev, ADE7854_MASK0, &irqen);
+	ret = st->read_reg(dev, ADE7854_MASK0, &irqen, DATA_SIZE_32_BITS);
 	if (ret < 0)
 		return ret;
 
diff --git a/drivers/staging/iio/meter/ade7854.h b/drivers/staging/iio/meter/ade7854.h
index aa29faf7c91f..78584f01d052 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,
+			int type);
 	int (*write_reg)(struct device *dev, u16 reg_address, u32 val,
 			 int type);
 	int irq;
-- 
2.16.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 8/8] 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 | 37 +------------------------------
 drivers/staging/iio/meter/ade7854-spi.c | 39 ++-------------------------------
 drivers/staging/iio/meter/ade7854.c     | 18 +++++++--------
 drivers/staging/iio/meter/ade7854.h     |  7 +++---
 4 files changed, 15 insertions(+), 86 deletions(-)

diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
index 20db8eedb84a..162c171a8851 100644
--- a/drivers/staging/iio/meter/ade7854-i2c.c
+++ b/drivers/staging/iio/meter/ade7854-i2c.c
@@ -110,38 +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)
-{
-	return ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
-				    DATA_SIZE_8_BITS);
-}
-
-static int ade7854_i2c_read_reg_16(struct device *dev,
-				   u16 reg_address,
-				   u16 *val)
-{
-	return ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
-				    DATA_SIZE_16_BITS);
-}
-
-static int ade7854_i2c_read_reg_24(struct device *dev,
-				   u16 reg_address,
-				   u32 *val)
-{
-	return ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
-				    DATA_SIZE_24_BITS);
-}
-
-static int ade7854_i2c_read_reg_32(struct device *dev,
-				   u16 reg_address,
-				   u32 *val)
-{
-	return ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
-				    DATA_SIZE_32_BITS);
-}
-
 static int ade7854_i2c_probe(struct i2c_client *client,
 			     const struct i2c_device_id *id)
 {
@@ -153,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 964d6c6e76d1..66b8a8767a26 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 unlock;
@@ -120,38 +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)
-{
-	return ade7854_spi_read_reg(dev, reg_address, (u32 *)val,
-				    DATA_SIZE_8_BITS);
-}
-
-static int ade7854_spi_read_reg_16(struct device *dev,
-				   u16 reg_address,
-				   u16 *val)
-{
-	return ade7854_spi_read_reg(dev, reg_address, (u32 *)val,
-				    DATA_SIZE_16_BITS);
-}
-
-static int ade7854_spi_read_reg_24(struct device *dev,
-				   u16 reg_address,
-				   u32 *val)
-{
-	return ade7854_spi_read_reg(dev, reg_address, (u32 *)val,
-				    DATA_SIZE_24_BITS);
-}
-
-static int ade7854_spi_read_reg_32(struct device *dev,
-				   u16 reg_address,
-				   u32 *val)
-{
-	return ade7854_spi_read_reg(dev, reg_address, (u32 *)val,
-				    DATA_SIZE_32_BITS);
-}
-
 static int ade7854_spi_probe(struct spi_device *spi)
 {
 	struct ade7854_state *st;
@@ -162,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 4aa7c6ce0016..09fd8c067738 100644
--- a/drivers/staging/iio/meter/ade7854.c
+++ b/drivers/staging/iio/meter/ade7854.c
@@ -27,12 +27,12 @@ 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);
+	ret = st->read_reg(dev, this_attr->address, &val, DATA_SIZE_8_BITS);
 	if (ret < 0)
 		return ret;
 
@@ -44,12 +44,12 @@ 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);
+	ret = st->read_reg(dev, this_attr->address, &val, DATA_SIZE_16_BITS);
 	if (ret < 0)
 		return ret;
 
@@ -66,7 +66,7 @@ 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);
+	ret = st->read_reg(dev, this_attr->address, &val, DATA_SIZE_24_BITS);
 	if (ret < 0)
 		return ret;
 
@@ -83,7 +83,7 @@ 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);
+	ret = st->read_reg(dev, this_attr->address, &val, DATA_SIZE_32_BITS);
 	if (ret < 0)
 		return ret;
 
@@ -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,7 +415,7 @@ static int ade7854_set_irq(struct device *dev, bool enable)
 	int ret;
 	u32 irqen;
 
-	ret = st->read_reg_32(dev, ADE7854_MASK0, &irqen);
+	ret = st->read_reg(dev, ADE7854_MASK0, &irqen, DATA_SIZE_32_BITS);
 	if (ret < 0)
 		return ret;
 
diff --git a/drivers/staging/iio/meter/ade7854.h b/drivers/staging/iio/meter/ade7854.h
index aa29faf7c91f..78584f01d052 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,
+			int type);
 	int (*write_reg)(struct device *dev, u16 reg_address, u32 val,
 			 int type);
 	int irq;
-- 
2.16.2

Re: [PATCH v2 8/8] staging:iio:ade7854: Remove read_reg_* duplications

[Jonathan Cameron]

On Fri, 16 Mar 2018 19:50:29 -0300
Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> wrote:

> 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>
The meaning of John's signed-off-by here needs clarifying..

Also fix up the common on error handling as I believe you now do that in
an earlier patch.

Thanks and looking forward to v3.  This is a nice little bit of cleanup.
These weird devices with variable register sizes are always somewhat of
a pain to deal with.

Jonathan

> ---
>  drivers/staging/iio/meter/ade7854-i2c.c | 37 +------------------------------
>  drivers/staging/iio/meter/ade7854-spi.c | 39 ++-------------------------------
>  drivers/staging/iio/meter/ade7854.c     | 18 +++++++--------
>  drivers/staging/iio/meter/ade7854.h     |  7 +++---
>  4 files changed, 15 insertions(+), 86 deletions(-)
> 
> diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
> index 20db8eedb84a..162c171a8851 100644
> --- a/drivers/staging/iio/meter/ade7854-i2c.c
> +++ b/drivers/staging/iio/meter/ade7854-i2c.c
> @@ -110,38 +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)
> -{
> -	return ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
> -				    DATA_SIZE_8_BITS);
> -}
> -
> -static int ade7854_i2c_read_reg_16(struct device *dev,
> -				   u16 reg_address,
> -				   u16 *val)
> -{
> -	return ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
> -				    DATA_SIZE_16_BITS);
> -}
> -
> -static int ade7854_i2c_read_reg_24(struct device *dev,
> -				   u16 reg_address,
> -				   u32 *val)
> -{
> -	return ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
> -				    DATA_SIZE_24_BITS);
> -}
> -
> -static int ade7854_i2c_read_reg_32(struct device *dev,
> -				   u16 reg_address,
> -				   u32 *val)
> -{
> -	return ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
> -				    DATA_SIZE_32_BITS);
> -}
> -
>  static int ade7854_i2c_probe(struct i2c_client *client,
>  			     const struct i2c_device_id *id)
>  {
> @@ -153,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 964d6c6e76d1..66b8a8767a26 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 unlock;
> @@ -120,38 +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)
> -{
> -	return ade7854_spi_read_reg(dev, reg_address, (u32 *)val,
> -				    DATA_SIZE_8_BITS);
> -}
> -
> -static int ade7854_spi_read_reg_16(struct device *dev,
> -				   u16 reg_address,
> -				   u16 *val)
> -{
> -	return ade7854_spi_read_reg(dev, reg_address, (u32 *)val,
> -				    DATA_SIZE_16_BITS);
> -}
> -
> -static int ade7854_spi_read_reg_24(struct device *dev,
> -				   u16 reg_address,
> -				   u32 *val)
> -{
> -	return ade7854_spi_read_reg(dev, reg_address, (u32 *)val,
> -				    DATA_SIZE_24_BITS);
> -}
> -
> -static int ade7854_spi_read_reg_32(struct device *dev,
> -				   u16 reg_address,
> -				   u32 *val)
> -{
> -	return ade7854_spi_read_reg(dev, reg_address, (u32 *)val,
> -				    DATA_SIZE_32_BITS);
> -}
> -
>  static int ade7854_spi_probe(struct spi_device *spi)
>  {
>  	struct ade7854_state *st;
> @@ -162,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 4aa7c6ce0016..09fd8c067738 100644
> --- a/drivers/staging/iio/meter/ade7854.c
> +++ b/drivers/staging/iio/meter/ade7854.c
> @@ -27,12 +27,12 @@ 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);
> +	ret = st->read_reg(dev, this_attr->address, &val, DATA_SIZE_8_BITS);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -44,12 +44,12 @@ 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);
> +	ret = st->read_reg(dev, this_attr->address, &val, DATA_SIZE_16_BITS);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -66,7 +66,7 @@ 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);
> +	ret = st->read_reg(dev, this_attr->address, &val, DATA_SIZE_24_BITS);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -83,7 +83,7 @@ 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);
> +	ret = st->read_reg(dev, this_attr->address, &val, DATA_SIZE_32_BITS);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -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,7 +415,7 @@ static int ade7854_set_irq(struct device *dev, bool enable)
>  	int ret;
>  	u32 irqen;
>  
> -	ret = st->read_reg_32(dev, ADE7854_MASK0, &irqen);
> +	ret = st->read_reg(dev, ADE7854_MASK0, &irqen, DATA_SIZE_32_BITS);
>  	if (ret < 0)
>  		return ret;
>  
> diff --git a/drivers/staging/iio/meter/ade7854.h b/drivers/staging/iio/meter/ade7854.h
> index aa29faf7c91f..78584f01d052 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,
> +			int type);
>  	int (*write_reg)(struct device *dev, u16 reg_address, u32 val,
>  			 int type);
>  	int irq;

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 8/8] staging:iio:ade7854: Remove read_reg_* duplications

[Jonathan Cameron]

On Fri, 16 Mar 2018 19:50:29 -0300
Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> wrote:

> 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>
The meaning of John's signed-off-by here needs clarifying..

Also fix up the common on error handling as I believe you now do that in
an earlier patch.

Thanks and looking forward to v3.  This is a nice little bit of cleanup.
These weird devices with variable register sizes are always somewhat of
a pain to deal with.

Jonathan

> ---
>  drivers/staging/iio/meter/ade7854-i2c.c | 37 +------------------------------
>  drivers/staging/iio/meter/ade7854-spi.c | 39 ++-------------------------------
>  drivers/staging/iio/meter/ade7854.c     | 18 +++++++--------
>  drivers/staging/iio/meter/ade7854.h     |  7 +++---
>  4 files changed, 15 insertions(+), 86 deletions(-)
> 
> diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
> index 20db8eedb84a..162c171a8851 100644
> --- a/drivers/staging/iio/meter/ade7854-i2c.c
> +++ b/drivers/staging/iio/meter/ade7854-i2c.c
> @@ -110,38 +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)
> -{
> -	return ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
> -				    DATA_SIZE_8_BITS);
> -}
> -
> -static int ade7854_i2c_read_reg_16(struct device *dev,
> -				   u16 reg_address,
> -				   u16 *val)
> -{
> -	return ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
> -				    DATA_SIZE_16_BITS);
> -}
> -
> -static int ade7854_i2c_read_reg_24(struct device *dev,
> -				   u16 reg_address,
> -				   u32 *val)
> -{
> -	return ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
> -				    DATA_SIZE_24_BITS);
> -}
> -
> -static int ade7854_i2c_read_reg_32(struct device *dev,
> -				   u16 reg_address,
> -				   u32 *val)
> -{
> -	return ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
> -				    DATA_SIZE_32_BITS);
> -}
> -
>  static int ade7854_i2c_probe(struct i2c_client *client,
>  			     const struct i2c_device_id *id)
>  {
> @@ -153,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 964d6c6e76d1..66b8a8767a26 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 unlock;
> @@ -120,38 +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)
> -{
> -	return ade7854_spi_read_reg(dev, reg_address, (u32 *)val,
> -				    DATA_SIZE_8_BITS);
> -}
> -
> -static int ade7854_spi_read_reg_16(struct device *dev,
> -				   u16 reg_address,
> -				   u16 *val)
> -{
> -	return ade7854_spi_read_reg(dev, reg_address, (u32 *)val,
> -				    DATA_SIZE_16_BITS);
> -}
> -
> -static int ade7854_spi_read_reg_24(struct device *dev,
> -				   u16 reg_address,
> -				   u32 *val)
> -{
> -	return ade7854_spi_read_reg(dev, reg_address, (u32 *)val,
> -				    DATA_SIZE_24_BITS);
> -}
> -
> -static int ade7854_spi_read_reg_32(struct device *dev,
> -				   u16 reg_address,
> -				   u32 *val)
> -{
> -	return ade7854_spi_read_reg(dev, reg_address, (u32 *)val,
> -				    DATA_SIZE_32_BITS);
> -}
> -
>  static int ade7854_spi_probe(struct spi_device *spi)
>  {
>  	struct ade7854_state *st;
> @@ -162,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 4aa7c6ce0016..09fd8c067738 100644
> --- a/drivers/staging/iio/meter/ade7854.c
> +++ b/drivers/staging/iio/meter/ade7854.c
> @@ -27,12 +27,12 @@ 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);
> +	ret = st->read_reg(dev, this_attr->address, &val, DATA_SIZE_8_BITS);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -44,12 +44,12 @@ 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);
> +	ret = st->read_reg(dev, this_attr->address, &val, DATA_SIZE_16_BITS);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -66,7 +66,7 @@ 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);
> +	ret = st->read_reg(dev, this_attr->address, &val, DATA_SIZE_24_BITS);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -83,7 +83,7 @@ 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);
> +	ret = st->read_reg(dev, this_attr->address, &val, DATA_SIZE_32_BITS);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -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,7 +415,7 @@ static int ade7854_set_irq(struct device *dev, bool enable)
>  	int ret;
>  	u32 irqen;
>  
> -	ret = st->read_reg_32(dev, ADE7854_MASK0, &irqen);
> +	ret = st->read_reg(dev, ADE7854_MASK0, &irqen, DATA_SIZE_32_BITS);
>  	if (ret < 0)
>  		return ret;
>  
> diff --git a/drivers/staging/iio/meter/ade7854.h b/drivers/staging/iio/meter/ade7854.h
> index aa29faf7c91f..78584f01d052 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,
> +			int type);
>  	int (*write_reg)(struct device *dev, u16 reg_address, u32 val,
>  			 int type);
>  	int irq;