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, 9 Nov 2022 12:45:36 +0100
From:   Andreas Kemnade <andreas@kemnade.info>
To:     Marco Felsch <m.felsch@pengutronix.de>
Cc:     robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
        shawnguo@kernel.org, s.hauer@pengutronix.de, kernel@pengutronix.de,
        festevam@gmail.com, linux-imx@nxp.com, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        alistair@alistair23.me
Subject: Re: [PATCH v3] ARM: dts: imx: e60k02: Add touchscreen
Message-ID: <20221109124536.5154cb03@aktux>
In-Reply-To: <20221109092350.2ke6sbgbcp3wpelc@pengutronix.de>
References: <20221108191543.1752080-1-andreas@kemnade.info>
        <20221109092350.2ke6sbgbcp3wpelc@pengutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Wed, 9 Nov 2022 10:23:50 +0100
Marco Felsch <m.felsch@pengutronix.de> wrote:

> Hi Andreas,
> 
> On 22-11-08, Andreas Kemnade wrote:
> > Add the touchscreen now, since the driver is available.
> > 
> > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > ---
> > Changes in v3: no phandles pointing from dtsi to dts  
> 
> Thanks for this change...
> 
> > Changes in v2: fix pinmux naming
> > 
> >  arch/arm/boot/dts/e60k02.dtsi              |  9 ++++++++-
> >  arch/arm/boot/dts/imx6sl-tolino-shine3.dts | 12 ++++++++++++
> >  arch/arm/boot/dts/imx6sll-kobo-clarahd.dts | 12 ++++++++++++
> >  3 files changed, 32 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm/boot/dts/e60k02.dtsi
> > b/arch/arm/boot/dts/e60k02.dtsi index 935e2359f8df..99091db3ab2a
> > 100644 --- a/arch/arm/boot/dts/e60k02.dtsi
> > +++ b/arch/arm/boot/dts/e60k02.dtsi
> > @@ -104,7 +104,14 @@ &i2c2 {
> >  	clock-frequency = <100000>;
> >  	status = "okay";
> >  
> > -	/* TODO: CYTTSP5 touch controller at 0x24 */
> > +	cyttsp5: touchscreen@24 {
> > +		compatible = "cypress,tt21000";
> > +		reg = <0x24>;
> > +		interrupt-parent = <&gpio5>;
> > +		interrupts = <6 IRQ_TYPE_EDGE_FALLING>;
> > +		reset-gpios = <&gpio5 13 GPIO_ACTIVE_LOW>;
> > +		vdd-supply = <&ldo5_reg>;
> > +	};  
> 
> but we still have a cross-reference to the .dtsi file here. Therefore
> I said to move the interrupt/reset-gpio into the dts file too. I know
> this is a kind of a nitpick but I really don't like such
> cross-references.
> 
hmm. &gpio5 references to imx6sl[l].dtsi, not dts, so what is the
problem here?
And we have this pattern all over the place.

What is different to the touchscreen that this pattern is not wanted
here but
accepted everywhere else? It is there for
  - backlight
  - irq of pmic
  - reset/gpio-regulator of wifi
  - leds
  - keys

And you have also done some review work there.

Here I am caring a bit about readability since I have still to do
maintenance work on this file, so I am a bit more concerned than that
it a) just works and b) is being accepted upstream.

If it is not allowed to have common things in the e60k02.dtsi file, what
about ditching that file alltogether and just have the two .dts files?

I personally prefer the v2 variant, but v3 is a compromise.

For comparison, the feature-complete version used by postmarketOS is
here:
https://github.com/akemnade/linux/blob/kobo/drm-merged-5.19/arch/arm/boot/dts/e60k02.dtsi

Regards,
Andreas