linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/7] gpio: Add GPIO Aggregator/Repeater
@ 2019-11-27  8:42 Geert Uytterhoeven
  2019-11-27  8:42 ` [PATCH v3 1/7] gpiolib: Add GPIOCHIP_NAME definition Geert Uytterhoeven
                   ` (7 more replies)
  0 siblings, 8 replies; 48+ messages in thread
From: Geert Uytterhoeven @ 2019-11-27  8:42 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski, Jonathan Corbet, Rob Herring,
	Mark Rutland, Harish Jenny K N, Eugeniu Rosca
  Cc: Alexander Graf, Peter Maydell, Paolo Bonzini, Phil Reid,
	Marc Zyngier, Christoffer Dall, Magnus Damm, linux-gpio,
	linux-doc, devicetree, linux-renesas-soc, linux-kernel,
	qemu-devel, Geert Uytterhoeven

	Hi all,

GPIO controllers are exported to userspace using /dev/gpiochip*
character devices.  Access control to these devices is provided by
standard UNIX file system permissions, on an all-or-nothing basis:
either a GPIO controller is accessible for a user, or it is not.
Currently no mechanism exists to control access to individual GPIOs.

Hence this adds a GPIO driver to aggregate existing GPIOs, and expose
them as a new gpiochip.  This is useful for implementing access control,
and assigning a set of GPIOs to a specific user.  Furthermore, this
simplifies and hardens exporting GPIOs to a virtual machine, as the VM
can just grab the full GPIO controller, and no longer needs to care
about which GPIOs to grab and which not, reducing the attack surface.

Recently, other use cases have been discovered[1]:
  - Describing GPIO inverters in DT, as a generic GPIO Repeater,
  - Describing simple GPIO-operated devices in DT, and using the GPIO
    Aggregator as a generic GPIO driver for userspace.

Changes compared to v2[2] (more details in the individual patches):
  - Integrate GPIO Repeater functionality,
  - Absorb GPIO forwarder library, as the Aggregator and Repeater are
    now a single driver,
  - Use the aggregator parameters to create a GPIO lookup table instead
    of an array of GPIO descriptors,
  - Add documentation,
  - New patches:
      - "gpiolib: Add GPIOCHIP_NAME definition",
      - "gpiolib: Add support for gpiochipN-based table lookup",
      - "gpiolib: Add support for GPIO line table lookup",
      - "dt-bindings: gpio: Add gpio-repeater bindings",
      - "docs: gpio: Add GPIO Aggregator/Repeater documentation",
      - "MAINTAINERS: Add GPIO Aggregator/Repeater section".
  - Dropped patches:
      - "gpio: Export gpiod_{request,free}() to modular GPIO code",
      - "gpio: Export gpiochip_get_desc() to modular GPIO code",
      - "gpio: Export gpio_name_to_desc() to modular GPIO code",
      - "gpio: Add GPIO Forwarder Helper".

Changes compared to v1[3]:
  - Drop "virtual", rename to gpio-aggregator,
  - Create and use new GPIO Forwarder Helper, to allow sharing code with
    the GPIO inverter,
  - Lift limit on the maximum number of GPIOs,
  - Improve parsing of GPIO specifiers,
  - Fix modular build.

Aggregating GPIOs and exposing them as a new gpiochip was suggested in
response to my proof-of-concept for GPIO virtualization with QEMU[4][5].

For the first use case, aggregated GPIO controllers are instantiated and
destroyed by writing to atribute files in sysfs.
Sample session on the Renesas Koelsch development board:

  - Unbind LEDs from leds-gpio driver:

        echo leds > /sys/bus/platform/drivers/leds-gpio/unbind

  - Create aggregators:

    $ echo e6052000.gpio 19,20 \
        > /sys/bus/platform/drivers/gpio-aggregator/new_device

    gpio-aggregator gpio-aggregator.0: gpio 0 => gpio-953 (gpio-aggregator.0)
    gpio-aggregator gpio-aggregator.0: gpio 1 => gpio-954 (gpio-aggregator.0)
    gpiochip_find_base: found new base at 778
    gpio gpiochip8: (gpio-aggregator.0): added GPIO chardev (254:8)
    gpiochip_setup_dev: registered GPIOs 778 to 779 on device: gpiochip8 (gpio-aggregator.0)

    $ echo e6052000.gpio 21 e6050000.gpio 20-22 \
        > /sys/bus/platform/drivers/gpio-aggregator/new_device

    gpio-aggregator gpio-aggregator.1: gpio 0 => gpio-955 (gpio-aggregator.1)
    gpio-aggregator gpio-aggregator.1: gpio 1 => gpio-1012 (gpio-aggregator.1)
    gpio-aggregator gpio-aggregator.1: gpio 2 => gpio-1013 (gpio-aggregator.1)
    gpio-aggregator gpio-aggregator.1: gpio 3 => gpio-1014 (gpio-aggregator.1)
    gpiochip_find_base: found new base at 774
    gpio gpiochip9: (gpio-aggregator.1): added GPIO chardev (254:9)
    gpiochip_setup_dev: registered GPIOs 774 to 777 on device: gpiochip9 (gpio-aggregator.1)

  - Adjust permissions on /dev/gpiochip[89] (optional)

  - Control LEDs:

    $ gpioset gpiochip8 0=0 1=1 # LED6 OFF, LED7 ON
    $ gpioset gpiochip8 0=1 1=0 # LED6 ON, LED7 OFF
    $ gpioset gpiochip9 0=0     # LED8 OFF
    $ gpioset gpiochip9 0=1     # LED8 ON

  - Destroy aggregators:

    $ echo gpio-aggregator.0 \
            > /sys/bus/platform/drivers/gpio-aggregator/delete_device
    $ echo gpio-aggregator.1 \
            > /sys/bus/platform/drivers/gpio-aggregator/delete_device

Thanks for your comments!

