[PATCH v2 0/3] Rewrite GPIO LED trigger to use trigger-sources

[PATCH v2 0/3] Rewrite GPIO LED trigger to use trigger-sources

[Linus Walleij]

This rewrites the platform-data GPIO LED trigger to instead
use fwnode trigger-sources to describe the LED used.

This will work out-of-the-box with e.g. device tree.

Tested with real hardware by modifying a device tree adding
trigger-sources to a LED and trigger-source-cells to
a gpio chip, setting the trigger to a pushbutton. It works
like a charm, once the trigger is set to "gpio".

Adding trigger-sources to GPIO chips in a simple way requires
this patch to be merged to the generic dtschema:
https://lore.kernel.org/linux-devicetree/20230916-gpio-triggers-v1-1-6e5052bead9a@linaro.org/

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Changes in v2:
- Add a quirk to gpiolib-of to allow to read out trigger-sources as
  any regular GPIO.
- Fix a use-after-free bug found by Dan Carpenter.
- Tested on hardware.
- Link to v1: https://lore.kernel.org/r/20230912-gpio-led-trigger-dt-v1-0-1b50e3756dda@linaro.org

---
Linus Walleij (3):
      gpiolib: of: Allow "trigger-sources" to reference a GPIO
      dt-bindings: leds: Mention GPIO triggers
      leds: triggers: gpio: Rewrite to use trigger-sources

 Documentation/devicetree/bindings/leds/common.yaml |   2 +
 drivers/gpio/gpiolib-of.c                          |  28 +++++
 drivers/leds/trigger/Kconfig                       |   5 +-
 drivers/leds/trigger/ledtrig-gpio.c                | 137 ++++++---------------
 4 files changed, 71 insertions(+), 101 deletions(-)
---
base-commit: 8eb1c4d9b44873d30efc1846148944534f4a017d
change-id: 20230911-gpio-led-trigger-dt-922bbe21fa22

Best regards,
-- 
Linus Walleij <linus.walleij@linaro.org>

[PATCH v2 1/3] gpiolib: of: Allow "trigger-sources" to reference a GPIO

[Linus Walleij]

The "trigger-sources" phandle used for LED triggers are special:
the DT bindings mandate that such triggers have the same phandle
references no matter what the trigger is. A GPIO is just another
kind of device that can trigger a LED.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpiolib-of.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 531faabead0f..f4a660bf11fd 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -611,6 +611,33 @@ static struct gpio_desc *of_find_mt2701_gpio(struct device_node *np,
 	return desc;
 }
 
