diff mbox series

[v4,3/7] iio: light: al3320a slightly improve code formatting

Message ID 20200204093031.616409-4-david@ixit.cz (mailing list archive)
State Superseded
Headers show
Series AL3010 introduction | expand

Commit Message

David Heidelberg Feb. 4, 2020, 9:30 a.m. UTC 
- modified to be in part with al3010 driver
- cleanup using bitfield, no functionality change intended

Signed-off-by: David Heidelberg <david@ixit.cz>

v4
 - use GENMASK() and bitfields
 - sort headers
 - drop init.h unused header

Signed-off-by: David Heidelberg <david@ixit.cz>
---
 drivers/iio/light/al3320a.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

Comments

Jonathan Cameron Feb. 6, 2020, 11:42 a.m. UTC | #1 
On Tue,  4 Feb 2020 10:30:32 +0100
David Heidelberg <david@ixit.cz> wrote:

> - modified to be in part with al3010 driver
> - cleanup using bitfield, no functionality change intended
> 
> Signed-off-by: David Heidelberg <david@ixit.cz>
> 
> v4
>  - use GENMASK() and bitfields
>  - sort headers
>  - drop init.h unused header
> 
> Signed-off-by: David Heidelberg <david@ixit.cz>
Looks good to me.  Will pick it up once rest of series is ready.

Thanks,

Jonathan

> ---
>  drivers/iio/light/al3320a.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/light/al3320a.c b/drivers/iio/light/al3320a.c
> index a21aa99e74e4..89b935b4fe9e 100644
> --- a/drivers/iio/light/al3320a.c
> +++ b/drivers/iio/light/al3320a.c
> @@ -9,9 +9,9 @@
>   * TODO: interrupt support, thresholds
>   */
>  
> -#include <linux/module.h>
> -#include <linux/init.h>
> +#include <linux/bitfield.h>
>  #include <linux/i2c.h>
> +#include <linux/module.h>
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> @@ -36,8 +36,7 @@
>  #define AL3320A_CONFIG_DISABLE		0x00
>  #define AL3320A_CONFIG_ENABLE		0x01
>  
> -#define AL3320A_GAIN_SHIFT		1
> -#define AL3320A_GAIN_MASK		(BIT(2) | BIT(1))
> +#define AL3320A_GAIN_MASK		GENMASK(2, 1)
>  
>  /* chip params default values */
>  #define AL3320A_DEFAULT_MEAN_TIME	4
> @@ -90,7 +89,8 @@ static int al3320a_init(struct al3320a_data *data)
>  		return ret;
>  
>  	ret = i2c_smbus_write_byte_data(data->client, AL3320A_REG_CONFIG_RANGE,
> -					AL3320A_RANGE_3 << AL3320A_GAIN_SHIFT);
> +					FIELD_PREP(AL3320A_GAIN_MASK,
> +						   AL3320A_RANGE_3));
>  	if (ret < 0)
>  		return ret;
>  
> @@ -133,7 +133,7 @@ static int al3320a_read_raw(struct iio_dev *indio_dev,
>  		if (ret < 0)
>  			return ret;
>  
> -		ret = (ret & AL3320A_GAIN_MASK) >> AL3320A_GAIN_SHIFT;
> +		ret = FIELD_GET(AL3320A_GAIN_MASK, ret)
>  		*val = al3320a_scales[ret][0];
>  		*val2 = al3320a_scales[ret][1];
>  
> @@ -152,11 +152,13 @@ static int al3320a_write_raw(struct iio_dev *indio_dev,
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SCALE:
>  		for (i = 0; i < ARRAY_SIZE(al3320a_scales); i++) {
> -			if (val == al3320a_scales[i][0] &&
> -			    val2 == al3320a_scales[i][1])
> -				return i2c_smbus_write_byte_data(data->client,
> +			if (val != al3320a_scales[i][0] ||
> +			    val2 != al3320a_scales[i][1])
> +				continue;
> +
> +			return i2c_smbus_write_byte_data(data->client,
>  					AL3320A_REG_CONFIG_RANGE,
> -					i << AL3320A_GAIN_SHIFT);
> +					FIELD_PREP(AL3320A_GAIN_MASK, i));
>  		}
>  		break;
>  	}
Dmitry Osipenko Feb. 6, 2020, 2:50 p.m. UTC | #2 
04.02.2020 12:30, David Heidelberg пишет:
...
>  #define AL3320A_CONFIG_DISABLE		0x00
>  #define AL3320A_CONFIG_ENABLE		0x01
>  
> -#define AL3320A_GAIN_SHIFT		1
> -#define AL3320A_GAIN_MASK		(BIT(2) | BIT(1))
> +#define AL3320A_GAIN_MASK		GENMASK(2, 1)

Looks like you forgot to compile-test this driver because the
AL3320A_GAIN_SHIFT is removed above, while it's still in-use below.

>  /* chip params default values */
>  #define AL3320A_DEFAULT_MEAN_TIME	4
> @@ -90,7 +89,8 @@ static int al3320a_init(struct al3320a_data *data)
>  		return ret;
>  
>  	ret = i2c_smbus_write_byte_data(data->client, AL3320A_REG_CONFIG_RANGE,
> -					AL3320A_RANGE_3 << AL3320A_GAIN_SHIFT);
> +					FIELD_PREP(AL3320A_GAIN_MASK,
> +						   AL3320A_RANGE_3));
...
Dmitry Osipenko Feb. 6, 2020, 2:51 p.m. UTC | #3 
06.02.2020 17:50, Dmitry Osipenko пишет:
> 04.02.2020 12:30, David Heidelberg пишет:
> ...
>>  #define AL3320A_CONFIG_DISABLE		0x00
>>  #define AL3320A_CONFIG_ENABLE		0x01
>>  
>> -#define AL3320A_GAIN_SHIFT		1
>> -#define AL3320A_GAIN_MASK		(BIT(2) | BIT(1))
>> +#define AL3320A_GAIN_MASK		GENMASK(2, 1)
> 
> Looks like you forgot to compile-test this driver because the
> AL3320A_GAIN_SHIFT is removed above, while it's still in-use below.
> 
>>  /* chip params default values */
>>  #define AL3320A_DEFAULT_MEAN_TIME	4
>> @@ -90,7 +89,8 @@ static int al3320a_init(struct al3320a_data *data)
>>  		return ret;
>>  
>>  	ret = i2c_smbus_write_byte_data(data->client, AL3320A_REG_CONFIG_RANGE,
>> -					AL3320A_RANGE_3 << AL3320A_GAIN_SHIFT);
>> +					FIELD_PREP(AL3320A_GAIN_MASK,
>> +						   AL3320A_RANGE_3));
> ...
> 

