Messages in this thread | | | Date | Sat, 02 Nov 2013 23:38:49 -0700 | From | Guenter Roeck <> | Subject | Re: [PATCH] extcon-gpio: add devicetree support. |
| |
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
| |