| Message ID | 1467170438-5157-1-git-send-email-KCHSU0@nuvoton.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi On Tue, Jun 28, 2016 at 8:20 PM, John Hsu <KCHSU0@nuvoton.com> wrote: > The original design only covers the jack insertion logic is active low. > Add more condition to cover no matter the logic is active low and high. > > Signed-off-by: John Hsu <KCHSU0@nuvoton.com> > --- > sound/soc/codecs/nau8825.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/sound/soc/codecs/nau8825.c b/sound/soc/codecs/nau8825.c > index 3f30e6e..a2f0d03 100644 > --- a/sound/soc/codecs/nau8825.c > +++ b/sound/soc/codecs/nau8825.c > @@ -1345,10 +1345,18 @@ EXPORT_SYMBOL_GPL(nau8825_enable_jack_detect); > > static bool nau8825_is_jack_inserted(struct regmap *regmap) > { > - int status; > + int status, jkdet, res; > > regmap_read(regmap, NAU8825_REG_I2C_DEVICE_ID, &status); > - return !(status & NAU8825_GPIO2JD1); > + regmap_read(regmap, NAU8825_REG_JACK_DET_CTRL, &jkdet); > + > + /* return jack connection status according to jack insertion logic > + * active high or active low. > + */ > + res = !(status & NAU8825_GPIO2JD1) * !(jkdet & NAU8825_JACK_POLARITY) + > + (status & NAU8825_GPIO2JD1) * (jkdet & NAU8825_JACK_POLARITY); It makes more sense to use a more boolean-like expression. Something like return !!(status & NAU8825_GPIO2JD1) == !!(jkdet & NAU8825_JACK_POLARITY); (hope I translated expression above correctly) or even better to introduce readable bool flags: bool active_high = !!(jkdet & NAU8825_JACK_POLARITY); bool is_high = !!(status & NAU8825_GPIO2JD1); return active_high == is_high; > + > + return res ? true : false; > } > > static void nau8825_restart_jack_detection(struct regmap *regmap) > -- > 2.6.4 >
Hi On 6/29/2016 11:45 AM, Anatol Pomozov wrote: > Hi > > On Tue, Jun 28, 2016 at 8:20 PM, John Hsu <KCHSU0@nuvoton.com> wrote: > >> The original design only covers the jack insertion logic is active low. >> Add more condition to cover no matter the logic is active low and high. >> >> Signed-off-by: John Hsu <KCHSU0@nuvoton.com> >> --- >> sound/soc/codecs/nau8825.c | 12 ++++++++++-- >> 1 file changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/sound/soc/codecs/nau8825.c b/sound/soc/codecs/nau8825.c >> index 3f30e6e..a2f0d03 100644 >> --- a/sound/soc/codecs/nau8825.c >> +++ b/sound/soc/codecs/nau8825.c >> @@ -1345,10 +1345,18 @@ EXPORT_SYMBOL_GPL(nau8825_enable_jack_detect); >> >> static bool nau8825_is_jack_inserted(struct regmap *regmap) >> { >> - int status; >> + int status, jkdet, res; >> >> regmap_read(regmap, NAU8825_REG_I2C_DEVICE_ID, &status); >> - return !(status & NAU8825_GPIO2JD1); >> + regmap_read(regmap, NAU8825_REG_JACK_DET_CTRL, &jkdet); >> + >> + /* return jack connection status according to jack insertion logic >> + * active high or active low. >> + */ >> + res = !(status & NAU8825_GPIO2JD1) * !(jkdet & NAU8825_JACK_POLARITY) + >> + (status & NAU8825_GPIO2JD1) * (jkdet & NAU8825_JACK_POLARITY); >> > > It makes more sense to use a more boolean-like expression. Something like > > return !!(status & NAU8825_GPIO2JD1) == !!(jkdet & NAU8825_JACK_POLARITY); > (hope I translated expression above correctly) > > or even better to introduce readable bool flags: > bool active_high = !!(jkdet & NAU8825_JACK_POLARITY); > bool is_high = !!(status & NAU8825_GPIO2JD1); > return active_high == is_high; > > Yes, it'll be more readable. I have a question. Why to add !! in front of bit wise operation? What does it mean?
On Thu, Jun 30, 2016 at 03:51:51PM +0800, John Hsu wrote: > Yes, it'll be more readable. I have a question. Why to add !! in > front of bit wise operation? What does it mean? It's redundant in almost all cases. It translates an integer value into a 0/1 value (so if you've got a value of 2 it'll end up as 1) by doing a double not. This very rarely matters unless you're storing the value in an integer and comparing it to 1 to check for truth, otherwise C will translate any non-zero value into true in any logic context.
On 7/1/2016 11:57 PM, Mark Brown wrote: > On Thu, Jun 30, 2016 at 03:51:51PM +0800, John Hsu wrote: > > >> Yes, it'll be more readable. I have a question. Why to add !! in >> front of bit wise operation? What does it mean? >> > > It's redundant in almost all cases. It translates an integer value > into a 0/1 value (so if you've got a value of 2 it'll end up as 1) by > doing a double not. This very rarely matters unless you're storing the > value in an integer and comparing it to 1 to check for truth, otherwise > C will translate any non-zero value into true in any logic context. > Thank you for the explanation. I get the purpose now and will change the expression of function for clear intent.
Hi On Fri, Jul 1, 2016 at 8:57 AM, Mark Brown <broonie@kernel.org> wrote: > On Thu, Jun 30, 2016 at 03:51:51PM +0800, John Hsu wrote: > >> Yes, it'll be more readable. I have a question. Why to add !! in >> front of bit wise operation? What does it mean? > > It's redundant in almost all cases. It translates an integer value > into a 0/1 value (so if you've got a value of 2 it'll end up as 1) by > doing a double not. This very rarely matters unless you're storing the > value in an integer and comparing it to 1 to check for truth, otherwise > C will translate any non-zero value into true in any logic context. Thanks Mark for the comment. I added "!!" by following my muscle memory. That how I used to convert non-bools to bool. But your comment forced me to research this question. C99 and C11 standards clarify this issue http://port70.net/~nsz/c/c11/n1570.html#6.3.1.2p1 ..<QUOTE>.. When any scalar value is converted to _Bool, the result is 0 if the value compares equal to 0; otherwise, the result is 1 It is safe to drop "!!" idiom completely - compiler will take care of converting the result of bitmask to a proper bool value. My example above can be overwritten as bool active_high = jkdet & NAU8825_JACK_POLARITY; bool is_high = status & NAU8825_GPIO2JD1; return active_high == is_high;
diff --git a/sound/soc/codecs/nau8825.c b/sound/soc/codecs/nau8825.c index 3f30e6e..a2f0d03 100644 --- a/sound/soc/codecs/nau8825.c +++ b/sound/soc/codecs/nau8825.c @@ -1345,10 +1345,18 @@ EXPORT_SYMBOL_GPL(nau8825_enable_jack_detect); static bool nau8825_is_jack_inserted(struct regmap *regmap) { - int status; + int status, jkdet, res; regmap_read(regmap, NAU8825_REG_I2C_DEVICE_ID, &status); - return !(status & NAU8825_GPIO2JD1); + regmap_read(regmap, NAU8825_REG_JACK_DET_CTRL, &jkdet); + + /* return jack connection status according to jack insertion logic + * active high or active low. + */ + res = !(status & NAU8825_GPIO2JD1) * !(jkdet & NAU8825_JACK_POLARITY) + + (status & NAU8825_GPIO2JD1) * (jkdet & NAU8825_JACK_POLARITY); + + return res ? true : false; } static void nau8825_restart_jack_detection(struct regmap *regmap)
The original design only covers the jack insertion logic is active low. Add more condition to cover no matter the logic is active low and high. Signed-off-by: John Hsu <KCHSU0@nuvoton.com> --- sound/soc/codecs/nau8825.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)