diff mbox series

[06/16] iio: adc: at91-sama5d2_adc: add 64 and 256 oversampling ratio

Message ID 20220609083213.1795019-7-claudiu.beznea@microchip.com (mailing list archive)
State Changes Requested
Headers show
Series iio: adc: at91-sama5d2_adc: add support for temperature sensor | expand

Commit Message

Claudiu Beznea June 9, 2022, 8:32 a.m. UTC 
Add 64 and 256 oversampling ratio support. It is necessary for temperature
sensor.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/iio/adc/at91-sama5d2_adc.c | 31 +++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

Comments

Jonathan Cameron June 11, 2022, 5:47 p.m. UTC | #1 
On Thu, 9 Jun 2022 11:32:03 +0300
Claudiu Beznea <claudiu.beznea@microchip.com> wrote:

> Add 64 and 256 oversampling ratio support. It is necessary for temperature
> sensor.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
>  drivers/iio/adc/at91-sama5d2_adc.c | 31 +++++++++++++++++++++++++++---
>  1 file changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index 7321a4b519af..b52f1020feaf 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -142,6 +142,8 @@ struct at91_adc_reg_layout {
>  #define AT91_SAMA5D2_EMR_OSR_1SAMPLES		0
>  #define AT91_SAMA5D2_EMR_OSR_4SAMPLES		1
>  #define AT91_SAMA5D2_EMR_OSR_16SAMPLES		2
> +#define AT91_SAMA5D2_EMR_OSR_64SAMPLES		3
> +#define AT91_SAMA5D2_EMR_OSR_256SAMPLES		4
>  
>  /* Extended Mode Register - Averaging on single trigger event */
>  #define AT91_SAMA5D2_EMR_ASTE(V)		((V) << 20)
> @@ -308,6 +310,8 @@ static const struct at91_adc_reg_layout sama7g5_layout = {
>  #define AT91_OSR_1SAMPLES		1
>  #define AT91_OSR_4SAMPLES		4
>  #define AT91_OSR_16SAMPLES		16
> +#define AT91_OSR_64SAMPLES		64
> +#define AT91_OSR_256SAMPLES		256

These defines seems a bit silly.  Better to use the values inline than
to have these.

>  
>  #define AT91_SAMA5D2_CHAN_SINGLE(index, num, addr)			\
>  	{								\
> @@ -640,7 +644,9 @@ static const struct at91_adc_platform sama7g5_platform = {
>  	.osr_mask = GENMASK(18, 16),
>  	.osr_vals = BIT(AT91_SAMA5D2_EMR_OSR_1SAMPLES) |
>  		    BIT(AT91_SAMA5D2_EMR_OSR_4SAMPLES) |
> -		    BIT(AT91_SAMA5D2_EMR_OSR_16SAMPLES),
> +		    BIT(AT91_SAMA5D2_EMR_OSR_16SAMPLES) |
> +		    BIT(AT91_SAMA5D2_EMR_OSR_64SAMPLES) |
> +		    BIT(AT91_SAMA5D2_EMR_OSR_256SAMPLES),
>  	.chan_realbits = 16,
>  };
>  
> @@ -774,6 +780,18 @@ static int at91_adc_config_emr(struct at91_adc_state *st,
>  		emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_16SAMPLES,
>  					    osr_mask);
>  		break;
> +	case AT91_OSR_64SAMPLES:
> +		if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_64SAMPLES)))
> +			return -EINVAL;
> +		emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_64SAMPLES,
> +					    osr_mask);
> +		break;
> +	case AT91_OSR_256SAMPLES:
> +		if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_256SAMPLES)))
> +			return -EINVAL;
> +		emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_256SAMPLES,
> +					    osr_mask);
> +		break;
>  	}
>  
>  	at91_adc_writel(st, EMR, emr);
> @@ -791,6 +809,10 @@ static int at91_adc_adjust_val_osr(struct at91_adc_state *st, int *val)
>  		nbits = 13;
>  	else if (st->oversampling_ratio == AT91_OSR_16SAMPLES)
>  		nbits = 14;
> +	else if (st->oversampling_ratio == AT91_OSR_64SAMPLES)
> +		nbits = 15;
> +	else if (st->oversampling_ratio == AT91_OSR_256SAMPLES)
> +		nbits = 16;
>  
>  	/*
>  	 * We have nbits of real data and channel is registered as
> @@ -1679,7 +1701,8 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
>  	switch (mask) {
>  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>  		if ((val != AT91_OSR_1SAMPLES) && (val != AT91_OSR_4SAMPLES) &&
> -		    (val != AT91_OSR_16SAMPLES))
> +		    (val != AT91_OSR_16SAMPLES) && (val != AT91_OSR_64SAMPLES) &&
> +		    (val != AT91_OSR_256SAMPLES))
Dropping this partial validity check and moving into a default in the switch statement
in config_emr() would be nice cleanup (I also replied to earlier patch based on what
is visible here).

>  			return -EINVAL;
>  		/* if no change, optimize out */
>  		mutex_lock(&st->lock);
> @@ -1897,7 +1920,9 @@ static IIO_CONST_ATTR(hwfifo_watermark_max, AT91_HWFIFO_MAX_SIZE_STR);
>  static IIO_CONST_ATTR(oversampling_ratio_available,
>  		      __stringify(AT91_OSR_1SAMPLES) " "
>  		      __stringify(AT91_OSR_4SAMPLES) " "
> -		      __stringify(AT91_OSR_16SAMPLES));
> +		      __stringify(AT91_OSR_16SAMPLES) " "
> +		      __stringify(AT91_OSR_64SAMPLES) " "
> +		      __stringify(AT91_OSR_256SAMPLES));

