| Message ID | 20180320093200.19179-3-peda@axentia.se |
|---|---|
| State | Superseded |
| Delegated to: | Peter Rosin |
| Headers | show |
| Series | [1/3] i2c: mux: pca9541: use the BIT macro | expand |
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. >
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
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
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
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 >
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
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
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.
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(-)