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

[Mika Westerberg <mika.westerberg@linux.intel.com>]

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@ti.com>
> ---
>  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.