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

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

Posted by Rodrigo Gobbi 2 months, 3 weeks 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>
---
About @David comment at v2:

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

I`ll change that in a later moment.
Tks and regards.

Changelog:
v3: moving up include; fix indentation;
v2: https://lore.kernel.org/linux-iio/20250701213728.32064-1-rodrigo.gobbi.7@gmail.com/
v1: https://lore.kernel.org/linux-iio/20250621185301.9536-1-rodrigo.gobbi.7@gmail.com/#t
---
 drivers/iio/adc/spear_adc.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/spear_adc.c b/drivers/iio/adc/spear_adc.c
index e3a865c79686..0acc65c74221 100644
--- a/drivers/iio/adc/spear_adc.c
+++ b/drivers/iio/adc/spear_adc.c
@@ -17,6 +17,7 @@
 #include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/completion.h>
+#include <linux/bitfield.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
@@ -29,9 +30,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 +158,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 v3] iio: adc: spear_adc: mask SPEAR_ADC_STATUS channel and avg sample before setting register

Posted by Andy Shevchenko 2 months, 2 weeks ago

On Thu, Jul 17, 2025 at 07:13:49PM -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.

Is there any issue with a Shift key?

> Signed-off-by: Rodrigo Gobbi <rodrigo.gobbi.7@gmail.com>

...

>  #include <linux/clk.h>
>  #include <linux/err.h>
>  #include <linux/completion.h>
> +#include <linux/bitfield.h>

While it looks unordered, it's still better to squeeze a new header to the
place which organises a longest (but sparse) group of ordered headers. This
will reduce churn in the future for the ordering changes.

>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>

-- 
With Best Regards,
Andy Shevchenko

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

Posted by Jonathan Cameron 2 months, 2 weeks ago

On Wed, 23 Jul 2025 16:49:24 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Thu, Jul 17, 2025 at 07:13:49PM -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.  
> 
> Is there any issue with a Shift key?
Sprinkled some capitals.
> 
> > Signed-off-by: Rodrigo Gobbi <rodrigo.gobbi.7@gmail.com>  
> 
> ...
> 
> >  #include <linux/clk.h>
> >  #include <linux/err.h>
> >  #include <linux/completion.h>
> > +#include <linux/bitfield.h>  
> 
> While it looks unordered, it's still better to squeeze a new header to the
> place which organises a longest (but sparse) group of ordered headers. This
> will reduce churn in the future for the ordering changes.

The full set is currently:

#include <linux/mod_devicetable.h>
#include <linux/module.h>
#include <linux/platform_device.h>
#include <linux/property.h>
#include <linux/interrupt.h>
#include <linux/device.h>
#include <linux/kernel.h>
#include <linux/slab.h>
#include <linux/io.h>
#include <linux/clk.h>
#include <linux/err.h>
#include <linux/completion.h>
#include <linux/bitfield.h>

I'll move it to before clk, but that's only a really minor improvement!


> 
> >  #include <linux/iio/iio.h>
> >  #include <linux/iio/sysfs.h>  
>

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

Posted by Rodrigo Gobbi 2 months, 1 week ago

On 7/19/25 09:10, Jonathan Cameron wrote:
> On Thu, 17 Jul 2025 19:13:49 -0300
> Rodrigo Gobbi <rodrigo.gobbi.7@gmail.com> 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.
>> 
> I read this as meaning this is a hardening change against bad DT rather
> than a bug fix?
> 
> As such applied for next cycle to the testing branch of iio.git.
> 
> Thanks,
> 
> Jonathan

Sorry for the late response, yes, that’s correct — this is a hardening change.

On 7/24/25 09:47, Jonathan Cameron wrote:
> On Wed, 23 Jul 2025 16:49:24 +0300
> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> 
>> On Thu, Jul 17, 2025 at 07:13:49PM -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.  
>> 
>> Is there any issue with a Shift key?
> Sprinkled some capitals.
>> 

I'll make sure to pay more attention to that in future patches.
Tks for adjusting it, @Jonathan. 
Best regards to all!

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

Posted by Jonathan Cameron 2 months, 2 weeks ago

On Thu, 17 Jul 2025 19:13:49 -0300
Rodrigo Gobbi <rodrigo.gobbi.7@gmail.com> 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.
> 
I read this as meaning this is a hardening change against bad DT rather
than a bug fix?

As such applied for next cycle to the testing branch of iio.git.

Thanks,

Jonathan

> Signed-off-by: Rodrigo Gobbi <rodrigo.gobbi.7@gmail.com>
> ---
> About @David comment at v2:
> 
> > For bonus points, a separate patch that cleans up and sorts the existing
> > includes would be appreciated.  
> 
> I`ll change that in a later moment.
> Tks and regards.
> 
> Changelog:
> v3: moving up include; fix indentation;
> v2: https://lore.kernel.org/linux-iio/20250701213728.32064-1-rodrigo.gobbi.7@gmail.com/
> v1: https://lore.kernel.org/linux-iio/20250621185301.9536-1-rodrigo.gobbi.7@gmail.com/#t
> ---
>  drivers/iio/adc/spear_adc.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/spear_adc.c b/drivers/iio/adc/spear_adc.c
> index e3a865c79686..0acc65c74221 100644
> --- a/drivers/iio/adc/spear_adc.c
> +++ b/drivers/iio/adc/spear_adc.c
> @@ -17,6 +17,7 @@
>  #include <linux/clk.h>
>  #include <linux/err.h>
>  #include <linux/completion.h>
> +#include <linux/bitfield.h>
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> @@ -29,9 +30,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 +158,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)

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

Posted by David Lechner 2 months, 3 weeks ago

On 7/17/25 5:13 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>
> ---
Reviewed-by: David Lechner <dlechner@baylibre.com>