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
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
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
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
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
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
© 2016 - 2025 Red Hat, Inc.