Re: [PATCH v3 0/3] i2c: designware: Add TI TDA54 I2C support

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Fri, Sep 25, 2026 at 04:46:39PM +0530, Aniket Limaye wrote:
> On 25/09/26 15:40, Andy Shevchenko wrote:
> > On Fri, Sep 25, 2026 at 03:29:27PM +0530, Aniket Limaye wrote:
> > > On 25/09/26 15:11, Andy Shevchenko wrote:
> > > > On Fri, Sep 25, 2026 at 12:26:27PM +0530, Aniket Limaye wrote:

...

> > > > Still doesn't look good. The current register layout may be left as is. What
> > > > you need is translate it in the respective regmap callbacks in case we are
> > > > enumerated on the different IP. Also possible to have a different regmap
> > > > config for the different HW where you translate them only in one place.
> > > Is it confusing to keep using existing offsets in regmap_read/write() call
> > > sites for TDA54, and let the regmap silently handle the translation?
> > > 
> > > Given that we do *not* have any new registers in use on the tda54 version
> > > that were not there in the original one, I guess it works... will send a v4
> > > as per your suggestion.
> > Depends on the mapping. Your series also forgot to provide the differences
> > Check this as an example: e539f435cb9c ("spi: dw: Add support for DesignWare DWC_ssi").
> > 
> Ahh sorry about that, will remember to add a clean mapping in cover letter
> of next version.
> 
> For now here are the structural differences:
> 
> 1.  Reg offsets  Existing        New TDA54
> 
>     [DW_IC_CON]                     0x00           0x2c
>     [DW_IC_TAR]                     0x04            0x30
>     [DW_IC_SAR]                     0x08            0x34
>     [DW_IC_DATA_CMD]                0x10            0x80
>     [DW_IC_SS_SCL_HCNT]             0x14            0x4c
>     [DW_IC_SS_SCL_LCNT]             0x18            0x50
>     [DW_IC_FS_SCL_HCNT]             0x1c            0x4c  /* same as SS */
>     [DW_IC_FS_SCL_LCNT]             0x20            0x50  /* same as SS */
>     [DW_IC_HS_SCL_HCNT]             0x24         0x54
>     [DW_IC_HS_SCL_LCNT]             0x28            0x58
>     [DW_IC_INTR_STAT]               0x2c            0xc0
>     [DW_IC_INTR_MASK]               0x30            0xc4
>     [DW_IC_RAW_INTR_STAT]           0x34            0xc8
>     [DW_IC_RX_TL]                   0x38            0x84
>     [DW_IC_TX_TL]                   0x3c            0x88
>     [DW_IC_CLR_INTR]                0x40            0xcc
>     [DW_IC_CLR_RX_UNDER]            0x44            NA
>     [DW_IC_CLR_RX_OVER]             0x48            NA
>     [DW_IC_CLR_TX_OVER]             0x4c            NA
>     [DW_IC_CLR_RD_REQ]              0x50            NA
>     [DW_IC_CLR_TX_ABRT]             0x54            NA
>     [DW_IC_CLR_RX_DONE]             0x58            NA
>     [DW_IC_CLR_ACTIVITY]            0x5c            NA
>     [DW_IC_CLR_STOP_DET]            0x60            NA
>     [DW_IC_CLR_START_DET]           0x64            NA
>     [DW_IC_CLR_GEN_CALL]            0x68            NA
>     [DW_IC_ENABLE]                  0x6c            0x04
>     [DW_IC_STATUS]                  0x70            0xd8
>     [DW_IC_TXFLR]                   0x74            0xdc
>     [DW_IC_RXFLR]                   0x78            0xe0
>     [DW_IC_SDA_HOLD]                0x7c            0x5c
>     [DW_IC_TX_ABRT_SOURCE]          0x80            0xd4
>     [DW_IC_ENABLE_STATUS]           0x9c            0xd0
>     [DW_IC_CLR_RESTART_DET]         0xa8            NA
>     [DW_IC_SMBUS_INTR_STAT]         0xc8            NA
>     [DW_IC_SMBUS_INTR_MASK]         0xcc            NA
>     [DW_IC_CLR_SMBUS_INTR]          0xd4            NA
>     [DW_IC_COMP_PARAM_1]            0xf4            NA
>     [DW_IC_COMP_VERSION]            0xf8            0x100
>     [DW_IC_COMP_TYPE]               0xfc            0x104
> 
> 2. DW_IC_CON bitfields:
> 
>     DW_IC_CON_MASTER BIT(0)                  BIT(0)
>     DW_IC_CON_SPEED_STD                 (1 << 1)       (1 << 4)
>     DW_IC_CON_SPEED_FAST                (2 << 1)       (2 << 4)
>     DW_IC_CON_SPEED_HIGH                (3 << 1)       (3 << 4)
>     DW_IC_CON_SPEED_MASK                GENMASK(2, 1)  GENMASK(5, 4)
>     DW_IC_CON_10BITADDR_SLAVE           BIT(3) BIT(8)
>     DW_IC_CON_10BITADDR_MASTER          BIT(4) BIT(9)
>     DW_IC_CON_RESTART_EN                BIT(5) NA
>     DW_IC_CON_SLAVE_DISABLE             BIT(6) NA
>     DW_IC_CON_STOP_DET_IFADDRESSED      BIT(7) BIT(10)
>     DW_IC_CON_TX_EMPTY_CTRL             BIT(8) BIT(11)
>     DW_IC_CON_RX_FIFO_FULL_HLD_CTRL     BIT(9) BIT(12)
>     DW_IC_CON_BUS_CLEAR_CTRL            BIT(11)  BIT(14)

Thanks for providing this mapping!

> 3. To clear INTR,  Read DW_IC_CLR_* reg    Write bit to DW_IC_CLR_INTR
> 
> As you can see, it's an entirely different mapping, which is why I had 2
> independent enum -> reg offset maps instead of offset -> offset translation.
> Similarly, selecting a CON register bitfield layout too.
> 
> Let me know what you would prefer based on this...

This clears a bit the whole picture and what I would like to say is that better
to have the separate driver for it. On top of completely reworked RTL I believe
you won't need tons of hacks and workarounds that are applied during all these
years against the old IP.

TL;DR: Make a new clean and simple driver, which is hack-less and done properly
(using the all modern APIs and frameworks in the kernel).

-- 
With Best Regards,
Andy Shevchenko