Re: [PATCH] [v3] leds: gpio: make legacy gpiolib interface optional

[Lee Jones]

On Wed, 06 May 2026, Andy Shevchenko wrote:

> On Tue, May 05, 2026 at 05:58:48PM +0200, Arnd Bergmann wrote:
> 
> > There are still a handful of ancient mips/armv5/sh boards that use the
> > gpio_led:gpio member to pass an old-style gpio number, but all modern
> > users have been converted to gpio descriptors.
> > 
> > While the CONFIG_GPIOLIB_LEGACY option that guards devm_gpio_request_one()
> > and related helpers is currently turned on in all kernel builds,
> > the plan is to only enable it on the few platforms that actually
> > pass gpio numbers in any platform_data.
> > 
> > Split out the legacy portion of the platform_data handling into a custom
> > helper function that is guarded with in #ifdef block, to allow the
> > the leds-gpio driver to compile cleanly when CONFIG_GPIOLIB_LEGACY
> > gets turned off. Once the last user is converted, this function can
> > be removed.
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> There is a couple of nit-picks, but I'm not objecting if they are not
> going to be addressed.
> 
> ...
> 
> >  	gpiod = devm_gpiod_get_index_optional(dev, NULL, idx, GPIOD_OUT_LOW);
> > -	if (IS_ERR(gpiod))
> > -		return gpiod;
> > -	if (gpiod) {
> > +	if (!IS_ERR(gpiod))
> >  		gpiod_set_consumer_name(gpiod, template->name);
> > -		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.
> > -	 */
> > +	return gpiod;
> 
> Okay, let's stick with this form.
> 
> ...
> 
> > -			if (template->gpiod)
> > -				led_dat->gpiod = template->gpiod;
> > -			else
> > -				led_dat->gpiod =
> > -					gpio_led_get_gpiod(dev, i, template);
> > +			led_dat->gpiod = gpio_led_get_gpiod(dev, i, template);
> > +			if (!led_dat->gpiod)
> > +				led_dat->gpiod = gpio_led_get_legacy_gpiod(dev,
> > +								  i, template);
> 
> Why not keep the style as before:
> 
> 				led_dat->gpiod =
> 					gpio_led_get_legacy_gpiod(dev, i, template);
> 
> ? (Yes, it's a bit longer than 80, but I think in this case it's justified for
> readability).
> 
> > +
> 
> This blank like is not needed.

May as well fix them before I do a full review.

-- 
Lee Jones