References:
  [1] "[PATCH V4 2/2] gpio: inverter: document the inverter bindings"
      (https://lore.kernel.org/linux-gpio/1561699236-18620-3-git-send-email-harish_kandiga@mentor.com/)
  [2] "[PATCH/RFC v2 0/5] gpio: Add GPIO Aggregator Driver"
      (https://lore.kernel.org/linux-gpio/20190911143858.13024-1-geert+renesas@glider.be/)
  [3] "[PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver"
      (https://lore.kernel.org/lkml/20190705160536.12047-1-geert+renesas@glider.be/)
  [4] "[PATCH QEMU POC] Add a GPIO backend"
      (https://lore.kernel.org/linux-renesas-soc/20181003152521.23144-1-geert+renesas@glider.be/)
  [5] "Getting To Blinky: Virt Edition / Making device pass-through
       work on embedded ARM"
      (https://fosdem.org/2019/schedule/event/vai_getting_to_blinky/)

Geert Uytterhoeven (7):
  gpiolib: Add GPIOCHIP_NAME definition
  gpiolib: Add support for gpiochipN-based table lookup
  gpiolib: Add support for GPIO line table lookup
  dt-bindings: gpio: Add gpio-repeater bindings
  gpio: Add GPIO Aggregator/Repeater driver
  docs: gpio: Add GPIO Aggregator/Repeater documentation
  MAINTAINERS: Add GPIO Aggregator/Repeater section

 .../admin-guide/gpio/gpio-aggregator.rst      | 111 ++++
 Documentation/admin-guide/gpio/index.rst      |   1 +
 .../bindings/gpio/gpio-repeater.yaml          |  53 ++
 MAINTAINERS                                   |   8 +
 drivers/gpio/Kconfig                          |  13 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-aggregator.c                | 587 ++++++++++++++++++
 drivers/gpio/gpiolib-sysfs.c                  |   7 +-
 drivers/gpio/gpiolib.c                        |  38 +-
 drivers/gpio/gpiolib.h                        |   2 +
 include/linux/gpio/machine.h                  |   2 +-
 11 files changed, 815 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/admin-guide/gpio/gpio-aggregator.rst
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-repeater.yaml
 create mode 100644 drivers/gpio/gpio-aggregator.c

-- 
2.17.1


^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH v3 1/7] gpiolib: Add GPIOCHIP_NAME definition
  2019-11-27  8:42 [PATCH v3 0/7] gpio: Add GPIO Aggregator/Repeater Geert Uytterhoeven
@ 2019-11-27  8:42 ` Geert Uytterhoeven
  2019-11-28  3:38   ` Ulrich Hecht
                     ` (2 more replies)
  2019-11-27  8:42 ` [PATCH v3 2/7] gpiolib: Add support for gpiochipN-based table lookup Geert Uytterhoeven
                   ` (6 subsequent siblings)
  7 siblings, 3 replies; 48+ messages in thread
From: Geert Uytterhoeven @ 2019-11-27  8:42 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski, Jonathan Corbet, Rob Herring,
	Mark Rutland, Harish Jenny K N, Eugeniu Rosca
  Cc: Alexander Graf, Peter Maydell, Paolo Bonzini, Phil Reid,
	Marc Zyngier, Christoffer Dall, Magnus Damm, linux-gpio,
	linux-doc, devicetree, linux-renesas-soc, linux-kernel,
	qemu-devel, Geert Uytterhoeven

The string literal "gpiochip" is used in several places.
Add a definition for it, and use it everywhere, to make sure everything
stays in sync.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v3:
  - New.
---
 drivers/gpio/gpiolib-sysfs.c | 7 +++----
 drivers/gpio/gpiolib.c       | 4 ++--
 drivers/gpio/gpiolib.h       | 2 ++
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index fbf6b1a0a4fae6ce..23e3d335cd543d53 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -762,10 +762,9 @@ int gpiochip_sysfs_register(struct gpio_device *gdev)
 		parent = &gdev->dev;
 
 	/* use chip->base for the ID; it's already known to be unique */
-	dev = device_create_with_groups(&gpio_class, parent,
-					MKDEV(0, 0),
-					chip, gpiochip_groups,
-					"gpiochip%d", chip->base);
+	dev = device_create_with_groups(&gpio_class, parent, MKDEV(0, 0), chip,
+					gpiochip_groups, GPIOCHIP_NAME "%d",
+					chip->base);
 	if (IS_ERR(dev))
 		return PTR_ERR(dev);
 
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index dce0b31f4125a6b3..c9e47620d2434983 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1419,7 +1419,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *chip, void *data,
 		ret = gdev->id;
 		goto err_free_gdev;
 	}
-	dev_set_name(&gdev->dev, "gpiochip%d", gdev->id);
+	dev_set_name(&gdev->dev, GPIOCHIP_NAME "%d", gdev->id);
 	device_initialize(&gdev->dev);
 	dev_set_drvdata(&gdev->dev, gdev);
 	if (chip->parent && chip->parent->driver)
@@ -5105,7 +5105,7 @@ static int __init gpiolib_dev_init(void)
 		return ret;
 	}
 
-	ret = alloc_chrdev_region(&gpio_devt, 0, GPIO_DEV_MAX, "gpiochip");
+	ret = alloc_chrdev_region(&gpio_devt, 0, GPIO_DEV_MAX, GPIOCHIP_NAME);
 	if (ret < 0) {
 		pr_err("gpiolib: failed to allocate char dev region\n");
 		bus_unregister(&gpio_bus_type);
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index ca9bc1e4803c2979..a4a759920faa48ab 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -16,6 +16,8 @@
 #include <linux/module.h>
 #include <linux/cdev.h>
 
+#define GPIOCHIP_NAME	"gpiochip"
+
 /**
  * struct gpio_device - internal state container for GPIO devices
  * @id: numerical ID number for the GPIO chip
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH v3 2/7] gpiolib: Add support for gpiochipN-based table lookup
  2019-11-27  8:42 [PATCH v3 0/7] gpio: Add GPIO Aggregator/Repeater Geert Uytterhoeven
  2019-11-27  8:42 ` [PATCH v3 1/7] gpiolib: Add GPIOCHIP_NAME definition Geert Uytterhoeven
@ 2019-11-27  8:42 ` Geert Uytterhoeven
  2019-11-28  3:38   ` Ulrich Hecht
  2019-12-12 13:20   ` Linus Walleij
  2019-11-27  8:42 ` [PATCH v3 3/7] gpiolib: Add support for GPIO line " Geert Uytterhoeven
                   ` (5 subsequent siblings)
  7 siblings, 2 replies; 48+ messages in thread
From: Geert Uytterhoeven @ 2019-11-27  8:42 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski, Jonathan Corbet, Rob Herring,
	Mark Rutland, Harish Jenny K N, Eugeniu Rosca
  Cc: Alexander Graf, Peter Maydell, Paolo Bonzini, Phil Reid,
	Marc Zyngier, Christoffer Dall, Magnus Damm, linux-gpio,
	linux-doc, devicetree, linux-renesas-soc, linux-kernel,
	qemu-devel, Geert Uytterhoeven

Currently GPIO controllers can only be referred to by label in GPIO
lookup tables.

Add support for looking them up by "gpiochipN" name, with "N" either the
corresponding GPIO device's ID number, or the GPIO controller's first
GPIO number.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
If this is rejected, the GPIO Aggregator documentation must be updated.

The second variant is currently used by the legacy sysfs interface only,
so perhaps the chip->base check should be dropped?

v3:
  - New.
---
 drivers/gpio/gpiolib.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index c9e47620d2434983..d24a3d79dcfe69ad 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1746,9 +1746,29 @@ static int gpiochip_match_name(struct gpio_chip *chip, void *data)
 	return !strcmp(chip->label, name);
 }
 
+static int gpiochip_match_id(struct gpio_chip *chip, void *data)
+{
+	int id = (uintptr_t)data;
+
+	return id == chip->base || id == chip->gpiodev->id;
+}
+
 static struct gpio_chip *find_chip_by_name(const char *name)
 {
-	return gpiochip_find((void *)name, gpiochip_match_name);
+	struct gpio_chip *chip;
+	int id;
+
+	chip = gpiochip_find((void *)name, gpiochip_match_name);
+	if (chip)
+		return chip;
+
+	if (!str_has_prefix(name, GPIOCHIP_NAME))
+		return NULL;
+
+	if (kstrtoint(name + strlen(GPIOCHIP_NAME), 10, &id))
+		return NULL;
+
+	return gpiochip_find((void *)(uintptr_t)id, gpiochip_match_id);
 }
 
 #ifdef CONFIG_GPIOLIB_IRQCHIP
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH v3 3/7] gpiolib: Add support for GPIO line table lookup
  2019-11-27  8:42 [PATCH v3 0/7] gpio: Add GPIO Aggregator/Repeater Geert Uytterhoeven
  2019-11-27  8:42 ` [PATCH v3 1/7] gpiolib: Add GPIOCHIP_NAME definition Geert Uytterhoeven
  2019-11-27  8:42 ` [PATCH v3 2/7] gpiolib: Add support for gpiochipN-based table lookup Geert Uytterhoeven
@ 2019-11-27  8:42 ` Geert Uytterhoeven
  2019-11-28  3:39   ` Ulrich Hecht
  2019-12-12 13:40   ` Linus Walleij
  2019-11-27  8:42 ` [PATCH v3 4/7] dt-bindings: gpio: Add gpio-repeater bindings Geert Uytterhoeven
                   ` (4 subsequent siblings)
  7 siblings, 2 replies; 48+ messages in thread
From: Geert Uytterhoeven @ 2019-11-27  8:42 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski, Jonathan Corbet, Rob Herring,
	Mark Rutland, Harish Jenny K N, Eugeniu Rosca
  Cc: Alexander Graf, Peter Maydell, Paolo Bonzini, Phil Reid,
	Marc Zyngier, Christoffer Dall, Magnus Damm, linux-gpio,
	linux-doc, devicetree, linux-renesas-soc, linux-kernel,
	qemu-devel, Geert Uytterhoeven

Currently GPIOs can only be referred to by GPIO controller and offset in
GPIO lookup tables.

Add support for looking them up by line name.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
If this is rejected, the GPIO Aggregator documentation and code must be
updated.

v3:
  - New.
---
 drivers/gpio/gpiolib.c       | 12 ++++++++++++
 include/linux/gpio/machine.h |  2 +-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index d24a3d79dcfe69ad..cb608512ad6bbded 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -4475,6 +4475,18 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
 		if (p->con_id && (!con_id || strcmp(p->con_id, con_id)))
 			continue;
 
+		if (p->chip_hwnum == (u16)-1) {
+			desc = gpio_name_to_desc(p->chip_label);
+			if (desc) {
+				*flags = p->flags;
+				return desc;
+			}
+
+			dev_warn(dev, "cannot find GPIO line %s, deferring\n",
+				 p->chip_label);
+			return ERR_PTR(-EPROBE_DEFER);
+		}
+
 		chip = find_chip_by_name(p->chip_label);
 
 		if (!chip) {
diff --git a/include/linux/gpio/machine.h b/include/linux/gpio/machine.h
index 1ebe5be05d5f81fa..84c1c097e55eefaf 100644
--- a/include/linux/gpio/machine.h
+++ b/include/linux/gpio/machine.h
@@ -31,7 +31,7 @@ enum gpio_lookup_flags {
  */
 struct gpiod_lookup {
 	const char *chip_label;
-	u16 chip_hwnum;
+	u16 chip_hwnum;			/* if -1, chip_label is named line */
 	const char *con_id;
 	unsigned int idx;
 	unsigned long flags;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH v3 4/7] dt-bindings: gpio: Add gpio-repeater bindings
  2019-11-27  8:42 [PATCH v3 0/7] gpio: Add GPIO Aggregator/Repeater Geert Uytterhoeven
                   ` (2 preceding siblings ...)
  2019-11-27  8:42 ` [PATCH v3 3/7] gpiolib: Add support for GPIO line " Geert Uytterhoeven
@ 2019-11-27  8:42 ` Geert Uytterhoeven
  2019-11-28  3:39   ` Ulrich Hecht
                     ` (2 more replies)
  2019-11-27  8:42 ` [PATCH v3 5/7] gpio: Add GPIO Aggregator/Repeater driver Geert Uytterhoeven
                   ` (3 subsequent siblings)
  7 siblings, 3 replies; 48+ messages in thread
From: Geert Uytterhoeven @ 2019-11-27  8:42 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski, Jonathan Corbet, Rob Herring,
	Mark Rutland, Harish Jenny K N, Eugeniu Rosca
  Cc: Alexander Graf, Peter Maydell, Paolo Bonzini, Phil Reid,
	Marc Zyngier, Christoffer Dall, Magnus Damm, linux-gpio,
	linux-doc, devicetree, linux-renesas-soc, linux-kernel,
	qemu-devel, Geert Uytterhoeven

Add Device Tree bindings for a GPIO repeater, with optional translation
of physical signal properties.  This is useful for describing explicitly
the presence of e.g. an inverter on a GPIO line, and was inspired by the
non-YAML gpio-inverter bindings by Harish Jenny K N
<harish_kandiga@mentor.com>[1].

Note that this is different from a GPIO Nexus Node[2], which cannot do
physical signal property translation.

While an inverter can be described implicitly by exchanging the
GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW flags, this has its limitations.
Each GPIO line has only a single GPIO_ACTIVE_* flag, but applies to both
th provider and consumer sides:
  1. The GPIO provider (controller) looks at the flags to know the
     polarity, so it can translate between logical (active/not active)
     and physical (high/low) signal levels.
  2. While the signal polarity is usually fixed on the GPIO consumer
     side (e.g. an LED is tied to either the supply voltage or GND),
     it may be configurable on some devices, and both sides need to
     agree.  Hence the GPIO_ACTIVE_* flag as seen by the consumer must
     match the actual polarity.
     There exists a similar issue with interrupt flags, where both the
     interrupt controller and the device generating the interrupt need
     to agree, which breaks in the presence of a physical inverter not
     described in DT (see e.g. [3]).

[1] "[PATCH V4 2/2] gpio: inverter: document the inverter bindings"
    https://lore.kernel.org/linux-gpio/1561699236-18620-3-git-send-email-harish_kandiga@mentor.com/

[2] Devicetree Specification v0.3-rc2, Section 2.5
    https://github.com/devicetree-org/devicetree-specification/releases/tag/v0.3-rc2

[3] "[PATCH] wlcore/wl18xx: Add invert-irq OF property for physically
    inverted IRQ"
    https://lore.kernel.org/linux-renesas-soc/20190607172958.20745-1-erosca@de.adit-jv.com/

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v3:
  - New.
---
 .../bindings/gpio/gpio-repeater.yaml          | 53 +++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-repeater.yaml

diff --git a/Documentation/devicetree/bindings/gpio/gpio-repeater.yaml b/Documentation/devicetree/bindings/gpio/gpio-repeater.yaml
new file mode 100644
index 0000000000000000..efdee0c3be43f731
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-repeater.yaml
@@ -0,0 +1,53 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/gpio-repeater.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: GPIO Repeater
+
+maintainers:
+  - Harish Jenny K N <harish_kandiga@mentor.com>
+  - Geert Uytterhoeven <geert+renesas@glider.be>
+
+description:
+  This represents a repeater for one or more GPIOs, possibly including physical
+  signal property translation (e.g. polarity inversion).
+
+properties:
+  compatible:
+    const: gpio-repeater
+
+  "#gpio-cells":
+    const: 2
+
+  gpio-controller: true
+
+  gpios:
+    description:
+      Phandle and specifier, one for each repeated GPIO.
+
+  gpio-line-names:
+    description:
+      Strings defining the names of the GPIO lines going out of the GPIO
+      controller.
+
+required:
+  - compatible
+  - "#gpio-cells"
+  - gpio-controller
+  - gpios
+
+additionalProperties: false
+
+examples:
+  # Device node describing a polarity inverter for a single GPIO
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    inverter: gpio-repeater {
+        compatible = "gpio-repeater";
+        #gpio-cells = <2>;
+        gpio-controller;
+        gpios = <&gpio 95 GPIO_ACTIVE_LOW>;
+    };
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH v3 5/7] gpio: Add GPIO Aggregator/Repeater driver
  2019-11-27  8:42 [PATCH v3 0/7] gpio: Add GPIO Aggregator/Repeater Geert Uytterhoeven
                   ` (3 preceding siblings ...)
  2019-11-27  8:42 ` [PATCH v3 4/7] dt-bindings: gpio: Add gpio-repeater bindings Geert Uytterhoeven
@ 2019-11-27  8:42 ` Geert Uytterhoeven
  2019-11-27 14:15   ` Eugeniu Rosca
                     ` (4 more replies)
  2019-11-27  8:42 ` [PATCH v3 6/7] docs: gpio: Add GPIO Aggregator/Repeater documentation Geert Uytterhoeven
                   ` (2 subsequent siblings)
  7 siblings, 5 replies; 48+ messages in thread
From: Geert Uytterhoeven @ 2019-11-27  8:42 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski, Jonathan Corbet, Rob Herring,
	Mark Rutland, Harish Jenny K N, Eugeniu Rosca
  Cc: Alexander Graf, Peter Maydell, Paolo Bonzini, Phil Reid,
	Marc Zyngier, Christoffer Dall, Magnus Damm, linux-gpio,
	linux-doc, devicetree, linux-renesas-soc, linux-kernel,
	qemu-devel, Geert Uytterhoeven

GPIO controllers are exported to userspace using /dev/gpiochip*
character devices.  Access control to these devices is provided by
standard UNIX file system permissions, on an all-or-nothing basis:
either a GPIO controller is accessible for a user, or it is not.
Currently no mechanism exists to control access to individual GPIOs.

Hence add a GPIO driver to aggregate existing GPIOs, and expose them as
a new gpiochip.

This supports the following use cases:
  1. Aggregating GPIOs using Sysfs
     This is useful for implementing access control, and assigning a set
     of GPIOs to a specific user or virtual machine.

  2. GPIO Repeater in Device Tree
     This supports modelling e.g. GPIO inverters in DT.

  3. Generic GPIO Driver
     This provides userspace access to a simple GPIO-operated device
     described in DT, cfr. e.g. spidev for SPI-operated devices.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v3:
  - Absorb GPIO forwarder,
  - Integrate GPIO Repeater and Generic GPIO driver functionality,
  - Use the aggregator parameters to create a GPIO lookup table instead
    of an array of GPIO descriptors, which allows to simplify the code:
      1. This removes the need for calling gpio_name_to_desc(),
         gpiochip_find(), gpiochip_get_desc(), and gpiod_request(),
      2. This allows the platform device to always use
         devm_gpiod_get_index(), regardless of the origin of the GPIOs,
  - Move parameter parsing from platform device probe to sysfs attribute
    store, removing the need for platform data passing,
  - Use more devm_*() functions to simplify cleanup,
  - Add pr_fmt(),
  - General refactoring.

v2:
  - Add missing initialization of i in gpio_virt_agg_probe(),
  - Update for removed .need_valid_mask field and changed
    .init_valid_mask() signature,
  - Drop "virtual", rename to gpio-aggregator,
  - Drop bogus FIXME related to gpiod_set_transitory() expectations,
  - Use new GPIO Forwarder Helper,
  - Lift limit on the maximum number of GPIOs,
  - Improve parsing:
      - add support for specifying GPIOs by line name,
      - add support for specifying GPIO chips by ID,
      - add support for GPIO offset ranges,
      - names and offset specifiers must be separated by whitespace,
      - GPIO offsets must separated by spaces,
  - Use str_has_prefix() and kstrtouint().
---
 drivers/gpio/Kconfig           |  13 +
 drivers/gpio/Makefile          |   1 +
 drivers/gpio/gpio-aggregator.c | 587 +++++++++++++++++++++++++++++++++
 3 files changed, 601 insertions(+)
 create mode 100644 drivers/gpio/gpio-aggregator.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 8adffd42f8cb0559..36b6b57a6b05e906 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1507,6 +1507,19 @@ config GPIO_VIPERBOARD
 
 endmenu
 
+config GPIO_AGGREGATOR
+	tristate "GPIO Aggregator/Repeater"
+	help
+	  Say yes here to enable the GPIO Aggregator and repeater, which
+	  provides a way to aggregate and/or repeat existing GPIOs into a new
+	  GPIO device.
+	  This can serve the following purposes:
+	    1. Assign a collection of GPIOs to a user, or export them to a
+	       virtual machine,
+	    2. Support GPIOs that are connected to a physical inverter,
+	    3. Provide a generic driver for a GPIO-operated device, to be
+               controlled from userspace using the GPIO chardev interface.
+
 config GPIO_MOCKUP
 	tristate "GPIO Testing Driver"
 	select IRQ_SIM
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 34eb8b2b12dd656c..f9971eeb1f32335f 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -25,6 +25,7 @@ obj-$(CONFIG_GPIO_74XX_MMIO)		+= gpio-74xx-mmio.o
 obj-$(CONFIG_GPIO_ADNP)			+= gpio-adnp.o
 obj-$(CONFIG_GPIO_ADP5520)		+= gpio-adp5520.o
 obj-$(CONFIG_GPIO_ADP5588)		+= gpio-adp5588.o
+obj-$(CONFIG_GPIO_AGGREGATOR)		+= gpio-aggregator.o
 obj-$(CONFIG_GPIO_ALTERA_A10SR)		+= gpio-altera-a10sr.o
 obj-$(CONFIG_GPIO_ALTERA)  		+= gpio-altera.o
 obj-$(CONFIG_GPIO_AMD8111)		+= gpio-amd8111.o
diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
new file mode 100644
index 0000000000000000..873578c6f9683db8
--- /dev/null
+++ b/drivers/gpio/gpio-aggregator.c
@@ -0,0 +1,587 @@
+// SPDX-License-Identifier: GPL-2.0-only
+//
+// GPIO Aggregator and Repeater
+//
+// Copyright (C) 2019 Glider bvba
+
+#define DRV_NAME       "gpio-aggregator"
+#define pr_fmt(fmt)	DRV_NAME ": " fmt
+
+#include <linux/bitmap.h>
+#include <linux/bitops.h>
+#include <linux/ctype.h>
+#include <linux/gpio/driver.h>
+#include <linux/gpio/machine.h>
+#include <linux/idr.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/overflow.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <linux/string.h>
+
+#include "gpiolib.h"
+
+
+	/*
+	 * GPIO Aggregator sysfs interface
+	 */
+
+struct gpio_aggregator {
+	struct gpiod_lookup_table *lookups;
+	struct platform_device *pdev;
+	char args[];
+};
+
+static DEFINE_MUTEX(gpio_aggregator_lock);	/* protects idr */
+static DEFINE_IDR(gpio_aggregator_idr);
+
+static char *get_arg(char **args)
+{
+	char *start = *args, *end;
+
+	while (isspace(*start))
+		start++;
+
+	if (!*start)
+		return NULL;
+
+	if (*start == '"') {
+		/* Quoted arg */
+		end = strchr(++start, '"');
+		if (!end)
+			return ERR_PTR(-EINVAL);
+	} else {
+		/* Unquoted arg */
+		for (end = start; *end && !isspace(*end); end++) ;
+	}
+
+	if (*end)
+		*end++ = '\0';
+
+	*args = end;
+	return start;
+}
+
+static bool isrange(const char *s)
+{
+	size_t n = strlen(s);
+
+	if (IS_ERR_OR_NULL(s))
+		return false;
+
+	while (1) {
+		n = strspn(s, "0123456789");
+		if (!n)
+			return false;
+
+		s += n;
+
+		switch (*s++) {
+		case '\0':
+			return true;
+
+		case '-':
+		case ',':
+			break;
+
+		default:
+			return false;
+		}
+	}
+}
+
+static int aggr_add_gpio(struct gpio_aggregator *aggr, const char *label,
+			 int hwnum, unsigned int *n)
+{
+	struct gpiod_lookup_table *lookups;
+
+	lookups = krealloc(aggr->lookups, struct_size(lookups, table, *n + 2),
+			   GFP_KERNEL);
+	if (!lookups)
+		return -ENOMEM;
+
+	lookups->table[*n].chip_label = label;
+	lookups->table[*n].chip_hwnum = hwnum;
+	lookups->table[*n].idx = *n;
+
+	(*n)++;
+	memset(&lookups->table[*n], 0, sizeof(lookups->table[*n]));
+
+	aggr->lookups = lookups;
+	return 0;
+}
+
+static int aggr_parse(struct gpio_aggregator *aggr)
+{
+	char *name, *offsets, *first, *last, *next;
+	unsigned int a, b, i, n = 0;
+	char *args = aggr->args;
+	int error;
+
+	for (name = get_arg(&args), offsets = get_arg(&args); name;
+	     offsets = get_arg(&args)) {
+		if (IS_ERR(name)) {
+			pr_err("Cannot get GPIO specifier: %ld\n",
+			       PTR_ERR(name));
+			return PTR_ERR(name);
+		}
+
+		if (!isrange(offsets)) {
+			/* Named GPIO line */
+			error = aggr_add_gpio(aggr, name, -1, &n);
+			if (error)
+				return error;
+
+			name = offsets;
+			continue;
+		}
+
+		/* GPIO chip + offset(s) */
+		for (first = offsets; *first; first = next) {
+			next = strchrnul(first, ',');
+			if (*next)
+				*next++ = '\0';
+
+			last = strchr(first, '-');
+			if (last)
+				*last++ = '\0';
+
+			if (kstrtouint(first, 10, &a)) {
+				pr_err("Cannot parse GPIO index %s\n", first);
+				return -EINVAL;
+			}
+
+			if (!last) {
+				b = a;
+			} else if (kstrtouint(last, 10, &b)) {
+				pr_err("Cannot parse GPIO index %s\n", last);
+				return -EINVAL;
+			}
+
+			for (i = a; i <= b; i++) {
+				error = aggr_add_gpio(aggr, name, i, &n);
+				if (error)
+					return error;
+			}
+		}
+
+		name = get_arg(&args);
+	}
+
+	if (!n) {
+		pr_err("No GPIOs specified\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static ssize_t new_device_store(struct device_driver *driver, const char *buf,
+				size_t count)
+{
+	struct gpio_aggregator *aggr;
+	struct platform_device *pdev;
+	int res, id;
+
+	/* kernfs guarantees string termination, so count + 1 is safe */
+	aggr = kzalloc(sizeof(*aggr) + count + 1, GFP_KERNEL);
+	if (!aggr)
+		return -ENOMEM;
+
+	memcpy(aggr->args, buf, count + 1);
+
+	aggr->lookups = kzalloc(struct_size(aggr->lookups, table, 1),
+				GFP_KERNEL);
+	if (!aggr->lookups) {
+		res = -ENOMEM;
+		goto free_ga;
+	}
+
+	mutex_lock(&gpio_aggregator_lock);
+	id = idr_alloc(&gpio_aggregator_idr, aggr, 0, 0, GFP_KERNEL);
+	mutex_unlock(&gpio_aggregator_lock);
+
+	if (id < 0) {
+		res = id;
+		goto free_table;
+	}
+
+	aggr->lookups->dev_id = kasprintf(GFP_KERNEL, "%s.%d", DRV_NAME, id);
+	if (!aggr->lookups) {
+		res = -ENOMEM;
+		goto remove_idr;
+	}
+
+	res = aggr_parse(aggr);
+	if (res)
+		goto free_dev_id;
+
+	gpiod_add_lookup_table(aggr->lookups);
+
+	pdev = platform_device_register_simple(DRV_NAME, id, NULL, 0);
+	if (IS_ERR(pdev)) {
+		res = PTR_ERR(pdev);
+		goto remove_table;
+	}
+
+	aggr->pdev = pdev;
+	return count;
+
+remove_table:
+	gpiod_remove_lookup_table(aggr->lookups);
+free_dev_id:
+	kfree(aggr->lookups->dev_id);
+remove_idr:
+	mutex_lock(&gpio_aggregator_lock);
+	idr_remove(&gpio_aggregator_idr, id);
+	mutex_unlock(&gpio_aggregator_lock);
+free_table:
+	kfree(aggr->lookups);
+free_ga:
+	kfree(aggr);
+	return res;
+}
+
+static DRIVER_ATTR_WO(new_device);
+
+static void gpio_aggregator_free(struct gpio_aggregator *aggr)
+{
+	platform_device_unregister(aggr->pdev);
+	gpiod_remove_lookup_table(aggr->lookups);
+	kfree(aggr->lookups->dev_id);
+	kfree(aggr->lookups);
+	kfree(aggr);
+}
+
+static ssize_t delete_device_store(struct device_driver *driver,
+				   const char *buf, size_t count)
+{
+	struct gpio_aggregator *aggr;
+	unsigned int id;
+	int error;
+
+	if (!str_has_prefix(buf, DRV_NAME "."))
+		return -EINVAL;
+
+	error = kstrtouint(buf + strlen(DRV_NAME "."), 10, &id);
+	if (error)
+		return error;
+
+	mutex_lock(&gpio_aggregator_lock);
+	aggr = idr_remove(&gpio_aggregator_idr, id);
+	mutex_unlock(&gpio_aggregator_lock);
+	if (!aggr)
+		return -ENOENT;
+
+	gpio_aggregator_free(aggr);
+	return count;
+}
+static DRIVER_ATTR_WO(delete_device);
+
+static struct attribute *gpio_aggregator_attrs[] = {
+	&driver_attr_new_device.attr,
+	&driver_attr_delete_device.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(gpio_aggregator);
+
+static int __exit gpio_aggregator_idr_remove(int id, void *p, void *data)
+{
+	gpio_aggregator_free(p);
+	return 0;
+}
+
+static void __exit gpio_aggregator_remove_all(void)
+{
+	mutex_lock(&gpio_aggregator_lock);
+	idr_for_each(&gpio_aggregator_idr, gpio_aggregator_idr_remove, NULL);
+	idr_destroy(&gpio_aggregator_idr);
+	mutex_unlock(&gpio_aggregator_lock);
+}
+
+
+	/*
+	 *  Common GPIO Forwarder
+	 */
+
+struct gpiochip_fwd {
+	struct gpio_chip chip;
+	struct gpio_desc **descs;
+	union {
+		struct mutex mlock;	/* protects tmp[] if can_sleep */
+		spinlock_t slock;	/* protects tmp[] if !can_sleep */
+	};
+	unsigned long tmp[];		/* values and descs for multiple ops */
+};
+
+static int gpio_fwd_get_direction(struct gpio_chip *chip, unsigned int offset)
+{
+	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
+
+	return gpiod_get_direction(fwd->descs[offset]);
+}
+
+static int gpio_fwd_direction_input(struct gpio_chip *chip, unsigned int offset)
+{
+	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
+
+	return gpiod_direction_input(fwd->descs[offset]);
+}
+
+static int gpio_fwd_direction_output(struct gpio_chip *chip,
+				     unsigned int offset, int value)
+{
+	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
+
+	return gpiod_direction_output(fwd->descs[offset], value);
+}
+
+static int gpio_fwd_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
+
+	return gpiod_get_value(fwd->descs[offset]);
+}
+
+static int gpio_fwd_get_multiple(struct gpio_chip *chip, unsigned long *mask,
+				 unsigned long *bits)
+{
+	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
+	unsigned long *values, flags;
+	struct gpio_desc **descs;
+	unsigned int i, j = 0;
+	int error;
+
+	if (chip->can_sleep)
+		mutex_lock(&fwd->mlock);
+	else
+		spin_lock_irqsave(&fwd->slock, flags);
+
+	values = &fwd->tmp[0];
+	bitmap_clear(values, 0, fwd->chip.ngpio);
+	descs = (void *)&fwd->tmp[BITS_TO_LONGS(fwd->chip.ngpio)];
+
+	for_each_set_bit(i, mask, fwd->chip.ngpio)
+		descs[j++] = fwd->descs[i];
+
+	error = gpiod_get_array_value(j, descs, NULL, values);
+	if (!error) {
+		j = 0;
+		for_each_set_bit(i, mask, fwd->chip.ngpio)
+			__assign_bit(i, bits, test_bit(j++, values));
+	}
+
+	if (chip->can_sleep)
+		mutex_unlock(&fwd->mlock);
+	else
+		spin_unlock_irqrestore(&fwd->slock, flags);
+
+	return error;
+}
+
+static void gpio_fwd_set(struct gpio_chip *chip, unsigned int offset, int value)
+{
+	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
+
+	gpiod_set_value(fwd->descs[offset], value);
+}
+
+static void gpio_fwd_set_multiple(struct gpio_chip *chip, unsigned long *mask,
+				  unsigned long *bits)
+{
+	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
+	unsigned long *values, flags;
+	struct gpio_desc **descs;
+	unsigned int i, j = 0;
+
+	if (chip->can_sleep)
+		mutex_lock(&fwd->mlock);
+	else
+		spin_lock_irqsave(&fwd->slock, flags);
+
+	values = &fwd->tmp[0];
+	descs = (void *)&fwd->tmp[BITS_TO_LONGS(fwd->chip.ngpio)];
+
+	for_each_set_bit(i, mask, fwd->chip.ngpio) {
+		__assign_bit(j, values, test_bit(i, bits));
+		descs[j++] = fwd->descs[i];
+	}
+
+	gpiod_set_array_value(j, descs, NULL, values);
+
+	if (chip->can_sleep)
+		mutex_unlock(&fwd->mlock);
+	else
+		spin_unlock_irqrestore(&fwd->slock, flags);
+}
+
+static int gpio_fwd_set_config(struct gpio_chip *chip, unsigned int offset,
+			       unsigned long config)
+{
+	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
+
+	chip = fwd->descs[offset]->gdev->chip;
+	if (chip->set_config)
+		return chip->set_config(chip, offset, config);
+
+	return -ENOTSUPP;
+}
+
+static int gpio_fwd_init_valid_mask(struct gpio_chip *chip,
+				    unsigned long *valid_mask,
+				    unsigned int ngpios)
+{
+	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
+	unsigned int i;
+
+	for (i = 0; i < ngpios; i++) {
+		if (!gpiochip_line_is_valid(fwd->descs[i]->gdev->chip,
+					    gpio_chip_hwgpio(fwd->descs[i])))
+			clear_bit(i, valid_mask);
+	}
+
+	return 0;
+}
+
+/**
+ * gpiochip_fwd_create() - Create a new GPIO forwarder
+ * @dev: Parent device pointer
+ * @ngpios: Number of GPIOs in the forwarder.
+ * @descs: Array containing the GPIO descriptors to forward to.
+ *         This array must contain @ngpios entries, and must not be deallocated
+ *         before the forwarder has been destroyed again.
+ *
+ * This function creates a new gpiochip, which forwards all GPIO operations to
+ * the passed GPIO descriptors.
+ *
+ * Return: An opaque object pointer, or an ERR_PTR()-encoded negative error
+ *         code on failure.
+ */
+static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
+						unsigned int ngpios,
+						struct gpio_desc *descs[])
+{
+	const char *label = dev_name(dev);
+	struct gpiochip_fwd *fwd;
+	struct gpio_chip *chip;
+	unsigned int i;
+	int error;
+
+	fwd = devm_kzalloc(dev, struct_size(fwd, tmp,
+			   BITS_TO_LONGS(ngpios) + ngpios), GFP_KERNEL);
+	if (!fwd)
+		return ERR_PTR(-ENOMEM);
+
+	chip = &fwd->chip;
+
+	for (i = 0; i < ngpios; i++) {
+		dev_dbg(dev, "gpio %u => gpio-%d (%s)\n", i,
+			desc_to_gpio(descs[i]), descs[i]->label ? : "?");
+
+		if (gpiod_cansleep(descs[i]))
+			chip->can_sleep = true;
+		if (descs[i]->gdev->chip->set_config)
+			chip->set_config = gpio_fwd_set_config;
+		if (descs[i]->gdev->chip->init_valid_mask)
+			chip->init_valid_mask = gpio_fwd_init_valid_mask;
+	}
+
+	chip->label = label;
+	chip->parent = dev;
+	chip->owner = THIS_MODULE;
+	chip->get_direction = gpio_fwd_get_direction;
+	chip->direction_input = gpio_fwd_direction_input;
+	chip->direction_output = gpio_fwd_direction_output;
+	chip->get = gpio_fwd_get;
+	chip->get_multiple = gpio_fwd_get_multiple;
+	chip->set = gpio_fwd_set;
+	chip->set_multiple = gpio_fwd_set_multiple;
+	chip->base = -1;
+	chip->ngpio = ngpios;
+	fwd->descs = descs;
+
+	if (chip->can_sleep)
+		mutex_init(&fwd->mlock);
+	else
+		spin_lock_init(&fwd->slock);
+
+	error = devm_gpiochip_add_data(dev, chip, fwd);
+	if (error)
+		return ERR_PTR(error);
+
+	return fwd;
+}
+
+
+	/*
+	 *  Common GPIO Aggregator/Repeater platform device
+	 */
+
+static int gpio_aggregator_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct gpio_desc **descs;
+	struct gpiochip_fwd *fwd;
+	int i, n;
+
+	n = gpiod_count(dev, NULL);
+	if (n < 0)
+		return n;
+
+	descs = devm_kmalloc_array(dev, n, sizeof(*descs), GFP_KERNEL);
+	if (!descs)
+		return -ENOMEM;
+
+	for (i = 0; i < n; i++) {
+		descs[i] = devm_gpiod_get_index(dev, NULL, i, GPIOD_ASIS);
+		if (IS_ERR(descs[i]))
+			return PTR_ERR(descs[i]);
+	}
+
+	fwd = gpiochip_fwd_create(dev, n, descs);
+	if (IS_ERR(fwd))
+		return PTR_ERR(fwd);
+
+	platform_set_drvdata(pdev, fwd);
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id gpio_aggregator_dt_ids[] = {
+	{ .compatible = "gpio-repeater" },
+	/*
+	 * Add GPIO-operated devices controlled from userspace below,
+	 * or use "driver_override" in sysfs
+	 */
+	{},
+};
+MODULE_DEVICE_TABLE(of, gpio_aggregator_dt_ids);
+#endif
+
+static struct platform_driver gpio_aggregator_driver = {
+	.probe = gpio_aggregator_probe,
+	.driver = {
+		.name = DRV_NAME,
+		.groups = gpio_aggregator_groups,
+		.of_match_table = of_match_ptr(gpio_aggregator_dt_ids),
+	},
+};
+
+static int __init gpio_aggregator_init(void)
+{
+	return platform_driver_register(&gpio_aggregator_driver);
+}
+module_init(gpio_aggregator_init);
+
+static void __exit gpio_aggregator_exit(void)
+{
+	gpio_aggregator_remove_all();
+	platform_driver_unregister(&gpio_aggregator_driver);
+}
+module_exit(gpio_aggregator_exit);
+
+MODULE_AUTHOR("Geert Uytterhoeven <geert+renesas@glider.be>");
+MODULE_DESCRIPTION("GPIO Aggregator/Repeater");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH v3 6/7] docs: gpio: Add GPIO Aggregator/Repeater documentation
  2019-11-27  8:42 [PATCH v3 0/7] gpio: Add GPIO Aggregator/Repeater Geert Uytterhoeven
                   ` (4 preceding siblings ...)
  2019-11-27  8:42 ` [PATCH v3 5/7] gpio: Add GPIO Aggregator/Repeater driver Geert Uytterhoeven
@ 2019-11-27  8:42 ` Geert Uytterhoeven
  2019-11-28  3:41   ` Ulrich Hecht
  2019-12-12 14:42   ` Linus Walleij
  2019-11-27  8:42 ` [PATCH v3 7/7] MAINTAINERS: Add GPIO Aggregator/Repeater section Geert Uytterhoeven
  2020-01-18  1:46 ` [PATCH v3 0/7] gpio: Add GPIO Aggregator/Repeater Eugeniu Rosca
  7 siblings, 2 replies; 48+ messages in thread
From: Geert Uytterhoeven @ 2019-11-27  8:42 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski, Jonathan Corbet, Rob Herring,
	Mark Rutland, Harish Jenny K N, Eugeniu Rosca
  Cc: Alexander Graf, Peter Maydell, Paolo Bonzini, Phil Reid,
	Marc Zyngier, Christoffer Dall, Magnus Damm, linux-gpio,
	linux-doc, devicetree, linux-renesas-soc, linux-kernel,
	qemu-devel, Geert Uytterhoeven

Document the GPIO Aggregator/Repeater, and the three typical use-cases.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v3:
  - New.
---
 .../admin-guide/gpio/gpio-aggregator.rst      | 111 ++++++++++++++++++
 Documentation/admin-guide/gpio/index.rst      |   1 +
 2 files changed, 112 insertions(+)
 create mode 100644 Documentation/admin-guide/gpio/gpio-aggregator.rst

diff --git a/Documentation/admin-guide/gpio/gpio-aggregator.rst b/Documentation/admin-guide/gpio/gpio-aggregator.rst
new file mode 100644
index 0000000000000000..826146e260253299
--- /dev/null
+++ b/Documentation/admin-guide/gpio/gpio-aggregator.rst
@@ -0,0 +1,111 @@
+.. SPDX-License-Identifier: GPL-2.0-only
+
+GPIO Aggregator/Repeater
+========================
+
+The GPIO Aggregator/Repeater allows to aggregate GPIOs, and expose them as a
+new gpio_chip.  This supports the following use cases.
+
+
+Aggregating GPIOs using Sysfs
+-----------------------------
+
+GPIO controllers are exported to userspace using /dev/gpiochip* character
+devices.  Access control to these devices is provided by standard UNIX file
+system permissions, on an all-or-nothing basis: either a GPIO controller is
+accessible for a user, or it is not.
+
+The GPIO Aggregator allows access control for individual GPIOs, by aggregating
+them into a new gpio_chip, which can be assigned to a group or user using
+standard UNIX file ownership and permissions.  Furthermore, this simplifies and
+hardens exporting GPIOs to a virtual machine, as the VM can just grab the full
+GPIO controller, and no longer needs to care about which GPIOs to grab and
+which not, reducing the attack surface.
+
+Aggregated GPIO controllers are instantiated and destroyed by writing to
+write-only attribute files in sysfs.
+
+    /sys/bus/platform/drivers/gpio-aggregator/
+
+	"new_device" ...
+		Userspace may ask the kernel to instantiate an aggregated GPIO
+		controller by writing a string describing the GPIOs to
+		aggregate to the "new_device" file, using the format
+
+		.. code-block:: none
+
+		    [<gpioA>] [<gpiochipB> <offsets>] ...
+
+		Where:
+
+		    "<gpioA>" ...
+			    is a GPIO line name,
+
+		    "<gpiochipB>" ...
+			    is a GPIO chip label or name, and
+
+		    "<offsets>" ...
+			    is a comma-separated list of GPIO offsets and/or
+			    GPIO offset ranges denoted by dashes.
+
+		Example: Instantiate a new GPIO aggregator by aggregating GPIO
+		19 of "e6052000.gpio" and GPIOs 20-21 of "gpiochip2" into a new
+		gpio_chip:
+
+		.. code-block:: bash
+
+		    echo 'e6052000.gpio 19 gpiochip2 20-21' > new_device
+
+	"delete_device" ...
+		Userspace may ask the kernel to destroy an aggregated GPIO
+		controller after use by writing its device name to the
+		"delete_device" file.
+
+		Example: Destroy the previously-created aggregated GPIO
+		controller "gpio-aggregator.0":
+
+		.. code-block:: bash
+
+		    echo gpio-aggregator.0 > delete_device
+
+
+GPIO Repeater in Device Tree
+----------------------------
+
+A GPIO Repeater is a node in a Device Tree representing a repeater for one or
+more GPIOs, possibly including physical signal property translation (e.g.
+polarity inversion).  This allows to model e.g. inverters in DT.
+
+See Documentation/devicetree/bindings/gpio/gpio-repeater.yaml
+
+
+Generic GPIO Driver
+-------------------
+
+The GPIO Aggregator can also be used as a generic driver for a simple
+GPIO-operated device described in DT, without a dedicated in-kernel driver.
+This is not unlike e.g. spidev, which allows to communicated with an SPI device
+from userspace.
+
+Binding a device to the GPIO Aggregator is performed either by modifying the
+gpio-aggregator driver, or by writing to the "driver_override" file in Sysfs.
+
+Example: If "frobnicator" is a GPIO-operated device described in DT, using its
+own compatible value::
+
+        frobnicator {
+                compatible = "myvendor,frobnicator";
+
+                gpios = <&gpio2 19 GPIO_ACTIVE_HIGH>,
+                        <&gpio2 20 GPIO_ACTIVE_LOW>;
+        };
+
+it can be bound to the GPIO Aggregator by either:
+
+1. Adding its compatible value to ``gpio_aggregator_dt_ids[]``,
+2. Binding manually using "driver_override":
+
+.. code-block:: bash
+
+    echo gpio-aggregator > /sys/bus/platform/devices/frobnicator/driver_override
+    echo frobnicator > /sys/bus/platform/drivers/gpio-aggregator/bind
diff --git a/Documentation/admin-guide/gpio/index.rst b/Documentation/admin-guide/gpio/index.rst
index a244ba4e87d5398a..ef2838638e967777 100644
--- a/Documentation/admin-guide/gpio/index.rst
+++ b/Documentation/admin-guide/gpio/index.rst
@@ -7,6 +7,7 @@ gpio
 .. toctree::
     :maxdepth: 1
 
+    gpio-aggregator
     sysfs
 
 .. only::  subproject and html
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH v3 7/7] MAINTAINERS: Add GPIO Aggregator/Repeater section
  2019-11-27  8:42 [PATCH v3 0/7] gpio: Add GPIO Aggregator/Repeater Geert Uytterhoeven
                   ` (5 preceding siblings ...)
  2019-11-27  8:42 ` [PATCH v3 6/7] docs: gpio: Add GPIO Aggregator/Repeater documentation Geert Uytterhoeven
@ 2019-11-27  8:42 ` Geert Uytterhoeven
  2019-12-03  5:38   ` Harish Jenny K N
  2020-01-18  1:46 ` [PATCH v3 0/7] gpio: Add GPIO Aggregator/Repeater Eugeniu Rosca
  7 siblings, 1 reply; 48+ messages in thread
From: Geert Uytterhoeven @ 2019-11-27  8:42 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski, Jonathan Corbet, Rob Herring,
	Mark Rutland, Harish Jenny K N, Eugeniu Rosca
  Cc: Alexander Graf, Peter Maydell, Paolo Bonzini, Phil Reid,
	Marc Zyngier, Christoffer Dall, Magnus Damm, linux-gpio,
	linux-doc, devicetree, linux-renesas-soc, linux-kernel,
	qemu-devel, Geert Uytterhoeven

Add a maintainership section for the GPIO Aggregator/Repeater, covering
documentation, Device Tree bindings, and driver source code.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Harish: Do you want to be listed as maintainer, too?

v3:
  - New.
---
 MAINTAINERS | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index e5949b6827b72f2b..0f12ebdaa8faa76b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7043,6 +7043,14 @@ S:	Maintained
 F:	Documentation/firmware-guide/acpi/gpio-properties.rst
 F:	drivers/gpio/gpiolib-acpi.c
 
+GPIO AGGREGATOR/REPEATER
+M:	Geert Uytterhoeven <geert+renesas@glider.be>
+L:	linux-gpio@vger.kernel.org
+S:	Maintained
+F:	Documentation/admin-guide/gpio/gpio-aggregator.rst
+F:	Documentation/devicetree/bindings/gpio/gpio-repeater.yaml
+F:	drivers/gpio/gpio-aggregator.c
+
 GPIO IR Transmitter
 M:	Sean Young <sean@mess.org>
 L:	linux-media@vger.kernel.org
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 5/7] gpio: Add GPIO Aggregator/Repeater driver
  2019-11-27  8:42 ` [PATCH v3 5/7] gpio: Add GPIO Aggregator/Repeater driver Geert Uytterhoeven
@ 2019-11-27 14:15   ` Eugeniu Rosca
  2019-11-27 14:33     ` Geert Uytterhoeven
  2019-11-28  3:40   ` Ulrich Hecht
                     ` (3 subsequent siblings)
  4 siblings, 1 reply; 48+ messages in thread
From: Eugeniu Rosca @ 2019-11-27 14:15 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Linus Walleij, Bartosz Golaszewski, Jonathan Corbet, Rob Herring,
	Mark Rutland, Harish Jenny K N, Eugeniu Rosca, Alexander Graf,
	Peter Maydell, Paolo Bonzini, Phil Reid, Marc Zyngier,
	Christoffer Dall, Magnus Damm, linux-gpio, linux-doc, devicetree,
	linux-renesas-soc, linux-kernel, qemu-devel, Eugeniu Rosca

Hi Geert,

Many thanks for the series upgrade.
A few static-analysis findings below (could be false positives).

On Wed, Nov 27, 2019 at 09:42:51AM +0100, Geert Uytterhoeven wrote:

[..]

> +static bool isrange(const char *s)
> +{
> +	size_t n = strlen(s);

Cppcheck 1.40-18521-ge6d692d96058:
drivers/gpio/gpio-aggregator.c:69:11: style: Variable 'n' is assigned a value that is never used. [unreadVariable]

Smatch v0.5.0-6150-gc1ed13e4ee7b:
drivers/gpio/gpio-aggregator.c:69 isrange() warn: unused return: n = strlen()

[..]

> +	aggr->lookups->dev_id = kasprintf(GFP_KERNEL, "%s.%d", DRV_NAME, id);
> +	if (!aggr->lookups) {
> +		res = -ENOMEM;
> +		goto remove_idr;
> +	}

s/aggr->lookups/aggr->lookups->dev_id/ ?

[..]

> +static int gpio_fwd_get_multiple(struct gpio_chip *chip, unsigned long *mask,
> +				 unsigned long *bits)
> +{
> +	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> +	unsigned long *values, flags;

gcc 9.2.1:
warning: ‘flags’ may be used uninitialized in this function [-Wmaybe-uninitialized]

[..]

> +static void gpio_fwd_set_multiple(struct gpio_chip *chip, unsigned long *mask,
> +				  unsigned long *bits)
> +{
> +	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> +	unsigned long *values, flags;

gcc 9.2.1, same as above:
warning: ‘flags’ may be used uninitialized in this function [-Wmaybe-uninitialized]

Should these be silenced like in 2bf593f101f3ca ("xilinx_uartps.c:
suppress "may be used uninitialised" warning") ?

I plan to do some runtime testing soon.

-- 
Best Regards,
Eugeniu

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 5/7] gpio: Add GPIO Aggregator/Repeater driver
  2019-11-27 14:15   ` Eugeniu Rosca
@ 2019-11-27 14:33     ` Geert Uytterhoeven
  0 siblings, 0 replies; 48+ messages in thread
From: Geert Uytterhoeven @ 2019-11-27 14:33 UTC (permalink / raw)
  To: Eugeniu Rosca
  Cc: Geert Uytterhoeven, Linus Walleij, Bartosz Golaszewski,
	Jonathan Corbet, Rob Herring, Mark Rutland, Harish Jenny K N,
	Alexander Graf, Peter Maydell, Paolo Bonzini, Phil Reid,
	Marc Zyngier, Christoffer Dall, Magnus Damm,
	open list:GPIO SUBSYSTEM, open list:DOCUMENTATION,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, Linux Kernel Mailing List, QEMU Developers,
	Eugeniu Rosca

Hi Eugeniu,

On Wed, Nov 27, 2019 at 3:15 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> On Wed, Nov 27, 2019 at 09:42:51AM +0100, Geert Uytterhoeven wrote:
> > +static bool isrange(const char *s)
> > +{
> > +     size_t n = strlen(s);
>
> Cppcheck 1.40-18521-ge6d692d96058:
> drivers/gpio/gpio-aggregator.c:69:11: style: Variable 'n' is assigned a value that is never used. [unreadVariable]
>
> Smatch v0.5.0-6150-gc1ed13e4ee7b:
> drivers/gpio/gpio-aggregator.c:69 isrange() warn: unused return: n = strlen()

Correct, this is a remainder of code present temporarily during development.
Will drop.

(where are the days gcc itself warned about that?)

> > +     aggr->lookups->dev_id = kasprintf(GFP_KERNEL, "%s.%d", DRV_NAME, id);
> > +     if (!aggr->lookups) {
> > +             res = -ENOMEM;
> > +             goto remove_idr;
> > +     }
>
> s/aggr->lookups/aggr->lookups->dev_id/ ?

Thanks, will fix.

> > +static int gpio_fwd_get_multiple(struct gpio_chip *chip, unsigned long *mask,
> > +                              unsigned long *bits)
> > +{
> > +     struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> > +     unsigned long *values, flags;
>
> gcc 9.2.1:
> warning: ‘flags’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>
> [..]
>
> > +static void gpio_fwd_set_multiple(struct gpio_chip *chip, unsigned long *mask,
> > +                               unsigned long *bits)
> > +{
> > +     struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> > +     unsigned long *values, flags;
>
> gcc 9.2.1, same as above:
> warning: ‘flags’ may be used uninitialized in this function [-Wmaybe-uninitialized]

So newer gcc is (again) no longer smart enough to notice the check is
the same for initializer and user...

> Should these be silenced like in 2bf593f101f3ca ("xilinx_uartps.c:
> suppress "may be used uninitialised" warning") ?

TBH, I'm not a big fan of silencing false positives.
But if people like to see flags preinitialized to zero, that can be done...

> I plan to do some runtime testing soon.

Thanks, looking forward to the results!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 1/7] gpiolib: Add GPIOCHIP_NAME definition
  2019-11-27  8:42 ` [PATCH v3 1/7] gpiolib: Add GPIOCHIP_NAME definition Geert Uytterhoeven
@ 2019-11-28  3:38   ` Ulrich Hecht
  2019-12-02 21:17   ` Eugeniu Rosca
  2019-12-12 10:37   ` Linus Walleij
  2 siblings, 0 replies; 48+ messages in thread
From: Ulrich Hecht @ 2019-11-28  3:38 UTC (permalink / raw)
  To: Geert Uytterhoeven, Linus Walleij, Bartosz Golaszewski,
	Jonathan Corbet, Rob Herring, Mark Rutland, Harish Jenny K N,
	Eugeniu Rosca
  Cc: Alexander Graf, Peter Maydell, Paolo Bonzini, Phil Reid,
	Marc Zyngier, Christoffer Dall, Magnus Damm, linux-gpio,
	linux-doc, devicetree, linux-renesas-soc, linux-kernel,
	qemu-devel


> On November 27, 2019 at 9:42 AM Geert Uytterhoeven <geert+renesas@glider.be> wrote:
> 
> 
> The string literal "gpiochip" is used in several places.
> Add a definition for it, and use it everywhere, to make sure everything
> stays in sync.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v3:
>   - New.
> ---
>  drivers/gpio/gpiolib-sysfs.c | 7 +++----
>  drivers/gpio/gpiolib.c       | 4 ++--
>  drivers/gpio/gpiolib.h       | 2 ++
>  3 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
> index fbf6b1a0a4fae6ce..23e3d335cd543d53 100644
> --- a/drivers/gpio/gpiolib-sysfs.c
> +++ b/drivers/gpio/gpiolib-sysfs.c
> @@ -762,10 +762,9 @@ int gpiochip_sysfs_register(struct gpio_device *gdev)
>  		parent = &gdev->dev;
>  
>  	/* use chip->base for the ID; it's already known to be unique */
> -	dev = device_create_with_groups(&gpio_class, parent,
> -					MKDEV(0, 0),
> -					chip, gpiochip_groups,
> -					"gpiochip%d", chip->base);
> +	dev = device_create_with_groups(&gpio_class, parent, MKDEV(0, 0), chip,
> +					gpiochip_groups, GPIOCHIP_NAME "%d",
> +					chip->base);
>  	if (IS_ERR(dev))
>  		return PTR_ERR(dev);
>  
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index dce0b31f4125a6b3..c9e47620d2434983 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1419,7 +1419,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *chip, void *data,
>  		ret = gdev->id;
>  		goto err_free_gdev;
>  	}
> -	dev_set_name(&gdev->dev, "gpiochip%d", gdev->id);
> +	dev_set_name(&gdev->dev, GPIOCHIP_NAME "%d", gdev->id);
>  	device_initialize(&gdev->dev);
>  	dev_set_drvdata(&gdev->dev, gdev);
>  	if (chip->parent && chip->parent->driver)
> @@ -5105,7 +5105,7 @@ static int __init gpiolib_dev_init(void)
>  		return ret;
>  	}
>  
> -	ret = alloc_chrdev_region(&gpio_devt, 0, GPIO_DEV_MAX, "gpiochip");
> +	ret = alloc_chrdev_region(&gpio_devt, 0, GPIO_DEV_MAX, GPIOCHIP_NAME);
>  	if (ret < 0) {
>  		pr_err("gpiolib: failed to allocate char dev region\n");
>  		bus_unregister(&gpio_bus_type);
> diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
> index ca9bc1e4803c2979..a4a759920faa48ab 100644
> --- a/drivers/gpio/gpiolib.h
> +++ b/drivers/gpio/gpiolib.h
> @@ -16,6 +16,8 @@
>  #include <linux/module.h>
>  #include <linux/cdev.h>
>  
> +#define GPIOCHIP_NAME	"gpiochip"
> +
>  /**
>   * struct gpio_device - internal state container for GPIO devices
>   * @id: numerical ID number for the GPIO chip
> -- 
> 2.17.1
>

Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>

CU
Uli

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 2/7] gpiolib: Add support for gpiochipN-based table lookup
  2019-11-27  8:42 ` [PATCH v3 2/7] gpiolib: Add support for gpiochipN-based table lookup Geert Uytterhoeven
@ 2019-11-28  3:38   ` Ulrich Hecht
  2019-12-12 13:20   ` Linus Walleij
  1 sibling, 0 replies; 48+ messages in thread
From: Ulrich Hecht @ 2019-11-28  3:38 UTC (permalink / raw)
  To: Geert Uytterhoeven, Linus Walleij, Bartosz Golaszewski,
	Jonathan Corbet, Rob Herring, Mark Rutland, Harish Jenny K N,
	Eugeniu Rosca
  Cc: Alexander Graf, Peter Maydell, Paolo Bonzini, Phil Reid,
	Marc Zyngier, Christoffer Dall, Magnus Damm, linux-gpio,
	linux-doc, devicetree, linux-renesas-soc, linux-kernel,
	qemu-devel


> On November 27, 2019 at 9:42 AM Geert Uytterhoeven <geert+renesas@glider.be> wrote:
> 
> 
> Currently GPIO controllers can only be referred to by label in GPIO
> lookup tables.
> 
> Add support for looking them up by "gpiochipN" name, with "N" either the
> corresponding GPIO device's ID number, or the GPIO controller's first
> GPIO number.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> If this is rejected, the GPIO Aggregator documentation must be updated.
> 
> The second variant is currently used by the legacy sysfs interface only,
> so perhaps the chip->base check should be dropped?
> 
> v3:
>   - New.
> ---
>  drivers/gpio/gpiolib.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index c9e47620d2434983..d24a3d79dcfe69ad 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1746,9 +1746,29 @@ static int gpiochip_match_name(struct gpio_chip *chip, void *data)
>  	return !strcmp(chip->label, name);
>  }
>  
> +static int gpiochip_match_id(struct gpio_chip *chip, void *data)
> +{
> +	int id = (uintptr_t)data;
> +
> +	return id == chip->base || id == chip->gpiodev->id;
> +}
> +
>  static struct gpio_chip *find_chip_by_name(const char *name)
>  {
> -	return gpiochip_find((void *)name, gpiochip_match_name);
> +	struct gpio_chip *chip;
> +	int id;
> +
> +	chip = gpiochip_find((void *)name, gpiochip_match_name);
> +	if (chip)
> +		return chip;
> +
> +	if (!str_has_prefix(name, GPIOCHIP_NAME))
> +		return NULL;
> +
> +	if (kstrtoint(name + strlen(GPIOCHIP_NAME), 10, &id))
> +		return NULL;
> +
> +	return gpiochip_find((void *)(uintptr_t)id, gpiochip_match_id);
>  }
>  
>  #ifdef CONFIG_GPIOLIB_IRQCHIP
> -- 
> 2.17.1
>

Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>

CU
Uli

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 3/7] gpiolib: Add support for GPIO line table lookup
  2019-11-27  8:42 ` [PATCH v3 3/7] gpiolib: Add support for GPIO line " Geert Uytterhoeven
@ 2019-11-28  3:39   ` Ulrich Hecht
  2019-12-12 13:40   ` Linus Walleij
  1 sibling, 0 replies; 48+ messages in thread
From: Ulrich Hecht @ 2019-11-28  3:39 UTC (permalink / raw)
  To: Geert Uytterhoeven, Linus Walleij, Bartosz Golaszewski,
	Jonathan Corbet, Rob Herring, Mark Rutland, Harish Jenny K N,
	Eugeniu Rosca
  Cc: Alexander Graf, Peter Maydell, Paolo Bonzini, Phil Reid,
	Marc Zyngier, Christoffer Dall, Magnus Damm, linux-gpio,
	linux-doc, devicetree, linux-renesas-soc, linux-kernel,
	qemu-devel


> On November 27, 2019 at 9:42 AM Geert Uytterhoeven <geert+renesas@glider.be> wrote:
> 
> 
> Currently GPIOs can only be referred to by GPIO controller and offset in
> GPIO lookup tables.
> 
> Add support for looking them up by line name.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> If this is rejected, the GPIO Aggregator documentation and code must be
> updated.
> 
> v3:
>   - New.
> ---
>  drivers/gpio/gpiolib.c       | 12 ++++++++++++
>  include/linux/gpio/machine.h |  2 +-
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index d24a3d79dcfe69ad..cb608512ad6bbded 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -4475,6 +4475,18 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
>  		if (p->con_id && (!con_id || strcmp(p->con_id, con_id)))
>  			continue;
>  
> +		if (p->chip_hwnum == (u16)-1) {
> +			desc = gpio_name_to_desc(p->chip_label);
> +			if (desc) {
> +				*flags = p->flags;
> +				return desc;
> +			}
> +
> +			dev_warn(dev, "cannot find GPIO line %s, deferring\n",
> +				 p->chip_label);
> +			return ERR_PTR(-EPROBE_DEFER);
> +		}
> +
>  		chip = find_chip_by_name(p->chip_label);
>  
>  		if (!chip) {
> diff --git a/include/linux/gpio/machine.h b/include/linux/gpio/machine.h
> index 1ebe5be05d5f81fa..84c1c097e55eefaf 100644
> --- a/include/linux/gpio/machine.h
> +++ b/include/linux/gpio/machine.h
> @@ -31,7 +31,7 @@ enum gpio_lookup_flags {
>   */
>  struct gpiod_lookup {
>  	const char *chip_label;
> -	u16 chip_hwnum;
> +	u16 chip_hwnum;			/* if -1, chip_label is named line */

The magic number "-1" might deserve a #define.

>  	const char *con_id;
>  	unsigned int idx;
>  	unsigned long flags;
> -- 
> 2.17.1
>

With or without #define,
Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>

CU
Uli

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 4/7] dt-bindings: gpio: Add gpio-repeater bindings
  2019-11-27  8:42 ` [PATCH v3 4/7] dt-bindings: gpio: Add gpio-repeater bindings Geert Uytterhoeven
@ 2019-11-28  3:39   ` Ulrich Hecht
  2019-12-03  5:51   ` Harish Jenny K N
  2019-12-05 21:06   ` Rob Herring
  2 siblings, 0 replies; 48+ messages in thread
From: Ulrich Hecht @ 2019-11-28  3:39 UTC (permalink / raw)
  To: Geert Uytterhoeven, Linus Walleij, Bartosz Golaszewski,
	Jonathan Corbet, Rob Herring, Mark Rutland, Harish Jenny K N,
	Eugeniu Rosca
  Cc: Alexander Graf, Peter Maydell, Paolo Bonzini, Phil Reid,
	Marc Zyngier, Christoffer Dall, Magnus Damm, linux-gpio,
	linux-doc, devicetree, linux-renesas-soc, linux-kernel,
	qemu-devel


> On November 27, 2019 at 9:42 AM Geert Uytterhoeven <geert+renesas@glider.be> wrote:
> 
> 
> Add Device Tree bindings for a GPIO repeater, with optional translation
> of physical signal properties.  This is useful for describing explicitly
> the presence of e.g. an inverter on a GPIO line, and was inspired by the
> non-YAML gpio-inverter bindings by Harish Jenny K N
> <harish_kandiga@mentor.com>[1].
> 
> Note that this is different from a GPIO Nexus Node[2], which cannot do
> physical signal property translation.
> 
> While an inverter can be described implicitly by exchanging the
> GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW flags, this has its limitations.
> Each GPIO line has only a single GPIO_ACTIVE_* flag, but applies to both
> th provider and consumer sides:
>   1. The GPIO provider (controller) looks at the flags to know the
>      polarity, so it can translate between logical (active/not active)
>      and physical (high/low) signal levels.
>   2. While the signal polarity is usually fixed on the GPIO consumer
>      side (e.g. an LED is tied to either the supply voltage or GND),
>      it may be configurable on some devices, and both sides need to
>      agree.  Hence the GPIO_ACTIVE_* flag as seen by the consumer must
>      match the actual polarity.
>      There exists a similar issue with interrupt flags, where both the
>      interrupt controller and the device generating the interrupt need
>      to agree, which breaks in the presence of a physical inverter not
>      described in DT (see e.g. [3]).
> 
> [1] "[PATCH V4 2/2] gpio: inverter: document the inverter bindings"
>     https://lore.kernel.org/linux-gpio/1561699236-18620-3-git-send-email-harish_kandiga@mentor.com/
> 
> [2] Devicetree Specification v0.3-rc2, Section 2.5
>     https://github.com/devicetree-org/devicetree-specification/releases/tag/v0.3-rc2
> 
> [3] "[PATCH] wlcore/wl18xx: Add invert-irq OF property for physically
>     inverted IRQ"
>     https://lore.kernel.org/linux-renesas-soc/20190607172958.20745-1-erosca@de.adit-jv.com/
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v3:
>   - New.
> ---
>  .../bindings/gpio/gpio-repeater.yaml          | 53 +++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-repeater.yaml
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-repeater.yaml b/Documentation/devicetree/bindings/gpio/gpio-repeater.yaml
> new file mode 100644
> index 0000000000000000..efdee0c3be43f731
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-repeater.yaml
> @@ -0,0 +1,53 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/gpio-repeater.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: GPIO Repeater
> +
> +maintainers:
> +  - Harish Jenny K N <harish_kandiga@mentor.com>
> +  - Geert Uytterhoeven <geert+renesas@glider.be>
> +
> +description:
> +  This represents a repeater for one or more GPIOs, possibly including physical
> +  signal property translation (e.g. polarity inversion).
> +
> +properties:
> +  compatible:
> +    const: gpio-repeater
> +
> +  "#gpio-cells":
> +    const: 2
> +
> +  gpio-controller: true
> +
> +  gpios:
> +    description:
> +      Phandle and specifier, one for each repeated GPIO.
> +
> +  gpio-line-names:
> +    description:
> +      Strings defining the names of the GPIO lines going out of the GPIO
> +      controller.
> +
> +required:
> +  - compatible
> +  - "#gpio-cells"
> +  - gpio-controller
> +  - gpios
> +
> +additionalProperties: false
> +
> +examples:
> +  # Device node describing a polarity inverter for a single GPIO
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    inverter: gpio-repeater {
> +        compatible = "gpio-repeater";
> +        #gpio-cells = <2>;
> +        gpio-controller;
> +        gpios = <&gpio 95 GPIO_ACTIVE_LOW>;
> +    };
> -- 
> 2.17.1
>

Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>

CU
Uli

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 5/7] gpio: Add GPIO Aggregator/Repeater driver
  2019-11-27  8:42 ` [PATCH v3 5/7] gpio: Add GPIO Aggregator/Repeater driver Geert Uytterhoeven
  2019-11-27 14:15   ` Eugeniu Rosca
@ 2019-11-28  3:40   ` Ulrich Hecht
  2019-12-03  5:42   ` Harish Jenny K N
                     ` (2 subsequent siblings)
  4 siblings, 0 replies; 48+ messages in thread
From: Ulrich Hecht @ 2019-11-28  3:40 UTC (permalink / raw)
  To: Geert Uytterhoeven, Linus Walleij, Bartosz Golaszewski,
	Jonathan Corbet, Rob Herring, Mark Rutland, Harish Jenny K N,
	Eugeniu Rosca
  Cc: Alexander Graf, Peter Maydell, Paolo Bonzini, Phil Reid,
	Marc Zyngier, Christoffer Dall, Magnus Damm, linux-gpio,
	linux-doc, devicetree, linux-renesas-soc, linux-kernel,
	qemu-devel

Thank you for this series!

> On November 27, 2019 at 9:42 AM Geert Uytterhoeven <geert+renesas@glider.be> wrote:
> 
> 
> GPIO controllers are exported to userspace using /dev/gpiochip*
> character devices.  Access control to these devices is provided by
> standard UNIX file system permissions, on an all-or-nothing basis:
> either a GPIO controller is accessible for a user, or it is not.
> Currently no mechanism exists to control access to individual GPIOs.
> 
> Hence add a GPIO driver to aggregate existing GPIOs, and expose them as
> a new gpiochip.
> 
> This supports the following use cases:
>   1. Aggregating GPIOs using Sysfs
>      This is useful for implementing access control, and assigning a set
>      of GPIOs to a specific user or virtual machine.
> 
>   2. GPIO Repeater in Device Tree
>      This supports modelling e.g. GPIO inverters in DT.
> 
>   3. Generic GPIO Driver
>      This provides userspace access to a simple GPIO-operated device
>      described in DT, cfr. e.g. spidev for SPI-operated devices.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v3:
>   - Absorb GPIO forwarder,
>   - Integrate GPIO Repeater and Generic GPIO driver functionality,
>   - Use the aggregator parameters to create a GPIO lookup table instead
>     of an array of GPIO descriptors, which allows to simplify the code:
>       1. This removes the need for calling gpio_name_to_desc(),
>          gpiochip_find(), gpiochip_get_desc(), and gpiod_request(),
>       2. This allows the platform device to always use
>          devm_gpiod_get_index(), regardless of the origin of the GPIOs,
>   - Move parameter parsing from platform device probe to sysfs attribute
>     store, removing the need for platform data passing,
>   - Use more devm_*() functions to simplify cleanup,
>   - Add pr_fmt(),
>   - General refactoring.
> 
> v2:
>   - Add missing initialization of i in gpio_virt_agg_probe(),
>   - Update for removed .need_valid_mask field and changed
>     .init_valid_mask() signature,
>   - Drop "virtual", rename to gpio-aggregator,
>   - Drop bogus FIXME related to gpiod_set_transitory() expectations,
>   - Use new GPIO Forwarder Helper,
>   - Lift limit on the maximum number of GPIOs,
>   - Improve parsing:
>       - add support for specifying GPIOs by line name,
>       - add support for specifying GPIO chips by ID,
>       - add support for GPIO offset ranges,
>       - names and offset specifiers must be separated by whitespace,
>       - GPIO offsets must separated by spaces,
>   - Use str_has_prefix() and kstrtouint().
> ---
>  drivers/gpio/Kconfig           |  13 +
>  drivers/gpio/Makefile          |   1 +
>  drivers/gpio/gpio-aggregator.c | 587 +++++++++++++++++++++++++++++++++
>  3 files changed, 601 insertions(+)
>  create mode 100644 drivers/gpio/gpio-aggregator.c
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 8adffd42f8cb0559..36b6b57a6b05e906 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -1507,6 +1507,19 @@ config GPIO_VIPERBOARD
>  
>  endmenu
>  
> +config GPIO_AGGREGATOR
> +	tristate "GPIO Aggregator/Repeater"
> +	help
> +	  Say yes here to enable the GPIO Aggregator and repeater, which

Inconsistent capitalization (Aggregator vs. repeater).

> +	  provides a way to aggregate and/or repeat existing GPIOs into a new
> +	  GPIO device.
> +	  This can serve the following purposes:
> +	    1. Assign a collection of GPIOs to a user, or export them to a
> +	       virtual machine,
> +	    2. Support GPIOs that are connected to a physical inverter,
> +	    3. Provide a generic driver for a GPIO-operated device, to be
> +               controlled from userspace using the GPIO chardev interface.
> +
>  config GPIO_MOCKUP
>  	tristate "GPIO Testing Driver"
>  	select IRQ_SIM
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 34eb8b2b12dd656c..f9971eeb1f32335f 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -25,6 +25,7 @@ obj-$(CONFIG_GPIO_74XX_MMIO)		+= gpio-74xx-mmio.o
>  obj-$(CONFIG_GPIO_ADNP)			+= gpio-adnp.o
>  obj-$(CONFIG_GPIO_ADP5520)		+= gpio-adp5520.o
>  obj-$(CONFIG_GPIO_ADP5588)		+= gpio-adp5588.o
> +obj-$(CONFIG_GPIO_AGGREGATOR)		+= gpio-aggregator.o
>  obj-$(CONFIG_GPIO_ALTERA_A10SR)		+= gpio-altera-a10sr.o
>  obj-$(CONFIG_GPIO_ALTERA)  		+= gpio-altera.o
>  obj-$(CONFIG_GPIO_AMD8111)		+= gpio-amd8111.o
> diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
> new file mode 100644
> index 0000000000000000..873578c6f9683db8
> --- /dev/null
> +++ b/drivers/gpio/gpio-aggregator.c
> @@ -0,0 +1,587 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +//
> +// GPIO Aggregator and Repeater
> +//
> +// Copyright (C) 2019 Glider bvba
> +
> +#define DRV_NAME       "gpio-aggregator"
> +#define pr_fmt(fmt)	DRV_NAME ": " fmt
> +
> +#include <linux/bitmap.h>
> +#include <linux/bitops.h>
> +#include <linux/ctype.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/gpio/machine.h>
> +#include <linux/idr.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/overflow.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +#include <linux/string.h>
> +
> +#include "gpiolib.h"
> +
> +
> +	/*
> +	 * GPIO Aggregator sysfs interface
> +	 */

Should this comment really be indented?

> +
> +struct gpio_aggregator {
> +	struct gpiod_lookup_table *lookups;
> +	struct platform_device *pdev;
> +	char args[];
> +};
> +
> +static DEFINE_MUTEX(gpio_aggregator_lock);	/* protects idr */
> +static DEFINE_IDR(gpio_aggregator_idr);
> +
> +static char *get_arg(char **args)
> +{
> +	char *start = *args, *end;
> +
> +	while (isspace(*start))
> +		start++;

"start = skip_spaces(start);", perhaps. (Looking at a grep through drivers/, I would say usage of while(isspace(...))... vs. skip_spaces() is about 60/40.)

> +
> +	if (!*start)
> +		return NULL;
> +
> +	if (*start == '"') {
> +		/* Quoted arg */
> +		end = strchr(++start, '"');
> +		if (!end)
> +			return ERR_PTR(-EINVAL);
> +	} else {
> +		/* Unquoted arg */
> +		for (end = start; *end && !isspace(*end); end++) ;
> +	}
> +
> +	if (*end)
> +		*end++ = '\0';
> +
> +	*args = end;
> +	return start;
> +}
> +
> +static bool isrange(const char *s)
> +{
> +	size_t n = strlen(s);
> +
> +	if (IS_ERR_OR_NULL(s))
> +		return false;
> +
> +	while (1) {
> +		n = strspn(s, "0123456789");
> +		if (!n)
> +			return false;
> +
> +		s += n;
> +
> +		switch (*s++) {
> +		case '\0':
> +			return true;
> +
> +		case '-':
> +		case ',':
> +			break;
> +
> +		default:
> +			return false;
> +		}
> +	}
> +}
> +
> +static int aggr_add_gpio(struct gpio_aggregator *aggr, const char *label,
> +			 int hwnum, unsigned int *n)
> +{
> +	struct gpiod_lookup_table *lookups;
> +
> +	lookups = krealloc(aggr->lookups, struct_size(lookups, table, *n + 2),
> +			   GFP_KERNEL);
> +	if (!lookups)
> +		return -ENOMEM;
> +
> +	lookups->table[*n].chip_label = label;
> +	lookups->table[*n].chip_hwnum = hwnum;
> +	lookups->table[*n].idx = *n;
> +
> +	(*n)++;
> +	memset(&lookups->table[*n], 0, sizeof(lookups->table[*n]));
> +
> +	aggr->lookups = lookups;
> +	return 0;
> +}
> +
> +static int aggr_parse(struct gpio_aggregator *aggr)
> +{
> +	char *name, *offsets, *first, *last, *next;
> +	unsigned int a, b, i, n = 0;

Too many single-letter variables. "first_index" and "last_index" instead of "a" and "b" would make it easier to understand.

> +	char *args = aggr->args;
> +	int error;
> +
> +	for (name = get_arg(&args), offsets = get_arg(&args); name;
> +	     offsets = get_arg(&args)) {
> +		if (IS_ERR(name)) {
> +			pr_err("Cannot get GPIO specifier: %ld\n",
> +			       PTR_ERR(name));
> +			return PTR_ERR(name);
> +		}
> +
> +		if (!isrange(offsets)) {
> +			/* Named GPIO line */
> +			error = aggr_add_gpio(aggr, name, -1, &n);

The "-1" magic could use a #define, IMO.

> +			if (error)
> +				return error;
> +
> +			name = offsets;
> +			continue;
> +		}
> +
> +		/* GPIO chip + offset(s) */
> +		for (first = offsets; *first; first = next) {
> +			next = strchrnul(first, ',');
> +			if (*next)
> +				*next++ = '\0';
> +
> +			last = strchr(first, '-');
> +			if (last)
> +				*last++ = '\0';
> +
> +			if (kstrtouint(first, 10, &a)) {
> +				pr_err("Cannot parse GPIO index %s\n", first);
> +				return -EINVAL;
> +			}
> +
> +			if (!last) {
> +				b = a;
> +			} else if (kstrtouint(last, 10, &b)) {
> +				pr_err("Cannot parse GPIO index %s\n", last);
> +				return -EINVAL;
> +			}
> +
> +			for (i = a; i <= b; i++) {
> +				error = aggr_add_gpio(aggr, name, i, &n);
> +				if (error)
> +					return error;
> +			}
> +		}
> +
> +		name = get_arg(&args);
> +	}
> +
> +	if (!n) {
> +		pr_err("No GPIOs specified\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static ssize_t new_device_store(struct device_driver *driver, const char *buf,
> +				size_t count)
> +{
> +	struct gpio_aggregator *aggr;
> +	struct platform_device *pdev;
> +	int res, id;
> +
> +	/* kernfs guarantees string termination, so count + 1 is safe */
> +	aggr = kzalloc(sizeof(*aggr) + count + 1, GFP_KERNEL);
> +	if (!aggr)
> +		return -ENOMEM;
> +
> +	memcpy(aggr->args, buf, count + 1);
> +
> +	aggr->lookups = kzalloc(struct_size(aggr->lookups, table, 1),
> +				GFP_KERNEL);
> +	if (!aggr->lookups) {
> +		res = -ENOMEM;
> +		goto free_ga;
> +	}
> +
> +	mutex_lock(&gpio_aggregator_lock);
> +	id = idr_alloc(&gpio_aggregator_idr, aggr, 0, 0, GFP_KERNEL);
> +	mutex_unlock(&gpio_aggregator_lock);
> +
> +	if (id < 0) {
> +		res = id;
> +		goto free_table;
> +	}
> +
> +	aggr->lookups->dev_id = kasprintf(GFP_KERNEL, "%s.%d", DRV_NAME, id);
> +	if (!aggr->lookups) {
> +		res = -ENOMEM;
> +		goto remove_idr;
> +	}
> +
> +	res = aggr_parse(aggr);
> +	if (res)
> +		goto free_dev_id;
> +
> +	gpiod_add_lookup_table(aggr->lookups);
> +
> +	pdev = platform_device_register_simple(DRV_NAME, id, NULL, 0);
> +	if (IS_ERR(pdev)) {
> +		res = PTR_ERR(pdev);
> +		goto remove_table;
> +	}
> +
> +	aggr->pdev = pdev;
> +	return count;
> +
> +remove_table:
> +	gpiod_remove_lookup_table(aggr->lookups);
> +free_dev_id:
> +	kfree(aggr->lookups->dev_id);
> +remove_idr:
> +	mutex_lock(&gpio_aggregator_lock);
> +	idr_remove(&gpio_aggregator_idr, id);
> +	mutex_unlock(&gpio_aggregator_lock);
> +free_table:
> +	kfree(aggr->lookups);
> +free_ga:
> +	kfree(aggr);
> +	return res;
> +}
> +
> +static DRIVER_ATTR_WO(new_device);
> +
> +static void gpio_aggregator_free(struct gpio_aggregator *aggr)
> +{
> +	platform_device_unregister(aggr->pdev);
> +	gpiod_remove_lookup_table(aggr->lookups);
> +	kfree(aggr->lookups->dev_id);
> +	kfree(aggr->lookups);
> +	kfree(aggr);
> +}
> +
> +static ssize_t delete_device_store(struct device_driver *driver,
> +				   const char *buf, size_t count)
> +{
> +	struct gpio_aggregator *aggr;
> +	unsigned int id;
> +	int error;
> +
> +	if (!str_has_prefix(buf, DRV_NAME "."))
> +		return -EINVAL;
> +
> +	error = kstrtouint(buf + strlen(DRV_NAME "."), 10, &id);
> +	if (error)
> +		return error;
> +
> +	mutex_lock(&gpio_aggregator_lock);
> +	aggr = idr_remove(&gpio_aggregator_idr, id);
> +	mutex_unlock(&gpio_aggregator_lock);
> +	if (!aggr)
> +		return -ENOENT;
> +
> +	gpio_aggregator_free(aggr);
> +	return count;
> +}
> +static DRIVER_ATTR_WO(delete_device);
> +
> +static struct attribute *gpio_aggregator_attrs[] = {
> +	&driver_attr_new_device.attr,
> +	&driver_attr_delete_device.attr,
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(gpio_aggregator);
> +
> +static int __exit gpio_aggregator_idr_remove(int id, void *p, void *data)
> +{
> +	gpio_aggregator_free(p);
> +	return 0;
> +}
> +
> +static void __exit gpio_aggregator_remove_all(void)
> +{
> +	mutex_lock(&gpio_aggregator_lock);
> +	idr_for_each(&gpio_aggregator_idr, gpio_aggregator_idr_remove, NULL);
> +	idr_destroy(&gpio_aggregator_idr);
> +	mutex_unlock(&gpio_aggregator_lock);
> +}
> +
> +
> +	/*
> +	 *  Common GPIO Forwarder
> +	 */
> +
> +struct gpiochip_fwd {
> +	struct gpio_chip chip;
> +	struct gpio_desc **descs;
> +	union {
> +		struct mutex mlock;	/* protects tmp[] if can_sleep */
> +		spinlock_t slock;	/* protects tmp[] if !can_sleep */
> +	};
> +	unsigned long tmp[];		/* values and descs for multiple ops */
> +};
> +
> +static int gpio_fwd_get_direction(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> +
> +	return gpiod_get_direction(fwd->descs[offset]);
> +}
> +
> +static int gpio_fwd_direction_input(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> +
> +	return gpiod_direction_input(fwd->descs[offset]);
> +}
> +
> +static int gpio_fwd_direction_output(struct gpio_chip *chip,
> +				     unsigned int offset, int value)
> +{
> +	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> +
> +	return gpiod_direction_output(fwd->descs[offset], value);
> +}
> +
> +static int gpio_fwd_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> +
> +	return gpiod_get_value(fwd->descs[offset]);
> +}
> +
> +static int gpio_fwd_get_multiple(struct gpio_chip *chip, unsigned long *mask,
> +				 unsigned long *bits)
> +{
> +	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> +	unsigned long *values, flags;
> +	struct gpio_desc **descs;
> +	unsigned int i, j = 0;
> +	int error;
> +
> +	if (chip->can_sleep)
> +		mutex_lock(&fwd->mlock);
> +	else
> +		spin_lock_irqsave(&fwd->slock, flags);
> +
> +	values = &fwd->tmp[0];
> +	bitmap_clear(values, 0, fwd->chip.ngpio);
> +	descs = (void *)&fwd->tmp[BITS_TO_LONGS(fwd->chip.ngpio)];

This use of the tmp array, which I presume serves to avoid extra memory allocations at the time of device creation, is odd enough to deserve a comment.

> +
> +	for_each_set_bit(i, mask, fwd->chip.ngpio)
> +		descs[j++] = fwd->descs[i];
> +
> +	error = gpiod_get_array_value(j, descs, NULL, values);
> +	if (!error) {
> +		j = 0;
> +		for_each_set_bit(i, mask, fwd->chip.ngpio)
> +			__assign_bit(i, bits, test_bit(j++, values));
> +	}
> +
> +	if (chip->can_sleep)
> +		mutex_unlock(&fwd->mlock);
> +	else
> +		spin_unlock_irqrestore(&fwd->slock, flags);
> +
> +	return error;
> +}
> +
> +static void gpio_fwd_set(struct gpio_chip *chip, unsigned int offset, int value)
> +{
> +	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> +
> +	gpiod_set_value(fwd->descs[offset], value);
> +}
> +
> +static void gpio_fwd_set_multiple(struct gpio_chip *chip, unsigned long *mask,
> +				  unsigned long *bits)
> +{
> +	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> +	unsigned long *values, flags;
> +	struct gpio_desc **descs;
> +	unsigned int i, j = 0;
> +
> +	if (chip->can_sleep)
> +		mutex_lock(&fwd->mlock);
> +	else
> +		spin_lock_irqsave(&fwd->slock, flags);
> +
> +	values = &fwd->tmp[0];
> +	descs = (void *)&fwd->tmp[BITS_TO_LONGS(fwd->chip.ngpio)];
> +
> +	for_each_set_bit(i, mask, fwd->chip.ngpio) {
> +		__assign_bit(j, values, test_bit(i, bits));
> +		descs[j++] = fwd->descs[i];
> +	}
> +
> +	gpiod_set_array_value(j, descs, NULL, values);
> +
> +	if (chip->can_sleep)
> +		mutex_unlock(&fwd->mlock);
> +	else
> +		spin_unlock_irqrestore(&fwd->slock, flags);
> +}
> +
> +static int gpio_fwd_set_config(struct gpio_chip *chip, unsigned int offset,
> +			       unsigned long config)
> +{
> +	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> +
> +	chip = fwd->descs[offset]->gdev->chip;
> +	if (chip->set_config)
> +		return chip->set_config(chip, offset, config);
> +
> +	return -ENOTSUPP;
> +}
> +
> +static int gpio_fwd_init_valid_mask(struct gpio_chip *chip,
> +				    unsigned long *valid_mask,
> +				    unsigned int ngpios)
> +{
> +	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> +	unsigned int i;
> +
> +	for (i = 0; i < ngpios; i++) {
> +		if (!gpiochip_line_is_valid(fwd->descs[i]->gdev->chip,
> +					    gpio_chip_hwgpio(fwd->descs[i])))
> +			clear_bit(i, valid_mask);
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * gpiochip_fwd_create() - Create a new GPIO forwarder
> + * @dev: Parent device pointer
> + * @ngpios: Number of GPIOs in the forwarder.
> + * @descs: Array containing the GPIO descriptors to forward to.
> + *         This array must contain @ngpios entries, and must not be deallocated
> + *         before the forwarder has been destroyed again.
> + *
> + * This function creates a new gpiochip, which forwards all GPIO operations to
> + * the passed GPIO descriptors.
> + *
> + * Return: An opaque object pointer, or an ERR_PTR()-encoded negative error
> + *         code on failure.
> + */
> +static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
> +						unsigned int ngpios,
> +						struct gpio_desc *descs[])
> +{
> +	const char *label = dev_name(dev);
> +	struct gpiochip_fwd *fwd;
> +	struct gpio_chip *chip;
> +	unsigned int i;
> +	int error;
> +
> +	fwd = devm_kzalloc(dev, struct_size(fwd, tmp,
> +			   BITS_TO_LONGS(ngpios) + ngpios), GFP_KERNEL);
> +	if (!fwd)
> +		return ERR_PTR(-ENOMEM);
> +
> +	chip = &fwd->chip;
> +
> +	for (i = 0; i < ngpios; i++) {
> +		dev_dbg(dev, "gpio %u => gpio-%d (%s)\n", i,
> +			desc_to_gpio(descs[i]), descs[i]->label ? : "?");
> +
> +		if (gpiod_cansleep(descs[i]))
> +			chip->can_sleep = true;
> +		if (descs[i]->gdev->chip->set_config)
> +			chip->set_config = gpio_fwd_set_config;
> +		if (descs[i]->gdev->chip->init_valid_mask)
> +			chip->init_valid_mask = gpio_fwd_init_valid_mask;
> +	}
> +
> +	chip->label = label;
> +	chip->parent = dev;
> +	chip->owner = THIS_MODULE;
> +	chip->get_direction = gpio_fwd_get_direction;
> +	chip->direction_input = gpio_fwd_direction_input;
> +	chip->direction_output = gpio_fwd_direction_output;
> +	chip->get = gpio_fwd_get;
> +	chip->get_multiple = gpio_fwd_get_multiple;
> +	chip->set = gpio_fwd_set;
> +	chip->set_multiple = gpio_fwd_set_multiple;
> +	chip->base = -1;
> +	chip->ngpio = ngpios;
> +	fwd->descs = descs;
> +
> +	if (chip->can_sleep)
> +		mutex_init(&fwd->mlock);
> +	else
> +		spin_lock_init(&fwd->slock);
> +
> +	error = devm_gpiochip_add_data(dev, chip, fwd);
> +	if (error)
> +		return ERR_PTR(error);
> +
> +	return fwd;
> +}
> +
> +
> +	/*
> +	 *  Common GPIO Aggregator/Repeater platform device
> +	 */
> +
> +static int gpio_aggregator_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct gpio_desc **descs;
> +	struct gpiochip_fwd *fwd;
> +	int i, n;
> +
> +	n = gpiod_count(dev, NULL);
> +	if (n < 0)
> +		return n;
> +
> +	descs = devm_kmalloc_array(dev, n, sizeof(*descs), GFP_KERNEL);
> +	if (!descs)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < n; i++) {
> +		descs[i] = devm_gpiod_get_index(dev, NULL, i, GPIOD_ASIS);
> +		if (IS_ERR(descs[i]))
> +			return PTR_ERR(descs[i]);
> +	}
> +
> +	fwd = gpiochip_fwd_create(dev, n, descs);
> +	if (IS_ERR(fwd))
> +		return PTR_ERR(fwd);
> +
> +	platform_set_drvdata(pdev, fwd);
> +	return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id gpio_aggregator_dt_ids[] = {
> +	{ .compatible = "gpio-repeater" },
> +	/*
> +	 * Add GPIO-operated devices controlled from userspace below,
> +	 * or use "driver_override" in sysfs
> +	 */
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, gpio_aggregator_dt_ids);
> +#endif
> +
> +static struct platform_driver gpio_aggregator_driver = {
> +	.probe = gpio_aggregator_probe,
> +	.driver = {
> +		.name = DRV_NAME,
> +		.groups = gpio_aggregator_groups,
> +		.of_match_table = of_match_ptr(gpio_aggregator_dt_ids),
> +	},
> +};
> +
> +static int __init gpio_aggregator_init(void)
> +{
> +	return platform_driver_register(&gpio_aggregator_driver);
> +}
> +module_init(gpio_aggregator_init);
> +
> +static void __exit gpio_aggregator_exit(void)
> +{
> +	gpio_aggregator_remove_all();
> +	platform_driver_unregister(&gpio_aggregator_driver);
> +}
> +module_exit(gpio_aggregator_exit);
> +
> +MODULE_AUTHOR("Geert Uytterhoeven <geert+renesas@glider.be>");
> +MODULE_DESCRIPTION("GPIO Aggregator/Repeater");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.17.1
>

CU
Uli

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 6/7] docs: gpio: Add GPIO Aggregator/Repeater documentation
  2019-11-27  8:42 ` [PATCH v3 6/7] docs: gpio: Add GPIO Aggregator/Repeater documentation Geert Uytterhoeven
@ 2019-11-28  3:41   ` Ulrich Hecht
  2019-12-12 14:42   ` Linus Walleij
  1 sibling, 0 replies; 48+ messages in thread
From: Ulrich Hecht @ 2019-11-28  3:41 UTC (permalink / raw)
  To: Geert Uytterhoeven, Linus Walleij, Bartosz Golaszewski,
	Jonathan Corbet, Rob Herring, Mark Rutland, Harish Jenny K N,
	Eugeniu Rosca
  Cc: Alexander Graf, Peter Maydell, Paolo Bonzini, Phil Reid,
	Marc Zyngier, Christoffer Dall, Magnus Damm, linux-gpio,
	linux-doc, devicetree, linux-renesas-soc, linux-kernel,
	qemu-devel


> On November 27, 2019 at 9:42 AM Geert Uytterhoeven <geert+renesas@glider.be> wrote:
> 
> 
> Document the GPIO Aggregator/Repeater, and the three typical use-cases.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v3:
>   - New.
> ---
>  .../admin-guide/gpio/gpio-aggregator.rst      | 111 ++++++++++++++++++
>  Documentation/admin-guide/gpio/index.rst      |   1 +
>  2 files changed, 112 insertions(+)
>  create mode 100644 Documentation/admin-guide/gpio/gpio-aggregator.rst
> 
> diff --git a/Documentation/admin-guide/gpio/gpio-aggregator.rst b/Documentation/admin-guide/gpio/gpio-aggregator.rst
> new file mode 100644
> index 0000000000000000..826146e260253299
> --- /dev/null
> +++ b/Documentation/admin-guide/gpio/gpio-aggregator.rst
> @@ -0,0 +1,111 @@
> +.. SPDX-License-Identifier: GPL-2.0-only
> +
> +GPIO Aggregator/Repeater
> +========================
> +
> +The GPIO Aggregator/Repeater allows to aggregate GPIOs, and expose them as a
> +new gpio_chip.  This supports the following use cases.
> +
> +
> +Aggregating GPIOs using Sysfs
> +-----------------------------
> +
> +GPIO controllers are exported to userspace using /dev/gpiochip* character
> +devices.  Access control to these devices is provided by standard UNIX file
> +system permissions, on an all-or-nothing basis: either a GPIO controller is
> +accessible for a user, or it is not.
> +
> +The GPIO Aggregator allows access control for individual GPIOs, by aggregating
> +them into a new gpio_chip, which can be assigned to a group or user using
> +standard UNIX file ownership and permissions.  Furthermore, this simplifies and
> +hardens exporting GPIOs to a virtual machine, as the VM can just grab the full
> +GPIO controller, and no longer needs to care about which GPIOs to grab and
> +which not, reducing the attack surface.
> +
> +Aggregated GPIO controllers are instantiated and destroyed by writing to
> +write-only attribute files in sysfs.
> +
> +    /sys/bus/platform/drivers/gpio-aggregator/
> +
> +	"new_device" ...
> +		Userspace may ask the kernel to instantiate an aggregated GPIO
> +		controller by writing a string describing the GPIOs to
> +		aggregate to the "new_device" file, using the format
> +
> +		.. code-block:: none
> +
> +		    [<gpioA>] [<gpiochipB> <offsets>] ...
> +
> +		Where:
> +
> +		    "<gpioA>" ...
> +			    is a GPIO line name,
> +
> +		    "<gpiochipB>" ...
> +			    is a GPIO chip label or name, and
> +
> +		    "<offsets>" ...
> +			    is a comma-separated list of GPIO offsets and/or
> +			    GPIO offset ranges denoted by dashes.
> +
> +		Example: Instantiate a new GPIO aggregator by aggregating GPIO
> +		19 of "e6052000.gpio" and GPIOs 20-21 of "gpiochip2" into a new
> +		gpio_chip:
> +
> +		.. code-block:: bash
> +
> +		    echo 'e6052000.gpio 19 gpiochip2 20-21' > new_device
> +
> +	"delete_device" ...
> +		Userspace may ask the kernel to destroy an aggregated GPIO
> +		controller after use by writing its device name to the
> +		"delete_device" file.
> +
> +		Example: Destroy the previously-created aggregated GPIO
> +		controller "gpio-aggregator.0":
> +
> +		.. code-block:: bash
> +
> +		    echo gpio-aggregator.0 > delete_device
> +
> +
> +GPIO Repeater in Device Tree
> +----------------------------
> +
> +A GPIO Repeater is a node in a Device Tree representing a repeater for one or
> +more GPIOs, possibly including physical signal property translation (e.g.
> +polarity inversion).  This allows to model e.g. inverters in DT.
> +
> +See Documentation/devicetree/bindings/gpio/gpio-repeater.yaml
> +
> +
> +Generic GPIO Driver
> +-------------------
> +
> +The GPIO Aggregator can also be used as a generic driver for a simple
> +GPIO-operated device described in DT, without a dedicated in-kernel driver.
> +This is not unlike e.g. spidev, which allows to communicated with an SPI device
> +from userspace.
> +
> +Binding a device to the GPIO Aggregator is performed either by modifying the
> +gpio-aggregator driver, or by writing to the "driver_override" file in Sysfs.
> +
> +Example: If "frobnicator" is a GPIO-operated device described in DT, using its
> +own compatible value::
> +
> +        frobnicator {
> +                compatible = "myvendor,frobnicator";
> +
> +                gpios = <&gpio2 19 GPIO_ACTIVE_HIGH>,
> +                        <&gpio2 20 GPIO_ACTIVE_LOW>;
> +        };
> +
> +it can be bound to the GPIO Aggregator by either:
> +
> +1. Adding its compatible value to ``gpio_aggregator_dt_ids[]``,
> +2. Binding manually using "driver_override":
> +
> +.. code-block:: bash
> +
> +    echo gpio-aggregator > /sys/bus/platform/devices/frobnicator/driver_override
> +    echo frobnicator > /sys/bus/platform/drivers/gpio-aggregator/bind
> diff --git a/Documentation/admin-guide/gpio/index.rst b/Documentation/admin-guide/gpio/index.rst
> index a244ba4e87d5398a..ef2838638e967777 100644
> --- a/Documentation/admin-guide/gpio/index.rst
> +++ b/Documentation/admin-guide/gpio/index.rst
> @@ -7,6 +7,7 @@ gpio
>  .. toctree::
>      :maxdepth: 1
>  
> +    gpio-aggregator
>      sysfs
>  
>  .. only::  subproject and html
> -- 
> 2.17.1
>

Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>

CU
Uli

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 1/7] gpiolib: Add GPIOCHIP_NAME definition
  2019-11-27  8:42 ` [PATCH v3 1/7] gpiolib: Add GPIOCHIP_NAME definition Geert Uytterhoeven
  2019-11-28  3:38   ` Ulrich Hecht
@ 2019-12-02 21:17   ` Eugeniu Rosca
  2019-12-12 10:37   ` Linus Walleij
  2 siblings, 0 replies; 48+ messages in thread
From: Eugeniu Rosca @ 2019-12-02 21:17 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Linus Walleij, Bartosz Golaszewski, Jonathan Corbet, Rob Herring,
	Mark Rutland, Harish Jenny K N, Eugeniu Rosca, Alexander Graf,
	Peter Maydell, Paolo Bonzini, Phil Reid, Marc Zyngier,
	Christoffer Dall, Magnus Damm, linux-gpio, linux-doc, devicetree,
	linux-renesas-soc, linux-kernel, qemu-devel, Eugeniu Rosca

Hi Geert,

On Wed, Nov 27, 2019 at 09:42:47AM +0100, Geert Uytterhoeven wrote:
> The string literal "gpiochip" is used in several places.
> Add a definition for it, and use it everywhere, to make sure everything
> stays in sync.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v3:
>   - New.
> ---
>  drivers/gpio/gpiolib-sysfs.c | 7 +++----
>  drivers/gpio/gpiolib.c       | 4 ++--
>  drivers/gpio/gpiolib.h       | 2 ++
>  3 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
> index ca9bc1e4803c2979..a4a759920faa48ab 100644
> --- a/drivers/gpio/gpiolib.h
> +++ b/drivers/gpio/gpiolib.h
> @@ -16,6 +16,8 @@
>  #include <linux/module.h>
>  #include <linux/cdev.h>
>  
> +#define GPIOCHIP_NAME	"gpiochip"

[.02$/nit] I wonder if GPIOCHIP_NAME is actually an essential
part of kernel-user contract [1-2], in which case it could
probably be moved to include/uapi/linux/gpio.h ?

Regardless:
Reviewed-by: Eugeniu Rosca <erosca@de.adit-jv.com>

[1] linux (v5.4) git grep '"gpiochip' -- tools/
tools/gpio/lsgpio.c:                    if (check_prefix(ent->d_name, "gpiochip")) {
tools/testing/selftests/gpio/gpio-mockup-chardev.c:             if (check_prefix(ent->d_name, "gpiochip")) {

[2] libgpiod (v1.4-76-g00418dfdfc8b) git grep '"gpiochip'
lib/iter.c:	return !strncmp(dir->d_name, "gpiochip", 8);
tests/mockup/gpio-mockup.c:	rv = sscanf(sysname, "gpiochip%u", &chip->num);
tools/gpioget.c:		die("gpiochip must be specified");
tools/gpiomon.c:		die("gpiochip must be specified");
tools/gpioset.c:		die("gpiochip must be specified");

-- 
Best Regards,
Eugeniu

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 7/7] MAINTAINERS: Add GPIO Aggregator/Repeater section
  2019-11-27  8:42 ` [PATCH v3 7/7] MAINTAINERS: Add GPIO Aggregator/Repeater section Geert Uytterhoeven
@ 2019-12-03  5:38   ` Harish Jenny K N
  0 siblings, 0 replies; 48+ messages in thread
From: Harish Jenny K N @ 2019-12-03  5:38 UTC (permalink / raw)
  To: Geert Uytterhoeven, Linus Walleij, Bartosz Golaszewski,
	Jonathan Corbet, Rob Herring, Mark Rutland, Eugeniu Rosca
  Cc: Alexander Graf, Peter Maydell, Paolo Bonzini, Phil Reid,
	Marc Zyngier, Christoffer Dall, Magnus Damm, linux-gpio,
	linux-doc, devicetree, linux-renesas-soc, linux-kernel,
	qemu-devel


On 27/11/19 2:12 PM, Geert Uytterhoeven wrote:
> Add a maintainership section for the GPIO Aggregator/Repeater, covering
> documentation, Device Tree bindings, and driver source code.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Harish: Do you want to be listed as maintainer, too?


Yes. please.

>
> v3:
>   - New.
> ---
>  MAINTAINERS | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e5949b6827b72f2b..0f12ebdaa8faa76b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7043,6 +7043,14 @@ S:	Maintained
>  F:	Documentation/firmware-guide/acpi/gpio-properties.rst
>  F:	drivers/gpio/gpiolib-acpi.c
>  
> +GPIO AGGREGATOR/REPEATER
> +M:	Geert Uytterhoeven <geert+renesas@glider.be>
> +L:	linux-gpio@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/admin-guide/gpio/gpio-aggregator.rst
> +F:	Documentation/devicetree/bindings/gpio/gpio-repeater.yaml
> +F:	drivers/gpio/gpio-aggregator.c
> +
>  GPIO IR Transmitter
>  M:	Sean Young <sean@mess.org>
>  L:	linux-media@vger.kernel.org

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 5/7] gpio: Add GPIO Aggregator/Repeater driver
  2019-11-27  8:42 ` [PATCH v3 5/7] gpio: Add GPIO Aggregator/Repeater driver Geert Uytterhoeven
  2019-11-27 14:15   ` Eugeniu Rosca
  2019-11-28  3:40   ` Ulrich Hecht
@ 2019-12-03  5:42   ` Harish Jenny K N
  2019-12-03  8:17     ` Geert Uytterhoeven
  2019-12-03 10:51   ` Eugeniu Rosca
  2019-12-12 14:34   ` Linus Walleij
  4 siblings, 1 reply; 48+ messages in thread
From: Harish Jenny K N @ 2019-12-03  5:42 UTC (permalink / raw)
  To: Geert Uytterhoeven, Linus Walleij, Bartosz Golaszewski,
	Jonathan Corbet, Rob Herring, Mark Rutland, Eugeniu Rosca
  Cc: Alexander Graf, Peter Maydell, Paolo Bonzini, Phil Reid,
	Marc Zyngier, Christoffer Dall, Magnus Damm, linux-gpio,
	linux-doc, devicetree, linux-renesas-soc, linux-kernel,
	qemu-devel


> +static int gpio_aggregator_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct gpio_desc **descs;
> +	struct gpiochip_fwd *fwd;
> +	int i, n;
> +
> +	n = gpiod_count(dev, NULL);
> +	if (n < 0)
> +		return n;
> +
> +	descs = devm_kmalloc_array(dev, n, sizeof(*descs), GFP_KERNEL);
> +	if (!descs)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < n; i++) {
> +		descs[i] = devm_gpiod_get_index(dev, NULL, i, GPIOD_ASIS);

can you please add this check as well as we need to return EPROBE_DEFER.

if (desc[i] == ERR_PTR(-ENOENT))
<                 return -EPROBE_DEFER;


> +		if (IS_ERR(descs[i]))
> +			return PTR_ERR(descs[i]);
> +	}
> +
> +	fwd = gpiochip_fwd_create(dev, n, descs);
> +	if (IS_ERR(fwd))
> +		return PTR_ERR(fwd);
> +
> +	platform_set_drvdata(pdev, fwd);
> +	return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id gpio_aggregator_dt_ids[] = {
> +	{ .compatible = "gpio-repeater" },
> +	/*
> +	 * Add GPIO-operated devices controlled from userspace below,
> +	 * or use "driver_override" in sysfs
> +	 */
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, gpio_aggregator_dt_ids);
> +#endif
> +
> +static struct platform_driver gpio_aggregator_driver = {
> +	.probe = gpio_aggregator_probe,
> +	.driver = {
> +		.name = DRV_NAME,
> +		.groups = gpio_aggregator_groups,
> +		.of_match_table = of_match_ptr(gpio_aggregator_dt_ids),
> +	},
> +};
> +
> +static int __init gpio_aggregator_init(void)
> +{
> +	return platform_driver_register(&gpio_aggregator_driver);
> +}
> +module_init(gpio_aggregator_init);
> +
> +static void __exit gpio_aggregator_exit(void)
> +{
> +	gpio_aggregator_remove_all();
> +	platform_driver_unregister(&gpio_aggregator_driver);
> +}
> +module_exit(gpio_aggregator_exit);
> +
> +MODULE_AUTHOR("Geert Uytterhoeven <geert+renesas@glider.be>");
> +MODULE_DESCRIPTION("GPIO Aggregator/Repeater");
> +MODULE_LICENSE("GPL v2");




^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 4/7] dt-bindings: gpio: Add gpio-repeater bindings
  2019-11-27  8:42 ` [PATCH v3 4/7] dt-bindings: gpio: Add gpio-repeater bindings Geert Uytterhoeven
  2019-11-28  3:39   ` Ulrich Hecht
@ 2019-12-03  5:51   ` Harish Jenny K N
  2019-12-05 21:06   ` Rob Herring
  2 siblings, 0 replies; 48+ messages in thread
From: Harish Jenny K N @ 2019-12-03  5:51 UTC (permalink / raw)
  To: Geert Uytterhoeven, Linus Walleij, Bartosz Golaszewski,
	Jonathan Corbet, Rob Herring, Mark Rutland, Eugeniu Rosca
  Cc: Alexander Graf, Peter Maydell, Paolo Bonzini, Phil Reid,
	Marc Zyngier, Christoffer Dall, Magnus Damm, linux-gpio,
	linux-doc, devicetree, linux-renesas-soc, linux-kernel,
	qemu-devel


On 27/11/19 2:12 PM, Geert Uytterhoeven wrote:
> Add Device Tree bindings for a GPIO repeater, with optional translation
> of physical signal properties.  This is useful for describing explicitly
> the presence of e.g. an inverter on a GPIO line, and was inspired by the
> non-YAML gpio-inverter bindings by Harish Jenny K N
> <harish_kandiga@mentor.com>[1].
>
> Note that this is different from a GPIO Nexus Node[2], which cannot do
> physical signal property translation.
>
> While an inverter can be described implicitly by exchanging the
> GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW flags, this has its limitations.
> Each GPIO line has only a single GPIO_ACTIVE_* flag, but applies to both
> th provider and consumer sides:
>   1. The GPIO provider (controller) looks at the flags to know the
>      polarity, so it can translate between logical (active/not active)
>      and physical (high/low) signal levels.
>   2. While the signal polarity is usually fixed on the GPIO consumer
>      side (e.g. an LED is tied to either the supply voltage or GND),
>      it may be configurable on some devices, and both sides need to
>      agree.  Hence the GPIO_ACTIVE_* flag as seen by the consumer must
>      match the actual polarity.
>      There exists a similar issue with interrupt flags, where both the
>      interrupt controller and the device generating the interrupt need
>      to agree, which breaks in the presence of a physical inverter not
>      described in DT (see e.g. [3]).
>
> [1] "[PATCH V4 2/2] gpio: inverter: document the inverter bindings"
>     https://lore.kernel.org/linux-gpio/1561699236-18620-3-git-send-email-harish_kandiga@mentor.com/
>
> [2] Devicetree Specification v0.3-rc2, Section 2.5
>     https://github.com/devicetree-org/devicetree-specification/releases/tag/v0.3-rc2
>
> [3] "[PATCH] wlcore/wl18xx: Add invert-irq OF property for physically
>     inverted IRQ"
>     https://lore.kernel.org/linux-renesas-soc/20190607172958.20745-1-erosca@de.adit-jv.com/
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v3:
>   - New.
> ---
>  .../bindings/gpio/gpio-repeater.yaml          | 53 +++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-repeater.yaml
>
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-repeater.yaml b/Documentation/devicetree/bindings/gpio/gpio-repeater.yaml
> new file mode 100644
> index 0000000000000000..efdee0c3be43f731
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-repeater.yaml
> @@ -0,0 +1,53 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/gpio-repeater.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: GPIO Repeater
> +
> +maintainers:
> +  - Harish Jenny K N <harish_kandiga@mentor.com>
> +  - Geert Uytterhoeven <geert+renesas@glider.be>
> +
> +description:
> +  This represents a repeater for one or more GPIOs, possibly including physical
> +  signal property translation (e.g. polarity inversion).
> +
> +properties:
> +  compatible:
> +    const: gpio-repeater
> +
> +  "#gpio-cells":
> +    const: 2
> +
> +  gpio-controller: true
> +
> +  gpios:
> +    description:
> +      Phandle and specifier, one for each repeated GPIO.
> +
> +  gpio-line-names:
> +    description:
> +      Strings defining the names of the GPIO lines going out of the GPIO
> +      controller.
> +
> +required:
> +  - compatible
> +  - "#gpio-cells"
> +  - gpio-controller
> +  - gpios
> +
> +additionalProperties: false
> +
> +examples:
> +  # Device node describing a polarity inverter for a single GPIO
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    inverter: gpio-repeater {
> +        compatible = "gpio-repeater";
> +        #gpio-cells = <2>;
> +        gpio-controller;
> +        gpios = <&gpio 95 GPIO_ACTIVE_LOW>;
> +    };


just a suggestion: giving a gpio-line-names in the example would look useful.


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 5/7] gpio: Add GPIO Aggregator/Repeater driver
  2019-12-03  5:42   ` Harish Jenny K N
@ 2019-12-03  8:17     ` Geert Uytterhoeven
  2019-12-03  8:51       ` Harish Jenny K N
  0 siblings, 1 reply; 48+ messages in thread
From: Geert Uytterhoeven @ 2019-12-03  8:17 UTC (permalink / raw)
  To: Harish Jenny K N
  Cc: Geert Uytterhoeven, Linus Walleij, Bartosz Golaszewski,
	Jonathan Corbet, Rob Herring, Mark Rutland, Eugeniu Rosca,
	Alexander Graf, Peter Maydell, Paolo Bonzini, Phil Reid,
	Marc Zyngier, Christoffer Dall, Magnus Damm,
	open list:GPIO SUBSYSTEM, open list:DOCUMENTATION,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, Linux Kernel Mailing List, QEMU Developers

Hi Harish,

On Tue, Dec 3, 2019 at 6:42 AM Harish Jenny K N
<harish_kandiga@mentor.com> wrote:
> > +static int gpio_aggregator_probe(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct gpio_desc **descs;
> > +     struct gpiochip_fwd *fwd;
> > +     int i, n;
> > +
> > +     n = gpiod_count(dev, NULL);
> > +     if (n < 0)
> > +             return n;
> > +
> > +     descs = devm_kmalloc_array(dev, n, sizeof(*descs), GFP_KERNEL);
> > +     if (!descs)
> > +             return -ENOMEM;
> > +
> > +     for (i = 0; i < n; i++) {
> > +             descs[i] = devm_gpiod_get_index(dev, NULL, i, GPIOD_ASIS);
>
> can you please add this check as well as we need to return EPROBE_DEFER.
>
> if (desc[i] == ERR_PTR(-ENOENT))
> <                 return -EPROBE_DEFER;

So gpiod_get_index() nevers return -EPROBE_DEFER, but returns -ENOENT
instead?
How can a driver distinguish between "GPIO not found" and "gpiochip driver
not yet initialized"?
Worse, so the *_optional() variants will return NULL in both cases, too, so
the caller will always fall back to optional GPIO not present?

Or am I missing something?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 5/7] gpio: Add GPIO Aggregator/Repeater driver
  2019-12-03  8:17     ` Geert Uytterhoeven
@ 2019-12-03  8:51       ` Harish Jenny K N
  0 siblings, 0 replies; 48+ messages in thread
From: Harish Jenny K N @ 2019-12-03  8:51 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Geert Uytterhoeven, Linus Walleij, Bartosz Golaszewski,
	Jonathan Corbet, Rob Herring, Mark Rutland, Eugeniu Rosca,
	Alexander Graf, Peter Maydell, Paolo Bonzini, Phil Reid,
	Marc Zyngier, Christoffer Dall, Magnus Damm,
	open list:GPIO SUBSYSTEM, open list:DOCUMENTATION,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, Linux Kernel Mailing List, QEMU Developers


On 03/12/19 1:47 PM, Geert Uytterhoeven wrote:
> Hi Harish,
>
> On Tue, Dec 3, 2019 at 6:42 AM Harish Jenny K N
> <harish_kandiga@mentor.com> wrote:
>>> +static int gpio_aggregator_probe(struct platform_device *pdev)
>>> +{
>>> +     struct device *dev = &pdev->dev;
>>> +     struct gpio_desc **descs;
>>> +     struct gpiochip_fwd *fwd;
>>> +     int i, n;
>>> +
>>> +     n = gpiod_count(dev, NULL);
>>> +     if (n < 0)
>>> +             return n;
>>> +
>>> +     descs = devm_kmalloc_array(dev, n, sizeof(*descs), GFP_KERNEL);
>>> +     if (!descs)
>>> +             return -ENOMEM;
>>> +
>>> +     for (i = 0; i < n; i++) {
>>> +             descs[i] = devm_gpiod_get_index(dev, NULL, i, GPIOD_ASIS);
>> can you please add this check as well as we need to return EPROBE_DEFER.
>>
>> if (desc[i] == ERR_PTR(-ENOENT))
>> <                 return -EPROBE_DEFER;
> So gpiod_get_index() nevers return -EPROBE_DEFER, but returns -ENOENT
> instead?
> How can a driver distinguish between "GPIO not found" and "gpiochip driver
> not yet initialized"?
> Worse, so the *_optional() variants will return NULL in both cases, too, so
> the caller will always fall back to optional GPIO not present?
>
> Or am I missing something?
>
> Gr{oetje,eeting}s,
>
>                         Geert


We had earlier tested our changes on 4.14 kernel and the explicit return of -EPROBE_DEFER was needed in the inverter driver.

probably the commit 6662ae6af82df10259a70c7569b4c12ea7f3ba93 ( gpiolib: Keep returning EPROBE_DEFER when we should)

has fixed the issue and now it returns -EPROBE_DEFER.  you can ignore this comment as of now. I will test and let you know if needed.


Thanks,

Harish


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 5/7] gpio: Add GPIO Aggregator/Repeater driver
  2019-11-27  8:42 ` [PATCH v3 5/7] gpio: Add GPIO Aggregator/Repeater driver Geert Uytterhoeven
                     ` (2 preceding siblings ...)
  2019-12-03  5:42   ` Harish Jenny K N
@ 2019-12-03 10:51   ` Eugeniu Rosca
  2020-01-09 13:35     ` Geert Uytterhoeven
  2019-12-12 14:34   ` Linus Walleij
  4 siblings, 1 reply; 48+ messages in thread
From: Eugeniu Rosca @ 2019-12-03 10:51 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Linus Walleij, Bartosz Golaszewski, Jonathan Corbet, Rob Herring,
	Mark Rutland, Harish Jenny K N, Eugeniu Rosca, Alexander Graf,
	Peter Maydell, Paolo Bonzini, Phil Reid, Marc Zyngier,
	Christoffer Dall, Magnus Damm, linux-gpio, linux-doc, devicetree,
	linux-renesas-soc, linux-kernel, qemu-devel, Eugeniu Rosca

Hi Geert,

On Wed, Nov 27, 2019 at 09:42:51AM +0100, Geert Uytterhoeven wrote:
> +static int gpio_aggregator_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct gpio_desc **descs;
> +	struct gpiochip_fwd *fwd;
> +	int i, n;

FWIW/FTR, doing some blind creation and deletion of gpio aggregator
chips [1] on R-Car H3ULCB overnight, kmemleak reported once [2]. Not
sure this is something 100% reproducible.

[1] while true; do \
   echo e6055400.gpio 12,13 > /sys/bus/platform/drivers/gpio-aggregator/new_device; \
   echo gpio-aggregator.0 > /sys/bus/platform/drivers/gpio-aggregator/delete_device; \
   done 

[2] unreferenced object 0xffff0006d2c2e000 (size 128):
  comm "kworker/3:1", pid 55, jiffies 4294676978 (age 38546.676s)
  hex dump (first 32 bytes):
    00 d9 d2 d3 06 00 ff ff 0c 00 e0 0f ff ff ff ff  ................
    01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<00000000a8e18c13>] slab_post_alloc_hook+0x8c/0x94
    [<000000006f419a4f>] __kmalloc+0x170/0x218
    [<0000000060d185ea>] kobj_map+0x78/0x1c0
    [<00000000c96645f3>] cdev_add+0x68/0x94
    [<00000000a7a5a8ac>] cdev_device_add+0x74/0x90
    [<00000000497871d3>] gpiochip_setup_dev+0x84/0x1f0
    [<00000000b993f95f>] gpiochip_add_data_with_key+0xbcc/0x11f0
    [<00000000fd728c0e>] devm_gpiochip_add_data+0x60/0xa8
    [<00000000442e34c1>] gpio_aggregator_probe+0x210/0x3c8
    [<00000000076e13fb>] platform_drv_probe+0x70/0xe4
    [<00000000de84b58b>] really_probe+0x2d8/0x434
    [<00000000c95c9784>] driver_probe_device+0x15c/0x16c
    [<00000000afb7dd4f>] __device_attach_driver+0xdc/0x120
    [<00000000efa40cae>] bus_for_each_drv+0x12c/0x154
    [<00000000c149acef>] __device_attach+0x148/0x1e0
    [<00000000a74fd158>] device_initial_probe+0x24/0x30

-- 
Best Regards,
Eugeniu

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 4/7] dt-bindings: gpio: Add gpio-repeater bindings
  2019-11-27  8:42 ` [PATCH v3 4/7] dt-bindings: gpio: Add gpio-repeater bindings Geert Uytterhoeven
  2019-11-28  3:39   ` Ulrich Hecht
  2019-12-03  5:51   ` Harish Jenny K N
@ 2019-12-05 21:06   ` Rob Herring
  2019-12-06  9:17     ` Geert Uytterhoeven
  2 siblings, 1 reply; 48+ messages in thread
From: Rob Herring @ 2019-12-05 21:06 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Linus Walleij, Bartosz Golaszewski, Jonathan Corbet,
	Mark Rutland, Harish Jenny K N, Eugeniu Rosca, Alexander Graf,
	Peter Maydell, Paolo Bonzini, Phil Reid, Marc Zyngier,
	Christoffer Dall, Magnus Damm, linux-gpio, linux-doc, devicetree,
	linux-renesas-soc, linux-kernel, qemu-devel

On Wed, Nov 27, 2019 at 09:42:50AM +0100, Geert Uytterhoeven wrote:
> Add Device Tree bindings for a GPIO repeater, with optional translation
> of physical signal properties.  This is useful for describing explicitly
> the presence of e.g. an inverter on a GPIO line, and was inspired by the
> non-YAML gpio-inverter bindings by Harish Jenny K N
> <harish_kandiga@mentor.com>[1].
> 
> Note that this is different from a GPIO Nexus Node[2], which cannot do
> physical signal property translation.

It can't? Why not? The point of the passthru mask is to not do 
translation of flags, but without it you are always doing translation of 
cells.

> 
> While an inverter can be described implicitly by exchanging the
> GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW flags, this has its limitations.
> Each GPIO line has only a single GPIO_ACTIVE_* flag, but applies to both
> th provider and consumer sides:
>   1. The GPIO provider (controller) looks at the flags to know the
>      polarity, so it can translate between logical (active/not active)
>      and physical (high/low) signal levels.
>   2. While the signal polarity is usually fixed on the GPIO consumer
>      side (e.g. an LED is tied to either the supply voltage or GND),
>      it may be configurable on some devices, and both sides need to
>      agree.  Hence the GPIO_ACTIVE_* flag as seen by the consumer must
>      match the actual polarity.
>      There exists a similar issue with interrupt flags, where both the
>      interrupt controller and the device generating the interrupt need
>      to agree, which breaks in the presence of a physical inverter not
>      described in DT (see e.g. [3]).

Adding an inverted flag as I've suggested would also solve this issue.

> 
> [1] "[PATCH V4 2/2] gpio: inverter: document the inverter bindings"
>     https://lore.kernel.org/linux-gpio/1561699236-18620-3-git-send-email-harish_kandiga@mentor.com/
> 
> [2] Devicetree Specification v0.3-rc2, Section 2.5
>     https://github.com/devicetree-org/devicetree-specification/releases/tag/v0.3-rc2
> 
> [3] "[PATCH] wlcore/wl18xx: Add invert-irq OF property for physically
>     inverted IRQ"
>     https://lore.kernel.org/linux-renesas-soc/20190607172958.20745-1-erosca@de.adit-jv.com/
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 4/7] dt-bindings: gpio: Add gpio-repeater bindings
  2019-12-05 21:06   ` Rob Herring
@ 2019-12-06  9:17     ` Geert Uytterhoeven
  2019-12-06 15:03       ` Rob Herring
  0 siblings, 1 reply; 48+ messages in thread
From: Geert Uytterhoeven @ 2019-12-06  9:17 UTC (permalink / raw)
  To: Rob Herring
  Cc: Geert Uytterhoeven, Linus Walleij, Bartosz Golaszewski,
	Jonathan Corbet, Mark Rutland, Harish Jenny K N, Eugeniu Rosca,
	Alexander Graf, Peter Maydell, Paolo Bonzini, Phil Reid,
	Marc Zyngier, Christoffer Dall, Magnus Damm,
	open list:GPIO SUBSYSTEM, open list:DOCUMENTATION,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, Linux Kernel Mailing List, QEMU Developers

Hi Rob,

On Thu, Dec 5, 2019 at 10:06 PM Rob Herring <robh@kernel.org> wrote:
> On Wed, Nov 27, 2019 at 09:42:50AM +0100, Geert Uytterhoeven wrote:
> > Add Device Tree bindings for a GPIO repeater, with optional translation
> > of physical signal properties.  This is useful for describing explicitly
> > the presence of e.g. an inverter on a GPIO line, and was inspired by the
> > non-YAML gpio-inverter bindings by Harish Jenny K N
> > <harish_kandiga@mentor.com>[1].
> >
> > Note that this is different from a GPIO Nexus Node[2], which cannot do
> > physical signal property translation.
>
> It can't? Why not? The point of the passthru mask is to not do
> translation of flags, but without it you are always doing translation of
> cells.

Thanks for pushing me deeper into nexuses!
You're right, you can map from one type to another.
However, you cannot handle the "double inversion" of an ACTIVE_LOW
signal with a physical inverter added:

        nexus: led-nexus {
                #gpio-cells = <2>;
                gpio-map = <0 0 &gpio2 19 GPIO_ACTIVE_LOW>,     // inverted
                           <1 0 &gpio2 20 GPIO_ACTIVE_HIGH>,    // noninverted
                           <2 0 &gpio2 21 GPIO_ACTIVE_LOW>;     // inverted
                gpio-map-mask = <3 0>;
                // default gpio-map-pass-thru = <0 0>;
        };

        leds {
                compatible = "gpio-leds";
                led6-inverted {
                        gpios = <&nexus 0 GPIO_ACTIVE_HIGH>;
                };
                led7-noninverted {
                        gpios = <&nexus 1 GPIO_ACTIVE_HIGH>;
                };
                led8-double-inverted {  // FAILS: still inverted
                        gpios = <&nexus 2 GPIO_ACTIVE_LOW>;
                };
        };

It "works" if the last entry in gpio-map is changed to GPIO_ACTIVE_HIGH.
Still, the consumer would see the final translated polarity, and not the
actual one it needs to program the consumer for.

> > While an inverter can be described implicitly by exchanging the
> > GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW flags, this has its limitations.
> > Each GPIO line has only a single GPIO_ACTIVE_* flag, but applies to both
> > th provider and consumer sides:
> >   1. The GPIO provider (controller) looks at the flags to know the
> >      polarity, so it can translate between logical (active/not active)
> >      and physical (high/low) signal levels.
> >   2. While the signal polarity is usually fixed on the GPIO consumer
> >      side (e.g. an LED is tied to either the supply voltage or GND),
> >      it may be configurable on some devices, and both sides need to
> >      agree.  Hence the GPIO_ACTIVE_* flag as seen by the consumer must
> >      match the actual polarity.
> >      There exists a similar issue with interrupt flags, where both the
> >      interrupt controller and the device generating the interrupt need
> >      to agree, which breaks in the presence of a physical inverter not
> >      described in DT (see e.g. [3]).
>
> Adding an inverted flag as I've suggested would also solve this issue.

As per your suggestion in "Re: [PATCH V4 2/2] gpio: inverter: document
the inverter bindings"?
https://lore.kernel.org/linux-devicetree/CAL_JsqLp___2O-naU+2PPQy0QmJX6+aN3hByz-OB9+qFvWgN9Q@mail.gmail.com/

Oh, now I understand. I was misguided by Harish' interpretation
https://lore.kernel.org/linux-devicetree/dde73334-a26d-b53f-6b97-4101c1cdc185@mentor.com/
which assumed an "inverted" property, e.g.

    inverted = /bits/ 8 <0 0 0 0 0 0 1 0 0 0 0 0 0 0 0 0 0 0>;

But you actually meant a new GPIO_INVERTED flag, to be ORed into the 2nd
cell of a GPIO specifier? I.e. add to include/dt-bindings/gpio/gpio.h"

    /* Bit 6 expresses the presence of a physical inverter */
    #define GPIO_INVERTED 64

We need to be very careful in defining to which side the GPIO_ACTIVE_*
applies to (consumer?), and which side the GPIO_INVERTED flag (provider?).
Still, this doesn't help if e.g. a FET is used instead of a push-pull
inverter, as the former needs translation of other flags (which the
nexus can do, the caveats above still applies, though).

Same for adding IRQ_TYPE_INVERTED.

Related issue: how to handle physical inverters on SPI chip select lines,
if the SPI slave can be configured for both polarities?

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 4/7] dt-bindings: gpio: Add gpio-repeater bindings
  2019-12-06  9:17     ` Geert Uytterhoeven
