Re: [PATCH v7 2/5] iio: adc: add Versal SysMon driver

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

Hi Andy,

Thanks for the review, replies inline.

On 15/06/2026 15:22, Andy Shevchenko wrote:
> On Mon, Jun 15, 2026 at 12:37:19AM +0100, Salih Erim wrote:
>> Add the core driver and MMIO platform driver for the AMD/Xilinx Versal
>> System Monitor (SysMon) block.
>>
>> The SysMon block resides in the platform management controller (PMC) and
>> provides on-chip voltage and temperature monitoring through a 10-bit,
>> 200 kSPS ADC. It can monitor up to 160 voltage channels and 64
>> temperature satellites distributed across the SoC, with a consistent
>> sample rate of 8 kSPS per channel regardless of how many channels are
>> enabled.
>>
>> The hardware also provides four aggregate temperature registers that
>> are always present regardless of the device tree configuration: the
>> current max and min across all active satellites, and the peak and
>> trough values recorded since the last hardware reset.
>>
>> The driver is split into two compilation units:
>>    - versal-sysmon-core: Channel parsing, IIO registration, read_raw
>>    - versal-sysmon: MMIO platform driver with custom regmap accessors
>>
>> Voltage results are stored in a 19-bit modified floating-point format
>> and converted to millivolts. Temperature results are stored in Q8.7
>> signed fixed-point Celsius format and converted to millicelsius.
>>
>> The MMIO regmap backend uses a custom reg_write accessor that
>> automatically unlocks the NPI (NoC programming interface) lock
>> register before each write, as required by the hardware. The regmap
>> is configured with fast_io since the underlying MMIO accessors are
>> safe to call from atomic context.
> 
> ...
> 
>> +static int sysmon_read_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 regval;
>> +     int ret;
>> +
>> +     guard(mutex)(&sysmon->lock);
>> +
>> +     switch (chan->type) {
>> +     case IIO_TEMP:
>> +             if (mask == IIO_CHAN_INFO_SCALE) {
>> +                     /* Q8.7 to millicelsius: raw * 1000 / 128 */
>> +                     *val = MILLI;
> 
> Since this is about temperature, wouldn't be better to use
> 
>          MILLIDEGREE_PER_DEGREE
> 
> here?

Agreed, MILLIDEGREE_PER_DEGREE is semantically correct here.
Will change in v8.

> 
>> +                     *val2 = BIT(SYSMON_FRACTIONAL_SHIFT);
>> +                     return IIO_VAL_FRACTIONAL;
>> +             }
>> +             if (mask != IIO_CHAN_INFO_RAW)
>> +                     return -EINVAL;
>> +
>> +             ret = regmap_read(sysmon->regmap, chan->address, &regval);
>> +             if (ret)
>> +                     return ret;
>> +
>> +             *val = sign_extend32(regval, 15);
>> +             return IIO_VAL_INT;
>> +
>> +     case IIO_VOLTAGE:
>> +             if (mask != IIO_CHAN_INFO_PROCESSED)
>> +                     return -EINVAL;
>> +
>> +             ret = regmap_read(sysmon->regmap,
>> +                               chan->address * SYSMON_REG_STRIDE +
>> +                               SYSMON_SUPPLY_BASE, &regval);
>> +             if (ret)
>> +                     return ret;
>> +
>> +             sysmon_supply_rawtoprocessed(regval, val);
>> +             return IIO_VAL_INT;
>> +
>> +     default:
>> +             return -EINVAL;
>> +     }
>> +}
> 
> ...
> 
>> +static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev)
>> +{
>> +     unsigned int num_chan, idx, temp_chan_idx, volt_chan_idx;
>> +     unsigned int num_supply, num_temp;
>> +     struct iio_chan_spec *sysmon_channels;
>> +     const char *label;
>> +     u32 reg;
>> +     int ret;
>> +
>> +     struct fwnode_handle *supply_node __free(fwnode_handle) =
>> +             device_get_named_child_node(dev, "voltage-channels");
>> +     num_supply = fwnode_get_child_node_count(supply_node);
>> +
>> +     struct fwnode_handle *temp_node __free(fwnode_handle) =
>> +             device_get_named_child_node(dev, "temperature-channels");
>> +     num_temp = fwnode_get_child_node_count(temp_node);
>> +
>> +     num_chan = size_add(num_temp, size_add(ARRAY_SIZE(temp_channels), num_supply));
> 
> + overflow.h

Accepted, Will add.
> 
>> +     sysmon_channels = devm_kcalloc(dev, num_chan, sizeof(*sysmon_channels), GFP_KERNEL);
>> +     if (!sysmon_channels)
>> +             return -ENOMEM;
>> +
>> +     /* Static temperature channels first */
>> +     memcpy(sysmon_channels, temp_channels, sizeof(temp_channels));
>> +     idx = ARRAY_SIZE(temp_channels);
>> +
>> +     /* Supply channels from DT */
>> +     fwnode_for_each_child_node_scoped(supply_node, child) {
>> +             ret = fwnode_property_read_u32(child, "reg", &reg);
>> +             if (ret)
>> +                     return dev_err_probe(dev, ret,
>> +                                          "missing reg for supply channel\n");
>> +
>> +             if (reg > SYSMON_SUPPLY_IDX_MAX)
>> +                     return dev_err_probe(dev, -EINVAL,
>> +                                          "supply reg %u exceeds max %u\n",
>> +                                          reg, SYSMON_SUPPLY_IDX_MAX);
>> +
>> +             ret = fwnode_property_read_string(child, "label", &label);
>> +             if (ret)
>> +                     return dev_err_probe(dev, ret,
>> +                                          "missing label for supply channel\n");
>> +
>> +             sysmon_channels[idx++] = (struct iio_chan_spec) {
>> +                     .type = IIO_VOLTAGE,
>> +                     .indexed = 1,
>> +                     .address = reg,
>> +                     .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>> +                     .datasheet_name = label,
>> +             };
>> +     }
>> +
>> +     /* Temperature satellite channels from DT */
>> +     fwnode_for_each_child_node_scoped(temp_node, child) {
>> +             ret = fwnode_property_read_u32(child, "reg", &reg);
>> +             if (ret)
>> +                     return dev_err_probe(dev, ret,
>> +                                          "missing reg for temp channel\n");
>> +
>> +             if (reg < 1 || reg > SYSMON_TEMP_SAT_MAX)
>> +                     return dev_err_probe(dev, -EINVAL,
>> +                                          "temp reg %u out of range [1..%u]\n",
>> +                                          reg, SYSMON_TEMP_SAT_MAX);
>> +
>> +             ret = fwnode_property_read_string(child, "label", &label);
>> +             if (ret)
>> +                     return dev_err_probe(dev, ret,
>> +                                          "missing label for temp channel\n");
>> +
>> +             sysmon_channels[idx++] = (struct iio_chan_spec) {
>> +                     .type = IIO_TEMP,
>> +                     .indexed = 1,
>> +                     .address = SYSMON_TEMP_SAT_BASE +
>> +                                (reg - 1) * SYSMON_REG_STRIDE,
>> +                     .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>> +                     .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
>> +                     .datasheet_name = label,
>> +             };
>> +     }
>> +
>> +     indio_dev->num_channels = idx;
>> +     indio_dev->info = &sysmon_iio_info;
>> +
>> +     /*
>> +      * Assign per-type sequential channel numbers.
>> +      * IIO sysfs uses type prefix (in_tempN, in_voltageN)
>> +      * so numbers only need to be unique within each type.
>> +      */
>> +     temp_chan_idx = 0;
>> +     volt_chan_idx = 0;
>> +     for (unsigned int idx = 0; idx < indio_dev->num_channels; idx++) {
>> +             if (sysmon_channels[idx].type == IIO_TEMP)
>> +                     sysmon_channels[idx].channel = temp_chan_idx++;
>> +             else
>> +                     sysmon_channels[idx].channel = volt_chan_idx++;
>> +     }
>> +
>> +     indio_dev->channels = sysmon_channels;
>> +
>> +     return 0;
>> +}
> 
> ...
> 
>> +/**
>> + * sysmon_core_probe() - Initialize Versal SysMon core
> 
> It is managed, please name it accordingly: devm_sysmon_core_probe().

Will rename to devm_sysmon_core_probe() and update callers.

Thanks,
Salih

> 
>> + * @dev: Parent device
>> + * @regmap: Register map for hardware access
>> + *
>> + * Return: 0 on success, negative errno on failure.
>> + */
> 
> --
> With Best Regards,
> Andy Shevchenko
> 
>