Re: [PATCH v3 3/5] iio: adc: versal-sysmon: add I2C driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The I2C register read operation splits the write and read phases into two independent API calls, breaking bus atomicity and introducing an unsafe STOP condition.
- [Low] The VERSAL_SYSMON_I2C config is missing the architecture dependency present in the related VERSAL_SYSMON config.
--

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

iio: adc: versal-sysmon: add I2C driver

This commit adds an I2C transport driver for the Versal SysMon block,
allowing reading voltage and temperature measurements over I2C.

> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index c7f1905748497..8f9fc9de74a54 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
[ ... ]
> @@ -1963,6 +1963,19 @@ config VERSAL_SYSMON
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called versal-sysmon.
>  
> +config VERSAL_SYSMON_I2C
> +	tristate "AMD Versal SysMon I2C driver"
> +	depends on I2C
> +	select VERSAL_SYSMON_CORE

[Severity: Low]
Does this config need an architecture dependency?

The related VERSAL_SYSMON config restricts visibility with
"depends on ARCH_ZYNQMP || COMPILE_TEST". By omitting this dependency here,
will VERSAL_SYSMON_I2C be incorrectly presented as an available driver on
all I2C-enabled architectures?

> +	help
> +	  Say yes here to have support for the AMD/Xilinx Versal System
[ ... ]
> diff --git a/drivers/iio/adc/versal-sysmon-i2c.c b/drivers/iio/adc/versal-sysmon-i2c.c
> new file mode 100644
> index 0000000000000..92d149f517e57
> --- /dev/null
> +++ b/drivers/iio/adc/versal-sysmon-i2c.c
[ ... ]
> +static int sysmon_i2c_reg_read(void *context, unsigned int reg,
> +			       unsigned int *val)
> +{
> +	struct i2c_client *client = context;
> +	u8 write_buf[8] = { };
> +	u8 read_buf[4];
> +	int ret;
> +
> +	write_buf[SYSMON_I2C_OFS_LOW_IDX] =
> +		FIELD_GET(SYSMON_I2C_OFS_LOW_MASK, reg);
> +	write_buf[SYSMON_I2C_OFS_HIGH_IDX] =
> +		FIELD_GET(SYSMON_I2C_OFS_HIGH_MASK, reg);
> +	write_buf[SYSMON_I2C_INSTR_IDX] = SYSMON_I2C_INSTR_READ;
> +
> +	ret = i2c_master_send(client, write_buf, sizeof(write_buf));
> +	if (ret < 0)
> +		return ret;
> +	if (ret != sizeof(write_buf))
> +		return -EIO;
> +
> +	ret = i2c_master_recv(client, read_buf, sizeof(read_buf));

[Severity: Medium]
Will this sequence of independent send and receive calls break bus atomicity?

Since i2c_master_send() and i2c_master_recv() independently acquire and
release the I2C adapter lock, this approach generates a STOP condition on
the bus after the write phase.

On a shared I2C bus, couldn't this allow transactions for other devices to
interleave and disrupt the device state or abort the pending read sequence?

Would it be safer to use i2c_transfer() with an array of two i2c_msg
structures to ensure a Repeated START (Sr) condition?

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