[2/2] leds: triggers: gpio: Rewrite to use trigger-sources
Commit Message
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(-)
Comments
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");
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
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
@@ -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.
@@ -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);
}