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

[Aniket Limaye <a-limaye@ti.com>]

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 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.

Yeah that's definitely cleaner... will do in v2.

Thanks,
Aniket