[PATCH 0/4] Add JDI LPM102A188A display panel support

[PATCH 0/4] Add JDI LPM102A188A display panel support

[Diogo Ivo]

Hello!

These patches add support for the JDI LPM102A188A display panel,
found in the Google Pixel C.

Patch 1 adds the DT bindings for the panel.

Patch 2 adds an optional register clear to the Tegra DSI driver.

Patch 3 adds the panel driver, which is based on the downstream
kernel driver published by Google and developed by Sean Paul.

Patch 4 adds the DT node for the Google Pixel C. 

There is one point in this series on which I would like to ask for
some advice:

Since the device's bootloader leaves the display on and in patch 3 I
have assumed that the panel must be reset when probing, I was forced
to add patch 2, discovered by poking at the DSI module's registers until
the panel initialization sequence succeeded. However, if it is okay to
keep the panel on from the bootloader then it would be possible to
forego this second patch. Any comments on this would be highly appreciated.

Thank you!

Diogo Ivo (4):
  dt-bindings: display: Add bindings for JDI LPM102A188A
  drm/tegra: dsi: Clear enable register if powered by bootloader
  drm/panel: Add driver for JDI LPM102A188A
  arm64: dts: smaug: Add display panel node

 .../display/panel/jdi,lpm102a188a.yaml        | 100 ++++
 arch/arm64/boot/dts/nvidia/tegra210-smaug.dts |  72 +++
 drivers/gpu/drm/panel/Kconfig                 |  11 +
 drivers/gpu/drm/panel/Makefile                |   1 +
 drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c | 511 ++++++++++++++++++
 drivers/gpu/drm/tegra/dsi.c                   |  29 +
 6 files changed, 724 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/jdi,lpm102a188a.yaml
 create mode 100644 drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c

-- 
2.37.3

[PATCH 1/4] dt-bindings: display: Add bindings for JDI LPM102A188A

[Diogo Ivo]

The LPM102A188A is a 10.2" 2560x1800 IPS panel found in
the Google Pixel C.

Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
---
 .../display/panel/jdi,lpm102a188a.yaml        | 100 ++++++++++++++++++
 1 file changed, 100 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/jdi,lpm102a188a.yaml

diff --git a/Documentation/devicetree/bindings/display/panel/jdi,lpm102a188a.yaml b/Documentation/devicetree/bindings/display/panel/jdi,lpm102a188a.yaml
new file mode 100644
index 000000000000..97f9db7152da
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/jdi,lpm102a188a.yaml
@@ -0,0 +1,100 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/panel/jdi,lpm102a188a.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: JDI LPM102A188A 2560x1800 10.2" DSI Panel
+
+maintainers:
+  - Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
+
+description: |
+  This panel requires a dual-channel DSI host to operate. It supports two modes:
+  - left-right: each channel drives the left or right half of the screen
+  - even-odd: each channel drives the even or odd lines of the screen
+
+  Each of the DSI channels controls a separate DSI peripheral. The peripheral
+  driven by the first link (DSI-LINK1) is considered the primary peripheral
+  and controls the device. The 'link2' property contains a phandle to the
+  peripheral driven by the second link (DSI-LINK2).
+
+allOf:
+  - $ref: panel-common.yaml#
+
+properties:
+  compatible:
+    const: jdi,lpm102a188a
+
+  reg: true
+  enable-gpios: true
+  reset-gpios: true
+  power-supply: true
+  backlight: true
+
+  ts-reset-gpios:
+    maxItems: 1
+    description: |
+      Specifier for a GPIO connected to the touchscreen reset control signal.
+      The reset signal is active low.
+
+  ddi-supply:
+    description: The regulator that provides IOVCC (1.8V).
+
+  link2:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: |
+      phandle to the DSI peripheral on the secondary link. Note that the
+      presence of this property marks the containing node as DSI-LINK1.
+
+required:
+  - compatible
+  - reg
+
+if:
+  required:
+    - link2
+then:
+  required:
+    - power-supply
+    - ddi-supply
+    - enable-gpios
+    - reset-gpios
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/gpio/tegra-gpio.h>
+
+    dsia: dsi@54300000 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        reg = <0x0 0x54300000 0x0 0x00040000>;
+
+        link2: panel@0 {
+            compatible = "jdi,lpm102a188a";
+            reg = <0>;
+        };
+    };
+
+    dsib: dsi@54400000{
+        #address-cells = <1>;
+        #size-cells = <0>;
+        reg = <0x0 0x54400000 0x0 0x00040000>;
+        nvidia,ganged-mode = <&dsia>;
+
+        link1: panel@0 {
+            compatible = "jdi,lpm102a188a";
+            reg = <0>;
+            power-supply = <&pplcd_vdd>;
+            ddi-supply = <&pp1800_lcdio>;
+            enable-gpios = <&gpio TEGRA_GPIO(V, 1) GPIO_ACTIVE_HIGH>;
+            reset-gpios = <&gpio TEGRA_GPIO(V, 2) GPIO_ACTIVE_LOW>;
+            link2 = <&link2>;
+            backlight = <&backlight>;
+        };
+    };
+
+...
-- 
2.37.3

Re: [PATCH 1/4] dt-bindings: display: Add bindings for JDI LPM102A188A

[Krzysztof Kozlowski]

On 29/09/2022 19:04, Diogo Ivo wrote:
> The LPM102A188A is a 10.2" 2560x1800 IPS panel found in
> the Google Pixel C.
> 


Thank you for your patch. There is something to discuss/improve.

> +  Each of the DSI channels controls a separate DSI peripheral. The peripheral
> +  driven by the first link (DSI-LINK1) is considered the primary peripheral
> +  and controls the device. The 'link2' property contains a phandle to the
> +  peripheral driven by the second link (DSI-LINK2).
> +
> +allOf:
> +  - $ref: panel-common.yaml#
> +
> +properties:
> +  compatible:
> +    const: jdi,lpm102a188a
> +
> +  reg: true
> +  enable-gpios: true
> +  reset-gpios: true
> +  power-supply: true
> +  backlight: true
> +
> +  ts-reset-gpios:
> +    maxItems: 1
> +    description: |
> +      Specifier for a GPIO connected to the touchscreen reset control signal.
> +      The reset signal is active low.

Isn't touchscreen a separate (input) device?

> +
> +  ddi-supply:
> +    description: The regulator that provides IOVCC (1.8V).
> +
> +  link2:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: |
> +      phandle to the DSI peripheral on the secondary link. Note that the
> +      presence of this property marks the containing node as DSI-LINK1.

Best regards,
Krzysztof

Re: [PATCH 1/4] dt-bindings: display: Add bindings for JDI LPM102A188A

