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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACRpkdaKiz4wODUnH+2+ELY3XGSYYsQ_iFMm+iY=o6rmVcxmSw@mail.gmail.com>
Date:	Thu, 30 Oct 2014 16:40:05 +0100
From:	Linus Walleij <linus.walleij@...aro.org>
To:	Mika Westerberg <mika.westerberg@...ux.intel.com>
Cc:	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Grant Likely <grant.likely@...aro.org>,
	Arnd Bergmann <arnd@...db.de>,
	ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
	Aaron Lu <aaron.lu@...el.com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	Alexandre Courbot <gnurou@...il.com>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Bryan Wu <cooloney@...il.com>,
	Darren Hart <dvhart@...ux.intel.com>,
	Mark Rutland <mark.rutland@....com>
Subject: Re: [PATCH v5 07/12] leds: leds-gpio: Add support for GPIO descriptors

On Wed, Oct 29, 2014 at 9:53 AM, Mika Westerberg
<mika.westerberg@...ux.intel.com> 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@...ysocki.net> wrote:
>> > > From: Mika Westerberg <mika.westerberg@...ux.intel.com>
>> > >
>> > > 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@...ux.intel.com>
>> > > Acked-by: Alexandre Courbot <acourbot@...dia.com>
>> > > Acked-by: Bryan Wu <cooloney@...il.com>
>> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
>> > (...)
>> >
>> > >         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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/