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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Non-atomic I2C read operation splits the command write and data read into two distinct I2C transactions, risking state corruption from interleaved bus traffic.
--

commit ae0266701d933c996d4059785f81513fb71dc331
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. It
provides an I2C slave interface that allows an external master to read
voltage and temperature measurements through the same register map used by
the MMIO path.

> diff --git a/drivers/iio/adc/versal-sysmon-i2c.c b/drivers/iio/adc/versal-sysmon-i2c.c
> new file mode 100644
> index 0000000000000..e9a7629159ab1
> --- /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;
> +
> +	put_unaligned_le16(reg >> 2, &write_buf[SYSMON_I2C_REG_OFS]);
> +	write_buf[SYSMON_I2C_INSTR_OFS] = 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: High]
Does this sequence release the I2C bus lock between the command write and the
response read?

Using i2c_master_send() followed by i2c_master_recv() will independently
acquire and release the I2C adapter lock, generating a STOP condition on the
bus between the two operations.

If other devices share this I2C bus, could an interleaved transaction reset
the SysMon's internal state machine and cause the subsequent read to return
corrupted data?

Should this be implemented using a single i2c_transfer() with two messages
(using a Repeated START), or explicitly wrapped in i2c_lock_bus() and
i2c_unlock_bus() if a STOP condition is strictly required by the hardware?

> +	if (ret < 0)
> +		return ret;
> +	if (ret != sizeof(read_buf))
> +		return -EIO;
> +
> +	*val = get_unaligned_le32(read_buf);
> +
> +	return 0;
> +}

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