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

[Krzysztof Kozlowski]

On 01/07/2024 15:25, Andrew Lunn wrote:
> 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.

They cannot be here because the binding does not allow it. They are
redundant though, in a way that kernel will work.

> 
>> 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.

One can just read the binding. Is there address/size cells?

That's true that you need cleaned up platform to make efficient use of
the tools, but one can test one particular schema which would print just
limited amount of bindings.

> 
>>> +		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.

That's ok.

> 

Best regards,
Krzysztof