qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v6 0/8] gpio: Add GPIO Aggregator
@ 2020-03-24 13:53 Geert Uytterhoeven
  2020-03-24 13:56 ` [PATCH v6 1/8] ARM: integrator: impd1: Use GPIO_LOOKUP() helper macro Geert Uytterhoeven
  0 siblings, 1 reply; 19+ messages in thread
From: Geert Uytterhoeven @ 2020-03-24 13:53 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski, Jonathan Corbet,
	Harish Jenny K N, Eugeniu Rosca
  Cc: Mark Rutland, Peter Maydell, qemu-devel, Geert Uytterhoeven,
	linux-doc, Marc Zyngier, Magnus Damm, Christoffer Dall,
	linux-kernel, linux-renesas-soc, linux-gpio, Rob Herring,
	Alexander Graf, Paolo Bonzini, Phil Reid

	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 simple GPIO-operated devices in DT, and using the GPIO
    Aggregator as a generic GPIO driver for userspace, which is useful
    for industrial control.

Changes compared to v5[2]:
  - Convert raw gpiod_lookup users to GPIO_LOOKUP*(),
  - Update Documentation/driver-api/gpio/board.rst for gpiod_lookup
    changes,
  - Reword rationale behing looking up GPIOs by line name,
  - Introduce gpiod_set_config(),
  - Use gpiod_to_chip() instead of open-coding,
  - Drop debug print of gpio_desc.label, as it usually points to the
    GPIO Aggregator itself,
  - Drop no longer needed #include "gpiolib.h",
  - Fix missing offset translation in gpio_fwd_set_config(),
  - Fix "allows" without object,
  - Drop "gpiochipN" support,
  - Extend example,

Changes compared to v4[3]:
  - Add Reviewed-by, Tested-by,
  - Fix inconsistent indentation in documentation.

Changes compared to v3[4] (more details in the individual patches):
  - Drop controversial GPIO repeater,
  - Drop support for legacy sysfs interface based name matching,
  - Drop applied "gpiolib: Add GPIOCHIP_NAME definition",
  - Documentation improvements,
  - Lots of small cleanups.

Changes compared to v2[5] (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[6]:
  - 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[7][8].

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 gpio-aggregator.0: gpio 1 => gpio-954
    gpiochip_find_base: found new base at 758
    gpio gpiochip12: (gpio-aggregator.0): added GPIO chardev (254:13)
    gpiochip_setup_dev: registered GPIOs 758 to 759 on device: gpiochip12 (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 gpio-aggregator.1: gpio 1 => gpio-1012
    gpio-aggregator gpio-aggregator.1: gpio 2 => gpio-1013
    gpio-aggregator gpio-aggregator.1: gpio 3 => gpio-1014
    gpiochip_find_base: found new base at 754
    gpio gpiochip13: (gpio-aggregator.1): added GPIO chardev (254:13)
    gpiochip_setup_dev: registered GPIOs 754 to 757 on device: gpiochip13 (gpio-aggregator.1)

  - Adjust permissions on /dev/gpiochip1[23] (optional)

  - Control LEDs:

    $ gpioset gpiochip12 0=0 1=1 # LED6 OFF, LED7 ON
    $ gpioset gpiochip12 0=1 1=0 # LED6 ON, LED7 OFF
    $ gpioset gpiochip13 0=1     # LED8 ON
    $ gpioset gpiochip13 0=0     # LED8 OFF

  - 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

To ease testing, I have pushed this series to the
topic/gpio-aggregator-v6 branch of my renesas-drivers repository at
git://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git.
Kbuild test robot <lkp@intel.com> reported no issues.

Thanks!

References:
  [1] "[PATCH V4 2/2] gpio: inverter: document the inverter bindings"
      (https://lore.kernel.org/r/1561699236-18620-3-git-send-email-harish_kandiga@mentor.com/)
  [2] "[PATCH v5 0/5] gpio: Add GPIO Aggregator"
      (https://lore.kernel.org/r/20200218151812.7816-1-geert+renesas@glider.be/)
  [3] "[PATCH v4 0/5] gpio: Add GPIO Aggregator"
      (https://lore.kernel.org/r/20200115181523.23556-1-geert+renesas@glider.be)
  [4] "[PATCH v3 0/7] gpio: Add GPIO Aggregator/Repeater"
      (https://lore.kernel.org/r/20191127084253.16356-1-geert+renesas@glider.be/)
  [5] "[PATCH/RFC v2 0/5] gpio: Add GPIO Aggregator Driver"
      (https://lore.kernel.org/r/20190911143858.13024-1-geert+renesas@glider.be/)
  [6] "[PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver"
      (https://lore.kernel.org/r/20190705160536.12047-1-geert+renesas@glider.be/)
  [7] "[PATCH QEMU POC] Add a GPIO backend"
      (https://lore.kernel.org/r/20181003152521.23144-1-geert+renesas@glider.be/)
  [8] "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 (8):
  ARM: integrator: impd1: Use GPIO_LOOKUP() helper macro
  i2c: i801: Use GPIO_LOOKUP() helper macro
  mfd: sm501: Use GPIO_LOOKUP_IDX() helper macro
  gpiolib: Add support for GPIO lookup by line name
  gpiolib: Introduce gpiod_set_config()
  gpio: Add GPIO Aggregator
  docs: gpio: Add GPIO Aggregator documentation
  MAINTAINERS: Add GPIO Aggregator section

 .../admin-guide/gpio/gpio-aggregator.rst      | 111 ++++
 Documentation/admin-guide/gpio/index.rst      |   1 +
 Documentation/driver-api/gpio/board.rst       |  10 +-
 MAINTAINERS                                   |   7 +
 arch/arm/mach-integrator/impd1.c              |  11 +-
 drivers/gpio/Kconfig                          |  12 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-aggregator.c                | 568 ++++++++++++++++++
 drivers/gpio/gpiolib.c                        |  50 +-
 drivers/i2c/busses/i2c-i801.c                 |   6 +-
 drivers/mfd/sm501.c                           |  24 +-
 include/linux/gpio/consumer.h                 |   8 +
 include/linux/gpio/machine.h                  |  15 +-
 13 files changed, 776 insertions(+), 48 deletions(-)
 create mode 100644 Documentation/admin-guide/gpio/gpio-aggregator.rst
 create mode 100644 drivers/gpio/gpio-aggregator.c

-- 
2.17.1

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] 19+ messages in thread

* [PATCH v6 1/8] ARM: integrator: impd1: Use GPIO_LOOKUP() helper macro
  2020-03-24 13:53 [PATCH v6 0/8] gpio: Add GPIO Aggregator Geert Uytterhoeven
@ 2020-03-24 13:56 ` Geert Uytterhoeven
  2020-03-24 13:56   ` [PATCH v6 2/8] i2c: i801: " Geert Uytterhoeven
                     ` (7 more replies)
  0 siblings, 8 replies; 19+ messages in thread
From: Geert Uytterhoeven @ 2020-03-24 13:56 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski, Jonathan Corbet,
	Harish Jenny K N, Eugeniu Rosca
  Cc: Mark Rutland, Peter Maydell, qemu-devel, Geert Uytterhoeven,
	linux-doc, Marc Zyngier, Magnus Damm, Christoffer Dall,
	linux-kernel, linux-renesas-soc, linux-gpio, Rob Herring,
	Alexander Graf, Paolo Bonzini, Phil Reid, linux-arm-kernel

impd1_probe() fills in the GPIO lookup table by manually populating an
array of gpiod_lookup structures.  Use the existing GPIO_LOOKUP() helper
macro instead, to relax a dependency on the gpiod_lookup structure's
member names.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: linux-arm-kernel@lists.infradead.org
---
While this patch is a dependency for "[PATCH v6 4/8] gpiolib: Add
support for GPIO lookup by line name", it can be applied independently.
But an Acked-by would be nice, too.

Cover letter and full series at
https://lore.kernel.org/r/20200324135328.5796-1-geert+renesas@glider.be/

v6:
  - New.
---
 arch/arm/mach-integrator/impd1.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/arch/arm/mach-integrator/impd1.c b/arch/arm/mach-integrator/impd1.c
