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

2018-03-18 Thread Jonathan Cameron 
On Fri, 16 Mar 2018 19:49:59 -0300
Rodrigo Siqueira  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 
> Signed-off-by: John Syne 
> ---
>  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(>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(>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(>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(>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(>buf_lock);
> - return ret;
> + return ade7854_i2c_read_reg(dev, reg_address, (u32 *)val,
> + DATA_SIZE_24_BITS);
>  }
>  
>  static int

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

2018-03-18 Thread Jonathan Cameron 
On Fri, 16 Mar 2018 19:49:59 -0300
Rodrigo Siqueira  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 
> Signed-off-by: John Syne 
> ---
>  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(>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(>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(>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(>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(>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

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

2018-03-16 Thread 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 
Signed-off-by: John Syne 
---
 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(>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(>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(>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(>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(>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(>buf_lock);
-   st->tx[0] = (reg_address >> 8) & 0xFF;
-   st->tx[1] =

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

2018-03-16 Thread 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 
Signed-off-by: John Syne 
---
 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(>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(>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(>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(>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(>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(>buf_lock);
-   st->tx[0] = (reg_address >> 8) & 0xFF;
-   st->tx[1] = reg_address & 0xFF;
-
-   ret =