[v2] iio: adc: spear_adc: mask SPEAR_ADC_STATUS channel and avg sample before setting register

[PATCH v2] iio: adc: spear_adc: mask SPEAR_ADC_STATUS channel and avg sample before setting register

Posted by Rodrigo Gobbi 7 months, 1 week ago

avg sample info is a bit field coded inside the following
bits: 5,6,7 and 8 of a device status register.

channel num info the same, but over bits: 1, 2 and 3.

mask both values in order to avoid touching other register bits,
since the first info (avg sample), came from dt.

Signed-off-by: Rodrigo Gobbi <rodrigo.gobbi.7@gmail.com>
---
Tks for the tip, @David, I didn`t know those macros.
Best regards!

Changelog:
v2: use proper bitfield macros + change at commit msg
v1: https://lore.kernel.org/linux-iio/20250621185301.9536-1-rodrigo.gobbi.7@gmail.com/#t
---
 drivers/iio/adc/spear_adc.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/spear_adc.c b/drivers/iio/adc/spear_adc.c
index e3a865c79686..ff7fb13fe947 100644
--- a/drivers/iio/adc/spear_adc.c
+++ b/drivers/iio/adc/spear_adc.c
@@ -21,6 +21,8 @@
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 
+#include <linux/bitfield.h>
+
 /* SPEAR registers definitions */
 #define SPEAR600_ADC_SCAN_RATE_LO(x)	((x) & 0xFFFF)
 #define SPEAR600_ADC_SCAN_RATE_HI(x)	(((x) >> 0x10) & 0xFFFF)
@@ -29,9 +31,9 @@
 
 /* Bit definitions for SPEAR_ADC_STATUS */
 #define SPEAR_ADC_STATUS_START_CONVERSION	BIT(0)
-#define SPEAR_ADC_STATUS_CHANNEL_NUM(x)		((x) << 1)
+#define SPEAR_ADC_STATUS_CHANNEL_NUM_MASK    GENMASK(3, 1)
 #define SPEAR_ADC_STATUS_ADC_ENABLE		BIT(4)
-#define SPEAR_ADC_STATUS_AVG_SAMPLE(x)		((x) << 5)
+#define SPEAR_ADC_STATUS_AVG_SAMPLE_MASK    GENMASK(8, 5)
 #define SPEAR_ADC_STATUS_VREF_INTERNAL		BIT(9)
 
 #define SPEAR_ADC_DATA_MASK		0x03ff
@@ -157,8 +159,8 @@ static int spear_adc_read_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_RAW:
 		mutex_lock(&st->lock);
 
-		status = SPEAR_ADC_STATUS_CHANNEL_NUM(chan->channel) |
-			SPEAR_ADC_STATUS_AVG_SAMPLE(st->avg_samples) |
+		status = FIELD_PREP(SPEAR_ADC_STATUS_CHANNEL_NUM_MASK, chan->channel) |
+			FIELD_PREP(SPEAR_ADC_STATUS_AVG_SAMPLE_MASK, st->avg_samples) |
 			SPEAR_ADC_STATUS_START_CONVERSION |
 			SPEAR_ADC_STATUS_ADC_ENABLE;
 		if (st->vref_external == 0)
-- 
2.48.1

Re: [PATCH v2] iio: adc: spear_adc: mask SPEAR_ADC_STATUS channel and avg sample before setting register

Posted by Andy Shevchenko 7 months, 1 week ago

On Tue, Jul 01, 2025 at 06:34:05PM -0300, Rodrigo Gobbi wrote:
> avg sample info is a bit field coded inside the following
> bits: 5,6,7 and 8 of a device status register.
> 
> channel num info the same, but over bits: 1, 2 and 3.
> 
> mask both values in order to avoid touching other register bits,
> since the first info (avg sample), came from dt.

...

> +#define SPEAR_ADC_STATUS_CHANNEL_NUM_MASK    GENMASK(3, 1)

You have a problem with indentation.

...

> +#define SPEAR_ADC_STATUS_AVG_SAMPLE_MASK    GENMASK(8, 5)

Ditto,

...

And I don't want to check all '+' lines in your changes, please make sure your
editor, mail user agent, other tools you are using, do not mangle the patch and
setup correctly for the indentation style used in the code.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] iio: adc: spear_adc: mask SPEAR_ADC_STATUS channel and avg sample before setting register

Posted by David Lechner 7 months, 1 week ago

On 7/1/25 4:34 PM, Rodrigo Gobbi wrote:
> avg sample info is a bit field coded inside the following
> bits: 5,6,7 and 8 of a device status register.
> 
> channel num info the same, but over bits: 1, 2 and 3.
> 
> mask both values in order to avoid touching other register bits,
> since the first info (avg sample), came from dt.
> 
> Signed-off-by: Rodrigo Gobbi <rodrigo.gobbi.7@gmail.com>
> ---
> Tks for the tip, @David, I didn`t know those macros.
> Best regards!

