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(-)
diff --git a/drivers/i2c/busses/i2c-gxp.c b/drivers/i2c/busses/i2c-gxp.c
index d4b55d989a26..8ea3fb5e4c7f 100644
--- a/drivers/i2c/busses/i2c-gxp.c
+++ b/drivers/i2c/busses/i2c-gxp.c
@@ -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)
{
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 5ba89663ea86..13a1ce38cb0c 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -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
--
2.39.2
> 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]>
Thanks for finding and correcting this!
Reviewed-by: Nick Hawkins <[email protected]>
Hi Arnd,
> 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...
> -#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?
> -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
On Thu, Apr 13, 2023, at 21:27, Wolfram Sang wrote:
>> 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...
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.
>> -#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?
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
On Mon, Apr 03, 2023 at 09:49:13AM +0200, Arnd Bergmann wrote:
> 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]>
Applied to for-current, thanks!