[PATCH v2 0/5] Add basic microphone detection bindings

[PATCH v2 0/5] Add basic microphone detection bindings

[Charles Keepax]

This series adds basic microphone detection DT/ACPI bindings
using the device properties API.

The MFD parts and the extcon parts can go seperately through
there respective trees, but I sent them together so everyone
can see what is going on. There is no build dependency
although the MFD patch will fix a couple of Sparse warnings
that you get if using -Wsparse-all.

Changes since v1:
 - Removed double include of property.h

Thanks,
Charles

Charles Keepax (5):
  extcon: arizona: Update to use the new device properties API
  extcon: arizona: Add basic microphone detection DT/ACPI bindings
  extcon: arizona: Convert to gpiod
  mfd: arizona: Update DT doc for new mic detection bindings
  mfd: arizona: Update several pdata members to unsigned

 Documentation/devicetree/bindings/mfd/arizona.txt |   16 +++++
 drivers/extcon/extcon-arizona.c                   |   73 ++++++++++++++++++--
 include/linux/mfd/arizona/pdata.h                 |   12 ++--
 3 files changed, 87 insertions(+), 14 deletions(-)

-- 
1.7.2.5

[PATCH v2 1/5] extcon: arizona: Update to use the new device properties API

[Charles Keepax]

The device properties API will load data from both device tree and ACPI,
update the binding to use this API instead of the OF API.

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

diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
index 9262b45..1153929 100644
--- a/drivers/extcon/extcon-arizona.c
+++ b/drivers/extcon/extcon-arizona.c
@@ -24,6 +24,7 @@
 #include <linux/input.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/property.h>
 #include <linux/regulator/consumer.h>
 #include <linux/extcon.h>
 
@@ -1110,12 +1111,12 @@ static void arizona_micd_set_level(struct arizona *arizona, int index,
 	regmap_update_bits(arizona->regmap, reg, mask, level);
 }
 
