Re: [PATCH v2] extcon: arizona: Wait for any running HPDETs to complete on jack removal

[Chanwoo Choi <cw00.choi@samsung.com>]

On 2017년 01월 25일 18:34, Charles Keepax wrote:
> As the HPDET can't be aborted mid way through we should not allow any new
> insertion to be processed until the previous HPDET has finished. It is very
> unlikely but with low enough debounce settings you could start a new HPDET
> before the old one has completed, which results in an erroneous reading.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---
> 
> Changes since v1:
>  - Added defines for the count and delay
>  - Added a comment to explain why we call arizona_hpdet_wait
> 
> Thanks,
> Charles
> 
>  drivers/extcon/extcon-arizona.c | 45 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
> index ed78b7c..df98d84 100644
> --- a/drivers/extcon/extcon-arizona.c
> +++ b/drivers/extcon/extcon-arizona.c
> @@ -1049,6 +1049,42 @@ static void arizona_hpdet_work(struct work_struct *work)
>  	mutex_unlock(&info->lock);
>  }
>  
> +#define ARIZONA_HPDET_WAIT_COUNT 15
> +#define ARIZONA_HPDET_WAIT_DELAY_MS 20
> +
> +static int arizona_hpdet_wait(struct arizona_extcon_info *info)
> +{
> +	struct arizona *arizona = info->arizona;
> +	unsigned int val;
> +	int i, ret;
> +
> +	for (i = 0; i < ARIZONA_HPDET_WAIT_COUNT; i++) {
> +		ret = regmap_read(arizona->regmap, ARIZONA_HEADPHONE_DETECT_2,
> +				  &val);
> +		if (ret) {
> +			dev_err(arizona->dev,
> +				"Failed to read HPDET state: %d\n", ret);
> +			return ret;
> +		}
> +
> +		switch (info->hpdet_ip_version) {
> +		case 0:
> +			if (val & ARIZONA_HP_DONE)
> +				return 0;
> +			break;
> +		default:
> +			if (val & ARIZONA_HP_DONE_B)
> +				return 0;
> +			break;
> +		}
> +
> +		msleep(ARIZONA_HPDET_WAIT_DELAY_MS);
> +	}
> +
> +	dev_err(arizona->dev, "HPDET did not appear to complete\n");
> +	return -ETIMEDOUT;
> +}
> +
>  static irqreturn_t arizona_jackdet(int irq, void *data)
>  {
>  	struct arizona_extcon_info *info = data;
> @@ -1155,6 +1191,15 @@ static irqreturn_t arizona_jackdet(int irq, void *data)
>  					"Removal report failed: %d\n", ret);
>  		}
>  
> +		/*
> +		 * If the jack was removed during a headphone detection we
> +		 * need to wait for the headphone detection to finish, as
> +		 * it can not be aborted. We don't want to be able to start
> +		 * a new headphone detection from a fresh insert until this
> +		 * one is finished.
> +		 */
> +		arizona_hpdet_wait(info);

If there is no necessary to handle the error return value,
I recommend that you better to use the dev_warn() instead of dev_err().

	dev_warn(arizona->dev, "HPDET did not appear to complete\n");


How about changing the debug level with dev_warn()?

> +
>  		regmap_update_bits(arizona->regmap,
>  				   ARIZONA_JACK_DETECT_DEBOUNCE,
>  				   ARIZONA_MICD_CLAMP_DB | ARIZONA_JD1_DB,
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics