[PATCH 0/4] Add PinePhone Pro display support

[PATCH 0/4] Add PinePhone Pro display support

[Javier Martinez Canillas]

This series add support for the display present in the PinePhone Pro.

Patch #1 adds a driver for panels using the Himax HX8394 panel controller,
such as the HSD060BHW4 720x1440 TFT LCD panel present in the PinePhone Pro.

Patch #2 adds a devicetree binding schema for this driver and patch #3 adds
an entry for the driver in the MAINTAINERS file.

Finally patch #4 adds the needed devicetree nodes in the PinePhone Pro DTS,
to enable both the display and the touchscreen. This makes the upstream DTS
much more usable and will allow for example to enable support for the phone
in the Fedora distribution.

I only added myself as the maintainer for the driver because I don't know
if Kamil and Ondrej that worked in the driver would be interested. Please
let me know folks if you are, and I can add you too in the next revision.

The patches were tested on a PinePhone Pro Explorer Edition using a Fedora
37 Workstation image.

Best regards,
Javier


Javier Martinez Canillas (2):
  dt-bindings: display: Add Himax HX8394 panel controller bindings
  MAINTAINERS: Add entry for Himax HX8394 panel controller driver

Kamil Trzciński (1):
  drm: panel: Add Himax HX8394 panel controller driver

Ondrej Jirman (1):
  arm64: dts: rk3399-pinephone-pro: Add internal display support

 .../bindings/display/panel/himax,hx8394.yaml  |  68 +++
 MAINTAINERS                                   |   7 +
 .../dts/rockchip/rk3399-pinephone-pro.dts     | 124 +++++
 drivers/gpu/drm/panel/Kconfig                 |  12 +
 drivers/gpu/drm/panel/Makefile                |   1 +
 drivers/gpu/drm/panel/panel-himax-hx8394.c    | 460 ++++++++++++++++++
 6 files changed, 672 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/himax,hx8394.yaml
 create mode 100644 drivers/gpu/drm/panel/panel-himax-hx8394.c

-- 
2.38.1

[PATCH 2/4] dt-bindings: display: Add Himax HX8394 panel controller bindings

[Javier Martinez Canillas]

Add device tree bindings for panels based on the Himax HX8394 controller,
such as the HannStar HSD060BHW4 720x1440 TFT LCD panel that is connected
through a MIPI-DSI video interface.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 .../bindings/display/panel/himax,hx8394.yaml  | 68 +++++++++++++++++++
 1 file changed, 68 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/himax,hx8394.yaml

diff --git a/Documentation/devicetree/bindings/display/panel/himax,hx8394.yaml b/Documentation/devicetree/bindings/display/panel/himax,hx8394.yaml
new file mode 100644
index 000000000000..a8084e95f2fe
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/himax,hx8394.yaml
@@ -0,0 +1,68 @@
+# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/panel/himax,hx8394.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Himax HX8394 MIPI-DSI LCD panel controller device tree bindings
+
+maintainers:
+  - Javier Martinez Canillas <javierm@redhat.com>
+
+description:
+  Device tree bindings for panels based on the Himax HX8394 controller,
+  such as the HannStar HSD060BHW4 720x1440 TFT LCD panel connected with
+  a MIPI-DSI video interface.
+
+allOf:
+  - $ref: panel-common.yaml#
+
+properties:
+  compatible:
+    enum:
+      # HannStar HSD060BHW4 5.99" 720x1440 TFT LCD panel
+      - hannstar,hsd060bhw4
+
+  port: true
+  reg:
+    maxItems: 1
+    description: DSI virtual channel
+
+  vcc-supply:
+    description: Panel power supply
+
+  iovcc-supply:
+    description: I/O voltage supply
+
+  reset-gpios: true
+
+  backlight: true
+
+required:
+  - compatible
+  - reg
+  - vcc-supply
+  - iovcc-supply
+  - reset-gpios
+  - backlight
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    dsi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        panel@0 {
+            compatible = "hannstar,hsd060bhw4";
+            reg = <0>;
+            vcc-supply = <&reg_2v8_p>;
+            iovcc-supply = <&reg_1v8_p>;
+            reset-gpios = <&gpio3 13 GPIO_ACTIVE_LOW>;
+            backlight = <&backlight>;
+        };
+    };
+
+...
-- 
2.38.1