index 1ecbea5331d6ed8b..6f875ded841924d8 100644
--- a/arch/arm/mach-integrator/impd1.c
+++ b/arch/arm/mach-integrator/impd1.c
@@ -410,13 +410,10 @@ static int __ref impd1_probe(struct lm_device *dev)
 			 * 5 = Key lower right
 			 */
 			/* We need the two MMCI GPIO entries */
-			lookup->table[0].chip_label = chipname;
-			lookup->table[0].chip_hwnum = 3;
-			lookup->table[0].con_id = "wp";
-			lookup->table[1].chip_label = chipname;
-			lookup->table[1].chip_hwnum = 4;
-			lookup->table[1].con_id = "cd";
-			lookup->table[1].flags = GPIO_ACTIVE_LOW;
+			lookup->table[0] = (struct gpiod_lookup)
+				GPIO_LOOKUP(chipname, 3, "wp", 0);
+			lookup->table[1] = (struct gpiod_lookup)
+				GPIO_LOOKUP(chipname, 4, "cd", GPIO_ACTIVE_LOW);
 			gpiod_add_lookup_table(lookup);
 		}
 
-- 
2.17.1



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

* [PATCH v6 2/8] i2c: i801: Use GPIO_LOOKUP() helper macro
  2020-03-24 13:56 ` [PATCH v6 1/8] ARM: integrator: impd1: Use GPIO_LOOKUP() helper macro Geert Uytterhoeven
@ 2020-03-24 13:56   ` Geert Uytterhoeven
  2020-03-25  9:14     ` Jean Delvare
  2020-04-15 10:57     ` Wolfram Sang
  2020-03-24 13:56   ` [PATCH v6 3/8] mfd: sm501: Use GPIO_LOOKUP_IDX() " Geert Uytterhoeven
                     ` (6 subsequent siblings)
  7 siblings, 2 replies; 19+ messages in thread
From: Geert Uytterhoeven @ 2020-03-24 13:56 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski, Jonathan Corbet,
	Harish Jenny K N, Eugeniu Rosca
  Cc: Mark Rutland, Peter Maydell, qemu-devel, Jean Delvare,
	Geert Uytterhoeven, linux-doc, Marc Zyngier, Magnus Damm,
	Christoffer Dall, linux-kernel, linux-renesas-soc, linux-gpio,
	Rob Herring, Alexander Graf, linux-i2c, Paolo Bonzini, Phil Reid

i801_add_mux() fills in the GPIO lookup table by manually populating an
array of gpiod_lookup structures.  Use the existing GPIO_LOOKUP() helper
macro instead, to relax a dependency on the gpiod_lookup structure's
member names.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Jean Delvare <jdelvare@suse.com>
Cc: linux-i2c@vger.kernel.org
---
While this patch is a dependency for "[PATCH v6 4/8] gpiolib: Add
support for GPIO lookup by line name", it can be applied independently.
But an Acked-by would be nice, too.

Cover letter and full series at
https://lore.kernel.org/r/20200324135328.5796-1-geert+renesas@glider.be/

v6:
  - New.
---
 drivers/i2c/busses/i2c-i801.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index ca4f096fef749302..8e64a71bea684cc7 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1444,9 +1444,9 @@ static int i801_add_mux(struct i801_priv *priv)
 		return -ENOMEM;
 	lookup->dev_id = "i2c-mux-gpio";
 	for (i = 0; i < mux_config->n_gpios; i++) {
-		lookup->table[i].chip_label = mux_config->gpio_chip;
-		lookup->table[i].chip_hwnum = mux_config->gpios[i];
-		lookup->table[i].con_id = "mux";
+		lookup->table[i] = (struct gpiod_lookup)
+			GPIO_LOOKUP(mux_config->gpio_chip,
+				    mux_config->gpios[i], "mux", 0);
 	}
 	gpiod_add_lookup_table(lookup);
 	priv->lookup = lookup;
-- 
2.17.1



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

* [PATCH v6 3/8] mfd: sm501: Use GPIO_LOOKUP_IDX() helper macro
  2020-03-24 13:56 ` [PATCH v6 1/8] ARM: integrator: impd1: Use GPIO_LOOKUP() helper macro Geert Uytterhoeven
  2020-03-24 13:56   ` [PATCH v6 2/8] i2c: i801: " Geert Uytterhoeven
@ 2020-03-24 13:56   ` Geert Uytterhoeven
  2020-04-15  9:25     ` Lee Jones
  2020-03-24 13:56   ` [PATCH v6 4/8] gpiolib: Add support for GPIO lookup by line name Geert Uytterhoeven
                     ` (5 subsequent siblings)
  7 siblings, 1 reply; 19+ messages in thread
From: Geert Uytterhoeven @ 2020-03-24 13:56 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski, Jonathan Corbet,
	Harish Jenny K N, Eugeniu Rosca
  Cc: Mark Rutland, Peter Maydell, qemu-devel, Geert Uytterhoeven,
	linux-doc, Marc Zyngier, Magnus Damm, Christoffer Dall,
	linux-kernel, linux-renesas-soc, linux-gpio, Rob Herring,
	Alexander Graf, Paolo Bonzini, Phil Reid, Lee Jones

i801_add_mux() fills in the GPIO lookup table by manually populating an
array of gpiod_lookup structures.  Use the existing GPIO_LOOKUP_IDX()
helper macro instead, to relax a dependency on the gpiod_lookup
structure's member names.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Lee Jones <lee.jones@linaro.org>
---
While this patch is a dependency for "[PATCH v6 4/8] gpiolib: Add
support for GPIO lookup by line name", it can be applied independently.
But an Acked-by would be nice, too.

Cover letter and full series at
https://lore.kernel.org/r/20200324135328.5796-1-geert+renesas@glider.be/

v6:
  - New.
---
 drivers/mfd/sm501.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/drivers/mfd/sm501.c b/drivers/mfd/sm501.c
