[PATCH] extcon-gpio: add devicetree support.

[PATCH] extcon-gpio: add devicetree support.

[NeilBrown]

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


As this device is not vendor specific, I haven't included any "vendor,"
prefixes.  For my model I used "regulator-gpio" which takes a similar
approach.

Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/Documentation/devicetree/bindings/extcon/extcon-gpio.txt b/Documentation/devicetree/bindings/extcon/extcon-gpio.txt
new file mode 100644
index 000000000000..2346b61cc620
--- /dev/null
+++ b/Documentation/devicetree/bindings/extcon/extcon-gpio.txt
@@ -0,0 +1,26 @@
+* EXTCON detector using GPIO
+
+Required Properties:
+ - compatible: "extcon-gpio"
+ - gpios: gpio line that detects connector
+ - interrupts: interrupt generated by that gpio
+ - debounce-delay-ms: debouncing delay
+
+Optional Properties:
+ - label: name for connector.  If not given, device name is used.
+ - state-on: string to report when GPIO is high (else '0')
+ - state-off: string to report when GPIO is low (else '1')
+
+Example:
+
+antenna-detect {
+	compatible = "extcon-gpio";
+	label = "gps_antenna";
+	interrupt-parent = <&gpio5>;
+	interrupts = <16 IRQ_TYPE_EDGE_BOTH>;
+	gpios = <&gpio5 16 0>;
+	debounce-delay-ms = <10>;
+	state-on = "external";
+	state-off = "internal";
+};
+
diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
index f874c30ddbff..a683d2b99213 100644
--- a/drivers/extcon/extcon-gpio.c
+++ b/drivers/extcon/extcon-gpio.c
@@ -30,6 +30,9 @@
 #include <linux/gpio.h>
 #include <linux/extcon.h>
 #include <linux/extcon/extcon-gpio.h>
+#include <linux/of_platform.h>
+#include <linux/of_gpio.h>
+#include <linux/of_irq.h>
 
 struct gpio_extcon_data {
 	struct extcon_dev edev;
@@ -76,6 +79,45 @@ static ssize_t extcon_gpio_print_state(struct extcon_dev *edev, char *buf)
 	return -EINVAL;
 }
 
