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

[Erim, Salih]

Hi Andy,

Replies are inline.

On 04/05/2026 11:25, Andy Shevchenko wrote:

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

Accepted. Will audit and add missing includes.

> ...
>
> > +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).

These are byte offsets in the I2C payload buffer, mapped to the
hardware protocol. Will assign all items explicitly.

> > +     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.

Yes, the struct only wraps the client pointer and is not extended
in later patches. Will remove the wrapper and pass struct i2c_client
directly as the regmap context.

> ...
>
> > +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.

Accepted. Will use sizeof(write_buf) and sizeof(read_buf) throughout
instead of the defines.

> > +             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.

Accepted. Will use = { } instead.

> > +     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

All items will be addressed in v3.

Salih