Properly support FriendlyElec HD702E

The "E" alludes to the fact that FriendlyElec's HD702E is actually an eDP panel - move its compatible to the appropriate binding doc.

Cc <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---
 .../devicetree/bindings/display/panel/panel-edp-legacy.yaml     | 2 ++
 .../devicetree/bindings/display/panel/panel-simple.yaml         | 2 --
 2 files changed, 2 insertions(+), 2 deletions(-)

FriendlyELEC's HD702E module is an eDP panel (in as much as it's some LVDS LCD behind a Chrontel CH7511B eDP bridge), so move its data over to the eDP driver, also resolving the warning about the missing bpc value in the process.

Signed-off-by Robin Murphy <[email protected]>

---
  drivers/gpu/drm/panel/panel-edp.c    | 26 ++++++++++++++++++++++++++
  drivers/gpu/drm/panel/panel-simple.c | 25 -------------------------
  2 files changed, 26 insertions(+), 25 deletions(-)

drivers/gpu/drm/panel/panel-edp.c +26 -0

index 415b894890ad..dd53ccc209ce 100644
@@ -1256,6 +1256,29 @@ static const struct panel_desc boe_nv140fhmn49 = {
  	},
  };
 
+static const struct drm_display_mode friendlyarm_hd702e_mode = {
+	.clock		= 67185,
+	.hdisplay	= 800,
+	.hsync_start	= 800 + 20,
+	.hsync_end	= 800 + 20 + 24,
+	.htotal		= 800 + 20 + 24 + 20,
+	.vdisplay	= 1280,
+	.vsync_start	= 1280 + 4,
+	.vsync_end	= 1280 + 4 + 8,
+	.vtotal		= 1280 + 4 + 8 + 4,
+	.flags		= DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC,
+};
+
+static const struct panel_desc friendlyarm_hd702e = {
+	.modes = &friendlyarm_hd702e_mode,
+	.num_modes = 1,
+	.bpc = 8,
+	.size = {
+		.width	= 94,
+		.height	= 151,
+	},
+};
+
  static const struct drm_display_mode innolux_n116bca_ea1_mode = {
  	.clock = 76420,
  	.hdisplay = 1366,

@@ -1663,6 +1686,9 @@ static const struct of_device_id platform_of_match[] = {
  	}, {
  		.compatible = "boe,nv140fhmn49",
  		.data = &boe_nv140fhmn49,
+	}, {
+		.compatible = "friendlyarm,hd702e",
+		.data = &friendlyarm_hd702e,
  	}, {
  		.compatible = "innolux,n116bca-ea1",
  		.data = &innolux_n116bca_ea1,

drivers/gpu/drm/panel/panel-simple.c +0 -22

index b26b682826bc..3ea52667b858 100644
@@ -2359,28 +2359,6 @@ static const struct panel_desc frida_frd350h54004 = {
  	.connector_type = DRM_MODE_CONNECTOR_DPI,
  };
 
-static const struct drm_display_mode friendlyarm_hd702e_mode = {
-	.clock		= 67185,
-	.hdisplay	= 800,
-	.hsync_start	= 800 + 20,
-	.hsync_end	= 800 + 20 + 24,
-	.htotal		= 800 + 20 + 24 + 20,
-	.vdisplay	= 1280,
-	.vsync_start	= 1280 + 4,
-	.vsync_end	= 1280 + 4 + 8,
-	.vtotal		= 1280 + 4 + 8 + 4,
-	.flags		= DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC,
-};
-
-static const struct panel_desc friendlyarm_hd702e = {
-	.modes = &friendlyarm_hd702e_mode,
-	.num_modes = 1,
-	.size = {
-		.width	= 94,
-		.height	= 151,
-	},
-};
-
  static const struct drm_display_mode giantplus_gpg482739qs5_mode = {
  	.clock = 9000,
  	.hdisplay = 480,

FriendlyELEC's HD702E module is an eDP panel (in as much as it's some LVDS LCD behind a Chrontel CH7511B eDP bridge), so move its data over to the eDP driver, also resolving the warning about the missing bpc value in the process.

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/gpu/drm/panel/panel-edp.c    | 26 ++++++++++++++++++++++++++
 drivers/gpu/drm/panel/panel-simple.c | 25 -------------------------
 2 files changed, 26 insertions(+), 25 deletions(-)

drivers/gpu/drm/panel/panel-edp.c +23 -0

index 415b894890ad..dd53ccc209ce 100644
@@ -1256,6 +1256,29 @@ static const struct panel_desc boe_nv140fhmn49 = {
        },
 };
 
+static const struct drm_display_mode friendlyarm_hd702e_mode = {
+       .clock          = 67185,
+       .hdisplay       = 800,
+       .hsync_start    = 800 + 20,
+       .hsync_end      = 800 + 20 + 24,
+       .htotal         = 800 + 20 + 24 + 20,
+       .vdisplay       = 1280,
+       .vsync_start    = 1280 + 4,
+       .vsync_end      = 1280 + 4 + 8,
+       .vtotal         = 1280 + 4 + 8 + 4,
+       .flags          = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC,
+};
+
+static const struct panel_desc friendlyarm_hd702e = {
+       .modes = &friendlyarm_hd702e_mode,
+       .num_modes = 1,
+       .bpc = 8,
+       .size = {
+               .width  = 94,
+               .height = 151,
+       },
+};
+
 static const struct drm_display_mode innolux_n116bca_ea1_mode = {
        .clock = 76420,
        .hdisplay = 1366,

The EDP_HOTPLUG pin is optional, and muxed with other functions (notably HDMI CEC), so move its selection from the SoC DTSI to the boards which apparently want it, namely those which enable eDP without "force-hpd". By the same token we drop it from Pinebook Pro, which already uses "force-hpd", and according to the schematics does not have the pin wired at all.

Signed-off-by Robin Murphy <[email protected]>

---
 arch/arm64/boot/dts/rockchip/rk3399-base.dtsi              | 2 --
 arch/arm64/boot/dts/rockchip/rk3399-gru-chromebook.dtsi    | 2 ++
 arch/arm64/boot/dts/rockchip/rk3399-pinebook-pro.dts       | 2 --
 arch/arm64/boot/dts/rockchip/rk3399-sapphire-excavator.dts | 2 ++
 4 files changed, 4 insertions(+), 4 deletions(-)

arch/arm64/boot/dts/rockchip/rk3399-base.dtsi +0 -2

index 4dcceb9136b7..19a312baa9f1 100644
@@ -2145,8 +2145,6 @@ edp: dp@ff970000 {
 		interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH 0>;
 		clocks = <&cru PCLK_EDP>, <&cru PCLK_EDP_CTRL>, <&cru PCLK_VIO_GRF>;
 		clock-names = "dp", "pclk", "grf";
-		pinctrl-names = "default";
-		pinctrl-0 = <&edp_hpd>;
 		power-domains = <&power RK3399_PD_EDP>;
 		resets = <&cru SRST_P_EDP_CTRL>;
 		reset-names = "dp";

arch/arm64/boot/dts/rockchip/rk3399-gru-chromebook.dtsi +2 -0

index 9d07353df52c..3f3cb0eb5809 100644
@@ -241,6 +241,8 @@ &dmc {
 };
 
 &edp {
+	pinctrl-names = "default";
+	pinctrl-0 = <&edp_hpd>;
 	status = "okay";
 
 	/*

arch/arm64/boot/dts/rockchip/rk3399-pinebook-pro.dts +0 -2

index eaaca08a7601..dcab04863d28 100644
@@ -401,8 +401,6 @@ &cpu_l3 {
 
 &edp {
 	force-hpd;
-	pinctrl-names = "default";
-	pinctrl-0 = <&edp_hpd>;
 	status = "okay";
 };
 

arch/arm64/boot/dts/rockchip/rk3399-sapphire-excavator.dts +0 -0

index a4ceafe6dd7a..80d6ea0eda84 100644

Hello Robin,

Quoted 59 lines · Robin Murphy

The EDP_HOTPLUG pin is optional, and muxed with other functions (notably HDMI CEC), so move its selection from the SoC DTSI to the boards which apparently want it, namely those which enable eDP without "force-hpd". By the same token we drop it from Pinebook Pro, which already uses "force-hpd", and according to the schematics does not have the pin wired at all.

Signed-off-by Robin Murphy <[email protected]>

---
  arch/arm64/boot/dts/rockchip/rk3399-base.dtsi              | 2 --
  arch/arm64/boot/dts/rockchip/rk3399-gru-chromebook.dtsi    | 2 ++
  arch/arm64/boot/dts/rockchip/rk3399-pinebook-pro.dts       | 2 --
  arch/arm64/boot/dts/rockchip/rk3399-sapphire-excavator.dts | 2 ++
  4 files changed, 4 insertions(+), 4 deletions(-)

arch/arm64/boot/dts/rockchip/rk3399-base.dtsi +0 -2

index 4dcceb9136b7..19a312baa9f1 100644
@@ -2145,8 +2145,6 @@ edp: dp@ff970000 {
  		interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH 0>;
  		clocks = <&cru PCLK_EDP>, <&cru PCLK_EDP_CTRL>, <&cru PCLK_VIO_GRF>;
  		clock-names = "dp", "pclk", "grf";
-		pinctrl-names = "default";
-		pinctrl-0 = <&edp_hpd>;
  		power-domains = <&power RK3399_PD_EDP>;
  		resets = <&cru SRST_P_EDP_CTRL>;
  		reset-names = "dp";

arch/arm64/boot/dts/rockchip/rk3399-gru-chromebook.dtsi +2 -0

index 9d07353df52c..3f3cb0eb5809 100644
@@ -241,6 +241,8 @@ &dmc {
  };
 
  &edp {
+	pinctrl-names = "default";
+	pinctrl-0 = <&edp_hpd>;
  	status = "okay";
 
  	/*

arch/arm64/boot/dts/rockchip/rk3399-pinebook-pro.dts +0 -2

index eaaca08a7601..dcab04863d28 100644
@@ -401,8 +401,6 @@ &cpu_l3 {
 
  &edp {
  	force-hpd;
-	pinctrl-names = "default";
-	pinctrl-0 = <&edp_hpd>;
  	status = "okay";
  };
 

arch/arm64/boot/dts/rockchip/rk3399-sapphire-excavator.dts +0 -0

index a4ceafe6dd7a..80d6ea0eda84 100644

arch/arm64/boot/dts/rockchip/rk3399-sapphire-excavator.dts sdio_pwrseq: sdio-pwrseq { · line 147

Quoted 7 lines

arch/arm64/boot/dts/rockchip/rk3399-sapphire-excavator.dts +2 -0

@@ -141,6 +141,8 @@ sdio_pwrseq: sdio-pwrseq {
  };
 
  &edp {
+	pinctrl-names = "default";
+	pinctrl-0 = <&edp_hpd>;
  	status = "okay";
  };

Thanks for this patch! These changes are looking good to me, and the board dts files changed in this patch are the right ones, i.e. those that enable the "edp" node without specifying the "force-hpd" property.

Additionally,"/omit-if-no-ref/" should be added to the "edp" pinctrl node in rk3399-base.dtsi, to not include the "edp_hpd" pinctrl definition in the board dtb files where that actuually isn't used.

With the note above addressed, please feel free to include

Reviewed-by Dragan Simic <[email protected]>

+&{/} {
+ pwm_bl: backlight {

+ enable-gpios = <&gpio4 RK_PD5 GPIO_ACTIVE_HIGH>;

arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4-hd702e.dtso new file +9 -0

+
+                       port {
+                               panel_in_edp: endpoint {
+                                       remote-endpoint = <&edp_out_panel>;
+                               };
+                       };
+               };
+       };
+};

Hi,

Quoted 3 lines · Robin Murphy

+&{/} {
+ pwm_bl: backlight {

nit: up to you, but I happened to notice that other rk3399 boards just use the label "backlight:" instead of "pwm_bl:"

arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4-hd702e.dtso line 16

Quoted 2 lines

arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4-hd702e.dtso new file +2 -0

+               compatible = "pwm-backlight";
+               pwms = <&pwm0 0 25000 0>;

40kHz is awfully fast for a PWM backlight. Are you sure you need it that fast?

Quoted 2 lines

+ enable-gpios = <&gpio4 RK_PD5 GPIO_ACTIVE_HIGH>;

Not that I'm doing a thorough review here, but I happened to notice that you're referring to a GPIO without adding a pinctrl entry. I think the usual convention is to add one.

Quoted 8 lines

arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4-hd702e.dtso new file +8 -0

+               brightness-levels = <0 255>;
+               default-brightness-level = <200>;
+               num-interpolated-steps = <255>;
+       };
+};
+
+&edp {
+       force-hpd;

arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4-hd702e.dtso line 30

Quoted 5 lines

arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4-hd702e.dtso new file +5 -0

+       status = "okay";
+
+       aux-bus {
+               edp-panel {
+                       compatible = "friendlyarm,hd702e";

As per my response in your driver patch, any chance this can just be "edp-panel"?

arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4-hd702e.dtso line 33

Quoted 3 lines

arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4-hd702e.dtso new file +3 -0

+                       backlight = <&pwm_bl>;
+                       no-hpd;
+                       power-supply = <&vcc12v0_sys>;

While not strictly required, it seems highly likely that you want "hpd-absent-delay-ms". It's highly unlikely that you would have "no-hpd" plus a "power-supply" but no hardcoded delay to wait here. I haven't seen panels that turn on and respond instantly.

Quoted 13 lines

arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4-hd702e.dtso new file +13 -0

+
+                       port {
+                               panel_in_edp: endpoint {
+                                       remote-endpoint = <&edp_out_panel>;
+                               };
+                       };
+               };
+       };
+};
+
+&edp_out {
+       edp_out_panel: endpoint {
+               remote-endpoint = <&panel_in_edp>;

arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4-hd702e.dtso line 52

Quoted 6 lines

arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4-hd702e.dtso new file +6 -0

+       };
+};
+
+&i2c4 {
+       #address-cells = <1>;
+       #size-cells = <0>;

The base dts already specifies #address-cells and #size-cells, right?

arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4-hd702e.dtso line 60

Quoted 7 lines

arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4-hd702e.dtso new file +7 -0

+       touchscreen@5d {
+               compatible = "goodix,gt9271";
+               reg = <0x5d>;
+               interrupt-parent = <&gpio1>;
+               interrupts = <RK_PC4 IRQ_TYPE_EDGE_FALLING>;
+               irq-gpios = <&gpio1 RK_PC4 GPIO_ACTIVE_HIGH>;
+               reset-gpios = <&gpio1 RK_PB5 GPIO_ACTIVE_HIGH>;

There's no power supply here, so I'll assume it follows the power supply of the panel? You probably want to be a "panel-follower" then, right? AKA you'd want to add a "panel = " node here to refer to your edp-panel.

Oh, except that the goodix driver you're using doesn't support panel-follower. That's annoying. I guess you'd have to add support for that (see history around "is_panel_follower" in "i2c-hid-core.c")? Without it then I assume you'll just be lucky that things work? ...or you'd need to mark the power supply as always-on?

I guess I can also note the same comment that there is no pinctrl for your GPIOs here.

Quoted 6 lines

Quoted 3 lines

+&{/} {
+ pwm_bl: backlight {

nit: up to you, but I happened to notice that other rk3399 boards just use the label "backlight:" instead of "pwm_bl:"

Sure, if you prefer.

Quoted 4 lines

arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4-hd702e.dtso line 16

Quoted 2 lines

arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4-hd702e.dtso new file +2 -0

+               compatible = "pwm-backlight";
+               pwms = <&pwm0 0 25000 0>;

40kHz is awfully fast for a PWM backlight. Are you sure you need it that fast?

I guess I just copied this from somewhere without too much thought, but double-checking the schematic[1] now, the backlight driver where this signal ends up is apparently a SY7200A, whose datasheet says "And the recommend dimming frequency range is 20kHz~1MHz." So relative to that range it doesn't seem too bad!

Quoted 10 lines

arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4-hd702e.dtso line 30

Quoted 7 lines

+&edp {

arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4-hd702e.dtso new file +6 -0

+       force-hpd;
+       status = "okay";
+
+       aux-bus {
+               edp-panel {
+                       compatible = "friendlyarm,hd702e";

As per my response in your driver patch, any chance this can just be "edp-panel"?

Per the cover letter, I did try...

Quoted 8 lines

arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4-hd702e.dtso line 33

Quoted 3 lines

arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4-hd702e.dtso new file +3 -0

+                       backlight = <&pwm_bl>;
+                       no-hpd;
+                       power-supply = <&vcc12v0_sys>;

While not strictly required, it seems highly likely that you want "hpd-absent-delay-ms". It's highly unlikely that you would have "no-hpd" plus a "power-supply" but no hardcoded delay to wait here. I haven't seen panels that turn on and respond instantly.

...but even with both delays bumped up and up and up to a ridiculous 2000ms it still didn't work. It reads the EDID as all-zeros then fails to probe due to a lack of modes. Whereas with the hard-coded mode, the display lights up immediately upon probe, so I don't think it's a timing issue. My working theory is that there's some fundamental ordering issue based on the comments in analogix_dp_detect_hpd() about aux not working until HPD is forced at the controller end, and from a brief skim of the history, quite possibly related to f2600d08d4e8 ("drm/bridge:
analogix_dp: Improve panel on time") which maybe prevents that happening?

By that point it's well beyond my expertise, hence my conclusion that since I'm not *adding* the legacy panel data, just rearranging what's already upstream to finally put it to proper use, that's just about acceptable :)

The power-supply entry is really just for cleanliness, to avoid a "dummy regulator" message - the screen module has all its own regulation on board, which didn't seem worth modelling in detail as it's all fixed and always-on, but the source is the board's main 12V input, so that much *is* true.

Quoted 5 lines

arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4-hd702e.dtso line 52

Quoted 3 lines

+&i2c4 {

arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4-hd702e.dtso new file +2 -0

+       #address-cells = <1>;
+       #size-cells = <0>;

The base dts already specifies #address-cells and #size-cells, right?

Indeed, but dtc doesn't know that when compiling the .dtbo in isolation.

Quoted 13 lines

arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4-hd702e.dtso line 60

Quoted 7 lines

arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4-hd702e.dtso new file +7 -0

+       touchscreen@5d {
+               compatible = "goodix,gt9271";
+               reg = <0x5d>;
+               interrupt-parent = <&gpio1>;
+               interrupts = <RK_PC4 IRQ_TYPE_EDGE_FALLING>;
+               irq-gpios = <&gpio1 RK_PC4 GPIO_ACTIVE_HIGH>;
+               reset-gpios = <&gpio1 RK_PB5 GPIO_ACTIVE_HIGH>;

There's no power supply here, so I'll assume it follows the power supply of the panel? You probably want to be a "panel-follower" then, right? AKA you'd want to add a "panel = " node here to refer to your edp-panel.

Quoted 5 lines

Oh, except that the goodix driver you're using doesn't support panel-follower. That's annoying. I guess you'd have to add support for that (see history around "is_panel_follower" in "i2c-hid-core.c")? Without it then I assume you'll just be lucky that things work? ...or you'd need to mark the power supply as always-on?

Yeah, as above it is in fact always-on anyway - somehow I missed that the goodix driver is getting regulators too. The 12V input is stepped down to a main VDD_3.3V rail from which everything else is derived, so if I add a fixed-regulator node for that which is sufficiently close to the truth for both these and the panel supply, would that be clear enough?

Hi all,

I've been using the HD702E LCD/touchscreen on my NanoPC-T4 for a few years using my own DT with the "simple-panel" binding, but now we have upstream overlays I figured it's about time to try doing it properly. The screen itself doesn't seem to want to work as a generic "edp-panel", as it appears the EDID isn't readable until after the whole lot is enabled - I'm guessing this might be to do with the Analogix driver's force-hpd behaviour - but since we do already have the legacy data, it doesn't seem unreasonable to keep using it.