Re: [PATCH v1 1/1] add support for mws4 board

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 29/05/2022 13:53, Marian Ulbricht wrote:
> Subject: [PATCH v1 1/1] add support for mws4 board

Your patch is corrupted. The easiest to create proper patch is to use
`git format-patch`.

Subject: use prefixes matching the subsystem (git log oneline --)

> 
> mws4 is an arm based nuclear probe hardware used from
> german government to monitor nuclear activity

I have troubles parsing it... It was made by German government or used
by German government or leased from German government?

In any case please use proper full-stops and capital letters.

> 
> Signed-off-by: Ulbricht, Marian <ulbricht@innoroute.de>

Please use exactly the same name details as in From field.

> ---
> 
> Changes in v1:
> * add dts

Changelog describes changes against previous version. Was there v0?

> 
>   arch/arm/boot/dts/omap3-mws4.dts | 297 +++++++++++++++++++++++++++++++
>   1 file changed, 297 insertions(+)
>   create mode 100644 arch/arm/boot/dts/omap3-mws4.dts
> 
> diff --git a/arch/arm/boot/dts/omap3-mws4.dts 
> b/arch/arm/boot/dts/omap3-mws4.dts
> new file mode 100644
> index 000000000000..a3489d4af2cc
> --- /dev/null
> +++ b/arch/arm/boot/dts/omap3-mws4.dts
> @@ -0,0 +1,297 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com/
> + *
> + * Modified 2015 by Bernhard Gätzschmann, Ultratronik from Beagleboard xM
> + *
> + * Modified 2022 Marian Ulbricht ulbricht@innoroute.de
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.

No need for standard license boiler plate. None of other files have it.

> + */
> +/dts-v1/;
> +#include "omap36xx.dtsi"

Blank line.

> +/ {
> +	model = "Ultratronik BFS MWS4";
> +	compatible = "ti,omap3-mmi4", "ti,omap36xx", "ti,omap3";

The compatible looks weird - does not match your model. Are you sure
this is your board compatible?

> +	cpus {
> +			cpu@0 {

Indentation is corrupted. The code has to be looking proper, not like
combined from random pieces.


> +				cpu0-supply = <&vcc>;
> +			};
> +		};

missing blank line.

> +	memory {
> +		device_type = "memory";
> +		reg = <0x80000000 0x10000000>; // 256 MB
> +	};
> +	aliases {
> +		i2c0 = &i2c1;
> +		i2c1 = &i2c2;
> +		i2c2 = &i2c3;
> +		serial0 = &uart1;
> +		serial1 = &uart2;
> +		serial2 = &uart3;
> +		mmc1 = &mmc1;
> +	};
> +	netcard: AX88796BLI@ffdf0000 {
> +		compatible = "ax88796_dt";
> +		reg = <0xffdf0000 0x1000> ;
> +
> +};
> +
> +	watchdog_max: watchdog {
> +		compatible = "linux,wdt-gpio";
> +		gpios = <&gpio4 21 1>; //117
> +		hw_algo = "toggle";
> +		always-running;
> +		hw_margin_ms = <900>;
> +	};
> +	hsusb1_phy: hsusb1_phy {
> +		status = "disabled";
> +	};
> +
> +	/* HS USB Host PHY on PORT 2 */
> +	hsusb2_phy: hsusb2_phy {
> +		status = "disabled";
> +	};
> +	/* fixed 19.2MHz oscillator */
> +	hfclk_19m2: oscillator {
> +		#clock-cells = <0>;
> +		compatible = "fixed-clock";
> +		clock-frequency = <19200000>;
> +	};
> +leds {

Sorry, this code looks absolutely terrible. This does not fit any Linux
DTS coding style. Please open existing (recent) files and base your work
on them.

Best regards,
Krzysztof