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

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

Hi Jonathan,

On 12/06/2026 13:58, Jonathan Cameron wrote:
> On Thu, 11 Jun 2026 23:27:38 +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>
> 
> One minor comment inline.
> 
>>   drivers/iio/adc/versal-sysmon-core.c | 147 ++++++++++++++++++++++++++-
>>   drivers/iio/adc/versal-sysmon.h      |  17 ++++
>>   2 files changed, 163 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
>> index 20fd3a87d44..fa8f0dc868a 100644
>> --- a/drivers/iio/adc/versal-sysmon-core.c
>> +++ b/drivers/iio/adc/versal-sysmon-core.c
> 
>> +static int sysmon_osr_write(struct sysmon *sysmon, int channel_type, int val)
> 
> There is almost nothing shared in here between the two channel_types.
> Might make sense to just split it into two helpers, particularly as
> there is a channel type if / else at the caller.

Agreed. The only shared code outside the switch is the hw_val
computation. Will split into two helpers in v7.

Thanks,
Salih

> 
>> +{
>> +     /*
>> +      * 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;
>> +     unsigned int readback;
>> +     int ret;
>> +
>> +     switch (channel_type) {
>> +     case IIO_TEMP:
>> +             ret = regmap_update_bits(sysmon->regmap, SYSMON_CONFIG,
>> +                                     SYSMON_CONFIG_TEMP_SAT_OSR,
>> +                                     FIELD_PREP(SYSMON_CONFIG_TEMP_SAT_OSR, hw_val));
>> +             if (ret)
>> +                     return ret;
>> +
>> +             /*
>> +              * Readback fence: the SysMon CONFIG register resides in the
>> +              * PMC domain behind the NoC. A posted write may not reach the
>> +              * hardware before the next MMIO access. Reading the register
>> +              * back forces the interconnect to complete the write, preventing
>> +              * a bus hang on the subsequent access.
>> +              */
>> +             regmap_read(sysmon->regmap, SYSMON_CONFIG, &readback);
>> +
>> +             return sysmon_set_avg_enable(sysmon, SYSMON_TEMP_EN_AVG_BASE,
>> +                                          SYSMON_TEMP_EN_AVG_COUNT,
>> +                                          hw_val ? ~0U : 0);
>> +     case IIO_VOLTAGE:
>> +             ret = regmap_update_bits(sysmon->regmap, SYSMON_CONFIG,
>> +                                     SYSMON_CONFIG_SUPPLY_OSR,
>> +                                     FIELD_PREP(SYSMON_CONFIG_SUPPLY_OSR, hw_val));
>> +             if (ret)
>> +                     return ret;
>> +
>> +             /* Readback fence -- see above */
>> +             regmap_read(sysmon->regmap, SYSMON_CONFIG, &readback);
>> +
>> +             return sysmon_set_avg_enable(sysmon, SYSMON_SUPPLY_EN_AVG_BASE,
>> +                                          SYSMON_SUPPLY_EN_AVG_COUNT,
>> +                                          hw_val ? ~0U : 0);
>> +     default:
>> +             return -EINVAL;
>> +     }
>> +}
>> +
>> +static int sysmon_write_raw(struct iio_dev *indio_dev,
>> +                         struct iio_chan_spec const *chan,
>> +                         int val, int val2, long mask)
>> +{
>> +     struct sysmon *sysmon = iio_priv(indio_dev);
>> +     unsigned int i;
>> +     int ret;
>> +
>> +     if (mask != IIO_CHAN_INFO_OVERSAMPLING_RATIO)
>> +             return -EINVAL;
>> +
>> +     for (i = 0; i < ARRAY_SIZE(sysmon_oversampling_avail); i++) {
>> +             if (val == sysmon_oversampling_avail[i])
>> +                     break;
>> +     }
>> +     if (i == ARRAY_SIZE(sysmon_oversampling_avail))
>> +             return -EINVAL;
>> +
>> +     guard(mutex)(&sysmon->lock);
>> +
>> +     ret = sysmon_osr_write(sysmon, chan->type, val);
>> +     if (ret)
>> +             return ret;
>> +
>> +     if (chan->type == IIO_TEMP)
>> +             sysmon->temp_oversampling = val;
>> +     else
>> +             sysmon->supply_oversampling = val;
>> +
>> +     return 0;
>> +}