[PATCH 0/7] cm3323 fixes and cleanups

[PATCH 0/7] cm3323 fixes and cleanups

[Hartmut Knaack]

Some small fixes and cleanups.

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 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..1e6092a 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 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>
---
 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 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>
---
 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 1e6092a..f88a0ed 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 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 f88a0ed..8945e0c 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 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>
---
 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 8945e0c..6bb75e7 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 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 6bb75e7..4f362d5 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(4, 6)
 #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 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>
---
 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 4f362d5..fbd841e 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 6/7] iio:light:cm3323: make use of GENMASK

[Peter Meerwald]

> Use GENMASK to define the integration time bitmask.

Nack, should be GENMASK(6, 4) I think
 
> 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 6bb75e7..4f362d5 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(4, 6)
>  #define CM3323_CONF_IT_SHIFT	4
>  
>  #define CM3323_INT_TIME_AVAILABLE "0.04 0.08 0.16 0.32 0.64 1.28"
> 

-- 

Peter Meerwald
+43-664-2444418 (mobile)

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

[Hartmut Knaack]

Peter Meerwald schrieb am 17.06.2015 um 00:29:
> 
>> Use GENMASK to define the integration time bitmask.
> 
> Nack, should be GENMASK(6, 4) I think

Thanks, you are totally right. Seems like I wasn't in the
best shape for coding yesterday. Will fix this and actually
double check this series for other issues.
Thanks,

Hartmut

>  
>> 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 6bb75e7..4f362d5 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(4, 6)
>>  #define CM3323_CONF_IT_SHIFT	4
>>  
>>  #define CM3323_INT_TIME_AVAILABLE "0.04 0.08 0.16 0.32 0.64 1.28"
>>
> 

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

[Hartmut Knaack]

Hartmut Knaack schrieb am 17.06.2015 um 00:17:
> When setting the bits for integration time, the appropriate bitmask needs
> to be cleared first.
> 

And it needs to be done right. I will fix it in V2.

> 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..1e6092a 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,
> 

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

[Daniel Baluta]

On Wed, Jun 17, 2015 at 1:17 AM, Hartmut Knaack <knaack.h@gmx.de> 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>

> ---
>  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
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Daniel Baluta]

On Wed, Jun 17, 2015 at 1:17 AM, Hartmut Knaack <knaack.h@gmx.de> 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>

Agree, it's better this way.

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 1e6092a..f88a0ed 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
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Daniel Baluta]

On Wed, Jun 17, 2015 at 1:17 AM, Hartmut Knaack <knaack.h@gmx.de> wrote:
> 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>

I don't really like this. It makes code harder to read.

Other opinions?

> ---
>  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 f88a0ed..8945e0c 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");
>  }
>
> --

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

[Daniel Baluta]

On Wed, Jun 17, 2015 at 1:17 AM, Hartmut Knaack <knaack.h@gmx.de> 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>

Fair enough.

Reviewed-by: Daniel Baluta <daniel.baluta@intel.com>

Are you using a tool for finding such situations? Or just code reading ? :)

> ---
>  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 8945e0c..6bb75e7 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;

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

[Daniel Baluta]

On Wed, Jun 17, 2015 at 1:17 AM, Hartmut Knaack <knaack.h@gmx.de> 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>

> ---
>  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 4f362d5..fbd841e 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);
> --

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

[Hartmut Knaack]

Daniel Baluta schrieb am 17.06.2015 um 12:14:
> On Wed, Jun 17, 2015 at 1:17 AM, Hartmut Knaack <knaack.h@gmx.de> 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>
> 
> Fair enough.
> 
> Reviewed-by: Daniel Baluta <daniel.baluta@intel.com>
> 
> Are you using a tool for finding such situations? Or just code reading ? :)

Plain code reading FTW ;-) Working through a checklist and questioning every
piece of code for its benefit.

> 
>> ---
>>  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 8945e0c..6bb75e7 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
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[Jonathan Cameron]

On 17/06/15 11:09, Daniel Baluta wrote:
> On Wed, Jun 17, 2015 at 1:17 AM, Hartmut Knaack <knaack.h@gmx.de> wrote:
>> 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>
> 
> I don't really like this. It makes code harder to read.
> 
> Other opinions?
> 
I'm going to side with Daniel on this one...  Lets leave it as is.

Jonathan
>> ---
>>  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 f88a0ed..8945e0c 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");
>>  }
>>
>> --

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