[PATCH 2/3] realtek: introduce common DTSI for ZyXEL GS1900-10HP and GS1900-8HP.

[Stijn Segers]

Hi Adrian,

Op woensdag 6 januari 2021 om 14u19 schreef Adrian Schmutzler 
<mail at adrianschmutzler.de>:
> Hi,
> 
>>  Subject: [PATCH 2/3] realtek: introduce common DTSI for ZyXEL 
>> GS1900-10HP
>>  and GS1900-8HP.
> 
> Please, no full stop after the commit title. Looks too long by the 
> way.
> 

Noted.

>>  Memory node is moved out of the rtl838.dtsi and into the device DTS 
>> as well.
> 
> Please do that separately (= in a separate commit). BTW, it looks to 
> me like this will leave all the other devices without a memory node 
> then?
> 

It would, yes, but I remedied this in my v2 which I sent in like an 
hour ago (I also marked this patch set as superceded in Patchwork, but 
you might already have been looking at it at that point?).

I will use your remarks for a v3 and send that in.

Thanks for the review!

Stijn

>> 
>>  We also uppercase the vendor name like they carry it themselves 
>> (ZyXEL)
>>  and like used elsewhere in the OpenWrt tree.
>> 
>>  Signed-off-by: Stijn Segers <foss at volatilesystems.org>
>>  ---
>>   .../realtek/dts/rtl8380_zyxel_gs1900-10hp.dts | 233 
>> +-----------------
>>   .../realtek/dts/rtl8380_zyxel_gs1900.dtsi     | 143 +++++++++++
>>   target/linux/realtek/dts/rtl838x.dtsi         |   5 -
>>   target/linux/realtek/image/Makefile           |   2 +-
>>   4 files changed, 153 insertions(+), 230 deletions(-)  create mode 
>> 100644
>>  target/linux/realtek/dts/rtl8380_zyxel_gs1900.dtsi
>> 
>>  diff --git a/target/linux/realtek/dts/rtl8380_zyxel_gs1900-10hp.dts
>>  b/target/linux/realtek/dts/rtl8380_zyxel_gs1900-10hp.dts
>>  index 4458acee2e..0ecd8bd991 100644
>>  --- a/target/linux/realtek/dts/rtl8380_zyxel_gs1900-10hp.dts
>>  +++ b/target/linux/realtek/dts/rtl8380_zyxel_gs1900-10hp.dts
>>  @@ -1,54 +1,15 @@
>>   // SPDX-License-Identifier: GPL-2.0-or-later  /dts-v1/;
> 
> If you touch this anyway, please remove the dts-v1 here which is 
> already in rtl838x.dtsi.
> 
>> 
>>  -#include "rtl838x.dtsi"
>>  -
>>  -#include <dt-bindings/input/input.h>
>>  -#include <dt-bindings/gpio/gpio.h>
>>  +#include "rtl8380_zyxel_gs1900.dtsi"
>> 
>>   / {
>>   	compatible = "zyxel,gs1900-10hp", "realtek,rtl838x-soc";
>>  -	model = "Zyxel GS1900-10HP Switch";
>>  -
>>  -	aliases {
>>  -		led-boot = &led_sys;
>>  -		led-failsafe = &led_sys;
>>  -		led-running = &led_sys;
>>  -		led-upgrade = &led_sys;
>>  -	};
>>  +	model = "ZyXEL GS1900-10HP Switch";
> 
> It would be cleaner to have the ZyXEL rename in a separate commit as 
> well.
> However, that's by far the thing I care least about in this context.
> 
> [...]
> 
>>  diff --git a/target/linux/realtek/dts/rtl8380_zyxel_gs1900.dtsi
>>  b/target/linux/realtek/dts/rtl8380_zyxel_gs1900.dtsi
>>  new file mode 100644
>>  index 0000000000..515081008b
>>  --- /dev/null
>>  +++ b/target/linux/realtek/dts/rtl8380_zyxel_gs1900.dtsi
>>  @@ -0,0 +1,143 @@
>>  +// SPDX-License-Identifier: GPL-2.0-or-later /dts-v1/;
> 
> another dts-v1 that should be removed.
> 
>>  +
>>  +#include "rtl838x.dtsi"
>>  +
>>  +#include <dt-bindings/input/input.h>
>>  +#include <dt-bindings/gpio/gpio.h>
>>  +
>>  +/ {
>>  +	aliases {
>>  +		led-boot = &led_sys;
>>  +		led-failsafe = &led_sys;
>>  +		led-running = &led_sys;
>>  +		led-upgrade = &led_sys;
>>  +	};
>>  +
>>  +	chosen {
>>  +		bootargs = "console=ttyS0,115200";
>>  +	};
>>  +
>>  +	gpio1: rtl8231-gpio {
>>  +		status = "okay";
>>  +
>>  +		poe_enable {
>>  +			gpio-hog;
>>  +			gpios = <13 0>;
>>  +			output-high;
>>  +		};
>>  +	};
>>  +
>>  +	keys {
>>  +		compatible = "gpio-keys-polled";
>>  +		poll-interval = <20>;
>>  +
>>  +		reset {
>>  +			label = "reset";
>>  +			gpios = <&gpio1 3 GPIO_ACTIVE_LOW>;
>>  +			linux,code = <KEY_RESTART>;
>>  +		};
>>  +	};
>>  +
>>  +	leds {
>>  +		compatible = "gpio-leds";
>>  +
>>  +		led_sys: sys {
>>  +			label = "gs1900:green:sys";
> 
> Maybe not in this patch, but one should consider to remove the model 
> prefix here (particularly since this is used as sysled anyway, and 
> thus probably not linked otherwise).
> 
>>  +			gpios = <&gpio0 47 GPIO_ACTIVE_HIGH>;
>>  +		};
>>  +	};
> 
> Best
> 
> Adrian