index e49787e6bb93e5c8..ccd62b963952814e 100644
--- a/drivers/mfd/sm501.c
+++ b/drivers/mfd/sm501.c
@@ -1145,22 +1145,14 @@ static int sm501_register_gpio_i2c_instance(struct sm501_devdata *sm,
 		return -ENOMEM;
 
 	lookup->dev_id = "i2c-gpio";
-	if (iic->pin_sda < 32)
-		lookup->table[0].chip_label = "SM501-LOW";
-	else
-		lookup->table[0].chip_label = "SM501-HIGH";
-	lookup->table[0].chip_hwnum = iic->pin_sda % 32;
-	lookup->table[0].con_id = NULL;
-	lookup->table[0].idx = 0;
-	lookup->table[0].flags = GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN;
-	if (iic->pin_scl < 32)
-		lookup->table[1].chip_label = "SM501-LOW";
-	else
-		lookup->table[1].chip_label = "SM501-HIGH";
-	lookup->table[1].chip_hwnum = iic->pin_scl % 32;
-	lookup->table[1].con_id = NULL;
-	lookup->table[1].idx = 1;
-	lookup->table[1].flags = GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN;
+	lookup->table[0] = (struct gpiod_lookup)
+		GPIO_LOOKUP_IDX(iic->pin_sda < 32 ? "SM501-LOW" : "SM501-HIGH",
+				iic->pin_sda % 32, NULL, 0,
+				GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN);
+	lookup->table[1] = (struct gpiod_lookup)
+		GPIO_LOOKUP_IDX(iic->pin_scl < 32 ? "SM501-LOW" : "SM501-HIGH",
+				iic->pin_scl % 32, NULL, 1,
+				GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN);
 	gpiod_add_lookup_table(lookup);
 
 	icd = dev_get_platdata(&pdev->dev);
-- 
2.17.1



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

* [PATCH v6 4/8] gpiolib: Add support for GPIO lookup by line name
  2020-03-24 13:56 ` [PATCH v6 1/8] ARM: integrator: impd1: Use GPIO_LOOKUP() helper macro Geert Uytterhoeven
  2020-03-24 13:56   ` [PATCH v6 2/8] i2c: i801: " Geert Uytterhoeven
  2020-03-24 13:56   ` [PATCH v6 3/8] mfd: sm501: Use GPIO_LOOKUP_IDX() " Geert Uytterhoeven
@ 2020-03-24 13:56   ` Geert Uytterhoeven
  2020-03-26 21:18     ` Linus Walleij
  2020-03-24 13:56   ` [PATCH v6 5/8] gpiolib: Introduce gpiod_set_config() Geert Uytterhoeven
                     ` (4 subsequent siblings)
  7 siblings, 1 reply; 19+ messages in thread
From: Geert Uytterhoeven @ 2020-03-24 13:56 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski, Jonathan Corbet,
	Harish Jenny K N, Eugeniu Rosca
  Cc: Mark Rutland, Peter Maydell, qemu-devel, Geert Uytterhoeven,
	linux-doc, Marc Zyngier, Magnus Damm, Christoffer Dall,
	linux-kernel, linux-renesas-soc, linux-gpio, Rob Herring,
	Alexander Graf, Paolo Bonzini, Phil Reid

Currently a GPIO lookup table can only refer to a specific GPIO by a
tuple, consisting of a GPIO controller label and a GPIO offset inside
the controller.

However, a GPIO may also carry a line name, defined by DT or ACPI.
If present, the line name is the most use-centric way to refer to a
GPIO.  Hence add support for looking up GPIOs by line name.

Implement this by reusing the existing gpiod_lookup infrastructure.
Rename gpiod_lookup.chip_label to gpiod_lookup.key, to make it clear
that this field can have two meanings, and update the kerneldoc and
GPIO_LOOKUP*() macros.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>
Reviewed-by: Eugeniu Rosca <erosca@de.adit-jv.com>
Tested-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
v6:
  - Update Documentation/driver-api/gpio/board.rst,
  - Reword rationale,

v5:
  - Add Reviewed-by, Tested-by,

v4:
  - Add Reviewed-by,
  - Rename gpiod_lookup.chip_label.
  - Use U16_MAX instead of (u16)-1,

v3:
  - New.
---
 Documentation/driver-api/gpio/board.rst | 10 ++++++----
 drivers/gpio/gpiolib.c                  | 22 +++++++++++++++++-----
 include/linux/gpio/machine.h            | 15 ++++++++-------
 3 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/Documentation/driver-api/gpio/board.rst b/Documentation/driver-api/gpio/board.rst
index ce91518bf9f48ded..0ad1f8cacf5e5d26 100644
--- a/Documentation/driver-api/gpio/board.rst
+++ b/Documentation/driver-api/gpio/board.rst
@@ -113,13 +113,15 @@ files that desire to do so need to include the following header::
 GPIOs are mapped by the means of tables of lookups, containing instances of the
 gpiod_lookup structure. Two macros are defined to help declaring such mappings::
 
-	GPIO_LOOKUP(chip_label, chip_hwnum, con_id, flags)
-	GPIO_LOOKUP_IDX(chip_label, chip_hwnum, con_id, idx, flags)
+	GPIO_LOOKUP(key, chip_hwnum, con_id, flags)
+	GPIO_LOOKUP_IDX(key, chip_hwnum, con_id, idx, flags)
 
 where
 
-  - chip_label is the label of the gpiod_chip instance providing the GPIO
-  - chip_hwnum is the hardware number of the GPIO within the chip
+  - key is either the label of the gpiod_chip instance providing the GPIO, or
+    the GPIO line name
+  - chip_hwnum is the hardware number of the GPIO within the chip, or U16_MAX
+    to indicate that key is a GPIO line name
   - con_id is the name of the GPIO function from the device point of view. It
 	can be NULL, in which case it will match any function.
   - idx is the index of the GPIO within the function.
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 8d7366f4451fe695..c756602e249c052e 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -4643,7 +4643,7 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
 	if (!table)
 		return desc;
 
-	for (p = &table->table[0]; p->chip_label; p++) {
+	for (p = &table->table[0]; p->key; p++) {
 		struct gpio_chip *chip;
 
 		/* idx must always match exactly */
@@ -4654,18 +4654,30 @@ 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;
 
-		chip = find_chip_by_name(p->chip_label);
+		if (p->chip_hwnum == U16_MAX) {
+			desc = gpio_name_to_desc(p->key);
+			if (desc) {
+				*flags = p->flags;
+				return desc;
+			}
+
+			dev_warn(dev, "cannot find GPIO line %s, deferring\n",
+				 p->key);
+			return ERR_PTR(-EPROBE_DEFER);
+		}
+
+		chip = find_chip_by_name(p->key);
 
 		if (!chip) {
 			/*
 			 * As the lookup table indicates a chip with
-			 * p->chip_label should exist, assume it may
+			 * p->key should exist, assume it may
 			 * still appear later and let the interested
 			 * consumer be probed again or let the Deferred
 			 * Probe infrastructure handle the error.
 			 */
 			dev_warn(dev, "cannot find GPIO chip %s, deferring\n",
-				 p->chip_label);
+				 p->key);
 			return ERR_PTR(-EPROBE_DEFER);
 		}
 
@@ -4696,7 +4708,7 @@ static int platform_gpio_count(struct device *dev, const char *con_id)
 	if (!table)
 		return -ENOENT;
 
-	for (p = &table->table[0]; p->chip_label; p++) {
+	for (p = &table->table[0]; p->key; p++) {
 		if ((con_id && p->con_id && !strcmp(con_id, p->con_id)) ||
 		    (!con_id && !p->con_id))
 			count++;
diff --git a/include/linux/gpio/machine.h b/include/linux/gpio/machine.h
index 1ebe5be05d5f81fa..84c66fbf54fd5811 100644
--- a/include/linux/gpio/machine.h
+++ b/include/linux/gpio/machine.h
@@ -20,8 +20,9 @@ enum gpio_lookup_flags {
 
 /**
  * struct gpiod_lookup - lookup table
- * @chip_label: name of the chip the GPIO belongs to
- * @chip_hwnum: hardware number (i.e. relative to the chip) of the GPIO
+ * @key: either the name of the chip the GPIO belongs to, or the GPIO line name
+ * @chip_hwnum: hardware number (i.e. relative to the chip) of the GPIO, or
+ *              U16_MAX to indicate that @key is a GPIO line name
  * @con_id: name of the GPIO from the device's point of view
  * @idx: index of the GPIO in case several GPIOs share the same name
  * @flags: bitmask of gpio_lookup_flags GPIO_* values
@@ -30,7 +31,7 @@ enum gpio_lookup_flags {
  * functions using platform data.
  */
 struct gpiod_lookup {
-	const char *chip_label;
+	const char *key;
 	u16 chip_hwnum;
 	const char *con_id;
 	unsigned int idx;
@@ -63,17 +64,17 @@ struct gpiod_hog {
 /*
  * Simple definition of a single GPIO under a con_id
  */
-#define GPIO_LOOKUP(_chip_label, _chip_hwnum, _con_id, _flags) \
-	GPIO_LOOKUP_IDX(_chip_label, _chip_hwnum, _con_id, 0, _flags)
+#define GPIO_LOOKUP(_key, _chip_hwnum, _con_id, _flags) \
+	GPIO_LOOKUP_IDX(_key, _chip_hwnum, _con_id, 0, _flags)
 
 /*
  * Use this macro if you need to have several GPIOs under the same con_id.
  * Each GPIO needs to use a different index and can be accessed using
  * gpiod_get_index()
  */
-#define GPIO_LOOKUP_IDX(_chip_label, _chip_hwnum, _con_id, _idx, _flags)  \
+#define GPIO_LOOKUP_IDX(_key, _chip_hwnum, _con_id, _idx, _flags)         \
 {                                                                         \
-	.chip_label = _chip_label,                                        \
+	.key = _key,                                                      \
 	.chip_hwnum = _chip_hwnum,                                        \
 	.con_id = _con_id,                                                \
 	.idx = _idx,                                                      \
-- 
2.17.1



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

* [PATCH v6 5/8] gpiolib: Introduce gpiod_set_config()
  2020-03-24 13:56 ` [PATCH v6 1/8] ARM: integrator: impd1: Use GPIO_LOOKUP() helper macro Geert Uytterhoeven
                     ` (2 preceding siblings ...)
  2020-03-24 13:56   ` [PATCH v6 4/8] gpiolib: Add support for GPIO lookup by line name Geert Uytterhoeven
@ 2020-03-24 13:56   ` Geert Uytterhoeven
  2020-03-26 21:26     ` Linus Walleij
  2020-03-27 21:37     ` Linus Walleij
  2020-03-24 13:56   ` [PATCH v6 6/8] gpio: Add GPIO Aggregator Geert Uytterhoeven
                     ` (3 subsequent siblings)
  7 siblings, 2 replies; 19+ messages in thread
From: Geert Uytterhoeven @ 2020-03-24 13:56 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski, Jonathan Corbet,
	Harish Jenny K N, Eugeniu Rosca
  Cc: Mark Rutland, Peter Maydell, qemu-devel, Geert Uytterhoeven,
	linux-doc, Marc Zyngier, Magnus Damm, Christoffer Dall,
	linux-kernel, linux-renesas-soc, linux-gpio, Rob Herring,
	Alexander Graf, Paolo Bonzini, Phil Reid

The GPIO Aggregator will need a method to forward a .set_config() call
to its parent gpiochip.  This requires obtaining the gpio_chip and
offset for a given gpio_desc.  While gpiod_to_chip() is public,
gpio_chip_hwgpio() is not, so there is currently no method to obtain the
needed GPIO offset parameter.

Hence introduce a public gpiod_set_config() helper, which invokes the
.set_config() callback through a gpio_desc pointer, like is done for
most other gpio_chip callbacks.

Rewrite the existing gpiod_set_debounce() helper as a wrapper around
gpiod_set_config(), to avoid duplication.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v6:
  - New.
---
 drivers/gpio/gpiolib.c        | 28 ++++++++++++++++++++++------
 include/linux/gpio/consumer.h |  8 ++++++++
 2 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index c756602e249c052e..30ea75e972b5a3b1 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -3478,6 +3478,26 @@ int gpiod_direction_output(struct gpio_desc *desc, int value)
 }
 EXPORT_SYMBOL_GPL(gpiod_direction_output);
 
+/**
+ * gpiod_set_config - sets @config for a GPIO
+ * @desc: descriptor of the GPIO for which to set the configuration
+ * @config: Same packed config format as generic pinconf
+ *
+ * Returns:
+ * 0 on success, %-ENOTSUPP if the controller doesn't support setting the
+ * configuration.
+ */
+int gpiod_set_config(struct gpio_desc *desc, unsigned long config)
+{
+	struct gpio_chip *chip;
+
+	VALIDATE_DESC(desc);
+	chip = desc->gdev->chip;
+
+	return gpio_do_set_config(chip, gpio_chip_hwgpio(desc), config);
+}
+EXPORT_SYMBOL_GPL(gpiod_set_config);
+
 /**
  * gpiod_set_debounce - sets @debounce time for a GPIO
  * @desc: descriptor of the GPIO for which to set debounce time
@@ -3489,14 +3509,10 @@ EXPORT_SYMBOL_GPL(gpiod_direction_output);
  */
 int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
 {
-	struct gpio_chip	*chip;
-	unsigned long		config;
-
-	VALIDATE_DESC(desc);
-	chip = desc->gdev->chip;
+	unsigned long config;
 
 	config = pinconf_to_config_packed(PIN_CONFIG_INPUT_DEBOUNCE, debounce);
-	return gpio_do_set_config(chip, gpio_chip_hwgpio(desc), config);
+	return gpiod_set_config(desc, config);
 }
 EXPORT_SYMBOL_GPL(gpiod_set_debounce);
 
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index 0a72fccf60fff230..901aab89d025f3ff 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -157,6 +157,7 @@ int gpiod_set_raw_array_value_cansleep(unsigned int array_size,
 				       struct gpio_array *array_info,
 				       unsigned long *value_bitmap);
 
+int gpiod_set_config(struct gpio_desc *desc, unsigned long config);
 int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce);
 int gpiod_set_transitory(struct gpio_desc *desc, bool transitory);
 void gpiod_toggle_active_low(struct gpio_desc *desc);
@@ -473,6 +474,13 @@ static inline int gpiod_set_raw_array_value_cansleep(unsigned int array_size,
 	return 0;
 }
 
+static inline int gpiod_set_config(struct gpio_desc *desc, unsigned long config)
+{
+	/* GPIO can never have been requested */
+	WARN_ON(desc);
+	return -ENOSYS;
+}
+
 static inline int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
 {
 	/* GPIO can never have been requested */
-- 
2.17.1



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

* [PATCH v6 6/8] gpio: Add GPIO Aggregator
  2020-03-24 13:56 ` [PATCH v6 1/8] ARM: integrator: impd1: Use GPIO_LOOKUP() helper macro Geert Uytterhoeven
                     ` (3 preceding siblings ...)
  2020-03-24 13:56   ` [PATCH v6 5/8] gpiolib: Introduce gpiod_set_config() Geert Uytterhoeven
@ 2020-03-24 13:56   ` Geert Uytterhoeven
  2020-03-24 13:56   ` [PATCH v6 7/8] docs: gpio: Add GPIO Aggregator documentation Geert Uytterhoeven
                     ` (2 subsequent siblings)
  7 siblings, 0 replies; 19+ messages in thread
From: Geert Uytterhoeven @ 2020-03-24 13:56 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski, Jonathan Corbet,
	Harish Jenny K N, Eugeniu Rosca
  Cc: Mark Rutland, Peter Maydell, qemu-devel, Geert Uytterhoeven,
	linux-doc, Marc Zyngier, Magnus Damm, Christoffer Dall,
	linux-kernel, linux-renesas-soc, linux-gpio, Rob Herring,
	Alexander Graf, Paolo Bonzini, Phil Reid

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:
  - Aggregating GPIOs using Sysfs
    This is useful for implementing access control, and assigning a set
    of GPIOs to a specific user or virtual machine.
  - Generic GPIO Driver
    This is useful for industrial control, where it can provide
    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>
Reviewed-by: Eugeniu Rosca <erosca@de.adit-jv.com>
Tested-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
v6:
  - Use gpiod_to_chip() instead of open-coding,
  - Drop debug print of gpio_desc.label, as it usually just points to
    the GPIO Aggregator itself,
  - Drop no longer needed #include "gpiolib.h",
  - Fix missing offset translation in gpio_fwd_set_config(),
  - Use GPIO_LOOKUP_IDX() to populate struct gpiod_lookup,

v5:
  - Add Reviewed-by, Tested-by,

v4:
  - Remove unused assignment to n in isrange(),
  - Check correct pointer after aggr->lookups->dev_id allocation,
  - Preinitialize flags to 0 in gpio_fwd_[gs]et_multiple() to avoid
    may-be-used-uninitialized warning,
  - Drop controversial GPIO repeater,
  - Update for gpiod_lookup.chip_label rename,
  - Use %pe to format error pointers,
  - Use U16_MAX instead of (u16)-1,
  - Correct comment indentation,
  - Use skip_spaces() helper,
  - Rename a and b to first_index resp. last_index,
  - Add comment to tmp[] use,
  - Improve Kconfig help text,
  - Include <linux/gpio/consumer.h> for gpiod_[gs]et_*(),
  - Drop unneeded valid_mask handling,
  - Add comment about sleeping and .set_config() support,

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           |  12 +
 drivers/gpio/Makefile          |   1 +
 drivers/gpio/gpio-aggregator.c | 568 +++++++++++++++++++++++++++++++++
 3 files changed, 581 insertions(+)
 create mode 100644 drivers/gpio/gpio-aggregator.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 6234ccc90e7eb4a1..6ddc7353a46afdf6 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1541,6 +1541,18 @@ config GPIO_VIPERBOARD
 
 endmenu
 
+config GPIO_AGGREGATOR
+	tristate "GPIO Aggregator"
+	help
+	  Say yes here to enable the GPIO Aggregator, which provides a way to
+	  aggregate existing GPIO lines into a new virtual GPIO chip.
+	  This can serve the following purposes:
+	    - Assign permissions for a collection of GPIO lines to a user,
+	    - Export a collection of GPIO lines to a virtual machine,
+	    - Provide a generic driver for a GPIO-operated device in an
+	      industrial control context, to be operated 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 b2cfc21a97f3e52b..65bf3940e33cf734 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..9b0adbdddbfccb30
--- /dev/null
+++ b/drivers/gpio/gpio-aggregator.c
@@ -0,0 +1,568 @@
+// SPDX-License-Identifier: GPL-2.0-only
+//
+// GPIO Aggregator
+//
+// Copyright (C) 2019-2020 Glider bv
+
+#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/consumer.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>
+
+
+/*
+ * 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;
+
+	start = skip_spaces(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;
+
+	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 *key,
+			 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] =
+		(struct gpiod_lookup)GPIO_LOOKUP_IDX(key, hwnum, NULL, *n, 0);
+
+	(*n)++;
+	memset(&lookups->table[*n], 0, sizeof(lookups->table[*n]));
+
+	aggr->lookups = lookups;
+	return 0;
+}
+
+static int aggr_parse(struct gpio_aggregator *aggr)
+{
+	unsigned int first_index, last_index, i, n = 0;
+	char *name, *offsets, *first, *last, *next;
+	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: %pe\n", name);
+			return PTR_ERR(name);
+		}
+
+		if (!isrange(offsets)) {
+			/* Named GPIO line */
+			error = aggr_add_gpio(aggr, name, U16_MAX, &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, &first_index)) {
+				pr_err("Cannot parse GPIO index %s\n", first);
+				return -EINVAL;
+			}
+
+			if (!last) {
+				last_index = first_index;
+			} else if (kstrtouint(last, 10, &last_index)) {
+				pr_err("Cannot parse GPIO index %s\n", last);
+				return -EINVAL;
+			}
+
+			for (i = first_index; i <= last_index; 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->dev_id) {
+		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);
+}
+
+
+/*
+ *  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 = 0;
+	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);
+
+	/* Both values bitmap and desc pointers are stored in tmp[] */
+	values = &fwd->tmp[0];
+	descs = (void *)&fwd->tmp[BITS_TO_LONGS(fwd->chip.ngpio)];
+
+	bitmap_clear(values, 0, 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 = 0;
+	struct gpio_desc **descs;
+	unsigned int i, j = 0;
+
+	if (chip->can_sleep)
+		mutex_lock(&fwd->mlock);
+	else
+		spin_lock_irqsave(&fwd->slock, flags);
+
+	/* Both values bitmap and desc pointers are stored in tmp[] */
+	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);
+
+	return gpiod_set_config(fwd->descs[offset], config);
+}
+
+/**
+ * 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;
+
+	/*
+	 * If any of the GPIO lines are sleeping, then the entire forwarder
+	 * will be sleeping.
+	 * If any of the chips support .set_config(), then the forwarder will
+	 * support setting configs.
+	 */
+	for (i = 0; i < ngpios; i++) {
+		struct gpio_chip *parent = gpiod_to_chip(descs[i]);
+
+		dev_dbg(dev, "%u => gpio-%d\n", i, desc_to_gpio(descs[i]));
+
+		if (gpiod_cansleep(descs[i]))
+			chip->can_sleep = true;
+		if (parent && parent->set_config)
+			chip->set_config = gpio_fwd_set_config;
+	}
+
+	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;
+}
+
+
+/*
+ *  GPIO Aggregator 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[] = {
+	/*
+	 * 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");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1



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

* [PATCH v6 7/8] docs: gpio: Add GPIO Aggregator documentation
  2020-03-24 13:56 ` [PATCH v6 1/8] ARM: integrator: impd1: Use GPIO_LOOKUP() helper macro Geert Uytterhoeven
                     ` (4 preceding siblings ...)
  2020-03-24 13:56   ` [PATCH v6 6/8] gpio: Add GPIO Aggregator Geert Uytterhoeven
@ 2020-03-24 13:56   ` Geert Uytterhoeven
  2020-03-24 13:56   ` [PATCH v6 8/8] MAINTAINERS: Add GPIO Aggregator section Geert Uytterhoeven
  2020-03-26 21:07   ` [PATCH v6 1/8] ARM: integrator: impd1: Use GPIO_LOOKUP() helper macro Linus Walleij
  7 siblings, 0 replies; 19+ messages in thread
From: Geert Uytterhoeven @ 2020-03-24 13:56 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski, Jonathan Corbet,
	Harish Jenny K N, Eugeniu Rosca
  Cc: Mark Rutland, Peter Maydell, qemu-devel, Geert Uytterhoeven,
	linux-doc, Marc Zyngier, Magnus Damm, Christoffer Dall,
	linux-kernel, linux-renesas-soc, linux-gpio, Rob Herring,
	Alexander Graf, Paolo Bonzini, Phil Reid

Document the GPIO Aggregator, and the two typical use-cases.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>
Reviewed-by: Eugeniu Rosca <erosca@de.adit-jv.com>
Tested-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
v6:
  - Fix "allows" without object:
      -> provides a mechanism to aggregate GPIOs,
      -> provides access control for a set of one or more GPIOs,
      -> allows the user to communicate,
  - Drop "gpiochipN" support,
  - Extend example,

v5:
  - Add Reviewed-by, Tested-by,
  - Fix inconsistent indentation,

v4:
  - Add Reviewed-by,
  - Drop controversial GPIO repeater,
  - Clarify industrial control use case,
  - Fix typo s/communicated/communicate/,
  - Replace abstract frobnicator example by concrete door example with
    gpio-line-names,

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..5cd1e7221756504c
--- /dev/null
+++ b/Documentation/admin-guide/gpio/gpio-aggregator.rst
@@ -0,0 +1,111 @@
+.. SPDX-License-Identifier: GPL-2.0-only
+
+GPIO Aggregator
+===============
+
+The GPIO Aggregator provides a mechanism 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 provides access control for a set of one or more 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, 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
+		line 19 of "e6052000.gpio" and GPIO lines 20-21 of
+		"e6050000.gpio" into a new gpio_chip:
+
+		.. code-block:: sh
+
+		    $ echo 'e6052000.gpio 19 e6050000.gpio 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, assumed to be "gpio-aggregator.0":
+
+		.. code-block:: sh
+
+		    $ echo gpio-aggregator.0 > delete_device
+
+
+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 useful in industrial control, and is not unlike e.g. spidev, which
+allows the user to communicate 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 "door" is a GPIO-operated device described in DT, using its own
+compatible value::
+
+	door {
+		compatible = "myvendor,mydoor";
+
+		gpios = <&gpio2 19 GPIO_ACTIVE_HIGH>,
+			<&gpio2 20 GPIO_ACTIVE_LOW>;
+		gpio-line-names = "open", "lock";
+	};
+
+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:: sh
+
+    $ echo gpio-aggregator > /sys/bus/platform/devices/door/driver_override
+    $ echo door > /sys/bus/platform/drivers/gpio-aggregator/bind
+
+After that, a new gpiochip "door" has been created:
+
+.. code-block:: sh
+
+    $ gpioinfo door
+    gpiochip12 - 2 lines:
+	    line   0:       "open"       unused   input  active-high
+	    line   1:       "lock"       unused   input  active-high
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] 19+ messages in thread

* [PATCH v6 8/8] MAINTAINERS: Add GPIO Aggregator section
  2020-03-24 13:56 ` [PATCH v6 1/8] ARM: integrator: impd1: Use GPIO_LOOKUP() helper macro Geert Uytterhoeven
                     ` (5 preceding siblings ...)
  2020-03-24 13:56   ` [PATCH v6 7/8] docs: gpio: Add GPIO Aggregator documentation Geert Uytterhoeven
@ 2020-03-24 13:56   ` Geert Uytterhoeven
  2020-03-26 21:07   ` [PATCH v6 1/8] ARM: integrator: impd1: Use GPIO_LOOKUP() helper macro Linus Walleij
  7 siblings, 0 replies; 19+ messages in thread
From: Geert Uytterhoeven @ 2020-03-24 13:56 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski, Jonathan Corbet,
	Harish Jenny K N, Eugeniu Rosca
  Cc: Mark Rutland, Peter Maydell, qemu-devel, Geert Uytterhoeven,
	linux-doc, Marc Zyngier, Magnus Damm, Christoffer Dall,
	linux-kernel, linux-renesas-soc, linux-gpio, Rob Herring,
	Alexander Graf, Paolo Bonzini, Phil Reid

Add a maintainership section for the GPIO Aggregator, covering
documentation and driver source code.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Eugeniu Rosca <erosca@de.adit-jv.com>
Tested-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
v6:
  - No changes,

v5:
  - Add Reviewed-by, Tested-by,

v4:
  - Drop controversial GPIO repeater,

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

diff --git a/MAINTAINERS b/MAINTAINERS
index fcd79fc38928fafc..1fad69b956df1162 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7127,6 +7127,13 @@ F:	Documentation/firmware-guide/acpi/gpio-properties.rst
 F:	drivers/gpio/gpiolib-acpi.c
 F:	drivers/gpio/gpiolib-acpi.h
 
+GPIO AGGREGATOR
+M:	Geert Uytterhoeven <geert+renesas@glider.be>
+L:	linux-gpio@vger.kernel.org
+S:	Maintained
+F:	Documentation/admin-guide/gpio/gpio-aggregator.rst
+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] 19+ messages in thread

