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:   Mon, 7 Nov 2022 11:47:23 +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 v2] ARM: dts: imx: e60k02: Add touchscreen
Message-ID: <20221107114723.37e86b0e@aktux>
In-Reply-To: <20221107081234.wxup6lq3xph2obfv@pengutronix.de>
References: <20221104204251.1458958-1-andreas@kemnade.info>
        <20221107081234.wxup6lq3xph2obfv@pengutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi,

On Mon, 7 Nov 2022 09:12:34 +0100
Marco Felsch <m.felsch@pengutronix.de> wrote:

> Hi Andreas,
> 
[...]
> > diff --git a/arch/arm/boot/dts/imx6sl-tolino-shine3.dts b/arch/arm/boot/dts/imx6sl-tolino-shine3.dts
> > index e3f1e8d79528..db5d8509935f 100644
> > --- a/arch/arm/boot/dts/imx6sl-tolino-shine3.dts
> > +++ b/arch/arm/boot/dts/imx6sl-tolino-shine3.dts
> > @@ -52,6 +52,13 @@ &iomuxc {
> >  	pinctrl-names = "default";
> >  	pinctrl-0 = <&pinctrl_hog>;
> >  
> > +	pinctrl_cyttsp5_gpio: cyttsp5-gpiogrp {
> > +		fsl,pins = <
> > +			MX6SL_PAD_SD1_DAT3__GPIO5_IO06                0x17059 /* TP_INT */
> > +			MX6SL_PAD_SD1_DAT2__GPIO5_IO13                0x10059 /* TP_RST */
> > +		>;
> > +	};  
> 
> Hm.. I don't like that you enable it within the base dtsi and add the
> missing dependencies here. I would rather add the touchscreen within the
> base dtsi but with with status set to disabled and without specifying
> the interrupt and reset gpio. Within the dts and the below you can
> reference the touchscreen via phandle and add the missing properties.
> 
hmm, that makes things more complex and unreadable. Remember, we have two boards
differ in a pin-compatible soc here. Why I should set something present
in all cases to disabled?
And why I should add hardware just to set to disabled?

I notice that I have not followed the agreed pattern for this things like in
the lm3630 setting, there we have


       pinctrl_lm3630a_bl_gpio: lm3630a-bl-gpiogrp {
                fsl,pins = <
                        MX6SLL_PAD_EPDC_PWR_CTRL3__GPIO2_IO10   0x10059 /* HWEN */
                >;
        };
[...]
&lm3630a {
        pinctrl-names = "default";
        pinctrl-0 = <&pinctrl_lm3630a_bl_gpio>;
};

in the dts file, so there is no phandle pointing from dtsi to dts.
I will send an updated version following this widely adopted style.

Regards,
Andreas