[PATCH 0/2] gpio: of: Support cascaded GPIO

[PATCH 0/2] gpio: of: Support cascaded GPIO

[Pantelis Antoniou]

These two patches enable GPIO cascades where a match may be
retried until the finalr real GPIO is located.

The first patch removes the const specifier from gpiospec at of_xlate(),
while the second introduces the new method of_gpiochip_find() that
performs the cascade operation.

Pantelis Antoniou (2):
  gpio: Remove const from gpiospec in of_xlate
  gpio: Support cascaded GPIO chip lookup for OF

 drivers/gpio/gpio-brcmstb.c |  2 +-
 drivers/gpio/gpio-davinci.c |  2 +-
 drivers/gpio/gpio-etraxfs.c |  2 +-
 drivers/gpio/gpio-lpc32xx.c |  2 +-
 drivers/gpio/gpio-pxa.c     |  2 +-
 drivers/gpio/gpiolib-of.c   | 18 +++++----------
 drivers/gpio/gpiolib.c      | 54 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/gpio/gpiolib.h      | 14 ++++++++++++
 include/linux/gpio/driver.h |  2 +-
 include/linux/of_gpio.h     |  2 +-
 10 files changed, 80 insertions(+), 20 deletions(-)

-- 
1.7.12

[PATCH 1/2] gpio: Remove const from gpiospec in of_xlate

[Pantelis Antoniou]

Cascaded GPIO chips modify gpiospec to pass along
the next gpio chip to match against. Remove the const
specifier to make it possible.
Change all in kernel users also.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
---
 drivers/gpio/gpio-brcmstb.c | 2 +-
 drivers/gpio/gpio-davinci.c | 2 +-
 drivers/gpio/gpio-etraxfs.c | 2 +-
 drivers/gpio/gpio-lpc32xx.c | 2 +-
 drivers/gpio/gpio-pxa.c     | 2 +-
 drivers/gpio/gpiolib-of.c   | 2 +-
 include/linux/gpio/driver.h | 2 +-
 include/linux/of_gpio.h     | 2 +-
 8 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
