Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i56a14606:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-334-g8c072af647-fm-20230330.001-g8c072af6
Mime-Version: 1.0
Message-Id: <c84eee23-1a25-4367-9588-6cfd27a4345f@app.fastmail.com>
In-Reply-To: <ZDhXtDLiTtm2iXGW@sai>
References: <20230403074939.3785593-1-arnd@kernel.org> <ZDhXtDLiTtm2iXGW@sai>
Date:   Thu, 13 Apr 2023 22:19:26 +0200
From:   "Arnd Bergmann" <arnd@arndb.de>
To:     "Wolfram Sang" <wsa@kernel.org>, "Arnd Bergmann" <arnd@kernel.org>
Cc:     "Verdun, Jean-Marie" <verdun@hpe.com>,
        "Hawkins, Nick" <nick.hawkins@hpe.com>,
        "Joel Stanley" <joel@jms.id.au>,
        =?UTF-8?Q?Uwe_Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>,
        "Andy Shevchenko" <andriy.shevchenko@linux.intel.com>,
        linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] i2c: gxp: fix build failure without CONFIG_I2C_SLAVE
Content-Type: text/plain
Precedence: bulk

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