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

[PATCH 0/2] 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.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Linus Walleij (2):
      dt-bindings: leds: Mention GPIO triggers
      leds: triggers: gpio: Rewrite to use trigger-sources

 Documentation/devicetree/bindings/leds/common.yaml |   2 +
 drivers/leds/trigger/Kconfig                       |   5 +-
 drivers/leds/trigger/ledtrig-gpio.c                | 136 ++++++---------------
 3 files changed, 42 insertions(+), 101 deletions(-)
---
base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d
change-id: 20230911-gpio-led-trigger-dt-922bbe21fa22

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

[PATCH 1/2] 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>;
    };
};

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

Re: [PATCH 1/2] dt-bindings: leds: Mention GPIO triggers

[Conor Dooley]

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

On Tue, Sep 12, 2023 at 03:44:30PM +0200, Linus Walleij wrote:
> 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>;
>     };
> };
> 
> 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
> 

Acked-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

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

Re: [PATCH 1/2] dt-bindings: leds: Mention GPIO triggers

[Rob Herring]

On Tue, Sep 12, 2023 at 03:44:30PM +0200, Linus Walleij wrote:
> 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>;

BTW, this is not documented for any GPIO binding. If we want to specify 
the cell size, then it has to be added to every GPIO controller binding. 
If not, we then need to reference gpio.yaml in every GPIO controller 
binding (along with unevaluatedProperties). Doesn't have to be done for 
this patch to go in though.

> };
> 
> 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>;
>     };
> };
> 
> 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
> 

Re: [PATCH 1/2] dt-bindings: leds: Mention GPIO triggers

[Linus Walleij]

On Wed, Sep 13, 2023 at 3:34 PM Rob Herring <robh@kernel.org> wrote:
> On Tue, Sep 12, 2023 at 03:44:30PM +0200, Linus Walleij wrote:
> > 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>;
>
> BTW, this is not documented for any GPIO binding. If we want to specify
> the cell size, then it has to be added to every GPIO controller binding.
> If not, we then need to reference gpio.yaml in every GPIO controller
> binding (along with unevaluatedProperties). Doesn't have to be done for
> this patch to go in though.

Yeah I mean this trigger-sources = <...>; one-size-fits-all is a bit
weird in a way.

My other idea was to simply add trigger-gpios to the normal way
and be done with it, but now the trigger binding has this weird
thing.

Would trigger-gpios be better?

Yours,
Linus Walleij

Re: [PATCH 1/2] dt-bindings: leds: Mention GPIO triggers

[Rob Herring]

On Thu, Sep 14, 2023 at 3:40 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Wed, Sep 13, 2023 at 3:34 PM Rob Herring <robh@kernel.org> wrote:
> > On Tue, Sep 12, 2023 at 03:44:30PM +0200, Linus Walleij wrote:
> > > 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>;
> >
> > BTW, this is not documented for any GPIO binding. If we want to specify
> > the cell size, then it has to be added to every GPIO controller binding.
> > If not, we then need to reference gpio.yaml in every GPIO controller
> > binding (along with unevaluatedProperties). Doesn't have to be done for
> > this patch to go in though.
>
> Yeah I mean this trigger-sources = <...>; one-size-fits-all is a bit
> weird in a way.
>
> My other idea was to simply add trigger-gpios to the normal way
> and be done with it, but now the trigger binding has this weird
> thing.
>
> Would trigger-gpios be better?

Then GPIOs are different than everyone else. I think we have to think
about other bindings too. While we could standardize the naming here
with trigger-gpios, that won't work with the foos/foo-names style of
bindings.

trigger-sources is not widely used as it is just USB ATM and a few
platforms. We could come up with something different.
"trigger-sources-<cellname>" is the only idea I have. Then the
property name gives you the cell name to read. But variable property
names have their own challenges. We would need to look at all the
current trigger sources (i.e. the linux,default-trigger ones) and see
what else might need this.

Rob

Re: [PATCH 1/2] dt-bindings: leds: Mention GPIO triggers

[Linus Walleij]

On Thu, Sep 14, 2023 at 4:27 PM Rob Herring <robh@kernel.org> wrote:
> On Thu, Sep 14, 2023 at 3:40 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Wed, Sep 13, 2023 at 3:34 PM Rob Herring <robh@kernel.org> wrote:
> > > On Tue, Sep 12, 2023 at 03:44:30PM +0200, Linus Walleij wrote:
> > > > 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>;
> > >
> > > BTW, this is not documented for any GPIO binding. If we want to specify
> > > the cell size, then it has to be added to every GPIO controller binding.
> > > If not, we then need to reference gpio.yaml in every GPIO controller
> > > binding (along with unevaluatedProperties). Doesn't have to be done for
> > > this patch to go in though.
> >
> > Yeah I mean this trigger-sources = <...>; one-size-fits-all is a bit
> > weird in a way.
> >
> > My other idea was to simply add trigger-gpios to the normal way
> > and be done with it, but now the trigger binding has this weird
> > thing.
> >
> > Would trigger-gpios be better?
>
> Then GPIOs are different than everyone else. I think we have to think
> about other bindings too. While we could standardize the naming here
> with trigger-gpios, that won't work with the foos/foo-names style of
> bindings.
>
> trigger-sources is not widely used as it is just USB ATM and a few
> platforms. We could come up with something different.
> "trigger-sources-<cellname>" is the only idea I have. Then the
> property name gives you the cell name to read. But variable property
> names have their own challenges. We would need to look at all the
> current trigger sources (i.e. the linux,default-trigger ones) and see
> what else might need this.

I think it in a way is elegant with the trigger-sources phandle as it
is so I would stick with this.

I can just add '#trigger-source-cells' to the existing GPIO
bindings and it's a bit tedious since we don't have a common file
for the GPIO chip stuff, but it's just lots of lines.

I guess it would be better to break out gpio-common.yaml and
gpio-common-irq.yaml for GPIO controllers with or without
interrupt support and then add '#trigger-source-cells' to just
those supporting IRQs because I think only that makes sense,
polling for a line to change isn't quite a "trigger".

Yours,
Linus Walleij

Re: [PATCH 1/2] dt-bindings: leds: Mention GPIO triggers

[Rob Herring]

On Fri, Sep 15, 2023 at 7:01 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Thu, Sep 14, 2023 at 4:27 PM Rob Herring <robh@kernel.org> wrote:
> > On Thu, Sep 14, 2023 at 3:40 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> > > On Wed, Sep 13, 2023 at 3:34 PM Rob Herring <robh@kernel.org> wrote:
> > > > On Tue, Sep 12, 2023 at 03:44:30PM +0200, Linus Walleij wrote:
> > > > > 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>;
> > > >
> > > > BTW, this is not documented for any GPIO binding. If we want to specify
> > > > the cell size, then it has to be added to every GPIO controller binding.
> > > > If not, we then need to reference gpio.yaml in every GPIO controller
> > > > binding (along with unevaluatedProperties). Doesn't have to be done for
> > > > this patch to go in though.
> > >
> > > Yeah I mean this trigger-sources = <...>; one-size-fits-all is a bit
> > > weird in a way.
> > >
> > > My other idea was to simply add trigger-gpios to the normal way
> > > and be done with it, but now the trigger binding has this weird
> > > thing.
> > >
> > > Would trigger-gpios be better?
> >
> > Then GPIOs are different than everyone else. I think we have to think
> > about other bindings too. While we could standardize the naming here
> > with trigger-gpios, that won't work with the foos/foo-names style of
> > bindings.
> >
> > trigger-sources is not widely used as it is just USB ATM and a few
> > platforms. We could come up with something different.
> > "trigger-sources-<cellname>" is the only idea I have. Then the
> > property name gives you the cell name to read. But variable property
> > names have their own challenges. We would need to look at all the
> > current trigger sources (i.e. the linux,default-trigger ones) and see
> > what else might need this.
>
> I think it in a way is elegant with the trigger-sources phandle as it
> is so I would stick with this.
>
> I can just add '#trigger-source-cells' to the existing GPIO
> bindings and it's a bit tedious since we don't have a common file
> for the GPIO chip stuff, but it's just lots of lines.