+/*
+ * Trigger sources are special, they allow us to use any GPIO as a LED trigger
+ * and have the name "trigger-sources" no matter which kind of phandle it is
+ * pointing to, whether to a GPIO, a USB host, a network PHY etc. So in this case
+ * we allow looking something up that is not named "foo-gpios".
+ */
+static struct gpio_desc *of_find_trigger_gpio(struct device_node *np,
+					      const char *con_id,
+					      unsigned int idx,
+					      enum of_gpio_flags *of_flags)
+{
+	struct gpio_desc *desc;
+
+	if (!IS_ENABLED(CONFIG_LEDS_TRIGGER_GPIO))
+		return ERR_PTR(-ENOENT);
+
+	if (!con_id || strcmp(con_id, "trigger-sources"))
+		return ERR_PTR(-ENOENT);
+
+	desc = of_get_named_gpiod_flags(np, con_id, idx, of_flags);
+	if (!gpiod_not_found(desc))
+		pr_debug("%s is used as a trigger\n", of_node_full_name(np));
+
+	return desc;
+}
+
+
 typedef struct gpio_desc *(*of_find_gpio_quirk)(struct device_node *np,
 						const char *con_id,
 						unsigned int idx,
@@ -618,6 +645,7 @@ typedef struct gpio_desc *(*of_find_gpio_quirk)(struct device_node *np,
 static const of_find_gpio_quirk of_find_gpio_quirks[] = {
 	of_find_gpio_rename,
 	of_find_mt2701_gpio,
+	of_find_trigger_gpio,
 	NULL
 };
 

-- 
2.34.1

[PATCH v2 2/3] dt-bindings: leds: Mention GPIO triggers

[Linus Walleij]

We reuse the trigger-sources phandle to just point to
GPIOs we may want to use as LED triggers.

Example:

gpio: gpio@0 {
    compatible "my-gpio";
    gpio-controller;
    #gpio-cells = <2>;
    interrupt-controller;
    #interrupt-cells = <2>;
    #trigger-source-cells = <2>;
};

leds {
    compatible = "gpio-leds";
    led-my-gpio {
        label = "device:blue:myled";
        gpios = <&gpio 0 GPIO_ACTIVE_HIGH>;
        default-state = "off";
        linux,default-trigger = "gpio";
        trigger-sources = <&gpio 1 GPIO_ACTIVE_HIGH>;
    };
};

Acked-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 Documentation/devicetree/bindings/leds/common.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/common.yaml b/Documentation/devicetree/bindings/leds/common.yaml
index 5fb7007f3618..b42950643b9d 100644
--- a/Documentation/devicetree/bindings/leds/common.yaml
+++ b/Documentation/devicetree/bindings/leds/common.yaml
@@ -191,6 +191,8 @@ properties:
       each of them having its own LED assigned (assuming they are not
       hardwired). In such cases this property should contain phandle(s) of
       related source device(s).
+      Another example is a GPIO line that will be monitored and mirror the
+      state of the line (with or without inversion flags) to the LED.
       In many cases LED can be related to more than one device (e.g. one USB LED
       vs. multiple USB ports). Each source should be represented by a node in
       the device tree and be referenced by a phandle and a set of phandle

-- 
2.34.1

[PATCH v2 3/3] leds: triggers: gpio: Rewrite to use trigger-sources

[Linus Walleij]

By providing a GPIO line as "trigger-sources" in the FWNODE
(such as from the device tree) and combining with the
GPIO trigger, we can support a GPIO LED trigger in a natural
way from the hardware description instead of using the
custom sysfs and deprecated global GPIO numberspace.

Example:

gpio: gpio@0 {
    compatible "my-gpio";
    gpio-controller;
    #gpio-cells = <2>;
    interrupt-controller;
    #interrupt-cells = <2>;
    #trigger-source-cells = <2>;
};

leds {
    compatible = "gpio-leds";
    led-my-gpio {
        label = "device:blue:myled";
        gpios = <&gpio 0 GPIO_ACTIVE_HIGH>;
        default-state = "off";
        linux,default-trigger = "gpio";
        trigger-sources = <&gpio 1 GPIO_ACTIVE_HIGH>;
    };
};

Make this the norm, unmark the driver as broken.

Delete the sysfs handling of GPIOs.

Since GPIO descriptors inherently can describe inversion,
the inversion handling can just be deleted.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Fix a use-after-free bug found by Dan Carpenter
---
 drivers/leds/trigger/Kconfig        |   5 +-
 drivers/leds/trigger/ledtrig-gpio.c | 137 +++++++++++-------------------------
 2 files changed, 41 insertions(+), 101 deletions(-)

diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
index 2a57328eca20..d11d80176fc0 100644
--- a/drivers/leds/trigger/Kconfig
+++ b/drivers/leds/trigger/Kconfig
@@ -83,13 +83,10 @@ config LEDS_TRIGGER_ACTIVITY
 config LEDS_TRIGGER_GPIO
 	tristate "LED GPIO Trigger"
 	depends on GPIOLIB || COMPILE_TEST
-	depends on BROKEN
 	help
 	  This allows LEDs to be controlled by gpio events. It's good
 	  when using gpios as switches and triggering the needed LEDs
-	  from there. One use case is n810's keypad LEDs that could
-	  be triggered by this trigger when user slides up to show
-	  keypad.
+	  from there. Triggers are defined as device properties.
 
 	  If unsure, say N.
 
diff --git a/drivers/leds/trigger/ledtrig-gpio.c b/drivers/leds/trigger/ledtrig-gpio.c
index 0120faa3dafa..9b7fe5dd5208 100644
--- a/drivers/leds/trigger/ledtrig-gpio.c
+++ b/drivers/leds/trigger/ledtrig-gpio.c
@@ -3,12 +3,13 @@
  * ledtrig-gio.c - LED Trigger Based on GPIO events
  *
  * Copyright 2009 Felipe Balbi <me@felipebalbi.com>
+ * Copyright 2023 Linus Walleij <linus.walleij@linaro.org>
  */
 
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
-#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/interrupt.h>
 #include <linux/leds.h>
 #include <linux/slab.h>
@@ -16,10 +17,8 @@
 
 struct gpio_trig_data {
 	struct led_classdev *led;
-
 	unsigned desired_brightness;	/* desired brightness when led is on */
-	unsigned inverted;		/* true when gpio is inverted */
-	unsigned gpio;			/* gpio that triggers the leds */
+	struct gpio_desc *gpiod;	/* gpio that triggers the led */
 };
 
 static irqreturn_t gpio_trig_irq(int irq, void *_led)
@@ -28,10 +27,7 @@ static irqreturn_t gpio_trig_irq(int irq, void *_led)
 	struct gpio_trig_data *gpio_data = led_get_trigger_data(led);
 	int tmp;
 
-	tmp = gpio_get_value_cansleep(gpio_data->gpio);
-	if (gpio_data->inverted)
-		tmp = !tmp;
-
+	tmp = gpiod_get_value_cansleep(gpio_data->gpiod);
 	if (tmp) {
 		if (gpio_data->desired_brightness)
 			led_set_brightness_nosleep(gpio_data->led,
@@ -73,93 +69,8 @@ static ssize_t gpio_trig_brightness_store(struct device *dev,
 static DEVICE_ATTR(desired_brightness, 0644, gpio_trig_brightness_show,
 		gpio_trig_brightness_store);
 
-static ssize_t gpio_trig_inverted_show(struct device *dev,
-		struct device_attribute *attr, char *buf)
-{
-	struct gpio_trig_data *gpio_data = led_trigger_get_drvdata(dev);
-
-	return sprintf(buf, "%u\n", gpio_data->inverted);
-}
-
-static ssize_t gpio_trig_inverted_store(struct device *dev,
-		struct device_attribute *attr, const char *buf, size_t n)
-{
-	struct led_classdev *led = led_trigger_get_led(dev);
-	struct gpio_trig_data *gpio_data = led_trigger_get_drvdata(dev);
-	unsigned long inverted;
-	int ret;
-
-	ret = kstrtoul(buf, 10, &inverted);
-	if (ret < 0)
-		return ret;
-
-	if (inverted > 1)
-		return -EINVAL;
-
-	gpio_data->inverted = inverted;
-
-	/* After inverting, we need to update the LED. */
-	if (gpio_is_valid(gpio_data->gpio))
-		gpio_trig_irq(0, led);
-
-	return n;
-}
-static DEVICE_ATTR(inverted, 0644, gpio_trig_inverted_show,
-		gpio_trig_inverted_store);
-
-static ssize_t gpio_trig_gpio_show(struct device *dev,
-		struct device_attribute *attr, char *buf)
-{
-	struct gpio_trig_data *gpio_data = led_trigger_get_drvdata(dev);
-
-	return sprintf(buf, "%u\n", gpio_data->gpio);
-}
-
-static ssize_t gpio_trig_gpio_store(struct device *dev,
-		struct device_attribute *attr, const char *buf, size_t n)
-{
-	struct led_classdev *led = led_trigger_get_led(dev);
-	struct gpio_trig_data *gpio_data = led_trigger_get_drvdata(dev);
-	unsigned gpio;
-	int ret;
-
-	ret = sscanf(buf, "%u", &gpio);
-	if (ret < 1) {
-		dev_err(dev, "couldn't read gpio number\n");
-		return -EINVAL;
-	}
-
-	if (gpio_data->gpio == gpio)
-		return n;
-
-	if (!gpio_is_valid(gpio)) {
-		if (gpio_is_valid(gpio_data->gpio))
-			free_irq(gpio_to_irq(gpio_data->gpio), led);
-		gpio_data->gpio = gpio;
-		return n;
-	}
-
-	ret = request_threaded_irq(gpio_to_irq(gpio), NULL, gpio_trig_irq,
-			IRQF_ONESHOT | IRQF_SHARED | IRQF_TRIGGER_RISING
-			| IRQF_TRIGGER_FALLING, "ledtrig-gpio", led);
-	if (ret) {
-		dev_err(dev, "request_irq failed with error %d\n", ret);
-	} else {
-		if (gpio_is_valid(gpio_data->gpio))
-			free_irq(gpio_to_irq(gpio_data->gpio), led);
-		gpio_data->gpio = gpio;
-		/* After changing the GPIO, we need to update the LED. */
-		gpio_trig_irq(0, led);
-	}
-
-	return ret ? ret : n;
-}
-static DEVICE_ATTR(gpio, 0644, gpio_trig_gpio_show, gpio_trig_gpio_store);
-
 static struct attribute *gpio_trig_attrs[] = {
 	&dev_attr_desired_brightness.attr,
-	&dev_attr_inverted.attr,
-	&dev_attr_gpio.attr,
 	NULL
 };
 ATTRIBUTE_GROUPS(gpio_trig);
@@ -167,16 +78,48 @@ ATTRIBUTE_GROUPS(gpio_trig);
 static int gpio_trig_activate(struct led_classdev *led)
 {
 	struct gpio_trig_data *gpio_data;
+	struct device *dev = led->dev;
+	int ret;
 
 	gpio_data = kzalloc(sizeof(*gpio_data), GFP_KERNEL);
 	if (!gpio_data)
 		return -ENOMEM;
 
-	gpio_data->led = led;
-	gpio_data->gpio = -ENOENT;
+	/*
+	 * The generic property "trigger-sources" is followed,
+	 * and we hope that this is a GPIO.
+	 */
+	gpio_data->gpiod = fwnode_gpiod_get_index(dev->fwnode,
+						  "trigger-sources",
+						  0, GPIOD_IN,
+						  "led-trigger");
+	if (IS_ERR(gpio_data->gpiod)) {
+		ret = PTR_ERR(gpio_data->gpiod);
+		kfree(gpio_data);
+		return ret;
+	}
+	if (!gpio_data->gpiod) {
+		dev_err(dev, "no valid GPIO for the trigger\n");
+		kfree(gpio_data);
+		return -EINVAL;
+	}
 
+	gpio_data->led = led;
 	led_set_trigger_data(led, gpio_data);
 
+	ret = request_threaded_irq(gpiod_to_irq(gpio_data->gpiod), NULL, gpio_trig_irq,
+			IRQF_ONESHOT | IRQF_SHARED | IRQF_TRIGGER_RISING
+			| IRQF_TRIGGER_FALLING, "ledtrig-gpio", led);
+	if (ret) {
+		dev_err(dev, "request_irq failed with error %d\n", ret);
+		gpiod_put(gpio_data->gpiod);
+		kfree(gpio_data);
+		return ret;
+	}
+
+	/* Finally update the LED to initial status */
+	gpio_trig_irq(0, led);
+
 	return 0;
 }
 
@@ -184,8 +127,8 @@ static void gpio_trig_deactivate(struct led_classdev *led)
 {
 	struct gpio_trig_data *gpio_data = led_get_trigger_data(led);
 
-	if (gpio_is_valid(gpio_data->gpio))
-		free_irq(gpio_to_irq(gpio_data->gpio), led);
+	free_irq(gpiod_to_irq(gpio_data->gpiod), led);
+	gpiod_put(gpio_data->gpiod);
 	kfree(gpio_data);
 }
 

-- 
2.34.1

Re: (subset) [PATCH v2 0/3] Rewrite GPIO LED trigger to use trigger-sources

[Lee Jones]

On Tue, 26 Sep 2023 23:48:10 +0200, Linus Walleij wrote:
> This rewrites the platform-data GPIO LED trigger to instead
> use fwnode trigger-sources to describe the LED used.
> 
> This will work out-of-the-box with e.g. device tree.
> 
> Tested with real hardware by modifying a device tree adding
> trigger-sources to a LED and trigger-source-cells to
> a gpio chip, setting the trigger to a pushbutton. It works
> like a charm, once the trigger is set to "gpio".
> 
> [...]

Applied, thanks!

[2/3] dt-bindings: leds: Mention GPIO triggers
      commit: 4aa583f1a6b85c88e3278d2461797d592ac61fcc
[3/3] leds: triggers: gpio: Rewrite to use trigger-sources
      commit: 1222f5dbc7723cc78741343da2e414b80de83c8f

--
Lee Jones [李琼斯]

Re: [PATCH v2 1/3] gpiolib: of: Allow "trigger-sources" to reference a GPIO

[Linus Walleij]

On Tue, Sep 26, 2023 at 11:48 PM Linus Walleij <linus.walleij@linaro.org> wrote:

> The "trigger-sources" phandle used for LED triggers are special:
> the DT bindings mandate that such triggers have the same phandle
> references no matter what the trigger is. A GPIO is just another
> kind of device that can trigger a LED.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Bartosz could you queue this patch? Lee already queued patches 2+3,
and there is just runtime dependency with no in-tree users so it's fine
to merge them in the different trees.

Sorry for missing to CC you directly on the set :/

Yours,
Linus Walleij

Re: [PATCH v2 1/3] gpiolib: of: Allow "trigger-sources" to reference a GPIO

[Bartosz Golaszewski]

On Wed, Sep 27, 2023 at 12:41 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> The "trigger-sources" phandle used for LED triggers are special:
> the DT bindings mandate that such triggers have the same phandle
> references no matter what the trigger is. A GPIO is just another
> kind of device that can trigger a LED.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/gpio/gpiolib-of.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 531faabead0f..f4a660bf11fd 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -611,6 +611,33 @@ static struct gpio_desc *of_find_mt2701_gpio(struct device_node *np,
>         return desc;
>  }
>
> +/*
> + * Trigger sources are special, they allow us to use any GPIO as a LED trigger
> + * and have the name "trigger-sources" no matter which kind of phandle it is
> + * pointing to, whether to a GPIO, a USB host, a network PHY etc. So in this case
> + * we allow looking something up that is not named "foo-gpios".
> + */
> +static struct gpio_desc *of_find_trigger_gpio(struct device_node *np,
> +                                             const char *con_id,
> +                                             unsigned int idx,
> +                                             enum of_gpio_flags *of_flags)
> +{
> +       struct gpio_desc *desc;
> +
> +       if (!IS_ENABLED(CONFIG_LEDS_TRIGGER_GPIO))
> +               return ERR_PTR(-ENOENT);
> +
> +       if (!con_id || strcmp(con_id, "trigger-sources"))
> +               return ERR_PTR(-ENOENT);
> +
> +       desc = of_get_named_gpiod_flags(np, con_id, idx, of_flags);
> +       if (!gpiod_not_found(desc))
> +               pr_debug("%s is used as a trigger\n", of_node_full_name(np));
> +
> +       return desc;
> +}
> +
> +
>  typedef struct gpio_desc *(*of_find_gpio_quirk)(struct device_node *np,
>                                                 const char *con_id,
>                                                 unsigned int idx,
> @@ -618,6 +645,7 @@ typedef struct gpio_desc *(*of_find_gpio_quirk)(struct device_node *np,
>  static const of_find_gpio_quirk of_find_gpio_quirks[] = {
>         of_find_gpio_rename,
>         of_find_mt2701_gpio,
> +       of_find_trigger_gpio,
>         NULL
>  };
>
>
> --
> 2.34.1
>

Queued, thanks!

Bart