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?
> 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-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
