[PATCH 1/4] extcon: arizona: Add defines for microphone detection levels

[PATCH 1/4] extcon: arizona: Add defines for microphone detection levels

[Charles Keepax]

Improve readability by creating a define for each microphone detection
level.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 drivers/extcon/extcon-arizona.c       |   21 ++++++++++++++++-----
 include/linux/mfd/arizona/registers.h |    9 +++++++++
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
index 3c55ec8..6d914ba 100644
--- a/drivers/extcon/extcon-arizona.c
+++ b/drivers/extcon/extcon-arizona.c
@@ -44,6 +44,17 @@
 #define HPDET_DEBOUNCE 500
 #define DEFAULT_MICD_TIMEOUT 2000
 
+enum {
+	MICD_LVL_1_TO_7 = ARIZONA_MICD_LVL_1 | ARIZONA_MICD_LVL_2 |
+			  ARIZONA_MICD_LVL_3 | ARIZONA_MICD_LVL_4 |
+			  ARIZONA_MICD_LVL_5 | ARIZONA_MICD_LVL_6 |
+			  ARIZONA_MICD_LVL_7,
+
+	MICD_LVL_0_TO_7 = ARIZONA_MICD_LVL_0 | MICD_LVL_1_TO_7,
+
+	MICD_LVL_0_TO_8 = MICD_LVL_0_TO_7 | ARIZONA_MICD_LVL_8,
+};
+
 struct arizona_extcon_info {
 	struct device *dev;
 	struct arizona *arizona;
@@ -765,7 +776,7 @@ static void arizona_micd_detect(struct work_struct *work)
 
 	mutex_lock(&info->lock);
 
-	for (i = 0; i < 10 && !(val & 0x7fc); i++) {
+	for (i = 0; i < 10 && !(val & MICD_LVL_0_TO_8); i++) {
 		ret = regmap_read(arizona->regmap, ARIZONA_MIC_DETECT_3, &val);
 		if (ret != 0) {
 			dev_err(arizona->dev,
@@ -784,7 +795,7 @@ static void arizona_micd_detect(struct work_struct *work)
 		}
 	}
 
-	if (i == 10 && !(val & 0x7fc)) {
+	if (i == 10 && !(val & MICD_LVL_0_TO_8)) {
 		dev_err(arizona->dev, "Failed to get valid MICDET value\n");
 		mutex_unlock(&info->lock);
 		return;
@@ -798,7 +809,7 @@ static void arizona_micd_detect(struct work_struct *work)
 	}
 
 	/* If we got a high impedence we should have a headset, report it. */
-	if (info->detecting && (val & 0x400)) {
+	if (info->detecting && (val & ARIZONA_MICD_LVL_8)) {
 		arizona_identify_headphone(info);
 
 		ret = extcon_update_state(&info->edev,
@@ -827,7 +838,7 @@ static void arizona_micd_detect(struct work_struct *work)
 	 * plain headphones.  If both polarities report a low
 	 * impedence then give up and report headphones.
 	 */
-	if (info->detecting && (val & 0x3f8)) {
+	if (info->detecting && (val & MICD_LVL_1_TO_7)) {
 		if (info->jack_flips >= info->micd_num_modes * 10) {
 			dev_dbg(arizona->dev, "Detected HP/line\n");
 			arizona_identify_headphone(info);
@@ -851,7 +862,7 @@ static void arizona_micd_detect(struct work_struct *work)
 	 * If we're still detecting and we detect a short then we've
 	 * got a headphone.  Otherwise it's a button press.
 	 */
-	if (val & 0x3fc) {
+	if (val & MICD_LVL_0_TO_7) {
 		if (info->mic) {
 			dev_dbg(arizona->dev, "Mic button detected\n");
 
diff --git a/include/linux/mfd/arizona/registers.h b/include/linux/mfd/arizona/registers.h
index 4706d3d..10d9e70 100644
--- a/include/linux/mfd/arizona/registers.h
+++ b/include/linux/mfd/arizona/registers.h
@@ -2196,6 +2196,15 @@
 /*
  * R677 (0x2A5) - Mic Detect 3
  */
+#define ARIZONA_MICD_LVL_0                       0x0004  /* MICD_LVL - [2] */
+#define ARIZONA_MICD_LVL_1                       0x0008  /* MICD_LVL - [3] */
+#define ARIZONA_MICD_LVL_2                       0x0010  /* MICD_LVL - [4] */
+#define ARIZONA_MICD_LVL_3                       0x0020  /* MICD_LVL - [5] */
+#define ARIZONA_MICD_LVL_4                       0x0040  /* MICD_LVL - [6] */
+#define ARIZONA_MICD_LVL_5                       0x0080  /* MICD_LVL - [7] */
+#define ARIZONA_MICD_LVL_6                       0x0100  /* MICD_LVL - [8] */
+#define ARIZONA_MICD_LVL_7                       0x0200  /* MICD_LVL - [9] */
+#define ARIZONA_MICD_LVL_8                       0x0400  /* MICD_LVL - [10] */
 #define ARIZONA_MICD_LVL_MASK                    0x07FC  /* MICD_LVL - [10:2] */
 #define ARIZONA_MICD_LVL_SHIFT                        2  /* MICD_LVL - [10:2] */
 #define ARIZONA_MICD_LVL_WIDTH                        9  /* MICD_LVL - [10:2] */
-- 
1.7.2.5

[PATCH 2/4] 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 6d914ba..cce24de 100644
--- a/drivers/extcon/extcon-arizona.c
+++ b/drivers/extcon/extcon-arizona.c
@@ -605,9 +605,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 3/4] 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>
---
 drivers/extcon/extcon-arizona.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
index cce24de..0d70bf6 100644
--- a/drivers/extcon/extcon-arizona.c
+++ b/drivers/extcon/extcon-arizona.c
@@ -782,6 +782,19 @@ static void arizona_micd_detect(struct work_struct *work)
 
 	mutex_lock(&info->lock);
 
+	/* If the cable was removed while measuring ignore the result */
+	ret = extcon_get_cable_state_(&info->edev, ARIZONA_CABLE_MECHANICAL);
+	if (ret < 0) {
+		dev_err(arizona->dev, "Failed to check cable state: %d\n",
+				ret);
+		mutex_unlock(&info->lock);
+		return;
+	} else if (!ret) {
+		dev_dbg(arizona->dev, "Ignoring MICDET for removed cable\n");
+		mutex_unlock(&info->lock);
+		return;
+	}
+
 	for (i = 0; i < 10 && !(val & MICD_LVL_0_TO_8); i++) {
 		ret = regmap_read(arizona->regmap, ARIZONA_MIC_DETECT_3, &val);
 		if (ret != 0) {
-- 
1.7.2.5

[PATCH 4/4] extcon: arizona: Eliminate dead error handling code

[Charles Keepax]

As a small disclaimer I would personally prefer to not merge this patch.
I have added it based on previous code review of the other patches in
this chain.

arizona_hpdet_do_id currently can only return 0 or -EAGAIN making the
else if clause handling error codes redundant, this patch removes this
clause.

Whilst this clause is not currently hit removing it makes the code
fragile. It will not be obvious whilst editing arizona_hpdet_do_id that
you shouldn't add a return value other than 0 or -EAGAIN.

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

diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
index 0d70bf6..2313b1e 100644
--- a/drivers/extcon/extcon-arizona.c
+++ b/drivers/extcon/extcon-arizona.c
@@ -476,6 +476,9 @@ static int arizona_hpdet_read(struct arizona_extcon_info *info)
 	return val;
 }
 
+/* This function should only return 0 or -EAGAIN, if other return values are
+ * added additional handling should be added in arizona_hpdet_irq.
+ */
 static int arizona_hpdet_do_id(struct arizona_extcon_info *info, int *reading,
 			       bool *mic)
 {
@@ -591,8 +594,6 @@ static irqreturn_t arizona_hpdet_irq(int irq, void *data)
 	ret = arizona_hpdet_do_id(info, &reading, &mic);
 	if (ret == -EAGAIN)
 		goto out;
-	else if (ret < 0)
-		goto done;
 
 	/* Report high impedence cables as line outputs */
 	if (reading >= 5000)
-- 
1.7.2.5

Re: [PATCH 4/4] extcon: arizona: Eliminate dead error handling code

[Lee Jones]

> As a small disclaimer I would personally prefer to not merge this patch.
> I have added it based on previous code review of the other patches in
> this chain.

I'd prefer the functionally redundant 'else if' over the comment.

I'm happy not to merge this patch.

> arizona_hpdet_do_id currently can only return 0 or -EAGAIN making the
> else if clause handling error codes redundant, this patch removes this
> clause.
> 
> Whilst this clause is not currently hit removing it makes the code
> fragile. It will not be obvious whilst editing arizona_hpdet_do_id that
> you shouldn't add a return value other than 0 or -EAGAIN.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---
>  drivers/extcon/extcon-arizona.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
> index 0d70bf6..2313b1e 100644
> --- a/drivers/extcon/extcon-arizona.c
> +++ b/drivers/extcon/extcon-arizona.c
> @@ -476,6 +476,9 @@ static int arizona_hpdet_read(struct arizona_extcon_info *info)
>  	return val;
>  }
>  
> +/* This function should only return 0 or -EAGAIN, if other return values are
> + * added additional handling should be added in arizona_hpdet_irq.
> + */

Please see: Documentation/CodingStyle: Chapter 8: Commenting

>  static int arizona_hpdet_do_id(struct arizona_extcon_info *info, int *reading,
>  			       bool *mic)
>  {
> @@ -591,8 +594,6 @@ static irqreturn_t arizona_hpdet_irq(int irq, void *data)
>  	ret = arizona_hpdet_do_id(info, &reading, &mic);
>  	if (ret == -EAGAIN)
>  		goto out;
> -	else if (ret < 0)
> -		goto done;
>  
>  	/* Report high impedence cables as line outputs */
>  	if (reading >= 5000)

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 4/4] extcon: arizona: Eliminate dead error handling code

[Charles Keepax]

On Mon, Nov 11, 2013 at 09:00:14AM +0000, Lee Jones wrote:
> > As a small disclaimer I would personally prefer to not merge this patch.
> > I have added it based on previous code review of the other patches in
> > this chain.
> 
> I'd prefer the functionally redundant 'else if' over the comment.
> 
> I'm happy not to merge this patch.

I agree entirely the redundant 'else if' is much better.

Thanks,
Charles

Re: [PATCH 1/4] extcon: arizona: Add defines for microphone detection levels

[Chanwoo Choi]

Hi Charles,

On 11/08/2013 10:19 PM, Charles Keepax wrote:
> Improve readability by creating a define for each microphone detection
> level.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---
>  drivers/extcon/extcon-arizona.c       |   21 ++++++++++++++++-----
>  include/linux/mfd/arizona/registers.h |    9 +++++++++
>  2 files changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
> index 3c55ec8..6d914ba 100644
> --- a/drivers/extcon/extcon-arizona.c
> +++ b/drivers/extcon/extcon-arizona.c
> @@ -44,6 +44,17 @@
>  #define HPDET_DEBOUNCE 500
>  #define DEFAULT_MICD_TIMEOUT 2000
>  
> +enum {
> +	MICD_LVL_1_TO_7 = ARIZONA_MICD_LVL_1 | ARIZONA_MICD_LVL_2 |
> +			  ARIZONA_MICD_LVL_3 | ARIZONA_MICD_LVL_4 |
> +			  ARIZONA_MICD_LVL_5 | ARIZONA_MICD_LVL_6 |
> +			  ARIZONA_MICD_LVL_7,
> +
> +	MICD_LVL_0_TO_7 = ARIZONA_MICD_LVL_0 | MICD_LVL_1_TO_7,
> +
> +	MICD_LVL_0_TO_8 = MICD_LVL_0_TO_7 | ARIZONA_MICD_LVL_8,
> +};

MICD_LVL_1_TO_7 / MICD_LVL_0_TO_7 /MICD_LVL_0_TO_8 haven't the sequential value.
I prefer '#define' keyword to define MICD_LVL_1_TO_7 / MICD_LVL_0_TO_7 /MICD_LVL_0_TO_8
instead of enum keyword.

> +
>  struct arizona_extcon_info {
>  	struct device *dev;
>  	struct arizona *arizona;
> @@ -765,7 +776,7 @@ static void arizona_micd_detect(struct work_struct *work)
>  
>  	mutex_lock(&info->lock);
>  
> -	for (i = 0; i < 10 && !(val & 0x7fc); i++) {
> +	for (i = 0; i < 10 && !(val & MICD_LVL_0_TO_8); i++) {
>  		ret = regmap_read(arizona->regmap, ARIZONA_MIC_DETECT_3, &val);
>  		if (ret != 0) {
>  			dev_err(arizona->dev,
> @@ -784,7 +795,7 @@ static void arizona_micd_detect(struct work_struct *work)
>  		}
>  	}
>  
> -	if (i == 10 && !(val & 0x7fc)) {
> +	if (i == 10 && !(val & MICD_LVL_0_TO_8)) {
>  		dev_err(arizona->dev, "Failed to get valid MICDET value\n");
>  		mutex_unlock(&info->lock);
>  		return;
> @@ -798,7 +809,7 @@ static void arizona_micd_detect(struct work_struct *work)
>  	}
>  
>  	/* If we got a high impedence we should have a headset, report it. */
> -	if (info->detecting && (val & 0x400)) {
> +	if (info->detecting && (val & ARIZONA_MICD_LVL_8)) {
>  		arizona_identify_headphone(info);
>  
>  		ret = extcon_update_state(&info->edev,
> @@ -827,7 +838,7 @@ static void arizona_micd_detect(struct work_struct *work)
>  	 * plain headphones.  If both polarities report a low
>  	 * impedence then give up and report headphones.
>  	 */
> -	if (info->detecting && (val & 0x3f8)) {
> +	if (info->detecting && (val & MICD_LVL_1_TO_7)) {
>  		if (info->jack_flips >= info->micd_num_modes * 10) {
>  			dev_dbg(arizona->dev, "Detected HP/line\n");
>  			arizona_identify_headphone(info);
> @@ -851,7 +862,7 @@ static void arizona_micd_detect(struct work_struct *work)
>  	 * If we're still detecting and we detect a short then we've
>  	 * got a headphone.  Otherwise it's a button press.
>  	 */
> -	if (val & 0x3fc) {
> +	if (val & MICD_LVL_0_TO_7) {
>  		if (info->mic) {
>  			dev_dbg(arizona->dev, "Mic button detected\n");
>  
> diff --git a/include/linux/mfd/arizona/registers.h b/include/linux/mfd/arizona/registers.h
> index 4706d3d..10d9e70 100644
> --- a/include/linux/mfd/arizona/registers.h
> +++ b/include/linux/mfd/arizona/registers.h
> @@ -2196,6 +2196,15 @@
>  /*
>   * R677 (0x2A5) - Mic Detect 3
>   */
> +#define ARIZONA_MICD_LVL_0                       0x0004  /* MICD_LVL - [2] */
> +#define ARIZONA_MICD_LVL_1                       0x0008  /* MICD_LVL - [3] */
> +#define ARIZONA_MICD_LVL_2                       0x0010  /* MICD_LVL - [4] */
> +#define ARIZONA_MICD_LVL_3                       0x0020  /* MICD_LVL - [5] */
> +#define ARIZONA_MICD_LVL_4                       0x0040  /* MICD_LVL - [6] */
> +#define ARIZONA_MICD_LVL_5                       0x0080  /* MICD_LVL - [7] */
> +#define ARIZONA_MICD_LVL_6                       0x0100  /* MICD_LVL - [8] */
> +#define ARIZONA_MICD_LVL_7                       0x0200  /* MICD_LVL - [9] */
> +#define ARIZONA_MICD_LVL_8                       0x0400  /* MICD_LVL - [10] */

>  #define ARIZONA_MICD_LVL_MASK                    0x07FC  /* MICD_LVL - [10:2] */
>  #define ARIZONA_MICD_LVL_SHIFT                        2  /* MICD_LVL - [10:2] */
>  #define ARIZONA_MICD_LVL_WIDTH                        9  /* MICD_LVL - [10:2] */
> 

Thanks,
Chanwoo Choi

Re: [PATCH 1/4] extcon: arizona: Add defines for microphone detection levels

[Lee Jones]

> On 11/08/2013 10:19 PM, Charles Keepax wrote:
> > Improve readability by creating a define for each microphone detection
> > level.
> > 
> > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > ---
> >  drivers/extcon/extcon-arizona.c       |   21 ++++++++++++++++-----
> >  include/linux/mfd/arizona/registers.h |    9 +++++++++
> >  2 files changed, 25 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
> > index 3c55ec8..6d914ba 100644
> > --- a/drivers/extcon/extcon-arizona.c
> > +++ b/drivers/extcon/extcon-arizona.c
> > @@ -44,6 +44,17 @@
> >  #define HPDET_DEBOUNCE 500
> >  #define DEFAULT_MICD_TIMEOUT 2000
> >  
> > +enum {
> > +	MICD_LVL_1_TO_7 = ARIZONA_MICD_LVL_1 | ARIZONA_MICD_LVL_2 |
> > +			  ARIZONA_MICD_LVL_3 | ARIZONA_MICD_LVL_4 |
> > +			  ARIZONA_MICD_LVL_5 | ARIZONA_MICD_LVL_6 |
> > +			  ARIZONA_MICD_LVL_7,
> > +
> > +	MICD_LVL_0_TO_7 = ARIZONA_MICD_LVL_0 | MICD_LVL_1_TO_7,
> > +
> > +	MICD_LVL_0_TO_8 = MICD_LVL_0_TO_7 | ARIZONA_MICD_LVL_8,
> > +};
> 
> MICD_LVL_1_TO_7 / MICD_LVL_0_TO_7 /MICD_LVL_0_TO_8 haven't the sequential value.
> I prefer '#define' keyword to define MICD_LVL_1_TO_7 / MICD_LVL_0_TO_7 /MICD_LVL_0_TO_8
> instead of enum keyword.

Actually Charles has already sent me this patch and I applied it a
while ago.

I'm inclined to agree with you though, so if you want to send a patch
based on v3.14-rc1 I'd be happy to accept it.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/4] extcon: arizona: Add defines for microphone detection levels

[Chanwoo Choi]

On 11/11/2013 07:53 PM, Lee Jones wrote:
>> On 11/08/2013 10:19 PM, Charles Keepax wrote:
>>> Improve readability by creating a define for each microphone detection
>>> level.
>>>
>>> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
>>> ---
>>>  drivers/extcon/extcon-arizona.c       |   21 ++++++++++++++++-----
>>>  include/linux/mfd/arizona/registers.h |    9 +++++++++
>>>  2 files changed, 25 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
>>> index 3c55ec8..6d914ba 100644
>>> --- a/drivers/extcon/extcon-arizona.c
>>> +++ b/drivers/extcon/extcon-arizona.c
>>> @@ -44,6 +44,17 @@
>>>  #define HPDET_DEBOUNCE 500
>>>  #define DEFAULT_MICD_TIMEOUT 2000
>>>  
>>> +enum {
>>> +	MICD_LVL_1_TO_7 = ARIZONA_MICD_LVL_1 | ARIZONA_MICD_LVL_2 |
>>> +			  ARIZONA_MICD_LVL_3 | ARIZONA_MICD_LVL_4 |
>>> +			  ARIZONA_MICD_LVL_5 | ARIZONA_MICD_LVL_6 |
>>> +			  ARIZONA_MICD_LVL_7,
>>> +
>>> +	MICD_LVL_0_TO_7 = ARIZONA_MICD_LVL_0 | MICD_LVL_1_TO_7,
>>> +
>>> +	MICD_LVL_0_TO_8 = MICD_LVL_0_TO_7 | ARIZONA_MICD_LVL_8,
>>> +};
>>
>> MICD_LVL_1_TO_7 / MICD_LVL_0_TO_7 /MICD_LVL_0_TO_8 haven't the sequential value.
>> I prefer '#define' keyword to define MICD_LVL_1_TO_7 / MICD_LVL_0_TO_7 /MICD_LVL_0_TO_8
>> instead of enum keyword.
> 
> Actually Charles has already sent me this patch and I applied it a
> while ago.
> 

Why did you applied extcon driver patch on your tree?

Re: [PATCH 1/4] extcon: arizona: Add defines for microphone detection levels

[Charles Keepax]

On Mon, Nov 11, 2013 at 10:53:56AM +0000, Lee Jones wrote:
> > On 11/08/2013 10:19 PM, Charles Keepax wrote:
> > > Improve readability by creating a define for each microphone detection
> > > level.
> > > 
> > > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > > ---
> > >  drivers/extcon/extcon-arizona.c       |   21 ++++++++++++++++-----
> > >  include/linux/mfd/arizona/registers.h |    9 +++++++++
> > >  2 files changed, 25 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
> > > index 3c55ec8..6d914ba 100644
> > > --- a/drivers/extcon/extcon-arizona.c
> > > +++ b/drivers/extcon/extcon-arizona.c
> > > @@ -44,6 +44,17 @@
> > >  #define HPDET_DEBOUNCE 500
> > >  #define DEFAULT_MICD_TIMEOUT 2000
> > >  
> > > +enum {
> > > +	MICD_LVL_1_TO_7 = ARIZONA_MICD_LVL_1 | ARIZONA_MICD_LVL_2 |
> > > +			  ARIZONA_MICD_LVL_3 | ARIZONA_MICD_LVL_4 |
> > > +			  ARIZONA_MICD_LVL_5 | ARIZONA_MICD_LVL_6 |
> > > +			  ARIZONA_MICD_LVL_7,
> > > +
> > > +	MICD_LVL_0_TO_7 = ARIZONA_MICD_LVL_0 | MICD_LVL_1_TO_7,
> > > +
> > > +	MICD_LVL_0_TO_8 = MICD_LVL_0_TO_7 | ARIZONA_MICD_LVL_8,
> > > +};
> > 
> > MICD_LVL_1_TO_7 / MICD_LVL_0_TO_7 /MICD_LVL_0_TO_8 haven't the sequential value.
> > I prefer '#define' keyword to define MICD_LVL_1_TO_7 / MICD_LVL_0_TO_7 /MICD_LVL_0_TO_8
> > instead of enum keyword.


I will do an incremental patch to update them to defines.


> Actually Charles has already sent me this patch and I applied it a
> while ago.
> 
> I'm inclined to agree with you though, so if you want to send a patch
> based on v3.14-rc1 I'd be happy to accept it.


Apologies for causing confusion here I checked your tree for the
patch and didn't see it, so I assumed you had decided not to
apply it. I must have missed it some how.

Thanks,
Charles

Re: [PATCH 1/4] extcon: arizona: Add defines for microphone detection levels

[Lee Jones]

On Mon, 11 Nov 2013, Charles Keepax wrote:

> On Mon, Nov 11, 2013 at 10:53:56AM +0000, Lee Jones wrote:
> > > On 11/08/2013 10:19 PM, Charles Keepax wrote:
> > > > Improve readability by creating a define for each microphone detection
> > > > level.
> > > > 
> > > > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > > > ---
> > > >  drivers/extcon/extcon-arizona.c       |   21 ++++++++++++++++-----
> > > >  include/linux/mfd/arizona/registers.h |    9 +++++++++
> > > >  2 files changed, 25 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
> > > > index 3c55ec8..6d914ba 100644
> > > > --- a/drivers/extcon/extcon-arizona.c
> > > > +++ b/drivers/extcon/extcon-arizona.c
> > > > @@ -44,6 +44,17 @@
> > > >  #define HPDET_DEBOUNCE 500
> > > >  #define DEFAULT_MICD_TIMEOUT 2000
> > > >  
> > > > +enum {
> > > > +	MICD_LVL_1_TO_7 = ARIZONA_MICD_LVL_1 | ARIZONA_MICD_LVL_2 |
> > > > +			  ARIZONA_MICD_LVL_3 | ARIZONA_MICD_LVL_4 |
> > > > +			  ARIZONA_MICD_LVL_5 | ARIZONA_MICD_LVL_6 |
> > > > +			  ARIZONA_MICD_LVL_7,
> > > > +
> > > > +	MICD_LVL_0_TO_7 = ARIZONA_MICD_LVL_0 | MICD_LVL_1_TO_7,
> > > > +
> > > > +	MICD_LVL_0_TO_8 = MICD_LVL_0_TO_7 | ARIZONA_MICD_LVL_8,
> > > > +};
> > > 
> > > MICD_LVL_1_TO_7 / MICD_LVL_0_TO_7 /MICD_LVL_0_TO_8 haven't the sequential value.
> > > I prefer '#define' keyword to define MICD_LVL_1_TO_7 / MICD_LVL_0_TO_7 /MICD_LVL_0_TO_8
> > > instead of enum keyword.
> 
> 
> I will do an incremental patch to update them to defines.
> 
> 
> > Actually Charles has already sent me this patch and I applied it a
> > while ago.
> > 
> > I'm inclined to agree with you though, so if you want to send a patch
> > based on v3.14-rc1 I'd be happy to accept it.
> 
> Apologies for causing confusion here I checked your tree for the
> patch and didn't see it, so I assumed you had decided not to
> apply it. I must have missed it some how.

So I am partly to blame here. My public repos sometimes aren't as
up-to-date and my private ones, but if I've replied to a patch and
said I've applied it, I inevitably have.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/4] extcon: arizona: Add defines for microphone detection levels

[Lee Jones]

On Mon, 11 Nov 2013, Chanwoo Choi wrote:

> On 11/11/2013 07:53 PM, Lee Jones wrote:
> >> On 11/08/2013 10:19 PM, Charles Keepax wrote:
> >>> Improve readability by creating a define for each microphone detection
> >>> level.
> >>>
> >>> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> >>> ---
> >>>  drivers/extcon/extcon-arizona.c       |   21 ++++++++++++++++-----
> >>>  include/linux/mfd/arizona/registers.h |    9 +++++++++
> >>>  2 files changed, 25 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
> >>> index 3c55ec8..6d914ba 100644
> >>> --- a/drivers/extcon/extcon-arizona.c
> >>> +++ b/drivers/extcon/extcon-arizona.c
> >>> @@ -44,6 +44,17 @@
> >>>  #define HPDET_DEBOUNCE 500
> >>>  #define DEFAULT_MICD_TIMEOUT 2000
> >>>  
> >>> +enum {
> >>> +	MICD_LVL_1_TO_7 = ARIZONA_MICD_LVL_1 | ARIZONA_MICD_LVL_2 |
> >>> +			  ARIZONA_MICD_LVL_3 | ARIZONA_MICD_LVL_4 |
> >>> +			  ARIZONA_MICD_LVL_5 | ARIZONA_MICD_LVL_6 |
> >>> +			  ARIZONA_MICD_LVL_7,
> >>> +
> >>> +	MICD_LVL_0_TO_7 = ARIZONA_MICD_LVL_0 | MICD_LVL_1_TO_7,
> >>> +
> >>> +	MICD_LVL_0_TO_8 = MICD_LVL_0_TO_7 | ARIZONA_MICD_LVL_8,
> >>> +};
> >>
> >> MICD_LVL_1_TO_7 / MICD_LVL_0_TO_7 /MICD_LVL_0_TO_8 haven't the sequential value.
> >> I prefer '#define' keyword to define MICD_LVL_1_TO_7 / MICD_LVL_0_TO_7 /MICD_LVL_0_TO_8
> >> instead of enum keyword.
> > 
> > Actually Charles has already sent me this patch and I applied it a
> > while ago.
> 
> Why did you applied extcon driver patch on your tree?

Good point, well presented!

I should have just Acked the MFD parts in this case.

It's not too late to pull it back - I'll do just that.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/4] extcon: arizona: Add defines for microphone detection levels

[Chanwoo Choi]

On 11/11/2013 08:15 PM, Lee Jones wrote:
> On Mon, 11 Nov 2013, Charles Keepax wrote:
> 
>> On Mon, Nov 11, 2013 at 10:53:56AM +0000, Lee Jones wrote:
>>>> On 11/08/2013 10:19 PM, Charles Keepax wrote:
>>>>> Improve readability by creating a define for each microphone detection
>>>>> level.
>>>>>
>>>>> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
>>>>> ---
>>>>>  drivers/extcon/extcon-arizona.c       |   21 ++++++++++++++++-----
>>>>>  include/linux/mfd/arizona/registers.h |    9 +++++++++
>>>>>  2 files changed, 25 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
>>>>> index 3c55ec8..6d914ba 100644
>>>>> --- a/drivers/extcon/extcon-arizona.c
>>>>> +++ b/drivers/extcon/extcon-arizona.c
>>>>> @@ -44,6 +44,17 @@
>>>>>  #define HPDET_DEBOUNCE 500
>>>>>  #define DEFAULT_MICD_TIMEOUT 2000
>>>>>  
>>>>> +enum {
>>>>> +	MICD_LVL_1_TO_7 = ARIZONA_MICD_LVL_1 | ARIZONA_MICD_LVL_2 |
>>>>> +			  ARIZONA_MICD_LVL_3 | ARIZONA_MICD_LVL_4 |
>>>>> +			  ARIZONA_MICD_LVL_5 | ARIZONA_MICD_LVL_6 |
>>>>> +			  ARIZONA_MICD_LVL_7,
>>>>> +
>>>>> +	MICD_LVL_0_TO_7 = ARIZONA_MICD_LVL_0 | MICD_LVL_1_TO_7,
>>>>> +
>>>>> +	MICD_LVL_0_TO_8 = MICD_LVL_0_TO_7 | ARIZONA_MICD_LVL_8,
>>>>> +};
>>>>
>>>> MICD_LVL_1_TO_7 / MICD_LVL_0_TO_7 /MICD_LVL_0_TO_8 haven't the sequential value.
>>>> I prefer '#define' keyword to define MICD_LVL_1_TO_7 / MICD_LVL_0_TO_7 /MICD_LVL_0_TO_8
>>>> instead of enum keyword.
>>
>>
>> I will do an incremental patch to update them to defines.
>>
>>
>>> Actually Charles has already sent me this patch and I applied it a
>>> while ago.
>>>
>>> I'm inclined to agree with you though, so if you want to send a patch
>>> based on v3.14-rc1 I'd be happy to accept it.
>>
>> Apologies for causing confusion here I checked your tree for the
>> patch and didn't see it, so I assumed you had decided not to
>> apply it. I must have missed it some how.
> 
> So I am partly to blame here. My public repos sometimes aren't as
> up-to-date and my private ones, but if I've replied to a patch and
> said I've applied it, I inevitably have.
> 

But this patch has only the dependency of extcon subsystem.
At least, you have to get 'Acked-by' or 'Signed-off-by' from
subsystem maintainer. I didn't agree applying this patch on mainline tree.

Re: [PATCH 1/4] extcon: arizona: Add defines for microphone detection levels

[Lee Jones]

> >>> I'm inclined to agree with you though, so if you want to send a patch
> >>> based on v3.14-rc1 I'd be happy to accept it.
> >>
> >> Apologies for causing confusion here I checked your tree for the
> >> patch and didn't see it, so I assumed you had decided not to
> >> apply it. I must have missed it some how.
> > 
> > So I am partly to blame here. My public repos sometimes aren't as
> > up-to-date and my private ones, but if I've replied to a patch and
> > said I've applied it, I inevitably have.
> > 
> But this patch has only the dependency of extcon subsystem.
> At least, you have to get 'Acked-by' or 'Signed-off-by' from
> subsystem maintainer. I didn't agree applying this patch on mainline tree.

Please see my other mail.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/4] extcon: arizona: Add defines for microphone detection levels

[Lee Jones]

On Fri, 08 Nov 2013, Charles Keepax wrote:

> Improve readability by creating a define for each microphone detection
> level.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---
>  drivers/extcon/extcon-arizona.c       |   21 ++++++++++++++++-----
>  include/linux/mfd/arizona/registers.h |    9 +++++++++
>  2 files changed, 25 insertions(+), 5 deletions(-)

For the MFD parts:
  Acked-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/4] extcon: arizona: Add defines for microphone detection levels

[Chanwoo Choi]

On 11/11/2013 08:19 PM, Lee Jones wrote:
> On Mon, 11 Nov 2013, Chanwoo Choi wrote:
> 
>> On 11/11/2013 07:53 PM, Lee Jones wrote:
>>>> On 11/08/2013 10:19 PM, Charles Keepax wrote:
>>>>> Improve readability by creating a define for each microphone detection
>>>>> level.
>>>>>
>>>>> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
>>>>> ---
>>>>>  drivers/extcon/extcon-arizona.c       |   21 ++++++++++++++++-----
>>>>>  include/linux/mfd/arizona/registers.h |    9 +++++++++
>>>>>  2 files changed, 25 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
>>>>> index 3c55ec8..6d914ba 100644
>>>>> --- a/drivers/extcon/extcon-arizona.c
>>>>> +++ b/drivers/extcon/extcon-arizona.c
>>>>> @@ -44,6 +44,17 @@
>>>>>  #define HPDET_DEBOUNCE 500
>>>>>  #define DEFAULT_MICD_TIMEOUT 2000
>>>>>  
>>>>> +enum {
>>>>> +	MICD_LVL_1_TO_7 = ARIZONA_MICD_LVL_1 | ARIZONA_MICD_LVL_2 |
>>>>> +			  ARIZONA_MICD_LVL_3 | ARIZONA_MICD_LVL_4 |
>>>>> +			  ARIZONA_MICD_LVL_5 | ARIZONA_MICD_LVL_6 |
>>>>> +			  ARIZONA_MICD_LVL_7,
>>>>> +
>>>>> +	MICD_LVL_0_TO_7 = ARIZONA_MICD_LVL_0 | MICD_LVL_1_TO_7,
>>>>> +
>>>>> +	MICD_LVL_0_TO_8 = MICD_LVL_0_TO_7 | ARIZONA_MICD_LVL_8,
>>>>> +};
>>>>
>>>> MICD_LVL_1_TO_7 / MICD_LVL_0_TO_7 /MICD_LVL_0_TO_8 haven't the sequential value.
>>>> I prefer '#define' keyword to define MICD_LVL_1_TO_7 / MICD_LVL_0_TO_7 /MICD_LVL_0_TO_8
>>>> instead of enum keyword.
>>>
>>> Actually Charles has already sent me this patch and I applied it a
>>> while ago.
>>
>> Why did you applied extcon driver patch on your tree?
> 
> Good point, well presented!
> 
> I should have just Acked the MFD parts in this case.
> 

OK, I understand.

Thanks,
Chanwoo Choi

Re: [PATCH 4/4] extcon: arizona: Eliminate dead error handling code

[Chanwoo Choi]

Hi CHarles,

On 11/08/2013 10:19 PM, Charles Keepax wrote:
> As a small disclaimer I would personally prefer to not merge this patch.
> I have added it based on previous code review of the other patches in
> this chain.
> 
> arizona_hpdet_do_id currently can only return 0 or -EAGAIN making the
> else if clause handling error codes redundant, this patch removes this
> clause.
> 
> Whilst this clause is not currently hit removing it makes the code
> fragile. It will not be obvious whilst editing arizona_hpdet_do_id that
> you shouldn't add a return value other than 0 or -EAGAIN.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---
>  drivers/extcon/extcon-arizona.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
> index 0d70bf6..2313b1e 100644
> --- a/drivers/extcon/extcon-arizona.c
> +++ b/drivers/extcon/extcon-arizona.c
> @@ -476,6 +476,9 @@ static int arizona_hpdet_read(struct arizona_extcon_info *info)
>  	return val;
>  }
>  
> +/* This function should only return 0 or -EAGAIN, if other return values are
> + * added additional handling should be added in arizona_hpdet_irq.
> + */

As Lee Jones commented, you should modify this comment of arizona_hpdet_do_id() and add
the description of return value. Because arizoa_hpdet_do_id() has different meaning
between -EAGAIN and other minus value.

>  static int arizona_hpdet_do_id(struct arizona_extcon_info *info, int *reading,
>  			       bool *mic)
>  {
> @@ -591,8 +594,6 @@ static irqreturn_t arizona_hpdet_irq(int irq, void *data)
>  	ret = arizona_hpdet_do_id(info, &reading, &mic);
>  	if (ret == -EAGAIN)
>  		goto out;
> -	else if (ret < 0)
> -		goto done;
>  
>  	/* Report high impedence cables as line outputs */
>  	if (reading >= 5000)
> 

Thanks,
Chanwoo Choi

Re: [PATCH 1/4] extcon: arizona: Add defines for microphone detection levels

[Charles Keepax]

On Mon, Nov 11, 2013 at 11:26:38AM +0000, Lee Jones wrote:
> > >>> I'm inclined to agree with you though, so if you want to send a patch
> > >>> based on v3.14-rc1 I'd be happy to accept it.
> > >>
> > >> Apologies for causing confusion here I checked your tree for the
> > >> patch and didn't see it, so I assumed you had decided not to
> > >> apply it. I must have missed it some how.
> > > 
> > > So I am partly to blame here. My public repos sometimes aren't as
> > > up-to-date and my private ones, but if I've replied to a patch and
> > > said I've applied it, I inevitably have.
> > > 
> > But this patch has only the dependency of extcon subsystem.
> > At least, you have to get 'Acked-by' or 'Signed-off-by' from
> > subsystem maintainer. I didn't agree applying this patch on mainline tree.
> 
> Please see my other mail.

Sorry for the sake of clarity, we are backing out the patch and I
should send a fresh one?

Thanks,
Charles

Re: [PATCH 4/4] extcon: arizona: Eliminate dead error handling code

[Charles Keepax]

On Tue, Nov 12, 2013 at 09:15:29AM +0900, Chanwoo Choi wrote:
> Hi CHarles,
> 
> On 11/08/2013 10:19 PM, Charles Keepax wrote:
> > As a small disclaimer I would personally prefer to not merge this patch.
> > I have added it based on previous code review of the other patches in
> > this chain.
> > 
> > arizona_hpdet_do_id currently can only return 0 or -EAGAIN making the
> > else if clause handling error codes redundant, this patch removes this
> > clause.
> > 
> > Whilst this clause is not currently hit removing it makes the code
> > fragile. It will not be obvious whilst editing arizona_hpdet_do_id that
> > you shouldn't add a return value other than 0 or -EAGAIN.
> > 
> > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > ---
> >  drivers/extcon/extcon-arizona.c |    5 +++--
> >  1 files changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
> > index 0d70bf6..2313b1e 100644
> > --- a/drivers/extcon/extcon-arizona.c
> > +++ b/drivers/extcon/extcon-arizona.c
> > @@ -476,6 +476,9 @@ static int arizona_hpdet_read(struct arizona_extcon_info *info)
> >  	return val;
> >  }
> >  
> > +/* This function should only return 0 or -EAGAIN, if other return values are
> > + * added additional handling should be added in arizona_hpdet_irq.
> > + */
> 
> As Lee Jones commented, you should modify this comment of arizona_hpdet_do_id() and add
> the description of return value. Because arizoa_hpdet_do_id() has different meaning
> between -EAGAIN and other minus value.

I take it you are still very keen on applying a patch for this
dead code elimination? I really do feel it would be better to
leave this part of the code as it currently is, the extra safety
clearly outweights the cost of a redundant else if.

Thanks,
Charles

Re: [PATCH 1/4] extcon: arizona: Add defines for microphone detection levels

[Lee Jones]

On Tue, 12 Nov 2013, Charles Keepax wrote:

> On Mon, Nov 11, 2013 at 11:26:38AM +0000, Lee Jones wrote:
> > > >>> I'm inclined to agree with you though, so if you want to send a patch
> > > >>> based on v3.14-rc1 I'd be happy to accept it.
> > > >>
> > > >> Apologies for causing confusion here I checked your tree for the
> > > >> patch and didn't see it, so I assumed you had decided not to
> > > >> apply it. I must have missed it some how.
> > > > 
> > > > So I am partly to blame here. My public repos sometimes aren't as
> > > > up-to-date and my private ones, but if I've replied to a patch and
> > > > said I've applied it, I inevitably have.
> > > > 
> > > But this patch has only the dependency of extcon subsystem.
> > > At least, you have to get 'Acked-by' or 'Signed-off-by' from
> > > subsystem maintainer. I didn't agree applying this patch on mainline tree.
> > 
> > Please see my other mail.
> 
> Sorry for the sake of clarity, we are backing out the patch and I
> should send a fresh one?

I don't think there is a requirement to resend, but Chanwoo is right,
I can't take this patch. It's up to him.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 4/4] extcon: arizona: Eliminate dead error handling code

[Chanwoo Choi]

On 11/12/2013 11:30 PM, Charles Keepax wrote:
> On Tue, Nov 12, 2013 at 09:15:29AM +0900, Chanwoo Choi wrote:
>> Hi CHarles,
>>
>> On 11/08/2013 10:19 PM, Charles Keepax wrote:
>>> As a small disclaimer I would personally prefer to not merge this patch.
>>> I have added it based on previous code review of the other patches in
>>> this chain.
>>>
>>> arizona_hpdet_do_id currently can only return 0 or -EAGAIN making the
>>> else if clause handling error codes redundant, this patch removes this
>>> clause.
>>>
>>> Whilst this clause is not currently hit removing it makes the code
>>> fragile. It will not be obvious whilst editing arizona_hpdet_do_id that
>>> you shouldn't add a return value other than 0 or -EAGAIN.
>>>
>>> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
>>> ---
>>>  drivers/extcon/extcon-arizona.c |    5 +++--
>>>  1 files changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
>>> index 0d70bf6..2313b1e 100644
>>> --- a/drivers/extcon/extcon-arizona.c
>>> +++ b/drivers/extcon/extcon-arizona.c
>>> @@ -476,6 +476,9 @@ static int arizona_hpdet_read(struct arizona_extcon_info *info)
>>>  	return val;
>>>  }
>>>  
>>> +/* This function should only return 0 or -EAGAIN, if other return values are
>>> + * added additional handling should be added in arizona_hpdet_irq.
>>> + */
>>
>> As Lee Jones commented, you should modify this comment of arizona_hpdet_do_id() and add
>> the description of return value. Because arizoa_hpdet_do_id() has different meaning
>> between -EAGAIN and other minus value.
> 
> I take it you are still very keen on applying a patch for this
> dead code elimination? I really do feel it would be better to
> leave this part of the code as it currently is, the extra safety
> clearly outweights the cost of a redundant else if.

In that case, I think that you could modify arizona_hpdet_read() to add exception handling
for minus return value instead of elimination.

I knew about your opinion. But I think it isn't proper method remaining dead code on mainline tree.
People who don't know the history about this dead code would think that "Is this code necessary?".

Thanks,
Chanwoo Choi