| Message ID | 20221222223830.2494900-5-javierm@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add PinePhone Pro display support | expand |
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
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
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!
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
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.
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"; +};