cm3323 fixes and cleanups
Subsystems (2)
cover cm3323 fixes and cleanups
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(-)
Signature · 1 line
2.3.6
PATCH 1/7 iio:light:cm3323: clear bitmask before set
When setting the bits for integration time, the appropriate bitmask needs to be cleared first.
--- drivers/iio/light/cm3323.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/iio/light/cm3323.c +1 -1
@@ -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,
Signature · 1 line
2.3.6
Re: [PATCH 1/7] iio:light:cm3323: clear bitmask before set
Hartmut Knaack schrieb am 17.06.2015 um 00:17:
Quoted 3 lines
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.
Quoted 19 lines
Signed-off-by Hartmut Knaack <[email protected]>--- drivers/iio/light/cm3323.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/iio/light/cm3323.c +1
-1@@ -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,
PATCH 2/7 iio:light:Kconfig: fix typo in description
In reply to Hartmut KnaackFix the typo in the module description for the CM3323.
--- drivers/iio/light/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/iio/light/Kconfig +1 -1
@@ -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
Signature · 1 line
2.3.6
Re: [PATCH 2/7] iio:light:Kconfig: fix typo in description
Quoted 3 lines · Hartmut Knaack
Fix the typo in the module description for the CM3323.
Signed-off-by Hartmut Knaack <[email protected]>
Quoted 24 lines
--- drivers/iio/light/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/iio/light/Kconfig +1
-2@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 3/7 iio:light:cm3323: pass up error value
In reply to Hartmut Knaackcm3323_get_it_bits() returns a valid error code, so pass it up in cm3323_read_raw().
--- drivers/iio/light/cm3323.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/iio/light/cm3323.c +1 -1
@@ -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;
Signature · 1 line
2.3.6
Re: [PATCH 3/7] iio:light:cm3323: pass up error value
Quoted 4 lines · Hartmut Knaack
cm3323_get_it_bits()returns a valid error code, so pass it up incm3323_read_raw().Signed-off-by Hartmut Knaack <[email protected]>
Agree, it's better this way.
Quoted 24 lines
--- drivers/iio/light/cm3323.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/iio/light/cm3323.c +1
-2@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 4/7 iio:light:cm3323: drop barely used variable
In reply to Hartmut KnaackIn 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.
--- drivers/iio/light/cm3323.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
drivers/iio/light/cm3323.c +2 -4
@@ -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" ); }
Signature · 1 line
2.3.6
Re: [PATCH 4/7] iio:light:cm3323: drop barely used variable
Quoted 5 lines · 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 <[email protected]>
I don't really like this. It makes code harder to read.
Other opinions?
Quoted 24 lines
--- drivers/iio/light/cm3323.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)drivers/iio/light/cm3323.c +2
-5@@ -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" ); } --
Jonathan Cameron
maintainerRe: [PATCH 4/7] iio:light:cm3323: drop barely used variable
Quoted 11 lines · Daniel Baluta
Quoted 5 lines · 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 <[email protected]>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
Quoted 23 lines
Quoted 23 lines
--- drivers/iio/light/cm3323.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)drivers/iio/light/cm3323.c +2
-4@@ -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" ); }
Quoted 1 line
Quoted 1 line
--
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
PATCH 5/7 iio:light:cm3323: replace unneeded variable
In reply to Hartmut KnaackIn 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.
--- drivers/iio/light/cm3323.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/iio/light/cm3323.c +6 -6
@@ -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 ;
Signature · 1 line
2.3.6
Re: [PATCH 5/7] iio:light:cm3323: replace unneeded variable
Quoted 5 lines · 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 <[email protected]>
Fair enough.
Are you using a tool for finding such situations? Or just code reading ? :)
Quoted 37 lines
--- drivers/iio/light/cm3323.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)drivers/iio/light/cm3323.c +6
-6@@ -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 5/7] iio:light:cm3323: replace unneeded variable
Daniel Baluta schrieb am 17.06.2015 um 12:14:
Quoted 5 lines · Hartmut Knaack
Quoted 4 lines
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.
Quoted 7 lines
Quoted 1 line
Signed-off-by Hartmut Knaack <[email protected]>Fair enough.
Reviewed-by Daniel Baluta <[email protected]>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.
Quoted 43 lines
Quoted 28 lines
--- drivers/iio/light/cm3323.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)drivers/iio/light/cm3323.c +3
-4@@ -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;drivers/iio/light/cm3323.c static int cm3323_read_raw(struct iio_dev *indio_dev, · line 183
Quoted 10 lines
drivers/iio/light/cm3323.c +3
-2+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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 6/7 iio:light:cm3323: make use of GENMASK
In reply to Hartmut KnaackUse GENMASK to define the integration time bitmask.
--- drivers/iio/light/cm3323.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/iio/light/cm3323.c +1 -1
@@ -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"
Signature · 1 line
2.3.6
Peter Meerwald
NAKRe: [PATCH 6/7] iio:light:cm3323: make use of GENMASK
Quoted 2 lines
Use GENMASK to define the integration time bitmask.
Nack, should be GENMASK(6, 4) I think
Quoted 19 lines
Signed-off-by Hartmut Knaack <[email protected]>--- drivers/iio/light/cm3323.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/iio/light/cm3323.c +1
-1@@ -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"
Signature · 2 lines
Peter Meerwald +43-664-2444418 (mobile)
Re: [PATCH 6/7] iio:light:cm3323: make use of GENMASK
Peter Meerwald schrieb am 17.06.2015 um 00:29:
Quoted 4 lines
Quoted 2 lines
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
Quoted 21 lines
Quoted 20 lines
Signed-off-by Hartmut Knaack <[email protected]>--- drivers/iio/light/cm3323.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/iio/light/cm3323.c +1
-1@@ -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"
PATCH 7/7 iio:light:cm3323: add empty lines for code structure
In reply to Hartmut KnaackAdd some empty lines to visually separate logical structure blocks, as after if-blocks or before regular returns.
--- drivers/iio/light/cm3323.c | 5 +++++ 1 file changed, 5 insertions(+)
drivers/iio/light/cm3323.c +5 -0
@@ -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);
Signature · 1 line
2.3.6
Re: [PATCH 7/7] iio:light:cm3323: add empty lines for code structure
Quoted 4 lines · 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 <[email protected]>
Quoted 43 lines
--- drivers/iio/light/cm3323.c | 5 +++++ 1 file changed, 5 insertions(+)drivers/iio/light/cm3323.c +5
-1@@ -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); --