index e648914..29a71d6 100644
--- a/drivers/gpio/gpio-brcmstb.c
+++ b/drivers/gpio/gpio-brcmstb.c
@@ -302,7 +302,7 @@ static int brcmstb_gpio_remove(struct platform_device *pdev)
 }
 
 static int brcmstb_gpio_of_xlate(struct gpio_chip *gc,
-		const struct of_phandle_args *gpiospec, u32 *flags)
+		struct of_phandle_args *gpiospec, u32 *flags)
 {
 	struct brcmstb_gpio_priv *priv = brcmstb_gpio_gc_to_priv(gc);
 	struct brcmstb_gpio_bank *bank = gpiochip_get_data(gc);
diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
index dd262f0..87add80 100644
--- a/drivers/gpio/gpio-davinci.c
+++ b/drivers/gpio/gpio-davinci.c
@@ -173,7 +173,7 @@ of_err:
 
 #ifdef CONFIG_OF_GPIO
 static int davinci_gpio_of_xlate(struct gpio_chip *gc,
-			     const struct of_phandle_args *gpiospec,
+			     struct of_phandle_args *gpiospec,
 			     u32 *flags)
 {
 	struct davinci_gpio_controller *chips = dev_get_drvdata(gc->parent);
diff --git a/drivers/gpio/gpio-etraxfs.c b/drivers/gpio/gpio-etraxfs.c
index 00b022c..660bffb 100644
--- a/drivers/gpio/gpio-etraxfs.c
+++ b/drivers/gpio/gpio-etraxfs.c
@@ -180,7 +180,7 @@ static unsigned int etraxfs_gpio_chip_to_port(struct gpio_chip *gc)
 }
 
 static int etraxfs_gpio_of_xlate(struct gpio_chip *gc,
-			       const struct of_phandle_args *gpiospec,
+			       struct of_phandle_args *gpiospec,
 			       u32 *flags)
 {
 	/*
diff --git a/drivers/gpio/gpio-lpc32xx.c b/drivers/gpio/gpio-lpc32xx.c
index fc5f197..3be9169 100644
--- a/drivers/gpio/gpio-lpc32xx.c
+++ b/drivers/gpio/gpio-lpc32xx.c
@@ -478,7 +478,7 @@ static struct lpc32xx_gpio_chip lpc32xx_gpiochip[] = {
 };
 
 static int lpc32xx_of_xlate(struct gpio_chip *gc,
-			    const struct of_phandle_args *gpiospec, u32 *flags)
+			    struct of_phandle_args *gpiospec, u32 *flags)
 {
 	/* Is this the correct bank? */
 	u32 bank = gpiospec->args[0];
diff --git a/drivers/gpio/gpio-pxa.c b/drivers/gpio/gpio-pxa.c
index 76ac906..47e35ab 100644
--- a/drivers/gpio/gpio-pxa.c
+++ b/drivers/gpio/gpio-pxa.c
@@ -317,7 +317,7 @@ static void pxa_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
 
 #ifdef CONFIG_OF_GPIO
 static int pxa_gpio_of_xlate(struct gpio_chip *gc,
-			     const struct of_phandle_args *gpiospec,
+			     struct of_phandle_args *gpiospec,
 			     u32 *flags)
 {
 	if (gpiospec->args[0] > pxa_last_gpio)
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index d22dcc3..50cb787 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -285,7 +285,7 @@ static int of_gpiochip_scan_gpios(struct gpio_chip *chip)
  * is less than ngpios (that is specified in the gpio_chip).
  */
 int of_gpio_simple_xlate(struct gpio_chip *gc,
-			 const struct of_phandle_args *gpiospec, u32 *flags)
+			 struct of_phandle_args *gpiospec, u32 *flags)
 {
 	/*
 	 * We're discouraging gpio_cells < 2, since that way you'll have to
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 50882e0..793297c 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -201,7 +201,7 @@ struct gpio_chip {
 	struct device_node *of_node;
 	int of_gpio_n_cells;
 	int (*of_xlate)(struct gpio_chip *gc,
-			const struct of_phandle_args *gpiospec, u32 *flags);
+			struct of_phandle_args *gpiospec, u32 *flags);
 #endif
 };
 
diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
index 092186c..bdeb4b1 100644
--- a/include/linux/of_gpio.h
+++ b/include/linux/of_gpio.h
@@ -64,7 +64,7 @@ extern void of_mm_gpiochip_remove(struct of_mm_gpio_chip *mm_gc);
 extern int of_gpiochip_add(struct gpio_chip *gc);
 extern void of_gpiochip_remove(struct gpio_chip *gc);
 extern int of_gpio_simple_xlate(struct gpio_chip *gc,
-				const struct of_phandle_args *gpiospec,
+				struct of_phandle_args *gpiospec,
 				u32 *flags);
 
 #else /* CONFIG_OF_GPIO */
-- 
1.7.12

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] gpio: Support cascaded GPIO chip lookup for OF

[Pantelis Antoniou]

In certain cases it makes sense to create cascaded GPIO which
are not real GPIOs, merely point to the real backend GPIO chip.

In order to support this, cascaded of_xlate lookup need to be
performed.

For example let's take a connector that we want to abstract
having two GPIO pins from different GPIO controllers, connector
pin #0 connected to gpioA controller with offset 10 and gpioB
with 4.

In pseudo DT form this is analogous to:

	gpioA: gpioa@80000 {
		compatible = "foocorp,gpio";
		...
	};

	gpioB: gpiob@80800 {
		compatible = "foocorp,gpio";
		....
	};

	gpioC: controller_gpio {
		compatible = "cascaded-gpio";
		gpios = <&gpioA 10>, <&gpioB 5>;
	};

For example a user of gpioC (let's take a led driver) will have a
reference to gpioC like this:

	leds {
		compatible = "gpio-leds";
		led@0 {
			gpios = <&gpioC 0 GPIO_ACTIVE_HIGH>;
			...
		};
		led@1 {
			gpios = <&gpioC 1 GPIO_ACTIVE_HIGH>;
			..
		};
	};

We want the matches for gpioC to instead refer to gpioA & gpioB.

This is accomplished by a new method of_gpiochip_find() which
is an extension of the standard gpiochip_find() method.

A cascaded GPIO controller can modify the gpiospec node pointer
and arg[0] to point to the next GPIO in sequence and return -EAGAIN.
of_gpiochip_find() will restart the match using the new data
until either the final real GPIO is found or a maximum iteration
limit is reached.

In our example the cascaded-gpio driver can have a of_xlate method that
will point to gpioA/10 for gpioC/0 and gpioB/5 for gpioC/1.

Note that the cascade-gpio driver needs not to define any other member
methods since no actual reference will ever be made to it.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 drivers/gpio/gpiolib-of.c | 16 ++++----------
 drivers/gpio/gpiolib.c    | 54 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/gpio/gpiolib.h    | 14 ++++++++++++
 3 files changed, 72 insertions(+), 12 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 50cb787..771060f 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -26,18 +26,10 @@
 
 #include "gpiolib.h"
 
-/* Private data structure for of_gpiochip_find_and_xlate */
-struct gg_data {
-	enum of_gpio_flags *flags;
-	struct of_phandle_args gpiospec;
-
-	struct gpio_desc *out_gpio;
-};
-
 /* Private function for resolving node pointer to gpio_chip */
-static int of_gpiochip_find_and_xlate(struct gpio_chip *gc, void *data)
+static int of_gpiochip_find_and_xlate(struct gpio_chip *gc,
+		struct gg_data *gg_data)
 {
-	struct gg_data *gg_data = data;
 	int ret;
 
 	if ((gc->of_node != gg_data->gpiospec.np) ||
@@ -95,7 +87,7 @@ struct gpio_desc *of_get_named_gpiod_flags(struct device_node *np,
 		return ERR_PTR(ret);
 	}
 
-	gpiochip_find(&gg_data, of_gpiochip_find_and_xlate);
+	of_gpiochip_find(&gg_data, of_gpiochip_find_and_xlate);
 
 	of_node_put(gg_data.gpiospec.np);
 	pr_debug("%s: parsed '%s' property of node '%s[%d]' - status (%d)\n",
@@ -166,7 +158,7 @@ static struct gpio_desc *of_parse_own_gpio(struct device_node *np,
 			return ERR_PTR(ret);
 	}
 
-	gpiochip_find(&gg_data, of_gpiochip_find_and_xlate);
+	of_gpiochip_find(&gg_data, of_gpiochip_find_and_xlate);
 	if (!gg_data.out_gpio) {
 		if (np->parent == np)
 			return ERR_PTR(-ENXIO);
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 24f60d2..e719499 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -884,6 +884,60 @@ struct gpio_chip *gpiochip_find(void *data,
 }
 EXPORT_SYMBOL_GPL(gpiochip_find);
 
+#ifdef CONFIG_OF_GPIO
+
+/* allow a maximum of 10 GPIO cascades (should be enough) */
+#define OF_GPIOCHIP_RETRY_COUNT_MAX	10
+
+/**
+ * of_gpiochip_find() - iterator for locating a specific gpio_chip
+ * @gg_data: data to pass to match function
+ * @callback: Callback function to check gpio_chip
+ *
+ * Similar to bus_find_device.  It returns a reference to a gpio_chip as
+ * determined by a user supplied @match callback.  The callback should return
+ * 0 if the device doesn't match and non-zero if it does.  If the callback is
+ * non-zero, this function will return to the caller and not iterate over any
+ * more gpio_chips.
+ */
+struct gpio_chip *of_gpiochip_find(struct gg_data *gg_data,
+		int (*match)(struct gpio_chip *chip, struct gg_data *gg_data))
+{
+	struct gpio_device *gdev;
+	struct gpio_chip *chip;
+	unsigned long flags;
+	int i;
+
+	spin_lock_irqsave(&gpio_lock, flags);
+	/* always start with defer */
+	gg_data->out_gpio = ERR_PTR(-EPROBE_DEFER);
+	for (i = 0; gg_data->out_gpio == ERR_PTR(-EPROBE_DEFER) &&
+			i < OF_GPIOCHIP_RETRY_COUNT_MAX; i++) {
+
+		list_for_each_entry(gdev, &gpio_devices, list) {
+			if (match(gdev->chip, gg_data))
+				break;
+			/* again? cascaded; try agan */
+			if (gg_data->out_gpio == ERR_PTR(-EAGAIN)) {
+				/* defer is the default again */
+				gg_data->out_gpio = ERR_PTR(-EPROBE_DEFER);
+				break;
+			}
+		}
+	}
+
+	/* no match or maximum retry limit? */
+	if (&gdev->list == &gpio_devices || i >= OF_GPIOCHIP_RETRY_COUNT_MAX)
+		chip = NULL;
+	else
+		chip = gdev->chip;
+	spin_unlock_irqrestore(&gpio_lock, flags);
+
+	return chip;
+}
+EXPORT_SYMBOL_GPL(of_gpiochip_find);
+#endif
+
 static int gpiochip_match_name(struct gpio_chip *chip, void *data)
 {
 	const char *name = data;
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 2d9ea5e..58cbf75 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -236,4 +236,18 @@ static inline void gpiochip_sysfs_unregister(struct gpio_device *gdev)
 
 #endif /* CONFIG_GPIO_SYSFS */
 
+#ifdef CONFIG_OF_GPIO
+
+/* Private data structure for of_gpiochip_find_and_xlate */
+struct gg_data {
+	enum of_gpio_flags *flags;
+	struct of_phandle_args gpiospec;
+	struct gpio_desc *out_gpio;
+};
+
+extern struct gpio_chip *of_gpiochip_find(struct gg_data *gg_data,
+		int (*match)(struct gpio_chip *chip, struct gg_data *gg_data));
+
+#endif
+
 #endif /* GPIOLIB_H */
-- 
1.7.12

Re: [PATCH 2/2] gpio: Support cascaded GPIO chip lookup for OF

[Rob Herring]

+Mark R

On Fri, Jun 3, 2016 at 3:26 PM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> In certain cases it makes sense to create cascaded GPIO which
> are not real GPIOs, merely point to the real backend GPIO chip.

In what cases? Make it clear what this is for. Connectors of course,
but are there any other use cases you have in mind.

> In order to support this, cascaded of_xlate lookup need to be
> performed.
>
> For example let's take a connector that we want to abstract
> having two GPIO pins from different GPIO controllers, connector
> pin #0 connected to gpioA controller with offset 10 and gpioB
> with 4.

A connector's GPIO number may or may not be related to connector pins.

> In pseudo DT form this is analogous to:
>
>         gpioA: gpioa@80000 {
>                 compatible = "foocorp,gpio";
>                 ...
>         };
>
>         gpioB: gpiob@80800 {
>                 compatible = "foocorp,gpio";
>                 ....
>         };
>
>         gpioC: controller_gpio {
>                 compatible = "cascaded-gpio";

This compatible is kind of meaningless. I'd expect this to be a
connector compatible.

>                 gpios = <&gpioA 10>, <&gpioB 5>;

As we discussed at ELC, I think this should be modeled after
interrupt-map property like this:

gpio-map = <0 0 &soc_gpio 10 0>, <1 0 &soc_gpio 5 0>;
gpio-map-mask = <0xff 0>;

This is more flexible, a known pattern, and allows remapping of flag cells.

Also, we will likely have interrupt capable GPIOs, so they are going
to need interrupt-map anyway.

Rob

Re: [PATCH 2/2] gpio: Support cascaded GPIO chip lookup for OF

[Pantelis Antoniou]

Hi Rob,

> On Jun 8, 2016, at 00:00 , Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> +Mark R
> 
> On Fri, Jun 3, 2016 at 3:26 PM, Pantelis Antoniou
> <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
>> In certain cases it makes sense to create cascaded GPIO which
>> are not real GPIOs, merely point to the real backend GPIO chip.
> 
> In what cases? Make it clear what this is for. Connectors of course,
> but are there any other use cases you have in mind.
> 

Connectors is one obvious use-case. In fact even when there is no
connector but there is an obvious interface abstraction point you
might want to use it.

For instance a SoC package may have a number of different GPIO
controllers (that may or may not use the same IP block). You could
abstract all the gpio controllers away in a single GPIO controller
block.

>> In order to support this, cascaded of_xlate lookup need to be
>> performed.
>> 
>> For example let's take a connector that we want to abstract
>> having two GPIO pins from different GPIO controllers, connector
>> pin #0 connected to gpioA controller with offset 10 and gpioB
>> with 4.
> 
> A connector's GPIO number may or may not be related to connector pins.
> 

Obviously, this is just an example.

>> In pseudo DT form this is analogous to:
>> 
>>        gpioA: gpioa@80000 {
>>                compatible = "foocorp,gpio";
>>                ...
>>        };
>> 
>>        gpioB: gpiob@80800 {
>>                compatible = "foocorp,gpio";
>>                ....
>>        };
>> 
>>        gpioC: controller_gpio {
>>                compatible = "cascaded-gpio";
> 
> This compatible is kind of meaningless. I'd expect this to be a
> connector compatible.
> 

No, because this gpio patch is completely independent of the
existence of a connector.
 
>>                gpios = <&gpioA 10>, <&gpioB 5>;
> 
> As we discussed at ELC, I think this should be modeled after
> interrupt-map property like this:
> 
> gpio-map = <0 0 &soc_gpio 10 0>, <1 0 &soc_gpio 5 0>;
> gpio-map-mask = <0xff 0>;
> 
> This is more flexible, a known pattern, and allows remapping of flag cells.
> 

It’s just syntactic sugar. It can work either way.

> Also, we will likely have interrupt capable GPIOs, so they are going
> to need interrupt-map anyway.
> 

Devices that use interrupts usually convert the GPIO to an interrupt and use it.
Since the xlat op will return the real GPIO spec the interrupt conversion will work.

Bare interrupt lines are sort-of out of fashion nowadays I think. I’m eager to be
proven wrong though with a recent portable expansion board that uses them.

> Rob

Regards

— Pantelis

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] gpio: Support cascaded GPIO chip lookup for OF

[Rob Herring]

On Wed, Jun 8, 2016 at 8:41 AM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> Hi Rob,
>
>> On Jun 8, 2016, at 00:00 , Rob Herring <robherring2@gmail.com> wrote:
>>
>> +Mark R
>>
>> On Fri, Jun 3, 2016 at 3:26 PM, Pantelis Antoniou
>> <pantelis.antoniou@konsulko.com> wrote:
>>> In certain cases it makes sense to create cascaded GPIO which
>>> are not real GPIOs, merely point to the real backend GPIO chip.
>>
>> In what cases? Make it clear what this is for. Connectors of course,
>> but are there any other use cases you have in mind.
>>
>
> Connectors is one obvious use-case. In fact even when there is no
> connector but there is an obvious interface abstraction point you
> might want to use it.
>
> For instance a SoC package may have a number of different GPIO
> controllers (that may or may not use the same IP block). You could
> abstract all the gpio controllers away in a single GPIO controller
> block.

There had better be some good reason besides just wanting to make a
single virtual controller.

>>> In order to support this, cascaded of_xlate lookup need to be
>>> performed.
>>>
>>> For example let's take a connector that we want to abstract
>>> having two GPIO pins from different GPIO controllers, connector
>>> pin #0 connected to gpioA controller with offset 10 and gpioB
>>> with 4.
>>
>> A connector's GPIO number may or may not be related to connector pins.
>>
>
> Obviously, this is just an example.
>
>>> In pseudo DT form this is analogous to:
>>>
>>>        gpioA: gpioa@80000 {
>>>                compatible = "foocorp,gpio";
>>>                ...
>>>        };
>>>
>>>        gpioB: gpiob@80800 {
>>>                compatible = "foocorp,gpio";
>>>                ....
>>>        };
>>>
>>>        gpioC: controller_gpio {
>>>                compatible = "cascaded-gpio";
>>
>> This compatible is kind of meaningless. I'd expect this to be a
>> connector compatible.
>>
>
> No, because this gpio patch is completely independent of the
> existence of a connector.

My point is that "cascaded-gpio" is not something used to parse the
binding and will never be an accepted compatible string. I know it is
an example, but you should make that obvious like foocorp is.

>>>                gpios = <&gpioA 10>, <&gpioB 5>;
>>
>> As we discussed at ELC, I think this should be modeled after
>> interrupt-map property like this:
>>
>> gpio-map = <0 0 &soc_gpio 10 0>, <1 0 &soc_gpio 5 0>;
>> gpio-map-mask = <0xff 0>;
>>
>> This is more flexible, a known pattern, and allows remapping of flag cells.
>>
>
> It’s just syntactic sugar. It can work either way.
>
>> Also, we will likely have interrupt capable GPIOs, so they are going
>> to need interrupt-map anyway.
>>
>
> Devices that use interrupts usually convert the GPIO to an interrupt and use it.
> Since the xlat op will return the real GPIO spec the interrupt conversion will work.
>
> Bare interrupt lines are sort-of out of fashion nowadays I think. I’m eager to be
> proven wrong though with a recent portable expansion board that uses them.

Uh, no! Practically every gpio controller is also an interrupt
controller (in DT) and devices pretty much always see just an
interrupt line. Just go look at all the I2C devices with an interrupt
line. Unless devices have some special needs to control the gpio, we
always use interrupts. Expansion boards may be dealing with the GPIO
simply because that is the only option for userspace drivers.

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

Re: [PATCH 2/2] gpio: Support cascaded GPIO chip lookup for OF

[Pantelis Antoniou]

Hi Rob,

> On Jun 8, 2016, at 18:18 , Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> On Wed, Jun 8, 2016 at 8:41 AM, Pantelis Antoniou
> <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
>> Hi Rob,
>> 
>>> On Jun 8, 2016, at 00:00 , Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> 
>>> +Mark R
>>> 
>>> On Fri, Jun 3, 2016 at 3:26 PM, Pantelis Antoniou
>>> <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
>>>> In certain cases it makes sense to create cascaded GPIO which
>>>> are not real GPIOs, merely point to the real backend GPIO chip.
>>> 
>>> In what cases? Make it clear what this is for. Connectors of course,
>>> but are there any other use cases you have in mind.
>>> 
>> 
>> Connectors is one obvious use-case. In fact even when there is no
>> connector but there is an obvious interface abstraction point you
>> might want to use it.
>> 
>> For instance a SoC package may have a number of different GPIO
>> controllers (that may or may not use the same IP block). You could
>> abstract all the gpio controllers away in a single GPIO controller
>> block.
> 
> There had better be some good reason besides just wanting to make a
> single virtual controller.
> 

The reason is to forward a gpio reference to a real h/w gpio device
while not having to expose said real gpio device details. 

>>>> In order to support this, cascaded of_xlate lookup need to be
>>>> performed.
>>>> 
>>>> For example let's take a connector that we want to abstract
>>>> having two GPIO pins from different GPIO controllers, connector
>>>> pin #0 connected to gpioA controller with offset 10 and gpioB
>>>> with 4.
>>> 
>>> A connector's GPIO number may or may not be related to connector pins.
>>> 
>> 
>> Obviously, this is just an example.
>> 
>>>> In pseudo DT form this is analogous to:
>>>> 
>>>>       gpioA: gpioa@80000 {
>>>>               compatible = "foocorp,gpio";
>>>>               ...
>>>>       };
>>>> 
>>>>       gpioB: gpiob@80800 {
>>>>               compatible = "foocorp,gpio";
>>>>               ....
>>>>       };
>>>> 
>>>>       gpioC: controller_gpio {
>>>>               compatible = "cascaded-gpio";
>>> 
>>> This compatible is kind of meaningless. I'd expect this to be a
>>> connector compatible.
>>> 
>> 
>> No, because this gpio patch is completely independent of the
>> existence of a connector.
> 
> My point is that "cascaded-gpio" is not something used to parse the
> binding and will never be an accepted compatible string. I know it is
> an example, but you should make that obvious like foocorp is.
> 

It doesn’t parse the binding; the xlate method of a driver does.  

>>>>               gpios = <&gpioA 10>, <&gpioB 5>;
>>> 
>>> As we discussed at ELC, I think this should be modeled after
>>> interrupt-map property like this:
>>> 
>>> gpio-map = <0 0 &soc_gpio 10 0>, <1 0 &soc_gpio 5 0>;
>>> gpio-map-mask = <0xff 0>;
>>> 
>>> This is more flexible, a known pattern, and allows remapping of flag cells.
>>> 
>> 
>> It’s just syntactic sugar. It can work either way.
>> 
>>> Also, we will likely have interrupt capable GPIOs, so they are going
>>> to need interrupt-map anyway.
>>> 
>> 
>> Devices that use interrupts usually convert the GPIO to an interrupt and use it.
>> Since the xlat op will return the real GPIO spec the interrupt conversion will work.
>> 
>> Bare interrupt lines are sort-of out of fashion nowadays I think. I’m eager to be
>> proven wrong though with a recent portable expansion board that uses them.
> 
> Uh, no! Practically every gpio controller is also an interrupt
> controller (in DT) and devices pretty much always see just an
> interrupt line. Just go look at all the I2C devices with an interrupt
> line. Unless devices have some special needs to control the gpio, we
> always use interrupts. Expansion boards may be dealing with the GPIO
> simply because that is the only option for userspace drivers.
> 

Looking into a list of 54 capes for the beaglebone I can only find a single
one that uses an interrupt instead of a gpio, and that can easily be converted
to using the gpio.

The IRQ forwarding case is easier than the GPIO one TBH. The IRQ parsing core
can handle complex cases so adding a new one won’t be a big deal. 

> Rob

Regards

— Pantelis--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] gpio: Support cascaded GPIO chip lookup for OF

[Rob Herring]

On Wed, Jun 8, 2016 at 10:32 AM, Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote:
> Hi Rob,
>
>> On Jun 8, 2016, at 18:18 , Rob Herring <robherring2@gmail.com> wrote:
>>
>> On Wed, Jun 8, 2016 at 8:41 AM, Pantelis Antoniou
>> <pantelis.antoniou@konsulko.com> wrote:
>>> Hi Rob,
>>>
>>>> On Jun 8, 2016, at 00:00 , Rob Herring <robherring2@gmail.com> wrote:
>>>>
>>>> +Mark R
>>>>
>>>> On Fri, Jun 3, 2016 at 3:26 PM, Pantelis Antoniou
>>>> <pantelis.antoniou@konsulko.com> wrote:
>>>>> In certain cases it makes sense to create cascaded GPIO which
>>>>> are not real GPIOs, merely point to the real backend GPIO chip.
>>>>
>>>> In what cases? Make it clear what this is for. Connectors of course,
>>>> but are there any other use cases you have in mind.
>>>>
>>>
>>> Connectors is one obvious use-case. In fact even when there is no
>>> connector but there is an obvious interface abstraction point you
>>> might want to use it.
>>>
>>> For instance a SoC package may have a number of different GPIO
>>> controllers (that may or may not use the same IP block). You could
>>> abstract all the gpio controllers away in a single GPIO controller
>>> block.
>>
>> There had better be some good reason besides just wanting to make a
>> single virtual controller.
>>
>
> The reason is to forward a gpio reference to a real h/w gpio device
> while not having to expose said real gpio device details.

That's not a reason.

[...]

>>>>>       gpioC: controller_gpio {
>>>>>               compatible = "cascaded-gpio";
>>>>
>>>> This compatible is kind of meaningless. I'd expect this to be a
>>>> connector compatible.
>>>>
>>>
>>> No, because this gpio patch is completely independent of the
>>> existence of a connector.
>>
>> My point is that "cascaded-gpio" is not something used to parse the
>> binding and will never be an accepted compatible string. I know it is
>> an example, but you should make that obvious like foocorp is.
>>
>
> It doesn’t parse the binding; the xlate method of a driver does.

The GPIO core should be doing the xlate by default just like we have default 1 and 2 cell xlate for interrupts.

Another point, with just "gpios" that is already

>
>>>>>               gpios = <&gpioA 10>, <&gpioB 5>;
>>>>
>>>> As we discussed at ELC, I think this should be modeled after
>>>> interrupt-map property like this:
>>>>
>>>> gpio-map = <0 0 &soc_gpio 10 0>, <1 0 &soc_gpio 5 0>;
>>>> gpio-map-mask = <0xff 0>;
>>>>
>>>> This is more flexible, a known pattern, and allows remapping of flag cells.
>>>>
>>>
>>> It’s just syntactic sugar. It can work either way.
>>>
>>>> Also, we will likely have interrupt capable GPIOs, so they are going
>>>> to need interrupt-map anyway.
>>>>
>>>
>>> Devices that use interrupts usually convert the GPIO to an interrupt and use it.
>>> Since the xlat op will return the real GPIO spec the interrupt conversion will work.
>>>
>>> Bare interrupt lines are sort-of out of fashion nowadays I think. I’m eager to be
>>> proven wrong though with a recent portable expansion board that uses them.
>>
>> Uh, no! Practically every gpio controller is also an interrupt
>> controller (in DT) and devices pretty much always see just an
>> interrupt line. Just go look at all the I2C devices with an interrupt
>> line. Unless devices have some special needs to control the gpio, we
>> always use interrupts. Expansion boards may be dealing with the GPIO
>> simply because that is the only option for userspace drivers.
>>
>
> Looking into a list of 54 capes for the beaglebone I can only find a single
> one that uses an interrupt instead of a gpio, and that can easily be converted
> to using the gpio.

What downstream is using is not an argument. For upstream they are going to have to change when they get reviewed...

> The IRQ forwarding case is easier than the GPIO one TBH. The IRQ parsing core
> can handle complex cases so adding a new one won’t be a big deal.

Yes, when the infrastructure has already been done it is easier. But from a binding perspective I dow

Rob

Re: [PATCH 2/2] gpio: Support cascaded GPIO chip lookup for OF

[Linus Walleij]

On Wed, Jun 8, 2016 at 3:41 PM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:

> Devices that use interrupts usually convert the GPIO to an interrupt and use it.
> Since the xlat op will return the real GPIO spec the interrupt conversion will work.
>
> Bare interrupt lines are sort-of out of fashion nowadays I think. I’m eager to be
> proven wrong though with a recent portable expansion board that uses them.

No. We have established since several years back that gpiochip and interrupt
chip use cases are orthogonal, and there is absolutely *NO* requirement that
a consumer call .to_irq() on a GPIO line before using it as an interrupt, if the
driver exposes an interrupt chip.

It is true that external interrupt lines are out-of-fashion and usually replaced
with GPIOs that can trigger IRQs. However the latter have two interfaces
independent of each other.

A practical usecase is the SMSC911x ethernet drivers that have their IRQ
wired to both external interrupt lines and GPIO lines used as IRQ in different
use cases. It should not need to know whether the interrupt provider is a
"real" IRQ line or a GPIO line providing an IRQ. It just wants "some IRQ",
and this should be hidden from the driver and the hardware's DT bindings.

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