[PATCH] ASoC: nau8825: jack connection decision with different insertion logic

[PATCH] ASoC: nau8825: jack connection decision with different insertion logic

[John Hsu]

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);
+
+	return res ? true : false;
 }
 
 static void nau8825_restart_jack_detection(struct regmap *regmap)
-- 
2.6.4

Re: [PATCH] ASoC: nau8825: jack connection decision with different insertion logic

[Anatol Pomozov]

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
>

Re: [PATCH] ASoC: nau8825: jack connection decision with different insertion logic

[John Hsu]

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?

Re: [PATCH] ASoC: nau8825: jack connection decision with different insertion logic

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 527 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: nau8825: jack connection decision with different insertion logic

[John Hsu]

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.

Re: [PATCH] ASoC: nau8825: jack connection decision with different insertion logic

[Anatol Pomozov]

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;

[PATCH] ASoC: nau8825: jack connection decision with different insertion logic

[John Hsu]

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 | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/nau8825.c b/sound/soc/codecs/nau8825.c
index 3f30e6e..a97418d 100644
--- a/sound/soc/codecs/nau8825.c
+++ b/sound/soc/codecs/nau8825.c
@@ -1345,10 +1345,17 @@ EXPORT_SYMBOL_GPL(nau8825_enable_jack_detect);
 
 static bool nau8825_is_jack_inserted(struct regmap *regmap)
 {
-	int status;
+	bool active_high, is_high;
+	int status, jkdet;
 
+	regmap_read(regmap, NAU8825_REG_JACK_DET_CTRL, &jkdet);
+	active_high = !!(jkdet & NAU8825_JACK_POLARITY);
 	regmap_read(regmap, NAU8825_REG_I2C_DEVICE_ID, &status);
-	return !(status & NAU8825_GPIO2JD1);
+	is_high = !!(status & NAU8825_GPIO2JD1);
+	/* return jack connection status according to jack insertion logic
+	 * active high or active low.
+	 */
+	return active_high == is_high;
 }
 
 static void nau8825_restart_jack_detection(struct regmap *regmap)
-- 
2.6.4