Re: [PATCH] arm64: dts: rockchip: Pinebook Pro: Update WiFi

[Dragan Simic <dsimic@manjaro.org>]

Hello Peter,

On 2025-07-29 22:09, Peter Robinson wrote:
>> On 2025-07-29 20:58, Peter Robinson wrote:
>> > Update the WiFi configuration to include the wake-up
>> > pin and add an ethernet alias to allow assignment of
>> > a mac-address from the firmware.
>> >
>> > Signed-off-by: Peter Robinson <pbrobinson@gmail.com>
>> > ---
>> >  .../boot/dts/rockchip/rk3399-pinebook-pro.dts | 19 +++++++++++++++++++
>> >  1 file changed, 19 insertions(+)
>> >
>> > diff --git a/arch/arm64/boot/dts/rockchip/rk3399-pinebook-pro.dts
>> > b/arch/arm64/boot/dts/rockchip/rk3399-pinebook-pro.dts
>> > index 5a8551d9ffe47..05c48cb09df6f 100644
>> > --- a/arch/arm64/boot/dts/rockchip/rk3399-pinebook-pro.dts
>> > +++ b/arch/arm64/boot/dts/rockchip/rk3399-pinebook-pro.dts
>> > @@ -19,6 +19,7 @@ / {
>> >       chassis-type = "laptop";
>> >
>> >       aliases {
>> > +             ethernet0 = &brcmf;
>> 
>> I'm sorry, but this alias isn't acceptable.  In a few words, this
>> simply isn't an Ethernet interface.
> 
> Can you explain why? It's a wireless ethernet interface with a MAC
> address, there's other example in the kernel doing this so there's
> already a precedent, can you explain how else to do this or give me an
> example?

Strictly speaking, there's no such thing as Wireless Ethernet
interface.  Yes, WiFi interfaces do produce frames that, from the
drivers' perspective, look very much like wired Ethernet frames,
but WiFi technically isn't just "Ethernet over radio waves".  Truth
be told, WiFi used to be called that in its early days, perhaps
to make it more appealing, but the distinction became clear rather
soon.  It's somewhat similar to how tunneling protocols may produce
Ethernet frames, but they're still called tunnel interfaces.

I'm not aware of an already existing example of a WiFi interface
that's aliased as ethernetX.  Could you, please, be more precise?
Anyway, the correct way should be to use a wifi0 alias here, which
is already used in a few places in arch/arm64/boot/dts/qcom, for
example.  I'm not sure will a wifi0 alias work "automagically", but
if there are some issues preventing it from working as expected,
fixing those issues would be the right thing to do.

>> We're already stretching a bit the dynamic nature of, well, everything
>> with the aliases in the Rockchip DT files, and an alias like this one
>> would be stretching the whole thing beyond the breaking point.
> 
> Why? If you look through DT outside of rockchip there's even more
> stretching and I don't see how using this goes beyond breaking, I
> think this is a bit of an exaggeration. Rather than saying no how
> about explaining an alternative solution?

Sorry, I wasn't clear enough in this part of my response.  To clarify
it a bit, I referred to stretching it beyond breaking by adding a
technically incorrect alias.

Please see above for a technically correct alternative approach.

>> >               mmc0 = &sdio0;
>> >               mmc1 = &sdmmc;
>> >               mmc2 = &sdhci;
>> > @@ -883,6 +884,12 @@ vcc5v0_host_en_pin: vcc5v0-host-en-pin {
>> >               };
>> >       };
>> >
>> > +     wifi {
>> > +             wifi_host_wake_l: wifi-host-wake-l {
>> > +                     rockchip,pins = <0 RK_PA3 RK_FUNC_GPIO &pcfg_pull_none>;
>> > +             };
>> > +     };
>> > +
>> >       wireless-bluetooth {
>> >               bt_wake_pin: bt-wake-pin {
>> >                       rockchip,pins = <2 RK_PD3 RK_FUNC_GPIO &pcfg_pull_none>;
>> > @@ -940,7 +947,19 @@ &sdio0 {
>> >       pinctrl-names = "default";
>> >       pinctrl-0 = <&sdio0_bus4 &sdio0_cmd &sdio0_clk>;
>> >       sd-uhs-sdr104;
>> > +     #address-cells = <1>;
>> > +     #size-cells = <0>;
>> >       status = "okay";
>> > +
>> > +     brcmf: wifi@1 {
>> > +             reg = <1>;
>> > +             compatible = "brcm,bcm4329-fmac";
>> > +             interrupt-parent = <&gpio0>;
>> > +             interrupts = <RK_PA3 IRQ_TYPE_LEVEL_HIGH>;
>> > +             interrupt-names = "host-wake";
>> > +             pinctrl-names = "default";
>> > +             pinctrl-0 = <&wifi_host_wake_l>;
>> > +     };
>> >  };
>> >
>> >  &sdhci {