@ 2019-12-06 15:03       ` Rob Herring
  2020-01-06  8:12         ` Geert Uytterhoeven
  0 siblings, 1 reply; 48+ messages in thread
From: Rob Herring @ 2019-12-06 15:03 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Geert Uytterhoeven, Linus Walleij, Bartosz Golaszewski,
	Jonathan Corbet, Mark Rutland, Harish Jenny K N, Eugeniu Rosca,
	Alexander Graf, Peter Maydell, Paolo Bonzini, Phil Reid,
	Marc Zyngier, Christoffer Dall, Magnus Damm,
	open list:GPIO SUBSYSTEM, open list:DOCUMENTATION,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, Linux Kernel Mailing List, QEMU Developers

On Fri, Dec 6, 2019 at 3:17 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Rob,
>
> On Thu, Dec 5, 2019 at 10:06 PM Rob Herring <robh@kernel.org> wrote:
> > On Wed, Nov 27, 2019 at 09:42:50AM +0100, Geert Uytterhoeven wrote:
> > > Add Device Tree bindings for a GPIO repeater, with optional translation
> > > of physical signal properties.  This is useful for describing explicitly
> > > the presence of e.g. an inverter on a GPIO line, and was inspired by the
> > > non-YAML gpio-inverter bindings by Harish Jenny K N
> > > <harish_kandiga@mentor.com>[1].
> > >
> > > Note that this is different from a GPIO Nexus Node[2], which cannot do
> > > physical signal property translation.
> >
> > It can't? Why not? The point of the passthru mask is to not do
> > translation of flags, but without it you are always doing translation of
> > cells.
>
> Thanks for pushing me deeper into nexuses!
> You're right, you can map from one type to another.
> However, you cannot handle the "double inversion" of an ACTIVE_LOW
> signal with a physical inverter added:
>
>         nexus: led-nexus {
>                 #gpio-cells = <2>;
>                 gpio-map = <0 0 &gpio2 19 GPIO_ACTIVE_LOW>,     // inverted
>                            <1 0 &gpio2 20 GPIO_ACTIVE_HIGH>,    // noninverted
>                            <2 0 &gpio2 21 GPIO_ACTIVE_LOW>;     // inverted
>                 gpio-map-mask = <3 0>;
>                 // default gpio-map-pass-thru = <0 0>;
>         };
>
>         leds {
>                 compatible = "gpio-leds";
>                 led6-inverted {
>                         gpios = <&nexus 0 GPIO_ACTIVE_HIGH>;
>                 };
>                 led7-noninverted {
>                         gpios = <&nexus 1 GPIO_ACTIVE_HIGH>;
>                 };
>                 led8-double-inverted {  // FAILS: still inverted
>                         gpios = <&nexus 2 GPIO_ACTIVE_LOW>;
>                 };
>         };
>
> It "works" if the last entry in gpio-map is changed to GPIO_ACTIVE_HIGH.
> Still, the consumer would see the final translated polarity, and not the
> actual one it needs to program the consumer for.