Re: [PATCH 2/4] dt-bindings: display: Add Himax HX8394 panel controller bindings

[Krzysztof Kozlowski]

On 22/12/2022 23:38, Javier Martinez Canillas wrote:
> Add device tree bindings for panels based on the Himax HX8394 controller,
> such as the HannStar HSD060BHW4 720x1440 TFT LCD panel that is connected
> through a MIPI-DSI video interface.

Subject: drop second, redundant "bindings".

> 
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> 
>  .../bindings/display/panel/himax,hx8394.yaml  | 68 +++++++++++++++++++
>  1 file changed, 68 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/panel/himax,hx8394.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/panel/himax,hx8394.yaml b/Documentation/devicetree/bindings/display/panel/himax,hx8394.yaml
> new file mode 100644
> index 000000000000..a8084e95f2fe
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/panel/himax,hx8394.yaml
> @@ -0,0 +1,68 @@
> +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/panel/himax,hx8394.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Himax HX8394 MIPI-DSI LCD panel controller device tree bindings

Drop "device tree bindings"

> +
> +maintainers:
> +  - Javier Martinez Canillas <javierm@redhat.com>
> +
> +description:
> +  Device tree bindings for panels based on the Himax HX8394 controller,
> +  such as the HannStar HSD060BHW4 720x1440 TFT LCD panel connected with
> +  a MIPI-DSI video interface.
> +
> +allOf:
> +  - $ref: panel-common.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      # HannStar HSD060BHW4 5.99" 720x1440 TFT LCD panel
> +      - hannstar,hsd060bhw4
> +
> +  port: true

Put the port next to other "true" properties.

> +  reg:
> +    maxItems: 1
> +    description: DSI virtual channel
> +

With three above:

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH 2/4] dt-bindings: display: Add Himax HX8394 panel controller bindings

[Javier Martinez Canillas]

Hello Krzysztof,

On 12/23/22 09:12, Krzysztof Kozlowski wrote:

[...]

> With three above:
> 
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>

Thanks a lot for the review! I'll fix the issues you pointed out in v2.
 
> Best regards,
> Krzysztof
> 

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH 4/4] arm64: dts: rk3399-pinephone-pro: Add internal display support

[Javier Martinez Canillas]

From: Ondrej Jirman <megi@xff.cz>

The phone's display is using Hannstar LCD panel, and Goodix based
touchscreen. Support it.

Signed-off-by: Ondrej Jirman <megi@xff.cz>
Co-developed-by: Martijn Braam <martijn@brixit.nl>
Signed-off-by: Martijn Braam <martijn@brixit.nl>
Co-developed-by: Kamil Trzciński <ayufan@ayufan.eu>
Signed-off-by: Kamil Trzciński <ayufan@ayufan.eu>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 .../dts/rockchip/rk3399-pinephone-pro.dts     | 124 ++++++++++++++++++
 1 file changed, 124 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
index 0e4442b59a55..a0439a60395e 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
@@ -29,6 +29,12 @@ chosen {
 		stdout-path = "serial2:1500000n8";
 	};
 
