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

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

Hi Andy,

Thanks for all reviews.
On 09/06/2026 16:06, Andy Shevchenko wrote:
> On Mon, Jun 08, 2026 at 07:37:58PM +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 driver is split into three 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.
> 
> Almost there.
> 
> ...
> 
>> +static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev)
>> +{
>> +     unsigned int num_supply = 0, num_temp = 0;
> 
> Unneeded assignments.

Accepted.
> 
>> +     unsigned int idx, temp_chan_idx, volt_chan_idx;
>> +     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);
>> +
>> +     sysmon_channels = devm_kcalloc(dev,
>> +                                    size_add(size_add(ARRAY_SIZE(temp_channels),
>> +                                               num_supply), num_temp),
>> +                                    sizeof(*sysmon_channels), GFP_KERNEL);
> 
> Something happened to indentation of the third line (out of four). Taking into
> account nested size_add(), I would rewrite the whole thing as

Accepted. Will use a temporary variable for the channel count.

> 
>          sysmon_channels = devm_kcalloc(dev,
>                                         size_add(num_temp,
>                                                  size_add(ARRAY_SIZE(temp_channels), num_supply)),
>                                         sizeof(*sysmon_channels), GFP_KERNEL);
> 
> Or even use temporary variable
> 
>          unsigned int num_chan;
> 
>          num_chan = size_add(num_temp, size_add(ARRAY_SIZE(temp_channels), num_supply)),
>          sysmon_channels = devm_kcalloc(dev, num_chan, sizeof(*sysmon_channels), GFP_KERNEL);
> 
> still over 80, but a bit shorter.
> 
>> +     if (!sysmon_channels)
>> +             return -ENOMEM;
>> +
>> +     /* Static temperature channels first (fixed indices) */
> 
>> +     idx = 0;
> 
> Why?
> 
>> +     memcpy(sysmon_channels, temp_channels, sizeof(temp_channels));
>> +     idx += ARRAY_SIZE(temp_channels);
> 
> Just

Accepted. Will initialize idx directly.

> 
>          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),
> 
> Perfectly one line. Is it going to be expanded in the next changes?
> If not, join.

Not expanded. Will join on one line.

> 
>> +                     .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),
> 
> Ditto.

Not expanded. Will join on one line.

Regards,
Salih
> 
>> +                     .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;
>> +}
> 
> --
> With Best Regards,
> Andy Shevchenko
> 
>