*** 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
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 <[email protected]>
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
Cc: += [email protected]
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 <[email protected]>
> 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/ |
Cc Linus Walleij and [email protected].
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