[PATCH 1/2] ath79: add support for Mojo Networks AirTight C-75

[Tomasz Maciej Nowak]

W dniu 14.12.2020 o 17:28, Adrian Schmutzler pisze:
> Hi,
> 
> only have time for a quick subset of answers:
> 

[...]

>>>>  arduino,yun|\
>>>>  buffalo,bhr-4grv2|\
>>>>  devolo,magic-2-wifi|\
>>>> diff --git a/target/linux/ath79/dts/qca9550_mojo_c-75.dts
>>>> b/target/linux/ath79/dts/qca9550_mojo_c-75.dts
>>>> new file mode 100644
>>>> index 000000000000..51046a740a02
>>>> --- /dev/null
>>>> +++ b/target/linux/ath79/dts/qca9550_mojo_c-75.dts
>>>> @@ -0,0 +1,173 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-or-later OR MIT
>>>> +
>>>> +#include <dt-bindings/gpio/gpio.h>
>>>> +#include <dt-bindings/input/input.h>
>>>> +
>>>> +#include "qca955x.dtsi"
>>>
>>> This include needs to be first (after the license).
>>
>> I followed Linux kernel style in which first we add global includes in
>> alphabetical order then local includes. Will change that.
> 
> This is just because dts-v1 is introduced from ath79.dtsi via qca955x.dtsi and dts-v1 has to be before anything else. This is also done like this in kernel (but not consistently), e.g. ipq4019/ipq806x.

Ok.

[...]

>>>> +
>>>> +	aliases {
>>>> +		label-mac-device = &eth0;
>>>> +		led-boot = &led_power;
>>>> +		led-failsafe = &led_power;
>>>> +		led-upgrade = &led_power;
>>>
>>> No led-running?
>>
>> The power LED is by default on, so it makes led-running redundant. Is there
>> something else also using that alias?
> 
> default-on is first, then diag.sh starts doing stuff due to led-boot, i.e. will have the LED blinking. I'm not sure what happens after that without led-running, i.e. is the LED turned off or on. Anyway, that will be the job of diag.sh then, not of default-on anymore.

The power led behaves as intended with every led-* state and stays on after, so I don't think there's a need to duplicate that in userspace and will keep the current aliases. If I'll come across any issues with that, I'll send a patch.

> 
>>
>>>
>>>> +	};
>>>> +
>>>> +	keys {
>>>> +		compatible = "gpio-keys";
>>>> +
>>>> +		reset {
>>>> +			linux,code = <KEY_RESTART>;
>>>> +			gpios = <&gpio 17 GPIO_ACTIVE_LOW>;
>>>
>>> missing label?
>>
>> The label is set from the node name by default.
> 
> Hmm, so essentially we could drop label for about 50 % of our keys (in contrast to the more complex situation with LEDs)?

Sorry, got confused with LEDs and thought that's the same with gpio input, I'll add label to this node.

> 
>>
>>>
>>>> +		};
>>>> +	};
>>>> +
>>>> +	leds {
>>>> +		compatible = "gpio-leds";
>>>> +
>>>> +		led_power: power {
>>>> +			label = "amber:power";
>>>> +			gpios = <&gpio 14 GPIO_ACTIVE_HIGH>;
>>>> +			default-state = "on";
>>>> +		};
>>>> +
>>>> +		wlan2g {
>>>> +			label = "green:wlan2g";
>>>> +			gpios = <&gpio 13 GPIO_ACTIVE_LOW>;
>>>> +			linux,default-trigger = "phy1tpt";
>>>> +		};
>>>> +	};
>>>> +};
>>>> +
>>>> +&eth0 {
>>>> +	status = "okay";
>>>> +
>>>> +	mtd-mac-address = <&art 0x0>;
>>>> +	phy-handle = <&phy0>;
>>>> +	phy-mode = "rgmii";
>>>> +	pll-data = <0xa6000000 0x00000101 0x00001616>; };
>>>> +
>>>> +&eth1 {
>>>> +	status = "okay";
>>>> +
>>>> +	mtd-mac-address = <&art 0x6>;
>>>> +	phy-mode = "sgmii";
>>>> +	pll-data = <0x03000101 0x00000101 0x00001616>;
>>>> +
>>>> +	fixed-link {
>>>> +		speed = <1000>;
>>>> +		full-duplex;
>>>> +	};
>>>> +};
>>>> +
>>>> +&mdio0 {
>>>> +	status = "okay";
>>>> +
>>>> +	phy0: ethernet-phy at 0 {
>>>> +		reg = <0>;
>>>> +		qca,ar8327-initvals = <
>>>> +			0x0c 0x00080080
>>>> +			0x04 0x07600000
>>>> +			0x58 0xc935c935
>>>> +			0x5c 0x03ffff00
>>>> +			0x7c 0x0000007e
>>>> +			0x94 0x0000007e
>>>> +		>;
>>>> +	};
>>>> +};
>>>
>>> Please move mdio0 up so it's either directly before or after eth0.
>>
>> The nodes are sorted alphabetically. There are no includes specified
>> between nodes, which would make following applied sorting not possible.
>> What are the criteria saying that this node should be above &eth nodes?
> 
> Well, mdioX is a subnode of ethX. Thus, I tend to see them as related, and that's probably also the reason why they are typically grouped together in the ar****/qca**** DTSI files.
> It just makes reading the config easier for me when they are grouped this way. I won't force you to change that if you think alphabetic sorting is superior.

Ok, that's sensible I'll move it after eth0.

[...]

> 
> Best
> 
> Adrian
> 

Regards.

-- 
TMN