[PATCH v2 1/2] extcon: arizona: Fix reset of HPDET after race with removal

[PATCH v2 1/2] extcon: arizona: Fix reset of HPDET after race with removal

[Charles Keepax]

We need to make sure we reset back to our starting state, especially
making sure that we have disabled poll in the register cache.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 drivers/extcon/extcon-arizona.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
index ec9a14e..dac8ba0 100644
--- a/drivers/extcon/extcon-arizona.c
+++ b/drivers/extcon/extcon-arizona.c
@@ -596,9 +596,15 @@ static irqreturn_t arizona_hpdet_irq(int irq, void *data)
 		dev_err(arizona->dev, "Failed to report HP/line: %d\n",
 			ret);
 
+done:
+	/* Reset back to starting range */
+	regmap_update_bits(arizona->regmap,
+			   ARIZONA_HEADPHONE_DETECT_1,
+			   ARIZONA_HP_IMPEDANCE_RANGE_MASK | ARIZONA_HP_POLL,
+			   0);
+
 	arizona_extcon_do_magic(info, 0);
 
-done:
 	if (id_gpio)
 		gpio_set_value_cansleep(id_gpio, 0);
 
-- 
1.7.2.5

[PATCH 2/2] extcon: arizona: Fix race with microphone detection and removal

[Charles Keepax]

The microphone detection code is run as delayed work to provide
additional debounce, it is possible that the jack could have been
removed by the time we process the microphone detection. Turn this case
into a no op.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---

As Lee Jones took the first patch (extcon: arizona: Add
defines for microphone detection levels) of my series
yesterday through his tree I have based this patch on the
extcon tree without that change applied.

Thanks,
Charles

 drivers/extcon/extcon-arizona.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
index dac8ba0..9c20850 100644
--- a/drivers/extcon/extcon-arizona.c
+++ b/drivers/extcon/extcon-arizona.c
@@ -773,6 +773,12 @@ static void arizona_micd_detect(struct work_struct *work)
 
 	mutex_lock(&info->lock);
 