I'm not really following. Why isn't a double inversion just the same
as no inversion?

> > > While an inverter can be described implicitly by exchanging the
> > > GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW flags, this has its limitations.
> > > Each GPIO line has only a single GPIO_ACTIVE_* flag, but applies to both
> > > th provider and consumer sides:
> > >   1. The GPIO provider (controller) looks at the flags to know the
> > >      polarity, so it can translate between logical (active/not active)
> > >      and physical (high/low) signal levels.
> > >   2. While the signal polarity is usually fixed on the GPIO consumer
> > >      side (e.g. an LED is tied to either the supply voltage or GND),
> > >      it may be configurable on some devices, and both sides need to
> > >      agree.  Hence the GPIO_ACTIVE_* flag as seen by the consumer must
> > >      match the actual polarity.
> > >      There exists a similar issue with interrupt flags, where both the
> > >      interrupt controller and the device generating the interrupt need
> > >      to agree, which breaks in the presence of a physical inverter not
> > >      described in DT (see e.g. [3]).
> >
> > Adding an inverted flag as I've suggested would also solve this issue.
>
> As per your suggestion in "Re: [PATCH V4 2/2] gpio: inverter: document
> the inverter bindings"?
> https://lore.kernel.org/linux-devicetree/CAL_JsqLp___2O-naU+2PPQy0QmJX6+aN3hByz-OB9+qFvWgN9Q@mail.gmail.com/
>
> Oh, now I understand. I was misguided by Harish' interpretation
> https://lore.kernel.org/linux-devicetree/dde73334-a26d-b53f-6b97-4101c1cdc185@mentor.com/
> which assumed an "inverted" property, e.g.
>
>     inverted = /bits/ 8 <0 0 0 0 0 0 1 0 0 0 0 0 0 0 0 0 0 0>;
>
> But you actually meant a new GPIO_INVERTED flag, to be ORed into the 2nd
> cell of a GPIO specifier? I.e. add to include/dt-bindings/gpio/gpio.h"
>
>     /* Bit 6 expresses the presence of a physical inverter */
>     #define GPIO_INVERTED 64

