Hi Linus,
Thank you for the patch.
One trivial nit below.
On 09/07/2018 12:09 AM, Linus Walleij wrote:
> This augments the GPIO lookup code in the GPIO LEDs to
> attempt to look up a GPIO descriptor from the device with
> index.
>
> This makes it possible to use GPIO machine look-up tables
> and stop passing global GPIO numbers through platform data.
>
> Using this we can stepwise convert existing board files
> to use machine descriptor tables and then eventually drop
> the legacy GPIO support and only include
> <linux/gpio/consumer.h> and use descriptors exclusively.
>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---
> drivers/leds/leds-gpio.c | 93 +++++++++++++++++++++++++++-------------
> 1 file changed, 63 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
> index 764c31301f90..fbc623148595 100644
> --- a/drivers/leds/leds-gpio.c
> +++ b/drivers/leds/leds-gpio.c
> @@ -81,35 +81,6 @@ static int create_gpio_led(const struct gpio_led *template,
> {
> int ret, state;
>
> - 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
> - * rid of this block completely.
> - */
> - unsigned long flags = GPIOF_OUT_INIT_LOW;
> -
> - /* 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 (!led_dat->gpiod)
> - return -EINVAL;
> - }
> -
> led_dat->cdev.name = template->name;
> led_dat->cdev.default_trigger = template->default_trigger;
> led_dat->can_sleep = gpiod_cansleep(led_dat->gpiod);
> @@ -231,6 +202,53 @@ static const struct of_device_id of_gpio_leds_match[] = {
>
> MODULE_DEVICE_TABLE(of, of_gpio_leds_match);
>
> +static struct gpio_desc *gpio_led_get_gpiod(struct device *dev, int idx,
> + const struct gpio_led *template)
> +{
> + struct gpio_desc *gpiod;
> + unsigned long flags = GPIOF_OUT_INIT_LOW;
> + int ret;
> +
> + /*
> + * This means the LED does not come from the device tree
> + * or ACPI, so let's try just getting it by index from the
> + * device, this will hit the board file, if any and get
> + * the GPIO from there.
> + */
> + gpiod = devm_gpiod_get_index(dev, NULL, idx, flags);
> + if (!IS_ERR(gpiod)) {
> + gpiod_set_consumer_name(gpiod, template->name);
> + return gpiod;
> + }
> + if (PTR_ERR(gpiod) != -ENOENT)
> + return 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.
> + */
> +
> + /* skip leds that aren't available */
> + if (!gpio_is_valid(template->gpio)) {
> + return ERR_PTR(-ENOENT);
> + }
Curly braces are not needed for single statement block.
I can fix it up by myself if you agree.
> + if (template->active_low)
> + flags |= GPIOF_ACTIVE_LOW;
> +
> + ret = devm_gpio_request_one(dev, template->gpio, flags,
> + template->name);
> + if (ret < 0)
> + return ERR_PTR(ret);
> +
> + gpiod = gpio_to_desc(template->gpio);
> + if (!gpiod)
> + return ERR_PTR(-EINVAL);
> +
> + return gpiod;
> +}
> +
> static int gpio_led_probe(struct platform_device *pdev)
> {
> struct gpio_led_platform_data *pdata = dev_get_platdata(&pdev->dev);
> @@ -246,7 +264,22 @@ static int gpio_led_probe(struct platform_device *pdev)
>
> priv->num_leds = pdata->num_leds;
> for (i = 0; i < priv->num_leds; i++) {
> - ret = create_gpio_led(&pdata->leds[i], &priv->leds[i],
> + const struct gpio_led *template = &pdata->leds[i];
> + struct gpio_led_data *led_dat = &priv->leds[i];
> +
> + if (template->gpiod)
> + led_dat->gpiod = template->gpiod;
> + else
> + led_dat->gpiod =
> + gpio_led_get_gpiod(&pdev->dev,
> + i, template);
> + if (IS_ERR(led_dat->gpiod)) {
> + dev_info(&pdev->dev, "Skipping unavailable LED gpio %d (%s)\n",
> + template->gpio, template->name);
> + continue;
> + }
> +
> + ret = create_gpio_led(template, led_dat,
> &pdev->dev, NULL,
> pdata->gpio_blink_set);
> if (ret < 0)
>
--
Best regards,
Jacek Anaszewski
