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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The Kconfig dependency prevents building the driver on its intended architecture.
- [High] Raw temperature reads lack sign extension, and there is a mismatch in bit width definitions causing negative temperatures to be corrupted.
- [Medium] Exposing IIO_CHAN_INFO_RAW for packed floating-point hardware formats violates the IIO UAPI.
--

commit 084edfc78fc9fd37f78ac260bcd2da4ecb832e8a
Author: Salih Erim <salih.erim@amd.com>

iio: adc: add Versal SysMon driver

Adds the core driver and MMIO platform driver for the AMD/Xilinx Versal
System Monitor (SysMon) block, providing voltage and temperature monitoring.

> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig

[ ... ]

> @@ -1943,6 +1943,26 @@ config TWL6030_GPADC
>  	  This driver can also be built as a module. If so, the module will be
>  	  called twl6030-gpadc.
>  
> +config VERSAL_SYSMON_CORE
> +	tristate
> +	select REGMAP
> +
> +config VERSAL_SYSMON
> +	tristate "AMD Versal SysMon driver"
> +	depends on ARCH_ZYNQMP || COMPILE_TEST

[Severity: High]
Could this Kconfig dependency prevent the driver from being built on its
intended architecture? Since this driver targets the AMD Versal platform,
should it depend on ARCH_VERSAL instead of ARCH_ZYNQMP?

> +	depends on HAS_IOMEM
> +	select VERSAL_SYSMON_CORE

[ ... ]

> diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
> --- /dev/null
> +++ b/drivers/iio/adc/versal-sysmon-core.c

[ ... ]

> +/*
> + * Both RAW and PROCESSED are exposed: RAW is needed for event thresholds
> + * (which operate in hardware register format), PROCESSED gives userspace
> + * the converted millivolt or millicelsius value.
> + */

[Severity: Medium]
Does exposing RAW for packed floating-point formats violate the IIO UAPI?

The IIO ABI dictates that _raw attributes must represent a simple, unscaled
linear integer value. Exporting a packed format containing mantissa, exponent,
and format bits could break generic userspace clients expecting linear values.

Since event thresholds use their own dedicated sysfs attributes, is it
necessary to expose _raw on the main channel?

> +#define SYSMON_CHAN_TEMP(_chan, _address, _name) {		\
> +	.type = IIO_TEMP,					\
> +	.indexed = 1,						\
> +	.address = _address,					\
> +	.channel = _chan,					\
> +	.info_mask_separate =					\
> +		BIT(IIO_CHAN_INFO_RAW) |				\
> +		BIT(IIO_CHAN_INFO_PROCESSED),			\
> +	.scan_type = {						\
> +		.sign = 's',					\
> +		.realbits = 15,					\

[Severity: High]
There appears to be a mismatch between the realbits defined here and the
parameter type used in the conversion function below.

If the hardware format is actually 15-bit, will casting to s16 in
sysmon_q8p7_to_millicelsius() fail to sign-extend negative values? Or if
the hardware format is actually 16-bit, should realbits be set to 16?

> +		.storagebits = 16,				\
> +		.endianness = IIO_CPU,				\
> +	},							\
> +	.datasheet_name = _name,				\
> +}

[ ... ]

> +static void sysmon_q8p7_to_millicelsius(s16 raw_data, int *val)
> +{
> +	*val = (raw_data * (int)MILLI) >> SYSMON_FRACTIONAL_SHIFT;
> +}

[ ... ]

> +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 = regval;

[Severity: High]
Can this direct assignment corrupt negative temperature reads?

Since regval is unsigned int, assigning it directly to *val without
sign-extending the hardware value could result in a large positive integer
being exported to sysfs when the temperature is negative.

> +		return IIO_VAL_INT;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527114211.174288-1-salih.erim@amd.com?part=2