i2c: gxp: fix build failure without CONFIG_I2C_SLAVE

From: Arnd Bergmann <arnd@arndb.de>

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 <arnd@arndb.de>
---
 drivers/i2c/busses/i2c-gxp.c | 2 --
 include/linux/i2c.h          | 4 ++--
 2 files changed, 2 insertions(+), 4 deletions(-)

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 <arnd@arndb.de>
> 
> 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 <arnd@arndb.de>

Applied to for-current, thanks!