Commit 21f2aae91e902aad ("leds: leds-gpio: Add support for GPIO
descriptors") already converted most of the driver to use GPIO descriptors.
What is still missing is the platform specific hook gpio_blink_set() and
board files which pass legacy GPIO numbers to this driver in platform data.
In this patch we handle the former and convert gpio_blink_set() to take
GPIO descriptor instead. In order to do this we convert the existing four
users to accept GPIO descriptor and translate it to legacy GPIO number in
the platform code. This effectively "pushes" legacy GPIO number usage from
the driver to platforms.
Also add comment to the remaining block describing that it is legacy code
path and we are getting rid of it eventually.
Suggested-by: Linus Walleij <[email protected]>
Signed-off-by: Mika Westerberg <[email protected]>
---
Hi all,
This has been discussed in the following thread
http://marc.info/?l=devicetree&m=141468331302908&w=4
In brief, while making example driver changes for generic device property
accessors we converted these drivers from legacy GPIO numbers to GPIO
descriptors. We did this only partially leaving the legacy code path for
existing users (there are pretty many existing users for this).
It turned out that it is pretty simple to go bit further and convert
gpio_blink_set() to use GPIO descptors as well. This patch does just that.
I have not been able to test the ARM parts on a real hardware so they are
only compile tested.
This patch is on top of Rafael's linux-pm.git/device-properties branch [1]
[1] git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git
arch/arm/mach-s3c24xx/h1940-bluetooth.c | 4 ++--
arch/arm/mach-s3c24xx/h1940.h | 4 +++-
arch/arm/mach-s3c24xx/mach-h1940.c | 3 ++-
arch/arm/mach-s3c24xx/mach-rx1950.c | 3 ++-
arch/arm/plat-orion/gpio.c | 3 ++-
arch/arm/plat-orion/include/plat/orion-gpio.h | 5 ++++-
drivers/leds/leds-gpio.c | 31 +++++++++++----------------
include/linux/leds.h | 2 +-
8 files changed, 29 insertions(+), 26 deletions(-)
diff --git a/arch/arm/mach-s3c24xx/h1940-bluetooth.c b/arch/arm/mach-s3c24xx/h1940-bluetooth.c
index b4d14b864367..9c8b1279a4ba 100644
--- a/arch/arm/mach-s3c24xx/h1940-bluetooth.c
+++ b/arch/arm/mach-s3c24xx/h1940-bluetooth.c
@@ -41,7 +41,7 @@ static void h1940bt_enable(int on)
mdelay(10);
gpio_set_value(S3C2410_GPH(1), 0);
- h1940_led_blink_set(-EINVAL, GPIO_LED_BLINK, NULL, NULL);
+ h1940_led_blink_set(NULL, GPIO_LED_BLINK, NULL, NULL);
}
else {
gpio_set_value(S3C2410_GPH(1), 1);
@@ -50,7 +50,7 @@ static void h1940bt_enable(int on)
mdelay(10);
gpio_set_value(H1940_LATCH_BLUETOOTH_POWER, 0);
- h1940_led_blink_set(-EINVAL, GPIO_LED_NO_BLINK_LOW, NULL, NULL);
+ h1940_led_blink_set(NULL, GPIO_LED_NO_BLINK_LOW, NULL, NULL);
}
}
diff --git a/arch/arm/mach-s3c24xx/h1940.h b/arch/arm/mach-s3c24xx/h1940.h
index 2950cc466840..596d9f64c5b6 100644
--- a/arch/arm/mach-s3c24xx/h1940.h
+++ b/arch/arm/mach-s3c24xx/h1940.h
@@ -19,8 +19,10 @@
#define H1940_SUSPEND_RESUMEAT (0x30081000)
#define H1940_SUSPEND_CHECK (0x30080000)
+struct gpio_desc;
+
extern void h1940_pm_return(void);
-extern int h1940_led_blink_set(unsigned gpio, int state,
+extern int h1940_led_blink_set(struct gpio_desc *desc, int state,
unsigned long *delay_on,
unsigned long *delay_off);
diff --git a/arch/arm/mach-s3c24xx/mach-h1940.c b/arch/arm/mach-s3c24xx/mach-h1940.c
index d35ddc1d9991..d40d4f5244c6 100644
--- a/arch/arm/mach-s3c24xx/mach-h1940.c
+++ b/arch/arm/mach-s3c24xx/mach-h1940.c
@@ -359,10 +359,11 @@ static struct platform_device h1940_battery = {
static DEFINE_SPINLOCK(h1940_blink_spin);
-int h1940_led_blink_set(unsigned gpio, int state,
+int h1940_led_blink_set(struct gpio_desc *desc, int state,
unsigned long *delay_on, unsigned long *delay_off)
{
int blink_gpio, check_gpio1, check_gpio2;
+ int gpio = desc ? desc_to_gpio(desc) : -EINVAL;
switch (gpio) {
case H1940_LATCH_LED_GREEN:
diff --git a/arch/arm/mach-s3c24xx/mach-rx1950.c b/arch/arm/mach-s3c24xx/mach-rx1950.c
index c3f2682d0c62..1d35ff375a01 100644
--- a/arch/arm/mach-s3c24xx/mach-rx1950.c
+++ b/arch/arm/mach-s3c24xx/mach-rx1950.c
@@ -250,9 +250,10 @@ static void rx1950_disable_charger(void)
static DEFINE_SPINLOCK(rx1950_blink_spin);
-static int rx1950_led_blink_set(unsigned gpio, int state,
+static int rx1950_led_blink_set(struct gpio_desc *desc, int state,
unsigned long *delay_on, unsigned long *delay_off)
{
+ int gpio = desc_to_gpio(desc);
int blink_gpio, check_gpio;
switch (gpio) {
diff --git a/arch/arm/plat-orion/gpio.c b/arch/arm/plat-orion/gpio.c
index b61a3bcc2fa8..b357053f40d9 100644
--- a/arch/arm/plat-orion/gpio.c
+++ b/arch/arm/plat-orion/gpio.c
@@ -306,9 +306,10 @@ EXPORT_SYMBOL(orion_gpio_set_blink);
#define ORION_BLINK_HALF_PERIOD 100 /* ms */
-int orion_gpio_led_blink_set(unsigned gpio, int state,
+int orion_gpio_led_blink_set(struct gpio_desc *desc, int state,
unsigned long *delay_on, unsigned long *delay_off)
{
+ unsigned gpio = desc_to_gpio(desc);
if (delay_on && delay_off && !*delay_on && !*delay_off)
*delay_on = *delay_off = ORION_BLINK_HALF_PERIOD;
diff --git a/arch/arm/plat-orion/include/plat/orion-gpio.h b/arch/arm/plat-orion/include/plat/orion-gpio.h
index e763988b04b9..e856b073a9c8 100644
--- a/arch/arm/plat-orion/include/plat/orion-gpio.h
+++ b/arch/arm/plat-orion/include/plat/orion-gpio.h
@@ -14,12 +14,15 @@
#include <linux/init.h>
#include <linux/types.h>
#include <linux/irqdomain.h>
+
+struct gpio_desc;
+
/*
* Orion-specific GPIO API extensions.
*/
void orion_gpio_set_unused(unsigned pin);
void orion_gpio_set_blink(unsigned pin, int blink);
-int orion_gpio_led_blink_set(unsigned gpio, int state,
+int orion_gpio_led_blink_set(struct gpio_desc *desc, int state,
unsigned long *delay_on, unsigned long *delay_off);
#define GPIO_INPUT_OK (1 << 0)
diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index edd370dbb22f..ba4698c32bb0 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -28,7 +28,7 @@ struct gpio_led_data {
u8 new_level;
u8 can_sleep;
u8 blinking;
- int (*platform_gpio_blink_set)(unsigned gpio, int state,
+ int (*platform_gpio_blink_set)(struct gpio_desc *desc, int state,
unsigned long *delay_on, unsigned long *delay_off);
};
@@ -38,13 +38,8 @@ static void gpio_led_work(struct work_struct *work)
container_of(work, struct gpio_led_data, work);
if (led_dat->blinking) {
- int gpio = desc_to_gpio(led_dat->gpiod);
- int level = led_dat->new_level;
-
- if (gpiod_is_active_low(led_dat->gpiod))
- level = !level;
-
- led_dat->platform_gpio_blink_set(gpio, level, NULL, NULL);
+ led_dat->platform_gpio_blink_set(led_dat->gpiod,
+ led_dat->new_level, NULL, NULL);
led_dat->blinking = 0;
} else
gpiod_set_value_cansleep(led_dat->gpiod, led_dat->new_level);
@@ -71,13 +66,8 @@ static void gpio_led_set(struct led_classdev *led_cdev,
schedule_work(&led_dat->work);
} else {
if (led_dat->blinking) {
- int gpio = desc_to_gpio(led_dat->gpiod);
-
- if (gpiod_is_active_low(led_dat->gpiod))
- level = !level;
-
- led_dat->platform_gpio_blink_set(gpio, level, NULL,
- NULL);
+ led_dat->platform_gpio_blink_set(led_dat->gpiod, level,
+ NULL, NULL);
led_dat->blinking = 0;
} else
gpiod_set_value(led_dat->gpiod, level);
@@ -89,20 +79,25 @@ static int gpio_blink_set(struct led_classdev *led_cdev,
{
struct gpio_led_data *led_dat =
container_of(led_cdev, struct gpio_led_data, cdev);
- int gpio = desc_to_gpio(led_dat->gpiod);
led_dat->blinking = 1;
- return led_dat->platform_gpio_blink_set(gpio, GPIO_LED_BLINK,
+ return led_dat->platform_gpio_blink_set(led_dat->gpiod, GPIO_LED_BLINK,
delay_on, delay_off);
}
static int create_gpio_led(const struct gpio_led *template,
struct gpio_led_data *led_dat, struct device *parent,
- int (*blink_set)(unsigned, int, unsigned long *, unsigned long *))
+ int (*blink_set)(struct gpio_desc *, int, unsigned long *,
+ unsigned long *))
{
int ret, state;
if (!template->gpiod) {
+ /*
+ * This is the legacy code path for platform code that
+ * still uses GPIO numbers. Ultimately we would like to get
+ * rid of this block completely.
+ */
unsigned long flags = 0;
/* skip leds that aren't available */
diff --git a/include/linux/leds.h b/include/linux/leds.h
index f3af5c4d9084..361101fef270 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -274,7 +274,7 @@ struct gpio_led_platform_data {
#define GPIO_LED_NO_BLINK_LOW 0 /* No blink GPIO state low */
#define GPIO_LED_NO_BLINK_HIGH 1 /* No blink GPIO state high */
#define GPIO_LED_BLINK 2 /* Please, blink */
- int (*gpio_blink_set)(unsigned gpio, int state,
+ int (*gpio_blink_set)(struct gpio_desc *desc, int state,
unsigned long *delay_on,
unsigned long *delay_off);
};
--
2.1.1
On Fri, Oct 31, 2014 at 01:40:58PM +0200, Mika Westerberg wrote:
> Commit 21f2aae91e902aad ("leds: leds-gpio: Add support for GPIO
> descriptors") already converted most of the driver to use GPIO descriptors.
> What is still missing is the platform specific hook gpio_blink_set() and
> board files which pass legacy GPIO numbers to this driver in platform data.
>
> In this patch we handle the former and convert gpio_blink_set() to take
> GPIO descriptor instead. In order to do this we convert the existing four
> users to accept GPIO descriptor and translate it to legacy GPIO number in
> the platform code. This effectively "pushes" legacy GPIO number usage from
> the driver to platforms.
>
> Also add comment to the remaining block describing that it is legacy code
> path and we are getting rid of it eventually.
>
> Suggested-by: Linus Walleij <[email protected]>
> Signed-off-by: Mika Westerberg <[email protected]>
Hi Mika
For the plat-orion parts:
Acked-by: Andrew Lunn <[email protected]>
Andrew
On Fri, Oct 31, 2014 at 12:40 PM, Mika Westerberg
<[email protected]> wrote:
> Commit 21f2aae91e902aad ("leds: leds-gpio: Add support for GPIO
> descriptors") already converted most of the driver to use GPIO descriptors.
> What is still missing is the platform specific hook gpio_blink_set() and
> board files which pass legacy GPIO numbers to this driver in platform data.
>
> In this patch we handle the former and convert gpio_blink_set() to take
> GPIO descriptor instead. In order to do this we convert the existing four
> users to accept GPIO descriptor and translate it to legacy GPIO number in
> the platform code. This effectively "pushes" legacy GPIO number usage from
> the driver to platforms.
>
> Also add comment to the remaining block describing that it is legacy code
> path and we are getting rid of it eventually.
>
> Suggested-by: Linus Walleij <[email protected]>
> Signed-off-by: Mika Westerberg <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On 10/31/2014 08:40 PM, Mika Westerberg wrote:
> Commit 21f2aae91e902aad ("leds: leds-gpio: Add support for GPIO
> descriptors") already converted most of the driver to use GPIO descriptors.
> What is still missing is the platform specific hook gpio_blink_set() and
> board files which pass legacy GPIO numbers to this driver in platform data.
>
> In this patch we handle the former and convert gpio_blink_set() to take
> GPIO descriptor instead. In order to do this we convert the existing four
> users to accept GPIO descriptor and translate it to legacy GPIO number in
> the platform code. This effectively "pushes" legacy GPIO number usage from
> the driver to platforms.
>
> Also add comment to the remaining block describing that it is legacy code
> path and we are getting rid of it eventually.
>
> Suggested-by: Linus Walleij <[email protected]>
> Signed-off-by: Mika Westerberg <[email protected]>
Acked-by: Alexandre Courbot <[email protected]>
On Tuesday, November 04, 2014 12:10:41 PM Alexandre Courbot wrote:
> On 10/31/2014 08:40 PM, Mika Westerberg wrote:
> > Commit 21f2aae91e902aad ("leds: leds-gpio: Add support for GPIO
> > descriptors") already converted most of the driver to use GPIO descriptors.
> > What is still missing is the platform specific hook gpio_blink_set() and
> > board files which pass legacy GPIO numbers to this driver in platform data.
> >
> > In this patch we handle the former and convert gpio_blink_set() to take
> > GPIO descriptor instead. In order to do this we convert the existing four
> > users to accept GPIO descriptor and translate it to legacy GPIO number in
> > the platform code. This effectively "pushes" legacy GPIO number usage from
> > the driver to platforms.
> >
> > Also add comment to the remaining block describing that it is legacy code
> > path and we are getting rid of it eventually.
> >
> > Suggested-by: Linus Walleij <[email protected]>
> > Signed-off-by: Mika Westerberg <[email protected]>
>
> Acked-by: Alexandre Courbot <[email protected]>
Patch applied, thanks everyone!
Hi Mika, Rafael,
On Tue, Nov 4, 2014 at 11:50 PM, Rafael J. Wysocki <[email protected]> wrote:
> On Tuesday, November 04, 2014 12:10:41 PM Alexandre Courbot wrote:
>> On 10/31/2014 08:40 PM, Mika Westerberg wrote:
>> > Commit 21f2aae91e902aad ("leds: leds-gpio: Add support for GPIO
>> > descriptors") already converted most of the driver to use GPIO descriptors.
>> > What is still missing is the platform specific hook gpio_blink_set() and
>> > board files which pass legacy GPIO numbers to this driver in platform data.
>> >
>> > In this patch we handle the former and convert gpio_blink_set() to take
>> > GPIO descriptor instead. In order to do this we convert the existing four
>> > users to accept GPIO descriptor and translate it to legacy GPIO number in
>> > the platform code. This effectively "pushes" legacy GPIO number usage from
>> > the driver to platforms.
>> >
>> > Also add comment to the remaining block describing that it is legacy code
>> > path and we are getting rid of it eventually.
>> >
>> > Suggested-by: Linus Walleij <[email protected]>
>> > Signed-off-by: Mika Westerberg <[email protected]>
>>
>> Acked-by: Alexandre Courbot <[email protected]>
>
> Patch applied, thanks everyone!
"leds: leds-gpio: Add support for GPIO descriptors" broke leds-gpio on
non-DT platforms for me:
gpiod_direction_output: invalid GPIO
leds-gpio: probe of leds-gpio failed with error -22
(desc is NULL in gpiod_direction_output()).
DT shmobile reference/multi-platform are fine.
I noticed the hard way, as I wanted to add some LEDs to a new platform,
but couldn't get it work. It turned out it also had stopped working on
r8a7740/armadillo-legacy, so I started bisecting...
Unfortunately the offending patch can't just be reverted.
Reverting all three of these on pm/linux-next fixed the issue, though:
c673a2b400810352 leds: leds-gpio: Convert gpio_blink_set() to use GPIO descripto
a43f2cbbb009f962 leds: leds-gpio: Make use of device property API
5c51277a9ababfa4 leds: leds-gpio: Add support for GPIO descriptors
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Thu, Nov 06, 2014 at 10:52:35AM +0100, Geert Uytterhoeven wrote:
> Hi Mika, Rafael,
>
> On Tue, Nov 4, 2014 at 11:50 PM, Rafael J. Wysocki <[email protected]> wrote:
> > On Tuesday, November 04, 2014 12:10:41 PM Alexandre Courbot wrote:
> >> On 10/31/2014 08:40 PM, Mika Westerberg wrote:
> >> > Commit 21f2aae91e902aad ("leds: leds-gpio: Add support for GPIO
> >> > descriptors") already converted most of the driver to use GPIO descriptors.
> >> > What is still missing is the platform specific hook gpio_blink_set() and
> >> > board files which pass legacy GPIO numbers to this driver in platform data.
> >> >
> >> > In this patch we handle the former and convert gpio_blink_set() to take
> >> > GPIO descriptor instead. In order to do this we convert the existing four
> >> > users to accept GPIO descriptor and translate it to legacy GPIO number in
> >> > the platform code. This effectively "pushes" legacy GPIO number usage from
> >> > the driver to platforms.
> >> >
> >> > Also add comment to the remaining block describing that it is legacy code
> >> > path and we are getting rid of it eventually.
> >> >
> >> > Suggested-by: Linus Walleij <[email protected]>
> >> > Signed-off-by: Mika Westerberg <[email protected]>
> >>
> >> Acked-by: Alexandre Courbot <[email protected]>
> >
> > Patch applied, thanks everyone!
>
> "leds: leds-gpio: Add support for GPIO descriptors" broke leds-gpio on
> non-DT platforms for me:
>
> gpiod_direction_output: invalid GPIO
> leds-gpio: probe of leds-gpio failed with error -22
>
> (desc is NULL in gpiod_direction_output()).
>
> DT shmobile reference/multi-platform are fine.
>
> I noticed the hard way, as I wanted to add some LEDs to a new platform,
> but couldn't get it work. It turned out it also had stopped working on
> r8a7740/armadillo-legacy, so I started bisecting...
Which board file that is?
There is a bug that gpio_to_desc() returns NULL instead if ERR_PTR() in
that patch but I wonder why gpio_is_valid() and devm_gpio_request_one()
do not complain about that prior.
Hi Mika,
On Thu, Nov 6, 2014 at 11:30 AM, Mika Westerberg
<[email protected]> wrote:
>> "leds: leds-gpio: Add support for GPIO descriptors" broke leds-gpio on
>> non-DT platforms for me:
>>
>> gpiod_direction_output: invalid GPIO
>> leds-gpio: probe of leds-gpio failed with error -22
>>
>> (desc is NULL in gpiod_direction_output()).
>>
>> DT shmobile reference/multi-platform are fine.
>>
>> I noticed the hard way, as I wanted to add some LEDs to a new platform,
>> but couldn't get it work. It turned out it also had stopped working on
>> r8a7740/armadillo-legacy, so I started bisecting...
>
> Which board file that is?
>
> There is a bug that gpio_to_desc() returns NULL instead if ERR_PTR() in
> that patch but I wonder why gpio_is_valid() and devm_gpio_request_one()
> do not complain about that prior.
arch/arm/mach-shmobile/board-armadillo800eva.c
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Thu, Nov 06, 2014 at 11:32:19AM +0100, Geert Uytterhoeven wrote:
> Hi Mika,
>
> On Thu, Nov 6, 2014 at 11:30 AM, Mika Westerberg
> <[email protected]> wrote:
> >> "leds: leds-gpio: Add support for GPIO descriptors" broke leds-gpio on
> >> non-DT platforms for me:
> >>
> >> gpiod_direction_output: invalid GPIO
> >> leds-gpio: probe of leds-gpio failed with error -22
> >>
> >> (desc is NULL in gpiod_direction_output()).
> >>
> >> DT shmobile reference/multi-platform are fine.
> >>
> >> I noticed the hard way, as I wanted to add some LEDs to a new platform,
> >> but couldn't get it work. It turned out it also had stopped working on
> >> r8a7740/armadillo-legacy, so I started bisecting...
> >
> > Which board file that is?
> >
> > There is a bug that gpio_to_desc() returns NULL instead if ERR_PTR() in
> > that patch but I wonder why gpio_is_valid() and devm_gpio_request_one()
> > do not complain about that prior.
>
> arch/arm/mach-shmobile/board-armadillo800eva.c
Thanks.
Are you able to put some printks() to the 'if (!template->gpiod)' branch
so that it prints out gpio number and what does devm_gpio_request_one()
return?
Something like:
if (!template->gpiod) {
...
ret = devm_gpio_request_one(parent, template->gpio, flags,
template->name);
dev_info(parent, "GPIO %u, ret: %d\n", template->gpio, ret);
if (ret < 0)
...
led_dat->gpiod = gpio_to_desc(template->gpio);
dev_info(parent, "GPIOD: %p\n", led_dat->gpiod);
}
Hi Mika,
On Thu, Nov 6, 2014 at 11:58 AM, Mika Westerberg
<[email protected]> wrote:
> On Thu, Nov 06, 2014 at 11:32:19AM +0100, Geert Uytterhoeven wrote:
>> On Thu, Nov 6, 2014 at 11:30 AM, Mika Westerberg
>> <[email protected]> wrote:
>> >> "leds: leds-gpio: Add support for GPIO descriptors" broke leds-gpio on
>> >> non-DT platforms for me:
>> >>
>> >> gpiod_direction_output: invalid GPIO
>> >> leds-gpio: probe of leds-gpio failed with error -22
>> >>
>> >> (desc is NULL in gpiod_direction_output()).
>> >>
>> >> DT shmobile reference/multi-platform are fine.
>> >>
>> >> I noticed the hard way, as I wanted to add some LEDs to a new platform,
>> >> but couldn't get it work. It turned out it also had stopped working on
>> >> r8a7740/armadillo-legacy, so I started bisecting...
>> >
>> > Which board file that is?
>> >
>> > There is a bug that gpio_to_desc() returns NULL instead if ERR_PTR() in
>> > that patch but I wonder why gpio_is_valid() and devm_gpio_request_one()
>> > do not complain about that prior.
>>
>> arch/arm/mach-shmobile/board-armadillo800eva.c
>
> Are you able to put some printks() to the 'if (!template->gpiod)' branch
> so that it prints out gpio number and what does devm_gpio_request_one()
> return?
>
> Something like:
>
> if (!template->gpiod) {
> ...
> ret = devm_gpio_request_one(parent, template->gpio, flags,
> template->name);
> dev_info(parent, "GPIO %u, ret: %d\n", template->gpio, ret);
> if (ret < 0)
> ...
>
> led_dat->gpiod = gpio_to_desc(template->gpio);
> dev_info(parent, "GPIOD: %p\n", led_dat->gpiod);
Sure:
leds-gpio leds-gpio: GPIO 102, ret: 0
leds-gpio leds-gpio: GPIOD: c050e970
So led_dat is non-NULL. But it's overwritten by NULL later:
led_dat->gpiod = template->gpiod;
Whitespace damaged fix below, to fold into the original.
If you prefer a proper separate patch, let me know.
diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index ba4698c32bb04bde..b3c5d9d6a42bcd8b 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -92,7 +92,8 @@ static int create_gpio_led(const struct gpio_led *template,
{
int ret, state;
- if (!template->gpiod) {
+ led_dat->gpiod = template->gpiod;
+ if (!led_dat->gpiod) {
/*
* This is the legacy code path for platform code that
* still uses GPIO numbers. Ultimately we would like to get
@@ -122,8 +123,7 @@ static int create_gpio_led(const struct gpio_led *template,
led_dat->cdev.name = template->name;
led_dat->cdev.default_trigger = template->default_trigger;
- led_dat->gpiod = template->gpiod;
- led_dat->can_sleep = gpiod_cansleep(template->gpiod);
+ led_dat->can_sleep = gpiod_cansleep(led_dat->gpiod);
led_dat->blinking = 0;
if (blink_set) {
led_dat->platform_gpio_blink_set = blink_set;
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Thu, Nov 06, 2014 at 12:12:04PM +0100, Geert Uytterhoeven wrote:
> Hi Mika,
>
> On Thu, Nov 6, 2014 at 11:58 AM, Mika Westerberg
> <[email protected]> wrote:
> > On Thu, Nov 06, 2014 at 11:32:19AM +0100, Geert Uytterhoeven wrote:
> >> On Thu, Nov 6, 2014 at 11:30 AM, Mika Westerberg
> >> <[email protected]> wrote:
> >> >> "leds: leds-gpio: Add support for GPIO descriptors" broke leds-gpio on
> >> >> non-DT platforms for me:
> >> >>
> >> >> gpiod_direction_output: invalid GPIO
> >> >> leds-gpio: probe of leds-gpio failed with error -22
> >> >>
> >> >> (desc is NULL in gpiod_direction_output()).
> >> >>
> >> >> DT shmobile reference/multi-platform are fine.
> >> >>
> >> >> I noticed the hard way, as I wanted to add some LEDs to a new platform,
> >> >> but couldn't get it work. It turned out it also had stopped working on
> >> >> r8a7740/armadillo-legacy, so I started bisecting...
> >> >
> >> > Which board file that is?
> >> >
> >> > There is a bug that gpio_to_desc() returns NULL instead if ERR_PTR() in
> >> > that patch but I wonder why gpio_is_valid() and devm_gpio_request_one()
> >> > do not complain about that prior.
> >>
> >> arch/arm/mach-shmobile/board-armadillo800eva.c
> >
> > Are you able to put some printks() to the 'if (!template->gpiod)' branch
> > so that it prints out gpio number and what does devm_gpio_request_one()
> > return?
> >
> > Something like:
> >
> > if (!template->gpiod) {
> > ...
> > ret = devm_gpio_request_one(parent, template->gpio, flags,
> > template->name);
> > dev_info(parent, "GPIO %u, ret: %d\n", template->gpio, ret);
> > if (ret < 0)
> > ...
> >
> > led_dat->gpiod = gpio_to_desc(template->gpio);
> > dev_info(parent, "GPIOD: %p\n", led_dat->gpiod);
>
> Sure:
>
> leds-gpio leds-gpio: GPIO 102, ret: 0
> leds-gpio leds-gpio: GPIOD: c050e970
>
> So led_dat is non-NULL. But it's overwritten by NULL later:
>
> led_dat->gpiod = template->gpiod;
Ah, that's it. Nice catch!
> Whitespace damaged fix below, to fold into the original.
> If you prefer a proper separate patch, let me know.
It is up to Rafael. I think he wants to stabilize this branch so in that
case separate patch on top would work better.
Feel free to add,
Reviewed-by: Mika Westerberg <[email protected]>
to the patch.
>
> diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
> index ba4698c32bb04bde..b3c5d9d6a42bcd8b 100644
> --- a/drivers/leds/leds-gpio.c
> +++ b/drivers/leds/leds-gpio.c
> @@ -92,7 +92,8 @@ static int create_gpio_led(const struct gpio_led *template,
> {
> int ret, state;
>
> - if (!template->gpiod) {
> + led_dat->gpiod = template->gpiod;
> + if (!led_dat->gpiod) {
> /*
> * This is the legacy code path for platform code that
> * still uses GPIO numbers. Ultimately we would like to get
> @@ -122,8 +123,7 @@ static int create_gpio_led(const struct gpio_led *template,
>
> led_dat->cdev.name = template->name;
> led_dat->cdev.default_trigger = template->default_trigger;
> - led_dat->gpiod = template->gpiod;
> - led_dat->can_sleep = gpiod_cansleep(template->gpiod);
> + led_dat->can_sleep = gpiod_cansleep(led_dat->gpiod);
> led_dat->blinking = 0;
> if (blink_set) {
> led_dat->platform_gpio_blink_set = blink_set;
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
On Thu, Nov 6, 2014 at 12:22 PM, Mika Westerberg
<[email protected]> wrote:
> On Thu, Nov 06, 2014 at 12:12:04PM +0100, Geert Uytterhoeven wrote:
>> On Thu, Nov 6, 2014 at 11:58 AM, Mika Westerberg
>> <[email protected]> wrote:
>> > On Thu, Nov 06, 2014 at 11:32:19AM +0100, Geert Uytterhoeven wrote:
>> >> On Thu, Nov 6, 2014 at 11:30 AM, Mika Westerberg
>> >> <[email protected]> wrote:
>> >> >> "leds: leds-gpio: Add support for GPIO descriptors" broke leds-gpio on
>> >> >> non-DT platforms for me:
>> >> >>
>> >> >> gpiod_direction_output: invalid GPIO
>> >> >> leds-gpio: probe of leds-gpio failed with error -22
>> >> >>
>> >> >> (desc is NULL in gpiod_direction_output()).
>> leds-gpio leds-gpio: GPIO 102, ret: 0
>> leds-gpio leds-gpio: GPIOD: c050e970
>>
>> So led_dat is non-NULL. But it's overwritten by NULL later:
>>
>> led_dat->gpiod = template->gpiod;
>
> Ah, that's it. Nice catch!
>
>> Whitespace damaged fix below, to fold into the original.
>> If you prefer a proper separate patch, let me know.
>
> It is up to Rafael. I think he wants to stabilize this branch so in that
> case separate patch on top would work better.
I've already sent a patch, as I have to publish the fix in my git tree anyway.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds