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

[Erim, Salih]

Hi Jonathan,

On 28/05/2026 14:05, Jonathan Cameron wrote:
> On Wed, 27 May 2026 12:42:11 +0100
> Salih Erim <salih.erim@xxxxxxx> 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@xxxxxxx>
> 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)