Ah, my bad! Sorry :)
diff mbox series

Patch

diff --git a/drivers/iio/light/al3320a.c b/drivers/iio/light/al3320a.c
index a21aa99e74e4..89b935b4fe9e 100644
--- a/drivers/iio/light/al3320a.c
+++ b/drivers/iio/light/al3320a.c
@@ -9,9 +9,9 @@ 
  * TODO: interrupt support, thresholds
  */
 
-#include <linux/module.h>
-#include <linux/init.h>
+#include <linux/bitfield.h>
 #include <linux/i2c.h>
+#include <linux/module.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
@@ -36,8 +36,7 @@ 
 #define AL3320A_CONFIG_DISABLE		0x00
 #define AL3320A_CONFIG_ENABLE		0x01
 
-#define AL3320A_GAIN_SHIFT		1
-#define AL3320A_GAIN_MASK		(BIT(2) | BIT(1))
+#define AL3320A_GAIN_MASK		GENMASK(2, 1)
 
 /* chip params default values */
 #define AL3320A_DEFAULT_MEAN_TIME	4
@@ -90,7 +89,8 @@  static int al3320a_init(struct al3320a_data *data)
 		return ret;
 
 	ret = i2c_smbus_write_byte_data(data->client, AL3320A_REG_CONFIG_RANGE,
-					AL3320A_RANGE_3 << AL3320A_GAIN_SHIFT);
+					FIELD_PREP(AL3320A_GAIN_MASK,
+						   AL3320A_RANGE_3));
 	if (ret < 0)
 		return ret;
 
@@ -133,7 +133,7 @@  static int al3320a_read_raw(struct iio_dev *indio_dev,
 		if (ret < 0)
 			return ret;
 
-		ret = (ret & AL3320A_GAIN_MASK) >> AL3320A_GAIN_SHIFT;
+		ret = FIELD_GET(AL3320A_GAIN_MASK, ret)
 		*val = al3320a_scales[ret][0];
 		*val2 = al3320a_scales[ret][1];
 
@@ -152,11 +152,13 @@  static int al3320a_write_raw(struct iio_dev *indio_dev,
 	switch (mask) {
 	case IIO_CHAN_INFO_SCALE:
 		for (i = 0; i < ARRAY_SIZE(al3320a_scales); i++) {
-			if (val == al3320a_scales[i][0] &&
-			    val2 == al3320a_scales[i][1])
-				return i2c_smbus_write_byte_data(data->client,
+			if (val != al3320a_scales[i][0] ||
+			    val2 != al3320a_scales[i][1])
+				continue;
+
+			return i2c_smbus_write_byte_data(data->client,
 					AL3320A_REG_CONFIG_RANGE,
-					i << AL3320A_GAIN_SHIFT);
+					FIELD_PREP(AL3320A_GAIN_MASK, i));
 		}
 		break;
 	}