+	backlight: backlight {
+		compatible = "pwm-backlight";
+		pwms = <&pwm0 0 1000000 0>;
+		pwm-delay-us = <10000>;
+	};
+
 	gpio-keys {
 		compatible = "gpio-keys";
 		pinctrl-names = "default";
@@ -81,6 +87,32 @@ vcc1v8_codec: vcc1v8-codec-regulator {
 		regulator-max-microvolt = <1800000>;
 		vin-supply = <&vcc3v3_sys>;
 	};
+
+	/* MIPI DSI panel 1.8v supply */
+	vcc1v8_lcd: vcc1v8-lcd {
+		compatible = "regulator-fixed";
+		enable-active-high;
+		regulator-name = "vcc1v8_lcd";
+		regulator-min-microvolt = <1800000>;
+		regulator-max-microvolt = <1800000>;
+		vin-supply = <&vcc3v3_sys>;
+		gpio = <&gpio3 RK_PA5 GPIO_ACTIVE_HIGH>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&display_pwren1>;
+	};
+
+	/* MIPI DSI panel 2.8v supply */
+	vcc2v8_lcd: vcc2v8-lcd {
+		compatible = "regulator-fixed";
+		enable-active-high;
+		regulator-name = "vcc2v8_lcd";
+		regulator-min-microvolt = <2800000>;
+		regulator-max-microvolt = <2800000>;
+		vin-supply = <&vcc3v3_sys>;
+		gpio = <&gpio3 RK_PA1 GPIO_ACTIVE_HIGH>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&display_pwren>;
+	};
 };
 
 &cpu_l0 {
@@ -111,6 +143,11 @@ &emmc_phy {
 	status = "okay";
 };
 
+&gpu {
+	mali-supply = <&vdd_gpu>;
+	status = "okay";
+};
+
 &i2c0 {
 	clock-frequency = <400000>;
 	i2c-scl-rising-time-ns = <168>;
@@ -193,6 +230,9 @@ vcc3v0_touch: LDO_REG2 {
 				regulator-name = "vcc3v0_touch";
 				regulator-min-microvolt = <3000000>;
 				regulator-max-microvolt = <3000000>;
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
 			};
 
 			vcca1v8_codec: LDO_REG3 {
@@ -326,6 +366,26 @@ opp07 {
 	};
 };
 
+&i2c3 {
+	i2c-scl-rising-time-ns = <450>;
+	i2c-scl-falling-time-ns = <15>;
+	status = "okay";
+
+	touchscreen@14 {
+		compatible = "goodix,gt917s";
+		reg = <0x14>;
+		interrupt-parent = <&gpio3>;
+		interrupts = <RK_PB5 IRQ_TYPE_EDGE_RISING>;
+		irq-gpios = <&gpio3 RK_PB5 GPIO_ACTIVE_HIGH>;
+		reset-gpios = <&gpio3 RK_PB4 GPIO_ACTIVE_HIGH>;
+		AVDD28-supply = <&vcc3v0_touch>;
+		VDDIO-supply = <&vcc3v0_touch>;
+		touchscreen-size-x = <720>;
+		touchscreen-size-y = <1440>;
+		poweroff-in-suspend;
+	};
+};
+
 &io_domains {
 	bt656-supply = <&vcc1v8_dvp>;
 	audio-supply = <&vcca1v8_codec>;
@@ -334,6 +394,40 @@ &io_domains {
 	status = "okay";
 };
 
+&mipi_dsi {
+	status = "okay";
+	clock-master;
+
+	ports {
+		mipi_out: port@1 {
+			#address-cells = <0>;
+			#size-cells = <0>;
+			reg = <1>;
+
+			mipi_out_panel: endpoint {
+				remote-endpoint = <&mipi_in_panel>;
+			};
+		};
+	};
+
+	panel@0 {
+		compatible = "hannstar,hsd060bhw4";
+		reg = <0>;
+		backlight = <&backlight>;
+		reset-gpios = <&gpio4 RK_PD1 GPIO_ACTIVE_LOW>;
+		vcc-supply = <&vcc2v8_lcd>; // 2v8
+		iovcc-supply = <&vcc1v8_lcd>; // 1v8
+		pinctrl-names = "default";
+		pinctrl-0 = <&display_rst_l>;
+
+		port {
+			mipi_in_panel: endpoint {
+				remote-endpoint = <&mipi_out_panel>;
+			};
+		};
+	};
+};
+
 &pmu_io_domains {
 	pmu1830-supply = <&vcc_1v8>;
 	status = "okay";
@@ -360,6 +454,20 @@ vsel2_pin: vsel2-pin {
 		};
 	};
 
+	dsi {
+		display_rst_l: display-rst-l {
+			rockchip,pins = <4 RK_PD1 RK_FUNC_GPIO &pcfg_pull_down>;
+		};
+
+		display_pwren: display-pwren {
+			rockchip,pins = <3 RK_PA1 RK_FUNC_GPIO &pcfg_pull_down>;
+		};
+
+		display_pwren1: display-pwren1 {
+			rockchip,pins = <3 RK_PA5 RK_FUNC_GPIO &pcfg_pull_down>;
+		};
+	};
+
 	sound {
 		vcc1v8_codec_en: vcc1v8-codec-en {
 			rockchip,pins = <3 RK_PA4 RK_FUNC_GPIO &pcfg_pull_down>;
@@ -367,6 +475,10 @@ vcc1v8_codec_en: vcc1v8-codec-en {
 	};
 };
 
+&pwm0 {
+	status = "okay";
+};
+
 &sdmmc {
 	bus-width = <4>;
 	cap-sd-highspeed;
@@ -396,3 +508,15 @@ &tsadc {
 &uart2 {
 	status = "okay";
 };
+
+&vopb {
+	status = "okay";
+	assigned-clocks = <&cru DCLK_VOP0_DIV>, <&cru DCLK_VOP0>,
+			  <&cru ACLK_VOP0>, <&cru HCLK_VOP0>;
+	assigned-clock-rates = <0>, <0>, <400000000>, <100000000>;
+	assigned-clock-parents = <&cru PLL_CPLL>, <&cru DCLK_VOP0_FRAC>;
+};
+
+&vopb_mmu {
+	status = "okay";
+};
-- 
2.38.1

Re: [PATCH 4/4] arm64: dts: rk3399-pinephone-pro: Add internal display support

[Maya Matuszczyk]

Nice to see Pinephone Pro getting worked on.

czw., 22 gru 2022 o 23:39 Javier Martinez Canillas
<javierm@redhat.com> napisał(a):
>
> From: Ondrej Jirman <megi@xff.cz>
>
> The phone's display is using Hannstar LCD panel, and Goodix based
> touchscreen. Support it.
>
> Signed-off-by: Ondrej Jirman <megi@xff.cz>
> Co-developed-by: Martijn Braam <martijn@brixit.nl>
> Signed-off-by: Martijn Braam <martijn@brixit.nl>
> Co-developed-by: Kamil Trzciński <ayufan@ayufan.eu>
> Signed-off-by: Kamil Trzciński <ayufan@ayufan.eu>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>
>  .../dts/rockchip/rk3399-pinephone-pro.dts     | 124 ++++++++++++++++++
>  1 file changed, 124 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> index 0e4442b59a55..a0439a60395e 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> @@ -29,6 +29,12 @@ chosen {
>                 stdout-path = "serial2:1500000n8";
>         };
>
> +       backlight: backlight {
> +               compatible = "pwm-backlight";
> +               pwms = <&pwm0 0 1000000 0>;
> +               pwm-delay-us = <10000>;
> +       };
> +
>         gpio-keys {
>                 compatible = "gpio-keys";
>                 pinctrl-names = "default";
> @@ -81,6 +87,32 @@ vcc1v8_codec: vcc1v8-codec-regulator {
>                 regulator-max-microvolt = <1800000>;
>                 vin-supply = <&vcc3v3_sys>;
>         };
> +
> +       /* MIPI DSI panel 1.8v supply */
> +       vcc1v8_lcd: vcc1v8-lcd {
Node names should be generic, for example "vcc1v8-lcd-regulator".

> +               compatible = "regulator-fixed";
> +               enable-active-high;
Is this really needed?
You can set the polarity in "gpios" property.

> +               regulator-name = "vcc1v8_lcd";
> +               regulator-min-microvolt = <1800000>;
> +               regulator-max-microvolt = <1800000>;
> +               vin-supply = <&vcc3v3_sys>;
> +               gpio = <&gpio3 RK_PA5 GPIO_ACTIVE_HIGH>;
Is this a typo? Documentation says "gpios"

> +               pinctrl-names = "default";
> +               pinctrl-0 = <&display_pwren1>;
> +       };
> +
> +       /* MIPI DSI panel 2.8v supply */
> +       vcc2v8_lcd: vcc2v8-lcd {
> +               compatible = "regulator-fixed";
> +               enable-active-high;
Ditto

> +               regulator-name = "vcc2v8_lcd";
> +               regulator-min-microvolt = <2800000>;
> +               regulator-max-microvolt = <2800000>;
> +               vin-supply = <&vcc3v3_sys>;
> +               gpio = <&gpio3 RK_PA1 GPIO_ACTIVE_HIGH>;
Same as before

> +               pinctrl-names = "default";
> +               pinctrl-0 = <&display_pwren>;
> +       };
>  };
>
>  &cpu_l0 {
> @@ -111,6 +143,11 @@ &emmc_phy {
>         status = "okay";
>  };
>
> +&gpu {
> +       mali-supply = <&vdd_gpu>;
> +       status = "okay";
> +};
> +
>  &i2c0 {
>         clock-frequency = <400000>;
>         i2c-scl-rising-time-ns = <168>;
> @@ -193,6 +230,9 @@ vcc3v0_touch: LDO_REG2 {
>                                 regulator-name = "vcc3v0_touch";
>                                 regulator-min-microvolt = <3000000>;
>                                 regulator-max-microvolt = <3000000>;
> +                               regulator-state-mem {
> +                                       regulator-off-in-suspend;
> +                               };
>                         };
>
>                         vcca1v8_codec: LDO_REG3 {
> @@ -326,6 +366,26 @@ opp07 {
>         };
>  };
>
> +&i2c3 {
> +       i2c-scl-rising-time-ns = <450>;
> +       i2c-scl-falling-time-ns = <15>;
> +       status = "okay";
> +
> +       touchscreen@14 {
> +               compatible = "goodix,gt917s";
> +               reg = <0x14>;
> +               interrupt-parent = <&gpio3>;
> +               interrupts = <RK_PB5 IRQ_TYPE_EDGE_RISING>;
> +               irq-gpios = <&gpio3 RK_PB5 GPIO_ACTIVE_HIGH>;
> +               reset-gpios = <&gpio3 RK_PB4 GPIO_ACTIVE_HIGH>;
> +               AVDD28-supply = <&vcc3v0_touch>;
> +               VDDIO-supply = <&vcc3v0_touch>;
> +               touchscreen-size-x = <720>;
> +               touchscreen-size-y = <1440>;
> +               poweroff-in-suspend;
Are you really sure this property exists in touchscreen driver's dt bindings?

> +       };
> +};
> +
>  &io_domains {
>         bt656-supply = <&vcc1v8_dvp>;
>         audio-supply = <&vcca1v8_codec>;
> @@ -334,6 +394,40 @@ &io_domains {
>         status = "okay";
>  };
>
> +&mipi_dsi {
> +       status = "okay";
> +       clock-master;
> +
> +       ports {
> +               mipi_out: port@1 {
> +                       #address-cells = <0>;
> +                       #size-cells = <0>;
> +                       reg = <1>;
> +
> +                       mipi_out_panel: endpoint {
> +                               remote-endpoint = <&mipi_in_panel>;
> +                       };
> +               };
> +       };
> +
> +       panel@0 {
> +               compatible = "hannstar,hsd060bhw4";
> +               reg = <0>;
> +               backlight = <&backlight>;
> +               reset-gpios = <&gpio4 RK_PD1 GPIO_ACTIVE_LOW>;
> +               vcc-supply = <&vcc2v8_lcd>; // 2v8
What is the purpose of that comment?

> +               iovcc-supply = <&vcc1v8_lcd>; // 1v8
> +               pinctrl-names = "default";
> +               pinctrl-0 = <&display_rst_l>;
> +
> +               port {
> +                       mipi_in_panel: endpoint {
> +                               remote-endpoint = <&mipi_out_panel>;
> +                       };
> +               };
> +       };
> +};
> +
>  &pmu_io_domains {
>         pmu1830-supply = <&vcc_1v8>;
>         status = "okay";
> @@ -360,6 +454,20 @@ vsel2_pin: vsel2-pin {
>                 };
>         };
>
> +       dsi {
> +               display_rst_l: display-rst-l {
> +                       rockchip,pins = <4 RK_PD1 RK_FUNC_GPIO &pcfg_pull_down>;
> +               };
> +
> +               display_pwren: display-pwren {
> +                       rockchip,pins = <3 RK_PA1 RK_FUNC_GPIO &pcfg_pull_down>;
> +               };
> +
> +               display_pwren1: display-pwren1 {
> +                       rockchip,pins = <3 RK_PA5 RK_FUNC_GPIO &pcfg_pull_down>;
> +               };
> +       };
> +
>         sound {
>                 vcc1v8_codec_en: vcc1v8-codec-en {
>                         rockchip,pins = <3 RK_PA4 RK_FUNC_GPIO &pcfg_pull_down>;
> @@ -367,6 +475,10 @@ vcc1v8_codec_en: vcc1v8-codec-en {
>         };
>  };
>
> +&pwm0 {
> +       status = "okay";
> +};
> +
>  &sdmmc {
>         bus-width = <4>;
>         cap-sd-highspeed;
> @@ -396,3 +508,15 @@ &tsadc {
>  &uart2 {
>         status = "okay";
>  };
> +
> +&vopb {
> +       status = "okay";
> +       assigned-clocks = <&cru DCLK_VOP0_DIV>, <&cru DCLK_VOP0>,
> +                         <&cru ACLK_VOP0>, <&cru HCLK_VOP0>;
> +       assigned-clock-rates = <0>, <0>, <400000000>, <100000000>;
> +       assigned-clock-parents = <&cru PLL_CPLL>, <&cru DCLK_VOP0_FRAC>;
> +};
> +
> +&vopb_mmu {
> +       status = "okay";
> +};
> --
> 2.38.1
>
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Best Regards,
Maya Matuszczyk

Re: [PATCH 4/4] arm64: dts: rk3399-pinephone-pro: Add internal display support

[Javier Martinez Canillas]

Hello Maya,

On 12/22/22 23:57, Maya Matuszczyk wrote:
> Nice to see Pinephone Pro getting worked on.
>

Appreciate your feedback.

I agree with all your comments. Was too focused on the panel driver and didn't
pay that much attention to the DTS snippets. I'll clean these up on v2. Thanks!

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 4/4] arm64: dts: rk3399-pinephone-pro: Add internal display support

[Javier Martinez Canillas]

Hello Maya,

I'm going through this now and addressing your comments.

On 12/22/22 23:57, Maya Matuszczyk wrote:

[...]

>> +       /* MIPI DSI panel 1.8v supply */
>> +       vcc1v8_lcd: vcc1v8-lcd {
> Node names should be generic, for example "vcc1v8-lcd-regulator".
>

Fixed.
 
>> +               compatible = "regulator-fixed";
>> +               enable-active-high;
> Is this really needed?
> You can set the polarity in "gpios" property.
>

I understand that it is needed. The regulator-fixing binding says:

  enable-active-high:
    description:
      Polarity of GPIO is Active high. If this property is missing,
      the default assumed is Active low.
    type: boolean

and indeed by looking at the source in drivers/gpio/gpiolib-of.c, there is
a check for this property in the of_gpio_flags_quirks() function:

static void of_gpio_flags_quirks(const struct device_node *np,
				 const char *propname,
				 enum of_gpio_flags *flags,
				 int index)
{
	/*
	 * Some GPIO fixed regulator quirks.
	 * Note that active low is the default.
	 */
	if (IS_ENABLED(CONFIG_REGULATOR) &&
	    (of_device_is_compatible(np, "regulator-fixed") ||
	     of_device_is_compatible(np, "reg-fixed-voltage") ||
	     (!(strcmp(propname, "enable-gpio") &&
		strcmp(propname, "enable-gpios")) &&
	      of_device_is_compatible(np, "regulator-gpio")))) {
		bool active_low = !of_property_read_bool(np,
							 "enable-active-high");
		/*
		 * The regulator GPIO handles are specified such that the
		 * presence or absence of "enable-active-high" solely controls
		 * the polarity of the GPIO line. Any phandle flags must
		 * be actively ignored.
		 */
		if ((*flags & OF_GPIO_ACTIVE_LOW) && !active_low) {
			pr_warn("%s GPIO handle specifies active low - ignored\n",
				of_node_full_name(np));
			*flags &= ~OF_GPIO_ACTIVE_LOW;
		}
		if (active_low)
			*flags |= OF_GPIO_ACTIVE_LOW;
	}
...
}

So I'll keep those.
 
>> +               regulator-name = "vcc1v8_lcd";
>> +               regulator-min-microvolt = <1800000>;
>> +               regulator-max-microvolt = <1800000>;
>> +               vin-supply = <&vcc3v3_sys>;
>> +               gpio = <&gpio3 RK_PA5 GPIO_ACTIVE_HIGH>;
> Is this a typo? Documentation says "gpios"
>

Documentation/devicetree/bindings/gpio/gpio.txt indeed says "gpios" but "gpio"
is also supported for older DT that are using bindings that got it wrong. See
commits e7ae65ced7dd ("gpio: mention in DT binding doc that <name>-gpio is
deprecated") and dd34c37aa3e8 ("gpio: of: Allow -gpio suffix for property names").

The regulator-fixed bindings is such an example. See that its bindings schema
Documentation/devicetree/bindings/regulator/regulator.yaml mentions "gpio" and
not "gpios", also in the example.

So until that is fixed, I would prefer to stick with that's documented in the
regulator-fixed bindings doc.
 
[...]

>> +       touchscreen@14 {
>> +               compatible = "goodix,gt917s";
>> +               reg = <0x14>;
>> +               interrupt-parent = <&gpio3>;
>> +               interrupts = <RK_PB5 IRQ_TYPE_EDGE_RISING>;
>> +               irq-gpios = <&gpio3 RK_PB5 GPIO_ACTIVE_HIGH>;
>> +               reset-gpios = <&gpio3 RK_PB4 GPIO_ACTIVE_HIGH>;
>> +               AVDD28-supply = <&vcc3v0_touch>;
>> +               VDDIO-supply = <&vcc3v0_touch>;
>> +               touchscreen-size-x = <720>;
>> +               touchscreen-size-y = <1440>;
>> +               poweroff-in-suspend;
> Are you really sure this property exists in touchscreen driver's dt bindings?
>

It's not indeed. I've dropped that now.

[...]

>> +               vcc-supply = <&vcc2v8_lcd>; // 2v8
> What is the purpose of that comment?
> 
>> +               iovcc-supply = <&vcc1v8_lcd>; // 1v8

Yeah, not that useful. I've removed those two now.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 4/4] arm64: dts: rk3399-pinephone-pro: Add internal display support

[Krzysztof Kozlowski]

On 22/12/2022 23:38, Javier Martinez Canillas wrote:
> From: Ondrej Jirman <megi@xff.cz>
> 
> The phone's display is using Hannstar LCD panel, and Goodix based
> touchscreen. Support it.
> 
> Signed-off-by: Ondrej Jirman <megi@xff.cz>
> Co-developed-by: Martijn Braam <martijn@brixit.nl>
> Signed-off-by: Martijn Braam <martijn@brixit.nl>
> Co-developed-by: Kamil Trzciński <ayufan@ayufan.eu>
> Signed-off-by: Kamil Trzciński <ayufan@ayufan.eu>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> 
>  .../dts/rockchip/rk3399-pinephone-pro.dts     | 124 ++++++++++++++++++
>  1 file changed, 124 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> index 0e4442b59a55..a0439a60395e 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> @@ -29,6 +29,12 @@ chosen {
>  		stdout-path = "serial2:1500000n8";
>  	};
>  
> +	backlight: backlight {
> +		compatible = "pwm-backlight";
> +		pwms = <&pwm0 0 1000000 0>;
> +		pwm-delay-us = <10000>;
> +	};
> +
>  	gpio-keys {
>  		compatible = "gpio-keys";
>  		pinctrl-names = "default";
> @@ -81,6 +87,32 @@ vcc1v8_codec: vcc1v8-codec-regulator {
>  		regulator-max-microvolt = <1800000>;
>  		vin-supply = <&vcc3v3_sys>;
>  	};
> +
> +	/* MIPI DSI panel 1.8v supply */
> +	vcc1v8_lcd: vcc1v8-lcd {

Node names should be generic, so regulator suffix or prefix (looks like
other nodes already use suffix)

https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

> +		compatible = "regulator-fixed";
> +		enable-active-high;
> +		regulator-name = "vcc1v8_lcd";
> +		regulator-min-microvolt = <1800000>;
> +		regulator-max-microvolt = <1800000>;
> +		vin-supply = <&vcc3v3_sys>;
> +		gpio = <&gpio3 RK_PA5 GPIO_ACTIVE_HIGH>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&display_pwren1>;
> +	};
> +
> +	/* MIPI DSI panel 2.8v supply */
> +	vcc2v8_lcd: vcc2v8-lcd {

Node names should be generic.
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Best regards,
Krzysztof

Re: [PATCH 4/4] arm64: dts: rk3399-pinephone-pro: Add internal display support

[Javier Martinez Canillas]

On 12/23/22 09:13, Krzysztof Kozlowski wrote:
> On 22/12/2022 23:38, Javier Martinez Canillas wrote:

[...]

>> +
>> +	/* MIPI DSI panel 1.8v supply */
>> +	vcc1v8_lcd: vcc1v8-lcd {
> 
> Node names should be generic, so regulator suffix or prefix (looks like
> other nodes already use suffix)
>

Indeed, Maya already pointed this out. I missed this when forward porting
this patch from the pine64 downstream tree.
 -- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat