Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 27 Apr 2022 16:16:45 -0500
From:   Rob Herring <robh@kernel.org>
To:     Detlev Casanova <detlev.casanova@collabora.com>
Cc:     linux-kernel@vger.kernel.org, arnd@arndb.de,
        bcm-kernel-feedback-list@broadcom.com, devicetree@vger.kernel.org,
        f.fainelli@gmail.com, frowand.list@gmail.com,
        linux-arm-kernel@lists.infradead.org,
        linux-rpi-kernel@lists.infradead.org, masahiroy@kernel.org,
        michal.lkml@markovi.net, ndesaulniers@google.com,
        nsaenz@kernel.org, olof@lixom.net, rjui@broadcom.com,
        sbranden@broadcom.com, soc@kernel.org, stefan.wahren@i2se.com
Subject: Re: [RFC PATCH v2 3/3] ARM: dto: Add bcm2711-rpi-7-inches-ts.dts
 overlay
Message-ID: <YmmyvdjiG7s/Qil4@robh.at.kernel.org>
References: <20220427185243.173594-1-detlev.casanova@collabora.com>
 <20220427185243.173594-4-detlev.casanova@collabora.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220427185243.173594-4-detlev.casanova@collabora.com>
Precedence: bulk

On Wed, Apr 27, 2022 at 02:52:43PM -0400, Detlev Casanova wrote:
> Add a device tree overlay to support the official Raspberrypi 7" touchscreen for
> the bcm2711 devices.
> 
> The panel is connected on the DSI 1 port and uses the simple-panel
> driver.
> 
> The device tree also makes sure to activate the pixelvalve[0-4] CRTC modules
> 
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> ---
>  arch/arm/boot/dts/Makefile                    |   4 +
>  arch/arm/boot/dts/overlays/Makefile           |   3 +
>  .../dts/overlays/bcm2711-rpi-7-inches-ts.dts  | 125 ++++++++++++++++++

.dtso is preferred. I think... It was discussed, but I never got an 
updated patch to switch.

>  arch/arm64/boot/dts/broadcom/Makefile         |   4 +
>  .../arm64/boot/dts/broadcom/overlays/Makefile |   3 +
>  .../overlays/bcm2711-rpi-7-inches-ts.dts      |   2 +
>  6 files changed, 141 insertions(+)
>  create mode 100644 arch/arm/boot/dts/overlays/Makefile
>  create mode 100644 arch/arm/boot/dts/overlays/bcm2711-rpi-7-inches-ts.dts

A global (to arm) 'overlays' directory will create the same mess that we 
have in arch/arm/boot/dts/. IMO, first you should move all the Broadcom 
dts files to a 'broadcom' subdirectory like we have for arm64.

>  create mode 100644 arch/arm64/boot/dts/broadcom/overlays/Makefile
>  create mode 100644 arch/arm64/boot/dts/broadcom/overlays/bcm2711-rpi-7-inches-ts.dts
> 
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index 235ad559acb2..eb0b0b121947 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -1549,3 +1549,7 @@ dtb-$(CONFIG_ARCH_ASPEED) += \
>  	aspeed-bmc-vegman-n110.dtb \
>  	aspeed-bmc-vegman-rx20.dtb \
>  	aspeed-bmc-vegman-sx20.dtb
> +
> +ifeq ($(CONFIG_OF_OVERLAY),y)
> +subdir-y	+= overlays

I don't think this should depend on the config. If it does, you can do 
this in scripts/Makefile.lib by removing .dtbo targets. Or this could 
have been just:

subdir-$(CONFIG_OF_OVERLAY) += overlays

But I prefer the former so each platform is not picking their own way.

> +endif
> diff --git a/arch/arm/boot/dts/overlays/Makefile b/arch/arm/boot/dts/overlays/Makefile
> new file mode 100644
> index 000000000000..c90883dfaf91
> --- /dev/null
> +++ b/arch/arm/boot/dts/overlays/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +dtb-$(CONFIG_ARCH_BCM2835) += bcm2711-rpi-7-inches-ts.dtbo

