Skip to contentSkip to conversation

i2c: gxp: fix build failure without CONFIG_I2C_SLAVE

PATCH Applied Arnd Bergmann 4 participants 5 messages Apr 3, 2023 – May 3, 2023

Subsystems (3)
I2C subsystem host drivers I2C Arm/hpe gxp architecture
lore
Share & tools
lore

Arnd Bergmann

author
#0 lore raw

PATCH 1/1 i2c: gxp: fix build failure without CONFIG_I2C_SLAVE

From: Arnd Bergmann <[email protected]>

The gxp_i2c_slave_irq_handler() is hidden in an #ifdef, but the caller uses an IS_ENABLED() check:

drivers/i2c/busses/i2c-gxp.c: In function 'gxp_i2c_irq_handler':
drivers/i2c/busses/i2c-gxp.c:467:29: error: implicit declaration of function 'gxp_i2c_slave_irq_handler'; did you mean 'gxp_i2c_irq_handler'? [-Werror=implicit-function-declaration]

It has to consistently use one method or the other to avoid warnings, so move to IS_ENABLED() here for readability and build coverage, and move the #ifdef in linux/i2c.h to allow building it as dead code.

Fixes 4a55ed6f89f5 ("i2c: Add GXP SoC I2C Controller")
Signed-off-by Arnd Bergmann <[email protected]>
---
 drivers/i2c/busses/i2c-gxp.c | 2 --
 include/linux/i2c.h          | 4 ++--
 2 files changed, 2 insertions(+), 4 deletions(-)
drivers/i2c/busses/i2c-gxp.c +0 -2
index d4b55d989a26..8ea3fb5e4c7f 100644
@@ -353,7 +353,6 @@ static void gxp_i2c_chk_data_ack(struct gxp_i2c_drvdata *drvdata)
 	writew(value, drvdata->base + GXP_I2CMCMD);
 }
 
-#if IS_ENABLED(CONFIG_I2C_SLAVE)
 static bool gxp_i2c_slave_irq_handler(struct gxp_i2c_drvdata *drvdata)
 {
 	u8 value;

@@ -437,7 +436,6 @@ static bool gxp_i2c_slave_irq_handler(struct gxp_i2c_drvdata *drvdata)
 
 	return true;
 }
