Re: [PATCH v4 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:
- [Medium] Non-atomic I2C read transaction using separate send and receive operations.
--

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

iio: adc: versal-sysmon: add I2C driver

Adds an I2C transport driver for the Versal SysMon block, allowing external
masters to read voltage and temperature measurements using the MMIO register
map. It implements read and write operations via an 8-byte command frame.

> diff --git a/drivers/iio/adc/versal-sysmon-i2c.c b/drivers/iio/adc/versal-sysmon-i2c.c
> new file mode 100644
> index 0000000000000..27a5ff4a35f68
> --- /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: Medium]
Is it safe to split this read sequence into two distinct I2C transactions?

Using i2c_master_send() followed by i2c_master_recv() releases the adapter
lock and issues a stop condition between the two calls. While concurrent
accesses from this driver are serialized by the regmap lock, interleaving
transactions from other drivers on the same I2C bus could occur.

Should this use i2c_transfer() with an array of two i2c_msg structures to
perform an atomic transfer with a repeated start, or use i2c_lock_bus() if
the hardware requires a stop condition between the write and read phases?

> +	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/20260606051707.535281-1-salih.erim@amd.com?part=3