[Diogo Ivo]

On Fri, Sep 30, 2022 at 12:49:31PM +0200, Krzysztof Kozlowski wrote:
> > +  ts-reset-gpios:
> > +    maxItems: 1
> > +    description: |
> > +      Specifier for a GPIO connected to the touchscreen reset control signal.
> > +      The reset signal is active low.
> 
> Isn't touchscreen a separate (input) device?

Hello, thank you for the feedback.

According to the downstream kernel's log, it seems like the panel and
the touchscreen controller are considered to be embedded in the same unit
(for example in [1]), with the touch input being transmitted via HID-over-I2C,
and since I did not find any reset gpio handling in that driver I opted to
include this reset here, unless there is a better way of going about this.

Best regards,

Diogo

[1]: https://android.googlesource.com/kernel/tegra/+/bca61c34db9f72113af058f53eeb9fbd5e69a1d0

Re: [PATCH 1/4] dt-bindings: display: Add bindings for JDI LPM102A188A

[Krzysztof Kozlowski]

On 03/10/2022 19:06, Diogo Ivo wrote:
> On Fri, Sep 30, 2022 at 12:49:31PM +0200, Krzysztof Kozlowski wrote:
>>> +  ts-reset-gpios:
>>> +    maxItems: 1
>>> +    description: |
>>> +      Specifier for a GPIO connected to the touchscreen reset control signal.
>>> +      The reset signal is active low.
>>
>> Isn't touchscreen a separate (input) device?
> 
> Hello, thank you for the feedback.
> 
> According to the downstream kernel's log, it seems like the panel and
> the touchscreen controller are considered to be embedded in the same unit
> (for example in [1]), 

Downstream kernel is not a proof of proper description of hardware. If
downstream says orange is an apple, does it mean orange is really an
apple? No... Downstream creates a lot of junk, hacks and workarounds.

> with the touch input being transmitted via HID-over-I2C,
> and since I did not find any reset gpio handling in that driver I opted to
> include this reset here, unless there is a better way of going about this.

Instead it should be in touch screen device.

> 
> Best regards,
> 
> Diogo
> 
> [1]: https://android.googlesource.com/kernel/tegra/+/bca61c34db9f72113af058f53eeb9fbd5e69a1d0

Where is the DTS of that device?

Best regards,
Krzysztof

Re: [PATCH 1/4] dt-bindings: display: Add bindings for JDI LPM102A188A

[Diogo Ivo]

On Tue, Oct 04, 2022 at 01:05:04PM +0200, Krzysztof Kozlowski wrote:
> On 03/10/2022 19:06, Diogo Ivo wrote:
> > On Fri, Sep 30, 2022 at 12:49:31PM +0200, Krzysztof Kozlowski wrote:
> >> Isn't touchscreen a separate (input) device?
> > 
> > Hello, thank you for the feedback.
> > 
> > According to the downstream kernel's log, it seems like the panel and
> > the touchscreen controller are considered to be embedded in the same unit
> > (for example in [1]), 
> 
> Downstream kernel is not a proof of proper description of hardware. If
> downstream says orange is an apple, does it mean orange is really an
> apple? No... Downstream creates a lot of junk, hacks and workarounds.

After some searching (which I should have done sooner, so
apologies) I came across a teardown of the Pixel C ([1], for completeness),
which incorporates this panel. Indeed a separate touch controller was found,
so it seems the downstream kernel threw me off as per your warning.

[1]: https://www.ifixit.com/Teardown/Google+Pixel+C+Teardown/62277 (Step 4)

> > with the touch input being transmitted via HID-over-I2C,
> > and since I did not find any reset gpio handling in that driver I opted to
> > include this reset here, unless there is a better way of going about this.
> 
> Instead it should be in touch screen device.

Noted, I will remove it from the binding in the next version. 

> Where is the DTS of that device?

The relevant part of the DTS can be found here:
https://android.googlesource.com/kernel/tegra/+/refs/heads/android-tegra-dragon-3.18-oreo/arch/arm64/boot/dts/tegra/tegra210-smaug.dtsi

Best regards,
Diogo

[PATCH 2/4] drm/tegra: dsi: Clear enable register if powered by bootloader

[Diogo Ivo]

In cases where the DSI module is left on by the bootloader
some panels may fail to initialize if the enable register is not cleared
before the panel's initialization sequence. Clear it and add an optional
device tree property to inform the driver if this is the case.

Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
---
 drivers/gpu/drm/tegra/dsi.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
index de1333dc0d86..f66514379913 100644
--- a/drivers/gpu/drm/tegra/dsi.c
+++ b/drivers/gpu/drm/tegra/dsi.c
@@ -78,6 +78,8 @@ struct tegra_dsi {
 	unsigned int video_fifo_depth;
 	unsigned int host_fifo_depth;
 
+	bool enabled;
+
 	/* for ganged-mode support */
 	struct tegra_dsi *master;
 	struct tegra_dsi *slave;
@@ -460,6 +462,8 @@ static void tegra_dsi_enable(struct tegra_dsi *dsi)
 	value |= DSI_POWER_CONTROL_ENABLE;
 	tegra_dsi_writel(dsi, value, DSI_POWER_CONTROL);
 
+	dsi->enabled = true;
+
 	if (dsi->slave)
 		tegra_dsi_enable(dsi->slave);
 }
@@ -737,6 +741,8 @@ static void tegra_dsi_disable(struct tegra_dsi *dsi)
 	value &= ~DSI_POWER_CONTROL_ENABLE;
 	tegra_dsi_writel(dsi, value, DSI_POWER_CONTROL);
 
+	dsi->enabled = false;
+
 	if (dsi->slave)
 		tegra_dsi_disable(dsi->slave);
 
@@ -912,6 +918,27 @@ static void tegra_dsi_encoder_enable(struct drm_encoder *encoder)
 	u32 value;
 	int err;
 
