Re: [PATCH 2/3] i2c: designware: Introduce per-variant register offset and bit-layout tables
From: Aniket Limaye
Date: Mon Sep 21 2026 - 13:15:13 EST
On 21/09/26 16:41, Mika Westerberg wrote:
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 currentlyInstead of all this. Can't you do this inside the regmap so that here and
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);
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.
Yeah that's definitely cleaner... will do in v2.
Thanks,
Aniket