Message-ID: <20131102103323.17e208cf@notabene.brown>
Date:	Sat, 2 Nov 2013 10:33:23 +1100
From:	NeilBrown <neilb@...e.de>
To:	Mark Rutland <mark.rutland@....com>
Cc:	MyungJoo Ham <myungjoo.ham@...sung.com>,
	Chanwoo Choi <cw00.choi@...sung.com>,
	"grant.likely@...aro.org" <grant.likely@...aro.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Belisko Marek <marek.belisko@...il.com>,
	"Dr. H. Nikolaus Schaller" <hns@...delico.com>
Subject: Re: [PATCH] extcon-gpio: add devicetree support.

On Fri, 1 Nov 2013 10:16:44 -0700 Mark Rutland <mark.rutland@....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@...e.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

Download attachment "signature.asc" of type "application/pgp-signature" (829 bytes)

Powered by blists - more mailing lists