Re: [PATCH v3 2/3] i2c: designware: Support per-variant reg/bit offsets and intr handling

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

On 25/09/26 12:39, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] i2c: designware: wangxun hardware register access broken
> - [Low] i2c: designware: dw_i2c_reg_offsets array bloat in header
> --
>
> commit 5382481b0ab458e2922590488d443f56e4cc2c8b
> Author: Aniket Limaye <a-limaye@ti.com>
>
>      i2c: designware: Support per-variant reg/bit offsets and intr handling
>
> This commit introduces support for per-variant register and bit offsets,
> and interrupt handling in the DesignWare I2C driver. It changes register
> offsets from compile-time constants to a logical enum, resolving physical
> offsets via a regmap-based mapping table to prepare for different register
> layouts.
>
> Link: https://lore.kernel.org/all/20260925-tda54-upstream-i2c-v2-2-261785e58493@ti.com/
> Link: https://lore.kernel.org/r/20260919-tda54-upstream-i2c-v1-2-b0b9f77be18b@ti.com/
>
>> diff --git a/include/linux/designware_i2c.h b/include/linux/designware_i2c.h
>> index 53f37f18a7229..994e0e5ea623f 100644
>> --- a/include/linux/designware_i2c.h
>> +++ b/include/linux/designware_i2c.h
>> @@ -11,48 +11,103 @@
>>   #include <linux/bits.h>
>>   
>>   /*
>> - * Registers offset
>> + * Logical register IDs. Each ID is resolved to a physical offset via a
>> + * per-variant table (see dw_i2c_reg_offsets[] below); regmap backends
>> + * (dw_reg_read()/dw_reg_write() and the Xe I2C_REG() macro) perform that
>> + * lookup so callers never need to know the real offset.
>>    */
>> -#define DW_IC_CON				0x00
>> -#define DW_IC_TAR				0x04
> [ ... ]
>> -#define DW_IC_COMP_TYPE				0xfc
>> +enum dw_ic_reg_id {
>> +	DW_IC_CON,
>> +	DW_IC_TAR,
> [Severity: High]
> Does changing these constants to logical enum IDs break Wangxun hardware
> register access?
>
> When the Wangxun driver's parent (txgbe) provisions an external parent regmap,
> the I2C core skips its built-in regmap initialization:
>
> drivers/i2c/busses/i2c-designware-common.c:i2c_dw_init_regmap() {
>      ...
>      /*
>       * Skip detecting the registers map configuration if the regmap has
>       * already been provided by a higher code.
>       */
>      if (dev->map)
>          return 0;
>      ...
> }
>
> Because the parent regmap hasn't been updated to expect logical register IDs,
> it will pass these new enum values directly to the hardware. For example, it
> would pass 26 for DW_IC_ENABLE instead of the physical offset 0x6c.
>
> Will this cause the driver to read and write to incorrect register offsets on
> Wangxun platforms?

This is a legitimate miss in current version, and will be handled in v4
similar to how xe_i2c.c is handled currently.

Will wait for a bit to see if there are more reviews on the approach,
before sending v4.

>
> [ ... ]
>> +	DW_IC_COMP_TYPE,
>> +	AMD_UCSI_INTR_REG,
>> +	DW_IC_REG_MAX
>> +};
>> +
>> +/*
>> + * Default "snps,designware-i2c" register offsets, indexed by
>> + * enum dw_ic_reg_id.
>> + */
>> +static const __maybe_unused u32 dw_i2c_reg_offsets[DW_IC_REG_MAX] = {
>> +	[DW_IC_CON]			= 0x00,
> [Severity: Low]
> Does defining this static const array in a header file cause binary bloat?
>
> Including linux/designware_i2c.h will embed a private copy of this 164-byte
> array into every translation unit that includes the header.
>