On Tue, Jul 30, 2013 at 06:59:01PM +0100, Stephen Warren wrote:

On 07/30/2013 10:22 AM, Olof Johansson wrote:

quoted

On Tue, Jul 30, 2013 at 03:18:35PM +0400, Alexander Shiyan wrote:

quoted

This patch adds DT support for generic (MMIO) GPIO driver.

quoted

quoted

diff --git a/Documentation/devicetree/bindings/gpio/gpio-generic.txt b/Documentation/devicetree/bindings/gpio/gpio-generic.txt

quoted

quoted

+Generic memory-mapped GPIO controller
+
+Required properties:
+- compatible: Should be "basic-mmio-gpio" or "basic-mmio-gpio-be".

I think naming this "simple-gpio" would match other simple/basic/generic
bindings better.

quoted

quoted

+- reg: Physical base GPIO controller registers location and length.
+- reg-names: Should be the names of reg resources. Each register uses
+  its own reg name, so there should be as many reg names as referenced
+  registers:
+  "dat"		: Input/output register (Required),
+  "set"		: Register for set output bits (Optional),
+  "clr"		: Register for clear output bits (Optional),
+  "dirout"	: Register for setup direction as output (Optional),
+  "dirin"	: Register for setup direction as input (Optional).
+- gpio-controller: Marks the device node as a gpio controller.
+- #gpio-cells: Should be two.

...

quoted

I'm trying to figure out what to say about this binding. It's not really
describing hardware, instead it's strongly tied to how the basic-mmio-gpio
driver expects the platform data to look.

I'm not sure; the binding seems to only contain a direct representation
of pure HW properties; the addresses/offsets of various registers in the
HW block.

quoted

It makes more sense to actually describe the hardware in question,
and then have the driver handle that as expected. I.e. either have a
small conversion layer that binds to the actual hardware compatible
value and registers a basic-mmio-gpio device from there, or extend the
basic-mmio-gpio driver to do it by itself.

Ah, I guess the question more: Do we want generic bindings that describe
the low-level details of the HW in a completely generic fashion so that
new HW can be supported with zero kernel changes, or do we want a simple
driver with a lookup table that maps from compatible value to the HW
configuration? One of the potential benefits of DT is to be able to
support new HW without code changes, although perhaps that's more
typically considered in the context of new boards rather than new IP
blocks or SoCs.

I think that going forward it would be better to have have a compatible
string per different device. As Olof pointed out, we're leaking the way
we currently handle things in Linux into the binding, rather than
precisely describing the hardware (with a unique compatible string). I'm
not sure if this is much better than embedding a bytecode describing how
to poke devices.

Certainly there should be more-specific bindings for each device, so we
can later improve support for them. If we have that anyway, I think it
would be nicer to have the mapping from that compatible string to some
internal data passed to the simple-gpio driver, rather than explicitly
stating that the current version of the Linux simple-gpio driver is
capable of driving the device.

I think the issue is that we're describing a hardware block in general,
rather than the instance of the hardware block, and that limits how
flexibly we can use the data in the description.

If we reject this driver, we surely have to get rid of pinctrl-single,
and perhaps some others?

That's certainly something we need to consider. However, those bindings
are in active use, and this is not yet. I don't think that we should
necessarily follow that style of binding.

Thanks,
Mark.