[PATCH v2 0/7] cm3323 fixes and cleanups

[PATCH v2 0/7] cm3323 fixes and cleanups

[Hartmut Knaack]

Some small fixes and cleanups.

Changes in V2:
  - apply the integration time bitmask right
  - fix order of GENMASK arguments as pointed out by Peter Meerwald

Hartmut Knaack (7):
  iio:light:cm3323: clear bitmask before set
  iio:light:Kconfig: fix typo in description
  iio:light:cm3323: pass up error value
  iio:light:cm3323: drop barely used variable
  iio:light:cm3323: replace unneeded variable
  iio:light:cm3323: make use of GENMASK
  iio:light:cm3323: add empty lines for code structure

 drivers/iio/light/Kconfig  |  2 +-
 drivers/iio/light/cm3323.c | 27 +++++++++++++++------------
 2 files changed, 16 insertions(+), 13 deletions(-)

-- 
2.3.6

[PATCH v2 1/7] iio:light:cm3323: clear bitmask before set

[Hartmut Knaack]

When setting the bits for integration time, the appropriate bitmask needs
to be cleared first.

Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
---
 drivers/iio/light/cm3323.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/light/cm3323.c b/drivers/iio/light/cm3323.c
index 869033e..a1d4905 100644
--- a/drivers/iio/light/cm3323.c
+++ b/drivers/iio/light/cm3323.c
@@ -123,7 +123,7 @@ static int cm3323_set_it_bits(struct cm3323_data *data, int val, int val2)
 	for (i = 0; i < ARRAY_SIZE(cm3323_int_time); i++) {
 		if (val == cm3323_int_time[i].val &&
 		    val2 == cm3323_int_time[i].val2) {
-			reg_conf = data->reg_conf;
+			reg_conf = data->reg_conf & ~CM3323_CONF_IT_MASK;
 			reg_conf |= i << CM3323_CONF_IT_SHIFT;
 
 			ret = i2c_smbus_write_word_data(data->client,
-- 
2.3.6

[PATCH v2 2/7] iio:light:Kconfig: fix typo in description

[Hartmut Knaack]

Fix the typo in the module description for the CM3323.

Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
Reviewed-by: Daniel Baluta <daniel.baluta@intel.com>
---
 drivers/iio/light/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index e6198b7..554f8ee 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -86,7 +86,7 @@ config CM3323
 	depends on I2C
 	tristate "Capella CM3323 color light sensor"
 	help
-	 Say Y here if you want to build a driver for Capela CM3323
+	 Say Y here if you want to build a driver for Capella CM3323
 	 color sensor.
 
 	 To compile this driver as a module, choose M here: the module will
-- 
2.3.6

[PATCH v2 3/7] iio:light:cm3323: pass up error value

[Hartmut Knaack]

cm3323_get_it_bits() returns a valid error code, so pass it up in
cm3323_read_raw().

Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
Reviewed-by: Daniel Baluta <daniel.baluta@intel.com>
---
 drivers/iio/light/cm3323.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/light/cm3323.c b/drivers/iio/light/cm3323.c
index a1d4905..feb4dbc 100644
--- a/drivers/iio/light/cm3323.c
+++ b/drivers/iio/light/cm3323.c
@@ -175,7 +175,7 @@ static int cm3323_read_raw(struct iio_dev *indio_dev,
 		i = cm3323_get_it_bits(data);
 		if (i < 0) {
 			mutex_unlock(&data->mutex);
-			return -EINVAL;
+			return i;
 		}
 
 		*val = cm3323_int_time[i].val;
-- 
2.3.6

[PATCH v2 4/7] iio:light:cm3323: drop barely used variable

[Hartmut Knaack]

In cm3323_disable() ret is just used to check for errors during I2C write.
Consolidate the write and the check to save a variable and two lines of
code.

Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
---
 drivers/iio/light/cm3323.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/light/cm3323.c b/drivers/iio/light/cm3323.c
index feb4dbc..9cd7d2a 100644
--- a/drivers/iio/light/cm3323.c
+++ b/drivers/iio/light/cm3323.c
@@ -106,12 +106,10 @@ static int cm3323_init(struct iio_dev *indio_dev)
 
 static void cm3323_disable(struct iio_dev *indio_dev)
 {
-	int ret;
 	struct cm3323_data *data = iio_priv(indio_dev);
 
-	ret = i2c_smbus_write_word_data(data->client, CM3323_CMD_CONF,
-					CM3323_CONF_SD_BIT);
-	if (ret < 0)
+	if (i2c_smbus_write_word_data(data->client, CM3323_CMD_CONF,
+				      CM3323_CONF_SD_BIT) < 0)
 		dev_err(&data->client->dev, "Error writing reg_conf\n");
 }
 
-- 
2.3.6

[PATCH v2 5/7] iio:light:cm3323: replace unneeded variable

[Hartmut Knaack]

In cm3323_read_raw() i is used as return variable for the integration time
index. The also existing return variable ret however is unused in this
case, although appropriate. Replace i with ret and drop it.

Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
Reviewed-by: Daniel Baluta <daniel.baluta@intel.com>
---
 drivers/iio/light/cm3323.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/light/cm3323.c b/drivers/iio/light/cm3323.c
index 9cd7d2a..2c49ec2 100644
--- a/drivers/iio/light/cm3323.c
+++ b/drivers/iio/light/cm3323.c
@@ -153,7 +153,7 @@ static int cm3323_read_raw(struct iio_dev *indio_dev,
 			   struct iio_chan_spec const *chan, int *val,
 			   int *val2, long mask)
 {
-	int i, ret;
+	int ret;
 	struct cm3323_data *data = iio_priv(indio_dev);
 
 	switch (mask) {
@@ -170,14 +170,14 @@ static int cm3323_read_raw(struct iio_dev *indio_dev,
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_INT_TIME:
 		mutex_lock(&data->mutex);
-		i = cm3323_get_it_bits(data);
-		if (i < 0) {
+		ret = cm3323_get_it_bits(data);
+		if (ret < 0) {
 			mutex_unlock(&data->mutex);
-			return i;
+			return ret;
 		}
 
-		*val = cm3323_int_time[i].val;
-		*val2 = cm3323_int_time[i].val2;
+		*val = cm3323_int_time[ret].val;
+		*val2 = cm3323_int_time[ret].val2;
 		mutex_unlock(&data->mutex);
 
 		return IIO_VAL_INT_PLUS_MICRO;
-- 
2.3.6

[PATCH v2 6/7] iio:light:cm3323: make use of GENMASK

[Hartmut Knaack]

Use GENMASK to define the integration time bitmask.

Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
---
 drivers/iio/light/cm3323.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/light/cm3323.c b/drivers/iio/light/cm3323.c
index 2c49ec2..1fc9f78 100644
--- a/drivers/iio/light/cm3323.c
+++ b/drivers/iio/light/cm3323.c
@@ -29,7 +29,7 @@
 
 #define CM3323_CONF_SD_BIT	BIT(0) /* sensor disable */
 #define CM3323_CONF_AF_BIT	BIT(1) /* auto/manual force mode */
-#define CM3323_CONF_IT_MASK	(BIT(4) | BIT(5) | BIT(6))
+#define CM3323_CONF_IT_MASK	GENMASK(6, 4)
 #define CM3323_CONF_IT_SHIFT	4
 
 #define CM3323_INT_TIME_AVAILABLE "0.04 0.08 0.16 0.32 0.64 1.28"
-- 
2.3.6

[PATCH v2 7/7] iio:light:cm3323: add empty lines for code structure

[Hartmut Knaack]

Add some empty lines to visually separate logical structure blocks, as
after if-blocks or before regular returns.

Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
Reviewed-by: Daniel Baluta <daniel.baluta@intel.com>
---
 drivers/iio/light/cm3323.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/iio/light/cm3323.c b/drivers/iio/light/cm3323.c
index 1fc9f78..0831f06 100644
--- a/drivers/iio/light/cm3323.c
+++ b/drivers/iio/light/cm3323.c
@@ -131,9 +131,11 @@ static int cm3323_set_it_bits(struct cm3323_data *data, int val, int val2)
 				return ret;
 
 			data->reg_conf = reg_conf;
+
 			return 0;
 		}
 	}
+
 	return -EINVAL;
 }
 
@@ -146,6 +148,7 @@ static int cm3323_get_it_bits(struct cm3323_data *data)
 
 	if (bits >= ARRAY_SIZE(cm3323_int_time))
 		return -EINVAL;
+
 	return bits;
 }
 
@@ -241,11 +244,13 @@ static int cm3323_probe(struct i2c_client *client,
 		dev_err(&client->dev, "cm3323 chip init failed\n");
 		return ret;
 	}
+
 	ret = iio_device_register(indio_dev);
 	if (ret < 0) {
 		dev_err(&client->dev, "failed to register iio dev\n");
 		goto err_init;
 	}
+
 	return 0;
 err_init:
 	cm3323_disable(indio_dev);
-- 
2.3.6

Re: [PATCH v2 0/7] cm3323 fixes and cleanups

[Jonathan Cameron]

On 17/06/15 23:31, Hartmut Knaack wrote:
> Some small fixes and cleanups.
> 
> Changes in V2:
>   - apply the integration time bitmask right
>   - fix order of GENMASK arguments as pointed out by Peter Meerwald
> 
> Hartmut Knaack (7):
>   iio:light:cm3323: clear bitmask before set
>   iio:light:Kconfig: fix typo in description
>   iio:light:cm3323: pass up error value
>   iio:light:cm3323: drop barely used variable
>   iio:light:cm3323: replace unneeded variable
>   iio:light:cm3323: make use of GENMASK
>   iio:light:cm3323: add empty lines for code structure
> 
>  drivers/iio/light/Kconfig  |  2 +-
>  drivers/iio/light/cm3323.c | 27 +++++++++++++++------------
>  2 files changed, 16 insertions(+), 13 deletions(-)
> 
This whole series looks good to me, but I'll let them sit
for Daniel to have a look as it's his driver.

Path 1 is clearly a bug fix, but the others can go the slow
route.
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in

Re: [PATCH v2 0/7] cm3323 fixes and cleanups

[Jonathan Cameron]

On 21/06/15 14:19, Jonathan Cameron wrote:
> On 17/06/15 23:31, Hartmut Knaack wrote:
>> Some small fixes and cleanups.
>>
>> Changes in V2:
>>   - apply the integration time bitmask right
>>   - fix order of GENMASK arguments as pointed out by Peter Meerwald
>>
>> Hartmut Knaack (7):
>>   iio:light:cm3323: clear bitmask before set
>>   iio:light:Kconfig: fix typo in description
>>   iio:light:cm3323: pass up error value
>>   iio:light:cm3323: drop barely used variable
>>   iio:light:cm3323: replace unneeded variable
>>   iio:light:cm3323: make use of GENMASK
>>   iio:light:cm3323: add empty lines for code structure
>>
>>  drivers/iio/light/Kconfig  |  2 +-
>>  drivers/iio/light/cm3323.c | 27 +++++++++++++++------------
>>  2 files changed, 16 insertions(+), 13 deletions(-)
>>
> This whole series looks good to me, but I'll let them sit
> for Daniel to have a look as it's his driver.
Or I could just check beyond patch 1 and see that he's already
reviewed them in V1. oops.
> 
> Path 1 is clearly a bug fix, but the others can go the slow
> route.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in

Re: [PATCH v2 1/7] iio:light:cm3323: clear bitmask before set

[Jonathan Cameron]

On 17/06/15 23:31, Hartmut Knaack wrote:
> When setting the bits for integration time, the appropriate bitmask needs
> to be cleared first.
> 
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
Applied to the fixes-togreg branch of iio.git and marked for stable.

Thanks,

Jonathan
> ---
>  drivers/iio/light/cm3323.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/light/cm3323.c b/drivers/iio/light/cm3323.c
> index 869033e..a1d4905 100644
> --- a/drivers/iio/light/cm3323.c
> +++ b/drivers/iio/light/cm3323.c
> @@ -123,7 +123,7 @@ static int cm3323_set_it_bits(struct cm3323_data *data, int val, int val2)
>  	for (i = 0; i < ARRAY_SIZE(cm3323_int_time); i++) {
>  		if (val == cm3323_int_time[i].val &&
>  		    val2 == cm3323_int_time[i].val2) {
> -			reg_conf = data->reg_conf;
> +			reg_conf = data->reg_conf & ~CM3323_CONF_IT_MASK;
>  			reg_conf |= i << CM3323_CONF_IT_SHIFT;
>  
>  			ret = i2c_smbus_write_word_data(data->client,
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in

Re: [PATCH v2 2/7] iio:light:Kconfig: fix typo in description

[Jonathan Cameron]

On 17/06/15 23:32, Hartmut Knaack wrote:
> Fix the typo in the module description for the CM3323.
> 
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
> Reviewed-by: Daniel Baluta <daniel.baluta@intel.com>
Applied to the togreg branch of iio.git

Thanks,

Jonathan
> ---
>  drivers/iio/light/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index e6198b7..554f8ee 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -86,7 +86,7 @@ config CM3323
>  	depends on I2C
>  	tristate "Capella CM3323 color light sensor"
>  	help
> -	 Say Y here if you want to build a driver for Capela CM3323
> +	 Say Y here if you want to build a driver for Capella CM3323
>  	 color sensor.
>  
>  	 To compile this driver as a module, choose M here: the module will
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in

Re: [PATCH v2 3/7] iio:light:cm3323: pass up error value

[Jonathan Cameron]

On 17/06/15 23:32, Hartmut Knaack wrote:
> cm3323_get_it_bits() returns a valid error code, so pass it up in
> cm3323_read_raw().
> 
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
> Reviewed-by: Daniel Baluta <daniel.baluta@intel.com>
Applied to the togreg branch of iio.git.

Thanks,

Jonathan
> ---
>  drivers/iio/light/cm3323.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/light/cm3323.c b/drivers/iio/light/cm3323.c
> index a1d4905..feb4dbc 100644
> --- a/drivers/iio/light/cm3323.c
> +++ b/drivers/iio/light/cm3323.c
> @@ -175,7 +175,7 @@ static int cm3323_read_raw(struct iio_dev *indio_dev,
>  		i = cm3323_get_it_bits(data);
>  		if (i < 0) {
>  			mutex_unlock(&data->mutex);
> -			return -EINVAL;
> +			return i;
>  		}
>  
>  		*val = cm3323_int_time[i].val;
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in

Re: [PATCH v2 5/7] iio:light:cm3323: replace unneeded variable

[Jonathan Cameron]

On 17/06/15 23:32, Hartmut Knaack wrote:
> In cm3323_read_raw() i is used as return variable for the integration time
> index. The also existing return variable ret however is unused in this
> case, although appropriate. Replace i with ret and drop it.
> 
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
> Reviewed-by: Daniel Baluta <daniel.baluta@intel.com>
applied to the togreg branch of iio.git

Thanks,
> ---
>  drivers/iio/light/cm3323.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/light/cm3323.c b/drivers/iio/light/cm3323.c
> index 9cd7d2a..2c49ec2 100644
> --- a/drivers/iio/light/cm3323.c
> +++ b/drivers/iio/light/cm3323.c
> @@ -153,7 +153,7 @@ static int cm3323_read_raw(struct iio_dev *indio_dev,
>  			   struct iio_chan_spec const *chan, int *val,
>  			   int *val2, long mask)
>  {
> -	int i, ret;
> +	int ret;
>  	struct cm3323_data *data = iio_priv(indio_dev);
>  
>  	switch (mask) {
> @@ -170,14 +170,14 @@ static int cm3323_read_raw(struct iio_dev *indio_dev,
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_INT_TIME:
>  		mutex_lock(&data->mutex);
> -		i = cm3323_get_it_bits(data);
> -		if (i < 0) {
> +		ret = cm3323_get_it_bits(data);
> +		if (ret < 0) {
>  			mutex_unlock(&data->mutex);
> -			return i;
> +			return ret;
>  		}
>  
> -		*val = cm3323_int_time[i].val;
> -		*val2 = cm3323_int_time[i].val2;
> +		*val = cm3323_int_time[ret].val;
> +		*val2 = cm3323_int_time[ret].val2;
>  		mutex_unlock(&data->mutex);
>  
>  		return IIO_VAL_INT_PLUS_MICRO;
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in

Re: [PATCH v2 6/7] iio:light:cm3323: make use of GENMASK

[Jonathan Cameron]

On 17/06/15 23:32, Hartmut Knaack wrote:
> Use GENMASK to define the integration time bitmask.
> 
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
Applied
> ---
>  drivers/iio/light/cm3323.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/light/cm3323.c b/drivers/iio/light/cm3323.c
> index 2c49ec2..1fc9f78 100644
> --- a/drivers/iio/light/cm3323.c
> +++ b/drivers/iio/light/cm3323.c
> @@ -29,7 +29,7 @@
>  
>  #define CM3323_CONF_SD_BIT	BIT(0) /* sensor disable */
>  #define CM3323_CONF_AF_BIT	BIT(1) /* auto/manual force mode */
> -#define CM3323_CONF_IT_MASK	(BIT(4) | BIT(5) | BIT(6))
> +#define CM3323_CONF_IT_MASK	GENMASK(6, 4)
>  #define CM3323_CONF_IT_SHIFT	4
>  
>  #define CM3323_INT_TIME_AVAILABLE "0.04 0.08 0.16 0.32 0.64 1.28"
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in

Re: [PATCH v2 7/7] iio:light:cm3323: add empty lines for code structure

[Jonathan Cameron]

On 17/06/15 23:32, Hartmut Knaack wrote:
> Add some empty lines to visually separate logical structure blocks, as
> after if-blocks or before regular returns.
> 
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
> Reviewed-by: Daniel Baluta <daniel.baluta@intel.com>
Applied.
> ---
>  drivers/iio/light/cm3323.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/iio/light/cm3323.c b/drivers/iio/light/cm3323.c
> index 1fc9f78..0831f06 100644
> --- a/drivers/iio/light/cm3323.c
> +++ b/drivers/iio/light/cm3323.c
> @@ -131,9 +131,11 @@ static int cm3323_set_it_bits(struct cm3323_data *data, int val, int val2)
>  				return ret;
>  
>  			data->reg_conf = reg_conf;
> +
>  			return 0;
>  		}
>  	}
> +
>  	return -EINVAL;
>  }
>  
> @@ -146,6 +148,7 @@ static int cm3323_get_it_bits(struct cm3323_data *data)
>  
>  	if (bits >= ARRAY_SIZE(cm3323_int_time))
>  		return -EINVAL;
> +
>  	return bits;
>  }
>  
> @@ -241,11 +244,13 @@ static int cm3323_probe(struct i2c_client *client,
>  		dev_err(&client->dev, "cm3323 chip init failed\n");
>  		return ret;
>  	}
> +
>  	ret = iio_device_register(indio_dev);
>  	if (ret < 0) {
>  		dev_err(&client->dev, "failed to register iio dev\n");
>  		goto err_init;
>  	}
> +
>  	return 0;
>  err_init:
>  	cm3323_disable(indio_dev);
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in