[RFC PATCH 5/7] realtek: rtl838x: replace pinctrl-single

[Olliver Schinagl]

On 29-07-2022 23:41, Sander Vanheule wrote:
> On Fri, 2022-07-29 at 14:58 +0200, Olliver Schinagl wrote:
>> On 16-07-2022 21:09, Sander Vanheule wrote:
>>> Replace the pinctrl-single node with the dedicated pinctrl driver for
>>> RTL838x SoCs. The node names are kept to stay compatible with existing
>>> references.
>>>
>>> Signed-off-by: Sander Vanheule <sander at svanheule.net>
>>> ---
>>>    target/linux/realtek/dts-5.10/rtl838x.dtsi | 38 ++++++++++------------
>>>    1 file changed, 17 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/target/linux/realtek/dts-5.10/rtl838x.dtsi b/target/linux/realtek/dts-
>>> 5.10/rtl838x.dtsi
>>> index 11cabc3f63cb..6aac2be95368 100644
>>> --- a/target/linux/realtek/dts-5.10/rtl838x.dtsi
>>> +++ b/target/linux/realtek/dts-5.10/rtl838x.dtsi
>>> @@ -169,33 +169,29 @@
>>>                  };
>>>          };
>>>    
>>> -       pinmux: pinmux at 1b001000 {
>>> -               compatible = "pinctrl-single";
>>> -               reg = <0x1b001000 0x4>;
>>> +       switchcore: switchcore-bus at 1b000000 {
>>> +               compatible = "realtek,rtl8380-switchcore", "syscon";
>>> +               reg = <0x1b000000 0x10000>;
>>>    
>>> -               pinctrl-single,bit-per-mux;
>>> -               pinctrl-single,register-width = <32>;
>>> -               pinctrl-single,function-mask = <0x1>;
>>> -               #pinctrl-cells = <2>;
>>> +               hw_led_sys: led-sys {
>>> +                       status = "disabled";
>>>    
>>> -               enable_uart1: pinmux_enable_uart1 {
>>> -                       pinctrl-single,bits = <0x0 0x10 0x10>;
>>> +                       label = "green:status";
>>>                  };
>>> -       };
>>>    
>>> -       /* LED_GLB_CTRL */
>>> -       pinmux_led: pinmux at 1b00a000 {
>>> -               compatible = "pinctrl-single";
>>> -               reg = <0x1b00a000 0x4>;
>>> +               pinctrl {
>>> +                       compatible = "realtek,rtl8380-pinctrl";
>>>    
>>> -               pinctrl-single,bit-per-mux;
>>> -               pinctrl-single,register-width = <32>;
>>> -               pinctrl-single,function-mask = <0x1>;
>>> -               #pinctrl-cells = <2>;
>>> +                       /* enable GPIO 0 */
>>> +                       pinmux_disable_sys_led: sys-led-mux {
>>> +                               groups = "sys-led";
>>> +                               function = "gpio";
>>> +                       };
>> with your changes, why not use the hw function?
> Because I am of the opinion that 64ms or 1024ms toggle interval is insufficient. And then
> we have to hope that OpenWrt will always choose the fast/slow blinking intervals in such a
> way that it ends up with the correct selection. Otherwise there will be no way for the
> user to distinguish the enter-failsafe moment from the rest of the boot.

I actually agree strongly with you here after giving it some more 
thought. For example, I want my LED to be first (obviously) be managed 
by openwrt for _consistent_ openwrt behavior; and then once the board is 
up; I want the power-led to be an 'inverted heartbeat'. So yes, having 
it by default be a GPIO, and then later let the user use some hardware 
accelerated functionality, makes a lot more sense, which I doubt we'll 
ever need.

Suppose it is useful for the boot scenario's, unmanaged switches etc, 
where you don't have an OS running yet, but other then that, I agree the 
hw accelerated blink doens't offer much once linux is running and can do 
a much better job.


(out of context here, but the same can be said for the netdev LEDs, 
having them initially as GPIO's or manually controlled leds is better, 
as that also allows a startup script to run some sort of LED tests we 
often see switches doing).

>>>    
>>> -               /* enable GPIO 0 */
>>> -               pinmux_disable_sys_led: disable_sys_led {
>>> -                       pinctrl-single,bits = <0x0 0x0 0x8000>;
>>> +                       enable_uart1: uart1-mux {
>> &enable_uart1 { status=disabled }; reads weird though :p
> This is just here for easy use later. It doesn't actually do anything until another node
> request this in their pinctrl property.
I meant the name of the alias :)
>
>>> +                               groups = "uart1";
>>> +                               function = "uart1";
>>> +                       };
>>>                  };
>>>          };
>>>    
>>>
>>
>> _______________________________________________
>> openwrt-devel mailing list
>> openwrt-devel at lists.openwrt.org
>> https://lists.openwrt.org/mailman/listinfo/openwrt-devel