We do, gpio.yaml in dtschema. We just didn't reference it in all
controllers because at the time unevaluatedProperties didn't
exist/work and you still have to list most properties to define their
constraints. In general, we reference the common schema for ones with
child nodes, but not ones which are just #foo-cells and other
properties. For GPIO, it's evolved to the point that referencing the
common schema makes sense mainly because we have the hog nodes now.

I said before we still need '#trigger-source-cells' in each schema,
but really I suppose if we just set it to 1 or 2 cells in the common
schema, that would be good enough. It's so rarely used. I expect
(because there's discussions on it) someday jsonschema will support
data dependent schemas (i.e. if the value of prop is X, then apply
schema). Actually, we can do that already with if/then schemas, but it
would be kind of verbose.

> I guess it would be better to break out gpio-common.yaml and
> gpio-common-irq.yaml for GPIO controllers with or without
> interrupt support and then add '#trigger-source-cells' to just
> those supporting IRQs because I think only that makes sense,
> polling for a line to change isn't quite a "trigger".

No need to split them just for that:

dependencies:
  '#trigger-source-cells': interrupt-controller

Though maybe a split makes sense anyways.

Rob

[PATCH 2/2] 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>
---
 drivers/leds/trigger/Kconfig        |   5 +-
 drivers/leds/trigger/ledtrig-gpio.c | 136 +++++++++++-------------------------
 2 files changed, 40 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..a9caab7998a9 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,47 @@ 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)) {
+		kfree(gpio_data);
+		return PTR_ERR(gpio_data->gpiod);
+	}
+	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 +126,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: [PATCH 2/2] leds: triggers: gpio: Rewrite to use trigger-sources

[Dan Carpenter]

Hi Linus,

kernel test robot noticed the following build warnings:

url:    https://github.com/intel-lab-lkp/linux/commits/Linus-Walleij/dt-bindings-leds-Mention-GPIO-triggers/20230912-214554
base:   0bb80ecc33a8fb5a682236443c1e740d5c917d1d
patch link:    https://lore.kernel.org/r/20230912-gpio-led-trigger-dt-v1-2-1b50e3756dda%40linaro.org
patch subject: [PATCH 2/2] leds: triggers: gpio: Rewrite to use trigger-sources
config: x86_64-randconfig-161-20230913 (https://download.01.org/0day-ci/archive/20230914/202309140825.cVUTHU1K-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230914/202309140825.cVUTHU1K-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202309140825.cVUTHU1K-lkp@intel.com/

smatch warnings:
drivers/leds/trigger/ledtrig-gpio.c:98 gpio_trig_activate() error: dereferencing freed memory 'gpio_data'

vim +/gpio_data +98 drivers/leds/trigger/ledtrig-gpio.c

2282e125a406e0 drivers/leds/trigger/ledtrig-gpio.c Uwe Kleine-König 2018-07-02   78  static int gpio_trig_activate(struct led_classdev *led)
17354bfe85275f drivers/leds/ledtrig-gpio.c         Felipe Balbi     2009-02-17   79  {
17354bfe85275f drivers/leds/ledtrig-gpio.c         Felipe Balbi     2009-02-17   80  	struct gpio_trig_data *gpio_data;
2689cea6283a47 drivers/leds/trigger/ledtrig-gpio.c Linus Walleij    2023-09-12   81  	struct device *dev = led->dev;
2689cea6283a47 drivers/leds/trigger/ledtrig-gpio.c Linus Walleij    2023-09-12   82  	int ret;
17354bfe85275f drivers/leds/ledtrig-gpio.c         Felipe Balbi     2009-02-17   83  
17354bfe85275f drivers/leds/ledtrig-gpio.c         Felipe Balbi     2009-02-17   84  	gpio_data = kzalloc(sizeof(*gpio_data), GFP_KERNEL);
17354bfe85275f drivers/leds/ledtrig-gpio.c         Felipe Balbi     2009-02-17   85  	if (!gpio_data)
9bfd7d9e5d6353 drivers/leds/trigger/ledtrig-gpio.c Uwe Kleine-König 2018-07-02   86  		return -ENOMEM;
17354bfe85275f drivers/leds/ledtrig-gpio.c         Felipe Balbi     2009-02-17   87  
2689cea6283a47 drivers/leds/trigger/ledtrig-gpio.c Linus Walleij    2023-09-12   88  	/*
2689cea6283a47 drivers/leds/trigger/ledtrig-gpio.c Linus Walleij    2023-09-12   89  	 * The generic property "trigger-sources" is followed,
2689cea6283a47 drivers/leds/trigger/ledtrig-gpio.c Linus Walleij    2023-09-12   90  	 * and we hope that this is a GPIO.
2689cea6283a47 drivers/leds/trigger/ledtrig-gpio.c Linus Walleij    2023-09-12   91  	 */
2689cea6283a47 drivers/leds/trigger/ledtrig-gpio.c Linus Walleij    2023-09-12   92  	gpio_data->gpiod = fwnode_gpiod_get_index(dev->fwnode,
2689cea6283a47 drivers/leds/trigger/ledtrig-gpio.c Linus Walleij    2023-09-12   93  						  "trigger-sources",
2689cea6283a47 drivers/leds/trigger/ledtrig-gpio.c Linus Walleij    2023-09-12   94  						  0, GPIOD_IN,
2689cea6283a47 drivers/leds/trigger/ledtrig-gpio.c Linus Walleij    2023-09-12   95  						  "led-trigger");
2689cea6283a47 drivers/leds/trigger/ledtrig-gpio.c Linus Walleij    2023-09-12   96  	if (IS_ERR(gpio_data->gpiod)) {
2689cea6283a47 drivers/leds/trigger/ledtrig-gpio.c Linus Walleij    2023-09-12   97  		kfree(gpio_data);
                                                                                                ^^^^^^^^^^^^^^^^

2689cea6283a47 drivers/leds/trigger/ledtrig-gpio.c Linus Walleij    2023-09-12  @98  		return PTR_ERR(gpio_data->gpiod);
                                                                                                               ^^^^^^^^^^^^^^^^
Dereferencing freed memory.

2689cea6283a47 drivers/leds/trigger/ledtrig-gpio.c Linus Walleij    2023-09-12   99  	}
2689cea6283a47 drivers/leds/trigger/ledtrig-gpio.c Linus Walleij    2023-09-12  100  	if (!gpio_data->gpiod) {
2689cea6283a47 drivers/leds/trigger/ledtrig-gpio.c Linus Walleij    2023-09-12  101  		dev_err(dev, "no valid GPIO for the trigger\n");

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/2] leds: triggers: gpio: Rewrite to use trigger-sources

[Rob Herring]

On Tue, Sep 12, 2023 at 03:44:31PM +0200, Linus Walleij wrote:
> 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>
> ---

[...]

> @@ -167,16 +78,47 @@ 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");

Isn't this going to look for "trigger-sources-gpio" and 
""trigger-sources-gpios"?

Rob

Re: [PATCH 2/2] leds: triggers: gpio: Rewrite to use trigger-sources

[Linus Walleij]

On Fri, Sep 15, 2023 at 4:15 PM Rob Herring <robh@kernel.org> wrote:

> > +     /*
> > +      * 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");
>
> Isn't this going to look for "trigger-sources-gpio" and
> ""trigger-sources-gpios"?

Indeed. I'll simply code an exception for this into
gpiolib-of.c, it's an OF pecularity after all.

Yours,
Linus Walleij