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

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

Hi Andy,

Thanks for the review, replies inline.

On 5/4/2026 11:18 AM, Andy Shevchenko wrote:
> On Sat, May 02, 2026 at 12:19:48PM +0100, Salih Erim wrote:
>> Add the AMD/Xilinx Versal System Monitor (SysMon) IIO driver.
>>
>> The driver is split into a bus-agnostic core module
>> (versal-sysmon-core) and a memory-mapped I/O platform driver
>> (versal-sysmon). The core uses the regmap API so that different
>> bus implementations can share the same IIO logic.
>>
>> The core provides:
>>    - Static temperature channels (current max/min, peak max/min)
>>    - Supply voltage channels parsed from DT container nodes
>>    - Temperature satellite channels parsed from DT container nodes
>>    - read_raw for IIO_CHAN_INFO_RAW and IIO_CHAN_INFO_PROCESSED
>>    - read_label using the DT label property
>>
>> The MMIO platform driver provides:
>>    - Memory-mapped register access via custom regmap callbacks
>>    - NPI unlock before every register write (platform management
>>      controller may re-lock NPI unpredictably on Versal devices)
>>
>> Threshold events, oversampling, and I2C bus support are added in
>> subsequent patches.
> 
> ...
> 
> IWYU please:
> 
> + array_size.h
> 
>> +#include <linux/bitfield.h>
>> +#include <linux/bits.h>
>> +#include <linux/cleanup.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/module.h>
>> +#include <linux/property.h>
>> +#include <linux/regmap.h>
> 
> + string.h
> + types.h
> 
> (and maybe more that I missed).

Accepted. Will audit all three files (core, MMIO, header) for IWYU
and add missing includes.

> 
> 
> ...
> 
>> +#define SYSMON_CHAN_TEMP(_chan, _address, _ext) {            \
>> +     .type = IIO_TEMP,                                       \
>> +     .indexed = 1,                                           \
>> +     .address = _address,                                    \
>> +     .channel = _chan,                                       \
> 
>> +     .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |          \
>> +             BIT(IIO_CHAN_INFO_PROCESSED),                   \
> 
> It's better to use the following style:
> 
>          .info_mask_separate =                                   \
>                  BIT(IIO_CHAN_INFO_RAW) |                        \
>                  BIT(IIO_CHAN_INFO_PROCESSED),                   \

Accepted.

> 
> 
>> +     .scan_type = {                                          \
>> +             .sign = 's',                                    \
>> +             .realbits = 15,                                 \
>> +             .storagebits = 16,                              \
>> +             .endianness = IIO_CPU,                          \
>> +     },                                                      \
>> +     .datasheet_name = _ext,                                 \
>> +}
> 
> ...
> 
>> +static void sysmon_q8p7_to_millicelsius(int raw_data, int *val)
>> +{
>> +     *val = ((s16)raw_data * SYSMON_MILLI) >> SYSMON_FRACTIONAL_SHIFT;
> 
> No casting is needed, just make the type s16 to begin with.

Accepted. Will change the parameter type to s16.

> 
>> +}
> 
> ...
> 
>> +static void sysmon_supply_rawtoprocessed(int raw_data, int *val)
>> +{
>> +     int mantissa, format, exponent;
>> +
>> +     mantissa = FIELD_GET(SYSMON_MANTISSA_MASK, raw_data);
>> +     exponent = SYSMON_SUPPLY_MANTISSA_BITS - FIELD_GET(SYSMON_MODE_MASK, raw_data);
>> +     format = FIELD_GET(SYSMON_FMT_MASK, raw_data);
>> +     /*
>> +      * When format bit is set the mantissa is two's complement
>> +      * (per hardware spec); sign-extend to int for correct arithmetic.
>> +      */
>> +     if (format)
>> +             mantissa = (int)(s16)mantissa;
> 
> Potential user of FIELD_GET_SIGNED(), but for now just use explicit call to
> sign_extend32().

Accepted. Will use sign_extend32(mantissa, 15).

> 
>> +     *val = (mantissa * SYSMON_MILLI) >> exponent;
>> +}
> 
> ...
> 
>> +static int sysmon_read_raw(struct iio_dev *indio_dev,
>> +                        struct iio_chan_spec const *chan, int *val,
>> +                        int *val2, long mask)
> 
> Use logical split:

Accepted. Will use:

static int sysmon_read_raw(struct iio_dev *indio_dev,
                            struct iio_chan_spec const *chan,
                            int *val, int *val2, long mask)