Exactly.

> We need to be very careful in defining to which side the GPIO_ACTIVE_*
> applies to (consumer?), and which side the GPIO_INVERTED flag (provider?).
> Still, this doesn't help if e.g. a FET is used instead of a push-pull
> inverter, as the former needs translation of other flags (which the
> nexus can do, the caveats above still applies, though).

Yes. Historically the cells values are meaningful to the provider and
opaque to the consumer. Standardized cell values changes that
somewhat. I think we want the active flag to be from the provider's
prospective because the provider always needs to know. The consumer
often doesn't need to know. That also means things work without the
GPIO_INVERTED flag if the consumer doesn't care which is what we have
today already and we can't go back in time.


> Same for adding IRQ_TYPE_INVERTED.

I suppose so, yes.

> Related issue: how to handle physical inverters on SPI chip select lines,
> if the SPI slave can be configured for both polarities?

Good question. Perhaps in a different way because we have to handle
both h/w controlled and gpio chip selects.

However, how would one configure the polarity in the device in the
first place? You have to assert the CS first to give a command to
reprogram it.

Rob

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 1/7] gpiolib: Add GPIOCHIP_NAME definition
  2019-11-27  8:42 ` [PATCH v3 1/7] gpiolib: Add GPIOCHIP_NAME definition Geert Uytterhoeven
  2019-11-28  3:38   ` Ulrich Hecht
  2019-12-02 21:17   ` Eugeniu Rosca
@ 2019-12-12 10:37   ` Linus Walleij
  2 siblings, 0 replies; 48+ messages in thread
From: Linus Walleij @ 2019-12-12 10:37 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Bartosz Golaszewski, Jonathan Corbet, Rob Herring, Mark Rutland,
	Harish Jenny K N, Eugeniu Rosca, Alexander Graf, Peter Maydell,
	Paolo Bonzini, Phil Reid, Marc Zyngier, Christoffer Dall,
	Magnus Damm, open list:GPIO SUBSYSTEM, Linux Doc Mailing List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-kernel, QEMU Developers

On Wed, Nov 27, 2019 at 9:43 AM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:

> The string literal "gpiochip" is used in several places.
> Add a definition for it, and use it everywhere, to make sure everything
> stays in sync.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v3:
>   - New.

This is a good patch on its own merits so I have applied
this with the ACKs. (Haven't looked at the rest yet...)

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 2/7] gpiolib: Add support for gpiochipN-based table lookup
  2019-11-27  8:42 ` [PATCH v3 2/7] gpiolib: Add support for gpiochipN-based table lookup Geert Uytterhoeven
  2019-11-28  3:38   ` Ulrich Hecht
@ 2019-12-12 13:20   ` Linus Walleij
  2019-12-12 13:33     ` Geert Uytterhoeven
  1 sibling, 1 reply; 48+ messages in thread
From: Linus Walleij @ 2019-12-12 13:20 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Bartosz Golaszewski, Jonathan Corbet, Rob Herring, Mark Rutland,
	Harish Jenny K N, Eugeniu Rosca, Alexander Graf, Peter Maydell,
	Paolo Bonzini, Phil Reid, Marc Zyngier, Christoffer Dall,
	Magnus Damm, open list:GPIO SUBSYSTEM, Linux Doc Mailing List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-kernel, QEMU Developers

Hi Geert!

On Wed, Nov 27, 2019 at 9:43 AM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:

> Currently GPIO controllers can only be referred to by label in GPIO
> lookup tables.
>
> Add support for looking them up by "gpiochipN" name, with "N" either the
> corresponding GPIO device's ID number, or the GPIO controller's first
> GPIO number.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

What the commit message is missing is a rationale, why is this needed?

> If this is rejected, the GPIO Aggregator documentation must be updated.
>
> The second variant is currently used by the legacy sysfs interface only,
> so perhaps the chip->base check should be dropped?

Anything improving the sysfs is actively discouraged by me.
If it is just about staying compatible it is another thing.

> +static int gpiochip_match_id(struct gpio_chip *chip, void *data)
> +{
> +       int id = (uintptr_t)data;
> +
> +       return id == chip->base || id == chip->gpiodev->id;
> +}
>  static struct gpio_chip *find_chip_by_name(const char *name)
>  {
> -       return gpiochip_find((void *)name, gpiochip_match_name);
> +       struct gpio_chip *chip;
> +       int id;
> +
> +       chip = gpiochip_find((void *)name, gpiochip_match_name);
> +       if (chip)
> +               return chip;
> +
> +       if (!str_has_prefix(name, GPIOCHIP_NAME))
> +               return NULL;
> +
> +       if (kstrtoint(name + strlen(GPIOCHIP_NAME), 10, &id))
> +               return NULL;
> +
> +       return gpiochip_find((void *)(uintptr_t)id, gpiochip_match_id);

Isn't it easier to just  augment the existing match function to
check like this:

static int gpiochip_match_name(struct gpio_chip *chip, void *data)
{
        const char *name = data;

        if (!strcmp(chip->label, name))
               return 0;
        return !strcmp(dev_name(&chip->gpiodev->dev), name);
}

We should I guess also add some kerneldoc to say we first
match on the label and second on dev_name().

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 2/7] gpiolib: Add support for gpiochipN-based table lookup
  2019-12-12 13:20   ` Linus Walleij
@ 2019-12-12 13:33     ` Geert Uytterhoeven
  2019-12-12 14:36       ` Linus Walleij
  0 siblings, 1 reply; 48+ messages in thread
From: Geert Uytterhoeven @ 2019-12-12 13:33 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Geert Uytterhoeven, Bartosz Golaszewski, Jonathan Corbet,
	Rob Herring, Mark Rutland, Harish Jenny K N, Eugeniu Rosca,
	Alexander Graf, Peter Maydell, Paolo Bonzini, Phil Reid,
	Marc Zyngier, Christoffer Dall, Magnus Damm,
	open list:GPIO SUBSYSTEM, Linux Doc Mailing List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-kernel, QEMU Developers

Hi Linus,

On Thu, Dec 12, 2019 at 2:20 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Nov 27, 2019 at 9:43 AM Geert Uytterhoeven
> <geert+renesas@glider.be> wrote:
> > Currently GPIO controllers can only be referred to by label in GPIO
> > lookup tables.
> >
> > Add support for looking them up by "gpiochipN" name, with "N" either the
> > corresponding GPIO device's ID number, or the GPIO controller's first
> > GPIO number.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> What the commit message is missing is a rationale, why is this needed?

Right. To be added: so they can be looked up in the GPIO lookup table
using either the chip's label, or the "gpiochipN" name.

> > If this is rejected, the GPIO Aggregator documentation must be updated.
> >
> > The second variant is currently used by the legacy sysfs interface only,
> > so perhaps the chip->base check should be dropped?
>
> Anything improving the sysfs is actively discouraged by me.
> If it is just about staying compatible it is another thing.

OK, so N must be the corresponding GPIO device's ID number.

> > +static int gpiochip_match_id(struct gpio_chip *chip, void *data)
> > +{
> > +       int id = (uintptr_t)data;
> > +
> > +       return id == chip->base || id == chip->gpiodev->id;
> > +}
> >  static struct gpio_chip *find_chip_by_name(const char *name)
> >  {
> > -       return gpiochip_find((void *)name, gpiochip_match_name);
> > +       struct gpio_chip *chip;
> > +       int id;
> > +
> > +       chip = gpiochip_find((void *)name, gpiochip_match_name);
> > +       if (chip)
> > +               return chip;
> > +
> > +       if (!str_has_prefix(name, GPIOCHIP_NAME))
> > +               return NULL;
> > +
> > +       if (kstrtoint(name + strlen(GPIOCHIP_NAME), 10, &id))
> > +               return NULL;
> > +
> > +       return gpiochip_find((void *)(uintptr_t)id, gpiochip_match_id);
>
> Isn't it easier to just  augment the existing match function to
> check like this:
>
> static int gpiochip_match_name(struct gpio_chip *chip, void *data)
> {
>         const char *name = data;
>
>         if (!strcmp(chip->label, name))
>                return 0;

return true;

>         return !strcmp(dev_name(&chip->gpiodev->dev), name);
> }

Oh, didn't think of using dev_name() on the gpiodev.
Yes, with the chip->base check removed, the code can be simplified.

Or just

        return !strcmp(chip->label, name) ||
               !strcmp(dev_name(&chip->gpiodev->dev), name);

> We should I guess also add some kerneldoc to say we first
> match on the label and second on dev_name().

OK.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 3/7] gpiolib: Add support for GPIO line table lookup
  2019-11-27  8:42 ` [PATCH v3 3/7] gpiolib: Add support for GPIO line " Geert Uytterhoeven
  2019-11-28  3:39   ` Ulrich Hecht
@ 2019-12-12 13:40   ` Linus Walleij
  1 sibling, 0 replies; 48+ messages in thread
From: Linus Walleij @ 2019-12-12 13:40 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Bartosz Golaszewski, Jonathan Corbet, Rob Herring, Mark Rutland,
	Harish Jenny K N, Eugeniu Rosca, Alexander Graf, Peter Maydell,
	Paolo Bonzini, Phil Reid, Marc Zyngier, Christoffer Dall,
	Magnus Damm, open list:GPIO SUBSYSTEM, Linux Doc Mailing List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-kernel, QEMU Developers

On Wed, Nov 27, 2019 at 9:43 AM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:

> Currently GPIOs can only be referred to by GPIO controller and offset in
> GPIO lookup tables.
>
> Add support for looking them up by line name.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

OK I see what you want to do...

