diff mbox series

[v21,4/4] ARM: dts: imx7d-remarkable2: Enable lcdif

Message ID 20220525115554.430971-5-alistair@alistair23.me (mailing list archive)
State New, archived
Headers show
Series Add support for the silergy,sy7636a | expand

Commit Message

Alistair Francis May 25, 2022, 11:55 a.m. UTC 
Connect the dispaly on the reMarkable2.

Signed-off-by: Alistair Francis <alistair@alistair23.me>
---
 arch/arm/boot/dts/imx7d-remarkable2.dts | 74 +++++++++++++++++++++++++
 1 file changed, 74 insertions(+)

Comments

Samuel Holland May 28, 2022, 6:20 p.m. UTC | #1 
Hi Alistair,

On 5/25/22 6:55 AM, Alistair Francis wrote:
> Connect the dispaly on the reMarkable2.
> 
> Signed-off-by: Alistair Francis <alistair@alistair23.me>
> ---
>  arch/arm/boot/dts/imx7d-remarkable2.dts | 74 +++++++++++++++++++++++++
>  1 file changed, 74 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/imx7d-remarkable2.dts b/arch/arm/boot/dts/imx7d-remarkable2.dts
> index 99ac0d242936..03a4029e1e57 100644
> --- a/arch/arm/boot/dts/imx7d-remarkable2.dts
> +++ b/arch/arm/boot/dts/imx7d-remarkable2.dts
> @@ -68,6 +68,16 @@ reg_digitizer: regulator-digitizer {
>  		startup-delay-us = <100000>; /* 100 ms */
>  	};
>  
> +	reg_sdoe: regulator-sdoe {
> +		compatible = "regulator-fixed";
> +		regulator-name = "SDOE";
> +		pinctrl-names = "default", "sleep";
> +		pinctrl-0 = <&pinctrl_sdoe_reg>;
> +		pinctrl-1 = <&pinctrl_sdoe_reg>;
> +		gpio = <&gpio3 27 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +	};
> +
>  	wifi_pwrseq: wifi_pwrseq {
>  		compatible = "mmc-pwrseq-simple";
>  		pinctrl-names = "default";
> @@ -76,6 +86,16 @@ wifi_pwrseq: wifi_pwrseq {
>  		clocks = <&clks IMX7D_CLKO2_ROOT_DIV>;
>  		clock-names = "ext_clock";
>  	};
> +
> +	panel {
> +		compatible = "eink,vb3300-kca";
> +
> +		port {
> +			panel_in: endpoint {
> +				remote-endpoint = <&display_out>;
> +			};
> +		};
> +	};

From the discussion at [1], this is not safe to merge. It exposes an
electrophoretic display to fbcon/userspace as if it was an LCD, which it very
much is not. Trying to write RGB pixel data to the panel could damage it.

So at the very least before hooking this up, the LCD controller has to know that
the EPD needs special handling and that it cannot accept RGB.

That doesn't necessarily mean there is a problem with the content of this patch
-- the special handling may all be taken care of based on the compatible string
-- but I think it's a really bad idea to merge this with how "eink,vb3300-kca"
is currently represented in panel-simple.

Regards,
Samuel

[1]: https://lore.kernel.org/lkml/Yo5kz%2F9cSd6ewC5f@phenom.ffwll.local/

>  };
>  
>  &clks {
> @@ -132,6 +152,20 @@ reg_epdpmic: vcom {
>  	};
>  };
>  
> +&lcdif {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_lcdif>;
> +	lcd-supply = <&reg_epdpmic>;
> +	lcd2-supply = <&reg_sdoe>;
> +	status = "okay";
> +
> +	port {
> +		display_out: endpoint {
> +			remote-endpoint = <&panel_in>;
> +		};
> +	};
> +};
> +
>  &snvs_pwrkey {
>  	status = "okay";
>  };
> @@ -246,6 +280,46 @@ MX7D_PAD_I2C4_SCL__I2C4_SCL		0x4000007f
>  		>;
>  	};
>  
> +	pinctrl_lcdif: lcdifgrp {
> +		fsl,pins = <
> +			MX7D_PAD_LCD_DATA00__LCD_DATA0		0x79
> +			MX7D_PAD_LCD_DATA01__LCD_DATA1		0x79
> +			MX7D_PAD_LCD_DATA02__LCD_DATA2		0x79
> +			MX7D_PAD_LCD_DATA03__LCD_DATA3		0x79
> +			MX7D_PAD_LCD_DATA04__LCD_DATA4		0x79
> +			MX7D_PAD_LCD_DATA05__LCD_DATA5		0x79
> +			MX7D_PAD_LCD_DATA06__LCD_DATA6		0x79
> +			MX7D_PAD_LCD_DATA07__LCD_DATA7		0x79
> +			MX7D_PAD_LCD_DATA08__LCD_DATA8		0x79
> +			MX7D_PAD_LCD_DATA09__LCD_DATA9		0x79
> +			MX7D_PAD_LCD_DATA10__LCD_DATA10		0x79
> +			MX7D_PAD_LCD_DATA11__LCD_DATA11		0x79
> +			MX7D_PAD_LCD_DATA12__LCD_DATA12		0x79
> +			MX7D_PAD_LCD_DATA13__LCD_DATA13		0x79
> +			MX7D_PAD_LCD_DATA14__LCD_DATA14		0x79
> +			MX7D_PAD_LCD_DATA15__LCD_DATA15		0x79
> +
> +			MX7D_PAD_LCD_DATA17__LCD_DATA17		0x79
> +			MX7D_PAD_LCD_DATA18__LCD_DATA18		0x79
> +			MX7D_PAD_LCD_DATA19__LCD_DATA19		0x79
> +			MX7D_PAD_LCD_DATA20__LCD_DATA20		0x79
> +			MX7D_PAD_LCD_DATA21__LCD_DATA21		0x79
> +
> +			MX7D_PAD_LCD_DATA23__LCD_DATA23		0x79
> +			MX7D_PAD_LCD_CLK__LCD_CLK		0x79
> +			MX7D_PAD_LCD_ENABLE__LCD_ENABLE		0x79
> +			MX7D_PAD_LCD_VSYNC__LCD_VSYNC		0x79
> +			MX7D_PAD_LCD_HSYNC__LCD_HSYNC		0x79
> +			MX7D_PAD_LCD_RESET__LCD_RESET		0x79
> +		>;
> +	};
> +
> +	pinctrl_sdoe_reg: sdoereggrp {
> +		fsl,pins = <
> +			MX7D_PAD_LCD_DATA22__GPIO3_IO27		0x74
> +		>;
> +	};
> +
>  	pinctrl_uart1: uart1grp {
>  		fsl,pins = <
>  			MX7D_PAD_UART1_TX_DATA__UART1_DCE_TX	0x79
>
Alistair Francis Aug. 2, 2022, 12:43 p.m. UTC | #2 
On Sun, May 29, 2022 at 4:20 AM Samuel Holland <samuel@sholland.org> wrote:
>
> Hi Alistair,
>
> On 5/25/22 6:55 AM, Alistair Francis wrote:
> > Connect the dispaly on the reMarkable2.
> >
> > Signed-off-by: Alistair Francis <alistair@alistair23.me>
> > ---
> >  arch/arm/boot/dts/imx7d-remarkable2.dts | 74 +++++++++++++++++++++++++
> >  1 file changed, 74 insertions(+)
> >
> > diff --git a/arch/arm/boot/dts/imx7d-remarkable2.dts b/arch/arm/boot/dts/imx7d-remarkable2.dts
> > index 99ac0d242936..03a4029e1e57 100644
> > --- a/arch/arm/boot/dts/imx7d-remarkable2.dts
> > +++ b/arch/arm/boot/dts/imx7d-remarkable2.dts
> > @@ -68,6 +68,16 @@ reg_digitizer: regulator-digitizer {
> >               startup-delay-us = <100000>; /* 100 ms */
> >       };
> >
> > +     reg_sdoe: regulator-sdoe {
> > +             compatible = "regulator-fixed";
> > +             regulator-name = "SDOE";
> > +             pinctrl-names = "default", "sleep";
> > +             pinctrl-0 = <&pinctrl_sdoe_reg>;
> > +             pinctrl-1 = <&pinctrl_sdoe_reg>;
> > +             gpio = <&gpio3 27 GPIO_ACTIVE_HIGH>;
> > +             enable-active-high;
> > +     };
> > +
> >       wifi_pwrseq: wifi_pwrseq {
> >               compatible = "mmc-pwrseq-simple";
> >               pinctrl-names = "default";
> > @@ -76,6 +86,16 @@ wifi_pwrseq: wifi_pwrseq {
> >               clocks = <&clks IMX7D_CLKO2_ROOT_DIV>;
> >               clock-names = "ext_clock";
> >       };
> > +
> > +     panel {
> > +             compatible = "eink,vb3300-kca";
> > +
> > +             port {
> > +                     panel_in: endpoint {
> > +                             remote-endpoint = <&display_out>;
> > +                     };
> > +             };
> > +     };
>
> From the discussion at [1], this is not safe to merge. It exposes an
> electrophoretic display to fbcon/userspace as if it was an LCD, which it very
> much is not. Trying to write RGB pixel data to the panel could damage it.

Hey Samuel,

From what I can tell it's difficult to damage the display, but I see your point.

>
> So at the very least before hooking this up, the LCD controller has to know that
> the EPD needs special handling and that it cannot accept RGB.

Looking at [1] it seems like no decision was made about how to handle
a case like this where the EPC driving is all done in software. We
currently drive it from userspace via proprietary software. It seems
unlikely we will be able to support this in the kernel, so it would be
nice to somehow expose it to userspace.

>
> That doesn't necessarily mean there is a problem with the content of this patch
> -- the special handling may all be taken care of based on the compatible string

Ah ok. So it sounds like adding a check to the LCD controller based on
compatible string to reject RGB values would be a good start here.
That would at least block bogus values from making it to the screen

Alistair

> -- but I think it's a really bad idea to merge this with how "eink,vb3300-kca"
> is currently represented in panel-simple.
>
> Regards,
> Samuel
>
> [1]: https://lore.kernel.org/lkml/Yo5kz%2F9cSd6ewC5f@phenom.ffwll.local/
>
> >  };
> >
> >  &clks {
> > @@ -132,6 +152,20 @@ reg_epdpmic: vcom {
> >       };
> >  };
> >
> > +&lcdif {
> > +     pinctrl-names = "default";
> > +     pinctrl-0 = <&pinctrl_lcdif>;
> > +     lcd-supply = <&reg_epdpmic>;
> > +     lcd2-supply = <&reg_sdoe>;
> > +     status = "okay";
> > +
> > +     port {
> > +             display_out: endpoint {
> > +                     remote-endpoint = <&panel_in>;
> > +             };
> > +     };
> > +};
> > +
> >  &snvs_pwrkey {
> >       status = "okay";
> >  };
> > @@ -246,6 +280,46 @@ MX7D_PAD_I2C4_SCL__I2C4_SCL              0x4000007f
> >               >;
> >       };
> >
> > +     pinctrl_lcdif: lcdifgrp {
> > +             fsl,pins = <
> > +                     MX7D_PAD_LCD_DATA00__LCD_DATA0          0x79
> > +                     MX7D_PAD_LCD_DATA01__LCD_DATA1          0x79
> > +                     MX7D_PAD_LCD_DATA02__LCD_DATA2          0x79
> > +                     MX7D_PAD_LCD_DATA03__LCD_DATA3          0x79
> > +                     MX7D_PAD_LCD_DATA04__LCD_DATA4          0x79
> > +                     MX7D_PAD_LCD_DATA05__LCD_DATA5          0x79
> > +                     MX7D_PAD_LCD_DATA06__LCD_DATA6          0x79
> > +                     MX7D_PAD_LCD_DATA07__LCD_DATA7          0x79
> > +                     MX7D_PAD_LCD_DATA08__LCD_DATA8          0x79
> > +                     MX7D_PAD_LCD_DATA09__LCD_DATA9          0x79
> > +                     MX7D_PAD_LCD_DATA10__LCD_DATA10         0x79
> > +                     MX7D_PAD_LCD_DATA11__LCD_DATA11         0x79
> > +                     MX7D_PAD_LCD_DATA12__LCD_DATA12         0x79
> > +                     MX7D_PAD_LCD_DATA13__LCD_DATA13         0x79
> > +                     MX7D_PAD_LCD_DATA14__LCD_DATA14         0x79
> > +                     MX7D_PAD_LCD_DATA15__LCD_DATA15         0x79
> > +
> > +                     MX7D_PAD_LCD_DATA17__LCD_DATA17         0x79
> > +                     MX7D_PAD_LCD_DATA18__LCD_DATA18         0x79
> > +                     MX7D_PAD_LCD_DATA19__LCD_DATA19         0x79
> > +                     MX7D_PAD_LCD_DATA20__LCD_DATA20         0x79
> > +                     MX7D_PAD_LCD_DATA21__LCD_DATA21         0x79
> > +
> > +                     MX7D_PAD_LCD_DATA23__LCD_DATA23         0x79
> > +                     MX7D_PAD_LCD_CLK__LCD_CLK               0x79
> > +                     MX7D_PAD_LCD_ENABLE__LCD_ENABLE         0x79
> > +                     MX7D_PAD_LCD_VSYNC__LCD_VSYNC           0x79
> > +                     MX7D_PAD_LCD_HSYNC__LCD_HSYNC           0x79
> > +                     MX7D_PAD_LCD_RESET__LCD_RESET           0x79
> > +             >;
> > +     };
> > +
> > +     pinctrl_sdoe_reg: sdoereggrp {
> > +             fsl,pins = <
> > +                     MX7D_PAD_LCD_DATA22__GPIO3_IO27         0x74
> > +             >;
> > +     };
> > +
> >       pinctrl_uart1: uart1grp {
> >               fsl,pins = <
> >                       MX7D_PAD_UART1_TX_DATA__UART1_DCE_TX    0x79
> >
>
diff mbox series

Patch

diff --git a/arch/arm/boot/dts/imx7d-remarkable2.dts b/arch/arm/boot/dts/imx7d-remarkable2.dts
index 99ac0d242936..03a4029e1e57 100644
--- a/arch/arm/boot/dts/imx7d-remarkable2.dts
+++ b/arch/arm/boot/dts/imx7d-remarkable2.dts
@@ -68,6 +68,16 @@  reg_digitizer: regulator-digitizer {
 		startup-delay-us = <100000>; /* 100 ms */
 	};
 
+	reg_sdoe: regulator-sdoe {
+		compatible = "regulator-fixed";
+		regulator-name = "SDOE";
+		pinctrl-names = "default", "sleep";
+		pinctrl-0 = <&pinctrl_sdoe_reg>;
+		pinctrl-1 = <&pinctrl_sdoe_reg>;
+		gpio = <&gpio3 27 GPIO_ACTIVE_HIGH>;
+		enable-active-high;
+	};
+
 	wifi_pwrseq: wifi_pwrseq {
 		compatible = "mmc-pwrseq-simple";
 		pinctrl-names = "default";
@@ -76,6 +86,16 @@  wifi_pwrseq: wifi_pwrseq {
 		clocks = <&clks IMX7D_CLKO2_ROOT_DIV>;
 		clock-names = "ext_clock";
 	};
+
+	panel {
+		compatible = "eink,vb3300-kca";
+
+		port {
+			panel_in: endpoint {
+				remote-endpoint = <&display_out>;
+			};
+		};
+	};
 };
 
 &clks {
@@ -132,6 +152,20 @@  reg_epdpmic: vcom {
 	};
 };
 
+&lcdif {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_lcdif>;
+	lcd-supply = <&reg_epdpmic>;
+	lcd2-supply = <&reg_sdoe>;
+	status = "okay";
+
+	port {
+		display_out: endpoint {
+			remote-endpoint = <&panel_in>;
+		};
+	};
+};
+
 &snvs_pwrkey {
 	status = "okay";
 };
@@ -246,6 +280,46 @@  MX7D_PAD_I2C4_SCL__I2C4_SCL		0x4000007f
 		>;
 	};
 
+	pinctrl_lcdif: lcdifgrp {
+		fsl,pins = <
+			MX7D_PAD_LCD_DATA00__LCD_DATA0		0x79
+			MX7D_PAD_LCD_DATA01__LCD_DATA1		0x79
+			MX7D_PAD_LCD_DATA02__LCD_DATA2		0x79
+			MX7D_PAD_LCD_DATA03__LCD_DATA3		0x79
+			MX7D_PAD_LCD_DATA04__LCD_DATA4		0x79
+			MX7D_PAD_LCD_DATA05__LCD_DATA5		0x79
+			MX7D_PAD_LCD_DATA06__LCD_DATA6		0x79
+			MX7D_PAD_LCD_DATA07__LCD_DATA7		0x79
+			MX7D_PAD_LCD_DATA08__LCD_DATA8		0x79
+			MX7D_PAD_LCD_DATA09__LCD_DATA9		0x79
+			MX7D_PAD_LCD_DATA10__LCD_DATA10		0x79
+			MX7D_PAD_LCD_DATA11__LCD_DATA11		0x79
+			MX7D_PAD_LCD_DATA12__LCD_DATA12		0x79
+			MX7D_PAD_LCD_DATA13__LCD_DATA13		0x79
+			MX7D_PAD_LCD_DATA14__LCD_DATA14		0x79
+			MX7D_PAD_LCD_DATA15__LCD_DATA15		0x79
+
+			MX7D_PAD_LCD_DATA17__LCD_DATA17		0x79
+			MX7D_PAD_LCD_DATA18__LCD_DATA18		0x79
+			MX7D_PAD_LCD_DATA19__LCD_DATA19		0x79
+			MX7D_PAD_LCD_DATA20__LCD_DATA20		0x79
+			MX7D_PAD_LCD_DATA21__LCD_DATA21		0x79
+
+			MX7D_PAD_LCD_DATA23__LCD_DATA23		0x79
+			MX7D_PAD_LCD_CLK__LCD_CLK		0x79
+			MX7D_PAD_LCD_ENABLE__LCD_ENABLE		0x79
+			MX7D_PAD_LCD_VSYNC__LCD_VSYNC		0x79
+			MX7D_PAD_LCD_HSYNC__LCD_HSYNC		0x79
+			MX7D_PAD_LCD_RESET__LCD_RESET		0x79
+		>;
+	};
+
+	pinctrl_sdoe_reg: sdoereggrp {
+		fsl,pins = <
+			MX7D_PAD_LCD_DATA22__GPIO3_IO27		0x74
+		>;
+	};
+
 	pinctrl_uart1: uart1grp {
 		fsl,pins = <
 			MX7D_PAD_UART1_TX_DATA__UART1_DCE_TX	0x79