Re: [PATCH v3 5/5] iio: adc: versal-sysmon: add oversampling support

["Erim, Salih" <salih.erim@amd.com>]

Hi Jonathan,

On 28/05/2026 14:05, Jonathan Cameron wrote:
> 
> 
> On Wed, 27 May 2026 12:42:11 +0100
> Salih Erim <salih.erim@amd.com> wrote:
> 
>> Add support for reading and writing the oversampling ratio through
>> the IIO oversampling_ratio attribute. The hardware supports averaging
>> 2, 4, 8, or 16 samples, plus a ratio of 1 (no averaging).
>>
>> Temperature and supply channels share oversampling configuration at
>> the type level (all temperature channels share one ratio, all supply
>> channels share another), exposed through info_mask_shared_by_type.
>>
>> The hardware encoding uses sample_count / 2 in a 4-bit field within
>> the CONFIG register. Per-channel averaging enable registers must also
>> be updated to activate or deactivate averaging.
>>
>> Signed-off-by: Salih Erim <salih.erim@amd.com>
> Really minor stuff from a reread

Thanks for all reviews, it has been really teachful every time.

> 
>> +static int sysmon_osr_write(struct sysmon *sysmon, int channel_type, int val)
>> +{
>> +     /*
>> +      * HW register encoding is sample_count / 2:
>> +      * 0=none, 1=2x, 2=4x, 4=8x, 8=16x (not log2-based).
>> +      */
>> +     int hw_val = val >> 1;
>> +     int ret;
>> +
>> +     if (channel_type == IIO_TEMP) {
>> +             ret = regmap_update_bits(sysmon->regmap, SYSMON_CONFIG,
>> +                                     SYSMON_TEMP_SAT_CONFIG_MASK,
>> +                                     FIELD_PREP(SYSMON_TEMP_SAT_CONFIG_MASK,
>> +                                                hw_val));
>> +             if (ret)
>> +                     return ret;
> blank line

Accepted.

>> +             ret = sysmon_set_avg_enable(sysmon, SYSMON_TEMP_EN_AVG_BASE,
>> +                                         SYSMON_TEMP_EN_AVG_COUNT,
>> +                                         hw_val ? ~0U : 0);
>> +             if (ret)
>> +                     return ret;
>                  return sysmon_set...

Accepted. Will return directly.

> 
>> +     } else if (channel_type == IIO_VOLTAGE) {
> Won't need the else if returned already.

Accepted.

>> +             ret = regmap_update_bits(sysmon->regmap, SYSMON_CONFIG,
>> +                                     SYSMON_SUPPLY_CONFIG_MASK,
>> +                                     FIELD_PREP(SYSMON_SUPPLY_CONFIG_MASK,
>> +                                                hw_val));
>> +             if (ret)
>> +                     return ret;
> blank line

Accepted.

>> +             ret = sysmon_set_avg_enable(sysmon, SYSMON_SUPPLY_EN_AVG_BASE,
>> +                                         SYSMON_SUPPLY_EN_AVG_COUNT,
>> +                                         hw_val ? ~0U : 0);
>> +             if (ret)
>> +                     return ret;
>                  return sysmon_set...

Accepted.

> 
>> +     } else {
> No else needed here either

Accepted.

> 
>> +             return -EINVAL;
>> +     }
>> +
>> +     return 0
> And this isn't needed at all.

Accepted.

> 
>> +}
> 
>> diff --git a/drivers/iio/adc/versal-sysmon.h b/drivers/iio/adc/versal-sysmon.h
>> index a78362f95e6..cf69be62709 100644
>> --- a/drivers/iio/adc/versal-sysmon.h
>> +++ b/drivers/iio/adc/versal-sysmon.h
>> @@ -25,11 +25,13 @@ struct regmap;
>>   #define SYSMON_IMR                   0x0048
>>   #define SYSMON_IER                   0x004C
>>   #define SYSMON_IDR                   0x0050
>> +#define SYSMON_CONFIG                        0x0100
>>   #define SYSMON_TEMP_MAX                      0x1030
>>   #define SYSMON_TEMP_MIN                      0x1034
>>   #define SYSMON_SUPPLY_BASE           0x1040
>>   #define SYSMON_ALARM_FLAG            0x1018
>>   #define SYSMON_ALARM_REG             0x1940
>> +#define SYSMON_SUPPLY_EN_AVG_BASE    0x1958
>>   #define SYSMON_TEMP_TH_LOW           0x1970
>>   #define SYSMON_TEMP_TH_UP            0x1974
>>   #define SYSMON_OT_TH_LOW             0x1978
>> @@ -41,6 +43,7 @@ struct regmap;
>>   #define SYSMON_TEMP_MAX_MAX          0x1F90
>>   #define SYSMON_STATUS_RESET          0x1F94
>>   #define SYSMON_TEMP_SAT_BASE         0x1FAC
>> +#define SYSMON_TEMP_EN_AVG_BASE              0x24B4
>>   #define SYSMON_MAX_REG                       0x24C0
>>
>>   /* NPI unlock value written to SYSMON_NPI_LOCK */
>> @@ -57,6 +60,16 @@ struct regmap;
>>   /* ISR/IMR temperature and OT alarm mask (bits 9:8) */
>>   #define SYSMON_TEMP_INTR_MASK                GENMASK(9, 8)
>>
>> +/* Config register: supply oversampling field (bits 17:14) */
>> +#define SYSMON_SUPPLY_CONFIG_MASK    GENMASK(17, 14)
>> +
>> +/* Config register: temp satellite oversampling field (bits 27:24) */
> 
> I missed this before, but given the GENMASK just below the bits part
> of these comments is pointles. Drop it.
> 
> Ideally also name them in a way that makes it clear what register
> they are fields of.

Accepted. Will drop the "bits X:Y" comments and rename to
make the register association clear.

All items will be addressed in v4.

Salih

> 
>> +#define SYSMON_TEMP_SAT_CONFIG_MASK  GENMASK(27, 24)
> 
>