Re: [Linux-stm32] [PATCH v7 10/12] ARM: dts: stm32: Fix schema warnings for pwm-leds

From: Alexander Dahl <ada@thorsis.com>
To: Ahmad Fatoum <a.fatoum@pengutronix.de>
Cc: Pavel Machek <pavel@ucw.cz>, Dan Murphy <dmurphy@ti.com>,
	Rob Herring <robh+dt@kernel.org>,
	linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
	linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-mips@vger.kernel.org,
	Jacek Anaszewski <jacek.anaszewski@gmail.com>,
	Maxime Coquelin <mcoquelin.stm32@gmail.com>,
	linux-amlogic@lists.infradead.org, linux-omap@vger.kernel.org,
	linux-stm32@st-md-mailman.stormreply.com,
	linux-leds@vger.kernel.org
Subject: Re: [Linux-stm32] [PATCH v7 10/12] ARM: dts: stm32: Fix schema warnings for pwm-leds
Date: Wed, 28 Oct 2020 08:34:38 +0100	[thread overview]
Message-ID: <5231529.NqohY00Rok@ada> (raw)
In-Reply-To: <f6ed201d-51b6-f278-7a95-3e3e49dc19ee@pengutronix.de>

Hello Ahmad,

Am Dienstag, 27. Oktober 2020, 11:58:10 CET schrieb Ahmad Fatoum:
> Hello,
> 
> On 10/27/20 11:05 AM, Alexander Dahl wrote:
> > Hello Ahmad,
> > 
> > thanks for your feedback, comments below.
> > 
> >>> -	led-rgb {
> >>> +	led-controller-2 {
> >> 
> >> Is a single RGB LED really a controller?
> > 
> > I just followed the recommendations by Rob here.
> 
> Do you happen to know if the new multicolor LED support could be used here?

AFAIK not yet. The multicolor class should be ready and it is used by some 
drivers for I²C connected LED controllers, but if I understood Pavel 
correctly, additional work has to be done for a gpio and/or pwm multicolor 
driver. See this thread from August for example:

https://lore.kernel.org/linux-leds/2530787.iFCFyWWcSu@g550jk/

> 
> I find it unfortunate that the device tree loses information relevant to
> humans to adhere to a fixed nomenclature. Apparently led-controller isn't
> even codified in the YAML binding (It's just in the examples). If you
> respin, please add a comment that this is a single RGB led. I'd prefer to
> keep the information in the DTB as well though.

The "new" attributes 'function' and 'color' attributes should cover this 
information. IIRC those were introduced sometime before v5.4 and documentation 
is in the leds/common.yaml binding. I don't see it in the scope of this patch 
series, but if we would merge this warning fix first, the information is lost, 
so maybe those attributes should be added before? 

My heuristics on that would be looking at the label and if there's a distinct 
color in it, add the color property. I could do that for all pwm LEDs known to 
the tree currently. That would be a bigger task for GPIO leds though. ;-)

> 
> >>>  		compatible = "pwm-leds";
> >>> 
> >>> -		led-red {
> >>> +		led-2 {
> >> 
> >> Shouldn't this have been led-1 as well or is the numbering "global" ?
> > 
> > Also good question. This numbering is for dts only, it usually does
> > not correspond with LEDs on the board, so it could be numbered per
> > led-controller as well?
> 
> I'd prefer that it starts by 1. That way it's aligned with PWM channel
> ID.

Ack.

> 
> Thanks for fixing the dtschema warnings by the way!

Well, I "introduced" them by converting the leds-pwm binding to yaml (not 
merged yet), so I could as well fix the warnings then? ;-)

Greets
Alex

> 
> Cheers,
> Ahmad
> 
> > Greets
> > Alex
> > 
> >>>  			label = "mc1:red:rgb";
> >>>  			pwms = <&leds_pwm 1 1000000 0>;
> >>>  			max-brightness = <255>;
> >>>  			active-low;
> >>>  		
> >>>  		};
> >>> 
> >>> -		led-green {
> >>> +		led-3 {
> >>> 
> >>>  			label = "mc1:green:rgb";
> >>>  			pwms = <&leds_pwm 2 1000000 0>;
> >>>  			max-brightness = <255>;
> >>>  			active-low;
> >>>  		
> >>>  		};
> >>> 
> >>> -		led-blue {
> >>> +		led-4 {
> >>> 
> >>>  			label = "mc1:blue:rgb";
> >>>  			pwms = <&leds_pwm 3 1000000 0>;
> >>>  			max-brightness = <255>;

-- 