[PATCH] ASoC: codecs: Fix WCD_MBHC_HPH_PA_EN usage

[PATCH] ASoC: codecs: Fix WCD_MBHC_HPH_PA_EN usage

[Christophe JAILLET]

'hphpa_on' is known to be false, so the if block at the end of the function
is dead code.

Turn it into a meaningful code by having 'hphpa_on' be static. Use is as a
flip-flop variable.

Fixes: 0e5c9e7ff899 ("ASoC: codecs: wcd: add multi button Headset detection support")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
The purpose of this patch is not to be correct (!) but to draw attention
on several points:
   - in 'wcd_mbhc_adc_hs_rem_irq()', the "if (hphpa_on)" path is dead code
     because 'hphpa_on' is known to be false
   - What is this magic number '3'?
     All 'wcd_mbhc_read_field()' look for 0 or non-0
   - a 'mutex_[un]lock()' in an IRQ handler looks spurious to me

Instead of this (likely broken) patch, it is likely that something is
missing elsewhere. Maybe in 'wcd_mbhc_adc_hs_ins_irq()'.
I also guess that 'hphpa_on' should be read for somewhere else.
---
 sound/soc/codecs/wcd-mbhc-v2.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/wcd-mbhc-v2.c b/sound/soc/codecs/wcd-mbhc-v2.c
index 405128ccb4b0..783d8c35bc1b 100644
--- a/sound/soc/codecs/wcd-mbhc-v2.c
+++ b/sound/soc/codecs/wcd-mbhc-v2.c
@@ -1176,7 +1176,7 @@ static irqreturn_t wcd_mbhc_adc_hs_rem_irq(int irq, void *data)
 	struct wcd_mbhc *mbhc = data;
 	unsigned long timeout;
 	int adc_threshold, output_mv, retry = 0;
-	bool hphpa_on = false;
+	static bool hphpa_on = false;
 
 	mutex_lock(&mbhc->lock);
 	timeout = jiffies + msecs_to_jiffies(WCD_FAKE_REMOVAL_MIN_PERIOD_MS);
@@ -1212,6 +1212,9 @@ static irqreturn_t wcd_mbhc_adc_hs_rem_irq(int irq, void *data)
 
 	if (hphpa_on) {
 		hphpa_on = false;
+		wcd_mbhc_write_field(mbhc, WCD_MBHC_HPH_PA_EN, 0);
+	} else {
+		hphpa_on = true;
 		wcd_mbhc_write_field(mbhc, WCD_MBHC_HPH_PA_EN, 3);
 	}
 exit:
-- 
2.30.2

Re: [PATCH] ASoC: codecs: Fix WCD_MBHC_HPH_PA_EN usage

[Srinivas Kandagatla]

On 17/10/2021 08:31, Christophe JAILLET wrote:
> 'hphpa_on' is known to be false, so the if block at the end of the function
> is dead code.

Yes, this is a dead code we should remove it.

This code was part of moisture detection logic which is not enabled in 
upstream code yet.

During Moisture detection if the PA is on then we switch it off and do 
moisture measurements and at the end of the function we restore the 
state of PA.

> 
> Turn it into a meaningful code by having 'hphpa_on' be static. Use is as a
> flip-flop variable.

No, It does not.

Have you even tested this patch in anyway?

> 
> Fixes: 0e5c9e7ff899 ("ASoC: codecs: wcd: add multi button Headset detection support")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> The purpose of this patch is not to be correct (!) but to draw attention
> on several points:
>     - in 'wcd_mbhc_adc_hs_rem_irq()', the "if (hphpa_on)" path is dead code
>       because 'hphpa_on' is known to be false
>     - What is this magic number '3'?
>       All 'wcd_mbhc_read_field()' look for 0 or non-0
>     - a 'mutex_[un]lock()' in an IRQ handler looks spurious to me
> 
> Instead of this (likely broken) patch, it is likely that something is
> missing elsewhere. Maybe in 'wcd_mbhc_adc_hs_ins_irq()'.
> I also guess that 'hphpa_on' should be read for somewhere else.
> ---
>   sound/soc/codecs/wcd-mbhc-v2.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/soc/codecs/wcd-mbhc-v2.c b/sound/soc/codecs/wcd-mbhc-v2.c
> index 405128ccb4b0..783d8c35bc1b 100644
> --- a/sound/soc/codecs/wcd-mbhc-v2.c
> +++ b/sound/soc/codecs/wcd-mbhc-v2.c
> @@ -1176,7 +1176,7 @@ static irqreturn_t wcd_mbhc_adc_hs_rem_irq(int irq, void *data)
>   	struct wcd_mbhc *mbhc = data;
>   	unsigned long timeout;
>   	int adc_threshold, output_mv, retry = 0;
> -	bool hphpa_on = false;
> +	static bool hphpa_on = false;
>   
>   	mutex_lock(&mbhc->lock);
>   	timeout = jiffies + msecs_to_jiffies(WCD_FAKE_REMOVAL_MIN_PERIOD_MS);
> @@ -1212,6 +1212,9 @@ static irqreturn_t wcd_mbhc_adc_hs_rem_irq(int irq, void *data)
>   
>   	if (hphpa_on) {
>   		hphpa_on = false;
> +		wcd_mbhc_write_field(mbhc, WCD_MBHC_HPH_PA_EN, 0);
> +	} else {
> +		hphpa_on = true;
>   		wcd_mbhc_write_field(mbhc, WCD_MBHC_HPH_PA_EN, 3);

Just remove this dead code.

--srini
>   	}
>   exit:
> 

