On 25/09/26 12:39, [email protected] 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 <[email protected]>
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/[email protected]/
Link:
https://lore.kernel.org/r/[email protected]/
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.
