* [PATCH 0/2] Fix LED GPIO trigger behavior
@ 2019-05-16 21:42 Kun Yi
2019-05-16 21:42 ` [PATCH 1/2] ledtrig-gpio: Request user input pin as GPIO Kun Yi
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Kun Yi @ 2019-05-16 21:42 UTC (permalink / raw)
To: linux-leds
Cc: Kun Yi, jacek.anaszewski, pavel, dmurphy, u.kleine-koenig, linux-kernel
*** BLURB HERE ***
Hello there,
I recently tested ledtrig-gpio on an embedded controller and one of the
issues I had involve not requesting the user input pin as GPIO.
In many embedded systems, a pin could be muxed as several functions, and
requesting the pin as GPIO is necessary to let pinmux select the pin as
a GPIO instead of, say an I2C pin. I'd like to learn whether it is appropriate
to assume user of ledtrig-gpio really intends to use GPIOs and not some
weird pins that are used as other functions.
Kun Yi (2):
ledtrig-gpio: Request user input pin as GPIO
ledtrig-gpio: 0 is a valid GPIO number
drivers/leds/trigger/ledtrig-gpio.c | 35 ++++++++++++++++++++---------
1 file changed, 24 insertions(+), 11 deletions(-)
--
2.21.0.1020.gf2820cf01a-goog
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] ledtrig-gpio: Request user input pin as GPIO
2019-05-16 21:42 [PATCH 0/2] Fix LED GPIO trigger behavior Kun Yi
@ 2019-05-16 21:42 ` Kun Yi
2019-05-17 6:26 ` Uwe Kleine-König
2019-05-16 21:42 ` [PATCH 2/2] ledtrig-gpio: 0 is a valid GPIO number Kun Yi
2019-05-17 16:56 ` [PATCH 0/2] Fix LED GPIO trigger behavior Jacek Anaszewski
2 siblings, 1 reply; 6+ messages in thread
From: Kun Yi @ 2019-05-16 21:42 UTC (permalink / raw)
To: linux-leds
Cc: Kun Yi, jacek.anaszewski, pavel, dmurphy, u.kleine-koenig, linux-kernel
The ledtrig-gpio logic assumes the input pin can be directly converted
to IRQ using gpio_to_irq. This is problematic since there is no
guarantee on the pinmux function nor the direction of the pin. Request
the pin as an input GPIO before requesting it as an IRQ.
Tested: a free pin can be correctly requested as GPIO
Signed-off-by: Kun Yi <kunyi@google.com>
Change-Id: I657e3e108552612506775cc348a8b4b35d40cac5
---
drivers/leds/trigger/ledtrig-gpio.c | 31 +++++++++++++++++++++++------
1 file changed, 25 insertions(+), 6 deletions(-)
diff --git a/drivers/leds/trigger/ledtrig-gpio.c b/drivers/leds/trigger/ledtrig-gpio.c
index ed0db8ed825f..f6d50e031492 100644
--- a/drivers/leds/trigger/ledtrig-gpio.c
+++ b/drivers/leds/trigger/ledtrig-gpio.c
@@ -117,6 +117,16 @@ static ssize_t gpio_trig_gpio_show(struct device *dev,
return sprintf(buf, "%u\n", gpio_data->gpio);
}
+static inline void free_used_gpio_if_valid(unsigned int gpio,
+ struct led_classdev *led)
+{
+ if (gpio == 0)
+ return;
+
+ free_irq(gpio_to_irq(gpio), led);
+ gpio_free(gpio);
+}
+
static ssize_t gpio_trig_gpio_store(struct device *dev,
struct device_attribute *attr, const char *buf, size_t n)
{
@@ -135,20 +145,30 @@ static ssize_t gpio_trig_gpio_store(struct device *dev,
return n;
if (!gpio) {
- if (gpio_data->gpio != 0)
- free_irq(gpio_to_irq(gpio_data->gpio), led);
+ free_used_gpio_if_valid(gpio_data->gpio, led);
gpio_data->gpio = 0;
return n;
}
+ ret = gpio_request(gpio, "ledtrig-gpio");
+ if (ret) {
+ dev_err(dev, "gpio_request failed with error %d\n", ret);
+ return ret;
+ }
+
+ ret = gpio_direction_input(gpio);
+ if (ret) {
+ dev_err(dev, "gpio_direction_input failed with err %d\n", ret);
+ return ret;
+ }
+
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_data->gpio != 0)
- free_irq(gpio_to_irq(gpio_data->gpio), led);
+ free_used_gpio_if_valid(gpio_data->gpio, led);
gpio_data->gpio = gpio;
/* After changing the GPIO, we need to update the LED. */
gpio_trig_irq(0, led);
@@ -184,8 +204,7 @@ static void gpio_trig_deactivate(struct led_classdev *led)
{
struct gpio_trig_data *gpio_data = led_get_trigger_data(led);
- if (gpio_data->gpio != 0)
- free_irq(gpio_to_irq(gpio_data->gpio), led);
+ free_used_gpio_if_valid(gpio_data->gpio, led);
kfree(gpio_data);
}
--
2.21.0.1020.gf2820cf01a-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] ledtrig-gpio: 0 is a valid GPIO number
2019-05-16 21:42 [PATCH 0/2] Fix LED GPIO trigger behavior Kun Yi
2019-05-16 21:42 ` [PATCH 1/2] ledtrig-gpio: Request user input pin as GPIO Kun Yi
@ 2019-05-16 21:42 ` Kun Yi
2019-05-17 6:27 ` Uwe Kleine-König
2019-05-17 16:56 ` [PATCH 0/2] Fix LED GPIO trigger behavior Jacek Anaszewski
2 siblings, 1 reply; 6+ messages in thread
From: Kun Yi @ 2019-05-16 21:42 UTC (permalink / raw)
To: linux-leds
Cc: Kun Yi, jacek.anaszewski, pavel, dmurphy, u.kleine-koenig, linux-kernel
GPIO number 0 is a valid case to handle. Use -1 as initial value
and use gpio_is_valid() to determine validity of the GPIO
number.
Signed-off-by: Kun Yi <kunyi@google.com>
Change-Id: I4a29f98b237fd0d8ba4dd2a28219d4429f2ccfff
---
drivers/leds/trigger/ledtrig-gpio.c | 16 +++++-----------
1 file changed, 5 insertions(+), 11 deletions(-)
diff --git a/drivers/leds/trigger/ledtrig-gpio.c b/drivers/leds/trigger/ledtrig-gpio.c
index f6d50e031492..48d8ef8538bd 100644
--- a/drivers/leds/trigger/ledtrig-gpio.c
+++ b/drivers/leds/trigger/ledtrig-gpio.c
@@ -22,7 +22,7 @@ struct gpio_trig_data {
unsigned desired_brightness; /* desired brightness when led is on */
unsigned inverted; /* true when gpio is inverted */
- unsigned gpio; /* gpio that triggers the leds */
+ int gpio; /* gpio that triggers the leds */
};
static irqreturn_t gpio_trig_irq(int irq, void *_led)
@@ -114,13 +114,12 @@ static ssize_t gpio_trig_gpio_show(struct device *dev,
{
struct gpio_trig_data *gpio_data = led_trigger_get_drvdata(dev);
- return sprintf(buf, "%u\n", gpio_data->gpio);
+ return sprintf(buf, "%d\n", gpio_data->gpio);
}
-static inline void free_used_gpio_if_valid(unsigned int gpio,
- struct led_classdev *led)
+static inline void free_used_gpio_if_valid(int gpio, struct led_classdev *led)
{
- if (gpio == 0)
+ if (!gpio_is_valid(gpio))
return;
free_irq(gpio_to_irq(gpio), led);
@@ -144,12 +143,6 @@ static ssize_t gpio_trig_gpio_store(struct device *dev,
if (gpio_data->gpio == gpio)
return n;
- if (!gpio) {
- free_used_gpio_if_valid(gpio_data->gpio, led);
- gpio_data->gpio = 0;
- return n;
- }
-
ret = gpio_request(gpio, "ledtrig-gpio");
if (ret) {
dev_err(dev, "gpio_request failed with error %d\n", ret);
@@ -195,6 +188,7 @@ static int gpio_trig_activate(struct led_classdev *led)
return -ENOMEM;
gpio_data->led = led;
+ gpio_data->gpio = -1;
led_set_trigger_data(led, gpio_data);
return 0;
--
2.21.0.1020.gf2820cf01a-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] ledtrig-gpio: Request user input pin as GPIO
2019-05-16 21:42 ` [PATCH 1/2] ledtrig-gpio: Request user input pin as GPIO Kun Yi
@ 2019-05-17 6:26 ` Uwe Kleine-König
0 siblings, 0 replies; 6+ messages in thread
From: Uwe Kleine-König @ 2019-05-17 6:26 UTC (permalink / raw)
To: Kun Yi
Cc: linux-leds, jacek.anaszewski, pavel, dmurphy, linux-kernel, linux-gpio
Cc: += linux-gpio@vger.kernel.org
On Thu, May 16, 2019 at 02:42:08PM -0700, Kun Yi wrote:
> The ledtrig-gpio logic assumes the input pin can be directly converted
> to IRQ using gpio_to_irq. This is problematic since there is no
> guarantee on the pinmux function nor the direction of the pin. Request
> the pin as an input GPIO before requesting it as an IRQ.
When reading this I thought the driver requested the gpio only after
converting to an irq. But in fact it didn't request and set the
direction at all.
> Tested: a free pin can be correctly requested as GPIO
This doesn't belong into the signed-off-area.
> Signed-off-by: Kun Yi <kunyi@google.com>
> Change-Id: I657e3e108552612506775cc348a8b4b35d40cac5
This doesn't belong into the linux history either.
> ---
> drivers/leds/trigger/ledtrig-gpio.c | 31 +++++++++++++++++++++++------
> 1 file changed, 25 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/leds/trigger/ledtrig-gpio.c b/drivers/leds/trigger/ledtrig-gpio.c
> index ed0db8ed825f..f6d50e031492 100644
> --- a/drivers/leds/trigger/ledtrig-gpio.c
> +++ b/drivers/leds/trigger/ledtrig-gpio.c
> @@ -117,6 +117,16 @@ static ssize_t gpio_trig_gpio_show(struct device *dev,
> return sprintf(buf, "%u\n", gpio_data->gpio);
> }
>
> +static inline void free_used_gpio_if_valid(unsigned int gpio,
> + struct led_classdev *led)
Please stick to the function prefix used in this driver. I'd call this
function gpio_trig_free_gpio and not put "if_valid" into the name.
> +{
> + if (gpio == 0)
> + return;
> +
> + free_irq(gpio_to_irq(gpio), led);
> + gpio_free(gpio);
> +}
> +
> static ssize_t gpio_trig_gpio_store(struct device *dev,
> struct device_attribute *attr, const char *buf, size_t n)
> {
> @@ -135,20 +145,30 @@ static ssize_t gpio_trig_gpio_store(struct device *dev,
> return n;
>
> if (!gpio) {
> - if (gpio_data->gpio != 0)
> - free_irq(gpio_to_irq(gpio_data->gpio), led);
> + free_used_gpio_if_valid(gpio_data->gpio, led);
> gpio_data->gpio = 0;
> return n;
> }
>
> + ret = gpio_request(gpio, "ledtrig-gpio");
> + if (ret) {
> + dev_err(dev, "gpio_request failed with error %d\n", ret);
> + return ret;
> + }
> +
> + ret = gpio_direction_input(gpio);
> + if (ret) {
> + dev_err(dev, "gpio_direction_input failed with err %d\n", ret);
> + return ret;
> + }
Please use gpio_request_one which implements both gpio_request() and
gpio_direction_*(). This also fixes the missing gpio_free() in the error
path of gpio_direction_input().
> +
> 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_data->gpio != 0)
> - free_irq(gpio_to_irq(gpio_data->gpio), led);
> + free_used_gpio_if_valid(gpio_data->gpio, led);
> gpio_data->gpio = gpio;
> /* After changing the GPIO, we need to update the LED. */
> gpio_trig_irq(0, led);
> @@ -184,8 +204,7 @@ static void gpio_trig_deactivate(struct led_classdev *led)
> {
> struct gpio_trig_data *gpio_data = led_get_trigger_data(led);
>
> - if (gpio_data->gpio != 0)
> - free_irq(gpio_to_irq(gpio_data->gpio), led);
> + free_used_gpio_if_valid(gpio_data->gpio, led);
> kfree(gpio_data);
> }
>
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] ledtrig-gpio: 0 is a valid GPIO number
2019-05-16 21:42 ` [PATCH 2/2] ledtrig-gpio: 0 is a valid GPIO number Kun Yi
@ 2019-05-17 6:27 ` Uwe Kleine-König
0 siblings, 0 replies; 6+ messages in thread
From: Uwe Kleine-König @ 2019-05-17 6:27 UTC (permalink / raw)
To: Kun Yi
Cc: linux-leds, jacek.anaszewski, pavel, dmurphy, linux-kernel, linux-gpio
On Thu, May 16, 2019 at 02:42:09PM -0700, Kun Yi wrote:
> GPIO number 0 is a valid case to handle. Use -1 as initial value
> and use gpio_is_valid() to determine validity of the GPIO
> number.
I think it's more sensible to convert to gpiod instead.
Best regards
Uwe
> Signed-off-by: Kun Yi <kunyi@google.com>
> Change-Id: I4a29f98b237fd0d8ba4dd2a28219d4429f2ccfff
> ---
> drivers/leds/trigger/ledtrig-gpio.c | 16 +++++-----------
> 1 file changed, 5 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/leds/trigger/ledtrig-gpio.c b/drivers/leds/trigger/ledtrig-gpio.c
> index f6d50e031492..48d8ef8538bd 100644
> --- a/drivers/leds/trigger/ledtrig-gpio.c
> +++ b/drivers/leds/trigger/ledtrig-gpio.c
> @@ -22,7 +22,7 @@ struct gpio_trig_data {
>
> unsigned desired_brightness; /* desired brightness when led is on */
> unsigned inverted; /* true when gpio is inverted */
> - unsigned gpio; /* gpio that triggers the leds */
> + int gpio; /* gpio that triggers the leds */
> };
>
> static irqreturn_t gpio_trig_irq(int irq, void *_led)
> @@ -114,13 +114,12 @@ static ssize_t gpio_trig_gpio_show(struct device *dev,
> {
> struct gpio_trig_data *gpio_data = led_trigger_get_drvdata(dev);
>
> - return sprintf(buf, "%u\n", gpio_data->gpio);
> + return sprintf(buf, "%d\n", gpio_data->gpio);
> }
>
> -static inline void free_used_gpio_if_valid(unsigned int gpio,
> - struct led_classdev *led)
> +static inline void free_used_gpio_if_valid(int gpio, struct led_classdev *led)
> {
> - if (gpio == 0)
> + if (!gpio_is_valid(gpio))
> return;
>
> free_irq(gpio_to_irq(gpio), led);
> @@ -144,12 +143,6 @@ static ssize_t gpio_trig_gpio_store(struct device *dev,
> if (gpio_data->gpio == gpio)
> return n;
>
> - if (!gpio) {
> - free_used_gpio_if_valid(gpio_data->gpio, led);
> - gpio_data->gpio = 0;
> - return n;
> - }
> -
> ret = gpio_request(gpio, "ledtrig-gpio");
> if (ret) {
> dev_err(dev, "gpio_request failed with error %d\n", ret);
> @@ -195,6 +188,7 @@ static int gpio_trig_activate(struct led_classdev *led)
> return -ENOMEM;
>
> gpio_data->led = led;
> + gpio_data->gpio = -1;
> led_set_trigger_data(led, gpio_data);
>
> return 0;
> --
> 2.21.0.1020.gf2820cf01a-goog
>
>
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] Fix LED GPIO trigger behavior
2019-05-16 21:42 [PATCH 0/2] Fix LED GPIO trigger behavior Kun Yi
2019-05-16 21:42 ` [PATCH 1/2] ledtrig-gpio: Request user input pin as GPIO Kun Yi
2019-05-16 21:42 ` [PATCH 2/2] ledtrig-gpio: 0 is a valid GPIO number Kun Yi
@ 2019-05-17 16:56 ` Jacek Anaszewski
2 siblings, 0 replies; 6+ messages in thread
From: Jacek Anaszewski @ 2019-05-17 16:56 UTC (permalink / raw)
To: Kun Yi, linux-leds
Cc: pavel, dmurphy, u.kleine-koenig, linux-kernel, Linus Walleij, linux-gpio
Cc Linus Walleij and leds-gpio@vger.kernel.org.
On 5/16/19 11:42 PM, Kun Yi wrote:
> *** BLURB HERE ***
> Hello there,
>
> I recently tested ledtrig-gpio on an embedded controller and one of the
> issues I had involve not requesting the user input pin as GPIO.
>
> In many embedded systems, a pin could be muxed as several functions, and
> requesting the pin as GPIO is necessary to let pinmux select the pin as
> a GPIO instead of, say an I2C pin. I'd like to learn whether it is appropriate
> to assume user of ledtrig-gpio really intends to use GPIOs and not some
> weird pins that are used as other functions.
>
> Kun Yi (2):
> ledtrig-gpio: Request user input pin as GPIO
> ledtrig-gpio: 0 is a valid GPIO number
>
> drivers/leds/trigger/ledtrig-gpio.c | 35 ++++++++++++++++++++---------
> 1 file changed, 24 insertions(+), 11 deletions(-)
>
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-05-17 16:56 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-16 21:42 [PATCH 0/2] Fix LED GPIO trigger behavior Kun Yi
2019-05-16 21:42 ` [PATCH 1/2] ledtrig-gpio: Request user input pin as GPIO Kun Yi
2019-05-17 6:26 ` Uwe Kleine-König
2019-05-16 21:42 ` [PATCH 2/2] ledtrig-gpio: 0 is a valid GPIO number Kun Yi
2019-05-17 6:27 ` Uwe Kleine-König
2019-05-17 16:56 ` [PATCH 0/2] Fix LED GPIO trigger behavior Jacek Anaszewski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).