| Message ID | 20221108191543.1752080-1-andreas@kemnade.info (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] ARM: dts: imx: e60k02: Add touchscreen | expand |
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 >
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>; + }; /* 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 */
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(-)