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

Message ID 20180320093200.19179-3-peda@axentia.se
State New
Delegated to: Peter Rosin
Headers show
Series
  • [1/3] i2c: mux: pca9541: use the BIT macro
Related show

Commit Message

Peter Rosin March 20, 2018, 9:31 a.m.
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(-)

Comments

Guenter Roeck March 20, 2018, 1:20 p.m. | #1
On 03/20/2018 02: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)

bool ?

> +{
> +	if (!(ctl & PCA9541_MYBUS))
> +		return 1;

true ?

> +	return (ctl & PCA9541_MYBUS) == PCA9541_MYBUS;
> +}
> +
> +static int pca9541_busoff(int ctl)

bool ?

> +{
> +	if (!(ctl & PCA9541_BUSON))
> +		return 1;

true ?

> +	return (ctl & PCA9541_BUSON) == PCA9541_BUSON;
> +}
> +
>   /*
>    * Write to chip register. Don't use i2c_transfer()/i2c_smbus_xfer()
>    * as they will try to lock the adapter a second time.
> @@ -181,7 +193,7 @@ static void pca9541_release_bus(struct i2c_client *client)
>   	int reg;
>   
>   	reg = pca9541_reg_read(client, PCA9541_CONTROL);
> -	if (reg >= 0 && !busoff(reg) && mybus(reg))
> +	if (reg >= 0 && !pca9541_busoff(reg) && pca9541_mybus(reg))
>   		pca9541_reg_write(client, PCA9541_CONTROL,
>   				  (reg & PCA9541_CTL_NBUSON) >> 1);
>   }
> @@ -233,7 +245,7 @@ static int pca9541_arbitrate(struct i2c_client *client)
>   	if (reg < 0)
>   		return reg;
>   
> -	if (busoff(reg)) {
> +	if (pca9541_busoff(reg)) {
>   		int istat;
>   		/*
>   		 * Bus is off. Request ownership or turn it on unless
> @@ -258,7 +270,7 @@ static int pca9541_arbitrate(struct i2c_client *client)
>   			 */
>   			data->select_timeout = SELECT_DELAY_LONG * 2;
>   		}
> -	} else if (mybus(reg)) {
> +	} else if (pca9541_mybus(reg)) {
>   		/*
>   		 * Bus is on, and we own it. We are done with acquisition.
>   		 * Reset NTESTON and BUSINIT, then return success.
>
Peter Rosin March 20, 2018, 9:48 p.m. | #2
On 2018-03-20 14:20, Guenter Roeck wrote:
> On 03/20/2018 02:31 AM, Peter Rosin wrote:
>> +static int pca9541_mybus(int ctl)
> 
> bool ?
> 
>> +{
>> +	if (!(ctl & PCA9541_MYBUS))
>> +		return 1;
> 
> true ?
> 
>> +	return (ctl & PCA9541_MYBUS) == PCA9541_MYBUS;
>> +}
>> +
>> +static int pca9541_busoff(int ctl)
> 
> bool ?
> 
>> +{
>> +	if (!(ctl & PCA9541_BUSON))
>> +		return 1;
> 
> true ?

bool? true? Isn't that C++? Sigh, but you're right, and old habits
die hard...

Would it be ok to add your reviewed-by tag as I fix that?

Cheers,
Peter
Guenter Roeck March 20, 2018, 9:57 p.m. | #3
On Tue, Mar 20, 2018 at 10:48:40PM +0100, Peter Rosin wrote:
> On 2018-03-20 14:20, Guenter Roeck wrote:
> > On 03/20/2018 02:31 AM, Peter Rosin wrote:
> >> +static int pca9541_mybus(int ctl)
> > 
> > bool ?
> > 
> >> +{
> >> +	if (!(ctl & PCA9541_MYBUS))
> >> +		return 1;
> > 
> > true ?
> > 
> >> +	return (ctl & PCA9541_MYBUS) == PCA9541_MYBUS;
> >> +}
> >> +
> >> +static int pca9541_busoff(int ctl)
> > 
> > bool ?
> > 
> >> +{
> >> +	if (!(ctl & PCA9541_BUSON))
> >> +		return 1;
> > 
> > true ?
> 
> bool? true? Isn't that C++? Sigh, but you're right, and old habits
> die hard...
> 
> Would it be ok to add your reviewed-by tag as I fix that?
> 
Sure.

Guenter

> Cheers,
> Peter
Vladimir Zapolskiy March 20, 2018, 11:24 p.m. | #4
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?

> +{
> +	if (!(ctl & PCA9541_MYBUS))
> +		return 1;
> +	return (ctl & PCA9541_MYBUS) == PCA9541_MYBUS;
> +}
> +
> +static int pca9541_busoff(int ctl)

static inline?

> +{
> +	if (!(ctl & PCA9541_BUSON))
> +		return 1;
> +	return (ctl & PCA9541_BUSON) == PCA9541_BUSON;
> +}

Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>

--
With best wishes,
Vladimir
Peter Rosin March 21, 2018, 5:53 a.m. | #5
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. 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 either. At least,
that is my understanding of the situation regarding the "inline"
keyword.

> 
>> +{
>> +	if (!(ctl & PCA9541_MYBUS))
>> +		return 1;
>> +	return (ctl & PCA9541_MYBUS) == PCA9541_MYBUS;
>> +}
>> +
>> +static int pca9541_busoff(int ctl)
> 
> static inline?
> 
>> +{
>> +	if (!(ctl & PCA9541_BUSON))
>> +		return 1;
>> +	return (ctl & PCA9541_BUSON) == PCA9541_BUSON;
>> +}
> 
> Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>

Thanks!

Cheers,
Peter

> 
> --
> With best wishes,
> Vladimir
>
Vladimir Zapolskiy March 21, 2018, 6:54 a.m. | #6
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.

> 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.

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

--
With best wishes,
Vladimir
Peter Rosin March 21, 2018, 7:35 a.m. | #7
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

Patch

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)
+{
+	if (!(ctl & PCA9541_MYBUS))
+		return 1;
+	return (ctl & PCA9541_MYBUS) == PCA9541_MYBUS;
+}
+
+static int pca9541_busoff(int ctl)
+{
+	if (!(ctl & PCA9541_BUSON))
+		return 1;
+	return (ctl & PCA9541_BUSON) == PCA9541_BUSON;
+}
+
 /*
  * Write to chip register. Don't use i2c_transfer()/i2c_smbus_xfer()
  * as they will try to lock the adapter a second time.
@@ -181,7 +193,7 @@  static void pca9541_release_bus(struct i2c_client *client)
 	int reg;
 
 	reg = pca9541_reg_read(client, PCA9541_CONTROL);
-	if (reg >= 0 && !busoff(reg) && mybus(reg))
+	if (reg >= 0 && !pca9541_busoff(reg) && pca9541_mybus(reg))
 		pca9541_reg_write(client, PCA9541_CONTROL,
 				  (reg & PCA9541_CTL_NBUSON) >> 1);
 }
@@ -233,7 +245,7 @@  static int pca9541_arbitrate(struct i2c_client *client)
 	if (reg < 0)
 		return reg;
 
-	if (busoff(reg)) {
+	if (pca9541_busoff(reg)) {
 		int istat;
 		/*
 		 * Bus is off. Request ownership or turn it on unless
@@ -258,7 +270,7 @@  static int pca9541_arbitrate(struct i2c_client *client)
 			 */
 			data->select_timeout = SELECT_DELAY_LONG * 2;
 		}
-	} else if (mybus(reg)) {
+	} else if (pca9541_mybus(reg)) {
 		/*
 		 * Bus is on, and we own it. We are done with acquisition.
 		 * Reset NTESTON and BUSINIT, then return success.