At somepoint it would be good to move this over to the read_avail() callback rather than
hand rolling it.  We are slowly working through doing this for all the IIO drivers
but it will take a long time yet!

>  
>  static struct attribute *at91_adc_attributes[] = {
>  	&iio_const_attr_oversampling_ratio_available.dev_attr.attr,
Claudiu Beznea June 14, 2022, 8:22 a.m. UTC | #2 
On 11.06.2022 20:47, Jonathan Cameron wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Thu, 9 Jun 2022 11:32:03 +0300
> Claudiu Beznea <claudiu.beznea@microchip.com> wrote:
> 
>> Add 64 and 256 oversampling ratio support. It is necessary for temperature
>> sensor.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>> ---
>>  drivers/iio/adc/at91-sama5d2_adc.c | 31 +++++++++++++++++++++++++++---
>>  1 file changed, 28 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
>> index 7321a4b519af..b52f1020feaf 100644
>> --- a/drivers/iio/adc/at91-sama5d2_adc.c
>> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
>> @@ -142,6 +142,8 @@ struct at91_adc_reg_layout {
>>  #define AT91_SAMA5D2_EMR_OSR_1SAMPLES                0
>>  #define AT91_SAMA5D2_EMR_OSR_4SAMPLES                1
>>  #define AT91_SAMA5D2_EMR_OSR_16SAMPLES               2
>> +#define AT91_SAMA5D2_EMR_OSR_64SAMPLES               3
>> +#define AT91_SAMA5D2_EMR_OSR_256SAMPLES              4
>>
>>  /* Extended Mode Register - Averaging on single trigger event */
>>  #define AT91_SAMA5D2_EMR_ASTE(V)             ((V) << 20)
>> @@ -308,6 +310,8 @@ static const struct at91_adc_reg_layout sama7g5_layout = {
>>  #define AT91_OSR_1SAMPLES            1
>>  #define AT91_OSR_4SAMPLES            4
>>  #define AT91_OSR_16SAMPLES           16
>> +#define AT91_OSR_64SAMPLES           64
>> +#define AT91_OSR_256SAMPLES          256
> 
> These defines seems a bit silly.  Better to use the values inline than
> to have these.
> 
>>
>>  #define AT91_SAMA5D2_CHAN_SINGLE(index, num, addr)                   \
>>       {                                                               \
>> @@ -640,7 +644,9 @@ static const struct at91_adc_platform sama7g5_platform = {
>>       .osr_mask = GENMASK(18, 16),
>>       .osr_vals = BIT(AT91_SAMA5D2_EMR_OSR_1SAMPLES) |
>>                   BIT(AT91_SAMA5D2_EMR_OSR_4SAMPLES) |
>> -                 BIT(AT91_SAMA5D2_EMR_OSR_16SAMPLES),
>> +                 BIT(AT91_SAMA5D2_EMR_OSR_16SAMPLES) |
>> +                 BIT(AT91_SAMA5D2_EMR_OSR_64SAMPLES) |
>> +                 BIT(AT91_SAMA5D2_EMR_OSR_256SAMPLES),
>>       .chan_realbits = 16,
>>  };
>>
>> @@ -774,6 +780,18 @@ static int at91_adc_config_emr(struct at91_adc_state *st,
>>               emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_16SAMPLES,
>>                                           osr_mask);
>>               break;
>> +     case AT91_OSR_64SAMPLES:
>> +             if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_64SAMPLES)))
>> +                     return -EINVAL;
>> +             emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_64SAMPLES,
>> +                                         osr_mask);
>> +             break;
>> +     case AT91_OSR_256SAMPLES:
>> +             if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_256SAMPLES)))
>> +                     return -EINVAL;
>> +             emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_256SAMPLES,
>> +                                         osr_mask);
>> +             break;
>>       }
>>
>>       at91_adc_writel(st, EMR, emr);
>> @@ -791,6 +809,10 @@ static int at91_adc_adjust_val_osr(struct at91_adc_state *st, int *val)
>>               nbits = 13;
>>       else if (st->oversampling_ratio == AT91_OSR_16SAMPLES)
>>               nbits = 14;
>> +     else if (st->oversampling_ratio == AT91_OSR_64SAMPLES)
>> +             nbits = 15;
>> +     else if (st->oversampling_ratio == AT91_OSR_256SAMPLES)
>> +             nbits = 16;
>>
>>       /*
>>        * We have nbits of real data and channel is registered as
>> @@ -1679,7 +1701,8 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
>>       switch (mask) {
>>       case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>>               if ((val != AT91_OSR_1SAMPLES) && (val != AT91_OSR_4SAMPLES) &&
>> -                 (val != AT91_OSR_16SAMPLES))
>> +                 (val != AT91_OSR_16SAMPLES) && (val != AT91_OSR_64SAMPLES) &&
>> +                 (val != AT91_OSR_256SAMPLES))
> Dropping this partial validity check and moving into a default in the switch statement
> in config_emr() would be nice cleanup (I also replied to earlier patch based on what
> is visible here).

Sure, I'll check it.

> 
>>                       return -EINVAL;
>>               /* if no change, optimize out */
>>               mutex_lock(&st->lock);
>> @@ -1897,7 +1920,9 @@ static IIO_CONST_ATTR(hwfifo_watermark_max, AT91_HWFIFO_MAX_SIZE_STR);
>>  static IIO_CONST_ATTR(oversampling_ratio_available,
>>                     __stringify(AT91_OSR_1SAMPLES) " "
>>                     __stringify(AT91_OSR_4SAMPLES) " "
>> -                   __stringify(AT91_OSR_16SAMPLES));
>> +                   __stringify(AT91_OSR_16SAMPLES) " "
>> +                   __stringify(AT91_OSR_64SAMPLES) " "
>> +                   __stringify(AT91_OSR_256SAMPLES));
> 
> At somepoint it would be good to move this over to the read_avail() callback rather than
> hand rolling it.  We are slowly working through doing this for all the IIO drivers
> but it will take a long time yet!

