Re: [PATCH 2/3] i2c: designware: Introduce per-variant register offset and bit-layout tables

From: Mika Westerberg

Date: Mon Sep 21 2026 - 07:28:17 EST

Hi,

On Sat, Sep 19, 2026 at 02:36:07PM +0530, Aniket Limaye wrote:
> Every DW_IC_* register offset and CON-register bit position is currently
> baked in as a compile-time constant, which only works while there is a
> single register layout. Introduce a logical register-ID enum (enum
> dw_i2c_reg_idx) plus a per-variant offset table (dev->regs[]) and a
> per-variant CON-register bit-layout descriptor (dev->con_bits), selected
> at probe time via the new i2c_dw_select_variant().
>
> Replace every direct DW_IC_* offset/bit-position reference with a lookup
> through dev->regs[]/dev->con_bits. Also fold the read-to-clear
> interrupt-acknowledgment pattern into a new i2c_dw_ack_intr() helper,
> driven by a per-variant dev->intr_clr[] table.
>
> Only one variant exists at this point (DW_apb_i2c), so this is a
> mechanical, behavior-preserving change: the values in
> dw_i2c_reg_offsets[] and dw_i2c_con_bits match the DW_IC_* macros
> exactly. It lays the groundwork for adding a second register layout
> (DWC_i2c) without duplicating the whole driver.
>
> Signed-off-by: Aniket Limaye <a-limaye@xxxxxx>
> ---
> drivers/i2c/busses/i2c-designware-amdisp.c | 1 +
> drivers/i2c/busses/i2c-designware-common.c | 158 ++++++++++++++++++++++------
> drivers/i2c/busses/i2c-designware-core.h | 108 ++++++++++++++++++-
> drivers/i2c/busses/i2c-designware-master.c | 125 +++++++++++-----------
> drivers/i2c/busses/i2c-designware-pcidrv.c | 2 +
> drivers/i2c/busses/i2c-designware-platdrv.c | 2 +
> drivers/i2c/busses/i2c-designware-slave.c | 42 ++++----
> 7 files changed, 318 insertions(+), 120 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-amdisp.c b/drivers/i2c/busses/i2c-designware-amdisp.c
> index 9f0ec0fae6f2..f7aa075c9977 100644
> --- a/drivers/i2c/busses/i2c-designware-amdisp.c
> +++ b/drivers/i2c/busses/i2c-designware-amdisp.c
> @@ -46,6 +46,7 @@ static int amd_isp_dw_i2c_plat_probe(struct platform_device *pdev)
> isp_i2c_dev->flags |= ACCESS_POLLING;
> platform_set_drvdata(pdev, isp_i2c_dev);
>
> + i2c_dw_select_variant(isp_i2c_dev);
> isp_i2c_dev->base = devm_platform_ioremap_resource(pdev, 0);
> if (IS_ERR(isp_i2c_dev->base))
> return dev_err_probe(&pdev->dev, PTR_ERR(isp_i2c_dev->base),
> diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
> index a1eca6cd4b75..a21aeb7f415a 100644
> --- a/drivers/i2c/busses/i2c-designware-common.c
> +++ b/drivers/i2c/busses/i2c-designware-common.c
> @@ -72,6 +72,95 @@ static const char *const abort_sources[] = {
> "incorrect slave-transmitter mode configuration",
> };
>
> +/* "snps,designware-i2c" flat register layout */
> +static const u32 dw_i2c_reg_offsets[DW_REG_IDX_MAX] = {
> + [DW_REG_IDX_CON] = DW_IC_CON,
> + [DW_REG_IDX_TAR] = DW_IC_TAR,
> + [DW_REG_IDX_SAR] = DW_IC_SAR,
> + [DW_REG_IDX_DATA_CMD] = DW_IC_DATA_CMD,
> + [DW_REG_IDX_SS_SCL_HCNT] = DW_IC_SS_SCL_HCNT,
> + [DW_REG_IDX_SS_SCL_LCNT] = DW_IC_SS_SCL_LCNT,
> + [DW_REG_IDX_FS_SCL_HCNT] = DW_IC_FS_SCL_HCNT,
> + [DW_REG_IDX_FS_SCL_LCNT] = DW_IC_FS_SCL_LCNT,
> + [DW_REG_IDX_HS_SCL_HCNT] = DW_IC_HS_SCL_HCNT,
> + [DW_REG_IDX_HS_SCL_LCNT] = DW_IC_HS_SCL_LCNT,
> + [DW_REG_IDX_INTR_STAT] = DW_IC_INTR_STAT,
> + [DW_REG_IDX_INTR_MASK] = DW_IC_INTR_MASK,
> + [DW_REG_IDX_RAW_INTR_STAT] = DW_IC_RAW_INTR_STAT,
> + [DW_REG_IDX_RX_TL] = DW_IC_RX_TL,
> + [DW_REG_IDX_TX_TL] = DW_IC_TX_TL,
> + [DW_REG_IDX_CLR_INTR] = DW_IC_CLR_INTR,
> + [DW_REG_IDX_CLR_RX_UNDER] = DW_IC_CLR_RX_UNDER,
> + [DW_REG_IDX_CLR_RX_OVER] = DW_IC_CLR_RX_OVER,
> + [DW_REG_IDX_CLR_TX_OVER] = DW_IC_CLR_TX_OVER,
> + [DW_REG_IDX_CLR_RD_REQ] = DW_IC_CLR_RD_REQ,
> + [DW_REG_IDX_CLR_TX_ABRT] = DW_IC_CLR_TX_ABRT,
> + [DW_REG_IDX_CLR_RX_DONE] = DW_IC_CLR_RX_DONE,
> + [DW_REG_IDX_CLR_ACTIVITY] = DW_IC_CLR_ACTIVITY,
> + [DW_REG_IDX_CLR_STOP_DET] = DW_IC_CLR_STOP_DET,
> + [DW_REG_IDX_CLR_START_DET] = DW_IC_CLR_START_DET,
> + [DW_REG_IDX_CLR_GEN_CALL] = DW_IC_CLR_GEN_CALL,
> + [DW_REG_IDX_ENABLE] = DW_IC_ENABLE,
> + [DW_REG_IDX_STATUS] = DW_IC_STATUS,
> + [DW_REG_IDX_TXFLR] = DW_IC_TXFLR,
> + [DW_REG_IDX_RXFLR] = DW_IC_RXFLR,
> + [DW_REG_IDX_SDA_HOLD] = DW_IC_SDA_HOLD,
> + [DW_REG_IDX_TX_ABRT_SOURCE] = DW_IC_TX_ABRT_SOURCE,
> + [DW_REG_IDX_ENABLE_STATUS] = DW_IC_ENABLE_STATUS,
> + [DW_REG_IDX_SMBUS_INTR_MASK] = DW_IC_SMBUS_INTR_MASK,
> + [DW_REG_IDX_COMP_PARAM_1] = DW_IC_COMP_PARAM_1,
> + [DW_REG_IDX_COMP_VERSION] = DW_IC_COMP_VERSION,
> + [DW_REG_IDX_COMP_TYPE] = DW_IC_COMP_TYPE,
> +};
> +
> +static const struct dw_i2c_con_bits dw_i2c_con_bits = {
> + .master = DW_IC_CON_MASTER,
> + .speed_std = DW_IC_CON_SPEED_STD,
> + .speed_fast = DW_IC_CON_SPEED_FAST,
> + .speed_high = DW_IC_CON_SPEED_HIGH,
> + .speed_mask = DW_IC_CON_SPEED_MASK,
> + .bit10_slave = DW_IC_CON_10BITADDR_SLAVE,
> + .bit10_master = DW_IC_CON_10BITADDR_MASTER,
> + .restart_en = DW_IC_CON_RESTART_EN,
> + .slave_disable = DW_IC_CON_SLAVE_DISABLE,
> + .stop_det_ifaddressed = DW_IC_CON_STOP_DET_IFADDRESSED,
> + .tx_empty_ctrl = DW_IC_CON_TX_EMPTY_CTRL,
> + .rx_fifo_full_hld_ctrl = DW_IC_CON_RX_FIFO_FULL_HLD_CTRL,
> + .bus_clear_ctrl = DW_IC_CON_BUS_CLEAR_CTRL,
> +};
> +
> +/* "snps,designware-i2c": dedicated read-to-clear register ID per logical interrupt */
> +static const u32 dw_i2c_intr_clr[DW_INTR_IDX_MAX] = {
> + [DW_INTR_IDX_ALL] = DW_REG_IDX_CLR_INTR,
> + [DW_INTR_IDX_RX_UNDER] = DW_REG_IDX_CLR_RX_UNDER,
> + [DW_INTR_IDX_RX_OVER] = DW_REG_IDX_CLR_RX_OVER,
> + [DW_INTR_IDX_TX_OVER] = DW_REG_IDX_CLR_TX_OVER,
> + [DW_INTR_IDX_RD_REQ] = DW_REG_IDX_CLR_RD_REQ,
> + [DW_INTR_IDX_TX_ABRT] = DW_REG_IDX_CLR_TX_ABRT,
> + [DW_INTR_IDX_RX_DONE] = DW_REG_IDX_CLR_RX_DONE,
> + [DW_INTR_IDX_ACTIVITY] = DW_REG_IDX_CLR_ACTIVITY,
> + [DW_INTR_IDX_STOP_DET] = DW_REG_IDX_CLR_STOP_DET,
> + [DW_INTR_IDX_START_DET] = DW_REG_IDX_CLR_START_DET,
> + [DW_INTR_IDX_GEN_CALL] = DW_REG_IDX_CLR_GEN_CALL,
> +};
> +
> +/**
> + * i2c_dw_select_variant() - Pick the register offset table, CON-register bit
> + * layout and interrupt-ack mapping matching this device's IP variant
> + * @dev: device private data
> + *
> + * Must be called after dev->flags has been populated from
> + * device_get_match_data()/ACPI id data, and before any register access
> + * (including i2c_dw_init_regmap()).
> + */
> +void i2c_dw_select_variant(struct dw_i2c_dev *dev)
> +{
> + dev->regs = dw_i2c_reg_offsets;
> + dev->con_bits = &dw_i2c_con_bits;
> + dev->intr_clr = dw_i2c_intr_clr;
> +}
> +EXPORT_SYMBOL_GPL(i2c_dw_select_variant);
> +
> static int dw_reg_read(void *context, unsigned int reg, unsigned int *val)
> {
> struct dw_i2c_dev *dev = context;
> @@ -147,7 +236,7 @@ static int i2c_dw_init_regmap(struct dw_i2c_dev *dev)
> .disable_locking = true,
> .reg_read = dw_reg_read,
> .reg_write = dw_reg_write,
> - .max_register = DW_IC_COMP_TYPE,
> + .max_register = dev->regs[DW_REG_IDX_COMP_TYPE],
> };
> u32 reg;
> int ret;
> @@ -163,7 +252,7 @@ static int i2c_dw_init_regmap(struct dw_i2c_dev *dev)
> if (ret)
> return ret;
>
> - reg = readl(dev->base + DW_IC_COMP_TYPE);
> + reg = readl(dev->base + dev->regs[DW_REG_IDX_COMP_TYPE]);
> i2c_dw_release_lock(dev);
>
> if ((dev->flags & MODEL_MASK) == MODEL_AMD_NAVI_GPU)
> @@ -365,17 +454,17 @@ static void i2c_dw_configure_mode(struct dw_i2c_dev *dev, int mode)
> {
> switch (mode) {
> case DW_IC_MASTER:
> - regmap_write(dev->map, DW_IC_TX_TL, dev->tx_fifo_depth / 2);
> - regmap_write(dev->map, DW_IC_RX_TL, 0);
> - regmap_write(dev->map, DW_IC_CON, dev->master_cfg);
> + regmap_write(dev->map, dev->regs[DW_REG_IDX_TX_TL], dev->tx_fifo_depth / 2);
> + regmap_write(dev->map, dev->regs[DW_REG_IDX_RX_TL], 0);
> + regmap_write(dev->map, dev->regs[DW_REG_IDX_CON], dev->master_cfg);

Instead of all this. Can't you do this inside the regmap so that here and
elsewhere in the driver we continue to do:

regmap_write(dev->map, DW_IC_RX_TL, 0);

but internally, depending on the hardware it then maps this into the
corresponding register offset.