* Re: [PATCH v6 2/8] i2c: i801: Use GPIO_LOOKUP() helper macro
  2020-03-24 13:56   ` [PATCH v6 2/8] i2c: i801: " Geert Uytterhoeven
@ 2020-03-25  9:14     ` Jean Delvare
  2020-04-15 10:57     ` Wolfram Sang
  1 sibling, 0 replies; 19+ messages in thread
From: Jean Delvare @ 2020-03-25  9:14 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Mark Rutland, Peter Maydell, qemu-devel, Phil Reid,
	Jonathan Corbet, Marc Zyngier, Linus Walleij, linux-doc,
	Magnus Damm, Christoffer Dall, linux-kernel, linux-renesas-soc,
	Bartosz Golaszewski, Rob Herring, Harish Jenny K N, linux-i2c,
	linux-gpio, Paolo Bonzini, Alexander Graf, Eugeniu Rosca

On Tue, 24 Mar 2020 14:56:47 +0100, Geert Uytterhoeven wrote:
> i801_add_mux() fills in the GPIO lookup table by manually populating an
> array of gpiod_lookup structures.  Use the existing GPIO_LOOKUP() helper
> macro instead, to relax a dependency on the gpiod_lookup structure's
> member names.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Jean Delvare <jdelvare@suse.com>
> Cc: linux-i2c@vger.kernel.org
> ---
> While this patch is a dependency for "[PATCH v6 4/8] gpiolib: Add
> support for GPIO lookup by line name", it can be applied independently.
> But an Acked-by would be nice, too.
> 
> Cover letter and full series at
> https://lore.kernel.org/r/20200324135328.5796-1-geert+renesas@glider.be/
> 
> v6:
>   - New.
> ---
>  drivers/i2c/busses/i2c-i801.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index ca4f096fef749302..8e64a71bea684cc7 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1444,9 +1444,9 @@ static int i801_add_mux(struct i801_priv *priv)
>  		return -ENOMEM;
>  	lookup->dev_id = "i2c-mux-gpio";
>  	for (i = 0; i < mux_config->n_gpios; i++) {
> -		lookup->table[i].chip_label = mux_config->gpio_chip;
> -		lookup->table[i].chip_hwnum = mux_config->gpios[i];
> -		lookup->table[i].con_id = "mux";
> +		lookup->table[i] = (struct gpiod_lookup)
> +			GPIO_LOOKUP(mux_config->gpio_chip,
> +				    mux_config->gpios[i], "mux", 0);
>  	}
>  	gpiod_add_lookup_table(lookup);
>  	priv->lookup = lookup;

Reviewed-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support


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

* Re: [PATCH v6 1/8] ARM: integrator: impd1: Use GPIO_LOOKUP() helper macro
  2020-03-24 13:56 ` [PATCH v6 1/8] ARM: integrator: impd1: Use GPIO_LOOKUP() helper macro Geert Uytterhoeven
                     ` (6 preceding siblings ...)
  2020-03-24 13:56   ` [PATCH v6 8/8] MAINTAINERS: Add GPIO Aggregator section Geert Uytterhoeven
