Re: [PATCH 2/3] leds: lp50xx: KConfig: fix dependencies

Re: [PATCH 2/3] leds: lp50xx: KConfig: fix dependencies

[Andy Shevchenko]

On Sat, Jul 31, 2021 at 12:01:17AM +0200, Jan Kundrát wrote:
> I just lost a few hours of debugging why the heck my sysfs nodes were
> not created even though devm_led_classdev_multicolor_register_ext() was
> returning 0. It turned out that I was missing support for the multicolor
> LED device class. No errors were reported, neither during the build, nor
> at runtime -- but the DTS configuration of the connected LEDs was
> silently ignored.

Which is okay. The feature as far as I can see is optional.

> The driver also really needs DTS -- probe fails if there are no children
> in the DT, so the direct binding from userspace probably doesn't work.
> That's interesting because commit
> ea1ff99c9d235b8a54571d4292c71fce60993117 suggests that a direct bind was
> supposed to work.

First of all, please use standard reference to the commit (it will give a bit
more context here), i.e.

ea1ff99c9d23 ("leds: lp50xx: Switch to new style i2c-driver probe function")

Second, it suggests that in general. While this driver currently won't be
instantiated via user space, it the future we potentially might have a way
how to construct software node(s) from user space (perhaps using configfs)
and it will make the above statement a complete truth.

...

> -	depends on LEDS_CLASS_MULTICOLOR || !LEDS_CLASS_MULTICOLOR
> +	depends on LEDS_CLASS_MULTICOLOR

No, until there is a justification why it should be non-optional.

You may add a debug message in order to avoid others to waste time on
understanding the issue in the cases same to yours.

...

> +	depends on OF

No, this driver is not OF-dependent. This change will bring a regression to it
on ACPI-based systems.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] leds: lp50xx: Fix chip name in KConfig

[Andy Shevchenko]

On Fri, Jul 30, 2021 at 11:59:11PM +0200, Jan Kundrát wrote:
> The 9-channel one is called LP5009, not LP509.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Jan Kundrát <jan.kundrat@cesnet.cz>
> ---
>  drivers/leds/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index bdf16180f5ff..a5101dfa2713 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -397,7 +397,7 @@ config LEDS_LP3952
>  	  module will be called leds-lp3952.
>  
>  config LEDS_LP50XX
> -	tristate "LED Support for TI LP5036/30/24/18/12/9 LED driver chip"
> +	tristate "LED Support for TI LP5036/30/24/18/12/09 LED driver chip"
>  	depends on LEDS_CLASS && REGMAP_I2C
>  	depends on LEDS_CLASS_MULTICOLOR || !LEDS_CLASS_MULTICOLOR
>  	help
> -- 
> 2.32.0
> 
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] leds: lp50xx: Fix chip name in KConfig

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 329 bytes --]

On Mon 2021-08-02 14:23:46, Andy Shevchenko wrote:
> On Fri, Jul 30, 2021 at 11:59:11PM +0200, Jan Kundrát wrote:
> > The 9-channel one is called LP5009, not LP509.
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Applied, thanks.
							Pavel
							
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 3/3] leds: lp50xx: LP5009 supports three LED banks

[Andy Shevchenko]

On Sat, Jul 31, 2021 at 12:07:57AM +0200, Jan Kundrát wrote:

Something is missed here...

-- 
With Best Regards,
Andy Shevchenko