Message-ID: <20131101171643.GA2643@kartoffel>
Date:	Fri, 1 Nov 2013 10:16:44 -0700
From:	Mark Rutland <mark.rutland@....com>
To:	NeilBrown <neilb@...e.de>
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.

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@...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"?

> +
> +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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists