Re: [PATCH 05/40] arm64: dts: rockchip: Add frl-enable-gpios to rk3576-luckfox-core3576

[Cristian Ciocaltea <cristian.ciocaltea@collabora.com>]

Hi Heiko,

On 4/18/26 2:12 AM, Heiko Stuebner wrote:
> Hi Cristian,
> 
> Am Freitag, 17. April 2026, 18:34:17 Mitteleuropäische Sommerzeit schrieb Cristian Ciocaltea:
>> On 4/17/26 2:32 PM, Heiko Stuebner wrote:
>>> the comments below apply sort of to all patches in that series.
>>>
>>> Am Freitag, 17. April 2026, 11:24:39 Mitteleuropäische Sommerzeit schrieb Cristian Ciocaltea:
>>>> The board exposes the GPIO4_C6 line to control the voltage bias on the
>>>> HDMI data lines.  It must be asserted when operating in HDMI 2.1 FRL
>>>> mode and deasserted for HDMI 1.4/2.0 TMDS mode.
>>>>
>>>> Wire up the HDMI node to the GPIO line using the frl-enable-gpios
>>>> property and drop the line from the vcc_5v0_hdmi regulator to allow
>>>> adjusting the bias when transitioning between TMDS and FRL operating
>>>> modes.

[...]

>>>
>>>
>>>> @@ -231,6 +228,8 @@ &gpu {
>>>>  };
>>>>  
>>>>  &hdmi {
>>>> +	pinctrl-0 = <&hdmi_txm0_pins &hdmi_tx_scl &hdmi_tx_sda &hdmi_frl_en>;
>>>> +	frl-enable-gpios = <&gpio4 RK_PC6 GPIO_ACTIVE_LOW>;
>>>
>>> this should be sorted the other way around I think.
>>>
>>> Also please provide a pinctrl-names property too. If for whatever reason
>>> the dw-hdmi aquires a 2nd pinctrl state in the future, this makes sure
>>> board DTs are staying in the "old" compatible mode until they are adapted.
>>
>> Just to make sure I fully understand, the convention is that 
>>
>>   pinctrl-names = "default";
>>
>> should be always provided, even when the node overrides an existing pinctrl-0
>> property?
>>
>> E.g. in rk3576.dtsi we have:
>>
>>   hdmi: hdmi@27da0000 {
>>     ...
>>     pinctrl-names = "default";
>>     pinctrl-0 = <&hdmi_txm0_pins &hdmi_tx_scl &hdmi_tx_sda>;
>>     ...
>>   }
>>
>> Hence I omitted pinctrl-names which doesn't change and just appended
>> &hdmi_frl_en to pinctrl-0's original value.
> 
> correct, please always provide a pinctrl-names entry when setting a new
> pinctrl-0 .
> 
> The background is, imagine you have a base:
> 
> pinctrl-names = "default";
> pinstrl-0 = <....>;
> 
> and override pinctrl-0 in a board.
> 
> Now a newer binding introduces a 2nd pinctrl state "foo". Of course
> we're backwards compatible, and both are valid and the driver checks
> what states are defined.
> 
> So the base sets:
> pinctrl-names = "default", "foo";
> pinctrl-0 = <...>;
> pinctrl-1 = <...>;
> 
> in your (old) board you override pinctrl-0, but the driver still sees
> the new variant with 2 pinctrl states, where it should've stayed with
> the legacy 1-state, until the board-dts might get adapted in the future.
> 
> 
> And I know, we're likely not doing that everywhere, and also in most
> cases it won't really matter, but still it is safer and sets the better
> precedent :-) .

Thanks for the detailed explanation, that clears things up!

There are several other nodes (e.g. i2c, pwm, uart) that also lack
pinctrl-names despite providing pinctrl-0 - I can address those in a
separate patch.

I also noticed an inconsistency in property ordering: some nodes place
pinctrl-names before pinctrl-<n> and others after.  I have always used
the former, but we should probably prefer the latter to stay consistent
with how clocks, resets, phys, etc. are ordered.

Thoughts?

> 
>>>>  	status = "okay";
>>>>  };
>>>>  
>>>> @@ -655,7 +654,7 @@ &pcie0 {
>>>>  
>>>>  &pinctrl {
>>>>  	hdmi {
>>>> -		hdmi_con_en: hdmi-con-en {
>>>> +		hdmi_frl_en: hdmi-frl-en {
>>>
>>> pinctrl names should ideally match the naming in schematics, for example the
>>> "HDMI0_TX_ON_H" for jaguar and tiger. This makes it way easier to> go from DT
>>> to schematics and back.
>>
>> I opted for a more descriptive name that could be used consistently across all
>> boards, given that not all schematics are publicly available.
>>
>> You make a fair point though, we should probably stick with the pretty terrible
>> hdmi[N]_tx_on_h naming instead.
> 
> yep, we're doing that everywhere else already too, and sticking to the
> schematics naming, also prevents any discussions about how something
> should be named ;-) .

Indeed. :)