+	/* If the bootloader enabled DSI it needs to be disabled
+	 * in order for the panel initialization commands to be
+	 * properly sent.
+	 */
+	if (dsi->enabled) {
+		if (dc) {
+			value = tegra_dc_readl(dc, DC_DISP_DISP_WIN_OPTIONS);
+			value &= ~DSI_ENABLE;
+			tegra_dc_writel(dc, value, DC_DISP_DISP_WIN_OPTIONS);
+
+			tegra_dc_commit(dc);
+		}
+
+		err = tegra_dsi_wait_idle(dsi, 100);
+		if (err < 0)
+			dev_dbg(dsi->dev, "failed to idle DSI: %d\n", err);
+
+		/* disable DSI controller */
+		tegra_dsi_disable(dsi);
+	}
+
 	err = tegra_dsi_prepare(dsi);
 	if (err < 0) {
 		dev_err(dsi->dev, "failed to prepare: %d\n", err);
@@ -1573,6 +1600,8 @@ static int tegra_dsi_probe(struct platform_device *pdev)
 
 	dsi->output.connector.polled = DRM_CONNECTOR_POLL_HPD;
 
+	/* Check if the DSI module was left on by bootloader. */
+	dsi->enabled = of_property_read_bool(pdev->dev.of_node, "nvidia,boot-on");
 	/*
 	 * Assume these values by default. When a DSI peripheral driver
 	 * attaches to the DSI host, the parameters will be taken from
-- 
2.37.3

Re: [PATCH 2/4] drm/tegra: dsi: Clear enable register if powered by bootloader

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3840 bytes --]

On Thu, Sep 29, 2022 at 06:05:00PM +0100, Diogo Ivo wrote:
> In cases where the DSI module is left on by the bootloader
> some panels may fail to initialize if the enable register is not cleared
> before the panel's initialization sequence. Clear it and add an optional
> device tree property to inform the driver if this is the case.
> 
> Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
> ---
>  drivers/gpu/drm/tegra/dsi.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
> index de1333dc0d86..f66514379913 100644
> --- a/drivers/gpu/drm/tegra/dsi.c
> +++ b/drivers/gpu/drm/tegra/dsi.c
> @@ -78,6 +78,8 @@ struct tegra_dsi {
>  	unsigned int video_fifo_depth;
>  	unsigned int host_fifo_depth;
>  
> +	bool enabled;

There should be no need to track this. DRM/KMS internally already knows,
so we should make use of the built-in mechanisms as much as possible.

> +
>  	/* for ganged-mode support */
>  	struct tegra_dsi *master;
>  	struct tegra_dsi *slave;
> @@ -460,6 +462,8 @@ static void tegra_dsi_enable(struct tegra_dsi *dsi)
>  	value |= DSI_POWER_CONTROL_ENABLE;
>  	tegra_dsi_writel(dsi, value, DSI_POWER_CONTROL);
>  
> +	dsi->enabled = true;
> +
>  	if (dsi->slave)
>  		tegra_dsi_enable(dsi->slave);
>  }
> @@ -737,6 +741,8 @@ static void tegra_dsi_disable(struct tegra_dsi *dsi)
>  	value &= ~DSI_POWER_CONTROL_ENABLE;
>  	tegra_dsi_writel(dsi, value, DSI_POWER_CONTROL);
>  
> +	dsi->enabled = false;
> +
>  	if (dsi->slave)
>  		tegra_dsi_disable(dsi->slave);
>  
> @@ -912,6 +918,27 @@ static void tegra_dsi_encoder_enable(struct drm_encoder *encoder)
>  	u32 value;
>  	int err;
>  
> +	/* If the bootloader enabled DSI it needs to be disabled
> +	 * in order for the panel initialization commands to be
> +	 * properly sent.
> +	 */
> +	if (dsi->enabled) {
> +		if (dc) {
> +			value = tegra_dc_readl(dc, DC_DISP_DISP_WIN_OPTIONS);
> +			value &= ~DSI_ENABLE;
> +			tegra_dc_writel(dc, value, DC_DISP_DISP_WIN_OPTIONS);
> +
> +			tegra_dc_commit(dc);
> +		}
> +
> +		err = tegra_dsi_wait_idle(dsi, 100);
> +		if (err < 0)
> +			dev_dbg(dsi->dev, "failed to idle DSI: %d\n", err);
> +
> +		/* disable DSI controller */
> +		tegra_dsi_disable(dsi);
> +	}

This is suboptimal because it is largely a duplication of what we
already have in tegra_dsi_disable(). Also, see below.

> +
>  	err = tegra_dsi_prepare(dsi);
>  	if (err < 0) {
>  		dev_err(dsi->dev, "failed to prepare: %d\n", err);
> @@ -1573,6 +1600,8 @@ static int tegra_dsi_probe(struct platform_device *pdev)
>  
>  	dsi->output.connector.polled = DRM_CONNECTOR_POLL_HPD;
>  
> +	/* Check if the DSI module was left on by bootloader. */
> +	dsi->enabled = of_property_read_bool(pdev->dev.of_node, "nvidia,boot-on");

The isn't a documented property. But before you go and add this, are
there no alternative ways to detect that the DSI controller is active?
Could we not read one of the registers to find out?

DRM/KMS has built-in mechanisms to read back hardware state on boot, so
I wonder if we can hook that up. It'd make the most sense if all sub-
drivers did this, because then we could eventually inherit the
bootloader configuration and transition to the kernel display driver
seamlessly, but doing this in DSI first may help prepare for that more
extended use-case.

A slightly simpler alternative would be to add the reset code to the
encoder's or connector's ->reset() implementation. This is called at the
right time (i.e. when the mode configuration is first reset), so you can
run the workaround from tegra_dsi_encoder_enable() there. That's better
than having this guarded by the dsi->enabled flag so that it is run only
once.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/4] drm/tegra: dsi: Clear enable register if powered by bootloader

[Diogo Ivo]

