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

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

Hi Andy,

Thanks for review, replies are inline.

On 07/06/2026 09:06, Andy Shevchenko wrote:
> 
> On Sat, Jun 06, 2026 at 06:17:04AM +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.
> 
> ...
> 
>> +#include <linux/array_size.h>
>> +#include <linux/bitfield.h>
>> +#include <linux/bitops.h>
>> +#include <linux/cleanup.h>
>> +#include <linux/device.h>
> 
> + err.h

Accepted.
> 
>> +#include <linux/module.h>
>> +#include <linux/property.h>
>> +#include <linux/regmap.h>
>> +#include <linux/string.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/units.h>
>> +
>> +#include <linux/iio/iio.h>
>> +
>> +#include "versal-sysmon.h"
> 
> ...
> 
>> +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 = (int)MILLI;
> 
> Casting is not required here.

Accepted. Will drop (int) cast in the scale assignment.

> 
>> +                     *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 = (s16)regval;
> 
> Basically you want to use sign_extend32(regval, 15) for consistency.

Accepted.

> 
>> +             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) +
> 
> Unneeded parentheses.

Accepted. Will remove throughout.

> 
>> +                               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_supply = 0, num_temp = 0;
>> +     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");
> 
>> +     if (supply_node)
> 
> Do you need this check? IIRC the below is NULL-aware.

You're right, fwnode_get_child_node_count() handles NULL
(fwnode_get_next_child_node returns NULL for NULL fwnode).
Will drop both checks.
  >
>> +             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");
>> +     if (temp_node)
> 
> Same Q here?

Will drop this check too.
> 
>> +             num_temp = fwnode_get_child_node_count(temp_node);
>> +
>> +     sysmon_channels = devm_kcalloc(dev,
>> +                                    size_add(ARRAY_SIZE(temp_channels),
>> +                                             num_supply + num_temp),
> 
> size_add() should be called twice.

Accepted.
> 
>> +                                    sizeof(*sysmon_channels), GFP_KERNEL);
>> +     if (!sysmon_channels)
>> +             return -ENOMEM;
> 
> ...
> 
>> +     /* Temperature satellite channels from DT */
>> +     fwnode_for_each_child_node_scoped(temp_node, child) {
>> +             ret = fwnode_property_read_u32(child, "reg", &reg);
>> +             if (ret < 0)
> 
> Can fwnode APIs like this return positive? If so, what is the meaning of that
> and why do we ignore it?

They cannot return positive. fwnode_property_read_u32() returns
0 on success or negative errno. Will change to if (ret) for
consistency.

> 
>> +                     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 < 0)
>> +                     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),
> 
> Too many parentheses.

Accepted. (reg - 1) needs parens for precedence, will remove the
outer pair.

> 
>> +                     .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>> +                     .info_mask_shared_by_type =
>> +                             BIT(IIO_CHAN_INFO_SCALE),
>> +                     .datasheet_name = label,
>> +             };
>> +     }
> 
> ...
> 
>> +     for (idx = 0; idx < indio_dev->num_channels; idx++) {
> 
>          for (unsigned int idx = 0; idx < indio_dev->num_channels; idx++) {
> 
> ?

Accepted.
> 
>> +             if (sysmon_channels[idx].type == IIO_TEMP)
>> +                     sysmon_channels[idx].channel = temp_chan_idx++;
>> +             else
>> +                     sysmon_channels[idx].channel = volt_chan_idx++;
>> +     }
> 
> ...
> 
>> +/**
>> + * sysmon_core_probe() - Initialize Versal SysMon core
>> + * @dev: Parent device
>> + * @regmap: Register map for hardware access
>> + *
>> + * Return: 0 on success, negative errno on failure.
>> + */
>> +int sysmon_core_probe(struct device *dev, struct regmap *regmap)
> 
> I'm wondering if the @regmap is the same as you can get from @dev via
> respective API.

Yes, dev_get_regmap(dev, NULL) would work since devm_regmap_init
registers it with devres. However, the explicit parameter makes
the dependency clear and avoids coupling core_probe to the devres
registration order. Happy to change if you prefer dev_get_regmap().

> 
> ...
> 
> + err.h // covers IS_ERR(), -Exxx

Accepted.

> 
>> +#include <linux/io.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
> 
> + types.h // covers NULL, __iomem
> 
> IWYU, please.

Accepted.

> 
>> +#include "versal-sysmon.h"
>> +
>> +struct sysmon_mmio {
>> +     void __iomem *base;
> 
> __iomem is not defined in the above headers.

Accepted, covered by types.h above.

> 
>> +};
> 
> ...
> 
>> +static int sysmon_platform_probe(struct platform_device *pdev)
>> +{
>> +     struct device *dev = &pdev->dev;
>> +     struct sysmon_mmio *mmio;
>> +     struct regmap *regmap;
>> +
>> +     mmio = devm_kzalloc(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);
> 
> IS_ERR() is not provided in the headers included.

Accepted, covered by err.h above.

> 
>> +
>> +     regmap = devm_regmap_init(dev, NULL, mmio, &sysmon_mmio_regmap_config);
> 
> NULL is not defined in the above headers.

Accepted, covered by types.h above.

Salih

...