The overlays should be applied to the base dtb(s) to ensure they 
actually apply and so we can validate them with schema. kbuild supports 
this already.


> diff --git a/arch/arm/boot/dts/overlays/bcm2711-rpi-7-inches-ts.dts 
> b/arch/arm/boot/dts/overlays/bcm2711-rpi-7-inches-ts.dts
> new file mode 100644
> index 000000000000..de98a6c1079a
> --- /dev/null
> +++ b/arch/arm/boot/dts/overlays/bcm2711-rpi-7-inches-ts.dts
> @@ -0,0 +1,125 @@
> +// SPDX-License-Identifier: GPL-2.0

No one uses RPi with *BSD? Dual licensing is preferred. Of course, what 
this applies to should have similar licensing.

> +
> +/dts-v1/;
> +/plugin/;
> +
> +&{/} {
> +	#address-cells = <2>;
> +	#size-cells = <1>;
> +
> +	panel_disp1: panel@0 {

What is '0' representing?

> +		reg = <0 0 0>;
> +		compatible = "raspberrypi,7inch-dsi", "simple-panel";
> +		backlight = <&reg_display>;
> +		power-supply = <&reg_display>;
> +		status = "okay";

That's the default. Same thing in a few other spots.

> +
> +		port {
> +			panel_in: endpoint {
> +				remote-endpoint = <&bridge_out>;
> +			};
> +		};
> +	};
> +
> +	reg_bridge: regulator@0 {

Oops! 2 different things at the same address!

> +		reg = <0 0 0>;

'regulator-fixed' doesn't have an address.

> +		compatible = "regulator-fixed";
> +		regulator-name = "bridge_reg";
> +		gpio = <&reg_display 0 0>;
> +		vin-supply = <&reg_display>;
> +		enable-active-high;
> +		status = "okay";
> +	};
> +};
> +
> +&i2c_csi_dsi {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	ft5406: touchscreen@38 {
> +		compatible = "edt,edt-ft5506";
> +		reg = <0x38>;
> +		status = "okay";
> +
> +		vcc-supply = <&reg_display>;
> +		reset-gpio = <&reg_display 1 1>;
> +
> +		touchscreen-size-x = < 800 >;
> +		touchscreen-size-y = < 480 >;
> +
> +		touchscreen-inverted-x;
> +		touchscreen-inverted-y;
> +	};
> +
> +	reg_display: regulator@45 {
> +		compatible = "raspberrypi,7inch-touchscreen-panel-regulator";
> +		reg = <0x45>;
> +		status = "okay";
> +
> +		gpio-controller;
> +		#gpio-cells = <2>;

The regulator is a gpio-controller?

> +	};
> +
> +};
> +
> +&dsi1 {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	status = "okay";
> +
> +	port {
> +		dsi_out: endpoint {
> +			remote-endpoint = <&bridge_in>;
> +		};
> +	};
> +
> +	bridge@0 {

> +		#address-cells = <1>;
> +		#size-cells = <0>;

Not valid here. But I shouldn't have to tell you this as the schema 
checks will. Please run them. Applied to a base should work for sure. As 
just a .dtbo, there's probably some issues, but complete nodes like this 
should validate fine.

> +
> +		reg = <0>;
> +		compatible = "toshiba,tc358762";
> +
> +		vddc-supply = <&reg_bridge>;
> +
> +		ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			port@0 {
> +				reg = <0>;
> +				bridge_in: endpoint {
> +					remote-endpoint = <&dsi_out>;
> +				};
> +			};
> +
> +			port@1 {
> +				reg = <1>;
> +				bridge_out: endpoint {
> +					remote-endpoint = <&panel_in>;
> +				};
> +			};
> +		};
> +	};
> +};
> +
> +&pixelvalve0 {
> +	status = "okay";
> +};
> +
> +&pixelvalve1 {
> +	status = "okay";
> +};
> +
> +&pixelvalve2 {
> +	status = "okay";
> +};
> +
> +&pixelvalve3 {
> +	status = "okay";
> +};
> +
> +&pixelvalve4 {
> +	status = "okay";
> +};