> +               if (p->chip_hwnum == (u16)-1) {

If you want to use this then use:

if (p->chip_hwnum == U16_MAX)

from <linux/limits.h>

> +                       desc = gpio_name_to_desc(p->chip_label);
> +                       if (desc) {
> +                               *flags = p->flags;
> +                               return desc;
> +                       }
> +
> +                       dev_warn(dev, "cannot find GPIO line %s, deferring\n",
> +                                p->chip_label);
> +                       return ERR_PTR(-EPROBE_DEFER);
> +               }
> +
>                 chip = find_chip_by_name(p->chip_label);
>
>                 if (!chip) {
> diff --git a/include/linux/gpio/machine.h b/include/linux/gpio/machine.h
> index 1ebe5be05d5f81fa..84c1c097e55eefaf 100644
> --- a/include/linux/gpio/machine.h
> +++ b/include/linux/gpio/machine.h
> @@ -31,7 +31,7 @@ enum gpio_lookup_flags {
>   */
>  struct gpiod_lookup {
>         const char *chip_label;
> -       u16 chip_hwnum;
> +       u16 chip_hwnum;                 /* if -1, chip_label is named line */

/* if U16_MAX then chip_label is the named line we are looking for */

But the member name "chip_label" is completely abused with this
setup, it should then be renamed as part of the patch set to something
like chip_label_or_line_name so it is clear what it is or
just name it "const char *key".

But I'm not entirely convinced about reusing the existing
struct gpio_lookup for this.

What about constructing a new lookup struct specifically for this?
I understand it is more work, but will that not be more
maintainable and readable?

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 5/7] gpio: Add GPIO Aggregator/Repeater driver
  2019-11-27  8:42 ` [PATCH v3 5/7] gpio: Add GPIO Aggregator/Repeater driver Geert Uytterhoeven
                     ` (3 preceding siblings ...)
  2019-12-03 10:51   ` Eugeniu Rosca
@ 2019-12-12 14:34   ` Linus Walleij
  2019-12-12 15:24     ` Geert Uytterhoeven
  4 siblings, 1 reply; 48+ messages in thread
From: Linus Walleij @ 2019-12-12 14:34 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Bartosz Golaszewski, Jonathan Corbet, Rob Herring, Mark Rutland,
	Harish Jenny K N, Eugeniu Rosca, Alexander Graf, Peter Maydell,
	Paolo Bonzini, Phil Reid, Marc Zyngier, Christoffer Dall,
	Magnus Damm, open list:GPIO SUBSYSTEM, Linux Doc Mailing List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-kernel, QEMU Developers

Hi Geert!

Thanks for this interesting patch!

On Wed, Nov 27, 2019 at 9:43 AM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:

> GPIO controllers are exported to userspace using /dev/gpiochip*
> character devices.  Access control to these devices is provided by
> standard UNIX file system permissions, on an all-or-nothing basis:
> either a GPIO controller is accessible for a user, or it is not.
> Currently no mechanism exists to control access to individual GPIOs.
>
> Hence add a GPIO driver to aggregate existing GPIOs, and expose them as
> a new gpiochip.
>
> This supports the following use cases:
>   1. Aggregating GPIOs using Sysfs
>      This is useful for implementing access control, and assigning a set
>      of GPIOs to a specific user or virtual machine.
>
>   2. GPIO Repeater in Device Tree
>      This supports modelling e.g. GPIO inverters in DT.
>
>   3. Generic GPIO Driver
>      This provides userspace access to a simple GPIO-operated device
>      described in DT, cfr. e.g. spidev for SPI-operated devices.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Overall I like how this is developing!

> +config GPIO_AGGREGATOR
> +       tristate "GPIO Aggregator/Repeater"
> +       help
> +         Say yes here to enable the GPIO Aggregator and repeater, which
> +         provides a way to aggregate and/or repeat existing GPIOs into a new
> +         GPIO device.

Should it say a "new virtual GPIO chip"?

> +         This can serve the following purposes:
> +           1. Assign a collection of GPIOs to a user, or export them to a
> +              virtual machine,

This is ambiguous. What is a "user"? A process calling from
userspace? A device tree node?

I would write "assign a collection of GPIO lines from any lines on
existing physical GPIO chips to form a new virtual GPIO chip"

That should be to the point, right?

> +           2. Support GPIOs that are connected to a physical inverter,

s/to/through/g

> +           3. Provide a generic driver for a GPIO-operated device, to be
> +               controlled from userspace using the GPIO chardev interface.

I don't understand this, it needs to be elaborated. What is meant
by a "GPIO-operated device" in this context? Example?

I consistently use the term "GPIO line" as opposed to "GPIO"
or "GPIO number" etc that are abigous, so please rephrase using
"GPIO lines" rather than just "GPIOs" above.

> +#include "gpiolib.h"

Whenever this is included in a driver I want it to come with a comment
explicitly stating exactly why and which internal symbols the driver
needs to access. Ideally all drivers should just need <linux/gpio/driver.h>...

> +static int aggr_add_gpio(struct gpio_aggregator *aggr, const char *label,
> +                        int hwnum, unsigned int *n)

u16 hwnum for the hardware number but if it is always -1/U16_MAX
then why pass the parameter at all.

Is "label" the right name of this parameter if that is going to actually
be line_name then use that.

> +{
> +       struct gpiod_lookup_table *lookups;
> +
> +       lookups = krealloc(aggr->lookups, struct_size(lookups, table, *n + 2),
> +                          GFP_KERNEL);
> +       if (!lookups)
> +               return -ENOMEM;
> +
> +       lookups->table[*n].chip_label = label;

This is pending the discussion on whether to just use "key" for this
name.

> +       lookups->table[*n].chip_hwnum = hwnum;

If this is always going to be U16_MAX (-1 in the current code)
then it can just be assigned as that here instead of passed as
parameter.

> +static int aggr_parse(struct gpio_aggregator *aggr)
> +{
> +       char *name, *offsets, *first, *last, *next;
> +       unsigned int a, b, i, n = 0;
> +       char *args = aggr->args;
> +       int error;
> +
> +       for (name = get_arg(&args), offsets = get_arg(&args); name;
> +            offsets = get_arg(&args)) {
> +               if (IS_ERR(name)) {
> +                       pr_err("Cannot get GPIO specifier: %ld\n",
> +                              PTR_ERR(name));
> +                       return PTR_ERR(name);
> +               }
> +
> +               if (!isrange(offsets)) {
> +                       /* Named GPIO line */
> +                       error = aggr_add_gpio(aggr, name, -1, &n);

So the third argument woule be U16_MAX here. Or not pass
a parameter at all.

But honestly, when I look at this I don't understand why you
have to avoid so hard to use offsets for the GPIO lines on
your aggregator?

Just put a u16 ngpios in your
struct gpio_aggregator and count it up every time you
add some new offsets here and you have
offset numbers for all your GPIO lines on the aggregator
and you can just drop the patch for lookup up lines by line
names.

Is there something wrong with my reasoning here?

At the pointe later when the lines are counted from the
allocated lookups using gpiod_count() that will just figure
out this number anyways, so it is not like we don't know
it at the end of the day.

So it seems the patch to gpiolib is just to use machine
descriptor tables as a substitute for a simple counter
variable in this local struct to me.

> +static void __exit gpio_aggregator_remove_all(void)
> +{
> +       mutex_lock(&gpio_aggregator_lock);
> +       idr_for_each(&gpio_aggregator_idr, gpio_aggregator_idr_remove, NULL);
> +       idr_destroy(&gpio_aggregator_idr);
> +       mutex_unlock(&gpio_aggregator_lock);
> +}
> +
> +
> +       /*
> +        *  Common GPIO Forwarder
> +        */
> +

Nitpick: lots and weird spacing here.

> +struct gpiochip_fwd {
> +       struct gpio_chip chip;
> +       struct gpio_desc **descs;
> +       union {
> +               struct mutex mlock;     /* protects tmp[] if can_sleep */
> +               spinlock_t slock;       /* protects tmp[] if !can_sleep */
> +       };

That was a very elegant use of union!

> +static int gpio_fwd_get_multiple(struct gpio_chip *chip, unsigned long *mask,
> +                                unsigned long *bits)
> +static void gpio_fwd_set_multiple(struct gpio_chip *chip, unsigned long *mask,
> +                                 unsigned long *bits)

I guess these can both be optimized to use get/set_multiple on
the target chip if the offsets are consecutive?

However that is going to be tricky so I'm not saying you should
implement that. So for now, let's say just add a TODO: comment
about it.

> +static int gpio_fwd_init_valid_mask(struct gpio_chip *chip,
> +                                   unsigned long *valid_mask,
> +                                   unsigned int ngpios)
> +{
> +       struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> +       unsigned int i;
> +
> +       for (i = 0; i < ngpios; i++) {
> +               if (!gpiochip_line_is_valid(fwd->descs[i]->gdev->chip,
> +                                           gpio_chip_hwgpio(fwd->descs[i])))
> +                       clear_bit(i, valid_mask);
> +       }

This is what uses "gpiolib.h" is it not?

devm_gpiod_get_index() will not succeed if the line
is not valid so I think this can be just dropped, since
what you do before this is exactly devm_gpiod_get_index()
on each line, then you call gpiochip_fwd_create()
with the result.

So I think you can just drop this entire function.
This will not happen.

If it does happen, add a comment above this loop
explaining which circumstances would make lines on
the forwarder invalid.

> +       for (i = 0; i < ngpios; i++) {
> +               dev_dbg(dev, "gpio %u => gpio-%d (%s)\n", i,
> +                       desc_to_gpio(descs[i]), descs[i]->label ? : "?");
> +
> +               if (gpiod_cansleep(descs[i]))
> +                       chip->can_sleep = true;
> +               if (descs[i]->gdev->chip->set_config)
> +                       chip->set_config = gpio_fwd_set_config;
> +               if (descs[i]->gdev->chip->init_valid_mask)
> +                       chip->init_valid_mask = gpio_fwd_init_valid_mask;
> +       }

I do not think you should need to inspect the init_valid_mask()
as explained above.

Add a comment above the loop that if any of the GPIO lines
are sleeping then the entire forwarder will be sleeping
and if any of the chips support .set_config() we will support
setting configs.

However the way that the .gpio_fwd_set_config() is coded
it looks like you can just unconditionally assign it and
only check the cansleep condition in this loop.

> +}
> +
> +
> +       /*
> +        *  Common GPIO Aggregator/Repeater platform device
> +        */
> +

Nitpick: weird and excess spacing again.

> +       for (i = 0; i < n; i++) {
> +               descs[i] = devm_gpiod_get_index(dev, NULL, i, GPIOD_ASIS);
> +               if (IS_ERR(descs[i]))
> +                       return PTR_ERR(descs[i]);
> +       }

If this succeeds none of the obtained gpio_desc:s can be
invalid.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 2/7] gpiolib: Add support for gpiochipN-based table lookup
  2019-12-12 13:33     ` Geert Uytterhoeven
@ 2019-12-12 14:36       ` Linus Walleij
  0 siblings, 0 replies; 48+ messages in thread
From: Linus Walleij @ 2019-12-12 14:36 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Geert Uytterhoeven, Bartosz Golaszewski, Jonathan Corbet,
	Rob Herring, Mark Rutland, Harish Jenny K N, Eugeniu Rosca,
	Alexander Graf, Peter Maydell, Paolo Bonzini, Phil Reid,
	Marc Zyngier, Christoffer Dall, Magnus Damm,
	open list:GPIO SUBSYSTEM, Linux Doc Mailing List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-kernel, QEMU Developers

On Thu, Dec 12, 2019 at 2:33 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Thu, Dec 12, 2019 at 2:20 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Wed, Nov 27, 2019 at 9:43 AM Geert Uytterhoeven
> > <geert+renesas@glider.be> wrote:
> > > Currently GPIO controllers can only be referred to by label in GPIO
> > > lookup tables.
> > >
> > > Add support for looking them up by "gpiochipN" name, with "N" either the
> > > corresponding GPIO device's ID number, or the GPIO controller's first
> > > GPIO number.
> > >
> > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >
> > What the commit message is missing is a rationale, why is this needed?
>
> Right. To be added: so they can be looked up in the GPIO lookup table
> using either the chip's label, or the "gpiochipN" name.

After reading the aggregator/forwarder driver I am not convinced
that this is needed at all and I think this patch can be dropped,
but check my review and see what you think!

Thanks,
Linus Walleij

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 6/7] docs: gpio: Add GPIO Aggregator/Repeater documentation
  2019-11-27  8:42 ` [PATCH v3 6/7] docs: gpio: Add GPIO Aggregator/Repeater documentation Geert Uytterhoeven
  2019-11-28  3:41   ` Ulrich Hecht
@ 2019-12-12 14:42   ` Linus Walleij
  2019-12-12 14:48     ` Geert Uytterhoeven
  1 sibling, 1 reply; 48+ messages in thread
From: Linus Walleij @ 2019-12-12 14:42 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Bartosz Golaszewski, Jonathan Corbet, Rob Herring, Mark Rutland,
	Harish Jenny K N, Eugeniu Rosca, Alexander Graf, Peter Maydell,
	Paolo Bonzini, Phil Reid, Marc Zyngier, Christoffer Dall,
	Magnus Damm, open list:GPIO SUBSYSTEM, Linux Doc Mailing List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-kernel, QEMU Developers

On Wed, Nov 27, 2019 at 9:43 AM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:

> +The GPIO Aggregator allows access control for individual GPIOs, by aggregating
> +them into a new gpio_chip, which can be assigned to a group or user using
> +standard UNIX file ownership and permissions.  Furthermore, this simplifies and
> +hardens exporting GPIOs to a virtual machine, as the VM can just grab the full
> +GPIO controller, and no longer needs to care about which GPIOs to grab and
> +which not, reducing the attack surface.
> +
> +Aggregated GPIO controllers are instantiated and destroyed by writing to
> +write-only attribute files in sysfs.

I suppose virtual machines will have a lengthy config file where
they specify which GPIO lines to pick and use for their GPIO
aggregator, and that will all be fine, the VM starts and the aggregator
is there and we can start executing.

I would perhaps point out a weakness as with all sysfs and with the current
gpio sysfs: if a process creates an aggregator device, and then that
process crashes, what happens when you try to restart the process and
run e.g. your VM again?

Time for a hard reboot? Or should we add some design guidelines for
these machines so that they can cleanly tear down aggregators
previously created by the crashed VM?

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 6/7] docs: gpio: Add GPIO Aggregator/Repeater documentation
  2019-12-12 14:42   ` Linus Walleij
@ 2019-12-12 14:48     ` Geert Uytterhoeven
  2020-01-04  0:21       ` Linus Walleij
  0 siblings, 1 reply; 48+ messages in thread
From: Geert Uytterhoeven @ 2019-12-12 14:48 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Geert Uytterhoeven, Bartosz Golaszewski, Jonathan Corbet,
	Rob Herring, Mark Rutland, Harish Jenny K N, Eugeniu Rosca,
	Alexander Graf, Peter Maydell, Paolo Bonzini, Phil Reid,
	Marc Zyngier, Christoffer Dall, Magnus Damm,
	open list:GPIO SUBSYSTEM, Linux Doc Mailing List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-kernel, QEMU Developers

Hi Linus,

On Thu, Dec 12, 2019 at 3:42 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Nov 27, 2019 at 9:43 AM Geert Uytterhoeven
> <geert+renesas@glider.be> wrote:
> > +The GPIO Aggregator allows access control for individual GPIOs, by aggregating
> > +them into a new gpio_chip, which can be assigned to a group or user using
> > +standard UNIX file ownership and permissions.  Furthermore, this simplifies and
> > +hardens exporting GPIOs to a virtual machine, as the VM can just grab the full
> > +GPIO controller, and no longer needs to care about which GPIOs to grab and
> > +which not, reducing the attack surface.
> > +
> > +Aggregated GPIO controllers are instantiated and destroyed by writing to
> > +write-only attribute files in sysfs.
>
> I suppose virtual machines will have a lengthy config file where
> they specify which GPIO lines to pick and use for their GPIO
> aggregator, and that will all be fine, the VM starts and the aggregator
> is there and we can start executing.
>
> I would perhaps point out a weakness as with all sysfs and with the current
> gpio sysfs: if a process creates an aggregator device, and then that
> process crashes, what happens when you try to restart the process and
> run e.g. your VM again?
>
> Time for a hard reboot? Or should we add some design guidelines for
> these machines so that they can cleanly tear down aggregators
> previously created by the crashed VM?

No, the VM does not create the aggregator.

The idea is for the user to create one or more aggregators, set up
permissions on /dev/gpiochipX, and launch the VM, passing the aggregated
/dev/gpiochipX as parameters.
If the VM crashes, just launch it again.

Destroying the aggregators is a manual and independent process, after
the VM has exited.

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 5/7] gpio: Add GPIO Aggregator/Repeater driver
  2019-12-12 14:34   ` Linus Walleij
@ 2019-12-12 15:24     ` Geert Uytterhoeven
  2020-01-04  0:38       ` Linus Walleij
  0 siblings, 1 reply; 48+ messages in thread
From: Geert Uytterhoeven @ 2019-12-12 15:24 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Geert Uytterhoeven, Bartosz Golaszewski, Jonathan Corbet,
	Rob Herring, Mark Rutland, Harish Jenny K N, Eugeniu Rosca,
	Alexander Graf, Peter Maydell, Paolo Bonzini, Phil Reid,
	Marc Zyngier, Christoffer Dall, Magnus Damm,
	open list:GPIO SUBSYSTEM, Linux Doc Mailing List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-kernel, QEMU Developers

Hi Linus,

On Thu, Dec 12, 2019 at 3:34 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Nov 27, 2019 at 9:43 AM Geert Uytterhoeven
> <geert+renesas@glider.be> wrote:
> > GPIO controllers are exported to userspace using /dev/gpiochip*
> > character devices.  Access control to these devices is provided by
> > standard UNIX file system permissions, on an all-or-nothing basis:
> > either a GPIO controller is accessible for a user, or it is not.
> > Currently no mechanism exists to control access to individual GPIOs.
> >
> > Hence add a GPIO driver to aggregate existing GPIOs, and expose them as
> > a new gpiochip.
> >
> > This supports the following use cases:
> >   1. Aggregating GPIOs using Sysfs
> >      This is useful for implementing access control, and assigning a set
> >      of GPIOs to a specific user or virtual machine.
> >
> >   2. GPIO Repeater in Device Tree
> >      This supports modelling e.g. GPIO inverters in DT.
> >
> >   3. Generic GPIO Driver
> >      This provides userspace access to a simple GPIO-operated device
> >      described in DT, cfr. e.g. spidev for SPI-operated devices.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> Overall I like how this is developing!
>
> > +config GPIO_AGGREGATOR
> > +       tristate "GPIO Aggregator/Repeater"
> > +       help
> > +         Say yes here to enable the GPIO Aggregator and repeater, which
> > +         provides a way to aggregate and/or repeat existing GPIOs into a new
> > +         GPIO device.
>
> Should it say a "new virtual GPIO chip"?

OK.

> > +         This can serve the following purposes:
> > +           1. Assign a collection of GPIOs to a user, or export them to a
> > +              virtual machine,
>
> This is ambiguous. What is a "user"? A process calling from
> userspace? A device tree node?

A user is an entity with a UID, typically listed in /etc/passwd.
This is similar to letting some, not all, people on the machine access
the CD-ROM drive.

> I would write "assign a collection of GPIO lines from any lines on
> existing physical GPIO chips to form a new virtual GPIO chip"
>
> That should be to the point, right?

Yes, that's WHAT it does. The WHY is the granular access control.

> > +           2. Support GPIOs that are connected to a physical inverter,
>
> s/to/through/g

OK.

> > +           3. Provide a generic driver for a GPIO-operated device, to be
> > +               controlled from userspace using the GPIO chardev interface.
>
> I don't understand this, it needs to be elaborated. What is meant
> by a "GPIO-operated device" in this context? Example?

E.g. a motor. Or a door opener.

        door-opener {
                compatible = "mydoor,opener";

                gpios = <&gpio2 19 GPIO_ACTIVE_HIGH>;
        };

You don't need a full-featured kernel driver for that, so just bind the
gpio-aggregator to the door-opener, and control it through libgpiod.

> I consistently use the term "GPIO line" as opposed to "GPIO"
> or "GPIO number" etc that are abigous, so please rephrase using
> "GPIO lines" rather than just "GPIOs" above.

OK.

> > +#include "gpiolib.h"
>
> Whenever this is included in a driver I want it to come with a comment
> explicitly stating exactly why and which internal symbols the driver
> needs to access. Ideally all drivers should just need <linux/gpio/driver.h>...

"gpiolib.h" is needed to access gpio_desc.gdev->chip in
gpio_fwd_set_config().  And for gpio_chip_hwgpio() (see below).

But indeed, I should add #include <linux/gpio/consumer.h>, for e.g. the
various gpiod_[gs]et_*() functions.

> > +static int aggr_add_gpio(struct gpio_aggregator *aggr, const char *label,
> > +                        int hwnum, unsigned int *n)
>
> u16 hwnum for the hardware number but if it is always -1/U16_MAX
> then why pass the parameter at all.
>
> Is "label" the right name of this parameter if that is going to actually
> be line_name then use that.

It's not always -1.
This function can be called either with a gpiochip label/name and an
offset, or a line-name and -1.

> > +{
> > +       struct gpiod_lookup_table *lookups;
> > +
> > +       lookups = krealloc(aggr->lookups, struct_size(lookups, table, *n + 2),
> > +                          GFP_KERNEL);
> > +       if (!lookups)
> > +               return -ENOMEM;
> > +
> > +       lookups->table[*n].chip_label = label;
>
> This is pending the discussion on whether to just use "key" for this
> name.

Which would require touching all users (board files and mfd drivers).

> > +       lookups->table[*n].chip_hwnum = hwnum;
>
> If this is always going to be U16_MAX (-1 in the current code)
> then it can just be assigned as that here instead of passed as
> parameter.

So it's not, see above.

> > +static int aggr_parse(struct gpio_aggregator *aggr)
> > +{
> > +       char *name, *offsets, *first, *last, *next;
> > +       unsigned int a, b, i, n = 0;
> > +       char *args = aggr->args;
> > +       int error;
> > +
> > +       for (name = get_arg(&args), offsets = get_arg(&args); name;
> > +            offsets = get_arg(&args)) {
> > +               if (IS_ERR(name)) {
> > +                       pr_err("Cannot get GPIO specifier: %ld\n",
> > +                              PTR_ERR(name));
> > +                       return PTR_ERR(name);
> > +               }
> > +
> > +               if (!isrange(offsets)) {
> > +                       /* Named GPIO line */
> > +                       error = aggr_add_gpio(aggr, name, -1, &n);
>
> So the third argument woule be U16_MAX here. Or not pass
> a parameter at all.
>
> But honestly, when I look at this I don't understand why you
> have to avoid so hard to use offsets for the GPIO lines on
> your aggregator?
>
> Just put a u16 ngpios in your
> struct gpio_aggregator and count it up every time you
> add some new offsets here and you have
> offset numbers for all your GPIO lines on the aggregator
> and you can just drop the patch for lookup up lines by line
> names.
>
> Is there something wrong with my reasoning here?

Yes, I think there is.
The offsets are not offsets on the aggregated gpiochip, but on the
original target gpiochip.

> At the pointe later when the lines are counted from the
> allocated lookups using gpiod_count() that will just figure
> out this number anyways, so it is not like we don't know
> it at the end of the day.
>
> So it seems the patch to gpiolib is just to use machine
> descriptor tables as a substitute for a simple counter
> variable in this local struct to me.

Nope, it's used for looking up the target GPIO lines.

> > +static void __exit gpio_aggregator_remove_all(void)
> > +{
> > +       mutex_lock(&gpio_aggregator_lock);
> > +       idr_for_each(&gpio_aggregator_idr, gpio_aggregator_idr_remove, NULL);
> > +       idr_destroy(&gpio_aggregator_idr);
> > +       mutex_unlock(&gpio_aggregator_lock);
> > +}
> > +
> > +
> > +       /*
> > +        *  Common GPIO Forwarder
> > +        */
> > +
>
> Nitpick: lots and weird spacing here.

OK.

> > +struct gpiochip_fwd {
> > +       struct gpio_chip chip;
> > +       struct gpio_desc **descs;
> > +       union {
> > +               struct mutex mlock;     /* protects tmp[] if can_sleep */
> > +               spinlock_t slock;       /* protects tmp[] if !can_sleep */
> > +       };
>
> That was a very elegant use of union!
>
> > +static int gpio_fwd_get_multiple(struct gpio_chip *chip, unsigned long *mask,
> > +                                unsigned long *bits)
> > +static void gpio_fwd_set_multiple(struct gpio_chip *chip, unsigned long *mask,
> > +                                 unsigned long *bits)
>
> I guess these can both be optimized to use get/set_multiple on
> the target chip if the offsets are consecutive?
>
> However that is going to be tricky so I'm not saying you should
> implement that. So for now, let's say just add a TODO: comment
> about it.

Doesn't gpiod_[gs]et_array_value() already call .[gs]et_multiple()?

> > +static int gpio_fwd_init_valid_mask(struct gpio_chip *chip,
> > +                                   unsigned long *valid_mask,
> > +                                   unsigned int ngpios)
> > +{
> > +       struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> > +       unsigned int i;
> > +
> > +       for (i = 0; i < ngpios; i++) {
> > +               if (!gpiochip_line_is_valid(fwd->descs[i]->gdev->chip,
> > +                                           gpio_chip_hwgpio(fwd->descs[i])))
> > +                       clear_bit(i, valid_mask);
> > +       }
>
> This is what uses "gpiolib.h" is it not?
>
> devm_gpiod_get_index() will not succeed if the line
> is not valid so I think this can be just dropped, since
> what you do before this is exactly devm_gpiod_get_index()
> on each line, then you call gpiochip_fwd_create()
> with the result.
>
> So I think you can just drop this entire function.
> This will not happen.

OK, if all lines are valid, the mask handling is indeed not needed.

> If it does happen, add a comment above this loop
> explaining which circumstances would make lines on
> the forwarder invalid.

OK, so cannot happen.

> > +       for (i = 0; i < ngpios; i++) {
> > +               dev_dbg(dev, "gpio %u => gpio-%d (%s)\n", i,
> > +                       desc_to_gpio(descs[i]), descs[i]->label ? : "?");
> > +
> > +               if (gpiod_cansleep(descs[i]))
> > +                       chip->can_sleep = true;
> > +               if (descs[i]->gdev->chip->set_config)
> > +                       chip->set_config = gpio_fwd_set_config;
> > +               if (descs[i]->gdev->chip->init_valid_mask)
> > +                       chip->init_valid_mask = gpio_fwd_init_valid_mask;
> > +       }
>
> I do not think you should need to inspect the init_valid_mask()
> as explained above.

OK.

> Add a comment above the loop that if any of the GPIO lines
> are sleeping then the entire forwarder will be sleeping
> and if any of the chips support .set_config() we will support
> setting configs.

OK.

> However the way that the .gpio_fwd_set_config() is coded
> it looks like you can just unconditionally assign it and
> only check the cansleep condition in this loop.

I wanted to avoid the overhead of calling into gpio_fwd_set_config() if
none of the targets gpiochips support .set_config(), see
gpiod_set_transitory().

> > +}
> > +
> > +
> > +       /*
> > +        *  Common GPIO Aggregator/Repeater platform device
> > +        */
> > +
>
> Nitpick: weird and excess spacing again.

Yeah, this dates back from when the aggregator, repeater, and
forwarder were all separate files and modules.

> > +       for (i = 0; i < n; i++) {
> > +               descs[i] = devm_gpiod_get_index(dev, NULL, i, GPIOD_ASIS);
> > +               if (IS_ERR(descs[i]))
> > +                       return PTR_ERR(descs[i]);
> > +       }
>
> If this succeeds none of the obtained gpio_desc:s can be
> invalid.

OK.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 6/7] docs: gpio: Add GPIO Aggregator/Repeater documentation
  2019-12-12 14:48     ` Geert Uytterhoeven
@ 2020-01-04  0:21       ` Linus Walleij
  2020-01-06  8:06         ` Geert Uytterhoeven
  0 siblings, 1 reply; 48+ messages in thread
From: Linus Walleij @ 2020-01-04  0:21 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Geert Uytterhoeven, Bartosz Golaszewski, Jonathan Corbet,
	Rob Herring, Mark Rutland, Harish Jenny K N, Eugeniu Rosca,
	Alexander Graf, Peter Maydell, Paolo Bonzini, Phil Reid,
	Marc Zyngier, Christoffer Dall, Magnus Damm,
	open list:GPIO SUBSYSTEM, Linux Doc Mailing List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-kernel, QEMU Developers

On Thu, Dec 12, 2019 at 3:48 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Thu, Dec 12, 2019 at 3:42 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Wed, Nov 27, 2019 at 9:43 AM Geert Uytterhoeven
> > <geert+renesas@glider.be> wrote:
> > > +The GPIO Aggregator allows access control for individual GPIOs, by aggregating
> > > +them into a new gpio_chip, which can be assigned to a group or user using
> > > +standard UNIX file ownership and permissions.  Furthermore, this simplifies and
> > > +hardens exporting GPIOs to a virtual machine, as the VM can just grab the full
> > > +GPIO controller, and no longer needs to care about which GPIOs to grab and
> > > +which not, reducing the attack surface.
> > > +
> > > +Aggregated GPIO controllers are instantiated and destroyed by writing to
> > > +write-only attribute files in sysfs.
> >
> > I suppose virtual machines will have a lengthy config file where
> > they specify which GPIO lines to pick and use for their GPIO
> > aggregator, and that will all be fine, the VM starts and the aggregator
> > is there and we can start executing.
> >
> > I would perhaps point out a weakness as with all sysfs and with the current
> > gpio sysfs: if a process creates an aggregator device, and then that
> > process crashes, what happens when you try to restart the process and
> > run e.g. your VM again?
> >
> > Time for a hard reboot? Or should we add some design guidelines for
> > these machines so that they can cleanly tear down aggregators
> > previously created by the crashed VM?
>
> No, the VM does not create the aggregator.
>
> The idea is for the user to create one or more aggregators, set up
> permissions on /dev/gpiochipX, and launch the VM, passing the aggregated
> /dev/gpiochipX as parameters.
> If the VM crashes, just launch it again.
>
> Destroying the aggregators is a manual and independent process, after
> the VM has exited.

I'm thinking about someone making some industrial application for some
control of a machinery say a robotic arm.

And do make sure this VM is only controlling these GPIOs related to
this robotic arm, they create a GPIO aggregator. And we care about
cases like that since we provide this security argument.

Surely that machine will be rebooted.

Surely they don't have a printed paper with all the commands lying
at the console, and asking whoever powers it back on to manually
type it all in again. That feels a bit 1981.

So they will have a script for this I suppose. Possibly in some
initscript so it is set up on boot. And this script echos stuff
all over the place to set up the aggregator.

Is this the use case you're thinking of?

I just like to have the whole picture here.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 5/7] gpio: Add GPIO Aggregator/Repeater driver
  2019-12-12 15:24     ` Geert Uytterhoeven
@ 2020-01-04  0:38       ` Linus Walleij
  2020-01-06  8:23         ` Geert Uytterhoeven
  0 siblings, 1 reply; 48+ messages in thread
From: Linus Walleij @ 2020-01-04  0:38 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Geert Uytterhoeven, Bartosz Golaszewski, Jonathan Corbet,
	Rob Herring, Mark Rutland, Harish Jenny K N, Eugeniu Rosca,
	Alexander Graf, Peter Maydell, Paolo Bonzini, Phil Reid,
	Marc Zyngier, Christoffer Dall, Magnus Damm,
	open list:GPIO SUBSYSTEM, Linux Doc Mailing List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-kernel, QEMU Developers

Sorry for slowness... christmas.

On Thu, Dec 12, 2019 at 4:24 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Thu, Dec 12, 2019 at 3:34 PM Linus Walleij <linus.walleij@linaro.org> wrote:

> > > +         This can serve the following purposes:
> > > +           1. Assign a collection of GPIOs to a user, or export them to a
> > > +              virtual machine,
> >
> > This is ambiguous. What is a "user"? A process calling from
> > userspace? A device tree node?
>
> A user is an entity with a UID, typically listed in /etc/passwd.
> This is similar to letting some, not all, people on the machine access
> the CD-ROM drive.

Ah I get it. Maybe we can say "assign permissions for a collection
of GPIOs to a user".

> > I would write "assign a collection of GPIO lines from any lines on
> > existing physical GPIO chips to form a new virtual GPIO chip"
> >
> > That should be to the point, right?
>
> Yes, that's WHAT it does. The WHY is the granular access control.

So I guess we can write both?

> > > +           3. Provide a generic driver for a GPIO-operated device, to be
> > > +               controlled from userspace using the GPIO chardev interface.
> >
> > I don't understand this, it needs to be elaborated. What is meant
> > by a "GPIO-operated device" in this context? Example?
>
> E.g. a motor. Or a door opener.
>
>         door-opener {
>                 compatible = "mydoor,opener";
>
>                 gpios = <&gpio2 19 GPIO_ACTIVE_HIGH>;
>         };
>
> You don't need a full-featured kernel driver for that, so just bind the
> gpio-aggregator to the door-opener, and control it through libgpiod.

Yep it's a perfect industrial control example, I get it.

Maybe we should blurb something about industrial control?

The rest I think we cleared out else I will see it when I review again.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 6/7] docs: gpio: Add GPIO Aggregator/Repeater documentation
  2020-01-04  0:21       ` Linus Walleij