Re: [PATCH] ASoC: codecs: Fix WCD_MBHC_HPH_PA_EN usage

[Christophe JAILLET]

Le 19/10/2021 à 15:47, Srinivas Kandagatla a écrit :
> 
> 
> On 17/10/2021 08:31, Christophe JAILLET wrote:
>> 'hphpa_on' is known to be false, so the if block at the end of the 
>> function
>> is dead code.
> 
> Yes, this is a dead code we should remove it.

Ok, thanks for the clarification.

> 
> This code was part of moisture detection logic which is not enabled in 
> upstream code yet.

If 'yet' is the important word of the sentence, maybe the best is to 
leave the code as-is.
If you prefer it to be removed, I can send a patch if it helps.

> 
> During Moisture detection if the PA is on then we switch it off and do 
> moisture measurements and at the end of the function we restore the 
> state of PA.
> 
>>
>> Turn it into a meaningful code by having 'hphpa_on' be static. Use is 
>> as a
>> flip-flop variable.
> 
> No, It does not.
> 
> Have you even tested this patch in anyway?

No, as said below the ---, the purpose of this patch was not to be 
correct (or tested). It was only to draw attention on odd things.

CJ



> 
>>
>> Fixes: 0e5c9e7ff899 ("ASoC: codecs: wcd: add multi button Headset 
>> detection support")
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>> The purpose of this patch is not to be correct (!) but to draw attention
>> on several points:
>>     - in 'wcd_mbhc_adc_hs_rem_irq()', the "if (hphpa_on)" path is dead 
>> code
>>       because 'hphpa_on' is known to be false
>>     - What is this magic number '3'?
>>       All 'wcd_mbhc_read_field()' look for 0 or non-0
>>     - a 'mutex_[un]lock()' in an IRQ handler looks spurious to me
>>
>> Instead of this (likely broken) patch, it is likely that something is
>> missing elsewhere. Maybe in 'wcd_mbhc_adc_hs_ins_irq()'.
>> I also guess that 'hphpa_on' should be read for somewhere else.
>> ---
>>   sound/soc/codecs/wcd-mbhc-v2.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/sound/soc/codecs/wcd-mbhc-v2.c 
>> b/sound/soc/codecs/wcd-mbhc-v2.c
>> index 405128ccb4b0..783d8c35bc1b 100644
>> --- a/sound/soc/codecs/wcd-mbhc-v2.c
>> +++ b/sound/soc/codecs/wcd-mbhc-v2.c
>> @@ -1176,7 +1176,7 @@ static irqreturn_t wcd_mbhc_adc_hs_rem_irq(int 
>> irq, void *data)
>>       struct wcd_mbhc *mbhc = data;
>>       unsigned long timeout;
>>       int adc_threshold, output_mv, retry = 0;
>> -    bool hphpa_on = false;
>> +    static bool hphpa_on = false;
>>       mutex_lock(&mbhc->lock);
>>       timeout = jiffies + 
>> msecs_to_jiffies(WCD_FAKE_REMOVAL_MIN_PERIOD_MS);
>> @@ -1212,6 +1212,9 @@ static irqreturn_t wcd_mbhc_adc_hs_rem_irq(int 
>> irq, void *data)
>>       if (hphpa_on) {
>>           hphpa_on = false;
>> +        wcd_mbhc_write_field(mbhc, WCD_MBHC_HPH_PA_EN, 0);
>> +    } else {
>> +        hphpa_on = true;
>>           wcd_mbhc_write_field(mbhc, WCD_MBHC_HPH_PA_EN, 3);
> 
> Just remove this dead code.
> 
> --srini
>>       }
>>   exit:
>>
> 

Re: [PATCH] ASoC: codecs: Fix WCD_MBHC_HPH_PA_EN usage

[Dan Carpenter]

On Tue, Oct 19, 2021 at 07:39:04PM +0200, Christophe JAILLET wrote:
> Le 19/10/2021 à 15:47, Srinivas Kandagatla a écrit :
> > 
> > 
> > On 17/10/2021 08:31, Christophe JAILLET wrote:
> > > 'hphpa_on' is known to be false, so the if block at the end of the
> > > function
> > > is dead code.
> > 
> > Yes, this is a dead code we should remove it.
> 
> Ok, thanks for the clarification.
> 
> > 
> > This code was part of moisture detection logic which is not enabled in
> > upstream code yet.
> 
> If 'yet' is the important word of the sentence, maybe the best is to leave
> the code as-is.
> If you prefer it to be removed, I can send a patch if it helps.

Just delete the dead code...  Never try to plan for the future, it's too
unpredictable.

regards,
dasn carpenter