> 
> static int sysmon_read_raw(struct iio_dev *indio_dev,
>                             struct iio_chan_spec const *chan, int *val, int *val2,
>                             long mask)
> 
> OR (to be strict in 80 limit)
> 
> 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;
>> +
>> +     if (mask != IIO_CHAN_INFO_RAW && mask != IIO_CHAN_INFO_PROCESSED)
>> +             return -EINVAL;
>> +
>> +     guard(mutex)(&sysmon->lock);
>> +
>> +     switch (chan->type) {
>> +     case IIO_TEMP:
>> +             ret = regmap_read(sysmon->regmap, chan->address, &regval);
>> +             if (ret)
>> +                     return ret;
>> +             if (mask == IIO_CHAN_INFO_PROCESSED)
>> +                     sysmon_q8p7_to_millicelsius(regval, val);
>> +             else
>> +                     *val = (int)regval;
> 
> Why casting?

Unneccessary, will remove.

> 
>> +             return IIO_VAL_INT;
>> +
>> +     case IIO_VOLTAGE:
>> +             ret = regmap_read(sysmon->regmap,
>> +                               (chan->address * SYSMON_REG_STRIDE) + SYSMON_SUPPLY_BASE,
>> +                               &regval);
>> +             if (ret)
>> +                     return ret;
>> +             if (mask == IIO_CHAN_INFO_PROCESSED)
>> +                     sysmon_supply_rawtoprocessed(regval, val);
>> +             else
>> +                     *val = (int)regval;
> 
> Ditto.
> 
>> +             return IIO_VAL_INT;
>> +
>> +     default:
>> +             return -EINVAL;
>> +     }
>> +}
> 
> ...
> 
>> +/**
>> + * sysmon_parse_fw() - Parse firmware nodes and configure IIO channels.
>> + * @indio_dev: IIO device instance
>> + * @dev: Parent device
>> + *
>> + * Reads supply-channels and temperature-channels container nodes from
>> + * firmware and builds the IIO channel array. Static temperature channels
>> + * are prepended, followed by supply and satellite channels from DT.
>> + *
>> + * Return: 0 on success, negative errno on failure.
>> + */
>> +static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev)
>> +{
>> +     unsigned int idx, temp_chan_idx, volt_chan_idx;
>> +     struct fwnode_handle *supply_node, *temp_node;
> 
>> +     unsigned int num_supply = 0, num_temp = 0;
> 
> Better for maintenance to split these assignments and perform them when they
> are really needed.

Accepted.
> 
>> +     struct iio_chan_spec *sysmon_channels;
>> +     const char *label;
>> +     u32 reg;
>> +     int ret;
> 
>> +     supply_node = device_get_named_child_node(dev, "supply-channels");
> 
> Use what cleanup.h provides you,

Accepted. Will use __free(fwnode_handle) for both supply_node and 
temp_node, eliminating the goto err_put pattern.

> 
>> +     if (supply_node)
>> +             num_supply = fwnode_get_child_node_count(supply_node);
>> +
>> +     temp_node = device_get_named_child_node(dev, "temperature-channels");
>> +     if (temp_node)
>> +             num_temp = fwnode_get_child_node_count(temp_node);
> 
> As per above.
> 
>> +     sysmon_channels = devm_kcalloc(dev,
>> +                                    ARRAY_SIZE(temp_channels) +
>> +                                    num_supply + num_temp,
> 
> Each of num_supply and num_temp may lead to overflow on 32-bit machines.

Will use size_add() for the allocation size computation.

> 
>> +                                    sizeof(*sysmon_channels), GFP_KERNEL);
>> +     if (!sysmon_channels) {
>> +             ret = -ENOMEM;
>> +             goto err_put;
>> +     }
>> +
>> +     /* Static temperature channels first (fixed indices) */
>> +     idx = 0;
>> +     memcpy(sysmon_channels, temp_channels, sizeof(temp_channels));
>> +     idx += ARRAY_SIZE(temp_channels);
>> +
>> +     /* Supply channels from DT */
> 
>> +     if (supply_node) {
> 
> Why? Do you expect the below loop to go on this being NULL?

With _free(fwnode_handle), the NULL guard becomes unnecessary since 
fwnode_for_each_child_node_scoped handles NULL parent. Will remove.

> 
>> +             fwnode_for_each_child_node_scoped(supply_node, child) {
>> +                     ret = fwnode_property_read_u32(child, "reg", &reg);
>> +                     if (ret < 0)
>> +                             goto err_put;
>> +
>> +                     if (reg > SYSMON_SUPPLY_IDX_MAX) {
>> +                             ret = -EINVAL;
>> +                             dev_err(dev, "supply reg %u exceeds max %u\n",
>> +                                     reg, SYSMON_SUPPLY_IDX_MAX);
>> +                             goto err_put;
>> +                     }
>> +
>> +                     ret = fwnode_property_read_string(child, "label",
>> +                                                       &label);
>> +                     if (ret < 0)
>> +                             goto err_put;
>> +
>> +                     sysmon_channels[idx++] = (struct iio_chan_spec) {
>> +                             .type = IIO_VOLTAGE,
>> +                             .indexed = 1,
>> +                             .address = reg,
>> +                             .info_mask_separate =
>> +                                     BIT(IIO_CHAN_INFO_RAW) |
>> +                                     BIT(IIO_CHAN_INFO_PROCESSED),
>> +                             .scan_type = {
>> +                                     .realbits = 19,
>> +                                     .storagebits = 32,
>> +                                     .endianness = IIO_CPU,
>> +                                     .sign = fwnode_property_read_bool(child,
>> +                                             "bipolar") ? 's' : 'u',
>> +                             },
>> +                             .datasheet_name = label,
>> +                     };
>> +             }
>> +             fwnode_handle_put(supply_node);
>> +             supply_node = NULL;
>> +     }
>> +
>> +     /* Temperature satellite channels from DT */
>> +     if (temp_node) {
> 
> As per above.
> 
>> +             fwnode_for_each_child_node_scoped(temp_node, child) {
>> +                     ret = fwnode_property_read_u32(child, "reg", &reg);
>> +                     if (ret < 0)
>> +                             goto err_put;
>> +
>> +                     if (reg < 1 || reg > SYSMON_TEMP_SAT_MAX) {
>> +                             ret = -EINVAL;
>> +                             dev_err(dev, "temp reg %u out of range [1..%u]\n",
>> +                                     reg, SYSMON_TEMP_SAT_MAX);
>> +                             goto err_put;
>> +                     }
>> +
>> +                     ret = fwnode_property_read_string(child, "label",
>> +                                                       &label);
>> +                     if (ret < 0)
>> +                             goto err_put;
>> +
>> +                     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) |
>> +                                     BIT(IIO_CHAN_INFO_PROCESSED),
>> +                             .scan_type = {
>> +                                     .sign = 's',
>> +                                     .realbits = 15,
>> +                                     .storagebits = 16,
>> +                                     .endianness = IIO_CPU,
>> +                             },
>> +                             .datasheet_name = label,
>> +                     };
>> +             }
>> +             fwnode_handle_put(temp_node);
>> +             temp_node = NULL;
>> +     }
>> +
>> +     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 (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;
>> +
>> +err_put:
>> +     fwnode_handle_put(supply_node);
>> +     fwnode_handle_put(temp_node);
>> +     return ret;
>> +}
> 
> ...
> 
>> +#include <linux/io.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
> 
> As per above, please respect and follow IWYU principle.

