[PATCH 3/3] ARM: dts: marvell: add support for D-Link DNS-320L

[Andrew Lunn]

On Mon, Jul 01, 2024 at 08:01:46AM +0200, Krzysztof Kozlowski wrote:
> On 29/06/2024 13:34, Zoltan HERPAI wrote:

> > +++ b/arch/arm/boot/dts/marvell/kirkwood-dns320l.dts
> > @@ -0,0 +1,197 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Device Tree file for D-Link DNS-320L
> > + *
> > + * Copyright (C) 2024, Zoltan HERPAI <wigyori at uid0.hu>
> > + * Copyright (C) 2015, Sunke Schluters <sunke-dev at schlueters.de>
> > + *
> > + * This file is based on the works of:
> > + * - Sunke Schluters <sunke-dev at schlueters.de>
> > + *   - https://github.com/scus1/dns320l/blob/master/kernel/dts/kirkwood-dns320l.dts
> > + * - Andreas Bohler <dev at aboehler.at>:
> > + *   - http://www.aboehler.at/doku/doku.php/projects:dns320l
> > + *   - http://www.aboehler.at/hg/linux-dns320l/file/ba7a60ad7687/linux-3.12/kirkwood-dns320l.dts
> > + */
> > +
> > +/dts-v1/;
> > +
> > +#include "kirkwood.dtsi"
> > +#include "kirkwood-6281.dtsi"
> > +
> > +/ {
> > +	model = "D-Link DNS-320L";
> > +	compatible = "dlink,dns320l", "marvell,kirkwood-88f6702", "marvell,kirkwood";
> > +
> > +	memory {
> > +		device_type = "memory";
> > +		reg = <0x00000000 0x10000000>;
> > +	};
> > +
> > +	chosen {
> > +		bootargs = "console=ttyS0,115200n8 earlyprintk";
> > +		stdout-path = &uart0;
> > +	};
> > +
> > +	gpio-keys {
> > +		compatible = "gpio-keys";
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> 
> Nope, these cannot be there.

Depends. The kernel, which is what really matters, is happy with them
there. Have a look at all the other kirkwood dts files.

> It does not look like you tested the DTS against bindings. Please run
> `make dtbs_check W=1` (see
> Documentation/devicetree/bindings/writing-schema.rst or
> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
> for instructions).

I suspect that is not going to be easy to interpret. kirkwood is very
old, much older than the YAML descriptions. DT descriptions of this
age were considered correct if the kernel understood them, and the
kernel is much more flexible than the YAML bindings. As a result,
there are going to be a huge number of warnings, and it will take a
lot of skill to pick out real warning which can be fixed from the
noise. Also, nobody really cares, because these devices have been out
of production for a decade. Nobody is going to clean up the DT files.

> > +		pinctrl-0 = <&pmx_buttons>;
> > +		pinctrl-names = "default";
> > +
> > +		button at 1 {
> > +			label = "Reset push button";
> > +			linux,code = <KEY_RESTART>;
> > +			gpios = <&gpio0 28 1>;
> > +		};
> > +
> > +		button at 2 {
> > +			label = "USB unmount button";
> > +			linux,code = <KEY_EJECTCD>;
> > +			gpios = <&gpio0 27 1>;
> > +		};
> > +	};
> > +
> > +	gpio-leds {
> > +		compatible = "gpio-leds";
> > +		pinctrl-0 = <&pmx_leds>;
> > +		pinctrl-names = "default";
> > +
> > +		blue-usb {
> 
> It does not look like you tested the DTS against bindings. Please run
> `make dtbs_check W=1` (see
> Documentation/devicetree/bindings/writing-schema.rst or
> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
> for instructions).

> > +	ocp at f1000000 {
> 
> Why you are not overriding by label/phandle?

Look at the old .dts files. That is the way it was done 10 years
ago. This is uniform with other kirkwood .dts files. There is
something to be said for being uniform with other files of the same
sort.

I think we need to find a balance here. I agree with some of your
comments, removing the regulator container, moving gpio-keys and
gpio-leds. But i think ocp at f1000000 and not using labels can stay.

	Andrew