[PATCH 1/6] dt-bindings: drm/panel: simple: Add no-hpd property

[PATCH 1/6] dt-bindings: drm/panel: simple: Add no-hpd property

[Douglas Anderson]

Some eDP panels that are designed to be always connected to a board
use their HPD signal to signal that they've finished powering on and
they're ready to be talked to.

However, for various reasons it's possible that the HPD signal from
the panel isn't actually hooked up.  In the case where the HPD isn't
hooked up you can look at the timing diagram on the panel datasheet
and insert a delay for the maximum amount of time that the HPD might
take to come up.

Let's add a property in the device tree for this concept.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 .../devicetree/bindings/display/panel/simple-panel.txt         | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/panel/simple-panel.txt b/Documentation/devicetree/bindings/display/panel/simple-panel.txt
index 45a457ad38f0..b2b872c710f2 100644
--- a/Documentation/devicetree/bindings/display/panel/simple-panel.txt
+++ b/Documentation/devicetree/bindings/display/panel/simple-panel.txt
@@ -11,6 +11,9 @@ Optional properties:
 - ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
 - enable-gpios: GPIO pin to enable or disable the panel
 - backlight: phandle of the backlight device attached to the panel
+- no-hpd: This panel is supposed to communicate that it's ready via HPD
+  (hot plug detect) signal, but the signal isn't hooked up so we should
+  hardcode the max delay from the panel spec when powering up the panel.
 
 Example:
 
-- 
2.19.1.568.g152ad8e336-goog

[PATCH 2/6] drm/panel: simple: Support panels with HPD where HPD isn't connected

[Douglas Anderson]

Some eDP panels that are designed to be always connected to a board
use their HPD signal to signal that they've finished powering on and
they're ready to be talked to.

However, for various reasons it's possible that the HPD signal from
the panel isn't actually hooked up.  In the case where the HPD isn't
hooked up you can look at the timing diagram on the panel datasheet
and insert a delay for the maximum amount of time that the HPD might
take to come up.

Let's add support in simple-panel for this concept.

At the moment we will co-opt the existing "prepare" delay to keep
track of the delay and we'll use a boolean to specify that a given
panel should only apply the delay if the "no-hpd" property was
specified.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/gpu/drm/panel/panel-simple.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 97964f7f2ace..38c646fb55fd 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -63,12 +63,15 @@ struct panel_desc {
 	 *           turn the display off (no content is visible)
 	 * @unprepare: the time (in milliseconds) that it takes for the panel
 	 *             to power itself down completely
+	 * @prepare_delay_only_if_no_hpd: The prepare delay should only be done
+	 *                                if we know Hot Plug Detect isn't used.
 	 */
 	struct {
 		unsigned int prepare;
 		unsigned int enable;
 		unsigned int disable;
 		unsigned int unprepare;
+		bool prepare_delay_only_if_no_hpd;
 	} delay;
 
 	u32 bus_format;
@@ -79,6 +82,7 @@ struct panel_simple {
 	struct drm_panel base;
 	bool prepared;
 	bool enabled;
+	bool no_hpd;
 
 	const struct panel_desc *desc;
 
@@ -215,7 +219,8 @@ static int panel_simple_prepare(struct drm_panel *panel)
 
 	gpiod_set_value_cansleep(p->enable_gpio, 1);
 
-	if (p->desc->delay.prepare)
+	if (p->desc->delay.prepare &&
+	    (!p->desc->delay.prepare_delay_only_if_no_hpd || p->no_hpd))
 		msleep(p->desc->delay.prepare);
 
 	p->prepared = true;
@@ -305,6 +310,8 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
 	panel->prepared = false;
 	panel->desc = desc;
 
+	panel->no_hpd = of_property_read_bool(dev->of_node, "no-hpd");
+
 	panel->supply = devm_regulator_get(dev, "power");
 	if (IS_ERR(panel->supply))
 		return PTR_ERR(panel->supply);
-- 
2.19.1.568.g152ad8e336-goog

[PATCH 3/6] drm/panel: simple: Add "no-hpd" delay for Innolux TV123WAM

If the HPD signal isn't hooked up to this panel we need a 200 ms
delay.  In the datasheet this is shown as the maximum time that HPD
will take to be asserted after power is given to the panel.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/gpu/drm/panel/panel-simple.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 38c646fb55fd..937e97490c30 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -1393,6 +1393,8 @@ static const struct panel_desc innolux_tv123wam = {
 		.height = 173,
 	},
 	.delay = {
+		.prepare = 200,
+		.prepare_delay_only_if_no_hpd = true,
 		.unprepare = 500,
 	},
 };
-- 
2.19.1.568.g152ad8e336-goog

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/6] drm/panel: simple: Add "no-hpd" delay for Innolux TV123WAM

[Douglas Anderson]

If the HPD signal isn't hooked up to this panel we need a 200 ms
delay.  In the datasheet this is shown as the maximum time that HPD
will take to be asserted after power is given to the panel.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/gpu/drm/panel/panel-simple.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 38c646fb55fd..937e97490c30 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -1393,6 +1393,8 @@ static const struct panel_desc innolux_tv123wam = {
 		.height = 173,
 	},
 	.delay = {
+		.prepare = 200,
+		.prepare_delay_only_if_no_hpd = true,
 		.unprepare = 500,
 	},
 };
-- 
2.19.1.568.g152ad8e336-goog

[PATCH 4/6] drm/bridge: ti-sn65dsi86: Remove the mystery delay

[Douglas Anderson]

Let's solve the mystery of commit bf1178c98930 ("drm/bridge:
ti-sn65dsi86: Add mystery delay to enable()").  Specifically the
reason we needed that mystery delay is that we weren't paying
attention to HPD.

Looking at the datasheet for the same panel that was tested for the
original commit, I see there's a timing "t3" that times from power on
to the aux channel being operational.  This time is specced as 0 - 200
ms.  The datasheet says that the aux channel is operational at exactly
the same time that HPD is asserted.

Scoping the signals on this board showed that HPD was asserted 84 ms
after power was asserted.  That very closely matches the magic 70 ms
delay that we had.  ...and actually, in my testing the 70 ms wasn't
quite enough of a delay and some percentage of the time the display
didn't come up until I bumped it to 100 ms (presumably 84 ms would
have worked too).

To solve this, we tried to hook up the HPD signal in the bridge.
...but in doing so we found that that the bridge didn't report that
HPD was asserted until ~280 ms after we powered it (!).  This is
explained by looking at the sn65dsi86 datasheet section "8.4.5.1 HPD
(Hot Plug/Unplug Detection)".  Reading there we see that the bridge
isn't even intended to report HPD until 100 ms after it's asserted.
...but that would have left us at 184 ms.  The extra 100 ms
(presumably) comes from this part in the datasheet:

> The HPD state machine operates off an internal ring oscillator. The
> ring oscillator frequency will vary [ ... ]. The min/max range in
> the HPD State Diagram refers to the possible times based off
> variation in the ring oscillator frequency.