I'll check this, too.

> 
>>
>>  static struct attribute *at91_adc_attributes[] = {
>>       &iio_const_attr_oversampling_ratio_available.dev_attr.attr,
>
diff mbox series

Patch

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 7321a4b519af..b52f1020feaf 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -142,6 +142,8 @@  struct at91_adc_reg_layout {
 #define AT91_SAMA5D2_EMR_OSR_1SAMPLES		0
 #define AT91_SAMA5D2_EMR_OSR_4SAMPLES		1
 #define AT91_SAMA5D2_EMR_OSR_16SAMPLES		2
+#define AT91_SAMA5D2_EMR_OSR_64SAMPLES		3
+#define AT91_SAMA5D2_EMR_OSR_256SAMPLES		4
 
 /* Extended Mode Register - Averaging on single trigger event */
 #define AT91_SAMA5D2_EMR_ASTE(V)		((V) << 20)
@@ -308,6 +310,8 @@  static const struct at91_adc_reg_layout sama7g5_layout = {
 #define AT91_OSR_1SAMPLES		1
 #define AT91_OSR_4SAMPLES		4
 #define AT91_OSR_16SAMPLES		16
+#define AT91_OSR_64SAMPLES		64
+#define AT91_OSR_256SAMPLES		256
 
 #define AT91_SAMA5D2_CHAN_SINGLE(index, num, addr)			\
 	{								\
@@ -640,7 +644,9 @@  static const struct at91_adc_platform sama7g5_platform = {
 	.osr_mask = GENMASK(18, 16),
 	.osr_vals = BIT(AT91_SAMA5D2_EMR_OSR_1SAMPLES) |
 		    BIT(AT91_SAMA5D2_EMR_OSR_4SAMPLES) |
-		    BIT(AT91_SAMA5D2_EMR_OSR_16SAMPLES),
+		    BIT(AT91_SAMA5D2_EMR_OSR_16SAMPLES) |
+		    BIT(AT91_SAMA5D2_EMR_OSR_64SAMPLES) |
+		    BIT(AT91_SAMA5D2_EMR_OSR_256SAMPLES),
 	.chan_realbits = 16,
 };
 
@@ -774,6 +780,18 @@  static int at91_adc_config_emr(struct at91_adc_state *st,
 		emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_16SAMPLES,
 					    osr_mask);
 		break;
+	case AT91_OSR_64SAMPLES:
+		if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_64SAMPLES)))
+			return -EINVAL;
+		emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_64SAMPLES,
+					    osr_mask);
+		break;
+	case AT91_OSR_256SAMPLES:
+		if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_256SAMPLES)))
+			return -EINVAL;
+		emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_256SAMPLES,
+					    osr_mask);
+		break;
 	}
 
 	at91_adc_writel(st, EMR, emr);
