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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Sat, May 02, 2026 at 12:19:49PM +0100, Salih Erim wrote:
> Add I2C bus driver for Versal SysMon to enable voltage and temperature
> monitoring when the Versal chip has SysMon configured with an I2C
> interface.
> 
> The I2C protocol uses a custom 8-byte write format:
>   - Bytes 0-3: Data (little-endian, 32-bit)
>   - Bytes 4-5: Register offset (split into low/high parts)
>   - Byte 6: Instruction (read=0x4, write=0x8)
>   - Byte 7: Reserved
> 
> For reads, the driver sends the 8-byte command then receives 4 bytes
> of data. For writes, it sends the 8-byte command with embedded data.
> 
> The driver uses the regmap API with custom read/write callbacks to
> share the bus-agnostic core driver (versal-sysmon-core).
> 
> Event support is not available on I2C since the SysMon interrupt
> lines are not routed over the I2C bus.

...

> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>

Follow IWYU.

...

> +enum sysmon_i2c_payload_idx {
> +	SYSMON_I2C_DATA0_IDX = 0,

Is it mapped to HW bits or is it pure Linux enum? If the former, assign *all*
items as per datasheet, otherwise drop explicit assignment (it's rare that we
need it in the software).

> +	SYSMON_I2C_DATA1_IDX,
> +	SYSMON_I2C_DATA2_IDX,
> +	SYSMON_I2C_DATA3_IDX,
> +	SYSMON_I2C_OFS_LOW_IDX,
> +	SYSMON_I2C_OFS_HIGH_IDX,
> +	SYSMON_I2C_INSTR_IDX,
> +};

...

> +struct sysmon_i2c {
> +	struct i2c_client *client;
> +};

Can't be the struct i2c_client used directly? (Haven't checked if this is going
to be extended or have special uses.

...

> +static int sysmon_i2c_reg_read(void *context, unsigned int reg,
> +			       unsigned int *val)
> +{
> +	u8 write_buf[SYSMON_I2C_WRITE_SIZE] = { 0 };
> +	u8 read_buf[SYSMON_I2C_READ_SIZE];
> +	struct sysmon_i2c *priv = context;
> +	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(priv->client, write_buf, SYSMON_I2C_WRITE_SIZE);

sizeof()

> +	if (ret < 0)
> +		return ret;
> +	if (ret != SYSMON_I2C_WRITE_SIZE)

sizeof()

> +		return -EIO;
> +
> +	ret = i2c_master_recv(priv->client, read_buf, SYSMON_I2C_READ_SIZE);

sizeof()

> +	if (ret < 0)
> +		return ret;
> +	if (ret != SYSMON_I2C_READ_SIZE)

sizeof()

With them the code will have one source of length an be robust to the changes
of the buffer sizes.

> +		return -EIO;
> +
> +	*val = FIELD_PREP(SYSMON_I2C_DATA0_MASK,
> +			  read_buf[SYSMON_I2C_DATA0_IDX]) |
> +	       FIELD_PREP(SYSMON_I2C_DATA1_MASK,
> +			  read_buf[SYSMON_I2C_DATA1_IDX]) |
> +	       FIELD_PREP(SYSMON_I2C_DATA2_MASK,
> +			  read_buf[SYSMON_I2C_DATA2_IDX]) |
> +	       FIELD_PREP(SYSMON_I2C_DATA3_MASK,
> +			  read_buf[SYSMON_I2C_DATA3_IDX]);
> +
> +	return 0;
> +}

...

> +static int sysmon_i2c_reg_write(void *context, unsigned int reg,
> +				unsigned int val)
> +{
> +	u8 write_buf[SYSMON_I2C_WRITE_SIZE] = { 0 };

'0' is redundant.

> +	struct sysmon_i2c *priv = context;
> +	int ret;
> +
> +	write_buf[SYSMON_I2C_DATA0_IDX] =
> +		FIELD_GET(SYSMON_I2C_DATA0_MASK, val);
> +	write_buf[SYSMON_I2C_DATA1_IDX] =
> +		FIELD_GET(SYSMON_I2C_DATA1_MASK, val);
> +	write_buf[SYSMON_I2C_DATA2_IDX] =
> +		FIELD_GET(SYSMON_I2C_DATA2_MASK, val);
> +	write_buf[SYSMON_I2C_DATA3_IDX] =
> +		FIELD_GET(SYSMON_I2C_DATA3_MASK, val);
> +	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_WRITE;

> +	ret = i2c_master_send(priv->client, write_buf, SYSMON_I2C_WRITE_SIZE);
> +	if (ret < 0)
> +		return ret;
> +	if (ret != SYSMON_I2C_WRITE_SIZE)
> +		return -EIO;

sizeof() in both cases.

> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko