| 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
| ||
|
Date: Tue, 28 Oct 2014 16:26:25 +0100
From: Linus Walleij <linus.walleij@...aro.org>
To: "Rafael J. Wysocki" <rjw@...ysocki.net>
Cc: 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>,
Mika Westerberg <mika.westerberg@...ux.intel.com>,
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 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?
> led_dat->blinking = 0;
> } else
> - gpio_set_value_cansleep(led_dat->gpio, led_dat->new_level);
> + gpiod_set_value_cansleep(led_dat->gpiod, led_dat->new_level);
(...)
> /* Setting GPIOs with I2C/etc requires a task context, and we don't
> * seem to have a reliable way to know if we're already in one; so
> * let's just assume the worst.
> @@ -72,11 +73,16 @@ static void gpio_led_set(struct led_clas
> schedule_work(&led_dat->work);
> } else {
> if (led_dat->blinking) {
> - led_dat->platform_gpio_blink_set(led_dat->gpio, level,
> - NULL, NULL);
> + 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);
Same comment.
> @@ -85,9 +91,10 @@ static int gpio_blink_set(struct led_cla
> {
> 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(led_dat->gpio, GPIO_LED_BLINK,
> + return led_dat->platform_gpio_blink_set(gpio, GPIO_LED_BLINK,
> delay_on, delay_off);
Same comment.
> @@ -97,24 +104,33 @@ static int create_gpio_led(const struct
> {
> int ret, state;
>
> - led_dat->gpio = -1;
> + if (!template->gpiod) {
> + unsigned long flags = 0;
>
> - /* skip leds that aren't available */
> - if (!gpio_is_valid(template->gpio)) {
> - dev_info(parent, "Skipping unavailable LED gpio %d (%s)\n",
> - template->gpio, template->name);
> - return 0;
> + /* skip leds that aren't available */
> + if (!gpio_is_valid(template->gpio)) {
> + dev_info(parent, "Skipping unavailable LED gpio %d (%s)\n",
> + template->gpio, template->name);
> + return 0;
> + }
> +
> + if (template->active_low)
> + flags |= GPIOF_ACTIVE_LOW;
> +
> + ret = devm_gpio_request_one(parent, template->gpio, flags,
> + template->name);
> + if (ret < 0)
> + return ret;
> +
> + led_dat->gpiod = gpio_to_desc(template->gpio);
> + if (IS_ERR(led_dat->gpiod))
> + return PTR_ERR(led_dat->gpiod);
> }
OK so this is the legacy codepath: point it out in a big fat
comment that this is the legacy codepath.
> 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, and add comments saying that either needs to be
specified but "gpio" is deprecated for "gpiod".
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/
Powered by blists - more mailing lists