@ 2020-03-26 21:07   ` Linus Walleij
  7 siblings, 0 replies; 19+ messages in thread
From: Linus Walleij @ 2020-03-26 21:07 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Mark Rutland, Peter Maydell, QEMU Developers, Phil Reid,
	Jonathan Corbet, Marc Zyngier, open list:GPIO SUBSYSTEM,
	Linux Doc Mailing List, Magnus Damm, Christoffer Dall,
	linux-kernel, Linux-Renesas, Bartosz Golaszewski, Rob Herring,
	Harish Jenny K N, Paolo Bonzini, Alexander Graf, Eugeniu Rosca,
	Linux ARM

On Tue, Mar 24, 2020 at 2:57 PM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:

> impd1_probe() fills in the GPIO lookup table by manually populating an
> array of gpiod_lookup structures.  Use the existing GPIO_LOOKUP() helper
> macro instead, to relax a dependency on the gpiod_lookup structure's
> member names.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: linux-arm-kernel@lists.infradead.org
> ---
> While this patch is a dependency for "[PATCH v6 4/8] gpiolib: Add
> support for GPIO lookup by line name", it can be applied independently.
> But an Acked-by would be nice, too.

I simply applied this patch for v5.7 in the GPIO tree since I am the
maintainer of this platform, and I might want to change stuff around
Integrator next cycle so it's good to have this covered.

Yours,
Linus Walleij


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

* Re: [PATCH v6 4/8] gpiolib: Add support for GPIO lookup by line name
  2020-03-24 13:56   ` [PATCH v6 4/8] gpiolib: Add support for GPIO lookup by line name Geert Uytterhoeven
