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

Andy Shevchenko
2026-05-04T10:25:35Z

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