* [PATCH v2] i2c: designware: Replace magic numbers with named constants
@ 2025-11-06 16:02 Artem Shimko
2025-11-07 6:54 ` kernel test robot
0 siblings, 1 reply; 3+ messages in thread
From: Artem Shimko @ 2025-11-06 16:02 UTC (permalink / raw)
To: Mika Westerberg, Andy Shevchenko, Jan Dabros, Andi Shyti
Cc: Artem Shimko, linux-i2c, linux-kernel
Replace various magic numbers with properly named constants to improve
code readability and maintainability. This includes constants for
register access, timing adjustments, timeouts, FIFO parameters,
and default values.
The change makes the code more self-documenting without altering any
functionality.
Signed-off-by: Artem Shimko <a.shimko.dev@gmail.com>
---
Hello maintainers and reviewers,
Fix replaces magic numbers throughout the DesignWare I2C driver with named
constants to improve code readability and maintainability.
The change introduces constants for register access, timing adjustments,
timeouts, FIFO parameters, and default values, all properly documented
with comments.
No functional changes.
Thank you for your consideration.
--
Best regards,
Artem Shimko
ChangeLog:
v1:
* https://lore.kernel.org/all/20251105161845.2535367-1-a.shimko.dev@gmail.com/T/#u
v2:
* Move register-related constants to i2c-designware-core.h
* Remove unnecessary comments to reduce clutter
* Keep only essential timeouts and default parameters in .c file
* Use FIELD_GET() for FIFO depth extraction as suggested
drivers/i2c/busses/i2c-designware-common.c | 32 ++++++++++++++--------
drivers/i2c/busses/i2c-designware-core.h | 13 +++++++++
2 files changed, 34 insertions(+), 11 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index 5b1e8f74c4ac..1aa10e712712 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -34,6 +34,14 @@
#include "i2c-designware-core.h"
+#define DW_IC_DEFAULT_BUS_CAPACITANCE_PF 100
+
+#define DW_IC_ABORT_TIMEOUT_US 10
+#define DW_IC_ABORT_TOTAL_TIMEOUT_US 100
+
+#define DW_IC_BUSY_POLL_TIMEOUT_US 1100
+#define DW_IC_BUSY_TOTAL_TIMEOUT_US 20000
+
static const char *const abort_sources[] = {
[ABRT_7B_ADDR_NOACK] =
"slave address not acknowledged (7bit mode)",
@@ -106,7 +114,7 @@ static int dw_reg_read_word(void *context, unsigned int reg, unsigned int *val)
struct dw_i2c_dev *dev = context;
*val = readw(dev->base + reg) |
- (readw(dev->base + reg + 2) << 16);
+ (readw(dev->base + reg + DW_IC_REG_STEP_BYTES) << DW_IC_REG_WORD_SHIFT);
return 0;
}
@@ -116,7 +124,7 @@ static int dw_reg_write_word(void *context, unsigned int reg, unsigned int val)
struct dw_i2c_dev *dev = context;
writew(val, dev->base + reg);
- writew(val >> 16, dev->base + reg + 2);
+ writew(val >> DW_IC_REG_WORD_SHIFT, dev->base + reg + DW_IC_REG_STEP_BYTES);
return 0;
}
@@ -165,7 +173,7 @@ int i2c_dw_init_regmap(struct dw_i2c_dev *dev)
if (reg == swab32(DW_IC_COMP_TYPE_VALUE)) {
map_cfg.reg_read = dw_reg_read_swab;
map_cfg.reg_write = dw_reg_write_swab;
- } else if (reg == (DW_IC_COMP_TYPE_VALUE & 0x0000ffff)) {
+ } else if (reg == lower_16_bits(DW_IC_COMP_TYPE_VALUE)) {
map_cfg.reg_read = dw_reg_read_word;
map_cfg.reg_write = dw_reg_write_word;
} else if (reg != DW_IC_COMP_TYPE_VALUE) {
@@ -384,7 +392,7 @@ int i2c_dw_fw_parse_and_configure(struct dw_i2c_dev *dev)
i2c_parse_fw_timings(device, t, false);
if (device_property_read_u32(device, "snps,bus-capacitance-pf", &dev->bus_capacitance_pF))
- dev->bus_capacitance_pF = 100;
+ dev->bus_capacitance_pF = DW_IC_DEFAULT_BUS_CAPACITANCE_PF;
dev->clk_freq_optimized = device_property_read_bool(device, "snps,clk-freq-optimized");
@@ -539,8 +547,9 @@ void __i2c_dw_disable(struct dw_i2c_dev *dev)
regmap_write(dev->map, DW_IC_ENABLE, enable | DW_IC_ENABLE_ABORT);
ret = regmap_read_poll_timeout(dev->map, DW_IC_ENABLE, enable,
- !(enable & DW_IC_ENABLE_ABORT), 10,
- 100);
+ !(enable & DW_IC_ENABLE_ABORT),
+ DW_IC_ABORT_TIMEOUT_US,
+ DW_IC_ABORT_TOTAL_TIMEOUT_US);
if (ret)
dev_err(dev->dev, "timeout while trying to abort current transfer\n");
}
@@ -552,7 +561,7 @@ void __i2c_dw_disable(struct dw_i2c_dev *dev)
* in that case this test reads zero and exits the loop.
*/
regmap_read(dev->map, DW_IC_ENABLE_STATUS, &status);
- if ((status & 1) == 0)
+ if (!(status & 1))
return;
/*
@@ -635,7 +644,8 @@ int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev)
ret = regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
!(status & DW_IC_STATUS_ACTIVITY),
- 1100, 20000);
+ DW_IC_BUSY_POLL_TIMEOUT_US,
+ DW_IC_BUSY_TOTAL_TIMEOUT_US);
if (ret) {
dev_warn(dev->dev, "timeout waiting for bus ready\n");
@@ -699,12 +709,12 @@ int i2c_dw_set_fifo_size(struct dw_i2c_dev *dev)
if (ret)
return ret;
- tx_fifo_depth = ((param >> 16) & 0xff) + 1;
- rx_fifo_depth = ((param >> 8) & 0xff) + 1;
+ tx_fifo_depth = FIELD_GET(DW_IC_FIFO_TX_FIELD, param) + 1;
+ rx_fifo_depth = FIELD_GET(DW_IC_FIFO_RX_FIELD, param) + 1;
if (!dev->tx_fifo_depth) {
dev->tx_fifo_depth = tx_fifo_depth;
dev->rx_fifo_depth = rx_fifo_depth;
- } else if (tx_fifo_depth >= 2) {
+ } else if (tx_fifo_depth >= DW_IC_FIFO_MIN_DEPTH) {
dev->tx_fifo_depth = min_t(u32, dev->tx_fifo_depth,
tx_fifo_depth);
dev->rx_fifo_depth = min_t(u32, dev->rx_fifo_depth,
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 347843b4f5dd..a699953bf5ae 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -41,6 +41,19 @@
#define DW_IC_DATA_CMD_DAT GENMASK(7, 0)
#define DW_IC_DATA_CMD_FIRST_DATA_BYTE BIT(11)
+/*
+ * Register access parameters
+ */
+#define DW_IC_REG_STEP_BYTES 2
+#define DW_IC_REG_WORD_SHIFT 16
+
+/*
+ * FIFO depth configuration
+ */
+#define DW_IC_FIFO_TX_FIELD GENMASK(23, 16)
+#define DW_IC_FIFO_RX_FIELD GENMASK(15, 8)
+#define DW_IC_FIFO_MIN_DEPTH 2
+
/*
* Registers offset
*/
--
2.43.0
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH v2] i2c: designware: Replace magic numbers with named constants
2025-11-06 16:02 [PATCH v2] i2c: designware: Replace magic numbers with named constants Artem Shimko
@ 2025-11-07 6:54 ` kernel test robot
2025-11-07 7:30 ` [PATCH v3] " Artem Shimko
0 siblings, 1 reply; 3+ messages in thread
From: kernel test robot @ 2025-11-07 6:54 UTC (permalink / raw)
To: Artem Shimko, Mika Westerberg, Andy Shevchenko, Jan Dabros,
Andi Shyti
Cc: oe-kbuild-all, Artem Shimko, linux-i2c, linux-kernel
Hi Artem,
kernel test robot noticed the following build errors:
[auto build test ERROR on andi-shyti/i2c/i2c-host]
[also build test ERROR on westeri-thunderbolt/next linus/master v6.18-rc4 next-20251107]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Artem-Shimko/i2c-designware-Replace-magic-numbers-with-named-constants/20251107-000812
base: https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
patch link: https://lore.kernel.org/r/20251106160206.2617785-1-a.shimko.dev%40gmail.com
patch subject: [PATCH v2] i2c: designware: Replace magic numbers with named constants
config: loongarch-randconfig-001-20251107 (https://download.01.org/0day-ci/archive/20251107/202511071402.qHS6LLi9-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 15.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251107/202511071402.qHS6LLi9-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202511071402.qHS6LLi9-lkp@intel.com/
All errors (new ones prefixed by >>):
drivers/i2c/busses/i2c-designware-common.c: In function 'i2c_dw_set_fifo_size':
>> drivers/i2c/busses/i2c-designware-common.c:712:25: error: implicit declaration of function 'FIELD_GET' [-Wimplicit-function-declaration]
712 | tx_fifo_depth = FIELD_GET(DW_IC_FIFO_TX_FIELD, param) + 1;
| ^~~~~~~~~
vim +/FIELD_GET +712 drivers/i2c/busses/i2c-designware-common.c
684
685 int i2c_dw_set_fifo_size(struct dw_i2c_dev *dev)
686 {
687 u32 tx_fifo_depth, rx_fifo_depth;
688 unsigned int param;
689 int ret;
690
691 /* DW_IC_COMP_PARAM_1 not implement for IP issue */
692 if ((dev->flags & MODEL_MASK) == MODEL_WANGXUN_SP) {
693 dev->tx_fifo_depth = TXGBE_TX_FIFO_DEPTH;
694 dev->rx_fifo_depth = TXGBE_RX_FIFO_DEPTH;
695
696 return 0;
697 }
698
699 /*
700 * Try to detect the FIFO depth if not set by interface driver,
701 * the depth could be from 2 to 256 from HW spec.
702 */
703 ret = i2c_dw_acquire_lock(dev);
704 if (ret)
705 return ret;
706
707 ret = regmap_read(dev->map, DW_IC_COMP_PARAM_1, ¶m);
708 i2c_dw_release_lock(dev);
709 if (ret)
710 return ret;
711
> 712 tx_fifo_depth = FIELD_GET(DW_IC_FIFO_TX_FIELD, param) + 1;
713 rx_fifo_depth = FIELD_GET(DW_IC_FIFO_RX_FIELD, param) + 1;
714 if (!dev->tx_fifo_depth) {
715 dev->tx_fifo_depth = tx_fifo_depth;
716 dev->rx_fifo_depth = rx_fifo_depth;
717 } else if (tx_fifo_depth >= DW_IC_FIFO_MIN_DEPTH) {
718 dev->tx_fifo_depth = min_t(u32, dev->tx_fifo_depth,
719 tx_fifo_depth);
720 dev->rx_fifo_depth = min_t(u32, dev->rx_fifo_depth,
721 rx_fifo_depth);
722 }
723
724 return 0;
725 }
726
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 3+ messages in thread* [PATCH v3] i2c: designware: Replace magic numbers with named constants
2025-11-07 6:54 ` kernel test robot
@ 2025-11-07 7:30 ` Artem Shimko
0 siblings, 0 replies; 3+ messages in thread
From: Artem Shimko @ 2025-11-07 7:30 UTC (permalink / raw)
To: mika.westerberg
Cc: a.shimko.dev, andi.shyti, andriy.shevchenko, jsd, linux-i2c,
linux-kernel, oe-kbuild-all, kernel test robot
Replace various magic numbers with properly named constants to improve
code readability and maintainability. This includes constants for
register access, timing adjustments, timeouts, FIFO parameters,
and default values.
The change makes the code more self-documenting without altering any
functionality.
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202511071402.qHS6LLi9-lkp@intel.com/
Signed-off-by: Artem Shimko <a.shimko.dev@gmail.com>
---
Hello
--
Best regards,
Artem Shimko
ChangeLog:
v1:
* https://lore.kernel.org/all/20251105161845.2535367-1-a.shimko.dev@gmail.com/T/#u
v2:
* Move register-related constants to i2c-designware-core.h
* Remove unnecessary comments to reduce clutter
* Keep only essential timeouts and default parameters in .c file
* Use FIELD_GET() for FIFO depth extraction as suggested
v3:
* Add missing include for linux/bitfield.h
drivers/i2c/busses/i2c-designware-common.c | 33 ++++++++++++++--------
drivers/i2c/busses/i2c-designware-core.h | 13 +++++++++
2 files changed, 35 insertions(+), 11 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index 5b1e8f74c4ac..3bc55068da03 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -12,6 +12,7 @@
#define DEFAULT_SYMBOL_NAMESPACE "I2C_DW_COMMON"
#include <linux/acpi.h>
+#include <linux/bitfield.h>
#include <linux/clk.h>
#include <linux/delay.h>
#include <linux/device.h>
@@ -34,6 +35,14 @@
#include "i2c-designware-core.h"
+#define DW_IC_DEFAULT_BUS_CAPACITANCE_PF 100
+
+#define DW_IC_ABORT_TIMEOUT_US 10
+#define DW_IC_ABORT_TOTAL_TIMEOUT_US 100
+
+#define DW_IC_BUSY_POLL_TIMEOUT_US 1100
+#define DW_IC_BUSY_TOTAL_TIMEOUT_US 20000
+
static const char *const abort_sources[] = {
[ABRT_7B_ADDR_NOACK] =
"slave address not acknowledged (7bit mode)",
@@ -106,7 +115,7 @@ static int dw_reg_read_word(void *context, unsigned int reg, unsigned int *val)
struct dw_i2c_dev *dev = context;
*val = readw(dev->base + reg) |
- (readw(dev->base + reg + 2) << 16);
+ (readw(dev->base + reg + DW_IC_REG_STEP_BYTES) << DW_IC_REG_WORD_SHIFT);
return 0;
}
@@ -116,7 +125,7 @@ static int dw_reg_write_word(void *context, unsigned int reg, unsigned int val)
struct dw_i2c_dev *dev = context;
writew(val, dev->base + reg);
- writew(val >> 16, dev->base + reg + 2);
+ writew(val >> DW_IC_REG_WORD_SHIFT, dev->base + reg + DW_IC_REG_STEP_BYTES);
return 0;
}
@@ -165,7 +174,7 @@ int i2c_dw_init_regmap(struct dw_i2c_dev *dev)
if (reg == swab32(DW_IC_COMP_TYPE_VALUE)) {
map_cfg.reg_read = dw_reg_read_swab;
map_cfg.reg_write = dw_reg_write_swab;
- } else if (reg == (DW_IC_COMP_TYPE_VALUE & 0x0000ffff)) {
+ } else if (reg == lower_16_bits(DW_IC_COMP_TYPE_VALUE)) {
map_cfg.reg_read = dw_reg_read_word;
map_cfg.reg_write = dw_reg_write_word;
} else if (reg != DW_IC_COMP_TYPE_VALUE) {
@@ -384,7 +393,7 @@ int i2c_dw_fw_parse_and_configure(struct dw_i2c_dev *dev)
i2c_parse_fw_timings(device, t, false);
if (device_property_read_u32(device, "snps,bus-capacitance-pf", &dev->bus_capacitance_pF))
- dev->bus_capacitance_pF = 100;
+ dev->bus_capacitance_pF = DW_IC_DEFAULT_BUS_CAPACITANCE_PF;
dev->clk_freq_optimized = device_property_read_bool(device, "snps,clk-freq-optimized");
@@ -539,8 +548,9 @@ void __i2c_dw_disable(struct dw_i2c_dev *dev)
regmap_write(dev->map, DW_IC_ENABLE, enable | DW_IC_ENABLE_ABORT);
ret = regmap_read_poll_timeout(dev->map, DW_IC_ENABLE, enable,
- !(enable & DW_IC_ENABLE_ABORT), 10,
- 100);
+ !(enable & DW_IC_ENABLE_ABORT),
+ DW_IC_ABORT_TIMEOUT_US,
+ DW_IC_ABORT_TOTAL_TIMEOUT_US);
if (ret)
dev_err(dev->dev, "timeout while trying to abort current transfer\n");
}
@@ -552,7 +562,7 @@ void __i2c_dw_disable(struct dw_i2c_dev *dev)
* in that case this test reads zero and exits the loop.
*/
regmap_read(dev->map, DW_IC_ENABLE_STATUS, &status);
- if ((status & 1) == 0)
+ if (!(status & 1))
return;
/*
@@ -635,7 +645,8 @@ int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev)
ret = regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
!(status & DW_IC_STATUS_ACTIVITY),
- 1100, 20000);
+ DW_IC_BUSY_POLL_TIMEOUT_US,
+ DW_IC_BUSY_TOTAL_TIMEOUT_US);
if (ret) {
dev_warn(dev->dev, "timeout waiting for bus ready\n");
@@ -699,12 +710,12 @@ int i2c_dw_set_fifo_size(struct dw_i2c_dev *dev)
if (ret)
return ret;
- tx_fifo_depth = ((param >> 16) & 0xff) + 1;
- rx_fifo_depth = ((param >> 8) & 0xff) + 1;
+ tx_fifo_depth = FIELD_GET(DW_IC_FIFO_TX_FIELD, param) + 1;
+ rx_fifo_depth = FIELD_GET(DW_IC_FIFO_RX_FIELD, param) + 1;
if (!dev->tx_fifo_depth) {
dev->tx_fifo_depth = tx_fifo_depth;
dev->rx_fifo_depth = rx_fifo_depth;
- } else if (tx_fifo_depth >= 2) {
+ } else if (tx_fifo_depth >= DW_IC_FIFO_MIN_DEPTH) {
dev->tx_fifo_depth = min_t(u32, dev->tx_fifo_depth,
tx_fifo_depth);
dev->rx_fifo_depth = min_t(u32, dev->rx_fifo_depth,
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 347843b4f5dd..a699953bf5ae 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -41,6 +41,19 @@
#define DW_IC_DATA_CMD_DAT GENMASK(7, 0)
#define DW_IC_DATA_CMD_FIRST_DATA_BYTE BIT(11)
+/*
+ * Register access parameters
+ */
+#define DW_IC_REG_STEP_BYTES 2
+#define DW_IC_REG_WORD_SHIFT 16
+
+/*
+ * FIFO depth configuration
+ */
+#define DW_IC_FIFO_TX_FIELD GENMASK(23, 16)
+#define DW_IC_FIFO_RX_FIELD GENMASK(15, 8)
+#define DW_IC_FIFO_MIN_DEPTH 2
+
/*
* Registers offset
*/
--
2.43.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-11-07 7:30 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-11-06 16:02 [PATCH v2] i2c: designware: Replace magic numbers with named constants Artem Shimko
2025-11-07 6:54 ` kernel test robot
2025-11-07 7:30 ` [PATCH v3] " Artem Shimko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).