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

["Erim, Salih" <salih.erim@amd.com>]

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