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

[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

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;

Applied "ASoC: nau8825: jack connection decision with different insertion logic" to the asoc tree

[Mark Brown]

The patch

   ASoC: nau8825: jack connection decision with different insertion logic

has been applied to the asoc tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From bff03e81502cb9ac99daeeb47b4d0e779cc48fde Mon Sep 17 00:00:00 2001
From: John Hsu <KCHSU0@nuvoton.com>
Date: Wed, 6 Jul 2016 10:09:35 +0800
Subject: [PATCH] ASoC: nau8825: jack connection decision with different
 insertion logic

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>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 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 3f30e6ed210c..a97418deb034 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.8.1