On Fri, Sep 30, 2022 at 01:11:10PM +0200, Thierry Reding wrote:
> On Thu, Sep 29, 2022 at 06:05:00PM +0100, Diogo Ivo wrote:
> > +
> >  	err = tegra_dsi_prepare(dsi);
> >  	if (err < 0) {
> >  		dev_err(dsi->dev, "failed to prepare: %d\n", err);
> > @@ -1573,6 +1600,8 @@ static int tegra_dsi_probe(struct platform_device *pdev)
> >  
> >  	dsi->output.connector.polled = DRM_CONNECTOR_POLL_HPD;
> >  
> > +	/* Check if the DSI module was left on by bootloader. */
> > +	dsi->enabled = of_property_read_bool(pdev->dev.of_node, "nvidia,boot-on");
> 
> The isn't a documented property. But before you go and add this, are
> there no alternative ways to detect that the DSI controller is active?
> Could we not read one of the registers to find out?

Hello, thank you for your feedback.

You are correct, it is possible to simply read a register to obtain
this information, and this property is not needed.

> DRM/KMS has built-in mechanisms to read back hardware state on boot, so
> I wonder if we can hook that up. It'd make the most sense if all sub-
> drivers did this, because then we could eventually inherit the
> bootloader configuration and transition to the kernel display driver
> seamlessly, but doing this in DSI first may help prepare for that more
> extended use-case.

I have only recently started digging in the DRM/KMS subsystem, could
you point out what those mechanisms are? That end goal seems like
something worth pursuing.

> A slightly simpler alternative would be to add the reset code to the
> encoder's or connector's ->reset() implementation. This is called at the
> right time (i.e. when the mode configuration is first reset), so you can
> run the workaround from tegra_dsi_encoder_enable() there. That's better
> than having this guarded by the dsi->enabled flag so that it is run only
> once.
> 
> Thierry

Regarding the placement of the workaround, I placed it in encoder_enable()
since my attempts of placing it in other functions (such as the connector's
->reset() method) resulted in a kernel hang, and I have no solution for this.
I'm assuming this is due to some part of the DSI hardware not being fully
initialized, but I haven't been able to confirm this.

Best regards,

Diogo

[PATCH 3/4] drm/panel: Add driver for JDI LPM102A188A

[Diogo Ivo]

The JDI LPM102A188A is a 2560x1800 IPS panel found in the Google Pixel C.
This driver is based on the downstream GPLv2 driver released by Google
written by Sean Paul [1], which was then adapted to the newer kernel APIs.

[1]: https://android.googlesource.com/kernel/tegra/+/refs/heads/android-tegra-dragon-3.18-oreo/drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c

Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
---
 drivers/gpu/drm/panel/Kconfig                 |  11 +
 drivers/gpu/drm/panel/Makefile                |   1 +
 drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c | 513 ++++++++++++++++++
 3 files changed, 525 insertions(+)
 create mode 100644 drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 38799effd00a..532a15a38b60 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -203,6 +203,17 @@ config DRM_PANEL_JDI_LT070ME05000
 	  The panel has a 1200(RGB)×1920 (WUXGA) resolution and uses
 	  24 bit per pixel.
 
+config DRM_PANEL_JDI_LPM102A188A
+	tristate "JDI LPM102A188A DSI panel"
+	depends on OF && GPIOLIB
+	depends on DRM_MIPI_DSI
+	depends on BACKLIGHT_CLASS_DEVICE
+	help
+	  Say Y here if you want to enable support for JDI LPM102A188A DSI
+	  control mode panel as found in Google Pixel C devices.
+	  The panel has a 2560×1800 resolution. It provides a MIPI DSI interface
+	  to the host.
+
 config DRM_PANEL_JDI_R63452
 	tristate "JDI R63452 Full HD DSI panel"
 	depends on OF
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index 42a7ab54234b..774a5ce0ebb8 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_DRM_PANEL_ILITEK_ILI9881C) += panel-ilitek-ili9881c.o
 obj-$(CONFIG_DRM_PANEL_INNOLUX_EJ030NA) += panel-innolux-ej030na.o
 obj-$(CONFIG_DRM_PANEL_INNOLUX_P079ZCA) += panel-innolux-p079zca.o
 obj-$(CONFIG_DRM_PANEL_JDI_LT070ME05000) += panel-jdi-lt070me05000.o
+obj-$(CONFIG_DRM_PANEL_JDI_LPM102A188A) += panel-jdi-lpm102a188a.o
 obj-$(CONFIG_DRM_PANEL_JDI_R63452) += panel-jdi-fhd-r63452.o
 obj-$(CONFIG_DRM_PANEL_KHADAS_TS050) += panel-khadas-ts050.o
 obj-$(CONFIG_DRM_PANEL_KINGDISPLAY_KD097D04) += panel-kingdisplay-kd097d04.o