Given that the 280 ms we'll end up delaying if we hook up HPD is
_slower_ than the 200 ms we could just hardcode, for now we'll solve
the problem by just hardcoding a 200 ms delay in the panel driver
using the patch in this series ("drm/panel: simple: Support panels
with HPD where HPD isn't connected").

If we later find a panel that needs to use this bridge where we need
HPD then we'll have to come up with some new code to handle it.  Given
the silly debouncing in the bridge chip, though, it seems unlikely.

One last note is that I tried to solve this through another way: In
ti_sn_bridge_enable() I tried to use various combinations of
dp_dpcd_writeb() and dp_dpcd_readb() to detect when the aux channel
was up.  In theory that would let me detect _exactly_ when I could
continue and do link training.  Unfortunately even if I did an aux
transfer w/out waiting I couldn't see any errors.  Possibly I could
keep looping over link training until it came back with success, but
that seemed a little overly hacky to me.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 29 +++++++++++++++------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index f8a931cf3665..680566d97adc 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -458,18 +458,6 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
 	unsigned int val;
 	int ret;
 
-	/*
-	 * FIXME:
-	 * This 70ms was found necessary by experimentation. If it's not
-	 * present, link training fails. It seems like it can go anywhere from
-	 * pre_enable() up to semi-auto link training initiation below.
-	 *
-	 * Neither the datasheet for the bridge nor the panel tested mention a
-	 * delay of this magnitude in the timing requirements. So for now, add
-	 * the mystery delay until someone figures out a better fix.
-	 */
-	msleep(70);
-
 	/* DSI_A lane config */
 	val = CHA_DSI_LANES(4 - pdata->dsi->lanes);
 	regmap_update_bits(pdata->regmap, SN_DSI_LANES_REG,
@@ -536,7 +524,22 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
 	/* configure bridge ref_clk */
 	ti_sn_bridge_set_refclk_freq(pdata);
 
-	/* in case drm_panel is connected then HPD is not supported */
+	/*
+	 * HPD on this bridge chip is a bit useless.  This is an eDP bridge
+	 * so the HPD is an internal signal that's only there to signal that
+	 * the panel is done powering up.  ...but the bridge chip debounces
+	 * this signal by between 100 ms and 400 ms (depending on process,
+	 * voltage, and temperate--I measured it at about 200 ms).  One
+	 * particular panel asserted HPD 84 ms after it was powered on meaning
+	 * that we saw HPD 284 ms after power on.  ...but the same panel said
+	 * that instead of looking at HPD you could just hardcode a delay of
+	 * 200 ms.  We'll assume that the panel driver will have the hardcoded
+	 * delay in its prepare and always disable HPD.
+	 *
+	 * If HPD somehow makes sense on some future panel we'll have to
+	 * change this to be conditional on someone specifying that HPD should
+	 * be used.
+	 */
 	regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
 			   HPD_DISABLE);
 
-- 
2.19.1.568.g152ad8e336-goog

[PATCH 5/6] drm/panel: simple: Innolux TV123WAM is actually P120ZDG-BF1

[Douglas Anderson]

As far as I can tell the panel that was added in commit da50bd4258db
("drm/panel: simple: Add Innolux TV123WAM panel driver support")
wasn't actually an Innolux TV123WAM but was actually an Innolux
P120ZDG-BF1.

As far as I can tell the Innolux TV123WAM isn't a real panel and but
it's a mosh between the TI TV123WAM and the Innolux P120ZDG-BF1.
Let's unmosh.

Here's my evidence:

* Searching for TV123WAM on the Internet turns up a TI panel.  While
  it's possible that an Innolux panel has the same model number as the
  TI Panel, it seems a little doubtful.  Looking up the datasheet from
  the TI Panel shows that it's 1920 x 1280 and 259.2 mm x 172.8 mm.

* As far as I know, the patch adding the Innolux Panel was supposed to
  be for the board that's sitting in front of me as I type this
  (support for that board is not yet upstream).  On the back of that
  panel I see Innolux P120ZDZ-EZ1 rev B1.

* Someone pointed me at a datasheet that's supposed to be for the
  panel in front of me (sorry, I can't share the datasheet).  That
  datasheet has the string "p120zdg-bf1"

* If I search for "P120ZDG-BF1" on the Internet I get hits for panels
  that are 2160x1440.  They don't have datasheets, but the fact that
  the resolution matches is a good sign.

In any case, let's update the name and also the physical size to match
the correct panel.

Fixes: da50bd4258db ("drm/panel: simple: Add Innolux TV123WAM panel driver support")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Cc: Sandeep Panda <spanda@codeaurora.org>
---

 drivers/gpu/drm/panel/panel-simple.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 937e97490c30..7ee1abc5d81b 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -1370,7 +1370,7 @@ static const struct panel_desc innolux_n156bge_l21 = {
 	},
 };
 
-static const struct drm_display_mode innolux_tv123wam_mode = {
+static const struct drm_display_mode innolux_p120zdg_bf1_mode = {
 	.clock = 206016,
 	.hdisplay = 2160,
 	.hsync_start = 2160 + 48,
@@ -1384,13 +1384,13 @@ static const struct drm_display_mode innolux_tv123wam_mode = {
 	.flags = DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC,
 };
 
-static const struct panel_desc innolux_tv123wam = {
-	.modes = &innolux_tv123wam_mode,
+static const struct panel_desc innolux_p120zdg_bf1 = {
+	.modes = &innolux_p120zdg_bf1_mode,
 	.num_modes = 1,
 	.bpc = 8,
 	.size = {
-		.width = 259,
-		.height = 173,
+		.width = 254,
+		.height = 169,
 	},
 	.delay = {
 		.prepare = 200,
@@ -2454,8 +2454,8 @@ static const struct of_device_id platform_of_match[] = {
 		.compatible = "innolux,n156bge-l21",
 		.data = &innolux_n156bge_l21,
 	}, {
-		.compatible = "innolux,tv123wam",
-		.data = &innolux_tv123wam,
+		.compatible = "innolux,p120zdg-bf1",
+		.data = &innolux_p120zdg_bf1,
 	}, {
 		.compatible = "innolux,zj070na-01p",
 		.data = &innolux_zj070na_01p,
-- 
2.19.1.568.g152ad8e336-goog

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/6] drm/panel: simple: Innolux TV123WAM is actually P120ZDG-BF1

[Douglas Anderson]

As far as I can tell the panel that was added in commit da50bd4258db
("drm/panel: simple: Add Innolux TV123WAM panel driver support")
wasn't actually an Innolux TV123WAM but was actually an Innolux
P120ZDG-BF1.

As far as I can tell the Innolux TV123WAM isn't a real panel and but
it's a mosh between the TI TV123WAM and the Innolux P120ZDG-BF1.
Let's unmosh.

Here's my evidence:

* Searching for TV123WAM on the Internet turns up a TI panel.  While
  it's possible that an Innolux panel has the same model number as the
  TI Panel, it seems a little doubtful.  Looking up the datasheet from
  the TI Panel shows that it's 1920 x 1280 and 259.2 mm x 172.8 mm.

* As far as I know, the patch adding the Innolux Panel was supposed to
  be for the board that's sitting in front of me as I type this
  (support for that board is not yet upstream).  On the back of that
  panel I see Innolux P120ZDZ-EZ1 rev B1.

* Someone pointed me at a datasheet that's supposed to be for the
  panel in front of me (sorry, I can't share the datasheet).  That
  datasheet has the string "p120zdg-bf1"

* If I search for "P120ZDG-BF1" on the Internet I get hits for panels
  that are 2160x1440.  They don't have datasheets, but the fact that
  the resolution matches is a good sign.

In any case, let's update the name and also the physical size to match
the correct panel.

Fixes: da50bd4258db ("drm/panel: simple: Add Innolux TV123WAM panel driver support")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Cc: Sandeep Panda <spanda@codeaurora.org>
---

 drivers/gpu/drm/panel/panel-simple.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 937e97490c30..7ee1abc5d81b 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -1370,7 +1370,7 @@ static const struct panel_desc innolux_n156bge_l21 = {
 	},
 };
 
-static const struct drm_display_mode innolux_tv123wam_mode = {
+static const struct drm_display_mode innolux_p120zdg_bf1_mode = {
 	.clock = 206016,
 	.hdisplay = 2160,
 	.hsync_start = 2160 + 48,
@@ -1384,13 +1384,13 @@ static const struct drm_display_mode innolux_tv123wam_mode = {
 	.flags = DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC,
 };
 
-static const struct panel_desc innolux_tv123wam = {
-	.modes = &innolux_tv123wam_mode,
+static const struct panel_desc innolux_p120zdg_bf1 = {
+	.modes = &innolux_p120zdg_bf1_mode,
 	.num_modes = 1,
 	.bpc = 8,
 	.size = {
-		.width = 259,
-		.height = 173,
+		.width = 254,
+		.height = 169,
 	},
 	.delay = {
 		.prepare = 200,
@@ -2454,8 +2454,8 @@ static const struct of_device_id platform_of_match[] = {
 		.compatible = "innolux,n156bge-l21",
 		.data = &innolux_n156bge_l21,
 	}, {
-		.compatible = "innolux,tv123wam",
-		.data = &innolux_tv123wam,
+		.compatible = "innolux,p120zdg-bf1",
+		.data = &innolux_p120zdg_bf1,
 	}, {
 		.compatible = "innolux,zj070na-01p",
 		.data = &innolux_zj070na_01p,
-- 
2.19.1.568.g152ad8e336-goog

[PATCH 6/6] dt-bindings: drm/panel: simple: Innolux TV123WAM is actually P120ZDG-BF1

[Douglas Anderson]

As far as I can tell the bindings that were added in commit
9c04400f7ea6 ("dt-bindings: drm/panel: Document Innolux TV123WAM panel
bindings") weren't actually for Innolux TV123WAM but were actually for
Innolux P120ZDG-BF1.

As far as I can tell the Innolux TV123WAM isn't a real panel and but
it's a mosh between the TI TV123WAM and the Innolux P120ZDG-BF1.
Let's unmosh.

Here's my evidence:

* Searching for TV123WAM on the Internet turns up a TI panel.  While
  it's possible that an Innolux panel has the same model number as the
  TI Panel, it seems a little doubtful.  Looking up the datasheet from
  the TI Panel shows that it's 1920 x 1280 and 259.2 mm x 172.8 mm.

* As far as I know, the patch adding the Innolux Panel was supposed to
  be for the board that's sitting in front of me as I type this
  (support for that board is not yet upstream).  On the back of that
  panel I see Innolux P120ZDZ-EZ1 rev B1.

* Someone pointed me at a datasheet that's supposed to be for the
  panel in front of me (sorry, I can't share the datasheet).  That
  datasheet has the string "p120zdg-bf1"

* If I search for "P120ZDG-BF1" on the Internet I get hits for panels
  that are 2160x1440.  They don't have datasheets, but the fact that
  the resolution matches is a good sign.

While we doing the rename, also mention that no-hpd can be used with
this panel.  See the previous patch in this series ("drm/panel:
simple: Add "no-hpd" delay for Innolux TV123WAM").

Fixes: 9c04400f7ea6 ("dt-bindings: drm/panel: Document Innolux TV123WAM panel bindings")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Cc: Sandeep Panda <spanda@codeaurora.org>
---

 .../{innolux,tv123wam.txt => innolux,p120zdg-bf1.txt}     | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
 rename Documentation/devicetree/bindings/display/panel/{innolux,tv123wam.txt => innolux,p120zdg-bf1.txt} (70%)

diff --git a/Documentation/devicetree/bindings/display/panel/innolux,tv123wam.txt b/Documentation/devicetree/bindings/display/panel/innolux,p120zdg-bf1.txt
similarity index 70%
rename from Documentation/devicetree/bindings/display/panel/innolux,tv123wam.txt
rename to Documentation/devicetree/bindings/display/panel/innolux,p120zdg-bf1.txt
index a9b35265fa13..513f03466aba 100644
--- a/Documentation/devicetree/bindings/display/panel/innolux,tv123wam.txt
+++ b/Documentation/devicetree/bindings/display/panel/innolux,p120zdg-bf1.txt
@@ -1,20 +1,22 @@
-Innolux TV123WAM 12.3 inch eDP 2K display panel
+Innolux P120ZDG-BF1 12.02 inch eDP 2K display panel
 
 This binding is compatible with the simple-panel binding, which is specified
 in simple-panel.txt in this directory.
 
 Required properties:
-- compatible: should be "innolux,tv123wam"
+- compatible: should be "innolux,p120zdg-bf1"
 - power-supply: regulator to provide the supply voltage
 
 Optional properties:
 - enable-gpios: GPIO pin to enable or disable the panel
 - backlight: phandle of the backlight device attached to the panel
+- no-hpd: If HPD isn't hooked up; add this property.
 
 Example:
 	panel_edp: panel-edp {
-		compatible = "innolux,tv123wam";
+		compatible = "innolux,p120zdg-bf1";
 		enable-gpios = <&msmgpio 31 GPIO_ACTIVE_LOW>;
 		power-supply = <&pm8916_l2>;
 		backlight = <&backlight>;
+		no-hpd;
 	};
-- 
2.19.1.568.g152ad8e336-goog

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 6/6] dt-bindings: drm/panel: simple: Innolux TV123WAM is actually P120ZDG-BF1

[Douglas Anderson]

As far as I can tell the bindings that were added in commit
9c04400f7ea6 ("dt-bindings: drm/panel: Document Innolux TV123WAM panel
bindings") weren't actually for Innolux TV123WAM but were actually for
Innolux P120ZDG-BF1.

As far as I can tell the Innolux TV123WAM isn't a real panel and but
it's a mosh between the TI TV123WAM and the Innolux P120ZDG-BF1.
Let's unmosh.

Here's my evidence:

* Searching for TV123WAM on the Internet turns up a TI panel.  While
  it's possible that an Innolux panel has the same model number as the
  TI Panel, it seems a little doubtful.  Looking up the datasheet from
  the TI Panel shows that it's 1920 x 1280 and 259.2 mm x 172.8 mm.

* As far as I know, the patch adding the Innolux Panel was supposed to
  be for the board that's sitting in front of me as I type this
  (support for that board is not yet upstream).  On the back of that
  panel I see Innolux P120ZDZ-EZ1 rev B1.

* Someone pointed me at a datasheet that's supposed to be for the
  panel in front of me (sorry, I can't share the datasheet).  That
  datasheet has the string "p120zdg-bf1"

* If I search for "P120ZDG-BF1" on the Internet I get hits for panels
  that are 2160x1440.  They don't have datasheets, but the fact that
  the resolution matches is a good sign.

While we doing the rename, also mention that no-hpd can be used with
this panel.  See the previous patch in this series ("drm/panel:
simple: Add "no-hpd" delay for Innolux TV123WAM").

Fixes: 9c04400f7ea6 ("dt-bindings: drm/panel: Document Innolux TV123WAM panel bindings")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Cc: Sandeep Panda <spanda@codeaurora.org>
---

 .../{innolux,tv123wam.txt => innolux,p120zdg-bf1.txt}     | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
 rename Documentation/devicetree/bindings/display/panel/{innolux,tv123wam.txt => innolux,p120zdg-bf1.txt} (70%)

diff --git a/Documentation/devicetree/bindings/display/panel/innolux,tv123wam.txt b/Documentation/devicetree/bindings/display/panel/innolux,p120zdg-bf1.txt
similarity index 70%
rename from Documentation/devicetree/bindings/display/panel/innolux,tv123wam.txt
rename to Documentation/devicetree/bindings/display/panel/innolux,p120zdg-bf1.txt
index a9b35265fa13..513f03466aba 100644
--- a/Documentation/devicetree/bindings/display/panel/innolux,tv123wam.txt
+++ b/Documentation/devicetree/bindings/display/panel/innolux,p120zdg-bf1.txt
@@ -1,20 +1,22 @@
-Innolux TV123WAM 12.3 inch eDP 2K display panel
+Innolux P120ZDG-BF1 12.02 inch eDP 2K display panel
 
 This binding is compatible with the simple-panel binding, which is specified
 in simple-panel.txt in this directory.
 
 Required properties:
-- compatible: should be "innolux,tv123wam"
+- compatible: should be "innolux,p120zdg-bf1"
 - power-supply: regulator to provide the supply voltage
 
 Optional properties:
 - enable-gpios: GPIO pin to enable or disable the panel
 - backlight: phandle of the backlight device attached to the panel
+- no-hpd: If HPD isn't hooked up; add this property.
 
 Example:
 	panel_edp: panel-edp {
-		compatible = "innolux,tv123wam";
+		compatible = "innolux,p120zdg-bf1";
 		enable-gpios = <&msmgpio 31 GPIO_ACTIVE_LOW>;
 		power-supply = <&pm8916_l2>;
 		backlight = <&backlight>;
+		no-hpd;
 	};
-- 
2.19.1.568.g152ad8e336-goog

Re: [PATCH 1/6] dt-bindings: drm/panel: simple: Add no-hpd property

[Sean Paul]

On Mon, Oct 22, 2018 at 01:46:34PM -0700, Douglas Anderson wrote:
> Some eDP panels that are designed to be always connected to a board
> use their HPD signal to signal that they've finished powering on and
> they're ready to be talked to.
> 
> However, for various reasons it's possible that the HPD signal from
> the panel isn't actually hooked up.  In the case where the HPD isn't
> hooked up you can look at the timing diagram on the panel datasheet
> and insert a delay for the maximum amount of time that the HPD might
> take to come up.
> 
> Let's add a property in the device tree for this concept.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Sean Paul <sean@poorly.run>

> ---
> 
>  .../devicetree/bindings/display/panel/simple-panel.txt         | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/panel/simple-panel.txt b/Documentation/devicetree/bindings/display/panel/simple-panel.txt
> index 45a457ad38f0..b2b872c710f2 100644
> --- a/Documentation/devicetree/bindings/display/panel/simple-panel.txt
> +++ b/Documentation/devicetree/bindings/display/panel/simple-panel.txt
> @@ -11,6 +11,9 @@ Optional properties:
>  - ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
>  - enable-gpios: GPIO pin to enable or disable the panel
>  - backlight: phandle of the backlight device attached to the panel
> +- no-hpd: This panel is supposed to communicate that it's ready via HPD
> +  (hot plug detect) signal, but the signal isn't hooked up so we should
> +  hardcode the max delay from the panel spec when powering up the panel.
>  
>  Example:
>  
> -- 
> 2.19.1.568.g152ad8e336-goog
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/6] dt-bindings: drm/panel: simple: Add no-hpd property

[Sean Paul]

On Mon, Oct 22, 2018 at 01:46:34PM -0700, Douglas Anderson wrote:
> Some eDP panels that are designed to be always connected to a board
> use their HPD signal to signal that they've finished powering on and
> they're ready to be talked to.
> 
> However, for various reasons it's possible that the HPD signal from
> the panel isn't actually hooked up.  In the case where the HPD isn't
> hooked up you can look at the timing diagram on the panel datasheet
> and insert a delay for the maximum amount of time that the HPD might
> take to come up.
> 
> Let's add a property in the device tree for this concept.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Sean Paul <sean@poorly.run>

> ---
> 
>  .../devicetree/bindings/display/panel/simple-panel.txt         | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/panel/simple-panel.txt b/Documentation/devicetree/bindings/display/panel/simple-panel.txt
> index 45a457ad38f0..b2b872c710f2 100644
> --- a/Documentation/devicetree/bindings/display/panel/simple-panel.txt
> +++ b/Documentation/devicetree/bindings/display/panel/simple-panel.txt
> @@ -11,6 +11,9 @@ Optional properties:
>  - ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
>  - enable-gpios: GPIO pin to enable or disable the panel
>  - backlight: phandle of the backlight device attached to the panel
> +- no-hpd: This panel is supposed to communicate that it's ready via HPD
> +  (hot plug detect) signal, but the signal isn't hooked up so we should
> +  hardcode the max delay from the panel spec when powering up the panel.
>  
>  Example:
>  
> -- 
> 2.19.1.568.g152ad8e336-goog
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH 2/6] drm/panel: simple: Support panels with HPD where HPD isn't connected

[Sean Paul]

On Mon, Oct 22, 2018 at 01:46:35PM -0700, Douglas Anderson wrote:
> Some eDP panels that are designed to be always connected to a board
> use their HPD signal to signal that they've finished powering on and
> they're ready to be talked to.
> 
> However, for various reasons it's possible that the HPD signal from
> the panel isn't actually hooked up.  In the case where the HPD isn't
> hooked up you can look at the timing diagram on the panel datasheet
> and insert a delay for the maximum amount of time that the HPD might
> take to come up.
> 
> Let's add support in simple-panel for this concept.
> 
> At the moment we will co-opt the existing "prepare" delay to keep
> track of the delay and we'll use a boolean to specify that a given
> panel should only apply the delay if the "no-hpd" property was
> specified.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
>  drivers/gpu/drm/panel/panel-simple.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index 97964f7f2ace..38c646fb55fd 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -63,12 +63,15 @@ struct panel_desc {
>  	 *           turn the display off (no content is visible)
>  	 * @unprepare: the time (in milliseconds) that it takes for the panel
>  	 *             to power itself down completely
> +	 * @prepare_delay_only_if_no_hpd: The prepare delay should only be done
> +	 *                                if we know Hot Plug Detect isn't used.

I think it'd be more clear if we just had a new 'hpd_absent_delay' which is
added to prepare if no_hpd is true.

Sean

>  	 */
>  	struct {
>  		unsigned int prepare;
>  		unsigned int enable;
>  		unsigned int disable;
>  		unsigned int unprepare;
> +		bool prepare_delay_only_if_no_hpd;
>  	} delay;
>  
>  	u32 bus_format;
> @@ -79,6 +82,7 @@ struct panel_simple {
>  	struct drm_panel base;
>  	bool prepared;
>  	bool enabled;
> +	bool no_hpd;
>  
>  	const struct panel_desc *desc;
>  
> @@ -215,7 +219,8 @@ static int panel_simple_prepare(struct drm_panel *panel)
>  
>  	gpiod_set_value_cansleep(p->enable_gpio, 1);
>  
> -	if (p->desc->delay.prepare)
> +	if (p->desc->delay.prepare &&
> +	    (!p->desc->delay.prepare_delay_only_if_no_hpd || p->no_hpd))
>  		msleep(p->desc->delay.prepare);
>  
>  	p->prepared = true;
> @@ -305,6 +310,8 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
>  	panel->prepared = false;
>  	panel->desc = desc;
>  
> +	panel->no_hpd = of_property_read_bool(dev->of_node, "no-hpd");
> +
>  	panel->supply = devm_regulator_get(dev, "power");
>  	if (IS_ERR(panel->supply))
>  		return PTR_ERR(panel->supply);
> -- 
> 2.19.1.568.g152ad8e336-goog
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH 4/6] drm/bridge: ti-sn65dsi86: Remove the mystery delay

[Sean Paul]

On Mon, Oct 22, 2018 at 01:46:37PM -0700, Douglas Anderson wrote:
> Let's solve the mystery of commit bf1178c98930 ("drm/bridge:
> ti-sn65dsi86: Add mystery delay to enable()").  Specifically the
> reason we needed that mystery delay is that we weren't paying
> attention to HPD.
> 
> Looking at the datasheet for the same panel that was tested for the
> original commit, I see there's a timing "t3" that times from power on
> to the aux channel being operational.  This time is specced as 0 - 200
> ms.  The datasheet says that the aux channel is operational at exactly
> the same time that HPD is asserted.
> 
> Scoping the signals on this board showed that HPD was asserted 84 ms
> after power was asserted.  That very closely matches the magic 70 ms
> delay that we had.  ...and actually, in my testing the 70 ms wasn't
> quite enough of a delay and some percentage of the time the display
> didn't come up until I bumped it to 100 ms (presumably 84 ms would
> have worked too).
> 
> To solve this, we tried to hook up the HPD signal in the bridge.
> ...but in doing so we found that that the bridge didn't report that
> HPD was asserted until ~280 ms after we powered it (!).  This is
> explained by looking at the sn65dsi86 datasheet section "8.4.5.1 HPD
> (Hot Plug/Unplug Detection)".  Reading there we see that the bridge
> isn't even intended to report HPD until 100 ms after it's asserted.
> ...but that would have left us at 184 ms.  The extra 100 ms
> (presumably) comes from this part in the datasheet:
> 
> > The HPD state machine operates off an internal ring oscillator. The
> > ring oscillator frequency will vary [ ... ]. The min/max range in
> > the HPD State Diagram refers to the possible times based off
> > variation in the ring oscillator frequency.
> 
> Given that the 280 ms we'll end up delaying if we hook up HPD is
> _slower_ than the 200 ms we could just hardcode, for now we'll solve
> the problem by just hardcoding a 200 ms delay in the panel driver
> using the patch in this series ("drm/panel: simple: Support panels
> with HPD where HPD isn't connected").
> 
> If we later find a panel that needs to use this bridge where we need
> HPD then we'll have to come up with some new code to handle it.  Given
> the silly debouncing in the bridge chip, though, it seems unlikely.
> 
> One last note is that I tried to solve this through another way: In
> ti_sn_bridge_enable() I tried to use various combinations of
> dp_dpcd_writeb() and dp_dpcd_readb() to detect when the aux channel
> was up.  In theory that would let me detect _exactly_ when I could
> continue and do link training.  Unfortunately even if I did an aux
> transfer w/out waiting I couldn't see any errors.  Possibly I could
> keep looping over link training until it came back with success, but
> that seemed a little overly hacky to me.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Awesome commit message and comment, thanks for solving the mystery!

Reviewed-by: Sean Paul <sean@poorly.run>


> ---
> 
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 29 +++++++++++++++------------
>  1 file changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index f8a931cf3665..680566d97adc 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -458,18 +458,6 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
>  	unsigned int val;
>  	int ret;
>  
> -	/*
> -	 * FIXME:
> -	 * This 70ms was found necessary by experimentation. If it's not
> -	 * present, link training fails. It seems like it can go anywhere from
> -	 * pre_enable() up to semi-auto link training initiation below.
> -	 *
> -	 * Neither the datasheet for the bridge nor the panel tested mention a
> -	 * delay of this magnitude in the timing requirements. So for now, add
> -	 * the mystery delay until someone figures out a better fix.
> -	 */
> -	msleep(70);
> -
>  	/* DSI_A lane config */
>  	val = CHA_DSI_LANES(4 - pdata->dsi->lanes);
>  	regmap_update_bits(pdata->regmap, SN_DSI_LANES_REG,
> @@ -536,7 +524,22 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
>  	/* configure bridge ref_clk */
>  	ti_sn_bridge_set_refclk_freq(pdata);
>  
> -	/* in case drm_panel is connected then HPD is not supported */
> +	/*
> +	 * HPD on this bridge chip is a bit useless.  This is an eDP bridge
> +	 * so the HPD is an internal signal that's only there to signal that
> +	 * the panel is done powering up.  ...but the bridge chip debounces
> +	 * this signal by between 100 ms and 400 ms (depending on process,
> +	 * voltage, and temperate--I measured it at about 200 ms).  One
> +	 * particular panel asserted HPD 84 ms after it was powered on meaning
> +	 * that we saw HPD 284 ms after power on.  ...but the same panel said
> +	 * that instead of looking at HPD you could just hardcode a delay of
> +	 * 200 ms.  We'll assume that the panel driver will have the hardcoded
> +	 * delay in its prepare and always disable HPD.
> +	 *
> +	 * If HPD somehow makes sense on some future panel we'll have to
> +	 * change this to be conditional on someone specifying that HPD should
> +	 * be used.
> +	 */
>  	regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
>  			   HPD_DISABLE);
>  
> -- 
> 2.19.1.568.g152ad8e336-goog
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH 5/6] drm/panel: simple: Innolux TV123WAM is actually P120ZDG-BF1

[Sean Paul]

On Mon, Oct 22, 2018 at 01:46:38PM -0700, Douglas Anderson wrote:
> As far as I can tell the panel that was added in commit da50bd4258db
> ("drm/panel: simple: Add Innolux TV123WAM panel driver support")
> wasn't actually an Innolux TV123WAM but was actually an Innolux
> P120ZDG-BF1.
> 
> As far as I can tell the Innolux TV123WAM isn't a real panel and but
> it's a mosh between the TI TV123WAM and the Innolux P120ZDG-BF1.
> Let's unmosh.
> 
> Here's my evidence:
> 
> * Searching for TV123WAM on the Internet turns up a TI panel.  While
>   it's possible that an Innolux panel has the same model number as the
>   TI Panel, it seems a little doubtful.  Looking up the datasheet from
>   the TI Panel shows that it's 1920 x 1280 and 259.2 mm x 172.8 mm.
> 
> * As far as I know, the patch adding the Innolux Panel was supposed to
>   be for the board that's sitting in front of me as I type this
>   (support for that board is not yet upstream).  On the back of that
>   panel I see Innolux P120ZDZ-EZ1 rev B1.
> 
> * Someone pointed me at a datasheet that's supposed to be for the
>   panel in front of me (sorry, I can't share the datasheet).  That
>   datasheet has the string "p120zdg-bf1"
> 
> * If I search for "P120ZDG-BF1" on the Internet I get hits for panels
>   that are 2160x1440.  They don't have datasheets, but the fact that
>   the resolution matches is a good sign.
> 
> In any case, let's update the name and also the physical size to match
> the correct panel.
> 
> Fixes: da50bd4258db ("drm/panel: simple: Add Innolux TV123WAM panel driver support")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Cc: Sandeep Panda <spanda@codeaurora.org>
> ---
> 
>  drivers/gpu/drm/panel/panel-simple.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index 937e97490c30..7ee1abc5d81b 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -1370,7 +1370,7 @@ static const struct panel_desc innolux_n156bge_l21 = {
>  	},
>  };
>  
> -static const struct drm_display_mode innolux_tv123wam_mode = {
> +static const struct drm_display_mode innolux_p120zdg_bf1_mode = {
>  	.clock = 206016,
>  	.hdisplay = 2160,
>  	.hsync_start = 2160 + 48,
> @@ -1384,13 +1384,13 @@ static const struct drm_display_mode innolux_tv123wam_mode = {
>  	.flags = DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC,
>  };
>  
> -static const struct panel_desc innolux_tv123wam = {
> -	.modes = &innolux_tv123wam_mode,
> +static const struct panel_desc innolux_p120zdg_bf1 = {
> +	.modes = &innolux_p120zdg_bf1_mode,
>  	.num_modes = 1,
>  	.bpc = 8,
>  	.size = {
> -		.width = 259,
> -		.height = 173,
> +		.width = 254,
> +		.height = 169,
>  	},
>  	.delay = {
>  		.prepare = 200,
> @@ -2454,8 +2454,8 @@ static const struct of_device_id platform_of_match[] = {
>  		.compatible = "innolux,n156bge-l21",
>  		.data = &innolux_n156bge_l21,
>  	}, {
> -		.compatible = "innolux,tv123wam",

I think we should update the struct, but we might want to keep this around.
Given the tv123wam panel is TI, we're likely not going to have a collision on
innolux,...

That said, I'll defer to robh on this one, I'm not sure if changing names is
cool once the bindings have hit mainline.

> -		.data = &innolux_tv123wam,
> +		.compatible = "innolux,p120zdg-bf1",
> +		.data = &innolux_p120zdg_bf1,
>  	}, {
>  		.compatible = "innolux,zj070na-01p",
>  		.data = &innolux_zj070na_01p,
> -- 
> 2.19.1.568.g152ad8e336-goog
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/6] drm/panel: simple: Innolux TV123WAM is actually P120ZDG-BF1

[Sean Paul]

On Mon, Oct 22, 2018 at 01:46:38PM -0700, Douglas Anderson wrote:
> As far as I can tell the panel that was added in commit da50bd4258db
> ("drm/panel: simple: Add Innolux TV123WAM panel driver support")
> wasn't actually an Innolux TV123WAM but was actually an Innolux
> P120ZDG-BF1.
> 
> As far as I can tell the Innolux TV123WAM isn't a real panel and but
> it's a mosh between the TI TV123WAM and the Innolux P120ZDG-BF1.
> Let's unmosh.
> 
> Here's my evidence:
> 
> * Searching for TV123WAM on the Internet turns up a TI panel.  While
>   it's possible that an Innolux panel has the same model number as the
>   TI Panel, it seems a little doubtful.  Looking up the datasheet from
>   the TI Panel shows that it's 1920 x 1280 and 259.2 mm x 172.8 mm.
> 
> * As far as I know, the patch adding the Innolux Panel was supposed to
>   be for the board that's sitting in front of me as I type this
>   (support for that board is not yet upstream).  On the back of that
>   panel I see Innolux P120ZDZ-EZ1 rev B1.
> 
> * Someone pointed me at a datasheet that's supposed to be for the
>   panel in front of me (sorry, I can't share the datasheet).  That
>   datasheet has the string "p120zdg-bf1"
> 
> * If I search for "P120ZDG-BF1" on the Internet I get hits for panels
>   that are 2160x1440.  They don't have datasheets, but the fact that
>   the resolution matches is a good sign.
> 
> In any case, let's update the name and also the physical size to match
> the correct panel.
> 
> Fixes: da50bd4258db ("drm/panel: simple: Add Innolux TV123WAM panel driver support")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Cc: Sandeep Panda <spanda@codeaurora.org>
> ---
> 
>  drivers/gpu/drm/panel/panel-simple.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index 937e97490c30..7ee1abc5d81b 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -1370,7 +1370,7 @@ static const struct panel_desc innolux_n156bge_l21 = {
>  	},
>  };
>  
> -static const struct drm_display_mode innolux_tv123wam_mode = {
> +static const struct drm_display_mode innolux_p120zdg_bf1_mode = {
>  	.clock = 206016,
>  	.hdisplay = 2160,
>  	.hsync_start = 2160 + 48,
> @@ -1384,13 +1384,13 @@ static const struct drm_display_mode innolux_tv123wam_mode = {
>  	.flags = DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC,
>  };
>  
> -static const struct panel_desc innolux_tv123wam = {
> -	.modes = &innolux_tv123wam_mode,
> +static const struct panel_desc innolux_p120zdg_bf1 = {
> +	.modes = &innolux_p120zdg_bf1_mode,
>  	.num_modes = 1,
>  	.bpc = 8,
>  	.size = {
> -		.width = 259,
> -		.height = 173,
> +		.width = 254,
> +		.height = 169,
>  	},
>  	.delay = {
>  		.prepare = 200,
> @@ -2454,8 +2454,8 @@ static const struct of_device_id platform_of_match[] = {
>  		.compatible = "innolux,n156bge-l21",
>  		.data = &innolux_n156bge_l21,
>  	}, {
> -		.compatible = "innolux,tv123wam",

I think we should update the struct, but we might want to keep this around.
Given the tv123wam panel is TI, we're likely not going to have a collision on
innolux,...

That said, I'll defer to robh on this one, I'm not sure if changing names is
cool once the bindings have hit mainline.

> -		.data = &innolux_tv123wam,
> +		.compatible = "innolux,p120zdg-bf1",
> +		.data = &innolux_p120zdg_bf1,
>  	}, {
>  		.compatible = "innolux,zj070na-01p",
>  		.data = &innolux_zj070na_01p,
> -- 
> 2.19.1.568.g152ad8e336-goog
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH 6/6] dt-bindings: drm/panel: simple: Innolux TV123WAM is actually P120ZDG-BF1

[Sean Paul]

On Mon, Oct 22, 2018 at 01:46:39PM -0700, Douglas Anderson wrote:
> As far as I can tell the bindings that were added in commit
> 9c04400f7ea6 ("dt-bindings: drm/panel: Document Innolux TV123WAM panel
> bindings") weren't actually for Innolux TV123WAM but were actually for
> Innolux P120ZDG-BF1.
> 
> As far as I can tell the Innolux TV123WAM isn't a real panel and but
> it's a mosh between the TI TV123WAM and the Innolux P120ZDG-BF1.
> Let's unmosh.
> 
> Here's my evidence:
> 
> * Searching for TV123WAM on the Internet turns up a TI panel.  While
>   it's possible that an Innolux panel has the same model number as the
>   TI Panel, it seems a little doubtful.  Looking up the datasheet from
>   the TI Panel shows that it's 1920 x 1280 and 259.2 mm x 172.8 mm.
> 
> * As far as I know, the patch adding the Innolux Panel was supposed to
>   be for the board that's sitting in front of me as I type this
>   (support for that board is not yet upstream).  On the back of that
>   panel I see Innolux P120ZDZ-EZ1 rev B1.
> 
> * Someone pointed me at a datasheet that's supposed to be for the
>   panel in front of me (sorry, I can't share the datasheet).  That
>   datasheet has the string "p120zdg-bf1"
> 
> * If I search for "P120ZDG-BF1" on the Internet I get hits for panels
>   that are 2160x1440.  They don't have datasheets, but the fact that
>   the resolution matches is a good sign.
> 
> While we doing the rename, also mention that no-hpd can be used with
> this panel.  See the previous patch in this series ("drm/panel:
> simple: Add "no-hpd" delay for Innolux TV123WAM").
> 
> Fixes: 9c04400f7ea6 ("dt-bindings: drm/panel: Document Innolux TV123WAM panel bindings")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Cc: Sandeep Panda <spanda@codeaurora.org>
> ---
> 
>  .../{innolux,tv123wam.txt => innolux,p120zdg-bf1.txt}     | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>  rename Documentation/devicetree/bindings/display/panel/{innolux,tv123wam.txt => innolux,p120zdg-bf1.txt} (70%)
> 
> diff --git a/Documentation/devicetree/bindings/display/panel/innolux,tv123wam.txt b/Documentation/devicetree/bindings/display/panel/innolux,p120zdg-bf1.txt
> similarity index 70%
> rename from Documentation/devicetree/bindings/display/panel/innolux,tv123wam.txt
> rename to Documentation/devicetree/bindings/display/panel/innolux,p120zdg-bf1.txt

Same concerns here, you might consider leaving a breadcrumb here to point to
p120zdg-bf1 (noting that the old compatible string will continue to work).

Again, if robh is Ok with wiping innolux,tv123wam from existance, please ignore
:)

Sean

> index a9b35265fa13..513f03466aba 100644
> --- a/Documentation/devicetree/bindings/display/panel/innolux,tv123wam.txt
> +++ b/Documentation/devicetree/bindings/display/panel/innolux,p120zdg-bf1.txt
> @@ -1,20 +1,22 @@
> -Innolux TV123WAM 12.3 inch eDP 2K display panel
> +Innolux P120ZDG-BF1 12.02 inch eDP 2K display panel
>  
>  This binding is compatible with the simple-panel binding, which is specified
>  in simple-panel.txt in this directory.
>  
>  Required properties:
> -- compatible: should be "innolux,tv123wam"
> +- compatible: should be "innolux,p120zdg-bf1"
>  - power-supply: regulator to provide the supply voltage
>  
>  Optional properties:
>  - enable-gpios: GPIO pin to enable or disable the panel
>  - backlight: phandle of the backlight device attached to the panel
> +- no-hpd: If HPD isn't hooked up; add this property.
>  
>  Example:
>  	panel_edp: panel-edp {
> -		compatible = "innolux,tv123wam";
> +		compatible = "innolux,p120zdg-bf1";
>  		enable-gpios = <&msmgpio 31 GPIO_ACTIVE_LOW>;
>  		power-supply = <&pm8916_l2>;
>  		backlight = <&backlight>;
> +		no-hpd;
>  	};
> -- 
> 2.19.1.568.g152ad8e336-goog
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/6] dt-bindings: drm/panel: simple: Innolux TV123WAM is actually P120ZDG-BF1

[Sean Paul]

On Mon, Oct 22, 2018 at 01:46:39PM -0700, Douglas Anderson wrote:
> As far as I can tell the bindings that were added in commit
> 9c04400f7ea6 ("dt-bindings: drm/panel: Document Innolux TV123WAM panel
> bindings") weren't actually for Innolux TV123WAM but were actually for
> Innolux P120ZDG-BF1.
> 
> As far as I can tell the Innolux TV123WAM isn't a real panel and but
> it's a mosh between the TI TV123WAM and the Innolux P120ZDG-BF1.
> Let's unmosh.
> 
> Here's my evidence:
> 
> * Searching for TV123WAM on the Internet turns up a TI panel.  While
>   it's possible that an Innolux panel has the same model number as the
>   TI Panel, it seems a little doubtful.  Looking up the datasheet from
>   the TI Panel shows that it's 1920 x 1280 and 259.2 mm x 172.8 mm.
> 
> * As far as I know, the patch adding the Innolux Panel was supposed to
>   be for the board that's sitting in front of me as I type this
>   (support for that board is not yet upstream).  On the back of that
>   panel I see Innolux P120ZDZ-EZ1 rev B1.
> 
> * Someone pointed me at a datasheet that's supposed to be for the
>   panel in front of me (sorry, I can't share the datasheet).  That
>   datasheet has the string "p120zdg-bf1"
> 
> * If I search for "P120ZDG-BF1" on the Internet I get hits for panels
>   that are 2160x1440.  They don't have datasheets, but the fact that
>   the resolution matches is a good sign.
> 
> While we doing the rename, also mention that no-hpd can be used with
> this panel.  See the previous patch in this series ("drm/panel:
> simple: Add "no-hpd" delay for Innolux TV123WAM").
> 
> Fixes: 9c04400f7ea6 ("dt-bindings: drm/panel: Document Innolux TV123WAM panel bindings")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Cc: Sandeep Panda <spanda@codeaurora.org>
> ---
> 
>  .../{innolux,tv123wam.txt => innolux,p120zdg-bf1.txt}     | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>  rename Documentation/devicetree/bindings/display/panel/{innolux,tv123wam.txt => innolux,p120zdg-bf1.txt} (70%)
> 
> diff --git a/Documentation/devicetree/bindings/display/panel/innolux,tv123wam.txt b/Documentation/devicetree/bindings/display/panel/innolux,p120zdg-bf1.txt
> similarity index 70%
> rename from Documentation/devicetree/bindings/display/panel/innolux,tv123wam.txt
> rename to Documentation/devicetree/bindings/display/panel/innolux,p120zdg-bf1.txt

Same concerns here, you might consider leaving a breadcrumb here to point to
p120zdg-bf1 (noting that the old compatible string will continue to work).

Again, if robh is Ok with wiping innolux,tv123wam from existance, please ignore
:)

Sean

> index a9b35265fa13..513f03466aba 100644
> --- a/Documentation/devicetree/bindings/display/panel/innolux,tv123wam.txt
> +++ b/Documentation/devicetree/bindings/display/panel/innolux,p120zdg-bf1.txt
> @@ -1,20 +1,22 @@
> -Innolux TV123WAM 12.3 inch eDP 2K display panel
> +Innolux P120ZDG-BF1 12.02 inch eDP 2K display panel
>  
>  This binding is compatible with the simple-panel binding, which is specified
>  in simple-panel.txt in this directory.
>  
>  Required properties:
> -- compatible: should be "innolux,tv123wam"
> +- compatible: should be "innolux,p120zdg-bf1"
>  - power-supply: regulator to provide the supply voltage
>  
>  Optional properties:
>  - enable-gpios: GPIO pin to enable or disable the panel
>  - backlight: phandle of the backlight device attached to the panel
> +- no-hpd: If HPD isn't hooked up; add this property.
>  
>  Example:
>  	panel_edp: panel-edp {
> -		compatible = "innolux,tv123wam";
> +		compatible = "innolux,p120zdg-bf1";
>  		enable-gpios = <&msmgpio 31 GPIO_ACTIVE_LOW>;
>  		power-supply = <&pm8916_l2>;
>  		backlight = <&backlight>;
> +		no-hpd;
>  	};
> -- 
> 2.19.1.568.g152ad8e336-goog
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH 5/6] drm/panel: simple: Innolux TV123WAM is actually P120ZDG-BF1

[Doug Anderson]

Hi,

On Thu, Oct 25, 2018 at 11:13 AM Sean Paul <sean@poorly.run> wrote:
>
> On Mon, Oct 22, 2018 at 01:46:38PM -0700, Douglas Anderson wrote:
> > As far as I can tell the panel that was added in commit da50bd4258db
> > ("drm/panel: simple: Add Innolux TV123WAM panel driver support")
> > wasn't actually an Innolux TV123WAM but was actually an Innolux
> > P120ZDG-BF1.
> >
> > As far as I can tell the Innolux TV123WAM isn't a real panel and but
> > it's a mosh between the TI TV123WAM and the Innolux P120ZDG-BF1.
> > Let's unmosh.
> >
> > Here's my evidence:
> >
> > * Searching for TV123WAM on the Internet turns up a TI panel.  While
> >   it's possible that an Innolux panel has the same model number as the
> >   TI Panel, it seems a little doubtful.  Looking up the datasheet from
> >   the TI Panel shows that it's 1920 x 1280 and 259.2 mm x 172.8 mm.
> >
> > * As far as I know, the patch adding the Innolux Panel was supposed to
> >   be for the board that's sitting in front of me as I type this
> >   (support for that board is not yet upstream).  On the back of that
> >   panel I see Innolux P120ZDZ-EZ1 rev B1.
> >
> > * Someone pointed me at a datasheet that's supposed to be for the
> >   panel in front of me (sorry, I can't share the datasheet).  That
> >   datasheet has the string "p120zdg-bf1"
> >
> > * If I search for "P120ZDG-BF1" on the Internet I get hits for panels
> >   that are 2160x1440.  They don't have datasheets, but the fact that
> >   the resolution matches is a good sign.
> >
> > In any case, let's update the name and also the physical size to match
> > the correct panel.
> >
> > Fixes: da50bd4258db ("drm/panel: simple: Add Innolux TV123WAM panel driver support")
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > Cc: Sandeep Panda <spanda@codeaurora.org>
> > ---
> >
> >  drivers/gpu/drm/panel/panel-simple.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> > index 937e97490c30..7ee1abc5d81b 100644
> > --- a/drivers/gpu/drm/panel/panel-simple.c
> > +++ b/drivers/gpu/drm/panel/panel-simple.c
> > @@ -1370,7 +1370,7 @@ static const struct panel_desc innolux_n156bge_l21 = {
> >       },
> >  };
> >
> > -static const struct drm_display_mode innolux_tv123wam_mode = {
> > +static const struct drm_display_mode innolux_p120zdg_bf1_mode = {
> >       .clock = 206016,
> >       .hdisplay = 2160,
> >       .hsync_start = 2160 + 48,
> > @@ -1384,13 +1384,13 @@ static const struct drm_display_mode innolux_tv123wam_mode = {
> >       .flags = DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC,
> >  };
> >
> > -static const struct panel_desc innolux_tv123wam = {
> > -     .modes = &innolux_tv123wam_mode,
> > +static const struct panel_desc innolux_p120zdg_bf1 = {
> > +     .modes = &innolux_p120zdg_bf1_mode,
> >       .num_modes = 1,
> >       .bpc = 8,
> >       .size = {
> > -             .width = 259,
> > -             .height = 173,
> > +             .width = 254,
> > +             .height = 169,
> >       },
> >       .delay = {
> >               .prepare = 200,
> > @@ -2454,8 +2454,8 @@ static const struct of_device_id platform_of_match[] = {
> >               .compatible = "innolux,n156bge-l21",
> >               .data = &innolux_n156bge_l21,
> >       }, {
> > -             .compatible = "innolux,tv123wam",
>
> I think we should update the struct, but we might want to keep this around.
> Given the tv123wam panel is TI, we're likely not going to have a collision on
> innolux,...
>
> That said, I'll defer to robh on this one, I'm not sure if changing names is
> cool once the bindings have hit mainline.

Whoops, I missed Rob on this patch--just had him on the bindings one.

...generally I believe Rob seems to be OK with wiping out backward
compatibility for things like this when the previous binding is super
new and there's no evidence that anyone ever used it (like if it was
added for a specific board and that board doesn't have a fully
functional DT anyway).

In this particular case I'm 99.9999% certain nobody is using the
existing binding.  If someone crawls out of the woodwork and says this
patch broke them, it would be trivially easy to add the backward
compatible string later.

Obviously Rob can feel free to correct me if I'm wrong.

I purposely put this patch at the end of the series so we can land the
earlier ones and we can sit on this one for a little while if desired.

-Doug
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/6] drm/panel: simple: Innolux TV123WAM is actually P120ZDG-BF1

[Doug Anderson]

Hi,

On Thu, Oct 25, 2018 at 11:13 AM Sean Paul <sean@poorly.run> wrote:
>
> On Mon, Oct 22, 2018 at 01:46:38PM -0700, Douglas Anderson wrote:
> > As far as I can tell the panel that was added in commit da50bd4258db
> > ("drm/panel: simple: Add Innolux TV123WAM panel driver support")
> > wasn't actually an Innolux TV123WAM but was actually an Innolux
> > P120ZDG-BF1.
> >
> > As far as I can tell the Innolux TV123WAM isn't a real panel and but
> > it's a mosh between the TI TV123WAM and the Innolux P120ZDG-BF1.
> > Let's unmosh.
> >
> > Here's my evidence:
> >
> > * Searching for TV123WAM on the Internet turns up a TI panel.  While
> >   it's possible that an Innolux panel has the same model number as the
> >   TI Panel, it seems a little doubtful.  Looking up the datasheet from
> >   the TI Panel shows that it's 1920 x 1280 and 259.2 mm x 172.8 mm.
> >
> > * As far as I know, the patch adding the Innolux Panel was supposed to
> >   be for the board that's sitting in front of me as I type this
> >   (support for that board is not yet upstream).  On the back of that
> >   panel I see Innolux P120ZDZ-EZ1 rev B1.
> >
> > * Someone pointed me at a datasheet that's supposed to be for the
> >   panel in front of me (sorry, I can't share the datasheet).  That
> >   datasheet has the string "p120zdg-bf1"
> >
> > * If I search for "P120ZDG-BF1" on the Internet I get hits for panels
> >   that are 2160x1440.  They don't have datasheets, but the fact that
> >   the resolution matches is a good sign.
> >
> > In any case, let's update the name and also the physical size to match
> > the correct panel.
> >
> > Fixes: da50bd4258db ("drm/panel: simple: Add Innolux TV123WAM panel driver support")
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > Cc: Sandeep Panda <spanda@codeaurora.org>
> > ---
> >
> >  drivers/gpu/drm/panel/panel-simple.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> > index 937e97490c30..7ee1abc5d81b 100644
> > --- a/drivers/gpu/drm/panel/panel-simple.c
> > +++ b/drivers/gpu/drm/panel/panel-simple.c
> > @@ -1370,7 +1370,7 @@ static const struct panel_desc innolux_n156bge_l21 = {
> >       },
> >  };
> >
> > -static const struct drm_display_mode innolux_tv123wam_mode = {
> > +static const struct drm_display_mode innolux_p120zdg_bf1_mode = {
> >       .clock = 206016,
> >       .hdisplay = 2160,
> >       .hsync_start = 2160 + 48,
> > @@ -1384,13 +1384,13 @@ static const struct drm_display_mode innolux_tv123wam_mode = {
> >       .flags = DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC,
> >  };
> >
> > -static const struct panel_desc innolux_tv123wam = {
> > -     .modes = &innolux_tv123wam_mode,
> > +static const struct panel_desc innolux_p120zdg_bf1 = {
> > +     .modes = &innolux_p120zdg_bf1_mode,
> >       .num_modes = 1,
> >       .bpc = 8,
> >       .size = {
> > -             .width = 259,
> > -             .height = 173,
> > +             .width = 254,
> > +             .height = 169,
> >       },
> >       .delay = {
> >               .prepare = 200,
> > @@ -2454,8 +2454,8 @@ static const struct of_device_id platform_of_match[] = {
> >               .compatible = "innolux,n156bge-l21",
> >               .data = &innolux_n156bge_l21,
> >       }, {
> > -             .compatible = "innolux,tv123wam",
>
> I think we should update the struct, but we might want to keep this around.
> Given the tv123wam panel is TI, we're likely not going to have a collision on
> innolux,...
>
> That said, I'll defer to robh on this one, I'm not sure if changing names is
> cool once the bindings have hit mainline.

Whoops, I missed Rob on this patch--just had him on the bindings one.

...generally I believe Rob seems to be OK with wiping out backward
compatibility for things like this when the previous binding is super
new and there's no evidence that anyone ever used it (like if it was
added for a specific board and that board doesn't have a fully
functional DT anyway).

In this particular case I'm 99.9999% certain nobody is using the
existing binding.  If someone crawls out of the woodwork and says this
patch broke them, it would be trivially easy to add the backward
compatible string later.

Obviously Rob can feel free to correct me if I'm wrong.

I purposely put this patch at the end of the series so we can land the
earlier ones and we can sit on this one for a little while if desired.

-Doug

Re: [PATCH 5/6] drm/panel: simple: Innolux TV123WAM is actually P120ZDG-BF1

[Abhinav Kumar]

On 2018-10-25 11:45, Doug Anderson wrote:
> Hi,
> 
> On Thu, Oct 25, 2018 at 11:13 AM Sean Paul <sean@poorly.run> wrote:
>> 
>> On Mon, Oct 22, 2018 at 01:46:38PM -0700, Douglas Anderson wrote:
>> > As far as I can tell the panel that was added in commit da50bd4258db
>> > ("drm/panel: simple: Add Innolux TV123WAM panel driver support")
>> > wasn't actually an Innolux TV123WAM but was actually an Innolux
>> > P120ZDG-BF1.
>> >
>> > As far as I can tell the Innolux TV123WAM isn't a real panel and but
>> > it's a mosh between the TI TV123WAM and the Innolux P120ZDG-BF1.
>> > Let's unmosh.
>> >
>> > Here's my evidence:
>> >
>> > * Searching for TV123WAM on the Internet turns up a TI panel.  While
>> >   it's possible that an Innolux panel has the same model number as the
>> >   TI Panel, it seems a little doubtful.  Looking up the datasheet from
>> >   the TI Panel shows that it's 1920 x 1280 and 259.2 mm x 172.8 mm.
>> >
>> > * As far as I know, the patch adding the Innolux Panel was supposed to
>> >   be for the board that's sitting in front of me as I type this
>> >   (support for that board is not yet upstream).  On the back of that
>> >   panel I see Innolux P120ZDZ-EZ1 rev B1.
>> >
>> > * Someone pointed me at a datasheet that's supposed to be for the
>> >   panel in front of me (sorry, I can't share the datasheet).  That
>> >   datasheet has the string "p120zdg-bf1"
>> >
>> > * If I search for "P120ZDG-BF1" on the Internet I get hits for panels
>> >   that are 2160x1440.  They don't have datasheets, but the fact that
>> >   the resolution matches is a good sign.
>> >
>> > In any case, let's update the name and also the physical size to match
>> > the correct panel.
>> >
>> > Fixes: da50bd4258db ("drm/panel: simple: Add Innolux TV123WAM panel driver support")
>> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
>> > Cc: Sandeep Panda <spanda@codeaurora.org>
>> > ---
If Rob is onboard with this binding change, please feel free to add
Reviewed-by: Abhinav Kumar <abhinavk@codeaurora.org>
>> >
>> >  drivers/gpu/drm/panel/panel-simple.c | 14 +++++++-------
>> >  1 file changed, 7 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
>> > index 937e97490c30..7ee1abc5d81b 100644
>> > --- a/drivers/gpu/drm/panel/panel-simple.c
>> > +++ b/drivers/gpu/drm/panel/panel-simple.c
>> > @@ -1370,7 +1370,7 @@ static const struct panel_desc innolux_n156bge_l21 = {
>> >       },
>> >  };
>> >
>> > -static const struct drm_display_mode innolux_tv123wam_mode = {
>> > +static const struct drm_display_mode innolux_p120zdg_bf1_mode = {
>> >       .clock = 206016,
>> >       .hdisplay = 2160,
>> >       .hsync_start = 2160 + 48,
>> > @@ -1384,13 +1384,13 @@ static const struct drm_display_mode innolux_tv123wam_mode = {
>> >       .flags = DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC,
>> >  };
>> >
>> > -static const struct panel_desc innolux_tv123wam = {
>> > -     .modes = &innolux_tv123wam_mode,
>> > +static const struct panel_desc innolux_p120zdg_bf1 = {
>> > +     .modes = &innolux_p120zdg_bf1_mode,
>> >       .num_modes = 1,
>> >       .bpc = 8,
>> >       .size = {
>> > -             .width = 259,
>> > -             .height = 173,
>> > +             .width = 254,
>> > +             .height = 169,
>> >       },
>> >       .delay = {
>> >               .prepare = 200,
>> > @@ -2454,8 +2454,8 @@ static const struct of_device_id platform_of_match[] = {
>> >               .compatible = "innolux,n156bge-l21",
>> >               .data = &innolux_n156bge_l21,
>> >       }, {
>> > -             .compatible = "innolux,tv123wam",
>> 
>> I think we should update the struct, but we might want to keep this 
>> around.
>> Given the tv123wam panel is TI, we're likely not going to have a 
>> collision on
>> innolux,...
>> 
>> That said, I'll defer to robh on this one, I'm not sure if changing 
>> names is
>> cool once the bindings have hit mainline.
> 
> Whoops, I missed Rob on this patch--just had him on the bindings one.
> 
> ...generally I believe Rob seems to be OK with wiping out backward
> compatibility for things like this when the previous binding is super
> new and there's no evidence that anyone ever used it (like if it was
> added for a specific board and that board doesn't have a fully
> functional DT anyway).
> 
> In this particular case I'm 99.9999% certain nobody is using the
> existing binding.  If someone crawls out of the woodwork and says this
> patch broke them, it would be trivially easy to add the backward
> compatible string later.
> 
> Obviously Rob can feel free to correct me if I'm wrong.
> 
> I purposely put this patch at the end of the series so we can land the
> earlier ones and we can sit on this one for a little while if desired.
> 
> -Doug

Re: [PATCH 1/6] dt-bindings: drm/panel: simple: Add no-hpd property

[Rob Herring]

On Mon, Oct 22, 2018 at 01:46:34PM -0700, Douglas Anderson wrote:
> Some eDP panels that are designed to be always connected to a board
> use their HPD signal to signal that they've finished powering on and
> they're ready to be talked to.
> 
> However, for various reasons it's possible that the HPD signal from
> the panel isn't actually hooked up.  In the case where the HPD isn't
> hooked up you can look at the timing diagram on the panel datasheet
> and insert a delay for the maximum amount of time that the HPD might
> take to come up.
> 
> Let's add a property in the device tree for this concept.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
>  .../devicetree/bindings/display/panel/simple-panel.txt         | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/panel/simple-panel.txt b/Documentation/devicetree/bindings/display/panel/simple-panel.txt
> index 45a457ad38f0..b2b872c710f2 100644
> --- a/Documentation/devicetree/bindings/display/panel/simple-panel.txt
> +++ b/Documentation/devicetree/bindings/display/panel/simple-panel.txt
> @@ -11,6 +11,9 @@ Optional properties:
>  - ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
>  - enable-gpios: GPIO pin to enable or disable the panel
>  - backlight: phandle of the backlight device attached to the panel
> +- no-hpd: This panel is supposed to communicate that it's ready via HPD
> +  (hot plug detect) signal, but the signal isn't hooked up so we should
> +  hardcode the max delay from the panel spec when powering up the panel.

If we have this here, then we should also have hpd-gpios defined here as 
where we describe a connection we should also describe no connection.

Now, hpd-gpios is a bit of a mess being defined in both connector nodes 
and bridge (HDMI/DP) nodes. I think that is just history pre-dating 
connector nodes. Connector nodes are now the preferred place. Connector 
nodes and panel nodes are essentially the same thing (the endpoint of 
display pipeline).

That being said, this patch is fine as is.

Reviewed-by: Rob Herring <robh@kernel.org>

Rob
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/6] dt-bindings: drm/panel: simple: Add no-hpd property

[Rob Herring]

On Mon, Oct 22, 2018 at 01:46:34PM -0700, Douglas Anderson wrote:
> Some eDP panels that are designed to be always connected to a board
> use their HPD signal to signal that they've finished powering on and
> they're ready to be talked to.
> 
> However, for various reasons it's possible that the HPD signal from
> the panel isn't actually hooked up.  In the case where the HPD isn't
> hooked up you can look at the timing diagram on the panel datasheet
> and insert a delay for the maximum amount of time that the HPD might
> take to come up.
> 
> Let's add a property in the device tree for this concept.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
>  .../devicetree/bindings/display/panel/simple-panel.txt         | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/panel/simple-panel.txt b/Documentation/devicetree/bindings/display/panel/simple-panel.txt
> index 45a457ad38f0..b2b872c710f2 100644
> --- a/Documentation/devicetree/bindings/display/panel/simple-panel.txt
> +++ b/Documentation/devicetree/bindings/display/panel/simple-panel.txt
> @@ -11,6 +11,9 @@ Optional properties:
>  - ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
>  - enable-gpios: GPIO pin to enable or disable the panel
>  - backlight: phandle of the backlight device attached to the panel
> +- no-hpd: This panel is supposed to communicate that it's ready via HPD
> +  (hot plug detect) signal, but the signal isn't hooked up so we should
> +  hardcode the max delay from the panel spec when powering up the panel.

If we have this here, then we should also have hpd-gpios defined here as 
where we describe a connection we should also describe no connection.

Now, hpd-gpios is a bit of a mess being defined in both connector nodes 
and bridge (HDMI/DP) nodes. I think that is just history pre-dating 
connector nodes. Connector nodes are now the preferred place. Connector 
nodes and panel nodes are essentially the same thing (the endpoint of 
display pipeline).

That being said, this patch is fine as is.

Reviewed-by: Rob Herring <robh@kernel.org>

Rob

Re: [PATCH 6/6] dt-bindings: drm/panel: simple: Innolux TV123WAM is actually P120ZDG-BF1

[Rob Herring]

On Mon, 22 Oct 2018 13:46:39 -0700, Douglas Anderson wrote:
> As far as I can tell the bindings that were added in commit
> 9c04400f7ea6 ("dt-bindings: drm/panel: Document Innolux TV123WAM panel
> bindings") weren't actually for Innolux TV123WAM but were actually for
> Innolux P120ZDG-BF1.
> 
> As far as I can tell the Innolux TV123WAM isn't a real panel and but
> it's a mosh between the TI TV123WAM and the Innolux P120ZDG-BF1.
> Let's unmosh.
> 
> Here's my evidence:
> 
> * Searching for TV123WAM on the Internet turns up a TI panel.  While
>   it's possible that an Innolux panel has the same model number as the
>   TI Panel, it seems a little doubtful.  Looking up the datasheet from
>   the TI Panel shows that it's 1920 x 1280 and 259.2 mm x 172.8 mm.
> 
> * As far as I know, the patch adding the Innolux Panel was supposed to
>   be for the board that's sitting in front of me as I type this
>   (support for that board is not yet upstream).  On the back of that
>   panel I see Innolux P120ZDZ-EZ1 rev B1.
> 
> * Someone pointed me at a datasheet that's supposed to be for the
>   panel in front of me (sorry, I can't share the datasheet).  That
>   datasheet has the string "p120zdg-bf1"
> 
> * If I search for "P120ZDG-BF1" on the Internet I get hits for panels
>   that are 2160x1440.  They don't have datasheets, but the fact that
>   the resolution matches is a good sign.
> 
> While we doing the rename, also mention that no-hpd can be used with
> this panel.  See the previous patch in this series ("drm/panel:
> simple: Add "no-hpd" delay for Innolux TV123WAM").
> 
> Fixes: 9c04400f7ea6 ("dt-bindings: drm/panel: Document Innolux TV123WAM panel bindings")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Cc: Sandeep Panda <spanda@codeaurora.org>
> ---
> 
>  .../{innolux,tv123wam.txt => innolux,p120zdg-bf1.txt}     | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>  rename Documentation/devicetree/bindings/display/panel/{innolux,tv123wam.txt => innolux,p120zdg-bf1.txt} (70%)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH 6/6] dt-bindings: drm/panel: simple: Innolux TV123WAM is actually P120ZDG-BF1

[Rob Herring]

On Mon, 22 Oct 2018 13:46:39 -0700, Douglas Anderson wrote:
> As far as I can tell the bindings that were added in commit
> 9c04400f7ea6 ("dt-bindings: drm/panel: Document Innolux TV123WAM panel
> bindings") weren't actually for Innolux TV123WAM but were actually for
> Innolux P120ZDG-BF1.
> 
> As far as I can tell the Innolux TV123WAM isn't a real panel and but
> it's a mosh between the TI TV123WAM and the Innolux P120ZDG-BF1.
> Let's unmosh.
> 
> Here's my evidence:
> 
> * Searching for TV123WAM on the Internet turns up a TI panel.  While
>   it's possible that an Innolux panel has the same model number as the
>   TI Panel, it seems a little doubtful.  Looking up the datasheet from
>   the TI Panel shows that it's 1920 x 1280 and 259.2 mm x 172.8 mm.
> 
> * As far as I know, the patch adding the Innolux Panel was supposed to
>   be for the board that's sitting in front of me as I type this
>   (support for that board is not yet upstream).  On the back of that
>   panel I see Innolux P120ZDZ-EZ1 rev B1.
> 
> * Someone pointed me at a datasheet that's supposed to be for the
>   panel in front of me (sorry, I can't share the datasheet).  That
>   datasheet has the string "p120zdg-bf1"
> 
> * If I search for "P120ZDG-BF1" on the Internet I get hits for panels
>   that are 2160x1440.  They don't have datasheets, but the fact that
>   the resolution matches is a good sign.
> 
> While we doing the rename, also mention that no-hpd can be used with
> this panel.  See the previous patch in this series ("drm/panel:
> simple: Add "no-hpd" delay for Innolux TV123WAM").
> 
> Fixes: 9c04400f7ea6 ("dt-bindings: drm/panel: Document Innolux TV123WAM panel bindings")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Cc: Sandeep Panda <spanda@codeaurora.org>
> ---
> 
>  .../{innolux,tv123wam.txt => innolux,p120zdg-bf1.txt}     | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>  rename Documentation/devicetree/bindings/display/panel/{innolux,tv123wam.txt => innolux,p120zdg-bf1.txt} (70%)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH 5/6] drm/panel: simple: Innolux TV123WAM is actually P120ZDG-BF1

[Doug Anderson]

Hi,

On Thu, Oct 25, 2018 at 11:13 AM Sean Paul <sean@poorly.run> wrote:
>
> On Mon, Oct 22, 2018 at 01:46:38PM -0700, Douglas Anderson wrote:
> > As far as I can tell the panel that was added in commit da50bd4258db
> > ("drm/panel: simple: Add Innolux TV123WAM panel driver support")
> > wasn't actually an Innolux TV123WAM but was actually an Innolux
> > P120ZDG-BF1.
> >
> > As far as I can tell the Innolux TV123WAM isn't a real panel and but
> > it's a mosh between the TI TV123WAM and the Innolux P120ZDG-BF1.
> > Let's unmosh.
> >
> > Here's my evidence:
> >
> > * Searching for TV123WAM on the Internet turns up a TI panel.  While
> >   it's possible that an Innolux panel has the same model number as the
> >   TI Panel, it seems a little doubtful.  Looking up the datasheet from
> >   the TI Panel shows that it's 1920 x 1280 and 259.2 mm x 172.8 mm.
> >
> > * As far as I know, the patch adding the Innolux Panel was supposed to
> >   be for the board that's sitting in front of me as I type this
> >   (support for that board is not yet upstream).  On the back of that
> >   panel I see Innolux P120ZDZ-EZ1 rev B1.
> >
> > * Someone pointed me at a datasheet that's supposed to be for the
> >   panel in front of me (sorry, I can't share the datasheet).  That
> >   datasheet has the string "p120zdg-bf1"
> >
> > * If I search for "P120ZDG-BF1" on the Internet I get hits for panels
> >   that are 2160x1440.  They don't have datasheets, but the fact that
> >   the resolution matches is a good sign.
> >
> > In any case, let's update the name and also the physical size to match
> > the correct panel.
> >
> > Fixes: da50bd4258db ("drm/panel: simple: Add Innolux TV123WAM panel driver support")
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > Cc: Sandeep Panda <spanda@codeaurora.org>
> > ---
> >
> >  drivers/gpu/drm/panel/panel-simple.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> > index 937e97490c30..7ee1abc5d81b 100644
> > --- a/drivers/gpu/drm/panel/panel-simple.c
> > +++ b/drivers/gpu/drm/panel/panel-simple.c
> > @@ -1370,7 +1370,7 @@ static const struct panel_desc innolux_n156bge_l21 = {
> >       },
> >  };
> >
> > -static const struct drm_display_mode innolux_tv123wam_mode = {
> > +static const struct drm_display_mode innolux_p120zdg_bf1_mode = {
> >       .clock = 206016,
> >       .hdisplay = 2160,
> >       .hsync_start = 2160 + 48,
> > @@ -1384,13 +1384,13 @@ static const struct drm_display_mode innolux_tv123wam_mode = {
> >       .flags = DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC,
> >  };
> >
> > -static const struct panel_desc innolux_tv123wam = {
> > -     .modes = &innolux_tv123wam_mode,
> > +static const struct panel_desc innolux_p120zdg_bf1 = {
> > +     .modes = &innolux_p120zdg_bf1_mode,
> >       .num_modes = 1,
> >       .bpc = 8,
> >       .size = {
> > -             .width = 259,
> > -             .height = 173,
> > +             .width = 254,
> > +             .height = 169,
> >       },
> >       .delay = {
> >               .prepare = 200,
> > @@ -2454,8 +2454,8 @@ static const struct of_device_id platform_of_match[] = {
> >               .compatible = "innolux,n156bge-l21",
> >               .data = &innolux_n156bge_l21,
> >       }, {
> > -             .compatible = "innolux,tv123wam",
>
> I think we should update the struct, but we might want to keep this around.
> Given the tv123wam panel is TI, we're likely not going to have a collision on
> innolux,...
>
> That said, I'll defer to robh on this one, I'm not sure if changing names is
> cool once the bindings have hit mainline.

Rob gave the bindings patch a Reviewed-by tag, so I'm assuming he's
cool with it.  v2 still doesn't keep the "innolux,tv123wam" around.
If you disagree then let me know and I'll do a v3.

-Doug

Re: [PATCH 5/6] drm/panel: simple: Innolux TV123WAM is actually P120ZDG-BF1

[Sean Paul]

On Thu, Oct 25, 2018 at 03:24:58PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Thu, Oct 25, 2018 at 11:13 AM Sean Paul <sean@poorly.run> wrote:
> >
> > On Mon, Oct 22, 2018 at 01:46:38PM -0700, Douglas Anderson wrote:
> > > As far as I can tell the panel that was added in commit da50bd4258db
> > > ("drm/panel: simple: Add Innolux TV123WAM panel driver support")
> > > wasn't actually an Innolux TV123WAM but was actually an Innolux
> > > P120ZDG-BF1.
> > >
> > > As far as I can tell the Innolux TV123WAM isn't a real panel and but
> > > it's a mosh between the TI TV123WAM and the Innolux P120ZDG-BF1.
> > > Let's unmosh.
> > >
> > > Here's my evidence:
> > >
> > > * Searching for TV123WAM on the Internet turns up a TI panel.  While
> > >   it's possible that an Innolux panel has the same model number as the
> > >   TI Panel, it seems a little doubtful.  Looking up the datasheet from
> > >   the TI Panel shows that it's 1920 x 1280 and 259.2 mm x 172.8 mm.
> > >
> > > * As far as I know, the patch adding the Innolux Panel was supposed to
> > >   be for the board that's sitting in front of me as I type this
> > >   (support for that board is not yet upstream).  On the back of that
> > >   panel I see Innolux P120ZDZ-EZ1 rev B1.
> > >
> > > * Someone pointed me at a datasheet that's supposed to be for the
> > >   panel in front of me (sorry, I can't share the datasheet).  That
> > >   datasheet has the string "p120zdg-bf1"
> > >
> > > * If I search for "P120ZDG-BF1" on the Internet I get hits for panels
> > >   that are 2160x1440.  They don't have datasheets, but the fact that
> > >   the resolution matches is a good sign.
> > >
> > > In any case, let's update the name and also the physical size to match
> > > the correct panel.
> > >
> > > Fixes: da50bd4258db ("drm/panel: simple: Add Innolux TV123WAM panel driver support")
> > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > Cc: Sandeep Panda <spanda@codeaurora.org>
> > > ---
> > >
> > >  drivers/gpu/drm/panel/panel-simple.c | 14 +++++++-------
> > >  1 file changed, 7 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> > > index 937e97490c30..7ee1abc5d81b 100644
> > > --- a/drivers/gpu/drm/panel/panel-simple.c
> > > +++ b/drivers/gpu/drm/panel/panel-simple.c
> > > @@ -1370,7 +1370,7 @@ static const struct panel_desc innolux_n156bge_l21 = {
> > >       },
> > >  };
> > >
> > > -static const struct drm_display_mode innolux_tv123wam_mode = {
> > > +static const struct drm_display_mode innolux_p120zdg_bf1_mode = {
> > >       .clock = 206016,
> > >       .hdisplay = 2160,
> > >       .hsync_start = 2160 + 48,
> > > @@ -1384,13 +1384,13 @@ static const struct drm_display_mode innolux_tv123wam_mode = {
> > >       .flags = DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC,
> > >  };
> > >
> > > -static const struct panel_desc innolux_tv123wam = {
> > > -     .modes = &innolux_tv123wam_mode,
> > > +static const struct panel_desc innolux_p120zdg_bf1 = {
> > > +     .modes = &innolux_p120zdg_bf1_mode,
> > >       .num_modes = 1,
> > >       .bpc = 8,
> > >       .size = {
> > > -             .width = 259,
> > > -             .height = 173,
> > > +             .width = 254,
> > > +             .height = 169,
> > >       },
> > >       .delay = {
> > >               .prepare = 200,
> > > @@ -2454,8 +2454,8 @@ static const struct of_device_id platform_of_match[] = {
> > >               .compatible = "innolux,n156bge-l21",
> > >               .data = &innolux_n156bge_l21,
> > >       }, {
> > > -             .compatible = "innolux,tv123wam",
> >
> > I think we should update the struct, but we might want to keep this around.
> > Given the tv123wam panel is TI, we're likely not going to have a collision on
> > innolux,...
> >
> > That said, I'll defer to robh on this one, I'm not sure if changing names is
> > cool once the bindings have hit mainline.
> 
> Rob gave the bindings patch a Reviewed-by tag, so I'm assuming he's
> cool with it.  v2 still doesn't keep the "innolux,tv123wam" around.
> If you disagree then let me know and I'll do a v3.

I happily defer to Rob on all things dt. So,

Reviewed-by: Sean Paul <sean@poorly.run>


> 
> -Doug

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/6] drm/panel: simple: Innolux TV123WAM is actually P120ZDG-BF1

[Sean Paul]

On Thu, Oct 25, 2018 at 03:24:58PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Thu, Oct 25, 2018 at 11:13 AM Sean Paul <sean@poorly.run> wrote:
> >
> > On Mon, Oct 22, 2018 at 01:46:38PM -0700, Douglas Anderson wrote:
> > > As far as I can tell the panel that was added in commit da50bd4258db
> > > ("drm/panel: simple: Add Innolux TV123WAM panel driver support")
> > > wasn't actually an Innolux TV123WAM but was actually an Innolux
> > > P120ZDG-BF1.
> > >
> > > As far as I can tell the Innolux TV123WAM isn't a real panel and but
> > > it's a mosh between the TI TV123WAM and the Innolux P120ZDG-BF1.
> > > Let's unmosh.
> > >
> > > Here's my evidence:
> > >
> > > * Searching for TV123WAM on the Internet turns up a TI panel.  While
> > >   it's possible that an Innolux panel has the same model number as the
> > >   TI Panel, it seems a little doubtful.  Looking up the datasheet from
> > >   the TI Panel shows that it's 1920 x 1280 and 259.2 mm x 172.8 mm.
> > >
> > > * As far as I know, the patch adding the Innolux Panel was supposed to
> > >   be for the board that's sitting in front of me as I type this
> > >   (support for that board is not yet upstream).  On the back of that
> > >   panel I see Innolux P120ZDZ-EZ1 rev B1.
> > >
> > > * Someone pointed me at a datasheet that's supposed to be for the
> > >   panel in front of me (sorry, I can't share the datasheet).  That
> > >   datasheet has the string "p120zdg-bf1"
> > >
> > > * If I search for "P120ZDG-BF1" on the Internet I get hits for panels
> > >   that are 2160x1440.  They don't have datasheets, but the fact that
> > >   the resolution matches is a good sign.
> > >
> > > In any case, let's update the name and also the physical size to match
> > > the correct panel.
> > >
> > > Fixes: da50bd4258db ("drm/panel: simple: Add Innolux TV123WAM panel driver support")
> > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > Cc: Sandeep Panda <spanda@codeaurora.org>
> > > ---
> > >
> > >  drivers/gpu/drm/panel/panel-simple.c | 14 +++++++-------
> > >  1 file changed, 7 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> > > index 937e97490c30..7ee1abc5d81b 100644
> > > --- a/drivers/gpu/drm/panel/panel-simple.c
> > > +++ b/drivers/gpu/drm/panel/panel-simple.c
> > > @@ -1370,7 +1370,7 @@ static const struct panel_desc innolux_n156bge_l21 = {
> > >       },
> > >  };
> > >
> > > -static const struct drm_display_mode innolux_tv123wam_mode = {
> > > +static const struct drm_display_mode innolux_p120zdg_bf1_mode = {
> > >       .clock = 206016,
> > >       .hdisplay = 2160,
> > >       .hsync_start = 2160 + 48,
> > > @@ -1384,13 +1384,13 @@ static const struct drm_display_mode innolux_tv123wam_mode = {
> > >       .flags = DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC,
> > >  };
> > >
> > > -static const struct panel_desc innolux_tv123wam = {
> > > -     .modes = &innolux_tv123wam_mode,
> > > +static const struct panel_desc innolux_p120zdg_bf1 = {
> > > +     .modes = &innolux_p120zdg_bf1_mode,
> > >       .num_modes = 1,
> > >       .bpc = 8,
> > >       .size = {
> > > -             .width = 259,
> > > -             .height = 173,
> > > +             .width = 254,
> > > +             .height = 169,
> > >       },
> > >       .delay = {
> > >               .prepare = 200,
> > > @@ -2454,8 +2454,8 @@ static const struct of_device_id platform_of_match[] = {
> > >               .compatible = "innolux,n156bge-l21",
> > >               .data = &innolux_n156bge_l21,
> > >       }, {
> > > -             .compatible = "innolux,tv123wam",
> >
> > I think we should update the struct, but we might want to keep this around.
> > Given the tv123wam panel is TI, we're likely not going to have a collision on
> > innolux,...
> >
> > That said, I'll defer to robh on this one, I'm not sure if changing names is
> > cool once the bindings have hit mainline.
> 
> Rob gave the bindings patch a Reviewed-by tag, so I'm assuming he's
> cool with it.  v2 still doesn't keep the "innolux,tv123wam" around.
> If you disagree then let me know and I'll do a v3.

I happily defer to Rob on all things dt. So,

Reviewed-by: Sean Paul <sean@poorly.run>


> 
> -Doug

-- 
Sean Paul, Software Engineer, Google / Chromium OS