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

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

Hi Jonathan,

On 28/05/2026 13:42, Jonathan Cameron wrote:
> 
> 
> On Wed, 27 May 2026 12:42:09 +0100
> Salih Erim <salih.erim@amd.com> wrote:
> 
>> Add an I2C transport driver for the Versal SysMon block. The SysMon
>> provides an I2C slave interface that allows an external master to
>> read voltage and temperature measurements through the same register
>> map used by the MMIO path.
>>
>> The I2C command frame is an 8-byte structure containing a 4-byte data
>> payload, a 2-byte register offset, and a 1-byte instruction field.
>> Read operations send the frame with a read instruction, then receive
>> a 4-byte response containing the register value.
>>
>> Events are not supported on the I2C path because there is no
>> interrupt line and the I2C regmap backend cannot be called from
>> atomic context.
>>
>> Co-developed-by: Conall O'Griofa <conall.ogriofa@amd.com>
>> Signed-off-by: Conall O'Griofa <conall.ogriofa@amd.com>
>> Signed-off-by: Salih Erim <salih.erim@amd.com>
> A few minor things inline.
> 
>> diff --git a/drivers/iio/adc/versal-sysmon-i2c.c b/drivers/iio/adc/versal-sysmon-i2c.c
>> new file mode 100644
>> index 00000000000..92d149f517e
>> --- /dev/null
>> +++ b/drivers/iio/adc/versal-sysmon-i2c.c
>> @@ -0,0 +1,153 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * AMD Versal SysMon I2C driver
>> + *
>> + * Copyright (C) 2023 - 2026, Advanced Micro Devices, Inc.
>> + */
>> +
>> +#include <linux/bitfield.h>
>> +#include <linux/bits.h>
>> +#include <linux/i2c.h>
>> +#include <linux/module.h>
>> +#include <linux/regmap.h>
>> +
>> +#include "versal-sysmon.h"
>> +
>> +#define SYSMON_I2C_INSTR_READ        BIT(2)
>> +#define SYSMON_I2C_INSTR_WRITE       BIT(3)
>> +
>> +#define SYSMON_I2C_DATA0_MASK        GENMASK(7, 0)
>> +#define SYSMON_I2C_DATA1_MASK        GENMASK(15, 8)
>> +#define SYSMON_I2C_DATA2_MASK        GENMASK(23, 16)
>> +#define SYSMON_I2C_DATA3_MASK        GENMASK(31, 24)
>> +
>> +#define SYSMON_I2C_OFS_LOW_MASK              GENMASK(9, 2)
>> +#define SYSMON_I2C_OFS_HIGH_MASK     GENMASK(15, 10)
>> +
>> +/* Byte positions within the 8-byte I2C command frame (HW-defined) */
>> +enum sysmon_i2c_payload_idx {
>> +     SYSMON_I2C_DATA0_IDX = 0,
>> +     SYSMON_I2C_DATA1_IDX = 1,
>> +     SYSMON_I2C_DATA2_IDX = 2,
>> +     SYSMON_I2C_DATA3_IDX = 3,
>> +     SYSMON_I2C_OFS_LOW_IDX = 4,
>> +     SYSMON_I2C_OFS_HIGH_IDX = 5,
> 
> With changes suggested below I think you only need the two base
> offsets and the final one.  As such maybe 3 defines makes more sense
> than an enum

Accepted. Will switch to defines.

> 
>> +     SYSMON_I2C_INSTR_IDX = 6,
> If you do keep an enum, it would be good to add an entry for the final
> byte to give some indication of why it is 8 bytes. Even if that is
> reserved0

Switching to defines as above.

>> +};
>> +
>> +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;
>> +
>> +     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);
> 
> I'd guess an unaligned put works here a well? though you'll need
> to do a FIELD_GET() to extract the slightly shifted content.

Accepted.

> 
>> +     write_buf[SYSMON_I2C_INSTR_IDX] = 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));
>> +     if (ret < 0)
>> +             return ret;
>> +     if (ret != sizeof(read_buf))
>> +             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]);
> Very complex way to express what I think is
>          *val = get_unaligned_le32(&read_buf[0]);

Accepted.

>> +
>> +     return 0;
>> +}
>> +
>> +static int sysmon_i2c_reg_write(void *context, unsigned int reg,
>> +                             unsigned int val)
>> +{
>> +     struct i2c_client *client = context;
>> +     u8 write_buf[8] = { };
> 
>> +     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);
> 
> That's a put_unaligned_le32() I think?

Accepted.

> 
>> +     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);
> 
> I'd guess put_unaligned_le16()? Will need a a FIELD_PREP()
> for the full thing though.

Accepted.

> 
>> +     write_buf[SYSMON_I2C_INSTR_IDX] = SYSMON_I2C_INSTR_WRITE;
>> +
>> +     ret = i2c_master_send(client, write_buf, sizeof(write_buf));
>> +     if (ret < 0)
>> +             return ret;
>> +     if (ret != sizeof(write_buf))
>> +             return -EIO;
>> +
>> +     return 0;
>> +}
> 
>> +static const struct of_device_id sysmon_i2c_of_match_table[] = {
>> +     { .compatible = "xlnx,versal-sysmon" },
>> +     { }
>> +};
>> +MODULE_DEVICE_TABLE(of, sysmon_i2c_of_match_table);
>> +
>> +static const struct i2c_device_id sysmon_i2c_id_table[] = {
>> +     { "versal-sysmon" },
> 
> Named initializer for this.  Uwe is cleaning these up across IIO;
> let us not add another one!

Accepted. Will use { .name = "versal-sysmon" }.

All items will be addressed in v4.

Salih
> 
>> +     { }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, sysmon_i2c_id_table);
>> +
>> +static struct i2c_driver sysmon_i2c_driver = {
>> +     .probe = sysmon_i2c_probe,
>> +     .driver = {
>> +             .name = "versal-sysmon-i2c",
>> +             .of_match_table = sysmon_i2c_of_match_table,
>> +     },
>> +     .id_table = sysmon_i2c_id_table,
>> +};
>> +module_i2c_driver(sysmon_i2c_driver);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("AMD Versal SysMon I2C Driver");
>> +MODULE_AUTHOR("Conall O'Griofa <conall.ogriofa@amd.com>");
>> +MODULE_AUTHOR("Salih Erim <salih.erim@amd.com>");
>