LinuxLists.cc - [PATCH v3 0/7] gpio: Add GPIO Aggregator/Repeater

[permalink] [raw]

Subject: [PATCH v3 6/7] docs: gpio: Add GPIO Aggregator/Repeater documentation

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

Signed-off-by: Geert Uytterhoeven <[email protected]>
---
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

2019-11-27 08:46:20

[permalink] [raw]

Subject: [PATCH v3 2/7] gpiolib: Add support for gpiochipN-based table lookup

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 <[email protected]>
---
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

2019-11-27 08:47:44

[permalink] [raw]

Subject: [PATCH v3 4/7] dt-bindings: gpio: Add gpio-repeater bindings

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
<[email protected]>[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/[email protected]/

[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/[email protected]/

Signed-off-by: Geert Uytterhoeven <[email protected]>
---
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 <[email protected]>
+ - Geert Uytterhoeven <[email protected]>
+
+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

2019-11-27 08:47:45

[permalink] [raw]

Subject: [PATCH v3 1/7] gpiolib: Add GPIOCHIP_NAME definition

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 <[email protected]>
---
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

2019-11-28 03:46:46

[permalink] [raw]

Subject: Re: [PATCH v3 2/7] gpiolib: Add support for gpiochipN-based table lookup

> On November 27, 2019 at 9:42 AM Geert Uytterhoeven <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>

CU
Uli

2019-11-28 03:48:22

[permalink] [raw]

Subject: Re: [PATCH v3 1/7] gpiolib: Add GPIOCHIP_NAME definition

> On November 27, 2019 at 9:42 AM Geert Uytterhoeven <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>

CU
Uli

2019-11-28 03:49:06

[permalink] [raw]

Subject: Re: [PATCH v3 6/7] docs: gpio: Add GPIO Aggregator/Repeater documentation

> On November 27, 2019 at 9:42 AM Geert Uytterhoeven <[email protected]> wrote:
>
>
> Document the GPIO Aggregator/Repeater, and the three typical use-cases.
>
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> ---
> 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 <[email protected]>

CU
Uli

2019-11-28 03:50:14

[permalink] [raw]

Subject: Re: [PATCH v3 4/7] dt-bindings: gpio: Add gpio-repeater bindings

> On November 27, 2019 at 9:42 AM Geert Uytterhoeven <[email protected]> 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
> <[email protected]>[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/[email protected]/
>
> [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/[email protected]/
>
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> ---
> 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 <[email protected]>
> + - Geert Uytterhoeven <[email protected]>
> +
> +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 <[email protected]>

CU
Uli

2019-12-02 21:19:05

by Eugeniu Rosca

[permalink] [raw]

Subject: Re: [PATCH v3 1/7] gpiolib: Add GPIOCHIP_NAME definition

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 <[email protected]>
> ---
> 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 <[email protected]>

[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

2019-12-03 05:53:29

by Harish Jenny K N

[permalink] [raw]

Subject: Re: [PATCH v3 4/7] dt-bindings: gpio: Add gpio-repeater bindings

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
> <[email protected]>[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/[email protected]/
>
> [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/[email protected]/
>
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> ---
> 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 <[email protected]>
> + - Geert Uytterhoeven <[email protected]>
> +
> +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.

2019-12-05 21:07:43

by Rob Herring

[permalink] [raw]

Subject: Re: [PATCH v3 4/7] dt-bindings: gpio: Add gpio-repeater bindings

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
> <[email protected]>[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/[email protected]/
>
> [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/[email protected]/
>
> Signed-off-by: Geert Uytterhoeven <[email protected]>

2019-12-06 09:18:45

[permalink] [raw]

Subject: Re: [PATCH v3 4/7] dt-bindings: gpio: Add gpio-repeater bindings

Hi Rob,

On Thu, Dec 5, 2019 at 10:06 PM Rob Herring <[email protected]> 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
> > <[email protected]>[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/[email protected]/
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 -- [email protected]

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

2019-12-06 15:06:40

by Rob Herring

[permalink] [raw]

Subject: Re: [PATCH v3 4/7] dt-bindings: gpio: Add gpio-repeater bindings

On Fri, Dec 6, 2019 at 3:17 AM Geert Uytterhoeven <[email protected]> wrote:
>
> Hi Rob,
>
> On Thu, Dec 5, 2019 at 10:06 PM Rob Herring <[email protected]> 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
> > > <[email protected]>[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/[email protected]/
> 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

2019-12-12 10:38:44

[permalink] [raw]

Subject: Re: [PATCH v3 1/7] gpiolib: Add GPIOCHIP_NAME definition

On Wed, Nov 27, 2019 at 9:43 AM Geert Uytterhoeven
<[email protected]> 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 <[email protected]>
> ---
> 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

2019-12-12 13:21:42

[permalink] [raw]

Subject: Re: [PATCH v3 2/7] gpiolib: Add support for gpiochipN-based table lookup

Hi Geert!

On Wed, Nov 27, 2019 at 9:43 AM Geert Uytterhoeven
<[email protected]> 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 <[email protected]>

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

2019-12-12 13:34:19

[permalink] [raw]

Subject: Re: [PATCH v3 2/7] gpiolib: Add support for gpiochipN-based table lookup

Hi Linus,

On Thu, Dec 12, 2019 at 2:20 PM Linus Walleij <[email protected]> wrote:
> On Wed, Nov 27, 2019 at 9:43 AM Geert Uytterhoeven
> <[email protected]> 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 <[email protected]>
>
> 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 -- [email protected]

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

2019-12-12 14:39:32

[permalink] [raw]

Subject: Re: [PATCH v3 2/7] gpiolib: Add support for gpiochipN-based table lookup

On Thu, Dec 12, 2019 at 2:33 PM Geert Uytterhoeven <[email protected]> wrote:
> On Thu, Dec 12, 2019 at 2:20 PM Linus Walleij <[email protected]> wrote:
> > On Wed, Nov 27, 2019 at 9:43 AM Geert Uytterhoeven
> > <[email protected]> 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 <[email protected]>
> >
> > 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

2019-12-12 14:43:09

[permalink] [raw]

Subject: Re: [PATCH v3 6/7] docs: gpio: Add GPIO Aggregator/Repeater documentation

On Wed, Nov 27, 2019 at 9:43 AM Geert Uytterhoeven
<[email protected]> 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

2019-12-12 14:49:44

[permalink] [raw]

Subject: Re: [PATCH v3 6/7] docs: gpio: Add GPIO Aggregator/Repeater documentation

Hi Linus,

On Thu, Dec 12, 2019 at 3:42 PM Linus Walleij <[email protected]> wrote:
> On Wed, Nov 27, 2019 at 9:43 AM Geert Uytterhoeven
> <[email protected]> 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 -- [email protected]

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

2020-01-04 00:22:34

[permalink] [raw]

Subject: Re: [PATCH v3 6/7] docs: gpio: Add GPIO Aggregator/Repeater documentation

On Thu, Dec 12, 2019 at 3:48 PM Geert Uytterhoeven <[email protected]> wrote:
> On Thu, Dec 12, 2019 at 3:42 PM Linus Walleij <[email protected]> wrote:
> > On Wed, Nov 27, 2019 at 9:43 AM Geert Uytterhoeven
> > <[email protected]> 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

2020-01-06 08:08:24

[permalink] [raw]

Subject: Re: [PATCH v3 6/7] docs: gpio: Add GPIO Aggregator/Repeater documentation

Hi Linus,

On Sat, Jan 4, 2020 at 1:21 AM Linus Walleij <[email protected]> wrote:
> On Thu, Dec 12, 2019 at 3:48 PM Geert Uytterhoeven <[email protected]> wrote:
> > On Thu, Dec 12, 2019 at 3:42 PM Linus Walleij <[email protected]> wrote:
> > > On Wed, Nov 27, 2019 at 9:43 AM Geert Uytterhoeven
> > > <[email protected]> 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 -- [email protected]

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

2020-01-06 08:14:44

[permalink] [raw]

Subject: Re: [PATCH v3 4/7] dt-bindings: gpio: Add gpio-repeater bindings

Hi Rob,

On Fri, Dec 6, 2019 at 4:04 PM Rob Herring <[email protected]> wrote:
> On Fri, Dec 6, 2019 at 3:17 AM Geert Uytterhoeven <[email protected]> wrote:
> > On Thu, Dec 5, 2019 at 10:06 PM Rob Herring <[email protected]> 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
> > > > <[email protected]>[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/[email protected]/
> > 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 -- [email protected]

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

2020-01-07 09:24:18

by Harish Jenny K N

[permalink] [raw]

Subject: Re: [PATCH v3 4/7] dt-bindings: gpio: Add gpio-repeater bindings

On 06/01/20 1:42 PM, Geert Uytterhoeven wrote:
> Hi Rob,
>
> On Fri, Dec 6, 2019 at 4:04 PM Rob Herring <[email protected]> wrote:
>> On Fri, Dec 6, 2019 at 3:17 AM Geert Uytterhoeven <[email protected]> wrote:
>>> On Thu, Dec 5, 2019 at 10:06 PM Rob Herring <[email protected]> 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
>>>>> <[email protected]>[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/[email protected]/
>>> 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
>

2020-01-16 05:42:04

by Harish Jenny K N

[permalink] [raw]

Subject: Re: [PATCH v3 4/7] dt-bindings: gpio: Add gpio-repeater bindings

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 <[email protected]> wrote:
>>> On Fri, Dec 6, 2019 at 3:17 AM Geert Uytterhoeven <[email protected]> wrote:
>>>> On Thu, Dec 5, 2019 at 10:06 PM Rob Herring <[email protected]> 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
>>>>>> <[email protected]>[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/[email protected]/
>>>> 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/[email protected]/ still not acceptable?

Thanks,

Harish

2020-01-18 01:49:02

by Eugeniu Rosca

[permalink] [raw]

Subject: Re: [PATCH v3 0/7] gpio: Add GPIO Aggregator/Repeater

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 <[email protected]>
Tested-by: Eugeniu Rosca <[email protected]>

Thanks!

--
Best Regards,
Eugeniu

2020-01-20 09:35:11