Will audit the MMIO driver and header for IWYU as well.

> 
> ...
> 
>> +static int sysmon_platform_probe(struct platform_device *pdev)
>> +{
>> +     struct sysmon_mmio *mmio;
>> +     struct regmap *regmap;
>> +     int irq;
>> +
>> +     mmio = devm_kzalloc(&pdev->dev, sizeof(*mmio), GFP_KERNEL);
>> +     if (!mmio)
>> +             return -ENOMEM;
>> +
>> +     mmio->base = devm_platform_ioremap_resource(pdev, 0);
>> +     if (IS_ERR(mmio->base))
>> +             return PTR_ERR(mmio->base);
>> +
>> +     regmap = devm_regmap_init(&pdev->dev, NULL, mmio,
> 
> Using
> 
>          struct device *dev = &pdev->dev;
> 
> at the top of the function makes this code neater and probably a single line.

Accepted.

> 
>> +                               &sysmon_mmio_regmap_config);
>> +     if (IS_ERR(regmap))
>> +             return PTR_ERR(regmap);
>> +
>> +     irq = platform_get_irq_optional(pdev, 0);
>> +
>> +     return sysmon_core_probe(&pdev->dev, regmap, irq);
>> +}
> 
> ...
> 
>> +#include <linux/iio/iio.h>
>> +#include <linux/mutex.h>
>> +#include <linux/regmap.h>
> 
> Ditto for the header files, use and follow the IWYU principle.
> 
> ...
> 
>> +/* Signed milli scale (MILLI from linux/units.h is unsigned long) */
>> +#define SYSMON_MILLI                 1000
> 
> I think you want a different approach. I already forgot how it's being used,
> but there shouldn't be a new (re-)definition just because of this.

The issue is that MILLI from units.h is 1000UL, which causes unsigned
promotion when multiplied with signed int values (e.g. negative
temperatures or two's complement mantissa). There is
MILLIDEGREE_PER_DEGREE (signed 1000) but using it for voltage
channel conversions would be semantically wrong. Will either use
(int)MILLI at the call sites or just use literal 1000 -- happy to
go with whichever you prefer.

> 
> --
> With Best Regards,
> Andy Shevchenko
> 
> 
All items will be addressed in v3.

Salih