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

[Erim, Salih]

Hi Andy,

On 07/06/2026 08:50, Andy Shevchenko wrote:
> On Sat, Jun 06, 2026 at 06:17:07AM +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
>
> ...
>
> > +static int sysmon_set_avg_enable(struct sysmon *sysmon,
> > +                              u32 base, u32 count, u32 val)
> > +{
> > +     int ret;
> > +
> > +     for (unsigned int i = 0; i < count; i++) {
> > +             ret = regmap_write(sysmon->regmap,
> > +                                base + (i * SYSMON_REG_STRIDE), val);
>
> Unneeded parentheses.

Accepted.
> > +             if (ret)
> > +                     return ret;
> > +     }
>
> Also you can use temporary for regmap
>
>          struct regmap *map = sysmon->regmap;
>          int ret;
>
>          for (unsigned int i = 0; i < count; i++) {
>                  ret = regmap_write(map, base + i * SYSMON_REG_STRIDE, val);
>                  if (ret)
>                          return ret;
>          }
>
> > +     return 0;
> > +}
>
> And use this trick in other places where appropriate, it makes code easier
> to read.

Accepted. Will use a local regmap pointer in functions with
repeated sysmon->regmap access.

> ...
>
> > +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_CONFIG_TEMP_SAT_OSR,
> > +                                     FIELD_PREP(SYSMON_CONFIG_TEMP_SAT_OSR,
> > +                                                hw_val));
> > +             if (ret)
> > +                     return ret;
> > +
> > +             return sysmon_set_avg_enable(sysmon, SYSMON_TEMP_EN_AVG_BASE,
> > +                                          SYSMON_TEMP_EN_AVG_COUNT,
> > +                                          hw_val ? ~0U : 0);
>
> > +     }
> > +
> > +     if (channel_type == IIO_VOLTAGE) {
>
> Can channel_type be both TEMP and VOLTAGE here? No. Why do we check it twice?

Good point. Will restructure to use switch or if/else to
make it clear only one path executes.

Thanks,
Salih
> > +             ret = regmap_update_bits(sysmon->regmap, SYSMON_CONFIG,
> > +                                     SYSMON_CONFIG_SUPPLY_OSR,
> > +                                     FIELD_PREP(SYSMON_CONFIG_SUPPLY_OSR,
> > +                                                hw_val));
> > +             if (ret)
> > +                     return ret;
> > +
> > +             return sysmon_set_avg_enable(sysmon, SYSMON_SUPPLY_EN_AVG_BASE,
> > +                                          SYSMON_SUPPLY_EN_AVG_COUNT,
> > +                                          hw_val ? ~0U : 0);
> > +     }
> > +     return -EINVAL;
> > +}
>
> --
> With Best Regards,
> Andy Shevchenko