diff --git a/drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c b/drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c
new file mode 100644
index 000000000000..c7f13719d4ff
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c
@@ -0,0 +1,513 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2014 Google, Inc.
+ *
+ * Copyright (C) 2022 Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
+ *
+ * Adapted from the downstream Pixel C driver written by Sean Paul
+ */
+
+#include <linux/backlight.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regulator/consumer.h>
+
+#include <video/mipi_display.h>
+
+#include <drm/drm_crtc.h>
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_panel.h>
+
+struct jdi_panel {
+	struct drm_panel base;
+	struct mipi_dsi_device *link1;
+	struct mipi_dsi_device *link2;
+
+	struct regulator *supply;
+	struct regulator *ddi_supply;
+	struct backlight_device *backlight;
+
+	struct gpio_desc *enable_gpio;
+	struct gpio_desc *reset_gpio;
+
+	const struct drm_display_mode *mode;
+
+	bool prepared;
+	bool enabled;
+};
+
+static inline struct jdi_panel *to_panel_jdi(struct drm_panel *panel)
+{
+	return container_of(panel, struct jdi_panel, base);
+}
+
+static void jdi_wait_frames(struct jdi_panel *jdi, unsigned int frames)
+{
+	unsigned int refresh = drm_mode_vrefresh(jdi->mode);
+
+	if (WARN_ON(frames > refresh))
+		return;
+
+	msleep(1000 / (refresh / frames));
+}
+
+static int jdi_panel_disable(struct drm_panel *panel)
+{
+	struct jdi_panel *jdi = to_panel_jdi(panel);
+
+	if (!jdi->enabled)
+		return 0;
+
+	backlight_disable(jdi->backlight);
+
+	jdi_wait_frames(jdi, 2);
+
+	jdi->enabled = false;
+
+	return 0;
+}
+
+static int jdi_panel_unprepare(struct drm_panel *panel)
+{
+	struct jdi_panel *jdi = to_panel_jdi(panel);
+	int ret;
+
+	if (!jdi->prepared)
+		return 0;
+
+	ret = mipi_dsi_dcs_set_display_off(jdi->link1);
+	if (ret < 0)
+		dev_err(panel->dev, "failed to set display off: %d\n", ret);
+	ret = mipi_dsi_dcs_set_display_off(jdi->link2);
+	if (ret < 0)
+		dev_err(panel->dev, "failed to set display off: %d\n", ret);
+
+	/* Specified by JDI @ 50ms, subject to change */
+	msleep(50);
+
+	ret = mipi_dsi_dcs_enter_sleep_mode(jdi->link1);
+	if (ret < 0)
+		dev_err(panel->dev, "failed to enter sleep mode: %d\n", ret);
+	ret = mipi_dsi_dcs_enter_sleep_mode(jdi->link2);
+	if (ret < 0)
+		dev_err(panel->dev, "failed to enter sleep mode: %d\n", ret);
+
+	/* Specified by JDI @ 150ms, subject to change */
+	msleep(150);
+
+	gpiod_set_value(jdi->reset_gpio, 1);
+
+	/* T4 = 1ms */
+	usleep_range(1000, 3000);
+
+	gpiod_set_value(jdi->enable_gpio, 0);
+
+	/* T5 = 2ms */
+	usleep_range(2000, 4000);
+
+	regulator_disable(jdi->ddi_supply);
+
+	/* T6 = 2ms */
+	usleep_range(5000, 6000);
+
+	regulator_disable(jdi->supply);
+
+	jdi->prepared = false;
+
+	return 0;
+}
+
+static int jdi_setup_symmetrical_split(struct mipi_dsi_device *left,
+				       struct mipi_dsi_device *right,
+				       const struct drm_display_mode *mode)
+{
+	int err;
+
+	err = mipi_dsi_dcs_set_column_address(left, 0, mode->hdisplay / 2 - 1);
+	if (err < 0) {
+		dev_err(&left->dev, "failed to set column address: %d\n", err);
+		return err;
+	}
+
+	err = mipi_dsi_dcs_set_column_address(right, 0, mode->hdisplay / 2 - 1);
+	if (err < 0) {
+		dev_err(&right->dev, "failed to set column address: %d\n", err);
+		return err;
+	}
+
+	err = mipi_dsi_dcs_set_page_address(left, 0, mode->vdisplay - 1);
+	if (err < 0) {
+		dev_err(&left->dev, "failed to set page address: %d\n", err);
+		return err;
+	}
+
+	err = mipi_dsi_dcs_set_page_address(right, 0, mode->vdisplay - 1);
+	if (err < 0) {
+		dev_err(&right->dev, "failed to set page address: %d\n", err);
+		return err;
+	}
+
+	return 0;
+}
+
+static int jdi_panel_prepare(struct drm_panel *panel)
+{
+	struct jdi_panel *jdi = to_panel_jdi(panel);
+	u8 format = MIPI_DCS_PIXEL_FMT_24BIT;
+	int err;
+
+	if (jdi->prepared)
+		return 0;
+
+	/* Disable backlight to avoid showing random pixels
+	 * with a conservative delay for it to take effect.
+	 */
+	backlight_disable(jdi->backlight);
+	msleep(200);
+
+	jdi->link1->mode_flags |= MIPI_DSI_MODE_LPM;
+	jdi->link2->mode_flags |= MIPI_DSI_MODE_LPM;
+
+	err = regulator_enable(jdi->supply);
+	if (err < 0) {
+		dev_err(panel->dev, "failed to enable supply: %d\n", err);
+		return err;
+	}
+	/* T1 = 2ms */
+	usleep_range(2000, 4000);
+
+	err = regulator_enable(jdi->ddi_supply);
+	if (err < 0) {
+		dev_err(panel->dev, "failed to enable ddi_supply: %d\n", err);
+		return err;
+	}
+	/* T2 = 1ms */
+	usleep_range(1000, 3000);
+
+	gpiod_set_value(jdi->enable_gpio, 1);
+	/* T3 = 10ms */
+	usleep_range(10000, 15000);
+
+	gpiod_set_value(jdi->reset_gpio, 0);
+	/* Specified by JDI @ 3ms, subject to change */
+	usleep_range(3000, 5000);
+
+	/*
+	 * TODO: The device supports both left-right and even-odd split
+	 * configurations, but this driver currently supports only the left-
+	 * right split. To support a different mode a mechanism needs to be
+	 * put in place to communicate the configuration back to the DSI host
+	 * controller.
+	 */
+
+	err = jdi_setup_symmetrical_split(jdi->link1, jdi->link2,
+					    jdi->mode);
+	if (err < 0) {
+		dev_err(panel->dev, "failed to set up symmetrical split: %d\n",
+			err);
+		goto poweroff;
+	}
+
+	err = mipi_dsi_dcs_set_tear_scanline(jdi->link1,
+					     jdi->mode->vdisplay - 16);
+	if (err < 0)
+		dev_err(panel->dev, "failed to set tear scanline: %d\n", err);
+
+	err = mipi_dsi_dcs_set_tear_scanline(jdi->link2,
+					     jdi->mode->vdisplay - 16);
+	if (err < 0)
+		dev_err(panel->dev, "failed to set tear scanline: %d\n", err);
+
+	err = mipi_dsi_dcs_set_tear_on(jdi->link1,
+				       MIPI_DSI_DCS_TEAR_MODE_VBLANK);
+	if (err < 0)
+		dev_err(panel->dev, "failed to set tear on: %d\n", err);
+
+	err = mipi_dsi_dcs_set_tear_on(jdi->link2,
+				       MIPI_DSI_DCS_TEAR_MODE_VBLANK);
+	if (err < 0)
+		dev_err(panel->dev, "failed to set tear on: %d\n", err);
+
+	err = mipi_dsi_dcs_set_pixel_format(jdi->link1, format);
+	if (err < 0) {
+		dev_err(panel->dev, "failed to set pixel format: %d\n", err);
+		goto poweroff;
+	}
+
+	err = mipi_dsi_dcs_set_pixel_format(jdi->link2, format);
+	if (err < 0) {
+		dev_err(panel->dev, "failed to set pixel format: %d\n", err);
+		goto poweroff;
+	}
+
+	err = mipi_dsi_dcs_exit_sleep_mode(jdi->link1);
+	if (err < 0) {
+		dev_err(panel->dev, "failed to exit sleep mode: %d\n", err);
+		goto poweroff;
+	}
+
+	err = mipi_dsi_dcs_exit_sleep_mode(jdi->link2);
+	if (err < 0) {
+		dev_err(panel->dev, "failed to exit sleep mode: %d\n", err);
+		goto poweroff;
+	}
+
+	/*
+	 * We need to wait 150ms between mipi_dsi_dcs_exit_sleep_mode() and
+	 * mipi_dsi_dcs_set_display_on().
+	 */
+	msleep(150);
+
+	err = mipi_dsi_dcs_set_display_on(jdi->link1);
+	if (err < 0)
+		dev_err(panel->dev, "failed to set display on: %d\n", err);
+
+
+	err = mipi_dsi_dcs_set_display_on(jdi->link2);
+	if (err < 0)
+		dev_err(panel->dev, "failed to set display on: %d\n", err);
+
+	jdi->prepared = true;
+
+	jdi->link1->mode_flags &= ~MIPI_DSI_MODE_LPM;
+	jdi->link2->mode_flags &= ~MIPI_DSI_MODE_LPM;
+
+	return 0;
+
+poweroff:
+	regulator_disable(jdi->ddi_supply);
+
+	/* T6 = 2ms */
+	usleep_range(7000, 9000);
+
+	regulator_disable(jdi->supply);
+
+	return err;
+}
+
+static int jdi_panel_enable(struct drm_panel *panel)
+{
+	struct jdi_panel *jdi = to_panel_jdi(panel);
+
+	if (jdi->enabled)
+		return 0;
+
+	/*
+	 * Ensure we send one frame of image data before backlight turn on,
+	 * to avoid the display showing random pixels (colored snow).
+	 */
+	jdi_wait_frames(jdi, 1);
+
+	backlight_enable(jdi->backlight);
+
+	jdi->enabled = true;
+
+	return 0;
+}
+
+static const struct drm_display_mode default_mode = {
+	.clock = 331334,
+	.hdisplay = 2560,
+	.hsync_start = 2560 + 80,
+	.hsync_end = 2560 + 80 + 80,
+	.htotal = 2560 + 80 + 80 + 80,
+	.vdisplay = 1800,
+	.vsync_start = 1800 + 4,
+	.vsync_end = 1800 + 4 + 4,
+	.vtotal = 1800 + 4 + 4 + 4,
+	.flags = 0,
+};
+
+static int jdi_panel_get_modes(struct drm_panel *panel,
+			       struct drm_connector *connector)
+{
+	struct drm_display_mode *mode;
+	struct jdi_panel *jdi = to_panel_jdi(panel);
+	struct device *dev = &jdi->link1->dev;
+
+	mode = drm_mode_duplicate(connector->dev, &default_mode);
+	if (!mode) {
+		dev_err(dev, "failed to add mode %ux%ux@%u\n",
+			default_mode.hdisplay, default_mode.vdisplay,
+			drm_mode_vrefresh(&default_mode));
+		return -ENOMEM;
+	}
+
+	drm_mode_set_name(mode);
+
+	drm_mode_probed_add(connector, mode);
+
+	connector->display_info.width_mm = 211;
+	connector->display_info.height_mm = 148;
+	connector->display_info.bpc = 8;
+
+	return 1;
+}
+
+static const struct drm_panel_funcs jdi_panel_funcs = {
+	.prepare = jdi_panel_prepare,
+	.enable = jdi_panel_enable,
+	.disable = jdi_panel_disable,
+	.unprepare = jdi_panel_unprepare,
+	.get_modes = jdi_panel_get_modes,
+};
+
+static const struct of_device_id jdi_of_match[] = {
+	{ .compatible = "jdi,lpm102a188a", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, jdi_of_match);
+
+static int jdi_panel_add(struct jdi_panel *jdi)
+{
+	struct device *dev = &jdi->link1->dev;
+
+	jdi->mode = &default_mode;
+
+	jdi->supply = devm_regulator_get(dev, "power");
+	if (IS_ERR(jdi->supply))
+		return dev_err_probe(dev, PTR_ERR(jdi->supply),
+				     "failed to get power regulator\n");
+
+	jdi->ddi_supply = devm_regulator_get(dev, "ddi");
+	if (IS_ERR(jdi->ddi_supply))
+		return dev_err_probe(dev, PTR_ERR(jdi->ddi_supply),
+				     "failed to get ddi regulator\n");
+
+	jdi->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(jdi->reset_gpio))
+		return dev_err_probe(dev, PTR_ERR(jdi->reset_gpio),
+				     "failed to get reset gpio\n");
+	usleep_range(1000, 3000);
+
+	jdi->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
+	if (IS_ERR(jdi->enable_gpio))
+		return dev_err_probe(dev, PTR_ERR(jdi->enable_gpio),
+				     "failed to get enable gpio\n");
+	usleep_range(2000, 4000);
+
+	jdi->backlight = devm_of_find_backlight(dev);
+	if (IS_ERR(jdi->backlight))
+		return dev_err_probe(dev, PTR_ERR(jdi->backlight),
+				     "failed to create backlight\n");
+
+	drm_panel_init(&jdi->base, &jdi->link1->dev, &jdi_panel_funcs,
+		       DRM_MODE_CONNECTOR_DSI);
+
+	drm_panel_add(&jdi->base);
+
+	return 0;
+}
+
+static void jdi_panel_del(struct jdi_panel *jdi)
+{
+	if (jdi->base.dev)
+		drm_panel_remove(&jdi->base);
+
+	if (jdi->link2)
+		put_device(&jdi->link2->dev);
+}
+
+static int jdi_panel_dsi_probe(struct mipi_dsi_device *dsi)
+{
+	struct mipi_dsi_device *secondary = NULL;
+	struct jdi_panel *jdi;
+	struct device_node *np;
+	int err;
+
+	dsi->lanes = 4;
+	dsi->format = MIPI_DSI_FMT_RGB888;
+	dsi->mode_flags = 0;
+
+	/* Find DSI-LINK1 */
+	np = of_parse_phandle(dsi->dev.of_node, "link2", 0);
+	if (np) {
+		secondary = of_find_mipi_dsi_device_by_node(np);
+		of_node_put(np);
+
+		if (!secondary)
+			return -EPROBE_DEFER;
+	}
+
+	/* register a panel for only the DSI-LINK1 interface */
+	if (secondary) {
+		jdi = devm_kzalloc(&dsi->dev, sizeof(*jdi), GFP_KERNEL);
+		if (!jdi) {
+			put_device(&secondary->dev);
+			return -ENOMEM;
+		}
+
+		mipi_dsi_set_drvdata(dsi, jdi);
+
+		jdi->link1 = dsi;
+		jdi->link2 = secondary;
+
+		err = jdi_panel_add(jdi);
+		if (err < 0) {
+			put_device(&secondary->dev);
+			return err;
+		}
+	}
+
+	err = mipi_dsi_attach(dsi);
+	if (err < 0) {
+		if (secondary)
+			jdi_panel_del(jdi);
+
+		return err;
+	}
+
+	return 0;
+}
+
+static int jdi_panel_dsi_remove(struct mipi_dsi_device *dsi)
+{
+	struct jdi_panel *jdi = mipi_dsi_get_drvdata(dsi);
+	int err;
+
+	/* only detach from host for the DSI-LINK2 interface */
+	if (!jdi) {
+		mipi_dsi_detach(dsi);
+		return 0;
+	}
+
+	err = jdi_panel_disable(&jdi->base);
+	if (err < 0)
+		dev_err(&dsi->dev, "failed to disable panel: %d\n", err);
+
+	err = mipi_dsi_detach(dsi);
+	if (err < 0)
+		dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", err);
+
+	jdi_panel_del(jdi);
+
+	return 0;
+}
+
+static void jdi_panel_dsi_shutdown(struct mipi_dsi_device *dsi)
+{
+	struct jdi_panel *jdi = mipi_dsi_get_drvdata(dsi);
+
+	if (!jdi)
+		return;
+
+	jdi_panel_disable(&jdi->base);
+}
+
+static struct mipi_dsi_driver jdi_panel_dsi_driver = {
+	.driver = {
+		.name = "panel-jdi-lpm102a188a-dsi",
+		.of_match_table = jdi_of_match,
+	},
+	.probe = jdi_panel_dsi_probe,
+	.remove = jdi_panel_dsi_remove,
+	.shutdown = jdi_panel_dsi_shutdown,
+};
+module_mipi_dsi_driver(jdi_panel_dsi_driver);
+
+MODULE_AUTHOR("Sean Paul <seanpaul@chromium.org>");
+MODULE_AUTHOR("Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>");
+MODULE_DESCRIPTION("DRM Driver for JDI LPM102A188A DSI panel, command mode");
+MODULE_LICENSE("GPL");
-- 
2.37.3