@ 2020-01-06  8:06         ` Geert Uytterhoeven
  0 siblings, 0 replies; 48+ messages in thread
From: Geert Uytterhoeven @ 2020-01-06  8:06 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Geert Uytterhoeven, Bartosz Golaszewski, Jonathan Corbet,
	Rob Herring, Mark Rutland, Harish Jenny K N, Eugeniu Rosca,
	Alexander Graf, Peter Maydell, Paolo Bonzini, Phil Reid,
	Marc Zyngier, Christoffer Dall, Magnus Damm,
	open list:GPIO SUBSYSTEM, Linux Doc Mailing List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-kernel, QEMU Developers

Hi Linus,

On Sat, Jan 4, 2020 at 1:21 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Dec 12, 2019 at 3:48 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Thu, Dec 12, 2019 at 3:42 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> > > On Wed, Nov 27, 2019 at 9:43 AM Geert Uytterhoeven
> > > <geert+renesas@glider.be> wrote:
> > > > +The GPIO Aggregator allows access control for individual GPIOs, by aggregating
> > > > +them into a new gpio_chip, which can be assigned to a group or user using
> > > > +standard UNIX file ownership and permissions.  Furthermore, this simplifies and
> > > > +hardens exporting GPIOs to a virtual machine, as the VM can just grab the full
> > > > +GPIO controller, and no longer needs to care about which GPIOs to grab and
> > > > +which not, reducing the attack surface.
> > > > +
> > > > +Aggregated GPIO controllers are instantiated and destroyed by writing to
> > > > +write-only attribute files in sysfs.
> > >
> > > I suppose virtual machines will have a lengthy config file where
> > > they specify which GPIO lines to pick and use for their GPIO
> > > aggregator, and that will all be fine, the VM starts and the aggregator
> > > is there and we can start executing.
> > >
> > > I would perhaps point out a weakness as with all sysfs and with the current
> > > gpio sysfs: if a process creates an aggregator device, and then that
> > > process crashes, what happens when you try to restart the process and
> > > run e.g. your VM again?
> > >
> > > Time for a hard reboot? Or should we add some design guidelines for
> > > these machines so that they can cleanly tear down aggregators
> > > previously created by the crashed VM?
> >
> > No, the VM does not create the aggregator.
> >
> > The idea is for the user to create one or more aggregators, set up
> > permissions on /dev/gpiochipX, and launch the VM, passing the aggregated
> > /dev/gpiochipX as parameters.
> > If the VM crashes, just launch it again.
> >
> > Destroying the aggregators is a manual and independent process, after
> > the VM has exited.
>
> I'm thinking about someone making some industrial application for some
> control of a machinery say a robotic arm.
>
> And do make sure this VM is only controlling these GPIOs related to
> this robotic arm, they create a GPIO aggregator. And we care about
> cases like that since we provide this security argument.
>
> Surely that machine will be rebooted.
>
> Surely they don't have a printed paper with all the commands lying
> at the console, and asking whoever powers it back on to manually
> type it all in again. That feels a bit 1981.
>
> So they will have a script for this I suppose. Possibly in some
> initscript so it is set up on boot. And this script echos stuff
> all over the place to set up the aggregator.
>
> Is this the use case you're thinking of?

Exactly.

And they can configure that by echoing the GPIO specifiers to
/sys/bus/platform/drivers/gpio-aggregator/new_device.

If their system has DT, another option is to describe the device in DT,
and add its compatible value to gpio_aggregator_dt_ids[], cfr. the
frobnicator example.

> I just like to have the whole picture here.

Sure. If anything is still unclear, please let me know!
Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 4/7] dt-bindings: gpio: Add gpio-repeater bindings
  2019-12-06 15:03       ` Rob Herring
@ 2020-01-06  8:12         ` Geert Uytterhoeven
  2020-01-07  9:22           ` Harish Jenny K N
  0 siblings, 1 reply; 48+ messages in thread
From: Geert Uytterhoeven @ 2020-01-06  8:12 UTC (permalink / raw)
  To: Rob Herring
  Cc: Geert Uytterhoeven, Linus Walleij, Bartosz Golaszewski,
	Jonathan Corbet, Mark Rutland, Harish Jenny K N, Eugeniu Rosca,
	Alexander Graf, Peter Maydell, Paolo Bonzini, Phil Reid,
	Marc Zyngier, Christoffer Dall, Magnus Damm,
	open list:GPIO SUBSYSTEM, open list:DOCUMENTATION,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, Linux Kernel Mailing List, QEMU Developers

Hi Rob,

On Fri, Dec 6, 2019 at 4:04 PM Rob Herring <robh@kernel.org> wrote:
> On Fri, Dec 6, 2019 at 3:17 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Thu, Dec 5, 2019 at 10:06 PM Rob Herring <robh@kernel.org> wrote:
> > > On Wed, Nov 27, 2019 at 09:42:50AM +0100, Geert Uytterhoeven wrote:
> > > > Add Device Tree bindings for a GPIO repeater, with optional translation
> > > > of physical signal properties.  This is useful for describing explicitly
> > > > the presence of e.g. an inverter on a GPIO line, and was inspired by the
> > > > non-YAML gpio-inverter bindings by Harish Jenny K N
> > > > <harish_kandiga@mentor.com>[1].
> > > >
> > > > Note that this is different from a GPIO Nexus Node[2], which cannot do
> > > > physical signal property translation.
> > >
> > > It can't? Why not? The point of the passthru mask is to not do
> > > translation of flags, but without it you are always doing translation of
> > > cells.
> >
> > Thanks for pushing me deeper into nexuses!
> > You're right, you can map from one type to another.
> > However, you cannot handle the "double inversion" of an ACTIVE_LOW
> > signal with a physical inverter added:
> >
> >         nexus: led-nexus {
> >                 #gpio-cells = <2>;
> >                 gpio-map = <0 0 &gpio2 19 GPIO_ACTIVE_LOW>,     // inverted
> >                            <1 0 &gpio2 20 GPIO_ACTIVE_HIGH>,    // noninverted
> >                            <2 0 &gpio2 21 GPIO_ACTIVE_LOW>;     // inverted
> >                 gpio-map-mask = <3 0>;
> >                 // default gpio-map-pass-thru = <0 0>;
> >         };
> >
> >         leds {
> >                 compatible = "gpio-leds";
> >                 led6-inverted {
> >                         gpios = <&nexus 0 GPIO_ACTIVE_HIGH>;
> >                 };
> >                 led7-noninverted {
> >                         gpios = <&nexus 1 GPIO_ACTIVE_HIGH>;
> >                 };
> >                 led8-double-inverted {  // FAILS: still inverted
> >                         gpios = <&nexus 2 GPIO_ACTIVE_LOW>;
> >                 };
> >         };
> >
> > It "works" if the last entry in gpio-map is changed to GPIO_ACTIVE_HIGH.
> > Still, the consumer would see the final translated polarity, and not the
> > actual one it needs to program the consumer for.
>
> I'm not really following. Why isn't a double inversion just the same
> as no inversion?

Because the nexus can only mask and/or substitute bits.
It cannot do a XOR operation on the GPIO flags.

> > > > While an inverter can be described implicitly by exchanging the
> > > > GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW flags, this has its limitations.
> > > > Each GPIO line has only a single GPIO_ACTIVE_* flag, but applies to both
> > > > th provider and consumer sides:
> > > >   1. The GPIO provider (controller) looks at the flags to know the
> > > >      polarity, so it can translate between logical (active/not active)
> > > >      and physical (high/low) signal levels.
> > > >   2. While the signal polarity is usually fixed on the GPIO consumer
> > > >      side (e.g. an LED is tied to either the supply voltage or GND),
> > > >      it may be configurable on some devices, and both sides need to
> > > >      agree.  Hence the GPIO_ACTIVE_* flag as seen by the consumer must
> > > >      match the actual polarity.
> > > >      There exists a similar issue with interrupt flags, where both the
> > > >      interrupt controller and the device generating the interrupt need
> > > >      to agree, which breaks in the presence of a physical inverter not
> > > >      described in DT (see e.g. [3]).
> > >
> > > Adding an inverted flag as I've suggested would also solve this issue.
> >
> > As per your suggestion in "Re: [PATCH V4 2/2] gpio: inverter: document
> > the inverter bindings"?
> > https://lore.kernel.org/linux-devicetree/CAL_JsqLp___2O-naU+2PPQy0QmJX6+aN3hByz-OB9+qFvWgN9Q@mail.gmail.com/
> >
> > Oh, now I understand. I was misguided by Harish' interpretation
> > https://lore.kernel.org/linux-devicetree/dde73334-a26d-b53f-6b97-4101c1cdc185@mentor.com/
> > which assumed an "inverted" property, e.g.
> >
> >     inverted = /bits/ 8 <0 0 0 0 0 0 1 0 0 0 0 0 0 0 0 0 0 0>;
> >
> > But you actually meant a new GPIO_INVERTED flag, to be ORed into the 2nd
> > cell of a GPIO specifier? I.e. add to include/dt-bindings/gpio/gpio.h"
> >
> >     /* Bit 6 expresses the presence of a physical inverter */
> >     #define GPIO_INVERTED 64
>
> Exactly.

OK, makes sense.

> > We need to be very careful in defining to which side the GPIO_ACTIVE_*
> > applies to (consumer?), and which side the GPIO_INVERTED flag (provider?).
> > Still, this doesn't help if e.g. a FET is used instead of a push-pull
> > inverter, as the former needs translation of other flags (which the
> > nexus can do, the caveats above still applies, though).
>
> Yes. Historically the cells values are meaningful to the provider and
> opaque to the consumer. Standardized cell values changes that
> somewhat. I think we want the active flag to be from the provider's
> prospective because the provider always needs to know. The consumer
> often doesn't need to know. That also means things work without the
> GPIO_INVERTED flag if the consumer doesn't care which is what we have
> today already and we can't go back in time.
>
>
> > Same for adding IRQ_TYPE_INVERTED.
>
> I suppose so, yes.
>
> > Related issue: how to handle physical inverters on SPI chip select lines,
> > if the SPI slave can be configured for both polarities?
>
> Good question. Perhaps in a different way because we have to handle
> both h/w controlled and gpio chip selects.
>
> However, how would one configure the polarity in the device in the
> first place? You have to assert the CS first to give a command to
> reprogram it.

That's indeed true for a simple SPI slave.
But if it is a smarter device (e.g. a generic micro controller), it may use the
system's DTB to configure itself.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 5/7] gpio: Add GPIO Aggregator/Repeater driver
  2020-01-04  0:38       ` Linus Walleij
@ 2020-01-06  8:23         ` Geert Uytterhoeven
  2020-01-08 23:12           ` Linus Walleij
  0 siblings, 1 reply; 48+ messages in thread
From: Geert Uytterhoeven @ 2020-01-06  8:23 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Geert Uytterhoeven, Bartosz Golaszewski, Jonathan Corbet,
	Rob Herring, Mark Rutland, Harish Jenny K N, Eugeniu Rosca,
	Alexander Graf, Peter Maydell, Paolo Bonzini, Phil Reid,
	Marc Zyngier, Christoffer Dall, Magnus Damm,
	open list:GPIO SUBSYSTEM, Linux Doc Mailing List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-kernel, QEMU Developers

Hi Linus,

On Sat, Jan 4, 2020 at 1:38 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> Sorry for slowness... christmas.

Np. Happy New Year!

> On Thu, Dec 12, 2019 at 4:24 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Thu, Dec 12, 2019 at 3:34 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> > > > +         This can serve the following purposes:
> > > > +           1. Assign a collection of GPIOs to a user, or export them to a
> > > > +              virtual machine,
> > >
> > > This is ambiguous. What is a "user"? A process calling from
> > > userspace? A device tree node?
> >
> > A user is an entity with a UID, typically listed in /etc/passwd.
> > This is similar to letting some, not all, people on the machine access
> > the CD-ROM drive.
>
> Ah I get it. Maybe we can say "assign permissions for a collection
> of GPIOs to a user".

OK

> > > I would write "assign a collection of GPIO lines from any lines on
> > > existing physical GPIO chips to form a new virtual GPIO chip"
> > >
> > > That should be to the point, right?
> >
> > Yes, that's WHAT it does. The WHY is the granular access control.
>
> So I guess we can write both?

OK.

> > > > +           3. Provide a generic driver for a GPIO-operated device, to be
> > > > +               controlled from userspace using the GPIO chardev interface.
> > >
> > > I don't understand this, it needs to be elaborated. What is meant
> > > by a "GPIO-operated device" in this context? Example?
> >
> > E.g. a motor. Or a door opener.
> >
> >         door-opener {
> >                 compatible = "mydoor,opener";
> >
> >                 gpios = <&gpio2 19 GPIO_ACTIVE_HIGH>;
> >         };
> >
> > You don't need a full-featured kernel driver for that, so just bind the
> > gpio-aggregator to the door-opener, and control it through libgpiod.
>
> Yep it's a perfect industrial control example, I get it.
>
> Maybe we should blurb something about industrial control?

OK.

> The rest I think we cleared out else I will see it when I review again.

The remaining discussion point is "GPIO Repeater in Device Tree", i.e.
the GPIO inverter usecase, which might be solved better by adding a
GPIO_INVERTED flag.

Shall I rip that out, incorporate review comments, and report?

Thanks!



--
Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 4/7] dt-bindings: gpio: Add gpio-repeater bindings
  2020-01-06  8:12         ` Geert Uytterhoeven
@ 2020-01-07  9:22           ` Harish Jenny K N
  2020-01-16  5:09             ` Harish Jenny K N
  0 siblings, 1 reply; 48+ messages in thread
From: Harish Jenny K N @ 2020-01-07  9:22 UTC (permalink / raw)
  To: Geert Uytterhoeven, Rob Herring
  Cc: Geert Uytterhoeven, Linus Walleij, Bartosz Golaszewski,
	Jonathan Corbet, Mark Rutland, Eugeniu Rosca, Alexander Graf,
	Peter Maydell, Paolo Bonzini, Phil Reid, Marc Zyngier,
	Christoffer Dall, Magnus Damm, open list:GPIO SUBSYSTEM,
	open list:DOCUMENTATION,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, Linux Kernel Mailing List, QEMU Developers


On 06/01/20 1:42 PM, Geert Uytterhoeven wrote:
> Hi Rob,
>
> On Fri, Dec 6, 2019 at 4:04 PM Rob Herring <robh@kernel.org> wrote:
>> On Fri, Dec 6, 2019 at 3:17 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> On Thu, Dec 5, 2019 at 10:06 PM Rob Herring <robh@kernel.org> wrote:
>>>> On Wed, Nov 27, 2019 at 09:42:50AM +0100, Geert Uytterhoeven wrote:
>>>>> Add Device Tree bindings for a GPIO repeater, with optional translation
>>>>> of physical signal properties.  This is useful for describing explicitly
>>>>> the presence of e.g. an inverter on a GPIO line, and was inspired by the
>>>>> non-YAML gpio-inverter bindings by Harish Jenny K N
>>>>> <harish_kandiga@mentor.com>[1].
>>>>>
>>>>> Note that this is different from a GPIO Nexus Node[2], which cannot do
>>>>> physical signal property translation.
>>>> It can't? Why not? The point of the passthru mask is to not do
>>>> translation of flags, but without it you are always doing translation of
>>>> cells.
>>> Thanks for pushing me deeper into nexuses!
>>> You're right, you can map from one type to another.
>>> However, you cannot handle the "double inversion" of an ACTIVE_LOW
>>> signal with a physical inverter added:
>>>
>>>         nexus: led-nexus {
>>>                 #gpio-cells = <2>;
>>>                 gpio-map = <0 0 &gpio2 19 GPIO_ACTIVE_LOW>,     // inverted
>>>                            <1 0 &gpio2 20 GPIO_ACTIVE_HIGH>,    // noninverted
>>>                            <2 0 &gpio2 21 GPIO_ACTIVE_LOW>;     // inverted
>>>                 gpio-map-mask = <3 0>;
>>>                 // default gpio-map-pass-thru = <0 0>;
>>>         };
>>>
>>>         leds {
>>>                 compatible = "gpio-leds";
>>>                 led6-inverted {
>>>                         gpios = <&nexus 0 GPIO_ACTIVE_HIGH>;
>>>                 };
>>>                 led7-noninverted {
>>>                         gpios = <&nexus 1 GPIO_ACTIVE_HIGH>;
>>>                 };
>>>                 led8-double-inverted {  // FAILS: still inverted
>>>                         gpios = <&nexus 2 GPIO_ACTIVE_LOW>;
>>>                 };
>>>         };
>>>
>>> It "works" if the last entry in gpio-map is changed to GPIO_ACTIVE_HIGH.
>>> Still, the consumer would see the final translated polarity, and not the
>>> actual one it needs to program the consumer for.
>> I'm not really following. Why isn't a double inversion just the same
>> as no inversion?
> Because the nexus can only mask and/or substitute bits.
> It cannot do a XOR operation on the GPIO flags.
>
>>>>> While an inverter can be described implicitly by exchanging the
>>>>> GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW flags, this has its limitations.
>>>>> Each GPIO line has only a single GPIO_ACTIVE_* flag, but applies to both
>>>>> th provider and consumer sides:
>>>>>   1. The GPIO provider (controller) looks at the flags to know the
>>>>>      polarity, so it can translate between logical (active/not active)
>>>>>      and physical (high/low) signal levels.
>>>>>   2. While the signal polarity is usually fixed on the GPIO consumer
>>>>>      side (e.g. an LED is tied to either the supply voltage or GND),
>>>>>      it may be configurable on some devices, and both sides need to
>>>>>      agree.  Hence the GPIO_ACTIVE_* flag as seen by the consumer must
>>>>>      match the actual polarity.
>>>>>      There exists a similar issue with interrupt flags, where both the
>>>>>      interrupt controller and the device generating the interrupt need
>>>>>      to agree, which breaks in the presence of a physical inverter not
>>>>>      described in DT (see e.g. [3]).
>>>> Adding an inverted flag as I've suggested would also solve this issue.
>>> As per your suggestion in "Re: [PATCH V4 2/2] gpio: inverter: document
>>> the inverter bindings"?
>>> https://lore.kernel.org/linux-devicetree/CAL_JsqLp___2O-naU+2PPQy0QmJX6+aN3hByz-OB9+qFvWgN9Q@mail.gmail.com/
>>>
>>> Oh, now I understand. I was misguided by Harish' interpretation
>>> https://lore.kernel.org/linux-devicetree/dde73334-a26d-b53f-6b97-4101c1cdc185@mentor.com/
>>> which assumed an "inverted" property, e.g.
>>>
>>>     inverted = /bits/ 8 <0 0 0 0 0 0 1 0 0 0 0 0 0 0 0 0 0 0>;
>>>
>>> But you actually meant a new GPIO_INVERTED flag, to be ORed into the 2nd
>>> cell of a GPIO specifier? I.e. add to include/dt-bindings/gpio/gpio.h"
>>>
>>>     /* Bit 6 expresses the presence of a physical inverter */
>>>     #define GPIO_INVERTED 64
>> Exactly.
> OK, makes sense.


The reason I went for "inverted" property is because, we can specify this for gpios at provider side.

The usecase needed to define the polarity which did not have kernel space consumer driver.


I am not sure how do we achieve this using GPIO_INVERTED flag. We need some sort of node/gpio-hog to specify these

type of properties? Otherwise gpio-pin will be held by kernel or the module using the hog property and the user space application will not be able to access pin.


or please let me know if I am missing something.


>
>>> We need to be very careful in defining to which side the GPIO_ACTIVE_*
>>> applies to (consumer?), and which side the GPIO_INVERTED flag (provider?).
>>> Still, this doesn't help if e.g. a FET is used instead of a push-pull
>>> inverter, as the former needs translation of other flags (which the
>>> nexus can do, the caveats above still applies, though).
>> Yes. Historically the cells values are meaningful to the provider and
>> opaque to the consumer. Standardized cell values changes that
>> somewhat. I think we want the active flag to be from the provider's
>> prospective because the provider always needs to know. The consumer
>> often doesn't need to know. That also means things work without the
>> GPIO_INVERTED flag if the consumer doesn't care which is what we have
>> today already and we can't go back in time.
>>

Things will work without GPIO_INVERTED flag for consumers which can specify GPIO_ACTIVE_* flags.



>>> Same for adding IRQ_TYPE_INVERTED.
>> I suppose so, yes.
>>
>>> Related issue: how to handle physical inverters on SPI chip select lines,
>>> if the SPI slave can be configured for both polarities?
>> Good question. Perhaps in a different way because we have to handle
>> both h/w controlled and gpio chip selects.
>>
>> However, how would one configure the polarity in the device in the
>> first place? You have to assert the CS first to give a command to
>> reprogram it.
> That's indeed true for a simple SPI slave.
> But if it is a smarter device (e.g. a generic micro controller), it may use the
> system's DTB to configure itself.
>
> Gr{oetje,eeting}s,
>
>                         Geert
>

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 5/7] gpio: Add GPIO Aggregator/Repeater driver
  2020-01-06  8:23         ` Geert Uytterhoeven
@ 2020-01-08 23:12           ` Linus Walleij
  0 siblings, 0 replies; 48+ messages in thread
From: Linus Walleij @ 2020-01-08 23:12 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Geert Uytterhoeven, Bartosz Golaszewski, Jonathan Corbet,
	Rob Herring, Mark Rutland, Harish Jenny K N, Eugeniu Rosca,
	Alexander Graf, Peter Maydell, Paolo Bonzini, Phil Reid,
	Marc Zyngier, Christoffer Dall, Magnus Damm,
	open list:GPIO SUBSYSTEM, Linux Doc Mailing List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-kernel, QEMU Developers

On Mon, Jan 6, 2020 at 9:23 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > The rest I think we cleared out else I will see it when I review again.
>
> The remaining discussion point is "GPIO Repeater in Device Tree", i.e.
> the GPIO inverter usecase, which might be solved better by adding a
> GPIO_INVERTED flag.
>
> Shall I rip that out, incorporate review comments, and report?

Don't know, if it blocks progress I guess the diplomatic solution is to
do that, divide and conquer. But review is a social process so I don't really
know the right strategy.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 5/7] gpio: Add GPIO Aggregator/Repeater driver
  2019-12-03 10:51   ` Eugeniu Rosca
@ 2020-01-09 13:35     ` Geert Uytterhoeven
  2020-01-09 13:49       ` Eugeniu Rosca
  0 siblings, 1 reply; 48+ messages in thread
From: Geert Uytterhoeven @ 2020-01-09 13:35 UTC (permalink / raw)
  To: Eugeniu Rosca
  Cc: Geert Uytterhoeven, Linus Walleij, Bartosz Golaszewski,
	Jonathan Corbet, Rob Herring, Mark Rutland, Harish Jenny K N,
	Eugeniu Rosca, Alexander Graf, Peter Maydell, Paolo Bonzini,
	Phil Reid, Marc Zyngier, Christoffer Dall, Magnus Damm,
	open list:GPIO SUBSYSTEM, open list:DOCUMENTATION,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, Linux Kernel Mailing List, QEMU Developers

Hi Eugeniu,

On Tue, Dec 3, 2019 at 11:51 AM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
> On Wed, Nov 27, 2019 at 09:42:51AM +0100, Geert Uytterhoeven wrote:
> > +static int gpio_aggregator_probe(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct gpio_desc **descs;
> > +     struct gpiochip_fwd *fwd;
> > +     int i, n;
>
> FWIW/FTR, doing some blind creation and deletion of gpio aggregator
> chips [1] on R-Car H3ULCB overnight, kmemleak reported once [2]. Not
> sure this is something 100% reproducible.
>
> [1] while true; do \
>    echo e6055400.gpio 12,13 > /sys/bus/platform/drivers/gpio-aggregator/new_device; \
>    echo gpio-aggregator.0 > /sys/bus/platform/drivers/gpio-aggregator/delete_device; \
>    done
>
> [2] unreferenced object 0xffff0006d2c2e000 (size 128):
>   comm "kworker/3:1", pid 55, jiffies 4294676978 (age 38546.676s)
>   hex dump (first 32 bytes):
>     00 d9 d2 d3 06 00 ff ff 0c 00 e0 0f ff ff ff ff  ................
>     01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>   backtrace:
>     [<00000000a8e18c13>] slab_post_alloc_hook+0x8c/0x94
>     [<000000006f419a4f>] __kmalloc+0x170/0x218
>     [<0000000060d185ea>] kobj_map+0x78/0x1c0
>     [<00000000c96645f3>] cdev_add+0x68/0x94
>     [<00000000a7a5a8ac>] cdev_device_add+0x74/0x90
>     [<00000000497871d3>] gpiochip_setup_dev+0x84/0x1f0
>     [<00000000b993f95f>] gpiochip_add_data_with_key+0xbcc/0x11f0
>     [<00000000fd728c0e>] devm_gpiochip_add_data+0x60/0xa8
>     [<00000000442e34c1>] gpio_aggregator_probe+0x210/0x3c8
>     [<00000000076e13fb>] platform_drv_probe+0x70/0xe4
>     [<00000000de84b58b>] really_probe+0x2d8/0x434
>     [<00000000c95c9784>] driver_probe_device+0x15c/0x16c
>     [<00000000afb7dd4f>] __device_attach_driver+0xdc/0x120
>     [<00000000efa40cae>] bus_for_each_drv+0x12c/0x154
>     [<00000000c149acef>] __device_attach+0x148/0x1e0
>     [<00000000a74fd158>] device_initial_probe+0x24/0x30

This is the allocation of the GPIO character device, which is allocated
in response to the creation of the GPIO chip, from .probe().
As that is done using devm_gpiochip_add_data(), the chardev should be
deallocated automatically by devm_gpio_chip_release() when
platform_device_unregister() is called.

Weird...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 5/7] gpio: Add GPIO Aggregator/Repeater driver
  2020-01-09 13:35     ` Geert Uytterhoeven