+#ifdef CONFIG_OF
+static struct gpio_extcon_platform_data *
+gpio_extcon_parse(struct device *dev)
+{
+	struct gpio_extcon_platform_data *pdata;
+	struct device_node *np = dev->of_node;
+	u32 num;
+	int irq;
+
+	if (!np)
+		return NULL;
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return NULL;
+	pdata->name = dev_name(dev);
+	of_property_read_string(np, "label", &pdata->name);
+	irq = irq_of_parse_and_map(np, 0);
+	pdata->irq_flags = irq_get_trigger_type(irq);
+	pdata->gpio = of_get_named_gpio(np, "gpios", 0);
+	if (pdata->gpio < 0)
+		return ERR_PTR(pdata->gpio);
+	if (irq != gpio_to_irq(pdata->gpio)) {
+		dev_err(dev, "IRQ doesn't match GPIO\n");
+		return ERR_PTR(-EINVAL);
+	}
+	if (of_property_read_u32(np, "debounce-delay-ms", &num) == 0)
+		pdata->debounce = num;
+	of_property_read_string(np, "state-on", &pdata->state_on);
+	of_property_read_string(np, "state-off", &pdata->state_off);
+	return pdata;
+}
+#else
+static inline struct gpio_extcon_platform_data *
+gpio_extcon_parse(struct device *dev)
+{
+	return NULL;
+}
+#endif
+
 static int gpio_extcon_probe(struct platform_device *pdev)
 {
 	struct gpio_extcon_platform_data *pdata = pdev->dev.platform_data;
@@ -83,7 +125,11 @@ static int gpio_extcon_probe(struct platform_device *pdev)
 	int ret = 0;
 
 	if (!pdata)
+		pdata = gpio_extcon_parse(&pdev->dev);
+	if (!pdata)
 		return -EBUSY;
+	if (IS_ERR(pdata))
+		return PTR_ERR(pdata);
 	if (!pdata->irq_flags) {
 		dev_err(&pdev->dev, "IRQ flag is not specified.\n");
 		return -EINVAL;
@@ -148,12 +194,19 @@ static int gpio_extcon_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct of_device_id gpio_extcon_match[] = {
+	{ .compatible = "extcon-gpio" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, gpio_extcon_mastch);
+
 static struct platform_driver gpio_extcon_driver = {
 	.probe		= gpio_extcon_probe,
 	.remove		= gpio_extcon_remove,
 	.driver		= {
 		.name	= "extcon-gpio",
 		.owner	= THIS_MODULE,
+		.of_match_table = gpio_extcon_match,
 	},
 };
 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

[PATCH] extcon-gpio: add devicetree support.

[NeilBrown]

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


As this device is not vendor specific, I haven't included any "vendor,"
prefixes.  For my model I used "regulator-gpio" which takes a similar
approach.

Signed-off-by: NeilBrown <neilb-l3A5Bk7waGM@public.gmane.org>

diff --git a/Documentation/devicetree/bindings/extcon/extcon-gpio.txt b/Documentation/devicetree/bindings/extcon/extcon-gpio.txt
new file mode 100644
index 000000000000..2346b61cc620
--- /dev/null
+++ b/Documentation/devicetree/bindings/extcon/extcon-gpio.txt
@@ -0,0 +1,26 @@
+* EXTCON detector using GPIO
+
+Required Properties:
+ - compatible: "extcon-gpio"
+ - gpios: gpio line that detects connector
+ - interrupts: interrupt generated by that gpio
+ - debounce-delay-ms: debouncing delay
+
+Optional Properties:
+ - label: name for connector.  If not given, device name is used.
+ - state-on: string to report when GPIO is high (else '0')
+ - state-off: string to report when GPIO is low (else '1')
+
+Example:
+
+antenna-detect {
+	compatible = "extcon-gpio";
+	label = "gps_antenna";
+	interrupt-parent = <&gpio5>;
+	interrupts = <16 IRQ_TYPE_EDGE_BOTH>;
+	gpios = <&gpio5 16 0>;
+	debounce-delay-ms = <10>;
+	state-on = "external";
+	state-off = "internal";
+};
+
diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
index f874c30ddbff..a683d2b99213 100644
--- a/drivers/extcon/extcon-gpio.c
+++ b/drivers/extcon/extcon-gpio.c
@@ -30,6 +30,9 @@
 #include <linux/gpio.h>
 #include <linux/extcon.h>
 #include <linux/extcon/extcon-gpio.h>
+#include <linux/of_platform.h>
+#include <linux/of_gpio.h>
+#include <linux/of_irq.h>
 
 struct gpio_extcon_data {
 	struct extcon_dev edev;
@@ -76,6 +79,45 @@ static ssize_t extcon_gpio_print_state(struct extcon_dev *edev, char *buf)
 	return -EINVAL;
 }
 
+#ifdef CONFIG_OF
+static struct gpio_extcon_platform_data *
+gpio_extcon_parse(struct device *dev)
+{
+	struct gpio_extcon_platform_data *pdata;
+	struct device_node *np = dev->of_node;
+	u32 num;
+	int irq;
+
+	if (!np)
+		return NULL;
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return NULL;
+	pdata->name = dev_name(dev);
+	of_property_read_string(np, "label", &pdata->name);
+	irq = irq_of_parse_and_map(np, 0);
+	pdata->irq_flags = irq_get_trigger_type(irq);
+	pdata->gpio = of_get_named_gpio(np, "gpios", 0);
+	if (pdata->gpio < 0)
+		return ERR_PTR(pdata->gpio);
+	if (irq != gpio_to_irq(pdata->gpio)) {
+		dev_err(dev, "IRQ doesn't match GPIO\n");
+		return ERR_PTR(-EINVAL);
+	}
+	if (of_property_read_u32(np, "debounce-delay-ms", &num) == 0)
+		pdata->debounce = num;
+	of_property_read_string(np, "state-on", &pdata->state_on);
+	of_property_read_string(np, "state-off", &pdata->state_off);
+	return pdata;
+}
+#else
+static inline struct gpio_extcon_platform_data *
+gpio_extcon_parse(struct device *dev)
+{
+	return NULL;
+}
+#endif
+
 static int gpio_extcon_probe(struct platform_device *pdev)
 {
 	struct gpio_extcon_platform_data *pdata = pdev->dev.platform_data;
@@ -83,7 +125,11 @@ static int gpio_extcon_probe(struct platform_device *pdev)
 	int ret = 0;
 
 	if (!pdata)
+		pdata = gpio_extcon_parse(&pdev->dev);
+	if (!pdata)
 		return -EBUSY;
+	if (IS_ERR(pdata))
+		return PTR_ERR(pdata);
 	if (!pdata->irq_flags) {
 		dev_err(&pdev->dev, "IRQ flag is not specified.\n");
 		return -EINVAL;
@@ -148,12 +194,19 @@ static int gpio_extcon_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct of_device_id gpio_extcon_match[] = {
+	{ .compatible = "extcon-gpio" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, gpio_extcon_mastch);
+
 static struct platform_driver gpio_extcon_driver = {
 	.probe		= gpio_extcon_probe,
 	.remove		= gpio_extcon_remove,
 	.driver		= {
 		.name	= "extcon-gpio",
 		.owner	= THIS_MODULE,
+		.of_match_table = gpio_extcon_match,
 	},
 };
 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] extcon-gpio: add devicetree support.

[Mark Rutland]

Hi Neil,

While I'm not fundamentally opposed to this binding, I have some issues with
its current form and would not want to see this version hit mainline.

On Fri, Nov 01, 2013 at 09:50:05AM +0000, NeilBrown wrote:
> 
> As this device is not vendor specific, I haven't included any "vendor,"
> prefixes.  For my model I used "regulator-gpio" which takes a similar
> approach.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> 
> diff --git a/Documentation/devicetree/bindings/extcon/extcon-gpio.txt b/Documentation/devicetree/bindings/extcon/extcon-gpio.txt
> new file mode 100644
> index 000000000000..2346b61cc620
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/extcon/extcon-gpio.txt
> @@ -0,0 +1,26 @@
> +* EXTCON detector using GPIO

EXTCON is _extremely_ Linux-specific. The binding document needs a description
of the class of device it's inteded to describe that does not just refer to
Linux internals.

I would prefer if we could have a better name for this that was not tied to the
Linux driver name. Perhaps "gpio-presence-detector"?

> +
> +Required Properties:
> + - compatible: "extcon-gpio"
> + - gpios: gpio line that detects connector
> + - interrupts: interrupt generated by that gpio

We don't need this. If we need the interrupt a gpio generates, we should ask
the gpio controller driver to map the gpio to an interrupt.

We have gpiod_to_irq for this in Linux.

> + - debounce-delay-ms: debouncing delay
> +
> +Optional Properties:
> + - label: name for connector.  If not given, device name is used.
> + - state-on: string to report when GPIO is high (else '0')
> + - state-off: string to report when GPIO is low (else '1')

I do not like these properties, they are very much a Linux implementation
detail.

Are extcon devices ever used standalone? If so, why?

If not I see _no_ reason at all for the label property. If a userspace
application needs to detect the presence of a particular external connector, it
will need to know this in relation to the device the external connectors are
attached to. In that case the application should find that device and traverse
its set of extcon devices. The names for the external connections will be a
property of the device, not the extcon devices themselves (along hte same lines
as clocks), and need not be a property of the extcon device.

As for state-on and state-off, we are exposing a binary property to userspace,
the meaning of which is already dependent on the particular device the extcon
devices are connected to. Given userspace already has to understand that, I see
no value in embedding arbitrary strings in the device tree which attempt to
replicate this knowledge. They provide no additional value and in my mind make
the interface to userspace harder to use because you have ot handle an
arbitrary set of values depending on how the dt author felt one morning rather
than just the binary value you actually care about.

Thanks,
Mark.

Re: [PATCH] extcon-gpio: add devicetree support.

[NeilBrown]

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

On Fri, 1 Nov 2013 10:16:44 -0700 Mark Rutland <mark.rutland@arm.com> wrote:

> Hi Neil,
> 
> While I'm not fundamentally opposed to this binding, I have some issues with
> its current form and would not want to see this version hit mainline.
> 

Thanks for the review.

> On Fri, Nov 01, 2013 at 09:50:05AM +0000, NeilBrown wrote:
> > 
> > As this device is not vendor specific, I haven't included any "vendor,"
> > prefixes.  For my model I used "regulator-gpio" which takes a similar
> > approach.
> > 
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > 
> > diff --git a/Documentation/devicetree/bindings/extcon/extcon-gpio.txt b/Documentation/devicetree/bindings/extcon/extcon-gpio.txt
> > new file mode 100644
> > index 000000000000..2346b61cc620
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/extcon/extcon-gpio.txt
> > @@ -0,0 +1,26 @@
> > +* EXTCON detector using GPIO
> 
> EXTCON is _extremely_ Linux-specific. The binding document needs a description
> of the class of device it's inteded to describe that does not just refer to
> Linux internals.
> 
> I would prefer if we could have a better name for this that was not tied to the
> Linux driver name. Perhaps "gpio-presence-detector"?

Maybe "cable-presence-detector" as in this case the GPIO is just an
implementation detail.  Which isn't much different from "external-connector"
which is where "extcon" comes from...

I propose "external-connector" if you don't like "extcon".


> 
> > +
> > +Required Properties:
> > + - compatible: "extcon-gpio"
> > + - gpios: gpio line that detects connector
> > + - interrupts: interrupt generated by that gpio
> 
> We don't need this. If we need the interrupt a gpio generates, we should ask
> the gpio controller driver to map the gpio to an interrupt.
> 
> We have gpiod_to_irq for this in Linux.

The reason I did this was that the pre-existing platform_data wants
'irq_flags'.  I could have an 'irq-flags' property, but it seems to make more
sense to use "interrupts" as that already provides a way to pass irq-flags to
a device.

On reflection though, I cannot imagine why any extcon-gpio would use anything
other than  IRQ_TYPE_EDGE_BOTH.  Maybe MyungJoo Ham can explain that???

If there is no need for specifying irq-flags per-platform, the "interrupts"
property can definitely go.


> 
> > + - debounce-delay-ms: debouncing delay
> > +
> > +Optional Properties:
> > + - label: name for connector.  If not given, device name is used.
> > + - state-on: string to report when GPIO is high (else '0')
> > + - state-off: string to report when GPIO is low (else '1')
> 
> I do not like these properties, they are very much a Linux implementation
> detail.
> 
> Are extcon devices ever used standalone? If so, why?

I'm not sure what you mean by stand alone - it is part of a mobile-phone
motherboard so it certainly isn't alone :-)

The board has a GPS which is connected to a serial port.  So the kernel
doesn't really need to know much about it.
There is an internal antenna and a connector for an external antenna.  There
is some clever electronics that detects when the external antenna is plugged
in and re-routes the antenna power to the external (and way from the
internal).
A gpio can read the state of this electronic switch.  It seems sensible to
present this to user-space as an 'extcon' device.  It is stand-alone only in
that the kernel doesn't "know" that it is related to anything else.  I and
the user-space software know that it isn't "alone".

Given that there a two antennas, internal and external, and always one is
connected, it seems sensible to present it that way.

However I don't object to the connection being called  "external-antenna"
which reports either "0" or "1".
That would make "state-on" and "state-off" unnecessary and I see no
problem with removing them.

I do think it is necessary to have a "label" for the  external connector
though.  It is a specific connector with a specific purpose and deserves to
have a "label", in exactly the same way that "leds" devices and provide a
label for each LED.

> 
> If not I see _no_ reason at all for the label property. If a userspace
> application needs to detect the presence of a particular external connector, it
> will need to know this in relation to the device the external connectors are
> attached to. In that case the application should find that device and traverse
> its set of extcon devices. The names for the external connections will be a
> property of the device, not the extcon devices themselves (along hte same lines
> as clocks), and need not be a property of the extcon device.

This sounds interesting but I don't follow exactly what you mean.
In particular, where would an application "find that device" and how would it
"traverse the set of extcon devices"?
And if there are multiple extcons in a parent, how can the name of the extcon
be a property of the parent?

Confused... maybe I should explore the 'clocks' to which you refer...

> 
> As for state-on and state-off, we are exposing a binary property to userspace,
> the meaning of which is already dependent on the particular device the extcon
> devices are connected to. Given userspace already has to understand that, I see
> no value in embedding arbitrary strings in the device tree which attempt to
> replicate this knowledge. They provide no additional value and in my mind make
> the interface to userspace harder to use because you have ot handle an
> arbitrary set of values depending on how the dt author felt one morning rather
> than just the binary value you actually care about.

I agree with this.  I think the history is that this driver was originally
described as a "switch" which could have two states which deserved names.
As an 'external-connector' device, that makes less sense.
(tough it might make sense to device a "switch" device-tree type that this
driver could also support...)

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] extcon-gpio: add devicetree support.

[NeilBrown]

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

On Fri, 1 Nov 2013 10:16:44 -0700 Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:

> Hi Neil,
> 
> While I'm not fundamentally opposed to this binding, I have some issues with
> its current form and would not want to see this version hit mainline.
> 

Thanks for the review.

> On Fri, Nov 01, 2013 at 09:50:05AM +0000, NeilBrown wrote:
> > 
> > As this device is not vendor specific, I haven't included any "vendor,"
> > prefixes.  For my model I used "regulator-gpio" which takes a similar
> > approach.
> > 
> > Signed-off-by: NeilBrown <neilb-l3A5Bk7waGM@public.gmane.org>
> > 
> > diff --git a/Documentation/devicetree/bindings/extcon/extcon-gpio.txt b/Documentation/devicetree/bindings/extcon/extcon-gpio.txt
> > new file mode 100644
> > index 000000000000..2346b61cc620
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/extcon/extcon-gpio.txt
> > @@ -0,0 +1,26 @@
> > +* EXTCON detector using GPIO
> 
> EXTCON is _extremely_ Linux-specific. The binding document needs a description
> of the class of device it's inteded to describe that does not just refer to
> Linux internals.
> 
> I would prefer if we could have a better name for this that was not tied to the
> Linux driver name. Perhaps "gpio-presence-detector"?

Maybe "cable-presence-detector" as in this case the GPIO is just an
implementation detail.  Which isn't much different from "external-connector"
which is where "extcon" comes from...

I propose "external-connector" if you don't like "extcon".


> 
> > +
> > +Required Properties:
> > + - compatible: "extcon-gpio"
> > + - gpios: gpio line that detects connector
> > + - interrupts: interrupt generated by that gpio
> 
> We don't need this. If we need the interrupt a gpio generates, we should ask
> the gpio controller driver to map the gpio to an interrupt.
> 
> We have gpiod_to_irq for this in Linux.

The reason I did this was that the pre-existing platform_data wants
'irq_flags'.  I could have an 'irq-flags' property, but it seems to make more
sense to use "interrupts" as that already provides a way to pass irq-flags to
a device.

On reflection though, I cannot imagine why any extcon-gpio would use anything
other than  IRQ_TYPE_EDGE_BOTH.  Maybe MyungJoo Ham can explain that???

If there is no need for specifying irq-flags per-platform, the "interrupts"
property can definitely go.


> 
> > + - debounce-delay-ms: debouncing delay
> > +
> > +Optional Properties:
> > + - label: name for connector.  If not given, device name is used.
> > + - state-on: string to report when GPIO is high (else '0')
> > + - state-off: string to report when GPIO is low (else '1')
> 
> I do not like these properties, they are very much a Linux implementation
> detail.
> 
> Are extcon devices ever used standalone? If so, why?

I'm not sure what you mean by stand alone - it is part of a mobile-phone
motherboard so it certainly isn't alone :-)

The board has a GPS which is connected to a serial port.  So the kernel
doesn't really need to know much about it.
There is an internal antenna and a connector for an external antenna.  There
is some clever electronics that detects when the external antenna is plugged
in and re-routes the antenna power to the external (and way from the
internal).
A gpio can read the state of this electronic switch.  It seems sensible to
present this to user-space as an 'extcon' device.  It is stand-alone only in
that the kernel doesn't "know" that it is related to anything else.  I and
the user-space software know that it isn't "alone".

Given that there a two antennas, internal and external, and always one is
connected, it seems sensible to present it that way.

However I don't object to the connection being called  "external-antenna"
which reports either "0" or "1".
That would make "state-on" and "state-off" unnecessary and I see no
problem with removing them.

I do think it is necessary to have a "label" for the  external connector
though.  It is a specific connector with a specific purpose and deserves to
have a "label", in exactly the same way that "leds" devices and provide a
label for each LED.

> 
> If not I see _no_ reason at all for the label property. If a userspace
> application needs to detect the presence of a particular external connector, it
> will need to know this in relation to the device the external connectors are
> attached to. In that case the application should find that device and traverse
> its set of extcon devices. The names for the external connections will be a
> property of the device, not the extcon devices themselves (along hte same lines
> as clocks), and need not be a property of the extcon device.

This sounds interesting but I don't follow exactly what you mean.
In particular, where would an application "find that device" and how would it
"traverse the set of extcon devices"?
And if there are multiple extcons in a parent, how can the name of the extcon
be a property of the parent?

Confused... maybe I should explore the 'clocks' to which you refer...

> 
> As for state-on and state-off, we are exposing a binary property to userspace,
> the meaning of which is already dependent on the particular device the extcon
> devices are connected to. Given userspace already has to understand that, I see
> no value in embedding arbitrary strings in the device tree which attempt to
> replicate this knowledge. They provide no additional value and in my mind make
> the interface to userspace harder to use because you have ot handle an
> arbitrary set of values depending on how the dt author felt one morning rather
> than just the binary value you actually care about.

I agree with this.  I think the history is that this driver was originally
described as a "switch" which could have two states which deserved names.
As an 'external-connector' device, that makes less sense.
(tough it might make sense to device a "switch" device-tree type that this
driver could also support...)

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] extcon-gpio: add devicetree support.

[NeilBrown]

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

On Sat, 2 Nov 2013 10:33:23 +1100 NeilBrown <neilb@suse.de> wrote:

> On Fri, 1 Nov 2013 10:16:44 -0700 Mark Rutland <mark.rutland@arm.com> wrote:
> 
> > Hi Neil,
> > 
> > While I'm not fundamentally opposed to this binding, I have some issues with
> > its current form and would not want to see this version hit mainline.
> > 
> 
> Thanks for the review.
> 
> > On Fri, Nov 01, 2013 at 09:50:05AM +0000, NeilBrown wrote:
> > > 
> > > As this device is not vendor specific, I haven't included any "vendor,"
> > > prefixes.  For my model I used "regulator-gpio" which takes a similar
> > > approach.
> > > 
> > > Signed-off-by: NeilBrown <neilb@suse.de>
> > > 
> > > diff --git a/Documentation/devicetree/bindings/extcon/extcon-gpio.txt b/Documentation/devicetree/bindings/extcon/extcon-gpio.txt
> > > new file mode 100644
> > > index 000000000000..2346b61cc620
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/extcon/extcon-gpio.txt
> > > @@ -0,0 +1,26 @@
> > > +* EXTCON detector using GPIO
> > 
> > EXTCON is _extremely_ Linux-specific. The binding document needs a description
> > of the class of device it's inteded to describe that does not just refer to
> > Linux internals.
> > 
> > I would prefer if we could have a better name for this that was not tied to the
> > Linux driver name. Perhaps "gpio-presence-detector"?
> 
> Maybe "cable-presence-detector" as in this case the GPIO is just an
> implementation detail.  Which isn't much different from "external-connector"
> which is where "extcon" comes from...
> 
> I propose "external-connector" if you don't like "extcon".

Uhm.. I just realised that what I said here doesn't make any sense - sorry.
I was thinking that you were suggesting "gpio-presence-detector" as a
replacement for "extcon", but of course you weren't.  It is a possible
replacement for "extcon-gpio", which makes lots of sense.

The "Extcon" driver works in two modes.  One in which is can detect which of
several possible cables is inserted, and one in which is simply detects if
any cable is inserted at all.

extcon-gpio only supports the second.  And for the second, I'm beginning to
think that "presence-detector" is quite a good name.

So yes: unless some serious objection arises, gpio-presence-detector is what
I'll use in my next patch.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] extcon-gpio: add devicetree support.

[Guenter Roeck]

On 11/01/2013 04:33 PM, NeilBrown wrote:
> On Fri, 1 Nov 2013 10:16:44 -0700 Mark Rutland <mark.rutland@arm.com> wrote:
>
>> Hi Neil,
>>
>> While I'm not fundamentally opposed to this binding, I have some issues with
>> its current form and would not want to see this version hit mainline.
>>
>
> Thanks for the review.
>
>> On Fri, Nov 01, 2013 at 09:50:05AM +0000, NeilBrown wrote:
>>>
>>> As this device is not vendor specific, I haven't included any "vendor,"
>>> prefixes.  For my model I used "regulator-gpio" which takes a similar
>>> approach.
>>>
>>> Signed-off-by: NeilBrown <neilb@suse.de>
>>>
>>> diff --git a/Documentation/devicetree/bindings/extcon/extcon-gpio.txt b/Documentation/devicetree/bindings/extcon/extcon-gpio.txt
>>> new file mode 100644
>>> index 000000000000..2346b61cc620
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/extcon/extcon-gpio.txt
>>> @@ -0,0 +1,26 @@
>>> +* EXTCON detector using GPIO
>>
>> EXTCON is _extremely_ Linux-specific. The binding document needs a description
>> of the class of device it's inteded to describe that does not just refer to
>> Linux internals.
>>
>> I would prefer if we could have a better name for this that was not tied to the
>> Linux driver name. Perhaps "gpio-presence-detector"?
>
> Maybe "cable-presence-detector" as in this case the GPIO is just an
> implementation detail.  Which isn't much different from "external-connector"
> which is where "extcon" comes from...
>
> I propose "external-connector" if you don't like "extcon".
>
>
>>
>>> +
>>> +Required Properties:
>>> + - compatible: "extcon-gpio"
>>> + - gpios: gpio line that detects connector
>>> + - interrupts: interrupt generated by that gpio
>>
>> We don't need this. If we need the interrupt a gpio generates, we should ask
>> the gpio controller driver to map the gpio to an interrupt.
>>
>> We have gpiod_to_irq for this in Linux.
>
> The reason I did this was that the pre-existing platform_data wants
> 'irq_flags'.  I could have an 'irq-flags' property, but it seems to make more
> sense to use "interrupts" as that already provides a way to pass irq-flags to
> a device.
>
> On reflection though, I cannot imagine why any extcon-gpio would use anything
> other than  IRQ_TYPE_EDGE_BOTH.  Maybe MyungJoo Ham can explain that???
>
> If there is no need for specifying irq-flags per-platform, the "interrupts"
> property can definitely go.
>

When I tried to add DT support to extcon-pio, I used IRQ_TYPE_EDGE_BOTH.
See [1]. A useful fallback for 'label' - if not specified - may be
np->name, to match LED functionality.

A patch to add 'active low' to the platform data for extcon-gpio is pending
in linux-next. It might make sense to extract this flag from the gpio flags
and add it as well. Again see [1] for an example how this could be implemented.

Guenter

[1] https://lkml.org/lkml/2013/8/30/26

Re: [PATCH] extcon-gpio: add devicetree support.

[Mark Brown]

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

On Fri, Nov 01, 2013 at 10:16:44AM -0700, Mark Rutland wrote:
> On Fri, Nov 01, 2013 at 09:50:05AM +0000, NeilBrown wrote:

> > + - label: name for connector.  If not given, device name is used.

> Are extcon devices ever used standalone? If so, why?

They are sometimes used for things that don't have/need any other
software representation in the system like detection of attachment of
covers or as bundles for other connectors (like the Apple 30 pin cables,
they have the base connector with multiple possible things connected on
it).

> If not I see _no_ reason at all for the label property. If a userspace
> application needs to detect the presence of a particular external connector, it
> will need to know this in relation to the device the external connectors are
> attached to. In that case the application should find that device and traverse
> its set of extcon devices. The names for the external connections will be a
> property of the device, not the extcon devices themselves (along hte same lines
> as clocks), and need not be a property of the extcon device.

This is often done for user display purposes rather than for the
application and is sometimes done from the perspective of "what's
plugged into my system" (eg, helping someone cable up their system)
rather than from the point of view of using an individual device.

For example the HDA spec connector objects include information like the
connector label (ie, writing on the case) and colour as part of the
object for the connector and DMI does similar things for PCI slots.

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

Re: [PATCH] extcon-gpio: add devicetree support.

[Mark Brown]

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

On Fri, Nov 01, 2013 at 10:16:44AM -0700, Mark Rutland wrote:
> On Fri, Nov 01, 2013 at 09:50:05AM +0000, NeilBrown wrote:

> > + - label: name for connector.  If not given, device name is used.

> Are extcon devices ever used standalone? If so, why?

They are sometimes used for things that don't have/need any other
software representation in the system like detection of attachment of
covers or as bundles for other connectors (like the Apple 30 pin cables,
they have the base connector with multiple possible things connected on
it).

> If not I see _no_ reason at all for the label property. If a userspace
> application needs to detect the presence of a particular external connector, it
> will need to know this in relation to the device the external connectors are
> attached to. In that case the application should find that device and traverse
> its set of extcon devices. The names for the external connections will be a
> property of the device, not the extcon devices themselves (along hte same lines
> as clocks), and need not be a property of the extcon device.

This is often done for user display purposes rather than for the
application and is sometimes done from the perspective of "what's
plugged into my system" (eg, helping someone cable up their system)
rather than from the point of view of using an individual device.

For example the HDA spec connector objects include information like the
connector label (ie, writing on the case) and colour as part of the
object for the connector and DMI does similar things for PCI slots.

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