@ 2020-03-26 21:18     ` Linus Walleij
  2020-05-11 10:18       ` Geert Uytterhoeven
  0 siblings, 1 reply; 19+ messages in thread
From: Linus Walleij @ 2020-03-26 21:18 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Mark Rutland, Peter Maydell, QEMU Developers, Phil Reid,
	Jonathan Corbet, Marc Zyngier, open list:GPIO SUBSYSTEM,
	Linux Doc Mailing List, Magnus Damm, Christoffer Dall,
	linux-kernel, Linux-Renesas, Bartosz Golaszewski, Rob Herring,
	Harish Jenny K N, Paolo Bonzini, Alexander Graf, Eugeniu Rosca

On Tue, Mar 24, 2020 at 2:57 PM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:

> Currently a GPIO lookup table can only refer to a specific GPIO by a
> tuple, consisting of a GPIO controller label and a GPIO offset inside
> the controller.
>
> However, a GPIO may also carry a line name, defined by DT or ACPI.
> If present, the line name is the most use-centric way to refer to a
> GPIO.  Hence add support for looking up GPIOs by line name.
>
> Implement this by reusing the existing gpiod_lookup infrastructure.
> Rename gpiod_lookup.chip_label to gpiod_lookup.key, to make it clear
> that this field can have two meanings, and update the kerneldoc and
> GPIO_LOOKUP*() macros.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>
> Reviewed-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> Tested-by: Eugeniu Rosca <erosca@de.adit-jv.com>

I kind of like this approach, however there are things here that
need to be considered: the line name is in no way globally unique,
and I think there are already quite a few GPIO chips that
have the same line names assigned for every instance of that
chip.

gpiochip_set_desc_names() only warns if there is a line with
the same name on the same gpio_chip.

I suppose we need to document that the line name look-up
will be on a first-come-first-served basis: whatever line
we find first with this name is what you will get a reference
to, no matter what chip it is on, and it is possible albeit
not recommended that some other chip has a line with the
same name.

Yours,
Linus Walleij


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

* Re: [PATCH v6 5/8] gpiolib: Introduce gpiod_set_config()
  2020-03-24 13:56   ` [PATCH v6 5/8] gpiolib: Introduce gpiod_set_config() Geert Uytterhoeven
@ 2020-03-26 21:26     ` Linus Walleij
  2020-03-27  8:45       ` Geert Uytterhoeven
  2020-03-27 21:37     ` Linus Walleij
  1 sibling, 1 reply; 19+ messages in thread
From: Linus Walleij @ 2020-03-26 21:26 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Mark Rutland, Peter Maydell, QEMU Developers, Phil Reid,
	Jonathan Corbet, Marc Zyngier, open list:GPIO SUBSYSTEM,
	Linux Doc Mailing List, Magnus Damm, Christoffer Dall,
	linux-kernel, Linux-Renesas, Bartosz Golaszewski, Rob Herring,
	Harish Jenny K N, Paolo Bonzini, Alexander Graf, Eugeniu Rosca

On Tue, Mar 24, 2020 at 2:57 PM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:

> The GPIO Aggregator will need a method to forward a .set_config() call
> to its parent gpiochip.  This requires obtaining the gpio_chip and
> offset for a given gpio_desc.  While gpiod_to_chip() is public,
> gpio_chip_hwgpio() is not, so there is currently no method to obtain the
> needed GPIO offset parameter.
>
> Hence introduce a public gpiod_set_config() helper, which invokes the
> .set_config() callback through a gpio_desc pointer, like is done for
> most other gpio_chip callbacks.
>
> Rewrite the existing gpiod_set_debounce() helper as a wrapper around
> gpiod_set_config(), to avoid duplication.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v6:
>   - New.

This is nice, I tried to actually just apply this (you also sent some
two cleanups that I tried to apply) byt Yue's cleanup patch
commit d18fddff061d2796525e6d4a958cb3d30aed8efd
"gpiolib: Remove duplicated function gpio_do_set_config()"
makes none of them apply :/

Yours,
Linus Walleij


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

* Re: [PATCH v6 5/8] gpiolib: Introduce gpiod_set_config()
  2020-03-26 21:26     ` Linus Walleij
@ 2020-03-27  8:45       ` Geert Uytterhoeven
  2020-03-27 21:33         ` Linus Walleij
  0 siblings, 1 reply; 19+ messages in thread
From: Geert Uytterhoeven @ 2020-03-27  8:45 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Mark Rutland, Peter Maydell, QEMU Developers, Phil Reid,
	Jonathan Corbet, Marc Zyngier, open list:GPIO SUBSYSTEM,
	Linux Doc Mailing List, Magnus Damm, Christoffer Dall,
	linux-kernel, Linux-Renesas, Bartosz Golaszewski, Rob Herring,
	Harish Jenny K N, Paolo Bonzini, Alexander Graf, Eugeniu Rosca

Hi Linus,

On Thu, Mar 26, 2020 at 10:26 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Mar 24, 2020 at 2:57 PM Geert Uytterhoeven
> <geert+renesas@glider.be> wrote:
> > The GPIO Aggregator will need a method to forward a .set_config() call
> > to its parent gpiochip.  This requires obtaining the gpio_chip and
> > offset for a given gpio_desc.  While gpiod_to_chip() is public,
> > gpio_chip_hwgpio() is not, so there is currently no method to obtain the
> > needed GPIO offset parameter.
> >
> > Hence introduce a public gpiod_set_config() helper, which invokes the
> > .set_config() callback through a gpio_desc pointer, like is done for
> > most other gpio_chip callbacks.
> >
> > Rewrite the existing gpiod_set_debounce() helper as a wrapper around
> > gpiod_set_config(), to avoid duplication.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > v6:
> >   - New.
>
> This is nice, I tried to actually just apply this (you also sent some
> two cleanups that I tried to apply) byt Yue's cleanup patch
> commit d18fddff061d2796525e6d4a958cb3d30aed8efd
> "gpiolib: Remove duplicated function gpio_do_set_config()"
> makes none of them apply :/

/me confused.

That commit was reverted later, so it shouldn't matter.

I have just verified, and both my full series and just this single
patch, do apply fine to all of current gpio/for-next, linus/master, and
next-20200327.  They even apply fine to gpio/for-next before or after
the two cleanups I sent, too.

What am I missing?
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] 19+ messages in thread

* Re: [PATCH v6 5/8] gpiolib: Introduce gpiod_set_config()
  2020-03-27  8:45       ` Geert Uytterhoeven
@ 2020-03-27 21:33         ` Linus Walleij
  0 siblings, 0 replies; 19+ messages in thread
From: Linus Walleij @ 2020-03-27 21:33 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Mark Rutland, Peter Maydell, QEMU Developers, Phil Reid,
	Jonathan Corbet, Marc Zyngier, open list:GPIO SUBSYSTEM,
	Linux Doc Mailing List, Magnus Damm, Christoffer Dall,
	linux-kernel, Linux-Renesas, Bartosz Golaszewski, Rob Herring,
	Harish Jenny K N, Paolo Bonzini, Alexander Graf, Eugeniu Rosca

On Fri, Mar 27, 2020 at 9:45 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Thu, Mar 26, 2020 at 10:26 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Tue, Mar 24, 2020 at 2:57 PM Geert Uytterhoeven
> > <geert+renesas@glider.be> wrote:
> > > The GPIO Aggregator will need a method to forward a .set_config() call
> > > to its parent gpiochip.  This requires obtaining the gpio_chip and
> > > offset for a given gpio_desc.  While gpiod_to_chip() is public,
> > > gpio_chip_hwgpio() is not, so there is currently no method to obtain the
> > > needed GPIO offset parameter.
> > >
> > > Hence introduce a public gpiod_set_config() helper, which invokes the
> > > .set_config() callback through a gpio_desc pointer, like is done for
> > > most other gpio_chip callbacks.
> > >
> > > Rewrite the existing gpiod_set_debounce() helper as a wrapper around
> > > gpiod_set_config(), to avoid duplication.
> > >
> > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > ---
> > > v6:
> > >   - New.
> >
> > This is nice, I tried to actually just apply this (you also sent some
> > two cleanups that I tried to apply) byt Yue's cleanup patch
> > commit d18fddff061d2796525e6d4a958cb3d30aed8efd
> > "gpiolib: Remove duplicated function gpio_do_set_config()"
> > makes none of them apply :/
>
> /me confused.
>
> That commit was reverted later, so it shouldn't matter.
>
> I have just verified, and both my full series and just this single
> patch, do apply fine to all of current gpio/for-next, linus/master, and
> next-20200327.  They even apply fine to gpio/for-next before or after
> the two cleanups I sent, too.
>
> What am I missing?

Ah I see, it is because my development branch is based on
v5.6-rc1. So I have to merge in a later -rc where this revert
is applied so that this applies.

Yours,
Linus Walleij


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

* Re: [PATCH v6 5/8] gpiolib: Introduce gpiod_set_config()
  2020-03-24 13:56   ` [PATCH v6 5/8] gpiolib: Introduce gpiod_set_config() Geert Uytterhoeven
  2020-03-26 21:26     ` Linus Walleij
@ 2020-03-27 21:37     ` Linus Walleij
  1 sibling, 0 replies; 19+ messages in thread
From: Linus Walleij @ 2020-03-27 21:37 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Mark Rutland, Peter Maydell, QEMU Developers, Phil Reid,
	Jonathan Corbet, Marc Zyngier, open list:GPIO SUBSYSTEM,
	Linux Doc Mailing List, Magnus Damm, Christoffer Dall,
	linux-kernel, Linux-Renesas, Bartosz Golaszewski, Rob Herring,
	Harish Jenny K N, Paolo Bonzini, Alexander Graf, Eugeniu Rosca

On Tue, Mar 24, 2020 at 2:57 PM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:

