[v3] ARM: dts: imx: e60k02: Add touchscreen

  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
 
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(-)
  

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.

Regards,
  Marco

>  
>  	/* TODO: TPS65185 PMIC for E Ink at 0x68 */
>  
> diff --git a/arch/arm/boot/dts/imx6sl-tolino-shine3.dts b/arch/arm/boot/dts/imx6sl-tolino-shine3.dts
> index e3f1e8d79528..e98dc302e2e3 100644
> --- a/arch/arm/boot/dts/imx6sl-tolino-shine3.dts
> +++ b/arch/arm/boot/dts/imx6sl-tolino-shine3.dts
> @@ -26,6 +26,11 @@ / {
>  	compatible = "kobo,tolino-shine3", "fsl,imx6sl";
>  };
>  
> +&cyttsp5 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_cyttsp5_gpio>;
> +};
> +
>  &gpio_keys {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&pinctrl_gpio_keys>;
> @@ -52,6 +57,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 */
> +		>;
> +	};
> +
>  	pinctrl_gpio_keys: gpio-keysgrp {
>  		fsl,pins = <
>  			MX6SL_PAD_SD1_DAT1__GPIO5_IO08	0x17059	/* PWR_SW */
> diff --git a/arch/arm/boot/dts/imx6sll-kobo-clarahd.dts b/arch/arm/boot/dts/imx6sll-kobo-clarahd.dts
> index 90b32f5eb529..6bb80720ea66 100644
> --- a/arch/arm/boot/dts/imx6sll-kobo-clarahd.dts
> +++ b/arch/arm/boot/dts/imx6sll-kobo-clarahd.dts
> @@ -36,6 +36,11 @@ &cpu0 {
>  	soc-supply = <&dcdc1_reg>;
>  };
>  
> +&cyttsp5 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_cyttsp5_gpio>;
> +};
> +
>  &gpio_keys {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&pinctrl_gpio_keys>;
> @@ -62,6 +67,13 @@ &iomuxc {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&pinctrl_hog>;
>  
> +	pinctrl_cyttsp5_gpio: cyttsp5-gpiogrp {
> +		fsl,pins = <
> +			MX6SLL_PAD_SD1_DATA3__GPIO5_IO06                0x17059 /* TP_INT */
> +			MX6SLL_PAD_SD1_DATA2__GPIO5_IO13                0x10059 /* TP_RST */
> +		>;
> +	};
> +
>  	pinctrl_gpio_keys: gpio-keysgrp {
>  		fsl,pins = <
>  			MX6SLL_PAD_SD1_DATA1__GPIO5_IO08	0x17059	/* PWR_SW */
> -- 
> 2.30.2
> 
> 
>
  

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
  

Hi Andreas,

On 22-11-09, Andreas Kemnade wrote:
> 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?

Sorry for the missunderstanding, I didn't mean the phandle. I mean the
mux setting which is done in the dts right? I'm just not a fan of
muxing pins in one file an using those 'assumptions' in others. Except
for platforms like the imx8mm-evk which is exactly the same hardware and
only differs in the RAM they used. But you have two different platforms
right?

Hope this clears some of the confusions.

> 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
>
  

Hi Marco,

On Fri, 11 Nov 2022 10:12:23 +0100
Marco Felsch <m.felsch@pengutronix.de> wrote:

> Hi Andreas,
> 
> On 22-11-09, Andreas Kemnade wrote:
> > 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?  
> 
> Sorry for the missunderstanding, I didn't mean the phandle. I mean the
> mux setting which is done in the dts right? I'm just not a fan of
> muxing pins in one file an using those 'assumptions' in others. Except
> for platforms like the imx8mm-evk which is exactly the same hardware and
> only differs in the RAM they used. But you have two different platforms
> right?
> 
Same board, same PCB marking, the only spotted difference is the name on the
case and the SoC (which is pin-compatible, so GPIOs will be all the same).

In the case of different hardware platforms I would understand your
ruffled feathers.

Regards,
Andreas
  

On 22-11-11, Andreas Kemnade wrote:
> Hi Marco,
> 
> On Fri, 11 Nov 2022 10:12:23 +0100
> Marco Felsch <m.felsch@pengutronix.de> wrote:
> 
> > Hi Andreas,
> > 
> > On 22-11-09, Andreas Kemnade wrote:
> > > 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?  
> > 
> > Sorry for the missunderstanding, I didn't mean the phandle. I mean the
> > mux setting which is done in the dts right? I'm just not a fan of
> > muxing pins in one file an using those 'assumptions' in others. Except
> > for platforms like the imx8mm-evk which is exactly the same hardware and
> > only differs in the RAM they used. But you have two different platforms
> > right?
> > 
> Same board, same PCB marking, the only spotted difference is the name on the
> case and the SoC (which is pin-compatible, so GPIOs will be all the same).
> 
> In the case of different hardware platforms I would understand your
> ruffled feathers.

Okay, if it is the same PCB, you're right. In that case v2 should be
sufficient. Sorry for the noise, but I didn't not assume that due to the
complete different the .dts file names.

Regards,
  Marco

> 
> Regards,
> Andreas
>