linux-kernel - Re: [PATCH] ARM: dts: imx: add CX9020 Embedded PC device tree

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170712094655.GC7472@leverpostej>
Date:   Wed, 12 Jul 2017 10:46:55 +0100
From:   Mark Rutland <mark.rutland@....com>
To:     linux-kernel-dev@...khoff.com
Cc:     robh+dt@...nel.org, shawnguo@...nel.org, kernel@...gutronix.de,
        linux@...linux.org.uk, fabio.estevam@....com,
        linux-arm-kernel@...ts.infradead.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Patrick Brünn <p.bruenn@...khoff.com>
Subject: Re: [PATCH] ARM: dts: imx: add CX9020 Embedded PC device tree

Hi,

On Wed, Jul 12, 2017 at 11:04:08AM +0200, linux-kernel-dev@...khoff.com wrote:
> +/dts-v1/;
> +#include "imx53.dtsi"
> +
> +#define MX53_PAD_EIM_D26__UART2_RXD_MUX    0x144 0x48c 0x880 0x2 0x0
> +#define MX53_PAD_EIM_D27__UART2_TXD_MUX    0x148 0x490 0x000 0x2 0x0
> +#define MX53_PAD_EIM_D28__UART2_RTS        0x14c 0x494 0x87c 0x2 0x0
> +#define MX53_PAD_EIM_D29__UART2_CTS        0x150 0x498 0x000 0x2 0x0
> +
> +/ {
> +	model = "Freescale i.MX53 based Beckhoff CX9020";
> +	compatible = "fsl,imx53-qsb", "fsl,imx53";
> +
> +	chosen {
> +		stdout-path = &uart2;

No baud-rate or bits configuration?

> +	};
> +
> +	memory {
> +		reg = <0x70000000 0x20000000>,
> +		      <0xb0000000 0x20000000>;
> +	};
> +
> +	ccat {
> +		compatible = "bhf,emi-ccat";
> +	};
> +
> +	display0: display@di0 {

This unit-address (the bit after the @) isn't valid, as that should
match a reg or ranges, but this node has neither.

Just call this display-0.

> +		#address-cells =<1>;
> +		#size-cells = <0>;
> +		compatible = "fsl,imx-parallel-display";
> +		interface-pix-fmt = "rgb24";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_ipu_disp0>;
> +		status = "okay";
> +
> +		port@0 {
> +			reg = <0>;
> +			display0_in: endpoint {
> +				remote-endpoint = <&ipu_di0_disp0>;
> +			};
> +		};
> +
> +		port@1 {
> +			reg = <1>;
> +			display0_out: endpoint {
> +				remote-endpoint = <&panel_in>;
> +			};
> +		};
> +	};
> +
> +	dvi_panel: display@0 {

Likewise you have no reg here, so the unit address isn't valid.

Surely panel-0?

> +		#address-cells =<1>;
> +		#size-cells = <0>;
> +		compatible = "simple,ddc-only";

I don't see that compatible string in my Linux tree, and it doesn't make
sense to me -- "simple" isn't a vendor-prefix.

Where has this come from?

> +		ddc-i2c-bus = <&i2c2>;
> +
> +		port {
> +			panel_in: endpoint {
> +				remote-endpoint = <&display0_out>;
> +			};
> +		};
> +	};

[...]

> +	regulators {
> +		compatible = "simple-bus";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		reg_3p2v: regulator@0 {
> +			compatible = "regulator-fixed";
> +			reg = <0>;

Meaningless reg entry.

> +			regulator-name = "3P2V";
> +			regulator-min-microvolt = <3200000>;
> +			regulator-max-microvolt = <3200000>;
> +			regulator-always-on;
> +		};
> +
> +		reg_usb_vbus: regulator@1 {
> +			compatible = "regulator-fixed";
> +			reg = <1>;

Likewise.

> +			regulator-name = "usb_vbus";
> +			regulator-min-microvolt = <5000000>;
> +			regulator-max-microvolt = <5000000>;
> +			gpio = <&gpio7 8 0>;
> +			enable-active-high;
> +		};
> +	};

There's no need for a simple-bus here. It doesn't represent HW, and you
can nothing. You can put these directly under the root node, without a
synthetic reg or unnecessary container:

	reg_3p2v: regulator-3p2v {
		compatible = "regulator-fixed";
		regulator-name = "3P2V";
		regulator-min-microvolt = <3200000>;
		regulator-max-microvolt = <3200000>;
		regulator-always-on;
	};

	reg_usb_vbus: regulator-usb-vbus {
		compatible = "regulator-fixed";
		regulator-name = "usb_vbus";
		regulator-min-microvolt = <5000000>;
		regulator-max-microvolt = <5000000>;
		gpio = <&gpio7 8 0>;
		enable-active-high;
	}

Otherwise, looks fine to me.

Thanks,
Mark.