[1/2] gpio: pl061: get gpio base from alias id

Message ID 1398130543-19454-1-git-send-email-haojian.zhuang@linaro.org
State New
Headers show

Commit Message

Haojian Zhuang April 22, 2014, 1:35 a.m.
If gpio base number isn't specified, the gpio base will be find from
the end of gpio number. In order to keep with schematics, use alias
to get the ID of gpio chip.

Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
---
 .../devicetree/bindings/gpio/gpio-pl061.txt        | 31 ++++++++++++++++++++++
 drivers/gpio/gpio-pl061.c                          | 14 +++++++++-
 2 files changed, 44 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-pl061.txt

Comments

Baruch Siach April 22, 2014, 3:44 a.m. | #1
Hi Haojian Zhuang,

On Tue, Apr 22, 2014 at 09:35:42AM +0800, Haojian Zhuang wrote:
> If gpio base number isn't specified, the gpio base will be find from
> the end of gpio number. In order to keep with schematics, use alias
> to get the ID of gpio chip.
> 
> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
> ---
>  .../devicetree/bindings/gpio/gpio-pl061.txt        | 31 ++++++++++++++++++++++

Binding documentation should be done in a separate patch.

>  drivers/gpio/gpio-pl061.c                          | 14 +++++++++-
>  2 files changed, 44 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-pl061.txt
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-pl061.txt b/Documentation/devicetree/bindings/gpio/gpio-pl061.txt
> new file mode 100644
> index 0000000..164b5ba
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-pl061.txt
> @@ -0,0 +1,31 @@
> +PL061 GPIO controller bindings
> +
> +Required properties:
> +- compatible:
> +  - "arm,pl061", "arm,primecell".
> +- #gpio-cells : Should be two.
> +  - first cell is the gpio pin number
> +  - second cell is used to specify the gpio polarity:
> +      0 = active high
> +      1 = active low
> +- gpio-controller : Marks the device node as a GPIO controller.
> +- interrupt-controller : Marks the device node as an interrupt controller.
> +- #interrupt-cells : Should be two.
> +  - first cell is the hw irq number
> +  - second cell is used to specify the interrupt type:
> +      0 = default, unspecified type
> +      1 = rising edge triggered
> +      2 = falling edge triggered
> +      4 = high level triggered
> +      8 = low level triggered
> +
> +Example:
> +	gpio0: gpio@fc806000 {
> +		compatible = "arm,pl061", "arm,primecell";
> +		reg = <0xfc806000 0x1000>;
> +		interrupts = <0 64 0x4>;

The "interrupts" property should be mentioned above.

> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		interrupt-controller;
> +		#interrupt-cells = <2>;
> +	};

baruch
Linus Walleij April 23, 2014, 1:21 p.m. | #2
On Tue, Apr 22, 2014 at 3:35 AM, Haojian Zhuang
<haojian.zhuang@linaro.org> wrote:

> If gpio base number isn't specified, the gpio base will be find from
> the end of gpio number. In order to keep with schematics, use alias
> to get the ID of gpio chip.
>
> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>

The idea with GPIO numbers is not that these shall correspond
to schematics or data sheets or anything like that. It's a completely
Linux-internal number and has nothing to do with anything else.

The same is true for IRQ numbers.

The long term goal is to get rid of both GPIO and IRQ numbers
and deal only with descriptors in the kernel.

> +++ b/Documentation/devicetree/bindings/gpio/gpio-pl061.txt

A patch to add the device tree bindings is *very* welcome.

>         } else {
> -               chip->gc.base = -1;
> +               chip->gc.base = pl061_parse_gpio_base(dev);
>                 irq_base = 0;
>         }