> The GPIO Aggregator will need a method to forward a .set_config() call
> to its parent gpiochip.  This requires obtaining the gpio_chip and
> offset for a given gpio_desc.  While gpiod_to_chip() is public,
> gpio_chip_hwgpio() is not, so there is currently no method to obtain the
> needed GPIO offset parameter.
>
> Hence introduce a public gpiod_set_config() helper, which invokes the
> .set_config() callback through a gpio_desc pointer, like is done for
> most other gpio_chip callbacks.
>
> Rewrite the existing gpiod_set_debounce() helper as a wrapper around
> gpiod_set_config(), to avoid duplication.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v6:
>   - New.

Patch applied in preparation for the next kernel cycle
so we get Geert's patch stack down.

Yours,
Linus Walleij


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

* Re: [PATCH v6 3/8] mfd: sm501: Use GPIO_LOOKUP_IDX() helper macro
  2020-03-24 13:56   ` [PATCH v6 3/8] mfd: sm501: Use GPIO_LOOKUP_IDX() " Geert Uytterhoeven
@ 2020-04-15  9:25     ` Lee Jones
  0 siblings, 0 replies; 19+ messages in thread
From: Lee Jones @ 2020-04-15  9:25 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Mark Rutland, Peter Maydell, qemu-devel, Phil Reid,
	Jonathan Corbet, Marc Zyngier, Linus Walleij, linux-doc,
	Magnus Damm, Christoffer Dall, linux-kernel, linux-renesas-soc,
	Bartosz Golaszewski, Rob Herring, Harish Jenny K N, linux-gpio,
	Paolo Bonzini, Alexander Graf, Eugeniu Rosca

On Tue, 24 Mar 2020, Geert Uytterhoeven wrote:

> i801_add_mux() fills in the GPIO lookup table by manually populating an
> array of gpiod_lookup structures.  Use the existing GPIO_LOOKUP_IDX()
> helper macro instead, to relax a dependency on the gpiod_lookup
> structure's member names.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Lee Jones <lee.jones@linaro.org>
> ---
> While this patch is a dependency for "[PATCH v6 4/8] gpiolib: Add
> support for GPIO lookup by line name", it can be applied independently.
> But an Acked-by would be nice, too.
> 
> Cover letter and full series at
> https://lore.kernel.org/r/20200324135328.5796-1-geert+renesas@glider.be/
> 
> v6:
>   - New.
> ---
>  drivers/mfd/sm501.c | 24 ++++++++----------------
>  1 file changed, 8 insertions(+), 16 deletions(-)

Acked-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog


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

* Re: [PATCH v6 2/8] i2c: i801: Use GPIO_LOOKUP() helper macro
  2020-03-24 13:56   ` [PATCH v6 2/8] i2c: i801: " Geert Uytterhoeven
  2020-03-25  9:14     ` Jean Delvare
@ 2020-04-15 10:57     ` Wolfram Sang
  1 sibling, 0 replies; 19+ messages in thread
From: Wolfram Sang @ 2020-04-15 10:57 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Mark Rutland, Peter Maydell, qemu-devel, Phil Reid,
	Jonathan Corbet, Marc Zyngier, Linus Walleij, linux-doc,
	Magnus Damm, Christoffer Dall, linux-kernel, linux-renesas-soc,
	Bartosz Golaszewski, Jean Delvare, Rob Herring, Harish Jenny K N,
	linux-i2c, linux-gpio, Paolo Bonzini, Alexander Graf,
	Eugeniu Rosca

[-- Attachment #1: Type: text/plain, Size: 483 bytes --]

On Tue, Mar 24, 2020 at 02:56:47PM +0100, Geert Uytterhoeven wrote:
> i801_add_mux() fills in the GPIO lookup table by manually populating an
> array of gpiod_lookup structures.  Use the existing GPIO_LOOKUP() helper
> macro instead, to relax a dependency on the gpiod_lookup structure's
> member names.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Jean Delvare <jdelvare@suse.com>
> Cc: linux-i2c@vger.kernel.org

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

* Re: [PATCH v6 4/8] gpiolib: Add support for GPIO lookup by line name
  2020-03-26 21:18     ` Linus Walleij
@ 2020-05-11 10:18       ` Geert Uytterhoeven
  0 siblings, 0 replies; 19+ messages in thread
From: Geert Uytterhoeven @ 2020-05-11 10:18 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Mark Rutland, Peter Maydell, QEMU Developers, Phil Reid,
	Jonathan Corbet, Marc Zyngier, open list:GPIO SUBSYSTEM,
	Linux Doc Mailing List, Magnus Damm, Christoffer Dall,
	linux-kernel, Linux-Renesas, Bartosz Golaszewski, Rob Herring,
	Harish Jenny K N, Paolo Bonzini, Alexander Graf, Eugeniu Rosca

Hi Linus,

On Thu, Mar 26, 2020 at 10:18 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Mar 24, 2020 at 2:57 PM Geert Uytterhoeven
> <geert+renesas@glider.be> wrote:
> > Currently a GPIO lookup table can only refer to a specific GPIO by a
> > tuple, consisting of a GPIO controller label and a GPIO offset inside
> > the controller.
> >
> > However, a GPIO may also carry a line name, defined by DT or ACPI.
> > If present, the line name is the most use-centric way to refer to a
> > GPIO.  Hence add support for looking up GPIOs by line name.
> >
> > Implement this by reusing the existing gpiod_lookup infrastructure.
> > Rename gpiod_lookup.chip_label to gpiod_lookup.key, to make it clear
> > that this field can have two meanings, and update the kerneldoc and
> > GPIO_LOOKUP*() macros.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>
> > Reviewed-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> > Tested-by: Eugeniu Rosca <erosca@de.adit-jv.com>
>
> I kind of like this approach, however there are things here that
> need to be considered: the line name is in no way globally unique,
> and I think there are already quite a few GPIO chips that
> have the same line names assigned for every instance of that
> chip.
>
> gpiochip_set_desc_names() only warns if there is a line with
> the same name on the same gpio_chip.

on a _different_ gpio chip.

> I suppose we need to document that the line name look-up
> will be on a first-come-first-served basis: whatever line
> we find first with this name is what you will get a reference
> to, no matter what chip it is on, and it is possible albeit
> not recommended that some other chip has a line with the
> same name.

Agreed.

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] 19+ messages in thread

end of thread, other threads:[~2020-05-11 10:19 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-24 13:53 [PATCH v6 0/8] gpio: Add GPIO Aggregator Geert Uytterhoeven
2020-03-24 13:56 ` [PATCH v6 1/8] ARM: integrator: impd1: Use GPIO_LOOKUP() helper macro Geert Uytterhoeven
2020-03-24 13:56   ` [PATCH v6 2/8] i2c: i801: " Geert Uytterhoeven
2020-03-25  9:14     ` Jean Delvare
2020-04-15 10:57     ` Wolfram Sang
2020-03-24 13:56   ` [PATCH v6 3/8] mfd: sm501: Use GPIO_LOOKUP_IDX() " Geert Uytterhoeven
2020-04-15  9:25     ` Lee Jones
2020-03-24 13:56   ` [PATCH v6 4/8] gpiolib: Add support for GPIO lookup by line name Geert Uytterhoeven
2020-03-26 21:18     ` Linus Walleij
2020-05-11 10:18       ` Geert Uytterhoeven
2020-03-24 13:56   ` [PATCH v6 5/8] gpiolib: Introduce gpiod_set_config() Geert Uytterhoeven
2020-03-26 21:26     ` Linus Walleij
2020-03-27  8:45       ` Geert Uytterhoeven
2020-03-27 21:33         ` Linus Walleij
2020-03-27 21:37     ` Linus Walleij
2020-03-24 13:56   ` [PATCH v6 6/8] gpio: Add GPIO Aggregator Geert Uytterhoeven
2020-03-24 13:56   ` [PATCH v6 7/8] docs: gpio: Add GPIO Aggregator documentation Geert Uytterhoeven
2020-03-24 13:56   ` [PATCH v6 8/8] MAINTAINERS: Add GPIO Aggregator section Geert Uytterhoeven
2020-03-26 21:07   ` [PATCH v6 1/8] ARM: integrator: impd1: Use GPIO_LOOKUP() helper macro Linus Walleij

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).