@@ -791,6 +809,10 @@  static int at91_adc_adjust_val_osr(struct at91_adc_state *st, int *val)
 		nbits = 13;
 	else if (st->oversampling_ratio == AT91_OSR_16SAMPLES)
 		nbits = 14;
+	else if (st->oversampling_ratio == AT91_OSR_64SAMPLES)
+		nbits = 15;
+	else if (st->oversampling_ratio == AT91_OSR_256SAMPLES)
+		nbits = 16;
 
 	/*
 	 * We have nbits of real data and channel is registered as
@@ -1679,7 +1701,8 @@  static int at91_adc_write_raw(struct iio_dev *indio_dev,
 	switch (mask) {
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
 		if ((val != AT91_OSR_1SAMPLES) && (val != AT91_OSR_4SAMPLES) &&
-		    (val != AT91_OSR_16SAMPLES))
+		    (val != AT91_OSR_16SAMPLES) && (val != AT91_OSR_64SAMPLES) &&
+		    (val != AT91_OSR_256SAMPLES))
 			return -EINVAL;
 		/* if no change, optimize out */
 		mutex_lock(&st->lock);
@@ -1897,7 +1920,9 @@  static IIO_CONST_ATTR(hwfifo_watermark_max, AT91_HWFIFO_MAX_SIZE_STR);
 static IIO_CONST_ATTR(oversampling_ratio_available,
 		      __stringify(AT91_OSR_1SAMPLES) " "
 		      __stringify(AT91_OSR_4SAMPLES) " "
-		      __stringify(AT91_OSR_16SAMPLES));
+		      __stringify(AT91_OSR_16SAMPLES) " "
+		      __stringify(AT91_OSR_64SAMPLES) " "
+		      __stringify(AT91_OSR_256SAMPLES));
 
 static struct attribute *at91_adc_attributes[] = {
 	&iio_const_attr_oversampling_ratio_available.dev_attr.attr,