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

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

Hi Andy,

Thanks for the review, replies inline.

On 15/06/2026 15:28, Andy Shevchenko wrote:
> On Mon, Jun 15, 2026 at 12:37:22AM +0100, Salih Erim 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.
> 
> ...
> 
>> +static int sysmon_osr_write_temp(struct sysmon *sysmon, 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;
> 
> If, for some reason, val happens to be a small negative number, here might be
> a surprising behaviour.

The caller validates val against the oversampling_avail list
{1, 2, 4, 8, 16} before calling, so negatives never reach here.
But the parameter should be unsigned int to make that obvious.
Will change in v8.

> 
>> +     unsigned int readback;
>> +     int ret;
>> +
>> +     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);
> 
> Is the last parameter > 32-bit? If not, drop 'U' as it might have a nice
> side-effect in case this become actually > 32-bit. Same for other cases.
> In other words, using ~0U should be quite cautious.

Will change to ~0. Same for both call sites.

Thanks,
Salih

> 
>> +}
> 
> --
> With Best Regards,
> Andy Shevchenko
> 
>