@ 2020-01-09 13:49       ` Eugeniu Rosca
  0 siblings, 0 replies; 48+ messages in thread
From: Eugeniu Rosca @ 2020-01-09 13:49 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Eugeniu Rosca, Geert Uytterhoeven, Linus Walleij,
	Bartosz Golaszewski, Jonathan Corbet, Rob Herring, Mark Rutland,
	Harish Jenny K N, Eugeniu Rosca, Alexander Graf, Peter Maydell,
	Paolo Bonzini, Phil Reid, Marc Zyngier, Christoffer Dall,
	Magnus Damm, open list:GPIO SUBSYSTEM, open list:DOCUMENTATION,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, Linux Kernel Mailing List, QEMU Developers

Hi Geert,

On Thu, Jan 09, 2020 at 02:35:10PM +0100, Geert Uytterhoeven wrote:
> On Tue, Dec 3, 2019 at 11:51 AM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
> >
> > FWIW/FTR, doing some blind creation and deletion of gpio aggregator
> > chips [1] on R-Car H3ULCB overnight, kmemleak reported once [2]. Not
> > sure this is something 100% reproducible.
> >
> > [1] while true; do \
> >    echo e6055400.gpio 12,13 > /sys/bus/platform/drivers/gpio-aggregator/new_device; \
> >    echo gpio-aggregator.0 > /sys/bus/platform/drivers/gpio-aggregator/delete_device; \
> >    done
> >
> > [2] unreferenced object 0xffff0006d2c2e000 (size 128):
> >   comm "kworker/3:1", pid 55, jiffies 4294676978 (age 38546.676s)
> >   hex dump (first 32 bytes):
> >     00 d9 d2 d3 06 00 ff ff 0c 00 e0 0f ff ff ff ff  ................
> >     01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
> >   backtrace:
> >     [<00000000a8e18c13>] slab_post_alloc_hook+0x8c/0x94
> >     [<000000006f419a4f>] __kmalloc+0x170/0x218
> >     [<0000000060d185ea>] kobj_map+0x78/0x1c0
> >     [<00000000c96645f3>] cdev_add+0x68/0x94
> >     [<00000000a7a5a8ac>] cdev_device_add+0x74/0x90
> >     [<00000000497871d3>] gpiochip_setup_dev+0x84/0x1f0
> >     [<00000000b993f95f>] gpiochip_add_data_with_key+0xbcc/0x11f0
> >     [<00000000fd728c0e>] devm_gpiochip_add_data+0x60/0xa8
> >     [<00000000442e34c1>] gpio_aggregator_probe+0x210/0x3c8
> >     [<00000000076e13fb>] platform_drv_probe+0x70/0xe4
> >     [<00000000de84b58b>] really_probe+0x2d8/0x434
> >     [<00000000c95c9784>] driver_probe_device+0x15c/0x16c
> >     [<00000000afb7dd4f>] __device_attach_driver+0xdc/0x120
> >     [<00000000efa40cae>] bus_for_each_drv+0x12c/0x154
> >     [<00000000c149acef>] __device_attach+0x148/0x1e0
> >     [<00000000a74fd158>] device_initial_probe+0x24/0x30
> 
> This is the allocation of the GPIO character device, which is allocated
> in response to the creation of the GPIO chip, from .probe().
> As that is done using devm_gpiochip_add_data(), the chardev should be
> deallocated automatically by devm_gpio_chip_release() when
> platform_device_unregister() is called.
> 
> Weird...

It might have been a false positive. Kmemleak is not w/o flaws.
I will retest and report later. In any case, it does not look
severe to me.

-- 
Best Regards,
Eugeniu

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 4/7] dt-bindings: gpio: Add gpio-repeater bindings
  2020-01-07  9:22           ` Harish Jenny K N
@ 2020-01-16  5:09             ` Harish Jenny K N
  0 siblings, 0 replies; 48+ messages in thread
From: Harish Jenny K N @ 2020-01-16  5:09 UTC (permalink / raw)
  To: Geert Uytterhoeven, Rob Herring
  Cc: Geert Uytterhoeven, Linus Walleij, Bartosz Golaszewski,
	Jonathan Corbet, Mark Rutland, Eugeniu Rosca, Alexander Graf,
	Peter Maydell, Paolo Bonzini, Phil Reid, Marc Zyngier,
	Christoffer Dall, Magnus Damm, open list:GPIO SUBSYSTEM,
	open list:DOCUMENTATION,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, Linux Kernel Mailing List, QEMU Developers

Hi Linus,


On 07/01/20 2:52 PM, Harish Jenny K N wrote:
> On 06/01/20 1:42 PM, Geert Uytterhoeven wrote:
>> Hi Rob,
>>
>> On Fri, Dec 6, 2019 at 4:04 PM Rob Herring <robh@kernel.org> wrote:
>>> On Fri, Dec 6, 2019 at 3:17 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>> On Thu, Dec 5, 2019 at 10:06 PM Rob Herring <robh@kernel.org> wrote:
>>>>> On Wed, Nov 27, 2019 at 09:42:50AM +0100, Geert Uytterhoeven wrote:
>>>>>> Add Device Tree bindings for a GPIO repeater, with optional translation
>>>>>> of physical signal properties.  This is useful for describing explicitly
>>>>>> the presence of e.g. an inverter on a GPIO line, and was inspired by the
>>>>>> non-YAML gpio-inverter bindings by Harish Jenny K N
>>>>>> <harish_kandiga@mentor.com>[1].
>>>>>>
>>>>>> Note that this is different from a GPIO Nexus Node[2], which cannot do
>>>>>> physical signal property translation.
>>>>> It can't? Why not? The point of the passthru mask is to not do
>>>>> translation of flags, but without it you are always doing translation of
>>>>> cells.
>>>> Thanks for pushing me deeper into nexuses!
>>>> You're right, you can map from one type to another.
>>>> However, you cannot handle the "double inversion" of an ACTIVE_LOW
>>>> signal with a physical inverter added:
>>>>
>>>>         nexus: led-nexus {
>>>>                 #gpio-cells = <2>;
>>>>                 gpio-map = <0 0 &gpio2 19 GPIO_ACTIVE_LOW>,     // inverted
>>>>                            <1 0 &gpio2 20 GPIO_ACTIVE_HIGH>,    // noninverted
>>>>                            <2 0 &gpio2 21 GPIO_ACTIVE_LOW>;     // inverted
>>>>                 gpio-map-mask = <3 0>;
>>>>                 // default gpio-map-pass-thru = <0 0>;
>>>>         };
>>>>
>>>>         leds {
>>>>                 compatible = "gpio-leds";
>>>>                 led6-inverted {
>>>>                         gpios = <&nexus 0 GPIO_ACTIVE_HIGH>;
>>>>                 };
>>>>                 led7-noninverted {
>>>>                         gpios = <&nexus 1 GPIO_ACTIVE_HIGH>;
>>>>                 };
>>>>                 led8-double-inverted {  // FAILS: still inverted
>>>>                         gpios = <&nexus 2 GPIO_ACTIVE_LOW>;
>>>>                 };
>>>>         };
>>>>
>>>> It "works" if the last entry in gpio-map is changed to GPIO_ACTIVE_HIGH.
>>>> Still, the consumer would see the final translated polarity, and not the
>>>> actual one it needs to program the consumer for.
>>> I'm not really following. Why isn't a double inversion just the same
>>> as no inversion?
>> Because the nexus can only mask and/or substitute bits.
>> It cannot do a XOR operation on the GPIO flags.
>>
>>>>>> While an inverter can be described implicitly by exchanging the
>>>>>> GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW flags, this has its limitations.
>>>>>> Each GPIO line has only a single GPIO_ACTIVE_* flag, but applies to both
>>>>>> th provider and consumer sides:
>>>>>>   1. The GPIO provider (controller) looks at the flags to know the
>>>>>>      polarity, so it can translate between logical (active/not active)
>>>>>>      and physical (high/low) signal levels.
>>>>>>   2. While the signal polarity is usually fixed on the GPIO consumer
>>>>>>      side (e.g. an LED is tied to either the supply voltage or GND),
>>>>>>      it may be configurable on some devices, and both sides need to
>>>>>>      agree.  Hence the GPIO_ACTIVE_* flag as seen by the consumer must
>>>>>>      match the actual polarity.
>>>>>>      There exists a similar issue with interrupt flags, where both the
>>>>>>      interrupt controller and the device generating the interrupt need
>>>>>>      to agree, which breaks in the presence of a physical inverter not
>>>>>>      described in DT (see e.g. [3]).
>>>>> Adding an inverted flag as I've suggested would also solve this issue.
>>>> As per your suggestion in "Re: [PATCH V4 2/2] gpio: inverter: document
>>>> the inverter bindings"?
>>>> https://lore.kernel.org/linux-devicetree/CAL_JsqLp___2O-naU+2PPQy0QmJX6+aN3hByz-OB9+qFvWgN9Q@mail.gmail.com/
>>>>
>>>> Oh, now I understand. I was misguided by Harish' interpretation
>>>> https://lore.kernel.org/linux-devicetree/dde73334-a26d-b53f-6b97-4101c1cdc185@mentor.com/
>>>> which assumed an "inverted" property, e.g.
>>>>
>>>>     inverted = /bits/ 8 <0 0 0 0 0 0 1 0 0 0 0 0 0 0 0 0 0 0>;
>>>>
>>>> But you actually meant a new GPIO_INVERTED flag, to be ORed into the 2nd
>>>> cell of a GPIO specifier? I.e. add to include/dt-bindings/gpio/gpio.h"
>>>>
>>>>     /* Bit 6 expresses the presence of a physical inverter */
>>>>     #define GPIO_INVERTED 64
>>> Exactly.
>> OK, makes sense.
>
> The reason I went for "inverted" property is because, we can specify this for gpios at provider side.
>
> The usecase needed to define the polarity which did not have kernel space consumer driver.
>
>
> I am not sure how do we achieve this using GPIO_INVERTED flag. We need some sort of node/gpio-hog to specify these
>
> type of properties? Otherwise gpio-pin will be held by kernel or the module using the hog property and the user space application will not be able to access pin.
>
>
> or please let me know if I am missing something.
>
>
>>>> We need to be very careful in defining to which side the GPIO_ACTIVE_*
>>>> applies to (consumer?), and which side the GPIO_INVERTED flag (provider?).
>>>> Still, this doesn't help if e.g. a FET is used instead of a push-pull
>>>> inverter, as the former needs translation of other flags (which the
>>>> nexus can do, the caveats above still applies, though).
>>> Yes. Historically the cells values are meaningful to the provider and
>>> opaque to the consumer. Standardized cell values changes that
>>> somewhat. I think we want the active flag to be from the provider's
>>> prospective because the provider always needs to know. The consumer
>>> often doesn't need to know. That also means things work without the
>>> GPIO_INVERTED flag if the consumer doesn't care which is what we have
>>> today already and we can't go back in time.
>>>
> Things will work without GPIO_INVERTED flag for consumers which can specify GPIO_ACTIVE_* flags.
>
>
>
>>>> Same for adding IRQ_TYPE_INVERTED.
>>> I suppose so, yes.
>>>
>>>> Related issue: how to handle physical inverters on SPI chip select lines,
>>>> if the SPI slave can be configured for both polarities?
>>> Good question. Perhaps in a different way because we have to handle
>>> both h/w controlled and gpio chip selects.
>>>
>>> However, how would one configure the polarity in the device in the
>>> first place? You have to assert the CS first to give a command to
>>> reprogram it.
>> That's indeed true for a simple SPI slave.
>> But if it is a smarter device (e.g. a generic micro controller), it may use the
>> system's DTB to configure itself.
>>
>> Gr{oetje,eeting}s,
>>
>>                         Geert
>>


Can you please let me know your inputs on this ?


Now that Geert has sent v4 patch of GPIO Aggregator by "Dropping controversial GPIO repeater", I do not see the above mentioned inverter usecase can be handled anymore.


Is the observation/patch submitted in https://lore.kernel.org/linux-devicetree/dde73334-a26d-b53f-6b97-4101c1cdc185@mentor.com/ still not acceptable?



Thanks,

Harish


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 0/7] gpio: Add GPIO Aggregator/Repeater
  2019-11-27  8:42 [PATCH v3 0/7] gpio: Add GPIO Aggregator/Repeater Geert Uytterhoeven
                   ` (6 preceding siblings ...)
  2019-11-27  8:42 ` [PATCH v3 7/7] MAINTAINERS: Add GPIO Aggregator/Repeater section Geert Uytterhoeven
@ 2020-01-18  1:46 ` Eugeniu Rosca
  2020-01-20  9:33   ` Geert Uytterhoeven
  7 siblings, 1 reply; 48+ messages in thread
From: Eugeniu Rosca @ 2020-01-18  1:46 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Linus Walleij, Bartosz Golaszewski, Jonathan Corbet, Rob Herring,
	Mark Rutland, Harish Jenny K N, Eugeniu Rosca, Alexander Graf,
	Peter Maydell, Paolo Bonzini, Phil Reid, Marc Zyngier,
	Christoffer Dall, Magnus Damm, linux-gpio, linux-doc, devicetree,
	linux-renesas-soc, linux-kernel, qemu-devel, Eugeniu Rosca

Hi Geert,

On Wed, Nov 27, 2019 at 09:42:46AM +0100, Geert Uytterhoeven wrote:
>   - Create aggregators:
> 
>     $ echo e6052000.gpio 19,20 \
>         > /sys/bus/platform/drivers/gpio-aggregator/new_device
> 
>     gpio-aggregator gpio-aggregator.0: gpio 0 => gpio-953 (gpio-aggregator.0)
>     gpio-aggregator gpio-aggregator.0: gpio 1 => gpio-954 (gpio-aggregator.0)
>     gpiochip_find_base: found new base at 778
>     gpio gpiochip8: (gpio-aggregator.0): added GPIO chardev (254:8)
>     gpiochip_setup_dev: registered GPIOs 778 to 779 on device: gpiochip8 (gpio-aggregator.0)
> 
>     $ echo e6052000.gpio 21 e6050000.gpio 20-22 \
>         > /sys/bus/platform/drivers/gpio-aggregator/new_device
> 
>     gpio-aggregator gpio-aggregator.1: gpio 0 => gpio-955 (gpio-aggregator.1)
>     gpio-aggregator gpio-aggregator.1: gpio 1 => gpio-1012 (gpio-aggregator.1)
>     gpio-aggregator gpio-aggregator.1: gpio 2 => gpio-1013 (gpio-aggregator.1)
>     gpio-aggregator gpio-aggregator.1: gpio 3 => gpio-1014 (gpio-aggregator.1)
>     gpiochip_find_base: found new base at 774
>     gpio gpiochip9: (gpio-aggregator.1): added GPIO chardev (254:9)
>     gpiochip_setup_dev: registered GPIOs 774 to 777 on device: gpiochip9 (gpio-aggregator.1)
> 
>   - Adjust permissions on /dev/gpiochip[89] (optional)
> 
>   - Control LEDs:
> 
>     $ gpioset gpiochip8 0=0 1=1 # LED6 OFF, LED7 ON
>     $ gpioset gpiochip8 0=1 1=0 # LED6 ON, LED7 OFF
>     $ gpioset gpiochip9 0=0     # LED8 OFF
>     $ gpioset gpiochip9 0=1     # LED8 ON
> 
>   - Destroy aggregators:
> 
>     $ echo gpio-aggregator.0 \
>             > /sys/bus/platform/drivers/gpio-aggregator/delete_device
>     $ echo gpio-aggregator.1 \
>             > /sys/bus/platform/drivers/gpio-aggregator/delete_device

Thanks for describing the test procedure in detail. It helps a lot.

Using similar commands on H3ULCB, I could successfully trigger the
gpiochip6-{12,13} leds on and off. 

The only unexpected thing is seeing below messages (where gpiochip99 and
gpiochip22 are inexisting gpiochip names, mistakenly provided on command
line prior to passing the correct name):

root@rcar-gen3:~# echo gpiochip6 12-13 > /sys/bus/platform/drivers/gpio-aggregator/new_device                                                                                                                                                                                                                                                                 
[  915.572905] gpio-aggregator gpio-aggregator.0: cannot find GPIO chip gpiochip99, deferring
[  915.584224] gpio-aggregator gpio-aggregator.2: cannot find GPIO chip gpiochip99, deferring
[  915.865281] gpio-aggregator gpio-aggregator.29: cannot find GPIO chip gpiochip22, deferring

Obviously, in the above case, due to a typo in the names, the gpio
chips will never be found, no matter how long gpio-aggregator defers
their probing. Unfortunately, the driver will continuously emit those
messages, upon each successfully created/aggregated gpiochip. I built
gpio-aggregator as a loadable module, if that's relevant.

Another comment is that, while the series _does_ allow specifying
gpio lines in the DTS (this would require a common compatible string
in gpio_aggregator_dt_ids[] and in the DTS node) and while those lines
are indeed exposed to userspace, based on my testing, these same gpio
lines are marked as "used/reserved" by the kernel. This means that
operating on those gpio pins from userspace will not be possible.
For instance, gpioget/gpioset return "Device or resource busy":

gpioget: error reading GPIO values: Device or resource busy
gpioset: error setting the GPIO line values: Device or resource busy

I guess Harish will be unhappy about that, as his expectation was that
upon merging gpio-aggregator with gpio-inverter, he will be able to
describe GPIO polarity and names in DTS without "hogging" the pins.
Perhaps this can be supplemented via an add-on patch later on?

For the whole series (leaving the above findings to your discretion):

Reviewed-by: Eugeniu Rosca <erosca@de.adit-jv.com>
Tested-by: Eugeniu Rosca <erosca@de.adit-jv.com>

Thanks!

-- 
Best Regards,
Eugeniu

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 0/7] gpio: Add GPIO Aggregator/Repeater
  2020-01-18  1:46 ` [PATCH v3 0/7] gpio: Add GPIO Aggregator/Repeater Eugeniu Rosca
@ 2020-01-20  9:33   ` Geert Uytterhoeven
  2020-01-20 12:14     ` Eugeniu Rosca
  0 siblings, 1 reply; 48+ messages in thread
From: Geert Uytterhoeven @ 2020-01-20  9:33 UTC (permalink / raw)
  To: Eugeniu Rosca
  Cc: Linus Walleij, Bartosz Golaszewski, Jonathan Corbet, Rob Herring,
	Mark Rutland, Harish Jenny K N, Alexander Graf, Peter Maydell,
	Paolo Bonzini, Phil Reid, Marc Zyngier, Christoffer Dall,
	Magnus Damm, open list:GPIO SUBSYSTEM, open list:DOCUMENTATION,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, Linux Kernel Mailing List, QEMU Developers,
	Eugeniu Rosca

Hi Eugeniu,

On Sat, Jan 18, 2020 at 2:46 AM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> On Wed, Nov 27, 2019 at 09:42:46AM +0100, Geert Uytterhoeven wrote:
> >   - Create aggregators:
> >
> >     $ echo e6052000.gpio 19,20 \
> >         > /sys/bus/platform/drivers/gpio-aggregator/new_device

> The only unexpected thing is seeing below messages (where gpiochip99 and
> gpiochip22 are inexisting gpiochip names, mistakenly provided on command
> line prior to passing the correct name):
>
> root@rcar-gen3:~# echo gpiochip6 12-13 > /sys/bus/platform/drivers/gpio-aggregator/new_device
> [  915.572905] gpio-aggregator gpio-aggregator.0: cannot find GPIO chip gpiochip99, deferring
> [  915.584224] gpio-aggregator gpio-aggregator.2: cannot find GPIO chip gpiochip99, deferring
> [  915.865281] gpio-aggregator gpio-aggregator.29: cannot find GPIO chip gpiochip22, deferring
>
> Obviously, in the above case, due to a typo in the names, the gpio
> chips will never be found, no matter how long gpio-aggregator defers

Indeed, that is expected behavior: you have created platform devices
referring to resources that are not available.

> their probing. Unfortunately, the driver will continuously emit those
> messages, upon each successfully created/aggregated gpiochip. I built

That is expected behavior, too: every time the driver core manages to
bind a device to a driver, it will retry all previously deferred probes,
in the hope they can be satisfied by the just bound device.

Note that you can destroy these bogus devices, using e.g.

    # echo gpio-aggregator.0 > \
    /sys/bus/platform/drivers/gpio-aggregator/delete_device

> gpio-aggregator as a loadable module, if that's relevant.

Modular or non-modular shouldn't matter w.r.t. this behavior.
Although unloading the module should get rid of the cruft.

> Another comment is that, while the series _does_ allow specifying
> gpio lines in the DTS (this would require a common compatible string
> in gpio_aggregator_dt_ids[] and in the DTS node) and while those lines
> are indeed exposed to userspace, based on my testing, these same gpio
> lines are marked as "used/reserved" by the kernel. This means that
> operating on those gpio pins from userspace will not be possible.
> For instance, gpioget/gpioset return "Device or resource busy":
>
> gpioget: error reading GPIO values: Device or resource busy
> gpioset: error setting the GPIO line values: Device or resource busy
>
> I guess Harish will be unhappy about that, as his expectation was that
> upon merging gpio-aggregator with gpio-inverter, he will be able to
> describe GPIO polarity and names in DTS without "hogging" the pins.
> Perhaps this can be supplemented via an add-on patch later on?

When aggregating GPIO lines, the original GPIO lines are indeed marked
used/reserved, so you cannot use them from userspace.
However, you are expected to use them through the newly created virtual
gpiochip representing the aggregated GPIO lines.

You can try this using the "door" example in
Documentation/admin-guide/gpio/gpio-aggregator.rst, after replacing
gpio2 {19,20} by gpio6 {12,13} to suit your H3ULCB.

> For the whole series (leaving the above findings to your discretion):
>
> Reviewed-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> Tested-by: Eugeniu Rosca <erosca@de.adit-jv.com>

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 0/7] gpio: Add GPIO Aggregator/Repeater
  2020-01-20  9:33   ` Geert Uytterhoeven
@ 2020-01-20 12:14     ` Eugeniu Rosca
  0 siblings, 0 replies; 48+ messages in thread
From: Eugeniu Rosca @ 2020-01-20 12:14 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Eugeniu Rosca, Linus Walleij, Bartosz Golaszewski,
	Jonathan Corbet, Rob Herring, Mark Rutland, Harish Jenny K N,
	Alexander Graf, Peter Maydell, Paolo Bonzini, Phil Reid,
	Marc Zyngier, Christoffer Dall, Magnus Damm,
	open list:GPIO SUBSYSTEM, open list:DOCUMENTATION,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, Linux Kernel Mailing List, QEMU Developers,
	Eugeniu Rosca

Hi Geert,

On Mon, Jan 20, 2020 at 10:33:53AM +0100, Geert Uytterhoeven wrote:
> On Sat, Jan 18, 2020 at 2:46 AM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> > The only unexpected thing is seeing below messages (where gpiochip99 and
> > gpiochip22 are inexisting gpiochip names, mistakenly provided on command
> > line prior to passing the correct name):
> >
> > root@rcar-gen3:~# echo gpiochip6 12-13 > /sys/bus/platform/drivers/gpio-aggregator/new_device
> > [  915.572905] gpio-aggregator gpio-aggregator.0: cannot find GPIO chip gpiochip99, deferring
> > [  915.584224] gpio-aggregator gpio-aggregator.2: cannot find GPIO chip gpiochip99, deferring
> > [  915.865281] gpio-aggregator gpio-aggregator.29: cannot find GPIO chip gpiochip22, deferring
> >
> > Obviously, in the above case, due to a typo in the names, the gpio
> > chips will never be found, no matter how long gpio-aggregator defers
> 
> Indeed, that is expected behavior: you have created platform devices
> referring to resources that are not available.

Got it. Sounds reasonable to me.

> 
> > their probing. Unfortunately, the driver will continuously emit those
> > messages, upon each successfully created/aggregated gpiochip. I built
> 
> That is expected behavior, too: every time the driver core manages to
> bind a device to a driver, it will retry all previously deferred probes,
> in the hope they can be satisfied by the just bound device.
> 
> Note that you can destroy these bogus devices, using e.g.
> 
>     # echo gpio-aggregator.0 > \
>     /sys/bus/platform/drivers/gpio-aggregator/delete_device

Yep, I can get rid of the bogus devices this way. Thanks!

> 
> > gpio-aggregator as a loadable module, if that's relevant.
> 
> Modular or non-modular shouldn't matter w.r.t. this behavior.
> Although unloading the module should get rid of the cruft.

Yes, indeed!

> 
> > Another comment is that, while the series _does_ allow specifying
> > gpio lines in the DTS (this would require a common compatible string
> > in gpio_aggregator_dt_ids[] and in the DTS node) and while those lines
> > are indeed exposed to userspace, based on my testing, these same gpio
> > lines are marked as "used/reserved" by the kernel. This means that
> > operating on those gpio pins from userspace will not be possible.
> > For instance, gpioget/gpioset return "Device or resource busy":
> >
> > gpioget: error reading GPIO values: Device or resource busy
> > gpioset: error setting the GPIO line values: Device or resource busy
> >
> > I guess Harish will be unhappy about that, as his expectation was that
> > upon merging gpio-aggregator with gpio-inverter, he will be able to
> > describe GPIO polarity and names in DTS without "hogging" the pins.
> > Perhaps this can be supplemented via an add-on patch later on?
> 
> When aggregating GPIO lines, the original GPIO lines are indeed marked
> used/reserved, so you cannot use them from userspace.
> However, you are expected to use them through the newly created virtual
> gpiochip representing the aggregated GPIO lines.
> 
> You can try this using the "door" example in
> Documentation/admin-guide/gpio/gpio-aggregator.rst, after replacing
> gpio2 {19,20} by gpio6 {12,13} to suit your H3ULCB.

Confirmed. The example works like a charm. One difference between
the runtime-created and DTS-created gpiochips is the name:
 - gpio-aggregator.<number>, for the ones created via sysfs
 - <name-of-DTS-node>, for the ones created via DTS

Seeing this behavior on my target, I believe the expectations of
Harish should be met w/o any major limitations.

> 
> > For the whole series (leaving the above findings to your discretion):
> >
> > Reviewed-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> > Tested-by: Eugeniu Rosca <erosca@de.adit-jv.com>

The recent [v3] discussion actually applies to [v4], for which I did
review and testing. Will relay the signatures to the latest version.

Thank you very much.

-- 
Best Regards,
Eugeniu

^ permalink raw reply	[flat|nested] 48+ messages in thread

end of thread, other threads:[~2020-01-20 12:14 UTC | newest]

Thread overview: 48+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-27  8:42 [PATCH v3 0/7] gpio: Add GPIO Aggregator/Repeater Geert Uytterhoeven
2019-11-27  8:42 ` [PATCH v3 1/7] gpiolib: Add GPIOCHIP_NAME definition Geert Uytterhoeven
2019-11-28  3:38   ` Ulrich Hecht
2019-12-02 21:17   ` Eugeniu Rosca
2019-12-12 10:37   ` Linus Walleij
2019-11-27  8:42 ` [PATCH v3 2/7] gpiolib: Add support for gpiochipN-based table lookup Geert Uytterhoeven
2019-11-28  3:38   ` Ulrich Hecht
2019-12-12 13:20   ` Linus Walleij
2019-12-12 13:33     ` Geert Uytterhoeven
2019-12-12 14:36       ` Linus Walleij
2019-11-27  8:42 ` [PATCH v3 3/7] gpiolib: Add support for GPIO line " Geert Uytterhoeven
2019-11-28  3:39   ` Ulrich Hecht
2019-12-12 13:40   ` Linus Walleij
2019-11-27  8:42 ` [PATCH v3 4/7] dt-bindings: gpio: Add gpio-repeater bindings Geert Uytterhoeven
2019-11-28  3:39   ` Ulrich Hecht
2019-12-03  5:51   ` Harish Jenny K N
2019-12-05 21:06   ` Rob Herring
2019-12-06  9:17     ` Geert Uytterhoeven
2019-12-06 15:03       ` Rob Herring
2020-01-06  8:12         ` Geert Uytterhoeven
2020-01-07  9:22           ` Harish Jenny K N
2020-01-16  5:09             ` Harish Jenny K N
2019-11-27  8:42 ` [PATCH v3 5/7] gpio: Add GPIO Aggregator/Repeater driver Geert Uytterhoeven
2019-11-27 14:15   ` Eugeniu Rosca
2019-11-27 14:33     ` Geert Uytterhoeven
2019-11-28  3:40   ` Ulrich Hecht
2019-12-03  5:42   ` Harish Jenny K N
2019-12-03  8:17     ` Geert Uytterhoeven
2019-12-03  8:51       ` Harish Jenny K N
2019-12-03 10:51   ` Eugeniu Rosca
2020-01-09 13:35     ` Geert Uytterhoeven
2020-01-09 13:49       ` Eugeniu Rosca
2019-12-12 14:34   ` Linus Walleij
2019-12-12 15:24     ` Geert Uytterhoeven
2020-01-04  0:38       ` Linus Walleij
2020-01-06  8:23         ` Geert Uytterhoeven
2020-01-08 23:12           ` Linus Walleij
2019-11-27  8:42 ` [PATCH v3 6/7] docs: gpio: Add GPIO Aggregator/Repeater documentation Geert Uytterhoeven
2019-11-28  3:41   ` Ulrich Hecht
2019-12-12 14:42   ` Linus Walleij
2019-12-12 14:48     ` Geert Uytterhoeven
2020-01-04  0:21       ` Linus Walleij
2020-01-06  8:06         ` Geert Uytterhoeven
2019-11-27  8:42 ` [PATCH v3 7/7] MAINTAINERS: Add GPIO Aggregator/Repeater section Geert Uytterhoeven
2019-12-03  5:38   ` Harish Jenny K N
2020-01-18  1:46 ` [PATCH v3 0/7] gpio: Add GPIO Aggregator/Repeater Eugeniu Rosca
2020-01-20  9:33   ` Geert Uytterhoeven
2020-01-20 12:14     ` Eugeniu Rosca

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).