Re: [PATCH 2/3] i2c: mux: pca9541: namespace cleanup

From: Peter Rosin <peda@axentia.se>
To: Vladimir Zapolskiy <vz@mleia.com>, linux-kernel@vger.kernel.org
Cc: Guenter Roeck <linux@roeck-us.net>,
	Wolfram Sang <wsa@the-dreams.de>,
	Ken Chen <chen.kenyy@inventec.com>,
	joel@jms.id.au, linux-i2c@vger.kernel.org
Subject: Re: [PATCH 2/3] i2c: mux: pca9541: namespace cleanup
Date: Wed, 21 Mar 2018 08:35:16 +0100	[thread overview]
Message-ID: <9e7acbcc-f9e2-581d-d02d-3c026e52b3ed@axentia.se> (raw)
In-Reply-To: <93631639-eefd-cd1c-48de-451662ce8de7@mleia.com>

On 2018-03-21 07:54, Vladimir Zapolskiy wrote:
> Hi Peter,
> 
> On 03/21/2018 07:53 AM, Peter Rosin wrote:
>> On 2018-03-21 00:24, Vladimir Zapolskiy wrote:
>>> Hi Peter,
>>>
>>> On 03/20/2018 11:31 AM, Peter Rosin wrote:
>>>> In preparation for PCA9641 support, convert the mybus and busoff macros
>>>> to functions, and in the process prefix them with pca9541_. Also prefix
>>>> remaining chip specific macros with PCA9541_.
>>>>
>>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>>> ---
>>>>  drivers/i2c/muxes/i2c-mux-pca9541.c | 26 +++++++++++++++++++-------
>>>>  1 file changed, 19 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c
>>>> index ad168125d23d..47685eb4e0e9 100644
>>>> --- a/drivers/i2c/muxes/i2c-mux-pca9541.c
>>>> +++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
>>>> @@ -59,10 +59,8 @@
>>>>  #define PCA9541_ISTAT_MYTEST	BIT(6)
>>>>  #define PCA9541_ISTAT_NMYTEST	BIT(7)
>>>>  
>>>> -#define BUSON		(PCA9541_CTL_BUSON | PCA9541_CTL_NBUSON)
>>>> -#define MYBUS		(PCA9541_CTL_MYBUS | PCA9541_CTL_NMYBUS)
>>>> -#define mybus(x)	(!((x) & MYBUS) || ((x) & MYBUS) == MYBUS)
>>>> -#define busoff(x)	(!((x) & BUSON) || ((x) & BUSON) == BUSON)
>>>> +#define PCA9541_BUSON	(PCA9541_CTL_BUSON | PCA9541_CTL_NBUSON)
>>>> +#define PCA9541_MYBUS	(PCA9541_CTL_MYBUS | PCA9541_CTL_NMYBUS)
>>>>  
>>>>  /* arbitration timeouts, in jiffies */
>>>>  #define ARB_TIMEOUT	(HZ / 8)	/* 125 ms until forcing bus ownership */
>>>> @@ -93,6 +91,20 @@ static const struct of_device_id pca9541_of_match[] = {
>>>>  MODULE_DEVICE_TABLE(of, pca9541_of_match);
>>>>  #endif
>>>>  
>>>> +static int pca9541_mybus(int ctl)
>>>
>>> static inline?
>>
>> No, "inline" is only used in header files in the kernel. 
> 
> No, it is an incorrect statement, you should be aware of that.

Yeah, that was sloppy wording on my part. Let's say I meant useful
instead of used. My point is that inline is quite useless (in a C
file), the compiler will do its thing anyway. Rhetorical question:
what is the point of having both noinline and __always_inline?
Because plain old inline is overridden by the compiler, just like
the register keyword.

>> The compiler is free to inline whatever function it likes anyway, and
>> in this case we do not know better than the compiler. We don't care
> 
> That's a candidate case, when we could know better than the compiler.

Could we? Maybe for specific compilers and architectures, but
probably not for all cases. And the future is in the cards etc. And
we don't actually know even for current compilers. Also, quoting
Documentation/process/4.Coding

	More recent compilers take an increasingly active role in deciding
	whether a given function should actually be inlined or not.  So the
	liberal placement of "inline" keywords may not just be excessive; it
	could also be irrelevant.

> But "don't care" argument is still valid :)

Yes :-)

Cheers,
Peter