I do not like a patch that changes the dynamic numbering to a
static one, that make things worse for our goals, not better :-(

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Rob Herring April 23, 2014, 6:52 p.m. | #3
On Mon, Apr 21, 2014 at 8:35 PM, Haojian Zhuang
<haojian.zhuang@linaro.org> wrote:
> If gpio base number isn't specified, the gpio base will be find from
> the end of gpio number. In order to keep with schematics, use alias
> to get the ID of gpio chip.

NAK. This is an abuse aliases.

> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
> ---
>  .../devicetree/bindings/gpio/gpio-pl061.txt        | 31 ++++++++++++++++++++++

Is something wrong with the binding doc already in pl061-gpio.txt?

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Haojian Zhuang April 25, 2014, 12:27 a.m. | #4
On 23 April 2014 21:21, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Apr 22, 2014 at 3:35 AM, Haojian Zhuang
> <haojian.zhuang@linaro.org> wrote:
>
>> If gpio base number isn't specified, the gpio base will be find from
>> the end of gpio number. In order to keep with schematics, use alias
>> to get the ID of gpio chip.
>>
>> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
>
> The idea with GPIO numbers is not that these shall correspond
> to schematics or data sheets or anything like that. It's a completely
> Linux-internal number and has nothing to do with anything else.
>
> The same is true for IRQ numbers.
>
> The long term goal is to get rid of both GPIO and IRQ numbers
> and deal only with descriptors in the kernel.

Go it. But the gpio sysfs interface is using the gpio name with an
internal number. It'll make developer confusion since it's different
from datasheet
or schematics. Is there any plan to remove the confusion?

Regards
Haojian
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Haojian Zhuang April 25, 2014, 12:28 a.m. | #5
On 24 April 2014 02:52, Rob Herring <robherring2@gmail.com> wrote:
> On Mon, Apr 21, 2014 at 8:35 PM, Haojian Zhuang
> <haojian.zhuang@linaro.org> wrote:
>> If gpio base number isn't specified, the gpio base will be find from
>> the end of gpio number. In order to keep with schematics, use alias
>> to get the ID of gpio chip.
>
> NAK. This is an abuse aliases.
>
>> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
>> ---
>>  .../devicetree/bindings/gpio/gpio-pl061.txt        | 31 ++++++++++++++++++++++
>
> Is something wrong with the binding doc already in pl061-gpio.txt?
>
Oh. I just copy my old patch to here. I didn't notice there's a new
pl061-gpio.txt. Thanks for reminder.

Regards
Haojian
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij April 25, 2014, 9:29 a.m. | #6
On Fri, Apr 25, 2014 at 2:27 AM, Haojian Zhuang
<haojian.zhuang@linaro.org> wrote:

> Go it. But the gpio sysfs interface is using the gpio name with an
> internal number. It'll make developer confusion since it's different
> from datasheet
> or schematics. Is there any plan to remove the confusion?

First I am as always very suspicious about the sysfs interface
since it is often used for madness like reimplementing
leds-gpio.c, gpio_keys.c, extcon-gpio.c, gpio-regulator.c
spi-gpio.c, w1-gpio.c or gpio_wdt.c in userspace for no
good reason when the kernel should handle it.

Yet there are exceptions where it's actually warranted.

There is a possibility to set a label on the gpio line,
so this should be used.

C.f.
commit 781f6d710d4482eab05cfaad50060a0ea8c0e4e0
"gpio: generic: Add label to platform data"

Something like this is needed in this and other drivers
needing to label its GPIO lines. It's much like how we name
all pins in a pin controller really.

I don't exactly know how we should achieve the same
for labels to offsets in any given gpio_chip using
some generic device tree labels for example, but I
imagine it could be implemented in a generic fashion?

Describing the names of gpio lines should be quite
obvious hardware description suitable for the device
tree.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/Documentation/devicetree/bindings/gpio/gpio-pl061.txt b/Documentation/devicetree/bindings/gpio/gpio-pl061.txt
new file mode 100644
index 0000000..164b5ba
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-pl061.txt
@@ -0,0 +1,31 @@ 
+PL061 GPIO controller bindings
+
+Required properties:
+- compatible:
+  - "arm,pl061", "arm,primecell".
+- #gpio-cells : Should be two.
+  - first cell is the gpio pin number
+  - second cell is used to specify the gpio polarity:
+      0 = active high
+      1 = active low
+- gpio-controller : Marks the device node as a GPIO controller.
+- interrupt-controller : Marks the device node as an interrupt controller.
+- #interrupt-cells : Should be two.
+  - first cell is the hw irq number
+  - second cell is used to specify the interrupt type:
+      0 = default, unspecified type
+      1 = rising edge triggered
+      2 = falling edge triggered
+      4 = high level triggered
+      8 = low level triggered
+
+Example:
+	gpio0: gpio@fc806000 {
+		compatible = "arm,pl061", "arm,primecell";
+		reg = <0xfc806000 0x1000>;
+		interrupts = <0 64 0x4>;
+		gpio-controller;
+		#gpio-cells = <2>;
+		interrupt-controller;
+		#interrupt-cells = <2>;
+	};
diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
index b0f4752..14f3ab5 100644
--- a/drivers/gpio/gpio-pl061.c
+++ b/drivers/gpio/gpio-pl061.c
@@ -236,6 +236,18 @@  static struct irq_chip pl061_irqchip = {
 	.irq_set_type	= pl061_irq_type,
 };
 
+/* Parse gpio base from DT */
+static int pl061_parse_gpio_base(struct device *dev)
+{
+	struct device_node *np = dev->of_node;
+	int ret, id;
+
+	id = of_alias_get_id(np, "gpio");
+	if (id < 0)
+		return id;
+	return (id * PL061_GPIO_NR);
+}
+
 static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
 {
 	struct device *dev = &adev->dev;
@@ -255,7 +267,7 @@  static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
 			return -ENODEV;
 		}
 	} else {
-		chip->gc.base = -1;
+		chip->gc.base = pl061_parse_gpio_base(dev);
 		irq_base = 0;
 	}