Re: [RFC PATCH v2 3/3] ARM: dto: Add bcm2711-rpi-7-inches-ts.dts overlay

[Rob Herring <robh@kernel.org>]

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";
> +};