Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
From:   Detlev Casanova <detlev.casanova@collabora.com>
To:     linux-kernel@vger.kernel.org,
        Stefan Wahren <stefan.wahren@i2se.com>
Cc:     Arnd Bergmann <arnd@arndb.de>, Olof Johansson <olof@lixom.net>,
        "maintainer:ARM AND ARM64 SoC SUB-ARCHITECTURES (COMMON PARTS)" 
        <soc@kernel.org>, Rob Herring <robh+dt@kernel.org>,
        Nicolas Saenz Julienne <nsaenz@kernel.org>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Ray Jui <rjui@broadcom.com>,
        Scott Branden <sbranden@broadcom.com>,
        "maintainer:BROADCOM BCM281XX/BCM11XXX/BCM216XX ARM ARCHITE..." 
        <bcm-kernel-feedback-list@broadcom.com>,
        "moderated list:ARM AND ARM64 SoC SUB-ARCHITECTURES (COMMON PARTS)" 
        <linux-arm-kernel@lists.infradead.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>,
        "moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" 
        <linux-rpi-kernel@lists.infradead.org>
Subject: Re: [PATCH 2/2] ARM: dts: Add bcm2711-rpi-4-b-7inch-ts-dsi.dts
Date:   Wed, 09 Feb 2022 14:44:08 -0500
Message-ID: <2136213.irdbgypaU6@falcon9>
In-Reply-To: <d00a934e-a2e0-fd3a-6452-fa88f07e13d4@i2se.com>
References: <20220209162515.706729-1-detlev.casanova@collabora.com> <20220209162515.706729-3-detlev.casanova@collabora.com> <d00a934e-a2e0-fd3a-6452-fa88f07e13d4@i2se.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

Hi Stefan,

On Wednesday, February 9, 2022 12:10:12 P.M. EST Stefan Wahren wrote:
> Hi Detlev,
> 
> Am 09.02.22 um 17:25 schrieb Detlev Casanova:
> > Add a device tree to support the official Raspberrypi 7" touchscreen for
> > the Raspberry Pi 4 B
> > 
> > The panel is connected on the DSI 1 port and uses the simple-panel
> > driver.
> > 
> > The device tree also makes sure to activate:
> >  * dvp: bcm2711 clock driver
> >  * hvs: Hardware Video Scaler
> >  * pixelvalve[0-4]: CRTC modules
> >  * txp: CRTC Writeback
> > 
> > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> > ---
> > 
> >  arch/arm/boot/dts/Makefile                    |   1 +
> >  .../boot/dts/bcm2711-rpi-4-b-7inch-ts-dsi.dts | 129 ++++++++++++++++++
> >  arch/arm64/boot/dts/broadcom/Makefile         |   1 +
> >  .../broadcom/bcm2711-rpi-4-b-7inch-ts-dsi.dts |   2 +
> >  4 files changed, 133 insertions(+)
> >  create mode 100644 arch/arm/boot/dts/bcm2711-rpi-4-b-7inch-ts-dsi.dts
> >  create mode 100644
> >  arch/arm64/boot/dts/broadcom/bcm2711-rpi-4-b-7inch-ts-dsi.dts> 
> > diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> > index 0de64f237cd8..b46daf2df4ce 100644
> > --- a/arch/arm/boot/dts/Makefile
> > +++ b/arch/arm/boot/dts/Makefile
> > @@ -94,6 +94,7 @@ dtb-$(CONFIG_ARCH_BCM2835) += \
> > 
> >  	bcm2837-rpi-cm3-io3.dtb \
> >  	bcm2711-rpi-400.dtb \
> >  	bcm2711-rpi-4-b.dtb \
> > 
> > +	bcm2711-rpi-4-b-7inch-ts-dsi.dtb \
> > 
> >  	bcm2711-rpi-cm4-io.dtb \
> >  	bcm2835-rpi-zero.dtb \
> >  	bcm2835-rpi-zero-w.dtb
> > 
> > diff --git a/arch/arm/boot/dts/bcm2711-rpi-4-b-7inch-ts-dsi.dts
> > b/arch/arm/boot/dts/bcm2711-rpi-4-b-7inch-ts-dsi.dts new file mode 100644
> > index 000000000000..62e986358c2a
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/bcm2711-rpi-4-b-7inch-ts-dsi.dts
> 
> i think this should be an overlay because this board can be connected to
> different boards and we want to avoid copy & paste. Unfortunately i
> don't know where this should be stored in the kernel tree.