-static int arizona_extcon_of_get_pdata(struct arizona *arizona)
+static int arizona_extcon_device_get_pdata(struct arizona *arizona)
 {
 	struct arizona_pdata *pdata = &arizona->pdata;
 	unsigned int val = ARIZONA_ACCDET_MODE_HPL;
 
-	of_property_read_u32(arizona->dev->of_node, "wlf,hpdet-channel", &val);
+	device_property_read_u32(arizona->dev, "wlf,hpdet-channel", &val);
 	switch (val) {
 	case ARIZONA_ACCDET_MODE_HPL:
 	case ARIZONA_ACCDET_MODE_HPR:
@@ -1149,7 +1150,7 @@ static int arizona_extcon_probe(struct platform_device *pdev)
 
 	if (IS_ENABLED(CONFIG_OF)) {
 		if (!dev_get_platdata(arizona->dev))
-			arizona_extcon_of_get_pdata(arizona);
+			arizona_extcon_device_get_pdata(arizona);
 	}
 
 	info->micvdd = devm_regulator_get(&pdev->dev, "MICVDD");
-- 
1.7.2.5

Re: [PATCH v2 1/5] extcon: arizona: Update to use the new device properties API

[Chanwoo Choi]

Hi Charles,

Looks good to me about this patch. But I add one comment.

On Thu, Jun 18, 2015 at 11:43 PM, Charles Keepax
<ckeepax@opensource.wolfsonmicro.com> wrote:
> The device properties API will load data from both device tree and ACPI,
> update the binding to use this API instead of the OF API.
>
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---
>  drivers/extcon/extcon-arizona.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
> index 9262b45..1153929 100644
> --- a/drivers/extcon/extcon-arizona.c
> +++ b/drivers/extcon/extcon-arizona.c
> @@ -24,6 +24,7 @@
>  #include <linux/input.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/property.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/extcon.h>
>
> @@ -1110,12 +1111,12 @@ static void arizona_micd_set_level(struct arizona *arizona, int index,
>         regmap_update_bits(arizona->regmap, reg, mask, level);
>  }
>
> -static int arizona_extcon_of_get_pdata(struct arizona *arizona)
> +static int arizona_extcon_device_get_pdata(struct arizona *arizona)
>  {
>         struct arizona_pdata *pdata = &arizona->pdata;
>         unsigned int val = ARIZONA_ACCDET_MODE_HPL;
>
> -       of_property_read_u32(arizona->dev->of_node, "wlf,hpdet-channel", &val);
> +       device_property_read_u32(arizona->dev, "wlf,hpdet-channel", &val);
>         switch (val) {
>         case ARIZONA_ACCDET_MODE_HPL:
>         case ARIZONA_ACCDET_MODE_HPR:
> @@ -1149,7 +1150,7 @@ static int arizona_extcon_probe(struct platform_device *pdev)
>
>         if (IS_ENABLED(CONFIG_OF)) {

If you remain the upper if statement, I think
"device_property_read_u32" function cannot get the data from ACPI. I
think that you can remove upper if statement of "CONFIG_OF".

>                 if (!dev_get_platdata(arizona->dev))
> -                       arizona_extcon_of_get_pdata(arizona);
> +                       arizona_extcon_device_get_pdata(arizona);
>         }
>
>         info->micvdd = devm_regulator_get(&pdev->dev, "MICVDD");

Thanks,
Chanwoo Choi

[PATCH v2 2/5] extcon: arizona: Add basic microphone detection DT/ACPI bindings

[Charles Keepax]

This patch adds bindings for the basic microphone detection platform
data.

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

diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
index 1153929..5bf1b19 100644
--- a/drivers/extcon/extcon-arizona.c
+++ b/drivers/extcon/extcon-arizona.c
@@ -1115,6 +1115,7 @@ static int arizona_extcon_device_get_pdata(struct arizona *arizona)
 {
 	struct arizona_pdata *pdata = &arizona->pdata;
 	unsigned int val = ARIZONA_ACCDET_MODE_HPL;
+	unsigned int dbtime;
 
 	device_property_read_u32(arizona->dev, "wlf,hpdet-channel", &val);
 	switch (val) {
@@ -1128,6 +1129,34 @@ static int arizona_extcon_device_get_pdata(struct arizona *arizona)
 		pdata->hpdet_channel = ARIZONA_ACCDET_MODE_HPL;
 	}
 
+	device_property_read_u32(arizona->dev, "wlf,micd-detect-debounce",
+				 &pdata->micd_detect_debounce);
+
+	device_property_read_u32(arizona->dev, "wlf,micd-bias-start-time",
+				 &pdata->micd_bias_start_time);
+
+	device_property_read_u32(arizona->dev, "wlf,micd-rate",
+				 &pdata->micd_rate);
+
+	dbtime = 0;
+	device_property_read_u32(arizona->dev, "wlf,micd-dbtime", &dbtime);
+	switch (dbtime) {
+	case 2:
+		pdata->micd_dbtime = 0x10000;
+		break;
+	case 4:
+		pdata->micd_dbtime = 0x0001;
+		break;
+	default:
+		break;
+	}
+
+	device_property_read_u32(arizona->dev, "wlf,micd-timeout",
+				 &pdata->micd_timeout);
+
+	pdata->micd_force_micbias = device_property_read_bool(arizona->dev,
+						"wlf,micd-force-micbias");
+
 	return 0;
 }
 
-- 
1.7.2.5

Re: [PATCH v2 2/5] extcon: arizona: Add basic microphone detection DT/ACPI bindings

[Chanwoo Choi]

Hi Charles,

On Thu, Jun 18, 2015 at 11:43 PM, Charles Keepax
<ckeepax@opensource.wolfsonmicro.com> wrote:
> This patch adds bindings for the basic microphone detection platform
> data.
>
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---
>  drivers/extcon/extcon-arizona.c |   29 +++++++++++++++++++++++++++++
>  1 files changed, 29 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
> index 1153929..5bf1b19 100644
> --- a/drivers/extcon/extcon-arizona.c
> +++ b/drivers/extcon/extcon-arizona.c
> @@ -1115,6 +1115,7 @@ static int arizona_extcon_device_get_pdata(struct arizona *arizona)
>  {
>         struct arizona_pdata *pdata = &arizona->pdata;
>         unsigned int val = ARIZONA_ACCDET_MODE_HPL;
> +       unsigned int dbtime;
>
>         device_property_read_u32(arizona->dev, "wlf,hpdet-channel", &val);
>         switch (val) {
> @@ -1128,6 +1129,34 @@ static int arizona_extcon_device_get_pdata(struct arizona *arizona)
>                 pdata->hpdet_channel = ARIZONA_ACCDET_MODE_HPL;
>         }
>
> +       device_property_read_u32(arizona->dev, "wlf,micd-detect-debounce",
> +                                &pdata->micd_detect_debounce);
> +
> +       device_property_read_u32(arizona->dev, "wlf,micd-bias-start-time",
> +                                &pdata->micd_bias_start_time);
> +
> +       device_property_read_u32(arizona->dev, "wlf,micd-rate",
> +                                &pdata->micd_rate);
> +
> +       dbtime = 0;
> +       device_property_read_u32(arizona->dev, "wlf,micd-dbtime", &dbtime);
> +       switch (dbtime) {
> +       case 2:

I'd like you to define the constant variable for specific integer
value to improve the readability.
e.g.,

#define MICD_DBTIME_XXX 2
#define MICD_DBTIME_XXX 4

> +               pdata->micd_dbtime = 0x10000;

Also, you better to define the constant variable for "0x10000" to
improbe readability in the include/linux/mfd/arizona.h. If you just
use the hex value, the other developer cannot analyze the meaning of
"0x10000".

> +               break;
> +       case 4:

ditto.

> +               pdata->micd_dbtime = 0x0001;

ditto.

> +               break;
> +       default:
> +               break;
> +       }
> +
> +       device_property_read_u32(arizona->dev, "wlf,micd-timeout",
> +                                &pdata->micd_timeout);
> +
> +       pdata->micd_force_micbias = device_property_read_bool(arizona->dev,
> +                                               "wlf,micd-force-micbias");
> +
>         return 0;
>  }

Thanks,
Chanwoo Choi

Re: [PATCH v2 2/5] extcon: arizona: Add basic microphone detection DT/ACPI bindings

[Charles Keepax]

On Fri, Jun 19, 2015 at 11:25:57AM +0900, Chanwoo Choi wrote:
> Hi Charles,
> 
> On Thu, Jun 18, 2015 at 11:43 PM, Charles Keepax
> <ckeepax@opensource.wolfsonmicro.com> wrote:
> > This patch adds bindings for the basic microphone detection platform
> > data.
> >
> > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > ---
> > +       dbtime = 0;
> > +       device_property_read_u32(arizona->dev, "wlf,micd-dbtime", &dbtime);
> > +       switch (dbtime) {
> > +       case 2:
> 
> I'd like you to define the constant variable for specific integer
> value to improve the readability.
> e.g.,
> 
> #define MICD_DBTIME_XXX 2
> #define MICD_DBTIME_XXX 4
> 
> > +               pdata->micd_dbtime = 0x10000;
> 
> Also, you better to define the constant variable for "0x10000" to
> improbe readability in the include/linux/mfd/arizona.h. If you just
> use the hex value, the other developer cannot analyze the meaning of
> "0x10000".
> 
> > +               break;
> > +       case 4:
> 
> ditto.
> 

I am not sure these two really warrant a define the number in DT
indicates the number of debounces to perform.

#define MICD_DBTIME_TWO 2

switch (dbtime)
case MICD_DBTIME_TWO:

Is if anything less clear than just using 2 directly. I could
perhaps rename the varible to num_debounces instead?

Thanks,
Charles

Re: [PATCH v2 2/5] extcon: arizona: Add basic microphone detection DT/ACPI bindings

[Chanwoo Choi]

On Fri, Jun 19, 2015 at 5:17 PM, Charles Keepax
<ckeepax@opensource.wolfsonmicro.com> wrote:
> On Fri, Jun 19, 2015 at 11:25:57AM +0900, Chanwoo Choi wrote:
>> Hi Charles,
>>
>> On Thu, Jun 18, 2015 at 11:43 PM, Charles Keepax
>> <ckeepax@opensource.wolfsonmicro.com> wrote:
>> > This patch adds bindings for the basic microphone detection platform
>> > data.
>> >
>> > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
>> > ---
>> > +       dbtime = 0;
>> > +       device_property_read_u32(arizona->dev, "wlf,micd-dbtime", &dbtime);
>> > +       switch (dbtime) {
>> > +       case 2:
>>
>> I'd like you to define the constant variable for specific integer
>> value to improve the readability.
>> e.g.,
>>
>> #define MICD_DBTIME_XXX 2
>> #define MICD_DBTIME_XXX 4
>>
>> > +               pdata->micd_dbtime = 0x10000;
>>
>> Also, you better to define the constant variable for "0x10000" to
>> improbe readability in the include/linux/mfd/arizona.h. If you just
>> use the hex value, the other developer cannot analyze the meaning of
>> "0x10000".
>>
>> > +               break;
>> > +       case 4:
>>
>> ditto.
>>
>
> I am not sure these two really warrant a define the number in DT
> indicates the number of debounces to perform.
>
> #define MICD_DBTIME_TWO 2

No. I mean that you should define the appropriate name of definition
to indicate the meaning of 2 or 4. Just not 2 -> TWO.

2 is 2 second? or 2 is milli-second? or Is there any other meaning?
I can never know the meaning of '2' without any description and any
proper name of definition.

Thanks,
Chanwoo CHOi

Re: [PATCH v2 2/5] extcon: arizona: Add basic microphone detection DT/ACPI bindings

[Charles Keepax]

On Fri, Jun 19, 2015 at 05:28:11PM +0900, Chanwoo Choi wrote:
> On Fri, Jun 19, 2015 at 5:17 PM, Charles Keepax
> <ckeepax@opensource.wolfsonmicro.com> wrote:
> > On Fri, Jun 19, 2015 at 11:25:57AM +0900, Chanwoo Choi wrote:
> >> Hi Charles,
> >>
> >> On Thu, Jun 18, 2015 at 11:43 PM, Charles Keepax
> >> <ckeepax@opensource.wolfsonmicro.com> wrote:
> >> > This patch adds bindings for the basic microphone detection platform
> >> > data.
> >> >
> >> > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> >> > ---
> >> > +       dbtime = 0;
> >> > +       device_property_read_u32(arizona->dev, "wlf,micd-dbtime", &dbtime);
> >> > +       switch (dbtime) {
> >> > +       case 2:
> >>
> >> I'd like you to define the constant variable for specific integer
> >> value to improve the readability.
> >> e.g.,
> >>
> >> #define MICD_DBTIME_XXX 2
> >> #define MICD_DBTIME_XXX 4
> >>
> >> > +               pdata->micd_dbtime = 0x10000;
> >>
> >> Also, you better to define the constant variable for "0x10000" to
> >> improbe readability in the include/linux/mfd/arizona.h. If you just
> >> use the hex value, the other developer cannot analyze the meaning of
> >> "0x10000".
> >>
> >> > +               break;
> >> > +       case 4:
> >>
> >> ditto.
> >>
> >
> > I am not sure these two really warrant a define the number in DT
> > indicates the number of debounces to perform.
> >
> > #define MICD_DBTIME_TWO 2
> 
> No. I mean that you should define the appropriate name of definition
> to indicate the meaning of 2 or 4. Just not 2 -> TWO.
> 
> 2 is 2 second? or 2 is milli-second? or Is there any other meaning?
> I can never know the meaning of '2' without any description and any
> proper name of definition.

It is literally two, two debounces, I really can't see what I
could call the define. What do you think to the idea I suggested
to rename the variable dbtime -> num_debounces?

Thanks,
Charles

Re: [PATCH v2 2/5] extcon: arizona: Add basic microphone detection DT/ACPI bindings

[Chanwoo Choi]

On Fri, Jun 19, 2015 at 6:08 PM, Charles Keepax
<ckeepax@opensource.wolfsonmicro.com> wrote:
> On Fri, Jun 19, 2015 at 05:28:11PM +0900, Chanwoo Choi wrote:
>> On Fri, Jun 19, 2015 at 5:17 PM, Charles Keepax
>> <ckeepax@opensource.wolfsonmicro.com> wrote:
>> > On Fri, Jun 19, 2015 at 11:25:57AM +0900, Chanwoo Choi wrote:
>> >> Hi Charles,
>> >>
>> >> On Thu, Jun 18, 2015 at 11:43 PM, Charles Keepax
>> >> <ckeepax@opensource.wolfsonmicro.com> wrote:
>> >> > This patch adds bindings for the basic microphone detection platform
>> >> > data.
>> >> >
>> >> > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
>> >> > ---
>> >> > +       dbtime = 0;
>> >> > +       device_property_read_u32(arizona->dev, "wlf,micd-dbtime", &dbtime);
>> >> > +       switch (dbtime) {
>> >> > +       case 2:
>> >>
>> >> I'd like you to define the constant variable for specific integer
>> >> value to improve the readability.
>> >> e.g.,
>> >>
>> >> #define MICD_DBTIME_XXX 2
>> >> #define MICD_DBTIME_XXX 4
>> >>
>> >> > +               pdata->micd_dbtime = 0x10000;
>> >>
>> >> Also, you better to define the constant variable for "0x10000" to
>> >> improbe readability in the include/linux/mfd/arizona.h. If you just
>> >> use the hex value, the other developer cannot analyze the meaning of
>> >> "0x10000".
>> >>
>> >> > +               break;
>> >> > +       case 4:
>> >>
>> >> ditto.
>> >>
>> >
>> > I am not sure these two really warrant a define the number in DT
>> > indicates the number of debounces to perform.
>> >
>> > #define MICD_DBTIME_TWO 2
>>
>> No. I mean that you should define the appropriate name of definition
>> to indicate the meaning of 2 or 4. Just not 2 -> TWO.
>>
>> 2 is 2 second? or 2 is milli-second? or Is there any other meaning?
>> I can never know the meaning of '2' without any description and any
>> proper name of definition.
>
> It is literally two, two debounces, I really can't see what I
> could call the define. What do you think to the idea I suggested
> to rename the variable dbtime -> num_debounces?

Is it either "debounce time" or "the number of debounce"?

Also,

When extcon-arizona driver update the bit for micd_dbtime, use the
ARIZONA_MICD_DBTIME_MASK (0x0002).

In following case, 0x10000 is larger than ARIZONA_MICD_DBTIME_MASK(0x0002).

          case 2:
                pdata->micd_dbtime = 0x10000;

Is it correct?

I thinkt that "arizona->pdata.micd_dbtime <<
ARIZONA_MICD_DBTIME_SHIFT" value should be included in the range of
ARIZONA_MICD_DBTIME_MASK(0x0002).

Thanks,
Chanwoo Choi

Re: [PATCH v2 2/5] extcon: arizona: Add basic microphone detection DT/ACPI bindings

[Charles Keepax]

On Fri, Jun 19, 2015 at 07:31:49PM +0900, Chanwoo Choi wrote:
> On Fri, Jun 19, 2015 at 6:08 PM, Charles Keepax
> <ckeepax@opensource.wolfsonmicro.com> wrote:
> > On Fri, Jun 19, 2015 at 05:28:11PM +0900, Chanwoo Choi wrote:
> >> On Fri, Jun 19, 2015 at 5:17 PM, Charles Keepax
> >> <ckeepax@opensource.wolfsonmicro.com> wrote:
> >> > On Fri, Jun 19, 2015 at 11:25:57AM +0900, Chanwoo Choi wrote:
> >> >> Hi Charles,
> >> >>
> >> >> On Thu, Jun 18, 2015 at 11:43 PM, Charles Keepax
> >> >> <ckeepax@opensource.wolfsonmicro.com> wrote:
> >> >> > This patch adds bindings for the basic microphone detection platform
> >> >> > data.
> >> >> >
> >> >> > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> >> >> > ---
> >> >> > +       dbtime = 0;
> >> >> > +       device_property_read_u32(arizona->dev, "wlf,micd-dbtime", &dbtime);
> >> >> > +       switch (dbtime) {
> >> >> > +       case 2:
> >> >>
> >> >> I'd like you to define the constant variable for specific integer
> >> >> value to improve the readability.
> >> >> e.g.,
> >> >>
> >> >> #define MICD_DBTIME_XXX 2
> >> >> #define MICD_DBTIME_XXX 4
> >> >>
> >> >> > +               pdata->micd_dbtime = 0x10000;
> >> >>
> >> >> Also, you better to define the constant variable for "0x10000" to
> >> >> improbe readability in the include/linux/mfd/arizona.h. If you just
> >> >> use the hex value, the other developer cannot analyze the meaning of
> >> >> "0x10000".
> >> >>
> >> >> > +               break;
> >> >> > +       case 4:
> >> >>
> >> >> ditto.
> >> >>
> >> >
> >> > I am not sure these two really warrant a define the number in DT
> >> > indicates the number of debounces to perform.
> >> >
> >> > #define MICD_DBTIME_TWO 2
> >>
> >> No. I mean that you should define the appropriate name of definition
> >> to indicate the meaning of 2 or 4. Just not 2 -> TWO.
> >>
> >> 2 is 2 second? or 2 is milli-second? or Is there any other meaning?
> >> I can never know the meaning of '2' without any description and any
> >> proper name of definition.
> >
> > It is literally two, two debounces, I really can't see what I
> > could call the define. What do you think to the idea I suggested
> > to rename the variable dbtime -> num_debounces?
> 
> Is it either "debounce time" or "the number of debounce"?
> 
> Also,
> 
> When extcon-arizona driver update the bit for micd_dbtime, use the
> ARIZONA_MICD_DBTIME_MASK (0x0002).
> 
> In following case, 0x10000 is larger than ARIZONA_MICD_DBTIME_MASK(0x0002).
> 
>           case 2:
>                 pdata->micd_dbtime = 0x10000;
> 
> Is it correct?
> 
> I thinkt that "arizona->pdata.micd_dbtime <<
> ARIZONA_MICD_DBTIME_SHIFT" value should be included in the range of
> ARIZONA_MICD_DBTIME_MASK(0x0002).

Basically this boils down to setting a single bit which chooses
between 2 and 4 debounces. The tricky thing is that the default
value for the pdata will be 0, and the value of the bit for 2
debounces will be 0. So inorder to show we have intentionally set
the pdata we set a value that is non-zero but won't get written
into the register. It is a little awkward but I don't want to
change how the pdata works as there are probably systems out of
tree that use it.

Thanks,
Charles

Re: [PATCH v2 2/5] extcon: arizona: Add basic microphone detection DT/ACPI bindings

[Chanwoo Choi]

On Fri, Jun 19, 2015 at 7:44 PM, Charles Keepax
<ckeepax@opensource.wolfsonmicro.com> wrote:
> On Fri, Jun 19, 2015 at 07:31:49PM +0900, Chanwoo Choi wrote:
>> On Fri, Jun 19, 2015 at 6:08 PM, Charles Keepax
>> <ckeepax@opensource.wolfsonmicro.com> wrote:
>> > On Fri, Jun 19, 2015 at 05:28:11PM +0900, Chanwoo Choi wrote:
>> >> On Fri, Jun 19, 2015 at 5:17 PM, Charles Keepax
>> >> <ckeepax@opensource.wolfsonmicro.com> wrote:
>> >> > On Fri, Jun 19, 2015 at 11:25:57AM +0900, Chanwoo Choi wrote:
>> >> >> Hi Charles,
>> >> >>
>> >> >> On Thu, Jun 18, 2015 at 11:43 PM, Charles Keepax
>> >> >> <ckeepax@opensource.wolfsonmicro.com> wrote:
>> >> >> > This patch adds bindings for the basic microphone detection platform
>> >> >> > data.
>> >> >> >
>> >> >> > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
>> >> >> > ---
>> >> >> > +       dbtime = 0;
>> >> >> > +       device_property_read_u32(arizona->dev, "wlf,micd-dbtime", &dbtime);
>> >> >> > +       switch (dbtime) {
>> >> >> > +       case 2:
>> >> >>
>> >> >> I'd like you to define the constant variable for specific integer
>> >> >> value to improve the readability.
>> >> >> e.g.,
>> >> >>
>> >> >> #define MICD_DBTIME_XXX 2
>> >> >> #define MICD_DBTIME_XXX 4
>> >> >>
>> >> >> > +               pdata->micd_dbtime = 0x10000;
>> >> >>
>> >> >> Also, you better to define the constant variable for "0x10000" to
>> >> >> improbe readability in the include/linux/mfd/arizona.h. If you just
>> >> >> use the hex value, the other developer cannot analyze the meaning of
>> >> >> "0x10000".
>> >> >>
>> >> >> > +               break;
>> >> >> > +       case 4:
>> >> >>
>> >> >> ditto.
>> >> >>
>> >> >
>> >> > I am not sure these two really warrant a define the number in DT
>> >> > indicates the number of debounces to perform.
>> >> >
>> >> > #define MICD_DBTIME_TWO 2
>> >>
>> >> No. I mean that you should define the appropriate name of definition
>> >> to indicate the meaning of 2 or 4. Just not 2 -> TWO.
>> >>
>> >> 2 is 2 second? or 2 is milli-second? or Is there any other meaning?
>> >> I can never know the meaning of '2' without any description and any
>> >> proper name of definition.
>> >
>> > It is literally two, two debounces, I really can't see what I
>> > could call the define. What do you think to the idea I suggested
>> > to rename the variable dbtime -> num_debounces?
>>
>> Is it either "debounce time" or "the number of debounce"?
>>
>> Also,
>>
>> When extcon-arizona driver update the bit for micd_dbtime, use the
>> ARIZONA_MICD_DBTIME_MASK (0x0002).
>>
>> In following case, 0x10000 is larger than ARIZONA_MICD_DBTIME_MASK(0x0002).
>>
>>           case 2:
>>                 pdata->micd_dbtime = 0x10000;
>>
>> Is it correct?
>>
>> I thinkt that "arizona->pdata.micd_dbtime <<
>> ARIZONA_MICD_DBTIME_SHIFT" value should be included in the range of
>> ARIZONA_MICD_DBTIME_MASK(0x0002).
>
> Basically this boils down to setting a single bit which chooses
> between 2 and 4 debounces. The tricky thing is that the default
> value for the pdata will be 0, and the value of the bit for 2
> debounces will be 0. So inorder to show we have intentionally set
> the pdata we set a value that is non-zero but won't get written
> into the register. It is a little awkward but I don't want to
> change how the pdata works as there are probably systems out of
> tree that use it.

This method cause the poor readabilidy of extcon-arizona.c.
If you don't explain this complicated reason of this code, me and
other developer will never understand the meaning of this code.

I don't agree your suggestion. you need other way.

Thanks,
Chanwoo Choi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/5] extcon: arizona: Add basic microphone detection DT/ACPI bindings

[Charles Keepax]

On Fri, Jun 19, 2015 at 08:03:37PM +0900, Chanwoo Choi wrote:
> On Fri, Jun 19, 2015 at 7:44 PM, Charles Keepax
> <ckeepax@opensource.wolfsonmicro.com> wrote:
> > On Fri, Jun 19, 2015 at 07:31:49PM +0900, Chanwoo Choi wrote:
> >> On Fri, Jun 19, 2015 at 6:08 PM, Charles Keepax
> >> <ckeepax@opensource.wolfsonmicro.com> wrote:
> >> > On Fri, Jun 19, 2015 at 05:28:11PM +0900, Chanwoo Choi wrote:
> >> >> On Fri, Jun 19, 2015 at 5:17 PM, Charles Keepax
> >> >> <ckeepax@opensource.wolfsonmicro.com> wrote:
> >> >> > On Fri, Jun 19, 2015 at 11:25:57AM +0900, Chanwoo Choi wrote:
> >> >> >> Hi Charles,
> >> >> >>
> >> >> >> On Thu, Jun 18, 2015 at 11:43 PM, Charles Keepax
> >> >> >> <ckeepax@opensource.wolfsonmicro.com> wrote:
> >> >> >> > This patch adds bindings for the basic microphone detection platform
> >> >> >> > data.
> >> >> >> >
> >> >> >> > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> >> >> >> > ---
> >> >> >> > +       dbtime = 0;
> >> >> >> > +       device_property_read_u32(arizona->dev, "wlf,micd-dbtime", &dbtime);
> >> >> >> > +       switch (dbtime) {
> >> >> >> > +       case 2:
> >> >> >>
> >> >> >> I'd like you to define the constant variable for specific integer
> >> >> >> value to improve the readability.
> >> >> >> e.g.,
> >> >> >>
> >> >> >> #define MICD_DBTIME_XXX 2
> >> >> >> #define MICD_DBTIME_XXX 4
> >> >> >>
> >> >> >> > +               pdata->micd_dbtime = 0x10000;
> >> >> >>
> >> >> >> Also, you better to define the constant variable for "0x10000" to
> >> >> >> improbe readability in the include/linux/mfd/arizona.h. If you just
> >> >> >> use the hex value, the other developer cannot analyze the meaning of
> >> >> >> "0x10000".
> >> >> >>
> >> >> >> > +               break;
> >> >> >> > +       case 4:
> >> >> >>
> >> >> >> ditto.
> >> >> >>
> >> >> >
> >> >> > I am not sure these two really warrant a define the number in DT
> >> >> > indicates the number of debounces to perform.
> >> >> >
> >> >> > #define MICD_DBTIME_TWO 2
> >> >>
> >> >> No. I mean that you should define the appropriate name of definition
> >> >> to indicate the meaning of 2 or 4. Just not 2 -> TWO.
> >> >>
> >> >> 2 is 2 second? or 2 is milli-second? or Is there any other meaning?
> >> >> I can never know the meaning of '2' without any description and any
> >> >> proper name of definition.
> >> >
> >> > It is literally two, two debounces, I really can't see what I
> >> > could call the define. What do you think to the idea I suggested
> >> > to rename the variable dbtime -> num_debounces?
> >>
> >> Is it either "debounce time" or "the number of debounce"?
> >>
> >> Also,
> >>
> >> When extcon-arizona driver update the bit for micd_dbtime, use the
> >> ARIZONA_MICD_DBTIME_MASK (0x0002).
> >>
> >> In following case, 0x10000 is larger than ARIZONA_MICD_DBTIME_MASK(0x0002).
> >>
> >>           case 2:
> >>                 pdata->micd_dbtime = 0x10000;
> >>
> >> Is it correct?
> >>
> >> I thinkt that "arizona->pdata.micd_dbtime <<
> >> ARIZONA_MICD_DBTIME_SHIFT" value should be included in the range of
> >> ARIZONA_MICD_DBTIME_MASK(0x0002).
> >
> > Basically this boils down to setting a single bit which chooses
> > between 2 and 4 debounces. The tricky thing is that the default
> > value for the pdata will be 0, and the value of the bit for 2
> > debounces will be 0. So inorder to show we have intentionally set
> > the pdata we set a value that is non-zero but won't get written
> > into the register. It is a little awkward but I don't want to
> > change how the pdata works as there are probably systems out of
> > tree that use it.
> 
> This method cause the poor readabilidy of extcon-arizona.c.
> If you don't explain this complicated reason of this code, me and
> other developer will never understand the meaning of this code.
> 
> I don't agree your suggestion. you need other way.

Well alright if you are sure, I guess if they are out of tree
anyway we don't really care whether we break their code from
mainline. I will respin to change the symantics of the pdata to
be more sensible.

Thanks,
Charles
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 3/5] extcon: arizona: Convert to gpiod

[Charles Keepax]

Convert to using the newer gpiod interface for the micd_pol_gpio.
Although we still carry support for the old gpio interface from pdata.

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

diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
index 5bf1b19..71d60ba 100644
--- a/drivers/extcon/extcon-arizona.c
+++ b/drivers/extcon/extcon-arizona.c
@@ -20,6 +20,7 @@
 #include <linux/slab.h>
 #include <linux/interrupt.h>
 #include <linux/err.h>
+#include <linux/gpio/consumer.h>
 #include <linux/gpio.h>
 #include <linux/input.h>
 #include <linux/platform_device.h>
@@ -95,6 +96,8 @@ struct arizona_extcon_info {
 	int hpdet_ip_version;
 
 	struct extcon_dev *edev;
+
+	struct gpio_desc *micd_pol_gpio;
 };
 
 static const struct arizona_micd_config micd_default_modes[] = {
@@ -205,6 +208,10 @@ static void arizona_extcon_set_mode(struct arizona_extcon_info *info, int mode)
 	if (arizona->pdata.micd_pol_gpio > 0)
 		gpio_set_value_cansleep(arizona->pdata.micd_pol_gpio,
 					info->micd_modes[mode].gpio);
+	else if (info->micd_pol_gpio)
+		gpiod_set_value_cansleep(info->micd_pol_gpio,
+					 info->micd_modes[mode].gpio);
+
 	regmap_update_bits(arizona->regmap, ARIZONA_MIC_DETECT_1,
 			   ARIZONA_MICD_BIAS_SRC_MASK,
 			   info->micd_modes[mode].bias <<
@@ -1271,6 +1278,22 @@ static int arizona_extcon_probe(struct platform_device *pdev)
 				arizona->pdata.micd_pol_gpio, ret);
 			goto err_register;
 		}
+	} else {
+		if (info->micd_modes[0].gpio)
+			mode = GPIOD_OUT_HIGH;
+		else
+			mode = GPIOD_OUT_LOW;
+
+		info->micd_pol_gpio = gpiod_get_optional(arizona->dev,
+							 "wlf,micd-pol",
+							 GPIOD_OUT_LOW);
+		if (IS_ERR(info->micd_pol_gpio)) {
+			ret = PTR_ERR(info->micd_pol_gpio);
+			dev_err(arizona->dev,
+				"Failed to get microphone polarity GPIO: %d\n",
+				ret);
+			goto err_register;
+		}
 	}
 
 	if (arizona->pdata.hpdet_id_gpio > 0) {
@@ -1281,7 +1304,7 @@ static int arizona_extcon_probe(struct platform_device *pdev)
 		if (ret != 0) {
 			dev_err(arizona->dev, "Failed to request GPIO%d: %d\n",
 				arizona->pdata.hpdet_id_gpio, ret);
-			goto err_register;
+			goto err_gpio;
 		}
 	}
 
@@ -1325,7 +1348,7 @@ static int arizona_extcon_probe(struct platform_device *pdev)
 				dev_err(arizona->dev,
 					"MICD ranges must be sorted\n");
 				ret = -EINVAL;
-				goto err_input;
+				goto err_gpio;
 			}
 		}
 	}
@@ -1344,7 +1367,7 @@ static int arizona_extcon_probe(struct platform_device *pdev)
 			dev_err(arizona->dev, "Unsupported MICD level %d\n",
 				info->micd_ranges[i].max);
 			ret = -EINVAL;
-			goto err_input;
+			goto err_gpio;
 		}
 
 		dev_dbg(arizona->dev, "%d ohms for MICD threshold %d\n",
@@ -1417,7 +1440,7 @@ static int arizona_extcon_probe(struct platform_device *pdev)
 	if (ret != 0) {
 		dev_err(&pdev->dev, "Failed to get JACKDET rise IRQ: %d\n",
 			ret);
-		goto err_input;
+		goto err_gpio;
 	}
 
 	ret = arizona_set_irq_wake(arizona, jack_irq_rise, 1);
@@ -1488,7 +1511,8 @@ err_rise_wake:
 	arizona_set_irq_wake(arizona, jack_irq_rise, 0);
 err_rise:
 	arizona_free_irq(arizona, jack_irq_rise, info);
-err_input:
+err_gpio:
+	gpiod_put(info->micd_pol_gpio);
 err_register:
 	pm_runtime_disable(&pdev->dev);
 	return ret;
@@ -1500,6 +1524,8 @@ static int arizona_extcon_remove(struct platform_device *pdev)
 	struct arizona *arizona = info->arizona;
 	int jack_irq_rise, jack_irq_fall;
 
+	gpiod_put(info->micd_pol_gpio);
+
 	pm_runtime_disable(&pdev->dev);
 
 	regmap_update_bits(arizona->regmap,
-- 
1.7.2.5

Re: [PATCH v2 3/5] extcon: arizona: Convert to gpiod

[Chanwoo Choi]

Hi Charles,

On Thu, Jun 18, 2015 at 11:43 PM, Charles Keepax
<ckeepax@opensource.wolfsonmicro.com> wrote:
> Convert to using the newer gpiod interface for the micd_pol_gpio.
> Although we still carry support for the old gpio interface from pdata.
>
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---
>  drivers/extcon/extcon-arizona.c |   36 +++++++++++++++++++++++++++++++-----
>  1 files changed, 31 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
> index 5bf1b19..71d60ba 100644
> --- a/drivers/extcon/extcon-arizona.c
> +++ b/drivers/extcon/extcon-arizona.c
> @@ -20,6 +20,7 @@
>  #include <linux/slab.h>
>  #include <linux/interrupt.h>
>  #include <linux/err.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/gpio.h>
>  #include <linux/input.h>
>  #include <linux/platform_device.h>
> @@ -95,6 +96,8 @@ struct arizona_extcon_info {
>         int hpdet_ip_version;
>
>         struct extcon_dev *edev;
> +
> +       struct gpio_desc *micd_pol_gpio;
>  };
>
>  static const struct arizona_micd_config micd_default_modes[] = {
> @@ -205,6 +208,10 @@ static void arizona_extcon_set_mode(struct arizona_extcon_info *info, int mode)
>         if (arizona->pdata.micd_pol_gpio > 0)
>                 gpio_set_value_cansleep(arizona->pdata.micd_pol_gpio,
>                                         info->micd_modes[mode].gpio);
> +       else if (info->micd_pol_gpio)
> +               gpiod_set_value_cansleep(info->micd_pol_gpio,
> +                                        info->micd_modes[mode].gpio);

gpiod_set_value_cansleep() check always the whether
info->micd_pol_gpiod is NULl or not. So, I think that you can modify
it as following without additional if statemet.

e.g.,
             else
                     gpiod_set_value_cansleep(info->micd_pol_gpio,


> +
>         regmap_update_bits(arizona->regmap, ARIZONA_MIC_DETECT_1,
>                            ARIZONA_MICD_BIAS_SRC_MASK,
>                            info->micd_modes[mode].bias <<
> @@ -1271,6 +1278,22 @@ static int arizona_extcon_probe(struct platform_device *pdev)
>                                 arizona->pdata.micd_pol_gpio, ret);
>                         goto err_register;
>                 }
> +       } else {
> +               if (info->micd_modes[0].gpio)

I'd like you to define the constant varaible for index (0) on separate patch.

> +                       mode = GPIOD_OUT_HIGH;
> +               else
> +                       mode = GPIOD_OUT_LOW;
> +
> +               info->micd_pol_gpio = gpiod_get_optional(arizona->dev,
> +                                                        "wlf,micd-pol",
> +                                                        GPIOD_OUT_LOW);

You can use the devm_gpiod_get_optional() to manage the system
resource automatically.

> +               if (IS_ERR(info->micd_pol_gpio)) {
> +                       ret = PTR_ERR(info->micd_pol_gpio);
> +                       dev_err(arizona->dev,
> +                               "Failed to get microphone polarity GPIO: %d\n",
> +                               ret);
> +                       goto err_register;
> +               }
>         }
>
>         if (arizona->pdata.hpdet_id_gpio > 0) {
> @@ -1281,7 +1304,7 @@ static int arizona_extcon_probe(struct platform_device *pdev)
>                 if (ret != 0) {
>                         dev_err(arizona->dev, "Failed to request GPIO%d: %d\n",
>                                 arizona->pdata.hpdet_id_gpio, ret);
> -                       goto err_register;
> +                       goto err_gpio;
>                 }
>         }
>
> @@ -1325,7 +1348,7 @@ static int arizona_extcon_probe(struct platform_device *pdev)
>                                 dev_err(arizona->dev,
>                                         "MICD ranges must be sorted\n");
>                                 ret = -EINVAL;
> -                               goto err_input;
> +                               goto err_gpio;
>                         }
>                 }
>         }
> @@ -1344,7 +1367,7 @@ static int arizona_extcon_probe(struct platform_device *pdev)
>                         dev_err(arizona->dev, "Unsupported MICD level %d\n",
>                                 info->micd_ranges[i].max);
>                         ret = -EINVAL;
> -                       goto err_input;
> +                       goto err_gpio;
>                 }
>
>                 dev_dbg(arizona->dev, "%d ohms for MICD threshold %d\n",
> @@ -1417,7 +1440,7 @@ static int arizona_extcon_probe(struct platform_device *pdev)
>         if (ret != 0) {
>                 dev_err(&pdev->dev, "Failed to get JACKDET rise IRQ: %d\n",
>                         ret);
> -               goto err_input;
> +               goto err_gpio;
>         }
>
>         ret = arizona_set_irq_wake(arizona, jack_irq_rise, 1);
> @@ -1488,7 +1511,8 @@ err_rise_wake:
>         arizona_set_irq_wake(arizona, jack_irq_rise, 0);
>  err_rise:
>         arizona_free_irq(arizona, jack_irq_rise, info);
> -err_input:
> +err_gpio:
> +       gpiod_put(info->micd_pol_gpio);
>  err_register:
>         pm_runtime_disable(&pdev->dev);
>         return ret;
> @@ -1500,6 +1524,8 @@ static int arizona_extcon_remove(struct platform_device *pdev)
>         struct arizona *arizona = info->arizona;
>         int jack_irq_rise, jack_irq_fall;
>
> +       gpiod_put(info->micd_pol_gpio);
> +
>         pm_runtime_disable(&pdev->dev);
>
>         regmap_update_bits(arizona->regmap,

Thanks,
Chanwoo Choi

Re: [PATCH v2 3/5] extcon: arizona: Convert to gpiod

[Charles Keepax]

On Fri, Jun 19, 2015 at 11:36:47AM +0900, Chanwoo Choi wrote:
> Hi Charles,
> 
> On Thu, Jun 18, 2015 at 11:43 PM, Charles Keepax
> <ckeepax@opensource.wolfsonmicro.com> wrote:
> > Convert to using the newer gpiod interface for the micd_pol_gpio.
> > Although we still carry support for the old gpio interface from pdata.
> >
> > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > ---
> > +                       mode = GPIOD_OUT_HIGH;
> > +               else
> > +                       mode = GPIOD_OUT_LOW;
> > +
> > +               info->micd_pol_gpio = gpiod_get_optional(arizona->dev,
> > +                                                        "wlf,micd-pol",
> > +                                                        GPIOD_OUT_LOW);
> 
> You can use the devm_gpiod_get_optional() to manage the system
> resource automatically.
> 

We can't actually use the devm call here, we need to pass
arizona->dev as that is where the DT will reside, which is the
device for the MFD. But if the devm is attached to the device for
the MFD then it will not clear up when the extcon driver is
unloaded. As such we have to do the put manually.

I will look at respinning for the other comments.

Thanks,
Charles

Re: [PATCH v2 3/5] extcon: arizona: Convert to gpiod

[Chanwoo Choi]

On Fri, Jun 19, 2015 at 5:14 PM, Charles Keepax
<ckeepax@opensource.wolfsonmicro.com> wrote:
> On Fri, Jun 19, 2015 at 11:36:47AM +0900, Chanwoo Choi wrote:
>> Hi Charles,
>>
>> On Thu, Jun 18, 2015 at 11:43 PM, Charles Keepax
>> <ckeepax@opensource.wolfsonmicro.com> wrote:
>> > Convert to using the newer gpiod interface for the micd_pol_gpio.
>> > Although we still carry support for the old gpio interface from pdata.
>> >
>> > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
>> > ---
>> > +                       mode = GPIOD_OUT_HIGH;
>> > +               else
>> > +                       mode = GPIOD_OUT_LOW;
>> > +
>> > +               info->micd_pol_gpio = gpiod_get_optional(arizona->dev,
>> > +                                                        "wlf,micd-pol",
>> > +                                                        GPIOD_OUT_LOW);
>>
>> You can use the devm_gpiod_get_optional() to manage the system
>> resource automatically.
>>
>
> We can't actually use the devm call here, we need to pass
> arizona->dev as that is where the DT will reside, which is the
> device for the MFD. But if the devm is attached to the device for
> the MFD then it will not clear up when the extcon driver is
> unloaded. As such we have to do the put manually.
>
> I will look at respinning for the other comments.

I don't understand. extcon-arizona.c used already following devm_* functions:
- devm_kzalloc()
- devm_regulator_get()
- devm_extcon_dev_*()
- devm_input_allocate_device()
- devm_gpio_request_one()

Thanks,
Chanwoo Choi

Re: [PATCH v2 3/5] extcon: arizona: Convert to gpiod

[Charles Keepax]

On Fri, Jun 19, 2015 at 05:39:22PM +0900, Chanwoo Choi wrote:
> On Fri, Jun 19, 2015 at 5:14 PM, Charles Keepax
> <ckeepax@opensource.wolfsonmicro.com> wrote:
> > On Fri, Jun 19, 2015 at 11:36:47AM +0900, Chanwoo Choi wrote:
> >> Hi Charles,
> >>
> >> On Thu, Jun 18, 2015 at 11:43 PM, Charles Keepax
> >> <ckeepax@opensource.wolfsonmicro.com> wrote:
> >> > Convert to using the newer gpiod interface for the micd_pol_gpio.
> >> > Although we still carry support for the old gpio interface from pdata.
> >> >
> >> > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> >> > ---
> >> > +                       mode = GPIOD_OUT_HIGH;
> >> > +               else
> >> > +                       mode = GPIOD_OUT_LOW;
> >> > +
> >> > +               info->micd_pol_gpio = gpiod_get_optional(arizona->dev,
> >> > +                                                        "wlf,micd-pol",
> >> > +                                                        GPIOD_OUT_LOW);
> >>
> >> You can use the devm_gpiod_get_optional() to manage the system
> >> resource automatically.
> >>
> >
> > We can't actually use the devm call here, we need to pass
> > arizona->dev as that is where the DT will reside, which is the
> > device for the MFD. But if the devm is attached to the device for
> > the MFD then it will not clear up when the extcon driver is
> > unloaded. As such we have to do the put manually.
> >
> > I will look at respinning for the other comments.
> 
> I don't understand. extcon-arizona.c used already following devm_* functions:
> - devm_kzalloc()
> - devm_regulator_get()
> - devm_extcon_dev_*()
> - devm_input_allocate_device()
> - devm_gpio_request_one()

Yes but if you look at those all of those are against &pdev->dev
which is the extcon device.

The gpiod interface expects the device passed to both contain
the of_node and be used for the devm operations. But we need to
use the extcon device for the devm operations, but the of_node is
contained on the MFD device.

So if I do:

devm_gpiod_get_optional(arizona->dev, ....

Then the gpiod won't be released when the extcon device is
removed. But if I do:

devm_gpiod_get_optional(&pdev->dev, ....

Then it won't be able to find the DT entries.

Thanks,
Charles

Re: [PATCH v2 3/5] extcon: arizona: Convert to gpiod

[Chanwoo Choi]

On Fri, Jun 19, 2015 at 6:13 PM, Charles Keepax
<ckeepax@opensource.wolfsonmicro.com> wrote:
> On Fri, Jun 19, 2015 at 05:39:22PM +0900, Chanwoo Choi wrote:
>> On Fri, Jun 19, 2015 at 5:14 PM, Charles Keepax
>> <ckeepax@opensource.wolfsonmicro.com> wrote:
>> > On Fri, Jun 19, 2015 at 11:36:47AM +0900, Chanwoo Choi wrote:
>> >> Hi Charles,
>> >>
>> >> On Thu, Jun 18, 2015 at 11:43 PM, Charles Keepax
>> >> <ckeepax@opensource.wolfsonmicro.com> wrote:
>> >> > Convert to using the newer gpiod interface for the micd_pol_gpio.
>> >> > Although we still carry support for the old gpio interface from pdata.
>> >> >
>> >> > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
>> >> > ---
>> >> > +                       mode = GPIOD_OUT_HIGH;
>> >> > +               else
>> >> > +                       mode = GPIOD_OUT_LOW;
>> >> > +
>> >> > +               info->micd_pol_gpio = gpiod_get_optional(arizona->dev,
>> >> > +                                                        "wlf,micd-pol",
>> >> > +                                                        GPIOD_OUT_LOW);
>> >>
>> >> You can use the devm_gpiod_get_optional() to manage the system
>> >> resource automatically.
>> >>
>> >
>> > We can't actually use the devm call here, we need to pass
>> > arizona->dev as that is where the DT will reside, which is the
>> > device for the MFD. But if the devm is attached to the device for
>> > the MFD then it will not clear up when the extcon driver is
>> > unloaded. As such we have to do the put manually.
>> >
>> > I will look at respinning for the other comments.
>>
>> I don't understand. extcon-arizona.c used already following devm_* functions:
>> - devm_kzalloc()
>> - devm_regulator_get()
>> - devm_extcon_dev_*()
>> - devm_input_allocate_device()
>> - devm_gpio_request_one()
>
> Yes but if you look at those all of those are against &pdev->dev
> which is the extcon device.
>
> The gpiod interface expects the device passed to both contain
> the of_node and be used for the devm operations. But we need to
> use the extcon device for the devm operations, but the of_node is
> contained on the MFD device.
>
> So if I do:
>
> devm_gpiod_get_optional(arizona->dev, ....
>
> Then the gpiod won't be released when the extcon device is
> removed. But if I do:
>
> devm_gpiod_get_optional(&pdev->dev, ....
>
> Then it won't be able to find the DT entries.

I understand the difference between arizona->dev and &pdev->dev.

But, extcon-arizona.c already get the instance of regulator by using
devm_regulator_get() with &pdev->dev as following:
The devm_regulator_get() can find the DT entry in dts file.
                 info->micvdd = devm_regulator_get(&pdev-dev, "MICVDD");

How did extcon-arizona.c get the instance of regulator throught
devm_regulator_get() in dts file?

Re: [PATCH v2 3/5] extcon: arizona: Convert to gpiod

[Charles Keepax]

On Fri, Jun 19, 2015 at 06:50:03PM +0900, Chanwoo Choi wrote:
> On Fri, Jun 19, 2015 at 6:13 PM, Charles Keepax
> <ckeepax@opensource.wolfsonmicro.com> wrote:
> > On Fri, Jun 19, 2015 at 05:39:22PM +0900, Chanwoo Choi wrote:
> >> On Fri, Jun 19, 2015 at 5:14 PM, Charles Keepax
> >> <ckeepax@opensource.wolfsonmicro.com> wrote:
> >> > On Fri, Jun 19, 2015 at 11:36:47AM +0900, Chanwoo Choi wrote:
> >> >> Hi Charles,
> >> >>
> >> >> On Thu, Jun 18, 2015 at 11:43 PM, Charles Keepax
> >> >> <ckeepax@opensource.wolfsonmicro.com> wrote:
> >> >> > Convert to using the newer gpiod interface for the micd_pol_gpio.
> >> >> > Although we still carry support for the old gpio interface from pdata.
> >> >> >
> >> >> > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> >> >> > ---
> >> >> > +                       mode = GPIOD_OUT_HIGH;
> >> >> > +               else
> >> >> > +                       mode = GPIOD_OUT_LOW;
> >> >> > +
> >> >> > +               info->micd_pol_gpio = gpiod_get_optional(arizona->dev,
> >> >> > +                                                        "wlf,micd-pol",
> >> >> > +                                                        GPIOD_OUT_LOW);
> >> >>
> >> >> You can use the devm_gpiod_get_optional() to manage the system
> >> >> resource automatically.
> >> >>
> >> >
> >> > We can't actually use the devm call here, we need to pass
> >> > arizona->dev as that is where the DT will reside, which is the
> >> > device for the MFD. But if the devm is attached to the device for
> >> > the MFD then it will not clear up when the extcon driver is
> >> > unloaded. As such we have to do the put manually.
> >> >
> >> > I will look at respinning for the other comments.
> >>
> >> I don't understand. extcon-arizona.c used already following devm_* functions:
> >> - devm_kzalloc()
> >> - devm_regulator_get()
> >> - devm_extcon_dev_*()
> >> - devm_input_allocate_device()
> >> - devm_gpio_request_one()
> >
> > Yes but if you look at those all of those are against &pdev->dev
> > which is the extcon device.
> >
> > The gpiod interface expects the device passed to both contain
> > the of_node and be used for the devm operations. But we need to
> > use the extcon device for the devm operations, but the of_node is
> > contained on the MFD device.
> >
> > So if I do:
> >
> > devm_gpiod_get_optional(arizona->dev, ....
> >
> > Then the gpiod won't be released when the extcon device is
> > removed. But if I do:
> >
> > devm_gpiod_get_optional(&pdev->dev, ....
> >
> > Then it won't be able to find the DT entries.
> 
> I understand the difference between arizona->dev and &pdev->dev.
> 
> But, extcon-arizona.c already get the instance of regulator by using
> devm_regulator_get() with &pdev->dev as following:
> The devm_regulator_get() can find the DT entry in dts file.
>                  info->micvdd = devm_regulator_get(&pdev-dev, "MICVDD");
> 
> How did extcon-arizona.c get the instance of regulator throught
> devm_regulator_get() in dts file?

The regulator API contains a feature called regulator aliases,
(see regulator_register_supply_alias) which the MFD driver for
Arizona registers a bunch of. So when the regulator lookup is
done the regulator core will actually do the lookup on the
parent MFD device rather than on the extcon device.

Thanks,
Charles

[PATCH v2 4/5] mfd: arizona: Update DT doc for new mic detection bindings

[Charles Keepax]

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 Documentation/devicetree/bindings/mfd/arizona.txt |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/mfd/arizona.txt b/Documentation/devicetree/bindings/mfd/arizona.txt
index 32a71b7..8d92c92 100644
--- a/Documentation/devicetree/bindings/mfd/arizona.txt
+++ b/Documentation/devicetree/bindings/mfd/arizona.txt
@@ -68,6 +68,22 @@ Optional properties:
     If this node is not mentioned or if the value is unknown, then
     headphone detection mode is set to HPDETL.
 
+  - wlf,micd-detect-debounce : Additional software microphone detection
+    debounce specified in milliseconds.
+  - wlf,micd-pol-gpio : GPIO specifier for the GPIO controlling the headset
+    polarity if one exists.
+  - wlf,micd-bias-start-time : Time allowed for MICBIAS to startup prior to
+    performing microphone detection, specified as per the ARIZONA_MICD_TIME_XXX
+    defines.
+  - wlf,micd-rate : Delay between successive microphone detection measurements,
+    specified as per the ARIZONA_MICD_TIME_XXX defines.
+  - wlf,micd-dbtime : Microphone detection hardware debounces specified as the
+    number of measurements to take, valid values being 2 and 4.
+  - wlf,micd-timeout : Timeout for microphone detection, specified in
+    milliseconds.
+  - wlf,micd-force-micbias : Force MICBIAS continuously on during microphone
+    detection.
+
   - DCVDD-supply, MICVDD-supply : Power supplies, only need to be specified if
     they are being externally supplied. As covered in
     Documentation/devicetree/bindings/regulator/regulator.txt
-- 
1.7.2.5

[PATCH v2 5/5] mfd: arizona: Update several pdata members to unsigned

[Charles Keepax]

Device tree and ACPI primarily deal with unsigned ints, many of the
pdata members in the Arizona driver are signed ints but are only ever
assigned positive values. Changing these pdata fields to unsigned ints
avoids us having to choose between overly verbose code and Sparse
warnings.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 include/linux/mfd/arizona/pdata.h |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/linux/mfd/arizona/pdata.h b/include/linux/mfd/arizona/pdata.h
index aa5c48b..77f91a7 100644
--- a/include/linux/mfd/arizona/pdata.h
+++ b/include/linux/mfd/arizona/pdata.h
@@ -101,7 +101,7 @@ struct arizona_pdata {
 	 * useful for systems where and I2S bus with multiple data
 	 * lines is mastered.
 	 */
-	int max_channels_clocked[ARIZONA_MAX_AIF];
+	unsigned int max_channels_clocked[ARIZONA_MAX_AIF];
 
 	/** GPIO5 is used for jack detection */
 	bool jd_gpio5;
@@ -125,22 +125,22 @@ struct arizona_pdata {
 	unsigned int hpdet_channel;
 
 	/** Extra debounce timeout used during initial mic detection (ms) */
-	int micd_detect_debounce;
+	unsigned int micd_detect_debounce;
 
 	/** GPIO for mic detection polarity */
 	int micd_pol_gpio;
 
 	/** Mic detect ramp rate */
-	int micd_bias_start_time;
+	unsigned int micd_bias_start_time;
 
 	/** Mic detect sample rate */
-	int micd_rate;
+	unsigned int micd_rate;
 
 	/** Mic detect debounce level */
-	int micd_dbtime;
+	unsigned int micd_dbtime;
 
 	/** Mic detect timeout (ms) */
-	int micd_timeout;
+	unsigned int micd_timeout;
 
 	/** Force MICBIAS on for mic detect */
 	bool micd_force_micbias;
-- 
1.7.2.5