:-)

> 
> Changelog:
> v2: use proper bitfield macros + change at commit msg
> v1: https://lore.kernel.org/linux-iio/20250621185301.9536-1-rodrigo.gobbi.7@gmail.com/#t
> ---
>  drivers/iio/adc/spear_adc.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/spear_adc.c b/drivers/iio/adc/spear_adc.c
> index e3a865c79686..ff7fb13fe947 100644
> --- a/drivers/iio/adc/spear_adc.c
> +++ b/drivers/iio/adc/spear_adc.c
> @@ -21,6 +21,8 @@
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
>  
> +#include <linux/bitfield.h>

This should be moved up with the other non-iio includes.

For bonus points, a separate patch that cleans up and sorts the existing
includes would be appreciated.

add:
#include "linux/array_size.h"        // for ARRAY_SIZE
#include "linux/compiler_types.h"    // for __iomem
#include "linux/dev_printk.h"        // for dev_err_probe, dev_info
#include "linux/math.h"              // for DIV_ROUND_UP
#include "linux/mutex.h"             // for mutex_unlock, mutex_lock, mutex_...

remove unused:
#include <linux/device.h>
#include <linux/iio/sysfs.h>
#include <linux/kernel.h>
#include <linux/slab.h>

> +
>  /* SPEAR registers definitions */
>  #define SPEAR600_ADC_SCAN_RATE_LO(x)	((x) & 0xFFFF)
>  #define SPEAR600_ADC_SCAN_RATE_HI(x)	(((x) >> 0x10) & 0xFFFF)
> @@ -29,9 +31,9 @@
>  
>  /* Bit definitions for SPEAR_ADC_STATUS */
>  #define SPEAR_ADC_STATUS_START_CONVERSION	BIT(0)
> -#define SPEAR_ADC_STATUS_CHANNEL_NUM(x)		((x) << 1)
> +#define SPEAR_ADC_STATUS_CHANNEL_NUM_MASK    GENMASK(3, 1)
>  #define SPEAR_ADC_STATUS_ADC_ENABLE		BIT(4)
> -#define SPEAR_ADC_STATUS_AVG_SAMPLE(x)		((x) << 5)
> +#define SPEAR_ADC_STATUS_AVG_SAMPLE_MASK    GENMASK(8, 5)
>  #define SPEAR_ADC_STATUS_VREF_INTERNAL		BIT(9)
>  
>  #define SPEAR_ADC_DATA_MASK		0x03ff
> @@ -157,8 +159,8 @@ static int spear_adc_read_raw(struct iio_dev *indio_dev,
>  	case IIO_CHAN_INFO_RAW:
>  		mutex_lock(&st->lock);
>  
> -		status = SPEAR_ADC_STATUS_CHANNEL_NUM(chan->channel) |
> -			SPEAR_ADC_STATUS_AVG_SAMPLE(st->avg_samples) |
> +		status = FIELD_PREP(SPEAR_ADC_STATUS_CHANNEL_NUM_MASK, chan->channel) |
> +			FIELD_PREP(SPEAR_ADC_STATUS_AVG_SAMPLE_MASK, st->avg_samples) |
>  			SPEAR_ADC_STATUS_START_CONVERSION |
>  			SPEAR_ADC_STATUS_ADC_ENABLE;
>  		if (st->vref_external == 0)