Yes, that is how it started but the upstream kernel doesn't use them. We 
thought that a separate device-tree makes sense as it is the official raspberry 
pi touchscreen. Do you know if there are plans for supporting device tree 
overlays in the upstream kernel ?

> > @@ -0,0 +1,129 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include "bcm2711-rpi-4-b.dts"
> > +
> > +/ {
> > +	model = "Raspberry Pi 4 Model B + Rpi 7inch touchscreen";
> > +
> > +	panel_disp1: panel_disp1@0 {
> > +		reg = <0 0 0>;
> > +		compatible = "raspberrypi,7inch-dsi", "simple-panel";
> > +		backlight = <&reg_display>;
> > +		power-supply = <&reg_display>;
> > +		status = "okay";
> > +
> > +		port {
> > +			panel_in: endpoint {
> > +				remote-endpoint = <&bridge_out>;
> > +			};
> > +		};
> > +	};
> > +
> > +	reg_bridge: reg_bridge@0 {
> > +		reg = <0 0 0>;
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "bridge_reg";
> > +		gpio = <&reg_display 0 0>;
> > +		vin-supply = <&reg_display>;
> > +		enable-active-high;
> > +		status = "okay";
> > +	};
> > +};
> > +
> > +&i2c_csi_dsi {
> > +	ft5406: ts@38 {
> 
> s/ts/touchscreen/
> 
> > +		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: reg_display@45 {
> 
> node name should be regulator

Will change it to `reg_display: regulator@45`.

> > +		compatible = "raspberrypi,7inch-touchscreen-panel-
regulator";
> > +		reg = <0x45>;
> > +		gpio-controller;
> > +		#gpio-cells = <2>;
> > +		status = "okay";
> > +	};
> > +
> > +};
> > +
> > +&dsi1 {
> > +	#address-cells = <1>;
> > +	#size-cells = <0>;
> > +	status = "okay";
> > +
> > +	port {
> > +		dsi_out: endpoint {
> > +			remote-endpoint = <&bridge_in>;
> > +		};
> > +	};
> > +
> > +	bridge@0 {
> > +		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>;
> > +				};
> > +			};
> > +		};
> > +	};
> > +};
> > +
> > +&aon_intr {
> > +	status = "okay";
> > +};
> > +
> > +&dvp {
> > +	status = "okay";
> > +};
> > +
> > +&hvs {
> > +	status = "okay";
> > +};
> 
> From my understanding these three are not necessary

Indeed, they are already enabled.

> > +
> > +&pixelvalve0 {
> > +	status = "okay";
> > +};
> > +
> > +&pixelvalve1 {
> > +	status = "okay";
> > +};
> > +
> > +&pixelvalve2 {
> > +	status = "okay";
> > +};
> > +
> > +&pixelvalve3 {
> > +	status = "okay";
> > +};
> > +
> > +&pixelvalve4 {
> > +	status = "okay";
> > +};
> > +
> > +&txp {
> > +	status = "okay";
> > +};
> 
> Also txp doesn't need to be enabled explicit.

Yes, already active. This was more to make sure they were enabled when porting 
to other devices, but that probably doesn't make much sense for them to be 
disabled.

> > diff --git a/arch/arm64/boot/dts/broadcom/Makefile
> > b/arch/arm64/boot/dts/broadcom/Makefile index c6882032a428..965361bff829
> > 100644
> > --- a/arch/arm64/boot/dts/broadcom/Makefile
> > +++ b/arch/arm64/boot/dts/broadcom/Makefile
> > @@ -1,6 +1,7 @@
> > 
> >  # SPDX-License-Identifier: GPL-2.0
> >  dtb-$(CONFIG_ARCH_BCM2835) += bcm2711-rpi-400.dtb \
> >  
> >  			      bcm2711-rpi-4-b.dtb \
> > 
> > +			      bcm2711-rpi-4-b-7inch-ts-dsi.dtb \
> > 
> >  			      bcm2711-rpi-cm4-io.dtb \
> >  			      bcm2837-rpi-3-a-plus.dtb \
> >  			      bcm2837-rpi-3-b.dtb \
> > 
> > diff --git a/arch/arm64/boot/dts/broadcom/bcm2711-rpi-4-b-7inch-ts-dsi.dts
> > b/arch/arm64/boot/dts/broadcom/bcm2711-rpi-4-b-7inch-ts-dsi.dts new file
> > mode 100644
> > index 000000000000..c325adc4f874
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/broadcom/bcm2711-rpi-4-b-7inch-ts-dsi.dts
> > @@ -0,0 +1,2 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include "arm/bcm2711-rpi-4-b-7inch-ts-dsi.dts"

Thanks for the fast review,

Detlev.