-#endif
 
 static irqreturn_t gxp_i2c_irq_handler(int irq, void *_drvdata)
 {
include/linux/i2c.h +2 -2
index 5ba89663ea86..13a1ce38cb0c 100644
@@ -385,7 +385,6 @@ static inline void i2c_set_clientdata(struct i2c_client *client, void *data)
 
 /* I2C slave support */
 
-#if IS_ENABLED(CONFIG_I2C_SLAVE)
 enum i2c_slave_event {
 	I2C_SLAVE_READ_REQUESTED,
 	I2C_SLAVE_WRITE_REQUESTED,

@@ -396,9 +395,10 @@ enum i2c_slave_event {
 
 int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb);
 int i2c_slave_unregister(struct i2c_client *client);
-bool i2c_detect_slave_mode(struct device *dev);
 int i2c_slave_event(struct i2c_client *client,
 		    enum i2c_slave_event event, u8 *val);
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+bool i2c_detect_slave_mode(struct device *dev);
 #else
 static inline bool i2c_detect_slave_mode(struct device *dev) { return false; }
 #endif
Signature · 1 line
2.39.2
maintainer Reviewed-by on v1 1/1
#1 lore raw
Quoted 9 lines

From: Arnd Bergmann <[email protected]>

The gxp_i2c_slave_irq_handler() is hidden in an #ifdef, but the caller uses an IS_ENABLED() check:

drivers/i2c/busses/i2c-gxp.c: In function 'gxp_i2c_irq_handler':
drivers/i2c/busses/i2c-gxp.c:467:29: error: implicit declaration of function 'gxp_i2c_slave_irq_handler'; did you mean 'gxp_i2c_irq_handler'? [-Werror=implicit-function-declaration]

Quoted 6 lines

It has to consistently use one method or the other to avoid warnings, so move to IS_ENABLED() here for readability and build coverage, and move the #ifdef in linux/i2c.h to allow building it as dead code.

Fixes 4a55ed6f89f5 ("i2c: Add GXP SoC I2C Controller")
Signed-off-by Arnd Bergmann <[email protected]>

Thanks for finding and correcting this!

Reviewed-by Nick Hawkins <[email protected]>
#2 lore raw
In reply to Arnd Bergmann

Hi Arnd,

Quoted 9 lines

The gxp_i2c_slave_irq_handler() is hidden in an #ifdef, but the caller uses an IS_ENABLED() check:

drivers/i2c/busses/i2c-gxp.c: In function 'gxp_i2c_irq_handler':
drivers/i2c/busses/i2c-gxp.c:467:29: error: implicit declaration of function 'gxp_i2c_slave_irq_handler'; did you mean 'gxp_i2c_irq_handler'? [-Werror=implicit-function-declaration]

It has to consistently use one method or the other to avoid warnings, so move to IS_ENABLED() here for readability and build coverage, and move the #ifdef in linux/i2c.h to allow building it as dead code.

Can't we have a solution which modifies this driver only (maybe by defining an empty irq handler for the non-IS_ENABLED part?)? Doesn't feel good to touch i2c.h only because of this...

include/linux/i2c.h enum i2c_slave_event { · line 397

Quoted 8 lines
include/linux/i2c.h +0 -1
-#if IS_ENABLED(CONFIG_I2C_SLAVE)
 enum i2c_slave_event {
 	I2C_SLAVE_READ_REQUESTED,
 	I2C_SLAVE_WRITE_REQUESTED,

@@ -396,9 +395,10 @@ enum i2c_slave_event {
 
 int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb);
 int i2c_slave_unregister(struct i2c_client *client);

... especially with moving these two prototypes out of the protected block. The functions themselves are also protected by the same symbol via the Makefile. I'd rather get a build error right away than a linker error later if a driver misses to select I2C_SLAVE. Or do I miss something?

include/linux/i2c.h enum i2c_slave_event { · line 404

Quoted 8 lines
include/linux/i2c.h +2 -1
-bool i2c_detect_slave_mode(struct device *dev);
 int i2c_slave_event(struct i2c_client *client,
 		    enum i2c_slave_event event, u8 *val);
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+bool i2c_detect_slave_mode(struct device *dev);
 #else
 static inline bool i2c_detect_slave_mode(struct device *dev) { return false; }
 #endif

All the best,

Wolfram

#3 lore raw
Quoted 6 lines · Wolfram Sang
Quoted 6 lines

The gxp_i2c_slave_irq_handler() is hidden in an #ifdef, but the caller uses an IS_ENABLED() check:

drivers/i2c/busses/i2c-gxp.c: In function 'gxp_i2c_irq_handler':
drivers/i2c/busses/i2c-gxp.c:467:29: error: implicit declaration of function 'gxp_i2c_slave_irq_handler'; did you mean 'gxp_i2c_irq_handler'? [-Werror=implicit-function-declaration]

Quoted 7 lines
Quoted 3 lines

It has to consistently use one method or the other to avoid warnings, so move to IS_ENABLED() here for readability and build coverage, and move the #ifdef in linux/i2c.h to allow building it as dead code.

Can't we have a solution which modifies this driver only (maybe by defining an empty irq handler for the non-IS_ENABLED part?)? Doesn't feel good to touch i2c.h only because of this...

The idea was to avoid the problem for the next driver as well. At the moment, there are #ifdef checks like this one in three more drivers, and I suspect we could clean them all up the same way.

Quoted 6 lines
Quoted 6 lines
include/linux/i2c.h +0 -1
-#if IS_ENABLED(CONFIG_I2C_SLAVE)
 enum i2c_slave_event {
 	I2C_SLAVE_READ_REQUESTED,
 	I2C_SLAVE_WRITE_REQUESTED,

@@ -396,9 +395,10 @@ enum i2c_slave_event {
Quoted 8 lines

include/linux/i2c.h enum i2c_slave_event { · line 397

Quoted 2 lines
include/linux/i2c.h +0 -0
 int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb);
 int i2c_slave_unregister(struct i2c_client *client);

... especially with moving these two prototypes out of the protected block. The functions themselves are also protected by the same symbol via the Makefile. I'd rather get a build error right away than a linker error later if a driver misses to select I2C_SLAVE. Or do I miss something?

I usually prefer having greater build coverage by allowing symbols to be referenced by dead code that gets eliminated during the compile stage. It helps find issues in the unused code paths at compile time, and tends to be easier to read. than a group of #ifdef checks.

The only time I would put a declaration in an #ifdef is when there is an #else path with an empty inline function.

Arnd

Applied
#4 lore raw
In reply to Arnd Bergmann
Quoted 8 lines · Arnd Bergmann

From: Arnd Bergmann <[email protected]>

The gxp_i2c_slave_irq_handler() is hidden in an #ifdef, but the caller uses an IS_ENABLED() check:

drivers/i2c/busses/i2c-gxp.c: In function 'gxp_i2c_irq_handler':
drivers/i2c/busses/i2c-gxp.c:467:29: error: implicit declaration of function 'gxp_i2c_slave_irq_handler'; did you mean 'gxp_i2c_irq_handler'? [-Werror=implicit-function-declaration]

Quoted 6 lines

It has to consistently use one method or the other to avoid warnings, so move to IS_ENABLED() here for readability and build coverage, and move the #ifdef in linux/i2c.h to allow building it as dead code.

Fixes 4a55ed6f89f5 ("i2c: Add GXP SoC I2C Controller")
Signed-off-by Arnd Bergmann <[email protected]>

Applied to for-current, thanks!