[PATCH 0/6] extcon-gpio: Add devicetree support

[PATCH 0/6] extcon-gpio: Add devicetree support

[Guenter Roeck]

The first three patches of this series are either cleanup or add functionality
unrelated to devicetree support and should hopefully be acceptable as-is or with
minor modifications.

Patch 1 of this series is a tiny cleanup patch.
Patch 2 adds support for hardware debounce. If the gpio chip supports debounce,
use it instead of software debounce.
Patch 3 adds support for low-active presence detect signals.

Patch 4 and 5 add devicetree support. Patch 4 is the actual code and patch 5
describes devicetree bindings. Both could possibly be merged into a single
patch.

Patch 6 adds possible properties for connectors supporting multiple cable
types to the devicetree bindings document. This is purely for discussion
and would be implemented in a separate patch. While I don't need this code
for our application, I would be happy to provide it if the community sees
value in it.

[PATCH 1/6] extcon-gpio: Do not unnecessarily initialize variables

[Guenter Roeck]

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/extcon/extcon-gpio.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
index 02bec32..77d35a7 100644
--- a/drivers/extcon/extcon-gpio.c
+++ b/drivers/extcon/extcon-gpio.c
@@ -80,7 +80,7 @@ static int gpio_extcon_probe(struct platform_device *pdev)
 {
 	struct gpio_extcon_platform_data *pdata = pdev->dev.platform_data;
 	struct gpio_extcon_data *extcon_data;
-	int ret = 0;
+	int ret;
 
 	if (!pdata)
 		return -EBUSY;
-- 
1.7.9.7

Re: [PATCH 1/6] extcon-gpio: Do not unnecessarily initialize variables

[Chanwoo Choi]

On 08/30/2013 01:29 PM, Guenter Roeck wrote:
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/extcon/extcon-gpio.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
> index 02bec32..77d35a7 100644
> --- a/drivers/extcon/extcon-gpio.c
> +++ b/drivers/extcon/extcon-gpio.c
> @@ -80,7 +80,7 @@ static int gpio_extcon_probe(struct platform_device *pdev)
>  {
>  	struct gpio_extcon_platform_data *pdata = pdev->dev.platform_data;
>  	struct gpio_extcon_data *extcon_data;
> -	int ret = 0;
> +	int ret;
>  
>  	if (!pdata)
>  		return -EBUSY;
> 

Applied it on extcon-linus branch.

Thanks.
Chanwoo Choi

[PATCH 2/6] extcon-gpio: If the gpio driver/chip supports debounce, use it

[Guenter Roeck]

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/extcon/extcon-gpio.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
index 77d35a7..973600e 100644
--- a/drivers/extcon/extcon-gpio.c
+++ b/drivers/extcon/extcon-gpio.c
@@ -111,6 +111,11 @@ static int gpio_extcon_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto err;
 
+	/* Use gpio debounce if available. If so, don't debounce in software. */
+	if (pdata->debounce &&
+	    !gpio_set_debounce(extcon_data->gpio, pdata->debounce * 1000))
+		extcon_data->debounce_jiffies = 0;
+
 	INIT_DELAYED_WORK(&extcon_data->work, gpio_extcon_work);
 
 	extcon_data->irq = gpio_to_irq(extcon_data->gpio);
-- 
1.7.9.7

Re: [PATCH 2/6] extcon-gpio: If the gpio driver/chip supports debounce, use it

[Chanwoo Choi]

Hi Guenter

I agree to use gpio_set_debounce() API but, I suggest following patch to code clean.
and I'd like you to use declarative sentence on patch name instead of 'If ...'.

On 08/30/2013 01:29 PM, Guenter Roeck wrote:
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/extcon/extcon-gpio.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
> index 77d35a7..973600e 100644
> --- a/drivers/extcon/extcon-gpio.c
> +++ b/drivers/extcon/extcon-gpio.c
> @@ -111,6 +111,11 @@ static int gpio_extcon_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		goto err;
>  
> +	/* Use gpio debounce if available. If so, don't debounce in software. */
> +	if (pdata->debounce &&
> +	    !gpio_set_debounce(extcon_data->gpio, pdata->debounce * 1000))
> +		extcon_data->debounce_jiffies = 0;
> +
>  	INIT_DELAYED_WORK(&extcon_data->work, gpio_extcon_work);
>  
>  	extcon_data->irq = gpio_to_irq(extcon_data->gpio);
> 

diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
index 3943ce2..0777e72 100644
--- a/drivers/extcon/extcon-gpio.c
+++ b/drivers/extcon/extcon-gpio.c
@@ -56,8 +56,10 @@ static irqreturn_t gpio_irq_handler(int irq, void *dev_id)
 {
        struct gpio_extcon_data *extcon_data = dev_id;
 
-       queue_delayed_work(system_power_efficient_wq, &extcon_data->work,
-                             extcon_data->debounce_jiffies);
+       if (extcon_data->debounce_jiffies)
+               queue_delayed_work(system_power_efficient_wq,
+                                  &extcon_data->work,
+                                  extcon_data->debounce_jiffies);
        return IRQ_HANDLED;
 }
 
@@ -100,7 +102,14 @@ static int gpio_extcon_probe(struct platform_device *pdev)
        extcon_data->state_off = pdata->state_off;
        if (pdata->state_on && pdata->state_off)
                extcon_data->edev.print_state = extcon_gpio_print_state;
-       extcon_data->debounce_jiffies = msecs_to_jiffies(pdata->debounce);
+       extcon_data->debounce_jiffies = 0;
+       if (pdata->debounce) {
+               ret = gpio_set_debounce(extcon_data->gpio,
+                                       pdata->debounce * 1000);
+               if (ret < 0)
+                       extcon_data->debounce_jiffies =
+                               msecs_to_jiffies(pdata->debounce);
+       }
 
        ret = extcon_dev_register(&extcon_data->edev, &pdev->dev);
        if (ret < 0)
@@ -111,11 +120,6 @@ static int gpio_extcon_probe(struct platform_device *pdev)
        if (ret < 0)
                goto err;
 
-       /* Use gpio debounce if available. If so, don't debounce in software. */
-       if (pdata->debounce &&
-           !gpio_set_debounce(extcon_data->gpio, pdata->debounce * 1000))
-               extcon_data->debounce_jiffies = 0;
-
        INIT_DELAYED_WORK(&extcon_data->work, gpio_extcon_work);
 
        extcon_data->irq = gpio_to_irq(extcon_data->gpio);
@@ -146,7 +150,8 @@ static int gpio_extcon_remove(struct platform_device *pdev)
 {
        struct gpio_extcon_data *extcon_data = platform_get_drvdata(pdev);
 
-       cancel_delayed_work_sync(&extcon_data->work);
+       if (extcon_data->debounce_jiffies)
+               cancel_delayed_work_sync(&extcon_data->work);
        free_irq(extcon_data->irq, extcon_data);
        extcon_dev_unregister(&extcon_data->edev);


Thanks,
Chanwoo Choi

Re: [PATCH 2/6] extcon-gpio: If the gpio driver/chip supports debounce, use it

[Guenter Roeck]

On 09/10/2013 06:16 PM, Chanwoo Choi wrote:
> Hi Guenter
>
> I agree to use gpio_set_debounce() API but, I suggest following patch to code clean.
> and I'd like you to use declarative sentence on patch name instead of 'If ...'.
>
> On 08/30/2013 01:29 PM, Guenter Roeck wrote:
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>>   drivers/extcon/extcon-gpio.c |    5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
>> index 77d35a7..973600e 100644
>> --- a/drivers/extcon/extcon-gpio.c
>> +++ b/drivers/extcon/extcon-gpio.c
>> @@ -111,6 +111,11 @@ static int gpio_extcon_probe(struct platform_device *pdev)
>>   	if (ret < 0)
>>   		goto err;
>>
>> +	/* Use gpio debounce if available. If so, don't debounce in software. */
>> +	if (pdata->debounce &&
>> +	    !gpio_set_debounce(extcon_data->gpio, pdata->debounce * 1000))
>> +		extcon_data->debounce_jiffies = 0;
>> +
>>   	INIT_DELAYED_WORK(&extcon_data->work, gpio_extcon_work);
>>
>>   	extcon_data->irq = gpio_to_irq(extcon_data->gpio);
>>
>
> diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
> index 3943ce2..0777e72 100644
> --- a/drivers/extcon/extcon-gpio.c
> +++ b/drivers/extcon/extcon-gpio.c
> @@ -56,8 +56,10 @@ static irqreturn_t gpio_irq_handler(int irq, void *dev_id)
>   {
>          struct gpio_extcon_data *extcon_data = dev_id;
>
> -       queue_delayed_work(system_power_efficient_wq, &extcon_data->work,
> -                             extcon_data->debounce_jiffies);
> +       if (extcon_data->debounce_jiffies)
> +               queue_delayed_work(system_power_efficient_wq,
> +                                  &extcon_data->work,
> +                                  extcon_data->debounce_jiffies);

I am a bit lost about this one. The above means that the workqueue would not be executed
at all if debounce_jiffies is 0 (and if pdata->debounce is 0), meaning an event would
never be generated. With the original code, the workqueue will be executed immediately
if debounce_jiffies is 0, which I think is exactly what we need.

>          return IRQ_HANDLED;
>   }
>
> @@ -100,7 +102,14 @@ static int gpio_extcon_probe(struct platform_device *pdev)
>          extcon_data->state_off = pdata->state_off;
>          if (pdata->state_on && pdata->state_off)
>                  extcon_data->edev.print_state = extcon_gpio_print_state;
> -       extcon_data->debounce_jiffies = msecs_to_jiffies(pdata->debounce);
> +       extcon_data->debounce_jiffies = 0;
> +       if (pdata->debounce) {
> +               ret = gpio_set_debounce(extcon_data->gpio,
> +                                       pdata->debounce * 1000);
> +               if (ret < 0)
> +                       extcon_data->debounce_jiffies =
> +                               msecs_to_jiffies(pdata->debounce);
> +       }
>
Ok, though it is unnecessary to initialize debounce_jiffies (it is pre-initialized
from the allocation), so I'll drop that line.

>          ret = extcon_dev_register(&extcon_data->edev, &pdev->dev);
>          if (ret < 0)
> @@ -111,11 +120,6 @@ static int gpio_extcon_probe(struct platform_device *pdev)
>          if (ret < 0)
>                  goto err;
>
> -       /* Use gpio debounce if available. If so, don't debounce in software. */
> -       if (pdata->debounce &&
> -           !gpio_set_debounce(extcon_data->gpio, pdata->debounce * 1000))
> -               extcon_data->debounce_jiffies = 0;
> -
>          INIT_DELAYED_WORK(&extcon_data->work, gpio_extcon_work);
>
>          extcon_data->irq = gpio_to_irq(extcon_data->gpio);
> @@ -146,7 +150,8 @@ static int gpio_extcon_remove(struct platform_device *pdev)
>   {
>          struct gpio_extcon_data *extcon_data = platform_get_drvdata(pdev);
>
> -       cancel_delayed_work_sync(&extcon_data->work);
> +       if (extcon_data->debounce_jiffies)
> +               cancel_delayed_work_sync(&extcon_data->work);

I think we would have to call cancel_work_sync() in the else case to make sure
that no work is in the process of being executed - which just turns out to execute
the same code as cancel_delayed_work_sync(). So the if/else would just add complexity
with no real gain.

Thanks,
Guenter

>          free_irq(extcon_data->irq, extcon_data);
>          extcon_dev_unregister(&extcon_data->edev);
>
>
> Thanks,
> Chanwoo Choi
>
>

Re: [PATCH 2/6] extcon-gpio: If the gpio driver/chip supports debounce, use it

[Chanwoo Choi]

On 09/11/2013 10:57 AM, Guenter Roeck wrote:
> On 09/10/2013 06:16 PM, Chanwoo Choi wrote:
>> Hi Guenter
>>
>> I agree to use gpio_set_debounce() API but, I suggest following patch to code clean.
>> and I'd like you to use declarative sentence on patch name instead of 'If ...'.
>>
>> On 08/30/2013 01:29 PM, Guenter Roeck wrote:
>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>> ---
>>>   drivers/extcon/extcon-gpio.c |    5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
>>> index 77d35a7..973600e 100644
>>> --- a/drivers/extcon/extcon-gpio.c
>>> +++ b/drivers/extcon/extcon-gpio.c
>>> @@ -111,6 +111,11 @@ static int gpio_extcon_probe(struct platform_device *pdev)
>>>       if (ret < 0)
>>>           goto err;
>>>
>>> +    /* Use gpio debounce if available. If so, don't debounce in software. */
>>> +    if (pdata->debounce &&
>>> +        !gpio_set_debounce(extcon_data->gpio, pdata->debounce * 1000))
>>> +        extcon_data->debounce_jiffies = 0;
>>> +
>>>       INIT_DELAYED_WORK(&extcon_data->work, gpio_extcon_work);
>>>
>>>       extcon_data->irq = gpio_to_irq(extcon_data->gpio);
>>>
>>
>> diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
>> index 3943ce2..0777e72 100644
>> --- a/drivers/extcon/extcon-gpio.c
>> +++ b/drivers/extcon/extcon-gpio.c
>> @@ -56,8 +56,10 @@ static irqreturn_t gpio_irq_handler(int irq, void *dev_id)
>>   {
>>          struct gpio_extcon_data *extcon_data = dev_id;
>>
>> -       queue_delayed_work(system_power_efficient_wq, &extcon_data->work,
>> -                             extcon_data->debounce_jiffies);
>> +       if (extcon_data->debounce_jiffies)
>> +               queue_delayed_work(system_power_efficient_wq,
>> +                                  &extcon_data->work,
>> +                                  extcon_data->debounce_jiffies);
> 
> I am a bit lost about this one. The above means that the workqueue would not be executed
> at all if debounce_jiffies is 0 (and if pdata->debounce is 0), meaning an event would
> never be generated. With the original code, the workqueue will be executed immediately
> if debounce_jiffies is 0, which I think is exactly what we need.
> 
>>          return IRQ_HANDLED;
>>   }
>>
>> @@ -100,7 +102,14 @@ static int gpio_extcon_probe(struct platform_device *pdev)
>>          extcon_data->state_off = pdata->state_off;
>>          if (pdata->state_on && pdata->state_off)
>>                  extcon_data->edev.print_state = extcon_gpio_print_state;
>> -       extcon_data->debounce_jiffies = msecs_to_jiffies(pdata->debounce);
>> +       extcon_data->debounce_jiffies = 0;
>> +       if (pdata->debounce) {
>> +               ret = gpio_set_debounce(extcon_data->gpio,
>> +                                       pdata->debounce * 1000);
>> +               if (ret < 0)
>> +                       extcon_data->debounce_jiffies =
>> +                               msecs_to_jiffies(pdata->debounce);
>> +       }
>>
> Ok, though it is unnecessary to initialize debounce_jiffies (it is pre-initialized
> from the allocation), so I'll drop that line.

OK.

> 
>>          ret = extcon_dev_register(&extcon_data->edev, &pdev->dev);
>>          if (ret < 0)
>> @@ -111,11 +120,6 @@ static int gpio_extcon_probe(struct platform_device *pdev)
>>          if (ret < 0)
>>                  goto err;
>>
>> -       /* Use gpio debounce if available. If so, don't debounce in software. */
>> -       if (pdata->debounce &&
>> -           !gpio_set_debounce(extcon_data->gpio, pdata->debounce * 1000))
>> -               extcon_data->debounce_jiffies = 0;
>> -
>>          INIT_DELAYED_WORK(&extcon_data->work, gpio_extcon_work);
>>
>>          extcon_data->irq = gpio_to_irq(extcon_data->gpio);
>> @@ -146,7 +150,8 @@ static int gpio_extcon_remove(struct platform_device *pdev)
>>   {
>>          struct gpio_extcon_data *extcon_data = platform_get_drvdata(pdev);
>>
>> -       cancel_delayed_work_sync(&extcon_data->work);
>> +       if (extcon_data->debounce_jiffies)
>> +               cancel_delayed_work_sync(&extcon_data->work);
> 
> I think we would have to call cancel_work_sync() in the else case to make sure
> that no work is in the process of being executed - which just turns out to execute
> the same code as cancel_delayed_work_sync(). So the if/else would just add complexity
> with no real gain.

OK.

Thanks,
Chanwoo Choi

[PATCH 3/6] extcon-gpio: Add support for active-low presence detect pins

[Guenter Roeck]

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/extcon/extcon-gpio.c       |    4 +++-
 include/linux/extcon/extcon-gpio.h |    1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
index 973600e..d4e3c89 100644
--- a/drivers/extcon/extcon-gpio.c
+++ b/drivers/extcon/extcon-gpio.c
@@ -34,6 +34,7 @@
 struct gpio_extcon_data {
 	struct extcon_dev edev;
 	unsigned gpio;
+	bool gpio_active_low;
 	const char *state_on;
 	const char *state_off;
 	int irq;
@@ -48,7 +49,7 @@ static void gpio_extcon_work(struct work_struct *work)
 		container_of(to_delayed_work(work), struct gpio_extcon_data,
 			     work);
 
-	state = gpio_get_value(data->gpio);
+	state = gpio_get_value(data->gpio) ^ data->gpio_active_low;
 	extcon_set_state(&data->edev, state);
 }
 
@@ -96,6 +97,7 @@ static int gpio_extcon_probe(struct platform_device *pdev)
 
 	extcon_data->edev.name = pdata->name;
 	extcon_data->gpio = pdata->gpio;
+	extcon_data->gpio_active_low = pdata->gpio_active_low;
 	extcon_data->state_on = pdata->state_on;
 	extcon_data->state_off = pdata->state_off;
 	if (pdata->state_on && pdata->state_off)
diff --git a/include/linux/extcon/extcon-gpio.h b/include/linux/extcon/extcon-gpio.h
index 2d8307f..1613899 100644
--- a/include/linux/extcon/extcon-gpio.h
+++ b/include/linux/extcon/extcon-gpio.h
@@ -41,6 +41,7 @@
 struct gpio_extcon_platform_data {
 	const char *name;
 	unsigned gpio;
+	bool gpio_active_low;
 	unsigned long debounce;
 	unsigned long irq_flags;
 
-- 
1.7.9.7

Re: [PATCH 3/6] extcon-gpio: Add support for active-low presence detect pins

[Chanwoo Choi]

On 08/30/2013 01:29 PM, Guenter Roeck wrote:
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/extcon/extcon-gpio.c       |    4 +++-
>  include/linux/extcon/extcon-gpio.h |    1 +
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
> index 973600e..d4e3c89 100644
> --- a/drivers/extcon/extcon-gpio.c
> +++ b/drivers/extcon/extcon-gpio.c
> @@ -34,6 +34,7 @@
>  struct gpio_extcon_data {
>  	struct extcon_dev edev;
>  	unsigned gpio;
> +	bool gpio_active_low;
>  	const char *state_on;
>  	const char *state_off;
>  	int irq;
> @@ -48,7 +49,7 @@ static void gpio_extcon_work(struct work_struct *work)
>  		container_of(to_delayed_work(work), struct gpio_extcon_data,
>  			     work);
>  
> -	state = gpio_get_value(data->gpio);
> +	state = gpio_get_value(data->gpio) ^ data->gpio_active_low;

I prefer below modified code instead of exclusive keyword(^) to improve readability.

       state = gpio_get_value(data->gpio);
       if (data->gpio_active_low)
               state = !state;

>  	extcon_set_state(&data->edev, state);
>  }
>  
> @@ -96,6 +97,7 @@ static int gpio_extcon_probe(struct platform_device *pdev)
>  
>  	extcon_data->edev.name = pdata->name;
>  	extcon_data->gpio = pdata->gpio;
> +	extcon_data->gpio_active_low = pdata->gpio_active_low;
>  	extcon_data->state_on = pdata->state_on;
>  	extcon_data->state_off = pdata->state_off;
>  	if (pdata->state_on && pdata->state_off)
> diff --git a/include/linux/extcon/extcon-gpio.h b/include/linux/extcon/extcon-gpio.h
> index 2d8307f..1613899 100644
> --- a/include/linux/extcon/extcon-gpio.h
> +++ b/include/linux/extcon/extcon-gpio.h
> @@ -41,6 +41,7 @@
>  struct gpio_extcon_platform_data {
>  	const char *name;
>  	unsigned gpio;
> +	bool gpio_active_low;

You have to add description of gpio_active_low as folloiwng patch:

diff --git a/include/linux/extcon/extcon-gpio.h b/include/linux/extcon/extcon-gpio.h
index 7a2f27b..ff0bfcb 100644
--- a/include/linux/extcon/extcon-gpio.h
+++ b/include/linux/extcon/extcon-gpio.h
@@ -27,6 +27,9 @@
  * struct gpio_extcon_platform_data - A simple GPIO-controlled extcon device.
  * @name:              The name of this GPIO extcon device.
  * @gpio:              Corresponding GPIO.
+ * @gpio_active_low:   Boolean which is whether gpio active state is 1 or 0
+ *                     If true, low state of gpio means active.
+ *                     If false, high state of gpio means active.
  * @debounce:          Debounce time for GPIO IRQ in ms.
  * @irq_flags:         IRQ Flags (e.g., IRQF_TRIGGER_LOW).
  * @state_on:          print_state is overriden with state_on if attached.

Thanks,
Chanwoo Choi

[PATCH v2 3/6] extcon-gpio: Add support for active-low presence detect pins

[Guenter Roeck]

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Document gpio_active_low variable in gpio_extcon_platform_data
    Rewrite active-low logic to be easier to read.

 drivers/extcon/extcon-gpio.c       |    4 ++++
 include/linux/extcon/extcon-gpio.h |    5 +++++
 2 files changed, 9 insertions(+)

diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
index 862743b..8491f86 100644
--- a/drivers/extcon/extcon-gpio.c
+++ b/drivers/extcon/extcon-gpio.c
@@ -34,6 +34,7 @@
 struct gpio_extcon_data {
 	struct extcon_dev edev;
 	unsigned gpio;
+	bool gpio_active_low;
 	const char *state_on;
 	const char *state_off;
 	int irq;
@@ -49,6 +50,8 @@ static void gpio_extcon_work(struct work_struct *work)
 			     work);
 
 	state = gpio_get_value(data->gpio);
+	if (data->gpio_active_low)
+		state = !state;
 	extcon_set_state(&data->edev, state);
 }
 
@@ -96,6 +99,7 @@ static int gpio_extcon_probe(struct platform_device *pdev)
 
 	extcon_data->edev.name = pdata->name;
 	extcon_data->gpio = pdata->gpio;
+	extcon_data->gpio_active_low = pdata->gpio_active_low;
 	extcon_data->state_on = pdata->state_on;
 	extcon_data->state_off = pdata->state_off;
 	if (pdata->state_on && pdata->state_off)
diff --git a/include/linux/extcon/extcon-gpio.h b/include/linux/extcon/extcon-gpio.h
index 2d8307f..51ef15c 100644
--- a/include/linux/extcon/extcon-gpio.h
+++ b/include/linux/extcon/extcon-gpio.h
@@ -27,6 +27,10 @@
  * struct gpio_extcon_platform_data - A simple GPIO-controlled extcon device.
  * @name	The name of this GPIO extcon device.
  * @gpio	Corresponding GPIO.
+ * @gpio_active_low:
+ * 		Boolean describing whether gpio active state is 1 or 0
+ * 		If true, low state of gpio means active.
+ * 		If false, high state of gpio means active.
  * @debounce	Debounce time for GPIO IRQ in ms.
  * @irq_flags	IRQ Flags (e.g., IRQF_TRIGGER_LOW).
  * @state_on	print_state is overriden with state_on if attached. If Null,
@@ -41,6 +45,7 @@
 struct gpio_extcon_platform_data {
 	const char *name;
 	unsigned gpio;
+	bool gpio_active_low;
 	unsigned long debounce;
 	unsigned long irq_flags;
 
-- 
1.7.9.7

Re: [PATCH v2 3/6] extcon-gpio: Add support for active-low presence detect pins

[Chanwoo Choi]

Hi Guenter,

As I mentioned on previous reply, I modify patch name as following:
extcon-gpio: Add support for active-low presence detect pins
->
extcon: gpio: Add support for active-low presence detect pins

I prefer you could add patch description. Also, you should test
on extcon latest branch to protect merge conflict. This patch
has conflict issue on extcon-linus branch. I fix up and applied it.

And I think you might use tab size is 4 characters. you should change tab size
from 4 characters to 8 characters. You can check linux kernel coding sytle on
"/kernel/Documentation/CodingStyle - chapter 1: Indentation".

On 09/11/2013 11:25 AM, Guenter Roeck wrote:
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> v2: Document gpio_active_low variable in gpio_extcon_platform_data
>     Rewrite active-low logic to be easier to read.
> 
>  drivers/extcon/extcon-gpio.c       |    4 ++++
>  include/linux/extcon/extcon-gpio.h |    5 +++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
> index 862743b..8491f86 100644
> --- a/drivers/extcon/extcon-gpio.c
> +++ b/drivers/extcon/extcon-gpio.c
> @@ -34,6 +34,7 @@
>  struct gpio_extcon_data {
>  	struct extcon_dev edev;
>  	unsigned gpio;
> +	bool gpio_active_low;
>  	const char *state_on;
>  	const char *state_off;
>  	int irq;
> @@ -49,6 +50,8 @@ static void gpio_extcon_work(struct work_struct *work)
>  			     work);
>  
>  	state = gpio_get_value(data->gpio);
> +	if (data->gpio_active_low)
> +		state = !state;
>  	extcon_set_state(&data->edev, state);
>  }
>  
> @@ -96,6 +99,7 @@ static int gpio_extcon_probe(struct platform_device *pdev)
>  
>  	extcon_data->edev.name = pdata->name;
>  	extcon_data->gpio = pdata->gpio;
> +	extcon_data->gpio_active_low = pdata->gpio_active_low;
>  	extcon_data->state_on = pdata->state_on;
>  	extcon_data->state_off = pdata->state_off;
>  	if (pdata->state_on && pdata->state_off)
> diff --git a/include/linux/extcon/extcon-gpio.h b/include/linux/extcon/extcon-gpio.h
> index 2d8307f..51ef15c 100644
> --- a/include/linux/extcon/extcon-gpio.h
> +++ b/include/linux/extcon/extcon-gpio.h
> @@ -27,6 +27,10 @@
>   * struct gpio_extcon_platform_data - A simple GPIO-controlled extcon device.
>   * @name	The name of this GPIO extcon device.
>   * @gpio	Corresponding GPIO.
> + * @gpio_active_low:
> + * 		Boolean describing whether gpio active state is 1 or 0
> + * 		If true, low state of gpio means active.
> + * 		If false, high state of gpio means active.

I think you may use tab size is 4 characters.

>   * @debounce	Debounce time for GPIO IRQ in ms.
>   * @irq_flags	IRQ Flags (e.g., IRQF_TRIGGER_LOW).
>   * @state_on	print_state is overriden with state_on if attached. If Null,
> @@ -41,6 +45,7 @@
>  struct gpio_extcon_platform_data {
>  	const char *name;
>  	unsigned gpio;
> +	bool gpio_active_low;
>  	unsigned long debounce;
>  	unsigned long irq_flags;
>  
> 

Re: [PATCH v2 3/6] extcon-gpio: Add support for active-low presence detect pins

[Guenter Roeck]

On 09/11/2013 04:57 PM, Chanwoo Choi wrote:
> Hi Guenter,
>
> As I mentioned on previous reply, I modify patch name as following:
> extcon-gpio: Add support for active-low presence detect pins
> ->
> extcon: gpio: Add support for active-low presence detect pins
>
> I prefer you could add patch description. Also, you should test

As mentioned before, I sent out v2 of this patch before I read your comments
to the previous patch.

> on extcon latest branch to protect merge conflict. This patch
> has conflict issue on extcon-linus branch. I fix up and applied it.
>
I'll make sure to so that in the future.

> And I think you might use tab size is 4 characters. you should change tab size
> from 4 characters to 8 characters. You can check linux kernel coding sytle on

Thanks for the reminder, but the problem is a different one. See below.

> "/kernel/Documentation/CodingStyle - chapter 1: Indentation".
>
> On 09/11/2013 11:25 AM, Guenter Roeck wrote:
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>> v2: Document gpio_active_low variable in gpio_extcon_platform_data
>>      Rewrite active-low logic to be easier to read.
>>
>>   drivers/extcon/extcon-gpio.c       |    4 ++++
>>   include/linux/extcon/extcon-gpio.h |    5 +++++
>>   2 files changed, 9 insertions(+)
>>
>> diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
>> index 862743b..8491f86 100644
>> --- a/drivers/extcon/extcon-gpio.c
>> +++ b/drivers/extcon/extcon-gpio.c
>> @@ -34,6 +34,7 @@
>>   struct gpio_extcon_data {
>>   	struct extcon_dev edev;
>>   	unsigned gpio;
>> +	bool gpio_active_low;
>>   	const char *state_on;
>>   	const char *state_off;
>>   	int irq;
>> @@ -49,6 +50,8 @@ static void gpio_extcon_work(struct work_struct *work)
>>   			     work);
>>
>>   	state = gpio_get_value(data->gpio);
>> +	if (data->gpio_active_low)
>> +		state = !state;
>>   	extcon_set_state(&data->edev, state);
>>   }
>>
>> @@ -96,6 +99,7 @@ static int gpio_extcon_probe(struct platform_device *pdev)
>>
>>   	extcon_data->edev.name = pdata->name;
>>   	extcon_data->gpio = pdata->gpio;
>> +	extcon_data->gpio_active_low = pdata->gpio_active_low;
>>   	extcon_data->state_on = pdata->state_on;
>>   	extcon_data->state_off = pdata->state_off;
>>   	if (pdata->state_on && pdata->state_off)
>> diff --git a/include/linux/extcon/extcon-gpio.h b/include/linux/extcon/extcon-gpio.h
>> index 2d8307f..51ef15c 100644
>> --- a/include/linux/extcon/extcon-gpio.h
>> +++ b/include/linux/extcon/extcon-gpio.h
>> @@ -27,6 +27,10 @@
>>    * struct gpio_extcon_platform_data - A simple GPIO-controlled extcon device.
>>    * @name	The name of this GPIO extcon device.
>>    * @gpio	Corresponding GPIO.
>> + * @gpio_active_low:
>> + * 		Boolean describing whether gpio active state is 1 or 0
>> + * 		If true, low state of gpio means active.
>> + * 		If false, high state of gpio means active.
>
> I think you may use tab size is 4 characters.
>
No, that isn't it. When I cut-and-pasted your proposed text I was not careful enough
and did not remove a space after the '*'. No idea how it got there, but it isn't the
tab setting.

Usually I _do_ run my patches through checkpatch.pl to avoid problems like that.
Sorry, my bad; looks like I was shooting too quickly last night and didn't do that.

Guenter

[RFC PATCH 4/6] extcon-gpio: Add devicetree support

[Guenter Roeck]

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/extcon/extcon-gpio.c |   59 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 57 insertions(+), 2 deletions(-)

diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
index d4e3c89..16951fe 100644
--- a/drivers/extcon/extcon-gpio.c
+++ b/drivers/extcon/extcon-gpio.c
@@ -30,6 +30,8 @@
 #include <linux/gpio.h>
 #include <linux/extcon.h>
 #include <linux/extcon/extcon-gpio.h>
+#include <linux/of_gpio.h>
+#include <linux/err.h>
 
 struct gpio_extcon_data {
 	struct extcon_dev edev;
@@ -77,14 +79,66 @@ static ssize_t extcon_gpio_print_state(struct extcon_dev *edev, char *buf)
 	return -EINVAL;
 }
 
+#ifdef CONFIG_OF_GPIO
+
+static struct gpio_extcon_platform_data *
+extcon_gpio_config_of(struct device *dev)
+{
+	struct gpio_extcon_platform_data *pdata;
+	struct device_node *np = dev->of_node;
+	enum of_gpio_flags flags;
+	int gpio, ret;
+	u32 debounce;
+
+	gpio = of_get_named_gpio_flags(np, "presence-detect-gpios", 0, &flags);
+	if (gpio < 0)
+		return ERR_PTR(gpio);
+
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return ERR_PTR(-ENOMEM);
+
+	pdata->gpio = gpio;
+	pdata->gpio_active_low = flags & OF_GPIO_ACTIVE_LOW;
+	pdata->irq_flags = IRQ_TYPE_EDGE_BOTH;
+
+	if (!of_property_read_u32(np, "debounce-interval", &debounce))
+		pdata->debounce = debounce;
+
+	ret = of_property_read_string(np, "name", &pdata->name);
+	if (ret < 0)
+		return ERR_PTR(ret);
+
+	of_property_read_string(np, "state-on", &pdata->state_on);
+	of_property_read_string(np, "state-off", &pdata->state_off);
+
+	return pdata;
+}
+
+static const struct of_device_id of_gpio_extcon_match[] = {
+	{ .compatible = "gpio-connector", },
+	{},
+};
+#else /* CONFIG_OF_GPIO */
+static struct gpio_extcon_platform_data *
+extcon_gpio_config_of(struct device *pdev)
+{
+	return ERR_PTR(-ENODEV);
+}
+#endif /* CONFIG_OF_GPIO */
+
 static int gpio_extcon_probe(struct platform_device *pdev)
 {
 	struct gpio_extcon_platform_data *pdata = pdev->dev.platform_data;
 	struct gpio_extcon_data *extcon_data;
 	int ret;
 
-	if (!pdata)
-		return -EBUSY;
+	if (!pdata) {
+		pdata = extcon_gpio_config_of(&pdev->dev);
+		if (IS_ERR(pdata))
+			return PTR_ERR(pdata);
+	}
+
 	if (!pdata->irq_flags) {
 		dev_err(&pdev->dev, "IRQ flag is not specified.\n");
 		return -EINVAL;
@@ -161,6 +215,7 @@ static struct platform_driver gpio_extcon_driver = {
 	.driver		= {
 		.name	= "extcon-gpio",
 		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(of_gpio_extcon_match),
 	},
 };
 
-- 
1.7.9.7

Re: [RFC PATCH 4/6] extcon-gpio: Add devicetree support

[Mark Rutland]

On Fri, Aug 30, 2013 at 05:29:36AM +0100, Guenter Roeck wrote:
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/extcon/extcon-gpio.c |   59 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 57 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
> index d4e3c89..16951fe 100644
> --- a/drivers/extcon/extcon-gpio.c
> +++ b/drivers/extcon/extcon-gpio.c
> @@ -30,6 +30,8 @@
>  #include <linux/gpio.h>
>  #include <linux/extcon.h>
>  #include <linux/extcon/extcon-gpio.h>
> +#include <linux/of_gpio.h>
> +#include <linux/err.h>
>  
>  struct gpio_extcon_data {
>  	struct extcon_dev edev;
> @@ -77,14 +79,66 @@ static ssize_t extcon_gpio_print_state(struct extcon_dev *edev, char *buf)
>  	return -EINVAL;
>  }
>  
> +#ifdef CONFIG_OF_GPIO
> +
> +static struct gpio_extcon_platform_data *
> +extcon_gpio_config_of(struct device *dev)
> +{
> +	struct gpio_extcon_platform_data *pdata;
> +	struct device_node *np = dev->of_node;
> +	enum of_gpio_flags flags;
> +	int gpio, ret;
> +	u32 debounce;
> +
> +	gpio = of_get_named_gpio_flags(np, "presence-detect-gpios", 0, &flags);
> +	if (gpio < 0)
> +		return ERR_PTR(gpio);
> +
> +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return ERR_PTR(-ENOMEM);
> +
> +	pdata->gpio = gpio;
> +	pdata->gpio_active_low = flags & OF_GPIO_ACTIVE_LOW;
> +	pdata->irq_flags = IRQ_TYPE_EDGE_BOTH;
> +
> +	if (!of_property_read_u32(np, "debounce-interval", &debounce))
> +		pdata->debounce = debounce;
> +
> +	ret = of_property_read_string(np, "name", &pdata->name);

This wasn't listed in the binding. What's this for?

Thanks,
Mark.

> +	if (ret < 0)
> +		return ERR_PTR(ret);
> +
> +	of_property_read_string(np, "state-on", &pdata->state_on);
> +	of_property_read_string(np, "state-off", &pdata->state_off);
> +
> +	return pdata;
> +}
> +
> +static const struct of_device_id of_gpio_extcon_match[] = {
> +	{ .compatible = "gpio-connector", },
> +	{},
> +};
> +#else /* CONFIG_OF_GPIO */
> +static struct gpio_extcon_platform_data *
> +extcon_gpio_config_of(struct device *pdev)
> +{
> +	return ERR_PTR(-ENODEV);
> +}
> +#endif /* CONFIG_OF_GPIO */
> +
>  static int gpio_extcon_probe(struct platform_device *pdev)
>  {
>  	struct gpio_extcon_platform_data *pdata = pdev->dev.platform_data;
>  	struct gpio_extcon_data *extcon_data;
>  	int ret;
>  
> -	if (!pdata)
> -		return -EBUSY;
> +	if (!pdata) {
> +		pdata = extcon_gpio_config_of(&pdev->dev);
> +		if (IS_ERR(pdata))
> +			return PTR_ERR(pdata);
> +	}
> +
>  	if (!pdata->irq_flags) {
>  		dev_err(&pdev->dev, "IRQ flag is not specified.\n");
>  		return -EINVAL;
> @@ -161,6 +215,7 @@ static struct platform_driver gpio_extcon_driver = {
>  	.driver		= {
>  		.name	= "extcon-gpio",
>  		.owner	= THIS_MODULE,
> +		.of_match_table = of_match_ptr(of_gpio_extcon_match),
>  	},
>  };
>  
> -- 
> 1.7.9.7
> 
> 

Re: [RFC PATCH 4/6] extcon-gpio: Add devicetree support

[Guenter Roeck]

On Thu, Sep 12, 2013 at 05:45:45PM +0100, Mark Rutland wrote:
> On Fri, Aug 30, 2013 at 05:29:36AM +0100, Guenter Roeck wrote:
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> >  drivers/extcon/extcon-gpio.c |   59 ++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 57 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
> > index d4e3c89..16951fe 100644
> > --- a/drivers/extcon/extcon-gpio.c
> > +++ b/drivers/extcon/extcon-gpio.c
> > @@ -30,6 +30,8 @@
> >  #include <linux/gpio.h>
> >  #include <linux/extcon.h>
> >  #include <linux/extcon/extcon-gpio.h>
> > +#include <linux/of_gpio.h>
> > +#include <linux/err.h>
> >  
> >  struct gpio_extcon_data {
> >  	struct extcon_dev edev;
> > @@ -77,14 +79,66 @@ static ssize_t extcon_gpio_print_state(struct extcon_dev *edev, char *buf)
> >  	return -EINVAL;
> >  }
> >  
> > +#ifdef CONFIG_OF_GPIO
> > +
> > +static struct gpio_extcon_platform_data *
> > +extcon_gpio_config_of(struct device *dev)
> > +{
> > +	struct gpio_extcon_platform_data *pdata;
> > +	struct device_node *np = dev->of_node;
> > +	enum of_gpio_flags flags;
> > +	int gpio, ret;
> > +	u32 debounce;
> > +
> > +	gpio = of_get_named_gpio_flags(np, "presence-detect-gpios", 0, &flags);
> > +	if (gpio < 0)
> > +		return ERR_PTR(gpio);
> > +
> > +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> > +	if (!pdata)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	pdata->gpio = gpio;
> > +	pdata->gpio_active_low = flags & OF_GPIO_ACTIVE_LOW;
> > +	pdata->irq_flags = IRQ_TYPE_EDGE_BOTH;
> > +
> > +	if (!of_property_read_u32(np, "debounce-interval", &debounce))
> > +		pdata->debounce = debounce;
> > +
> > +	ret = of_property_read_string(np, "name", &pdata->name);
> 
> This wasn't listed in the binding. What's this for?
> 
This returns the node name.
For example, if the bindings are

	my-connector {
		...
	};

it returns the string "my-connector".

I can add a reference to the bindings document if that helps. It is typically
not mentioned in bindings, though, so I didn't do it either.

Thanks,
Guenter

[RFC PATCH 5/6] extcon-gpio: Describe devicetree bindings

[Guenter Roeck]

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 .../devicetree/bindings/extcon/extcon-gpio         |   23 ++++++++++++++++++++
 1 file changed, 23 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/extcon/extcon-gpio

diff --git a/Documentation/devicetree/bindings/extcon/extcon-gpio b/Documentation/devicetree/bindings/extcon/extcon-gpio
new file mode 100644
index 0000000..091ddc6
--- /dev/null
+++ b/Documentation/devicetree/bindings/extcon/extcon-gpio
@@ -0,0 +1,23 @@
+Device-Tree bindings for extcon/extcon-gpio driver
+
+Required properties:
+	- compatible = "gpio-connector";
+	- presence-detect-gpios - presence detect gpio pin
+
+Optional properties:
+	- debounce-interval - debounce interval in milli-seconds
+	- state-on - on (connected) state
+	- state-off - off (disconnected) state
+	  Depending on the type of connector or cable, states may
+	  for example be reported as "connected"/"disconnected"
+	  or "inserted"/"removed".
+
+Example node:
+
+	some-connector {
+		compatible = "gpio-connector";
+		presence-detect-gpios = <&gpio1 7 1>;
+		debounce-interval = <1>;
+		state-on = "connected";
+		state-on = "disconnected";
+	};
-- 
1.7.9.7

Re: [RFC PATCH 5/6] extcon-gpio: Describe devicetree bindings

[Mark Rutland]

On Fri, Aug 30, 2013 at 05:29:37AM +0100, Guenter Roeck wrote:
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  .../devicetree/bindings/extcon/extcon-gpio         |   23 ++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/extcon/extcon-gpio
> 
> diff --git a/Documentation/devicetree/bindings/extcon/extcon-gpio b/Documentation/devicetree/bindings/extcon/extcon-gpio
> new file mode 100644
> index 0000000..091ddc6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/extcon/extcon-gpio
> @@ -0,0 +1,23 @@
> +Device-Tree bindings for extcon/extcon-gpio driver

Bindings shouldn't refer to Linux-specifics like particular drivers.
What class of hardware are you actually trying to describe?

> +
> +Required properties:
> +	- compatible = "gpio-connector";
> +	- presence-detect-gpios - presence detect gpio pin
> +
> +Optional properties:
> +	- debounce-interval - debounce interval in milli-seconds
> +	- state-on - on (connected) state
> +	- state-off - off (disconnected) state
> +	  Depending on the type of connector or cable, states may
> +	  for example be reported as "connected"/"disconnected"
> +	  or "inserted"/"removed".

I don't understand what the state-* properties describe. Do these
provide semantic information to drivers? What is the full set of valid
values?

> +
> +Example node:
> +
> +	some-connector {
> +		compatible = "gpio-connector";
> +		presence-detect-gpios = <&gpio1 7 1>;
> +		debounce-interval = <1>;
> +		state-on = "connected";
> +		state-on = "disconnected";
> +	};

I'm not sure how much value this adds to bindings over describing the
gpios directly. This seems to add a layer of indirection because of
Linux internals.

Thanks,
Mark.

Re: [RFC PATCH 5/6] extcon-gpio: Describe devicetree bindings

[Guenter Roeck]

On Thu, Sep 12, 2013 at 05:41:00PM +0100, Mark Rutland wrote:
> On Fri, Aug 30, 2013 at 05:29:37AM +0100, Guenter Roeck wrote:
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> >  .../devicetree/bindings/extcon/extcon-gpio         |   23 ++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/extcon/extcon-gpio
> > 
> > diff --git a/Documentation/devicetree/bindings/extcon/extcon-gpio b/Documentation/devicetree/bindings/extcon/extcon-gpio
> > new file mode 100644
> > index 0000000..091ddc6
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/extcon/extcon-gpio
> > @@ -0,0 +1,23 @@
> > +Device-Tree bindings for extcon/extcon-gpio driver
> 
> Bindings shouldn't refer to Linux-specifics like particular drivers.
> What class of hardware are you actually trying to describe?
> 
Agreed. Question is where to put the bindings, as they are not really 
specific to the extcon driver. The extcon driver merely implements 
the bindings. This is why the "compatible" statement reads "gpio-connector"
and not "extcon-something".

The bindings describe a connector managed through gpio pins.

> > +
> > +Required properties:
> > +	- compatible = "gpio-connector";
> > +	- presence-detect-gpios - presence detect gpio pin
> > +
> > +Optional properties:
> > +	- debounce-interval - debounce interval in milli-seconds
> > +	- state-on - on (connected) state
> > +	- state-off - off (disconnected) state
> > +	  Depending on the type of connector or cable, states may
> > +	  for example be reported as "connected"/"disconnected"
> > +	  or "inserted"/"removed".
> 
> I don't understand what the state-* properties describe. Do these
> provide semantic information to drivers? What is the full set of valid
> values?
> 
That is merely text which is ultimately passed on to the user.
Guess 'semantic information' might be a way to phrase it.

> > +
> > +Example node:
> > +
> > +	some-connector {
> > +		compatible = "gpio-connector";
> > +		presence-detect-gpios = <&gpio1 7 1>;
> > +		debounce-interval = <1>;
> > +		state-on = "connected";
> > +		state-on = "disconnected";
> > +	};
> 
> I'm not sure how much value this adds to bindings over describing the
> gpios directly. This seems to add a layer of indirection because of
> Linux internals.
> 
Not sure I understand what you mean with "describing the gpios directly".
Can you elaborate and/or provide an example ?

How would you describe the use of gpio pins used to detect board presence ?
That doesn't seem very Linux specific to me.

Thanks,
Guenter

Re: [RFC PATCH 5/6] extcon-gpio: Describe devicetree bindings

[Mark Rutland]

On Thu, Sep 12, 2013 at 05:53:04PM +0100, Guenter Roeck wrote:
> On Thu, Sep 12, 2013 at 05:41:00PM +0100, Mark Rutland wrote:
> > On Fri, Aug 30, 2013 at 05:29:37AM +0100, Guenter Roeck wrote:
> > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > ---
> > >  .../devicetree/bindings/extcon/extcon-gpio         |   23 ++++++++++++++++++++
> > >  1 file changed, 23 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/extcon/extcon-gpio
> > > 
> > > diff --git a/Documentation/devicetree/bindings/extcon/extcon-gpio b/Documentation/devicetree/bindings/extcon/extcon-gpio
> > > new file mode 100644
> > > index 0000000..091ddc6
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/extcon/extcon-gpio
> > > @@ -0,0 +1,23 @@
> > > +Device-Tree bindings for extcon/extcon-gpio driver
> > 
> > Bindings shouldn't refer to Linux-specifics like particular drivers.
> > What class of hardware are you actually trying to describe?
> > 
> Agreed. Question is where to put the bindings, as they are not really 
> specific to the extcon driver. The extcon driver merely implements 
> the bindings. This is why the "compatible" statement reads "gpio-connector"
> and not "extcon-something".
> 
> The bindings describe a connector managed through gpio pins.

Ok, then that's what the binding document should state.

> 
> > > +
> > > +Required properties:
> > > +	- compatible = "gpio-connector";
> > > +	- presence-detect-gpios - presence detect gpio pin
> > > +
> > > +Optional properties:
> > > +	- debounce-interval - debounce interval in milli-seconds
> > > +	- state-on - on (connected) state
> > > +	- state-off - off (disconnected) state
> > > +	  Depending on the type of connector or cable, states may
> > > +	  for example be reported as "connected"/"disconnected"
> > > +	  or "inserted"/"removed".
> > 
> > I don't understand what the state-* properties describe. Do these
> > provide semantic information to drivers? What is the full set of valid
> > values?
> > 
> That is merely text which is ultimately passed on to the user.
> Guess 'semantic information' might be a way to phrase it.

Where do these end up appearing to the user? Through names in the
filesystem? That's an ABI, which should be thoroughly described...

If it's arbitrary, why is it necessary at all? Surely sensible names for
the state of the connector can be coded in the driver for the device
attached to said connectors (which can be consistent and later changed
if necessary)...

> 
> > > +
> > > +Example node:
> > > +
> > > +	some-connector {
> > > +		compatible = "gpio-connector";
> > > +		presence-detect-gpios = <&gpio1 7 1>;
> > > +		debounce-interval = <1>;
> > > +		state-on = "connected";
> > > +		state-on = "disconnected";

I don't think that's a valid dts. I assume the second is meant to be
"state-off"?

> > > +	};
> > 
> > I'm not sure how much value this adds to bindings over describing the
> > gpios directly. This seems to add a layer of indirection because of
> > Linux internals.
> > 
> Not sure I understand what you mean with "describing the gpios directly".
> Can you elaborate and/or provide an example ?

Take a look at the MMC bindings [1], specifically the cd-gpios and
wp-gpios properties. I don't see why the connection of the GPIO needs to
be described by a wrapper device that doesn't really exist, when it can
be described directly.

> 
> How would you describe the use of gpio pins used to detect board presence ?
> That doesn't seem very Linux specific to me.

As above, with cd-gpios.

Thanks,
Mark.

[1] Documentation/devicetree/bindings/mmc/mmc.txt

Re: [RFC PATCH 5/6] extcon-gpio: Describe devicetree bindings

[Guenter Roeck]

On Mon, Sep 16, 2013 at 03:21:47PM +0100, Mark Rutland wrote:
> On Thu, Sep 12, 2013 at 05:53:04PM +0100, Guenter Roeck wrote:
> > On Thu, Sep 12, 2013 at 05:41:00PM +0100, Mark Rutland wrote:
> > > On Fri, Aug 30, 2013 at 05:29:37AM +0100, Guenter Roeck wrote:
> > > > Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> > > > ---
> > > >  .../devicetree/bindings/extcon/extcon-gpio         |   23 ++++++++++++++++++++
> > > >  1 file changed, 23 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/extcon/extcon-gpio
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/extcon/extcon-gpio b/Documentation/devicetree/bindings/extcon/extcon-gpio
> > > > new file mode 100644
> > > > index 0000000..091ddc6
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/extcon/extcon-gpio
> > > > @@ -0,0 +1,23 @@
> > > > +Device-Tree bindings for extcon/extcon-gpio driver
> > > 
> > > Bindings shouldn't refer to Linux-specifics like particular drivers.
> > > What class of hardware are you actually trying to describe?
> > > 
> > Agreed. Question is where to put the bindings, as they are not really 
> > specific to the extcon driver. The extcon driver merely implements 
> > the bindings. This is why the "compatible" statement reads "gpio-connector"
> > and not "extcon-something".
> > 
> > The bindings describe a connector managed through gpio pins.
> 
> Ok, then that's what the binding document should state.
> 
> > 
> > > > +
> > > > +Required properties:
> > > > +	- compatible = "gpio-connector";
> > > > +	- presence-detect-gpios - presence detect gpio pin
> > > > +
> > > > +Optional properties:
> > > > +	- debounce-interval - debounce interval in milli-seconds
> > > > +	- state-on - on (connected) state
> > > > +	- state-off - off (disconnected) state
> > > > +	  Depending on the type of connector or cable, states may
> > > > +	  for example be reported as "connected"/"disconnected"
> > > > +	  or "inserted"/"removed".
> > > 
> > > I don't understand what the state-* properties describe. Do these
> > > provide semantic information to drivers? What is the full set of valid
> > > values?
> > > 
> > That is merely text which is ultimately passed on to the user.
> > Guess 'semantic information' might be a way to phrase it.
> 
> Where do these end up appearing to the user? Through names in the

Yes.

> filesystem? That's an ABI, which should be thoroughly described...
> 
Tricky, as I can not really describe how the extcon driver implements it.
I can add something like above explanation, ie that the implementation is
expected to pass the property to the user. Would that be acceptable ?

> If it's arbitrary, why is it necessary at all? Surely sensible names for
> the state of the connector can be coded in the driver for the device
> attached to said connectors (which can be consistent and later changed
> if necessary)...
> 
Good point. I took a "hint" from the implementation in the extcon driver,
which lets one do that. But doesn't your comment apply to pretty much all
"label" and similar properties in the various devicetree descriptions ?

> > 
> > > > +
> > > > +Example node:
> > > > +
> > > > +	some-connector {
> > > > +		compatible = "gpio-connector";
> > > > +		presence-detect-gpios = <&gpio1 7 1>;
> > > > +		debounce-interval = <1>;
> > > > +		state-on = "connected";
> > > > +		state-on = "disconnected";
> 
> I don't think that's a valid dts. I assume the second is meant to be
> "state-off"?
> 
Yes, obviously.

> > > > +	};
> > > 
> > > I'm not sure how much value this adds to bindings over describing the
> > > gpios directly. This seems to add a layer of indirection because of
> > > Linux internals.
> > > 
> > Not sure I understand what you mean with "describing the gpios directly".
> > Can you elaborate and/or provide an example ?
> 
> Take a look at the MMC bindings [1], specifically the cd-gpios and
> wp-gpios properties. I don't see why the connection of the GPIO needs to
> be described by a wrapper device that doesn't really exist, when it can
> be described directly.
> 
Question is - described to what or for what ? 

I think what you are saying is that describing a generic connector via
devicetree is not acceptable, even though it _does_ describe hardware.
I would have to describe a specific connector for a specific hardware
instead, which in turn would need its own driver. Is that correct ?

Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 5/6] extcon-gpio: Describe devicetree bindings

[Mark Rutland]

On Mon, Sep 16, 2013 at 04:19:53PM +0100, Guenter Roeck wrote:
> On Mon, Sep 16, 2013 at 03:21:47PM +0100, Mark Rutland wrote:
> > On Thu, Sep 12, 2013 at 05:53:04PM +0100, Guenter Roeck wrote:
> > > On Thu, Sep 12, 2013 at 05:41:00PM +0100, Mark Rutland wrote:
> > > > On Fri, Aug 30, 2013 at 05:29:37AM +0100, Guenter Roeck wrote:
> > > > > Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> > > > > ---
> > > > >  .../devicetree/bindings/extcon/extcon-gpio         |   23 ++++++++++++++++++++
> > > > >  1 file changed, 23 insertions(+)
> > > > >  create mode 100644 Documentation/devicetree/bindings/extcon/extcon-gpio
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/extcon/extcon-gpio b/Documentation/devicetree/bindings/extcon/extcon-gpio
> > > > > new file mode 100644
> > > > > index 0000000..091ddc6
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/extcon/extcon-gpio
> > > > > @@ -0,0 +1,23 @@
> > > > > +Device-Tree bindings for extcon/extcon-gpio driver
> > > > 
> > > > Bindings shouldn't refer to Linux-specifics like particular drivers.
> > > > What class of hardware are you actually trying to describe?
> > > > 
> > > Agreed. Question is where to put the bindings, as they are not really 
> > > specific to the extcon driver. The extcon driver merely implements 
> > > the bindings. This is why the "compatible" statement reads "gpio-connector"
> > > and not "extcon-something".
> > > 
> > > The bindings describe a connector managed through gpio pins.
> > 
> > Ok, then that's what the binding document should state.
> > 
> > > 
> > > > > +
> > > > > +Required properties:
> > > > > +	- compatible = "gpio-connector";
> > > > > +	- presence-detect-gpios - presence detect gpio pin
> > > > > +
> > > > > +Optional properties:
> > > > > +	- debounce-interval - debounce interval in milli-seconds
> > > > > +	- state-on - on (connected) state
> > > > > +	- state-off - off (disconnected) state
> > > > > +	  Depending on the type of connector or cable, states may
> > > > > +	  for example be reported as "connected"/"disconnected"
> > > > > +	  or "inserted"/"removed".
> > > > 
> > > > I don't understand what the state-* properties describe. Do these
> > > > provide semantic information to drivers? What is the full set of valid
> > > > values?
> > > > 
> > > That is merely text which is ultimately passed on to the user.
> > > Guess 'semantic information' might be a way to phrase it.
> > 
> > Where do these end up appearing to the user? Through names in the
> 
> Yes.
> 
> > filesystem? That's an ABI, which should be thoroughly described...
> > 
> Tricky, as I can not really describe how the extcon driver implements it.
> I can add something like above explanation, ie that the implementation is
> expected to pass the property to the user. Would that be acceptable ?
> 
> > If it's arbitrary, why is it necessary at all? Surely sensible names for
> > the state of the connector can be coded in the driver for the device
> > attached to said connectors (which can be consistent and later changed
> > if necessary)...
> > 
> Good point. I took a "hint" from the implementation in the extcon driver,
> which lets one do that. But doesn't your comment apply to pretty much all
> "label" and similar properties in the various devicetree descriptions ?

I am also very much not a fan of those properties. They all give an
ill-defined ABI where Linux-specific details are in the DT. I don't see
why such properties should be necessary when it's possible to write
drivers in such a way as to not require them.

> 
> > > 
> > > > > +
> > > > > +Example node:
> > > > > +
> > > > > +	some-connector {
> > > > > +		compatible = "gpio-connector";
> > > > > +		presence-detect-gpios = <&gpio1 7 1>;
> > > > > +		debounce-interval = <1>;
> > > > > +		state-on = "connected";
> > > > > +		state-on = "disconnected";
> > 
> > I don't think that's a valid dts. I assume the second is meant to be
> > "state-off"?
> > 
> Yes, obviously.
> 
> > > > > +	};
> > > > 
> > > > I'm not sure how much value this adds to bindings over describing the
> > > > gpios directly. This seems to add a layer of indirection because of
> > > > Linux internals.
> > > > 
> > > Not sure I understand what you mean with "describing the gpios directly".
> > > Can you elaborate and/or provide an example ?
> > 
> > Take a look at the MMC bindings [1], specifically the cd-gpios and
> > wp-gpios properties. I don't see why the connection of the GPIO needs to
> > be described by a wrapper device that doesn't really exist, when it can
> > be described directly.
> > 
> Question is - described to what or for what ? 
> 
> I think what you are saying is that describing a generic connector via
> devicetree is not acceptable, even though it _does_ describe hardware.
> I would have to describe a specific connector for a specific hardware
> instead, which in turn would need its own driver. Is that correct ?

Regardless of how the connector is described, the block of hardware it
connects to will have to be described, and some description of the
connector will be necessary (either in the node for the block, or by
phandle to a node for the connector). I agree that having a combined IP
block + connector driver for each permutation is not good.

What I'm worried about is that we're pushing bindings for simple cases
(i.e. presences detect for single connectors detected via GPIOs) that we
have other mechanisms for describing (e.g. directly describing said
GPIOs), without considering the more general case and designing the
bindings to support that more general case from the start. We'll end up
designing bindings that are not flexible enough to describe the cases
that we already know exist, and then we'll have real pain attempting to
retrofit those cases into the binding.

The only existing consumer of extcon dt bindings seems to be omap-usb,
which seems to want a single extcon device describing the "USB" and
"USB-HOST" cables. I'm not happy with this binding staying in mainline
as-is, as it makes several assumptions that limits how future extcon
bindings can be implemented:
* It assumes an extcon provider (for lack of a better name) only has at
  most one cable of each type.
* It assumes the same extcon provider provides all cables required.
* It assumes extcon providers are referred to by phandle only (no
  additional cells of identifying information).

I think we need to consider the general case first, where a device
monitors the state of multiple cables, which may be of uniform or
disparate types.

I think it makes more sense to have a clk or gpio style binding, e.g.

gpio: {
	...
};

cables: some-extcon-provider {
	compatible = "vendor,cable-detect";
	/*
	 * This cable detector provides state detection for a power
	 * cable (0), and up to 8 USB host cables (1-8).
	 */
	#cable-detect-cells = <1>;
};

gpio-cable: some-gpio-detection {
	compatible = "linux,gpio-cable-detect";
	gpios = <&gpio 4 2 3>;
	/*
	 * Maybe multiple GPIOs could be allowed instead?
	 */
	#cable-detect-cells = <0>;
};

some-extcon-consumer {
	compatible = "vendor,usb-host-device";

	/*
	 * This could instead be separate power-cables and usb-cables
	 * properties like the gpio bindings, but then describing an
	 * arbitrary subset of USB host cables becomes difficult (you
	 * end up needing separate usbX-gpios properties, from the
	 * start).
	 */
	cables = <&cable 0>,
		 <&cable 5>,
		 <&cable 6>;,
		 <&gpio-cable>;
	cable-names = "power", "usb3", "usb4", "usb6";
};

Another concern is that extcon handles more than just a binary state for
each cable type, and the precise definition of these states is
Linux-specific AFAICS. I'd prefer to keep this concern out of the DT as
much as possible -- an extcon driver should know the types of its
cables, and the consumer should know the types it expects to request. As
such, there's no need to encode "USB-HOST" and friends in the DT -- only
the name of the particular instance of that type of cable should appear.
This obviously makes it difficult to have generic wrappers such as those
using GPIOs -- a platform may require an arbitrary set of GPIOs which
detect an arbitrary value when a certain cable is in a specific state.

We *need* to consider the general case before we start proliferating
bindings, or we'll end up creating bindings that cannot express the
general case. Shortcuts we take early on to make implementation easier
create real problems for us later on, because they become ABI.

Cheers,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 5/6] extcon-gpio: Describe devicetree bindings

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1177 bytes --]

On Wed, Sep 18, 2013 at 04:38:28PM +0100, Mark Rutland wrote:
> On Mon, Sep 16, 2013 at 04:19:53PM +0100, Guenter Roeck wrote:

> > I think what you are saying is that describing a generic connector via
> > devicetree is not acceptable, even though it _does_ describe hardware.
> > I would have to describe a specific connector for a specific hardware
> > instead, which in turn would need its own driver. Is that correct ?

> Regardless of how the connector is described, the block of hardware it
> connects to will have to be described, and some description of the
> connector will be necessary (either in the node for the block, or by
> phandle to a node for the connector). I agree that having a combined IP
> block + connector driver for each permutation is not good.

Many of the things described only have passive components attached, or
things that otherwise don't need drivers - things like power inputs or
headphone connectors, they're mainly providing information to allow
userspace to behave differently (eg, display a charging indicator in the
UI).  It's not 100% true but by and by large if detection is being done
using a GPIO it's probably something like that.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH 5/6] extcon-gpio: Describe devicetree bindings

[Guenter Roeck]

On Thu, Sep 19, 2013 at 05:42:45PM +0100, Mark Brown wrote:
> On Wed, Sep 18, 2013 at 04:38:28PM +0100, Mark Rutland wrote:
> > On Mon, Sep 16, 2013 at 04:19:53PM +0100, Guenter Roeck wrote:
> 
> > > I think what you are saying is that describing a generic connector via
> > > devicetree is not acceptable, even though it _does_ describe hardware.
> > > I would have to describe a specific connector for a specific hardware
> > > instead, which in turn would need its own driver. Is that correct ?
> 
> > Regardless of how the connector is described, the block of hardware it
> > connects to will have to be described, and some description of the
> > connector will be necessary (either in the node for the block, or by
> > phandle to a node for the connector). I agree that having a combined IP
> > block + connector driver for each permutation is not good.
> 
> Many of the things described only have passive components attached, or
> things that otherwise don't need drivers - things like power inputs or
> headphone connectors, they're mainly providing information to allow
> userspace to behave differently (eg, display a charging indicator in the
> UI).  It's not 100% true but by and by large if detection is being done
> using a GPIO it's probably something like that.

Correct. However, gpio based 'detect' pins typically need debounce support
which is not directly available through the gpio userspace API. I tried to add
that earlier, but was told to use extcon instead as it provides the necessary
infrastructure. Now it almost looks like I can not use it either because the
required devicetree bindings may be considered unacceptable.

Guenter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 5/6] extcon-gpio: Describe devicetree bindings

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1069 bytes --]

On Thu, Sep 19, 2013 at 11:28:50AM -0700, Guenter Roeck wrote:
> On Thu, Sep 19, 2013 at 05:42:45PM +0100, Mark Brown wrote:

> > Many of the things described only have passive components attached, or
> > things that otherwise don't need drivers - things like power inputs or
> > headphone connectors, they're mainly providing information to allow
> > userspace to behave differently (eg, display a charging indicator in the
> > UI).  It's not 100% true but by and by large if detection is being done
> > using a GPIO it's probably something like that.

> Correct. However, gpio based 'detect' pins typically need debounce support
> which is not directly available through the gpio userspace API. I tried to add
> that earlier, but was told to use extcon instead as it provides the necessary
> infrastructure. Now it almost looks like I can not use it either because the
> required devicetree bindings may be considered unacceptable.

Yes, I'm trying to explain to Mark why there are drivers just purely for
the connector without any software for the connected device.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[RFC PATCH 6/6] extcon-gpio: Describe possible properties to support multi-type cables

[Guenter Roeck]

This is purely a possible description and an RFC; there is no code (yet).

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 .../devicetree/bindings/extcon/extcon-gpio         |   26 ++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/Documentation/devicetree/bindings/extcon/extcon-gpio b/Documentation/devicetree/bindings/extcon/extcon-gpio
index 091ddc6..5836ac2 100644
--- a/Documentation/devicetree/bindings/extcon/extcon-gpio
+++ b/Documentation/devicetree/bindings/extcon/extcon-gpio
@@ -21,3 +21,29 @@ Example node:
 		state-on = "connected";
 		state-on = "disconnected";
 	};
+
+---
+TBD: Add support for multiple connectors
+
+An example node with multiple connectors might look as follows.
+
+	some-connector {
+		#size-cells = <1>;
+		compatible = "gpio-connector";
+		presence-detect-gpios = <&gpio1 7 1>;
+		id-gpios = <&gpio1 8 0>;
+		debounce-interval = <1>;
+		state-on = "connected";
+		state-on = "disconnected";
+
+		USB {
+			reg = <0>;
+		};
+		USB-Host {
+			reg = <1>;
+		};
+	};
+
+This describes a cable with a (low-active) presence detect pin and an ID pin.
+If the value returned by the ID pin is 0, the connected cable type is "USB".
+If the value is 1, the connected cable type is "USB-Host".
-- 
1.7.9.7