[PATCH 4/4] arm64: dts: smaug: Add display panel node

[Diogo Ivo]

The Google Pixel C has a JDI LPM102A188A display panel. Add a
DT node for it. Tested on Pixel C.

Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
---
 arch/arm64/boot/dts/nvidia/tegra210-smaug.dts | 72 +++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts b/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
index 20d092812984..271ef70747f1 100644
--- a/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
+++ b/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
@@ -31,6 +31,39 @@ memory {
 	};
 
 	host1x@50000000 {
+		dc@54200000 {
+			status = "okay";
+		};
+
+		dsia: dsi@54300000 {
+			avdd-dsi-csi-supply = <&vdd_dsi_csi>;
+			nvidia,boot-on;
+			status = "okay";
+
+			link2: panel@0 {
+				compatible = "jdi,lpm102a188a";
+				reg = <0>;
+			};
+		};
+
+		dsib: dsi@54400000 {
+			avdd-dsi-csi-supply = <&vdd_dsi_csi>;
+			nvidia,ganged-mode = <&dsia>;
+			nvidia,boot-on;
+			status = "okay";
+
+			link1: panel@0 {
+				compatible = "jdi,lpm102a188a";
+				reg = <0>;
+				power-supply = <&pplcd_vdd>;
+				ddi-supply = <&pp1800_lcdio>;
+				enable-gpios = <&gpio TEGRA_GPIO(V, 1) GPIO_ACTIVE_HIGH>;
+				reset-gpios = <&gpio TEGRA_GPIO(V, 2) GPIO_ACTIVE_LOW>;
+				link2 = <&link2>;
+				backlight = <&backlight>;
+			};
+		};
+
 		dpaux: dpaux@545c0000 {
 			status = "okay";
 		};
@@ -1627,6 +1660,37 @@ nau8825@1a {
 			status = "okay";
 		};
 
+		backlight: lp8557-backlight@2c {
+			compatible = "ti,lp8557";
+			reg = <0x2c>;
+			power-supply = <&pplcd_vdd>;
+			enable-supply = <&pp1800_lcdio>;
+			bl-name = "lp8557-backlight";
+			dev-ctrl = /bits/ 8 <0x01>;
+			init-brt = /bits/ 8 <0x80>;
+
+			/* Full scale current, 20mA */
+			rom_11h {
+				rom-addr = /bits/ 8 <0x11>;
+				rom-val = /bits/ 8 <0x05>;
+			};
+			/* Frequency = 4.9kHz, magic undocumented val */
+			rom_12h {
+				rom-addr = /bits/ 8 <0x12>;
+				rom-val = /bits/ 8 <0x29>;
+			};
+			/* Boost freq = 1MHz, BComp option = 1 */
+			rom_13h {
+				rom-addr = /bits/ 8 <0x13>;
+				rom-val = /bits/ 8 <0x03>;
+			};
+			/* 4V OV, 6 output LED string enabled */
+			rom_14h {
+				rom-addr = /bits/ 8 <0x14>;
+				rom-val = /bits/ 8 <0xbf>;
+			};
+		};
+
 		audio-codec@2d {
 			compatible = "realtek,rt5677";
 			reg = <0x2d>;
@@ -1908,4 +1972,12 @@ usbc_vbus: regulator-usbc-vbus {
 		regulator-min-microvolt = <5000000>;
 		regulator-max-microvolt = <5000000>;
 	};
+
+	vdd_dsi_csi: regulator-vdd-dsi-csi {
+		compatible = "regulator-fixed";
+		regulator-name = "AVDD_DSI_CSI_1V2";
+		regulator-min-microvolt = <1200000>;
+		regulator-max-microvolt = <1200000>;
+		vin-supply = <&pp1200_avdd>;
+	};
 };
-- 
2.37.3

Re: [PATCH 4/4] arm64: dts: smaug: Add display panel node

[Krzysztof Kozlowski]

On 29/09/2022 19:05, Diogo Ivo wrote:
> The Google Pixel C has a JDI LPM102A188A display panel. Add a
> DT node for it. Tested on Pixel C.
> 
> Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
> ---
>  arch/arm64/boot/dts/nvidia/tegra210-smaug.dts | 72 +++++++++++++++++++
>  1 file changed, 72 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts b/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
> index 20d092812984..271ef70747f1 100644
> --- a/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
> +++ b/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
> @@ -31,6 +31,39 @@ memory {
>  	};
>  
>  	host1x@50000000 {
> +		dc@54200000 {
> +			status = "okay";

You should override by labels, not by full path.

> +		};
> +
> +		dsia: dsi@54300000 {
> +			avdd-dsi-csi-supply = <&vdd_dsi_csi>;
> +			nvidia,boot-on;
> +			status = "okay";
> +
> +			link2: panel@0 {
> +				compatible = "jdi,lpm102a188a";
> +				reg = <0>;
> +			};
> +		};
> +
> +		dsib: dsi@54400000 {
> +			avdd-dsi-csi-supply = <&vdd_dsi_csi>;
> +			nvidia,ganged-mode = <&dsia>;
> +			nvidia,boot-on;
> +			status = "okay";
> +
> +			link1: panel@0 {
> +				compatible = "jdi,lpm102a188a";
> +				reg = <0>;
> +				power-supply = <&pplcd_vdd>;
> +				ddi-supply = <&pp1800_lcdio>;
> +				enable-gpios = <&gpio TEGRA_GPIO(V, 1) GPIO_ACTIVE_HIGH>;
> +				reset-gpios = <&gpio TEGRA_GPIO(V, 2) GPIO_ACTIVE_LOW>;
> +				link2 = <&link2>;
> +				backlight = <&backlight>;
> +			};
> +		};
> +
>  		dpaux: dpaux@545c0000 {
>  			status = "okay";
>  		};
> @@ -1627,6 +1660,37 @@ nau8825@1a {
>  			status = "okay";
>  		};
>  
> +		backlight: lp8557-backlight@2c {

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

> +			compatible = "ti,lp8557";
> +			reg = <0x2c>;
> +			power-supply = <&pplcd_vdd>;
> +			enable-supply = <&pp1800_lcdio>;
> +			bl-name = "lp8557-backlight";
> +			dev-ctrl = /bits/ 8 <0x01>;
> +			init-brt = /bits/ 8 <0x80>;
> +
> +			/* Full scale current, 20mA */
> +			rom_11h {

No underscores in node names, unless something requires it?

> +				rom-addr = /bits/ 8 <0x11>;
> +				rom-val = /bits/ 8 <0x05>;
> +			};

Best regards,
Krzysztof

Re: [PATCH 4/4] arm64: dts: smaug: Add display panel node

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1231 bytes --]

On Fri, Sep 30, 2022 at 12:51:07PM +0200, Krzysztof Kozlowski wrote:
> On 29/09/2022 19:05, Diogo Ivo wrote:
> > The Google Pixel C has a JDI LPM102A188A display panel. Add a
> > DT node for it. Tested on Pixel C.
> > 
> > Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
> > ---
> >  arch/arm64/boot/dts/nvidia/tegra210-smaug.dts | 72 +++++++++++++++++++
> >  1 file changed, 72 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts b/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
> > index 20d092812984..271ef70747f1 100644
> > --- a/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
> > +++ b/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
> > @@ -31,6 +31,39 @@ memory {
> >  	};
> >  
> >  	host1x@50000000 {
> > +		dc@54200000 {
> > +			status = "okay";
> 
> You should override by labels, not by full path.

Why exactly is that? I've always stayed away from that (and asked others
not to do so, at least on Tegra) because I find it impossible to parse
for my human brain. Replicating the original full hierarchy makes it
much more obvious to me where the changes are happening than the
spaghetti-like mess that you get from overriding by label reference.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/4] arm64: dts: smaug: Add display panel node

[Krzysztof Kozlowski]

On 30/09/2022 13:15, Thierry Reding wrote:
> On Fri, Sep 30, 2022 at 12:51:07PM +0200, Krzysztof Kozlowski wrote:
>> On 29/09/2022 19:05, Diogo Ivo wrote:
>>> The Google Pixel C has a JDI LPM102A188A display panel. Add a
>>> DT node for it. Tested on Pixel C.
>>>
>>> Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
>>> ---
>>>  arch/arm64/boot/dts/nvidia/tegra210-smaug.dts | 72 +++++++++++++++++++
>>>  1 file changed, 72 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts b/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
>>> index 20d092812984..271ef70747f1 100644
>>> --- a/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
>>> +++ b/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
>>> @@ -31,6 +31,39 @@ memory {
>>>  	};
>>>  
>>>  	host1x@50000000 {
>>> +		dc@54200000 {
>>> +			status = "okay";
>>
>> You should override by labels, not by full path.
> 
> Why exactly is that? I've always stayed away from that (and asked others
> not to do so, at least on Tegra) because I find it impossible to parse
> for my human brain. Replicating the original full hierarchy makes it
> much more obvious to me where the changes are happening than the
> spaghetti-like mess that you get from overriding by label reference.

Sure, it's entirely up to you. I forgot your preference.

But it is a really nice way to have duplicated nodes and mistakes (which
happen from time to time).

Best regards,
Krzysztof

Re: [PATCH 4/4] arm64: dts: smaug: Add display panel node

[Rob Herring]

On Fri, Sep 30, 2022 at 01:20:50PM +0200, Krzysztof Kozlowski wrote:
> On 30/09/2022 13:15, Thierry Reding wrote:
> > On Fri, Sep 30, 2022 at 12:51:07PM +0200, Krzysztof Kozlowski wrote:
> >> On 29/09/2022 19:05, Diogo Ivo wrote:
> >>> The Google Pixel C has a JDI LPM102A188A display panel. Add a
> >>> DT node for it. Tested on Pixel C.
> >>>
> >>> Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
> >>> ---
> >>>  arch/arm64/boot/dts/nvidia/tegra210-smaug.dts | 72 +++++++++++++++++++
> >>>  1 file changed, 72 insertions(+)
> >>>
> >>> diff --git a/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts b/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
> >>> index 20d092812984..271ef70747f1 100644
> >>> --- a/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
> >>> +++ b/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
> >>> @@ -31,6 +31,39 @@ memory {
> >>>  	};
> >>>  
> >>>  	host1x@50000000 {
> >>> +		dc@54200000 {
> >>> +			status = "okay";
> >>
> >> You should override by labels, not by full path.
> > 
> > Why exactly is that? I've always stayed away from that (and asked others
> > not to do so, at least on Tegra) because I find it impossible to parse
> > for my human brain. Replicating the original full hierarchy makes it
> > much more obvious to me where the changes are happening than the
> > spaghetti-like mess that you get from overriding by label reference.
> 
> Sure, it's entirely up to you. I forgot your preference.
> 
> But it is a really nice way to have duplicated nodes and mistakes (which
> happen from time to time).

We could have a schema or dtc check for that. We already warn for 
duplicate unit-addresses which would catch some typos. Checking for a 
node with only 'status' would probably work when that's the only 
addition. Maybe status without a compatible would be better? We also 
check for nodes without a specific schema, but child nodes in schemas 
aren't handled.

Rob

Re: [PATCH 4/4] arm64: dts: smaug: Add display panel node

[Krzysztof Kozlowski]

On 30/09/2022 23:14, Rob Herring wrote:
>>>>> +		dc@54200000 {
>>>>> +			status = "okay";
>>>>
>>>> You should override by labels, not by full path.
>>>
>>> Why exactly is that? I've always stayed away from that (and asked others
>>> not to do so, at least on Tegra) because I find it impossible to parse
>>> for my human brain. Replicating the original full hierarchy makes it
>>> much more obvious to me where the changes are happening than the
>>> spaghetti-like mess that you get from overriding by label reference.
>>
>> Sure, it's entirely up to you. I forgot your preference.
>>
>> But it is a really nice way to have duplicated nodes and mistakes (which
>> happen from time to time).
> 
> We could have a schema or dtc check for that. We already warn for 
> duplicate unit-addresses which would catch some typos. Checking for a 
> node with only 'status' would probably work when that's the only 
> addition. Maybe status without a compatible would be better? We also 
> check for nodes without a specific schema, but child nodes in schemas 
> aren't handled.

Usually these are overrides of few properties and status=okay, so
looking for nodes without a compatible would work. Except for all the
cases where we do not have schema yet...

Best regards,
Krzysztof