+	if (!info->cable) {
+		dev_dbg(arizona->dev, "Ignoring MICDET for removed cable\n");
+		mutex_unlock(&info->lock);
+		return;
+	}
+
 	for (i = 0; i < 10 && !(val & 0x7fc); i++) {
 		ret = regmap_read(arizona->regmap, ARIZONA_MIC_DETECT_3, &val);
 		if (ret != 0) {
-- 
1.7.2.5

Re: [PATCH v2 1/2] extcon: arizona: Fix reset of HPDET after race with removal

[Chanwoo Choi]

Hi Charles,

On 10/28/2013 01:19 AM, Charles Keepax wrote:
> We need to make sure we reset back to our starting state, especially
> making sure that we have disabled poll in the register cache.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---
>  drivers/extcon/extcon-arizona.c |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
> index ec9a14e..dac8ba0 100644
> --- a/drivers/extcon/extcon-arizona.c
> +++ b/drivers/extcon/extcon-arizona.c
> @@ -596,9 +596,15 @@ static irqreturn_t arizona_hpdet_irq(int irq, void *data)
>  		dev_err(arizona->dev, "Failed to report HP/line: %d\n",
>  			ret);
>  
> +done:
> +	/* Reset back to starting range */
> +	regmap_update_bits(arizona->regmap,
> +			   ARIZONA_HEADPHONE_DETECT_1,
> +			   ARIZONA_HP_IMPEDANCE_RANGE_MASK | ARIZONA_HP_POLL,
> +			   0);
> +
>  	arizona_extcon_do_magic(info, 0);
>  
> -done:
>  	if (id_gpio)
>  		gpio_set_value_cansleep(id_gpio, 0);
>  
> 

The arizona_hpdet_do_id() return only either -EAGIN or 0(zero).
extcon-arizona driver could never execute 'goto done;' statement.

	ret = arizona_hpdet_do_id(info, &reading, &mic);
	if (ret == -EAGAIN) {
		goto out;
	} else if (ret < 0) {
		goto done;
	}

Thanks,
Chanwoo Choi

Re: [PATCH 2/2] extcon: arizona: Fix race with microphone detection and removal

[Chanwoo Choi]

On 10/28/2013 01:19 AM, Charles Keepax wrote:
> The microphone detection code is run as delayed work to provide
> additional debounce, it is possible that the jack could have been
> removed by the time we process the microphone detection. Turn this case
> into a no op.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---
> 
> As Lee Jones took the first patch (extcon: arizona: Add
> defines for microphone detection levels) of my series
> yesterday through his tree I have based this patch on the
> extcon tree without that change applied.
> 
> Thanks,
> Charles
> 
>  drivers/extcon/extcon-arizona.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
> index dac8ba0..9c20850 100644
> --- a/drivers/extcon/extcon-arizona.c
> +++ b/drivers/extcon/extcon-arizona.c
> @@ -773,6 +773,12 @@ static void arizona_micd_detect(struct work_struct *work)
>  
>  	mutex_lock(&info->lock);
>  
> +	if (!info->cable) {
> +		dev_dbg(arizona->dev, "Ignoring MICDET for removed cable\n");
> +		mutex_unlock(&info->lock);
> +		return;
> +	}
> +
>  	for (i = 0; i < 10 && !(val & 0x7fc); i++) {
>  		ret = regmap_read(arizona->regmap, ARIZONA_MIC_DETECT_3, &val);
>  		if (ret != 0) {
> 

This patch has build break as below log:

drivers/extcon/extcon-arizona.c: In function ‘arizona_micd_detect’:
drivers/extcon/extcon-arizona.c:776:11: error: ‘struct arizona_extcon_info’ has no member named ‘cable’

The arizona_extcon_info structure hasn't included 'cable' field.

Cheers,
Chanwoo Choi

Re: [PATCH 2/2] extcon: arizona: Fix race with microphone detection and removal

[Charles Keepax]

On Mon, Nov 04, 2013 at 09:38:00AM +0900, Chanwoo Choi wrote:
> This patch has build break as below log:
> 
> drivers/extcon/extcon-arizona.c: In function ‘arizona_micd_detect’:
> drivers/extcon/extcon-arizona.c:776:11: error: ‘struct arizona_extcon_info’ has no member named ‘cable’
> 
> The arizona_extcon_info structure hasn't included 'cable' field.

Apologies, must have been accidentally being added by another
patch in my tree. I will sort that out and resend.

Thanks,
Charles

Re: [PATCH v2 1/2] extcon: arizona: Fix reset of HPDET after race with removal

[Charles Keepax]

On Mon, Nov 04, 2013 at 09:32:50AM +0900, Chanwoo Choi wrote:
> The arizona_hpdet_do_id() return only either -EAGIN or 0(zero).
> extcon-arizona driver could never execute 'goto done;' statement.
> 
> 	ret = arizona_hpdet_do_id(info, &reading, &mic);
> 	if (ret == -EAGAIN) {
> 		goto out;
> 	} else if (ret < 0) {
> 		goto done;
> 	}

True that else if is redundant at the moment, but personally I
would be inclined to leave it in, it feels safer against possible
future edits of arizona_hpdet_do_id. It would be reasonable for
someone to assume that other return values are acceptable whilst
editing it but they would then not be handled.

Thanks,
Charles

Re: [PATCH v2 1/2] extcon: arizona: Fix reset of HPDET after race with removal

[Chanwoo Choi]

On 11/04/2013 06:21 PM, Charles Keepax wrote:
> On Mon, Nov 04, 2013 at 09:32:50AM +0900, Chanwoo Choi wrote:
>> The arizona_hpdet_do_id() return only either -EAGIN or 0(zero).
>> extcon-arizona driver could never execute 'goto done;' statement.
>>
>> 	ret = arizona_hpdet_do_id(info, &reading, &mic);
>> 	if (ret == -EAGAIN) {
>> 		goto out;
>> 	} else if (ret < 0) {
>> 		goto done;
>> 	}
> 
> True that else if is redundant at the moment, but personally I
> would be inclined to leave it in, it feels safer against possible
> future edits of arizona_hpdet_do_id. It would be reasonable for
> someone to assume that other return values are acceptable whilst
> editing it but they would then not be handled.

I know your intention. But, I cannot apply this patch for potential issue.
Also, we have to fix below dead code after returning arizona_hpdet_do_id()

	>> 	} else if (ret < 0) {
	>> 		goto done;
	>> 	}


Thanks,
Chanwoo Choi