Re: [PATCH v5 07/12] leds: leds-gpio: Add support for GPIO descriptors

[Linus Walleij]

On Wed, Oct 29, 2014 at 9:53 AM, Mika Westerberg
<mika.westerberg@xxxxxxxxxxxxxxx> wrote:
> On Tue, Oct 28, 2014 at 10:56:09PM +0100, Rafael J. Wysocki wrote:
>> On Tuesday, October 28, 2014 04:26:25 PM Linus Walleij wrote:
>> > On Fri, Oct 17, 2014 at 2:11 PM, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote:
>> > > From: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
>> > >
>> > > GPIO descriptors are the preferred way over legacy GPIO numbers
>> > > nowadays. Convert the driver to use GPIO descriptors internally but
>> > > still allow passing legacy GPIO numbers from platform data to support
>> > > existing platforms.
>> > >
>> > > Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
>> > > Acked-by: Alexandre Courbot <acourbot@xxxxxxxxxx>
>> > > Acked-by: Bryan Wu <cooloney@xxxxxxxxx>
>> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>> > (...)
>> >
>> > >         if (led_dat->blinking) {
>> > > -               led_dat->platform_gpio_blink_set(led_dat->gpio,
>> > > -                                                led_dat->new_level,
>> > > -                                                NULL, NULL);
>> > > +               int gpio = desc_to_gpio(led_dat->gpiod);
>> > > +               int level = led_dat->new_level;
>> >
>> > So this desc_to_gpio() is done only to call the legacy callback below?
>> >
>> > > +               if (gpiod_is_active_low(led_dat->gpiod))
>> > > +                       level = !level;
>> >
>> > And that leads to making it necessary to have this helper variable
>> > to invert the level since that callback does not pass a descriptor
>> > (which would inherently know if it's active low)....
>> >
>> > > +
>> > > +               led_dat->platform_gpio_blink_set(gpio, level, NULL, NULL);
>> >
>> > Is it *really* impossible to change all the users of this callback?
>>
>> You said it could be done in a followup patch.  Here:
>> http://marc.info/?l=linux-acpi&m=141154536921643&w=4
>>
>> And Mika said he would add that to his TODO list:
>> http://marc.info/?l=linux-acpi&m=141155173924101&w=4
>>
>> I suppose that is still valid.
>
> Yes, I'll just let dust to settle before sending out a patch that
> converts the existing users of platform_gpio_blink_set() callback to
> gpio descriptors.

Dust settle = 2 weeks, 18 months, 5 years?

Sorry for nagging but I've been *SO* burnt by people in the past
saying they will "fix things later real soon" and then never get around
to it.

But I guess I'm gonna trust you anyway.

>> > > Index: linux-pm/include/linux/leds.h
>> > > ===================================================================
>> > > --- linux-pm.orig/include/linux/leds.h
>> > > +++ linux-pm/include/linux/leds.h
>> > > @@ -251,6 +251,7 @@ struct gpio_led {
>> > >         unsigned        retain_state_suspended : 1;
>> > >         unsigned        default_state : 2;
>> > >         /* default_state should be one of LEDS_GPIO_DEFSTATE_(ON|OFF|KEEP) */
>> > > +       struct gpio_desc *gpiod;
>> >
>> > Put the new struct member right below the current "gpio"
>> > member,
>>
>> It was done like that in previous versions, but turned out to cause problems
>> to happen in testing.  Unfortunately, I don't seem to be able to find a pointer
>> to the original report ATM, but perhaps Mika can.  Mika?
>
> It is burried inside this thread:
>
> http://www.spinics.net/lists/arm-kernel/msg369522.html

OK people not using C99 initalizers, sigh.

I hope Arnd's complimentary patch gets applied too so we
can move the member back where it should be, kernel structs
as ABI isn't really any fun.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/