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

[<Claudiu.Beznea@microchip.com>]

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,
>