linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 00/20] drm: Fix EDID reading on ti-sn65dsi86; solve some chicken-and-egg problems
@ 2021-04-23 16:58 Douglas Anderson
  2021-04-23 16:58 ` [PATCH v5 01/20] drm/panel: panel-simple: Add missing pm_runtime_disable() calls Douglas Anderson
                   ` (19 more replies)
  0 siblings, 20 replies; 36+ messages in thread
From: Douglas Anderson @ 2021-04-23 16:58 UTC (permalink / raw)
  To: Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, Sam Ravnborg, Wolfram Sang
  Cc: linux-arm-msm, robdclark, Stanislav Lisovskiy, Stephen Boyd,
	Steev Klimaszewski, Linus W, Maarten Lankhorst, linux-i2c,
	Bjorn Andersson, dri-devel, Douglas Anderson, Andy Gross,
	Daniel Vetter, David Airlie, Laurent Pinchart, Rob Herring,
	Robert Foss, Thierry Reding, devicetree, linux-kernel

The primary goal of this series is to try to properly fix EDID reading
for eDP panels using the ti-sn65dsi86 bridge.

Previously we had a patch that added EDID reading but it turned out
not to work at bootup. This caused some extra churn at bootup as we
tried (and failed) to read the EDID several times and also ended up
forcing us to use the hardcoded mode at boot. With this patch series I
believe EDID reading is reliable at boot now and we never use the
hardcoded mode.

This series is the logical successor to the 3-part series containing
the patch ("drm/bridge: ti-sn65dsi86: Properly get the EDID, but only
if refclk") [1] though only one actual patch is the same between the
two.

This series starts out with some general / obvious fixes and moves on
to some more specific and maybe controversial ones. I wouldn't object
to some of the earlier ones landing if they look ready.

This patch was developed agains linuxnext (next-20210416) with
drm-misc-next (as of 20210423) merged in on a sc7180-trogdor-lazor
device. To get things booting for me, I had to use Stephen's patch [2]
to keep from crashing but otherwise all the patches I needed were
here.

Primary change between v2 and v3 is to stop doing the EDID caching in
the core. I also added Andrzej's review tags.

Between v3 and v4 this series grew a whole lot. I changed it so that
the EDID reading is actually driven by the panel driver now as was
suggested by Andrzej. While I still believe that the old approach
wasn't too bad I'm still switching. Why?

The main reason is that I think it's useful in general for the panel
code to have access to the DDC bus and to be able to read the
EDID. This may allow us to more easily have the panel code support
multiple sources of panels--it can read the EDID and possibly adjust
timings based on the model ID. It also allows the panel code (or
perhaps backlight code?) to send DDC commands if they are need for a
particular panel.

At the moment, once the panel is provided the DDC bus then existing
code will assume that it should be in charge of reading the
EDID. While it doesn't have to work that way, it seems sane to build
on what's already there.

In order to expose the DDC bus to the panel, I had to solve a bunch of
chicken-and-egg problems in terms of probe ordering between the bridge
and the panel. I've broken the bridge driver into several sub drivers
to make this happen. At the moment the sub-drivers are just there to
solve the probe problem, but conceivably someone could use them to
break the driver up in the future if need be.

Between v4 and v5, high-level view of changes.
- Some of the early patches landed, so dropped from series.
- New pm_runtime_disable() fix (fixed a patch that already landed).
- Added Bjorn's tags to most patches
- Fixed problems when building as a module.
- Reordered debugfs patch and fixed error handling there.
- Dropped last patch. I'm not convinced it's safe w/out more work.

I apologize in advance for the length of this series. Hopefully you
can see that there are only so many because they're broken up into
nice and reviewable bite-sized-chunks. :-)

*** NOTE ***: my hope is to actually land patches that have been
reviewed sometime mid-to-late next week. Please shout if you think
this is too much of a rush and you know of someone who is planning to
review that needs more time.

[1] https://lore.kernel.org/r/20210304155144.3.I60a7fb23ce4589006bc95c64ab8d15c74b876e68@changeid/
[2] https://lore.kernel.org/r/161706912161.3012082.17313817257247946143@swboyd.mtv.corp.google.com/

Changes in v5:
- Missing pm_runtime_disable() patch new for v5.
- Reordered to debugfs change to avoid transient issue
- Don't print debugfs creation errors.
- Handle NULL from debugfs_create_dir() which is documented possible.
- Rebased atop the pm_runtime patch, which got reordered.
- Fix module compile problems (Bjorn + kbuild bot)
- Remove useless MODULE_DEVICE_TABLE (Bjorn).
- Fix module compile problems (Bjorn + kbuild bot)
- Remove useless MODULE_DEVICE_TABLE (Bjorn).

Douglas Anderson (20):
  drm/panel: panel-simple: Add missing pm_runtime_disable() calls
  drm/bridge: ti-sn65dsi86: Rename the main driver data structure
  drm/bridge: ti-sn65dsi86: More renames in prep for sub-devices
  drm/bridge: ti-sn65dsi86: Use devm to do our runtime_disable
  drm/bridge: ti-sn65dsi86: Clean debugfs code
  drm/bridge: ti-sn65dsi86: Add local var for "dev" to simplify probe
  drm/bridge: ti-sn65dsi86: Cleanup managing of drvdata
  drm/bridge: ti-sn65dsi86: Move all the chip-related init to the start
  drm/bridge: ti-sn65dsi86: Break GPIO and MIPI-to-eDP bridge into
    sub-drivers
  drm/panel: panel-simple: Get rid of hacky HPD chicken-and-egg code
  drm/bridge: ti-sn65dsi86: Use pm_runtime autosuspend
  drm/bridge: ti-sn65dsi86: Code motion of refclk management functions
  drm/bridge: ti-sn65dsi86: If refclk, DP AUX can happen w/out
    pre-enable
  drm/bridge: ti-sn65dsi86: Promote the AUX channel to its own sub-dev
  i2c: i2c-core-of: Fix corner case of finding adapter by node
  drm/panel: panel-simple: Remove extra call:
    drm_connector_update_edid_property()
  drm/panel: panel-simple: Power the panel when reading the EDID
  drm/panel: panel-simple: Cache the EDID as long as we retain power
  drm/bridge: ti-sn65dsi86: Don't read EDID blob over DDC
  arm64: dts: qcom: Link the panel to the bridge's DDC bus

 arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi |   1 +
 drivers/gpu/drm/bridge/Kconfig               |   1 +
 drivers/gpu/drm/bridge/ti-sn65dsi86.c        | 767 ++++++++++++-------
 drivers/gpu/drm/panel/panel-simple.c         |  49 +-
 drivers/i2c/i2c-core-of.c                    |  17 +-
 5 files changed, 538 insertions(+), 297 deletions(-)

-- 
2.31.1.498.g6c1eba8ee3d-goog


^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH v5 01/20] drm/panel: panel-simple: Add missing pm_runtime_disable() calls
  2021-04-23 16:58 [PATCH v5 00/20] drm: Fix EDID reading on ti-sn65dsi86; solve some chicken-and-egg problems Douglas Anderson
@ 2021-04-23 16:58 ` Douglas Anderson
  2021-04-28 16:32   ` Sean Paul
  2021-04-30  0:58   ` Linus Walleij
  2021-04-23 16:58 ` [PATCH v5 02/20] drm/bridge: ti-sn65dsi86: Rename the main driver data structure Douglas Anderson
                   ` (18 subsequent siblings)
  19 siblings, 2 replies; 36+ messages in thread
From: Douglas Anderson @ 2021-04-23 16:58 UTC (permalink / raw)
  To: Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, Sam Ravnborg, Wolfram Sang
  Cc: linux-arm-msm, robdclark, Stanislav Lisovskiy, Stephen Boyd,
	Steev Klimaszewski, Linus W, Maarten Lankhorst, linux-i2c,
	Bjorn Andersson, dri-devel, Douglas Anderson, Daniel Vetter,
	David Airlie, Laurent Pinchart, Thierry Reding, linux-kernel

In commit 3235b0f20a0a ("drm/panel: panel-simple: Use runtime pm to
avoid excessive unprepare / prepare") we started using pm_runtime, but
my patch neglected to add the proper pm_runtime_disable(). Doh! Add
them now.

Fixes: 3235b0f20a0a ("drm/panel: panel-simple: Use runtime pm to avoid excessive unprepare / prepare")
Reported-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v5:
- Missing pm_runtime_disable() patch new for v5.

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

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 6b22872b3281..9746eda6f675 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -797,12 +797,14 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
 
 	err = drm_panel_of_backlight(&panel->base);
 	if (err)
-		goto free_ddc;
+		goto disable_pm_runtime;
 
 	drm_panel_add(&panel->base);
 
 	return 0;
 
+disable_pm_runtime:
+	pm_runtime_disable(dev);
 free_ddc:
 	if (panel->ddc)
 		put_device(&panel->ddc->dev);
@@ -818,6 +820,7 @@ static int panel_simple_remove(struct device *dev)
 	drm_panel_disable(&panel->base);
 	drm_panel_unprepare(&panel->base);
 
+	pm_runtime_disable(dev);
 	if (panel->ddc)
 		put_device(&panel->ddc->dev);
 
-- 
2.31.1.498.g6c1eba8ee3d-goog


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v5 02/20] drm/bridge: ti-sn65dsi86: Rename the main driver data structure
  2021-04-23 16:58 [PATCH v5 00/20] drm: Fix EDID reading on ti-sn65dsi86; solve some chicken-and-egg problems Douglas Anderson
  2021-04-23 16:58 ` [PATCH v5 01/20] drm/panel: panel-simple: Add missing pm_runtime_disable() calls Douglas Anderson
@ 2021-04-23 16:58 ` Douglas Anderson
  2021-04-23 16:58 ` [PATCH v5 03/20] drm/bridge: ti-sn65dsi86: More renames in prep for sub-devices Douglas Anderson
                   ` (17 subsequent siblings)
  19 siblings, 0 replies; 36+ messages in thread
From: Douglas Anderson @ 2021-04-23 16:58 UTC (permalink / raw)
  To: Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, Sam Ravnborg, Wolfram Sang
  Cc: linux-arm-msm, robdclark, Stanislav Lisovskiy, Stephen Boyd,
	Steev Klimaszewski, Linus W, Maarten Lankhorst, linux-i2c,
	Bjorn Andersson, dri-devel, Douglas Anderson, Daniel Vetter,
	David Airlie, Robert Foss, linux-kernel

In preparation for splitting this driver into sub-drivers, let's
rename the main data structure so it's clear that it's holding data
for the whole device and not just the MIPI-eDP bridge part.

This is a no-op change.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

(no changes since v1)

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

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 51db30d573c1..f00ceb9dda29 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -112,7 +112,7 @@
 #define SN_LINK_TRAINING_TRIES		10
 
 /**
- * struct ti_sn_bridge - Platform data for ti-sn65dsi86 driver.
+ * struct ti_sn65dsi86 - Platform data for ti-sn65dsi86 driver.
  * @dev:          Pointer to our device.
  * @regmap:       Regmap for accessing i2c.
  * @aux:          Our aux channel.
@@ -140,7 +140,7 @@
  *                lock so concurrent users of our 4 GPIOs don't stomp on
  *                each other's read-modify-write.
  */
-struct ti_sn_bridge {
+struct ti_sn65dsi86 {
 	struct device			*dev;
 	struct regmap			*regmap;
 	struct drm_dp_aux		aux;
@@ -180,7 +180,7 @@ static const struct regmap_config ti_sn_bridge_regmap_config = {
 	.cache_type = REGCACHE_NONE,
 };
 
-static void ti_sn_bridge_write_u16(struct ti_sn_bridge *pdata,
+static void ti_sn_bridge_write_u16(struct ti_sn65dsi86 *pdata,
 				   unsigned int reg, u16 val)
 {
 	regmap_write(pdata->regmap, reg, val & 0xFF);
@@ -189,7 +189,7 @@ static void ti_sn_bridge_write_u16(struct ti_sn_bridge *pdata,
 
 static int __maybe_unused ti_sn_bridge_resume(struct device *dev)
 {
-	struct ti_sn_bridge *pdata = dev_get_drvdata(dev);
+	struct ti_sn65dsi86 *pdata = dev_get_drvdata(dev);
 	int ret;
 
 	ret = regulator_bulk_enable(SN_REGULATOR_SUPPLY_NUM, pdata->supplies);
@@ -205,7 +205,7 @@ static int __maybe_unused ti_sn_bridge_resume(struct device *dev)
 
 static int __maybe_unused ti_sn_bridge_suspend(struct device *dev)
 {
-	struct ti_sn_bridge *pdata = dev_get_drvdata(dev);
+	struct ti_sn65dsi86 *pdata = dev_get_drvdata(dev);
 	int ret;
 
 	gpiod_set_value(pdata->enable_gpio, 0);
@@ -225,7 +225,7 @@ static const struct dev_pm_ops ti_sn_bridge_pm_ops = {
 
 static int status_show(struct seq_file *s, void *data)
 {
-	struct ti_sn_bridge *pdata = s->private;
+	struct ti_sn65dsi86 *pdata = s->private;
 	unsigned int reg, val;
 
 	seq_puts(s, "STATUS REGISTERS:\n");
@@ -245,7 +245,7 @@ static int status_show(struct seq_file *s, void *data)
 
 DEFINE_SHOW_ATTRIBUTE(status);
 
-static void ti_sn_debugfs_init(struct ti_sn_bridge *pdata)
+static void ti_sn_debugfs_init(struct ti_sn65dsi86 *pdata)
 {
 	pdata->debugfs = debugfs_create_dir(dev_name(pdata->dev), NULL);
 
@@ -253,22 +253,22 @@ static void ti_sn_debugfs_init(struct ti_sn_bridge *pdata)
 			&status_fops);
 }
 
-static void ti_sn_debugfs_remove(struct ti_sn_bridge *pdata)
+static void ti_sn_debugfs_remove(struct ti_sn65dsi86 *pdata)
 {
 	debugfs_remove_recursive(pdata->debugfs);
 	pdata->debugfs = NULL;
 }
 
 /* Connector funcs */
-static struct ti_sn_bridge *
+static struct ti_sn65dsi86 *
 connector_to_ti_sn_bridge(struct drm_connector *connector)
 {
-	return container_of(connector, struct ti_sn_bridge, connector);
+	return container_of(connector, struct ti_sn65dsi86, connector);
 }
 
 static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
 {
-	struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
+	struct ti_sn65dsi86 *pdata = connector_to_ti_sn_bridge(connector);
 	struct edid *edid = pdata->edid;
 	int num, ret;
 
@@ -314,12 +314,12 @@ static const struct drm_connector_funcs ti_sn_bridge_connector_funcs = {
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
-static struct ti_sn_bridge *bridge_to_ti_sn_bridge(struct drm_bridge *bridge)
+static struct ti_sn65dsi86 *bridge_to_ti_sn_bridge(struct drm_bridge *bridge)
 {
-	return container_of(bridge, struct ti_sn_bridge, bridge);
+	return container_of(bridge, struct ti_sn65dsi86, bridge);
 }
 
-static int ti_sn_bridge_parse_regulators(struct ti_sn_bridge *pdata)
+static int ti_sn_bridge_parse_regulators(struct ti_sn65dsi86 *pdata)
 {
 	unsigned int i;
 	const char * const ti_sn_bridge_supply_names[] = {
@@ -337,7 +337,7 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
 			       enum drm_bridge_attach_flags flags)
 {
 	int ret, val;
-	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn_bridge(bridge);
 	struct mipi_dsi_host *host;
 	struct mipi_dsi_device *dsi;
 	const struct mipi_dsi_device_info info = { .type = "ti_sn_bridge",
@@ -430,7 +430,7 @@ static void ti_sn_bridge_detach(struct drm_bridge *bridge)
 
 static void ti_sn_bridge_disable(struct drm_bridge *bridge)
 {
-	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn_bridge(bridge);
 
 	drm_panel_disable(pdata->panel);
 
@@ -442,7 +442,7 @@ static void ti_sn_bridge_disable(struct drm_bridge *bridge)
 	regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 0);
 }
 
-static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn_bridge *pdata)
+static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn65dsi86 *pdata)
 {
 	u32 bit_rate_khz, clk_freq_khz;
 	struct drm_display_mode *mode =
@@ -473,7 +473,7 @@ static const u32 ti_sn_bridge_dsiclk_lut[] = {
 	460800000,
 };
 
-static void ti_sn_bridge_set_refclk_freq(struct ti_sn_bridge *pdata)
+static void ti_sn_bridge_set_refclk_freq(struct ti_sn65dsi86 *pdata)
 {
 	int i;
 	u32 refclk_rate;
@@ -500,7 +500,7 @@ static void ti_sn_bridge_set_refclk_freq(struct ti_sn_bridge *pdata)
 			   REFCLK_FREQ(i));
 }
 
-static void ti_sn_bridge_set_dsi_rate(struct ti_sn_bridge *pdata)
+static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)
 {
 	unsigned int bit_rate_mhz, clk_freq_mhz;
 	unsigned int val;
@@ -518,7 +518,7 @@ static void ti_sn_bridge_set_dsi_rate(struct ti_sn_bridge *pdata)
 	regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val);
 }
 
-static unsigned int ti_sn_bridge_get_bpp(struct ti_sn_bridge *pdata)
+static unsigned int ti_sn_bridge_get_bpp(struct ti_sn65dsi86 *pdata)
 {
 	if (pdata->connector.display_info.bpc <= 6)
 		return 18;
@@ -535,7 +535,7 @@ static const unsigned int ti_sn_bridge_dp_rate_lut[] = {
 	0, 1620, 2160, 2430, 2700, 3240, 4320, 5400
 };
 
-static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn_bridge *pdata)
+static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata)
 {
 	unsigned int bit_rate_khz, dp_rate_mhz;
 	unsigned int i;
@@ -556,7 +556,7 @@ static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn_bridge *pdata)
 	return i;
 }
 
-static void ti_sn_bridge_read_valid_rates(struct ti_sn_bridge *pdata,
+static void ti_sn_bridge_read_valid_rates(struct ti_sn65dsi86 *pdata,
 					  bool rate_valid[])
 {
 	unsigned int rate_per_200khz;
@@ -637,7 +637,7 @@ static void ti_sn_bridge_read_valid_rates(struct ti_sn_bridge *pdata,
 	}
 }
 
-static void ti_sn_bridge_set_video_timings(struct ti_sn_bridge *pdata)
+static void ti_sn_bridge_set_video_timings(struct ti_sn65dsi86 *pdata)
 {
 	struct drm_display_mode *mode =
 		&pdata->bridge.encoder->crtc->state->adjusted_mode;
@@ -676,7 +676,7 @@ static void ti_sn_bridge_set_video_timings(struct ti_sn_bridge *pdata)
 	usleep_range(10000, 10500); /* 10ms delay recommended by spec */
 }
 
-static unsigned int ti_sn_get_max_lanes(struct ti_sn_bridge *pdata)
+static unsigned int ti_sn_get_max_lanes(struct ti_sn65dsi86 *pdata)
 {
 	u8 data;
 	int ret;
@@ -691,7 +691,7 @@ static unsigned int ti_sn_get_max_lanes(struct ti_sn_bridge *pdata)
 	return data & DP_LANE_COUNT_MASK;
 }
 
-static int ti_sn_link_training(struct ti_sn_bridge *pdata, int dp_rate_idx,
+static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
 			       const char **last_err_str)
 {
 	unsigned int val;
@@ -751,7 +751,7 @@ static int ti_sn_link_training(struct ti_sn_bridge *pdata, int dp_rate_idx,
 
 static void ti_sn_bridge_enable(struct drm_bridge *bridge)
 {
-	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn_bridge(bridge);
 	bool rate_valid[ARRAY_SIZE(ti_sn_bridge_dp_rate_lut)] = { };
 	const char *last_err_str = "No supported DP rate";
 	int dp_rate_idx;
@@ -822,7 +822,7 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
 
 static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
 {
-	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn_bridge(bridge);
 
 	pm_runtime_get_sync(pdata->dev);
 
@@ -853,7 +853,7 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
 
 static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
 {
-	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn_bridge(bridge);
 
 	drm_panel_unprepare(pdata->panel);
 
@@ -871,15 +871,15 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
 	.post_disable = ti_sn_bridge_post_disable,
 };
 
-static struct ti_sn_bridge *aux_to_ti_sn_bridge(struct drm_dp_aux *aux)
+static struct ti_sn65dsi86 *aux_to_ti_sn_bridge(struct drm_dp_aux *aux)
 {
-	return container_of(aux, struct ti_sn_bridge, aux);
+	return container_of(aux, struct ti_sn65dsi86, aux);
 }
 
 static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
 				  struct drm_dp_aux_msg *msg)
 {
-	struct ti_sn_bridge *pdata = aux_to_ti_sn_bridge(aux);
+	struct ti_sn65dsi86 *pdata = aux_to_ti_sn_bridge(aux);
 	u32 request = msg->request & ~(DP_AUX_I2C_MOT | DP_AUX_I2C_WRITE_STATUS_UPDATE);
 	u32 request_val = AUX_CMD_REQ(msg->request);
 	u8 *buf = msg->buffer;
@@ -969,7 +969,7 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
 	return len;
 }
 
-static int ti_sn_bridge_parse_dsi_host(struct ti_sn_bridge *pdata)
+static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
 {
 	struct device_node *np = pdata->dev->of_node;
 
@@ -1004,7 +1004,7 @@ static int tn_sn_bridge_of_xlate(struct gpio_chip *chip,
 static int ti_sn_bridge_gpio_get_direction(struct gpio_chip *chip,
 					   unsigned int offset)
 {
-	struct ti_sn_bridge *pdata = gpiochip_get_data(chip);
+	struct ti_sn65dsi86 *pdata = gpiochip_get_data(chip);
 
 	/*
 	 * We already have to keep track of the direction because we use
@@ -1018,7 +1018,7 @@ static int ti_sn_bridge_gpio_get_direction(struct gpio_chip *chip,
 
 static int ti_sn_bridge_gpio_get(struct gpio_chip *chip, unsigned int offset)
 {
-	struct ti_sn_bridge *pdata = gpiochip_get_data(chip);
+	struct ti_sn65dsi86 *pdata = gpiochip_get_data(chip);
 	unsigned int val;
 	int ret;
 
@@ -1043,7 +1043,7 @@ static int ti_sn_bridge_gpio_get(struct gpio_chip *chip, unsigned int offset)
 static void ti_sn_bridge_gpio_set(struct gpio_chip *chip, unsigned int offset,
 				  int val)
 {
-	struct ti_sn_bridge *pdata = gpiochip_get_data(chip);
+	struct ti_sn65dsi86 *pdata = gpiochip_get_data(chip);
 	int ret;
 
 	if (!test_bit(offset, pdata->gchip_output)) {
@@ -1063,7 +1063,7 @@ static void ti_sn_bridge_gpio_set(struct gpio_chip *chip, unsigned int offset,
 static int ti_sn_bridge_gpio_direction_input(struct gpio_chip *chip,
 					     unsigned int offset)
 {
-	struct ti_sn_bridge *pdata = gpiochip_get_data(chip);
+	struct ti_sn65dsi86 *pdata = gpiochip_get_data(chip);
 	int shift = offset * 2;
 	int ret;
 
@@ -1091,7 +1091,7 @@ static int ti_sn_bridge_gpio_direction_input(struct gpio_chip *chip,
 static int ti_sn_bridge_gpio_direction_output(struct gpio_chip *chip,
 					      unsigned int offset, int val)
 {
-	struct ti_sn_bridge *pdata = gpiochip_get_data(chip);
+	struct ti_sn65dsi86 *pdata = gpiochip_get_data(chip);
 	int shift = offset * 2;
 	int ret;
 
@@ -1125,7 +1125,7 @@ static const char * const ti_sn_bridge_gpio_names[SN_NUM_GPIOS] = {
 	"GPIO1", "GPIO2", "GPIO3", "GPIO4"
 };
 
-static int ti_sn_setup_gpio_controller(struct ti_sn_bridge *pdata)
+static int ti_sn_setup_gpio_controller(struct ti_sn65dsi86 *pdata)
 {
 	int ret;
 
@@ -1157,14 +1157,14 @@ static int ti_sn_setup_gpio_controller(struct ti_sn_bridge *pdata)
 
 #else
 
-static inline int ti_sn_setup_gpio_controller(struct ti_sn_bridge *pdata)
+static inline int ti_sn_setup_gpio_controller(struct ti_sn65dsi86 *pdata)
 {
 	return 0;
 }
 
 #endif
 
-static void ti_sn_bridge_parse_lanes(struct ti_sn_bridge *pdata,
+static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,
 				     struct device_node *np)
 {
 	u32 lane_assignments[SN_MAX_DP_LANES] = { 0, 1, 2, 3 };
@@ -1216,7 +1216,7 @@ static void ti_sn_bridge_parse_lanes(struct ti_sn_bridge *pdata,
 static int ti_sn_bridge_probe(struct i2c_client *client,
 			      const struct i2c_device_id *id)
 {
-	struct ti_sn_bridge *pdata;
+	struct ti_sn65dsi86 *pdata;
 	int ret;
 
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
@@ -1224,7 +1224,7 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
 		return -ENODEV;
 	}
 
-	pdata = devm_kzalloc(&client->dev, sizeof(struct ti_sn_bridge),
+	pdata = devm_kzalloc(&client->dev, sizeof(struct ti_sn65dsi86),
 			     GFP_KERNEL);
 	if (!pdata)
 		return -ENOMEM;
@@ -1298,7 +1298,7 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
 
 static int ti_sn_bridge_remove(struct i2c_client *client)
 {
-	struct ti_sn_bridge *pdata = i2c_get_clientdata(client);
+	struct ti_sn65dsi86 *pdata = i2c_get_clientdata(client);
 
 	if (!pdata)
 		return -EINVAL;
-- 
2.31.1.498.g6c1eba8ee3d-goog


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v5 03/20] drm/bridge: ti-sn65dsi86: More renames in prep for sub-devices
  2021-04-23 16:58 [PATCH v5 00/20] drm: Fix EDID reading on ti-sn65dsi86; solve some chicken-and-egg problems Douglas Anderson
  2021-04-23 16:58 ` [PATCH v5 01/20] drm/panel: panel-simple: Add missing pm_runtime_disable() calls Douglas Anderson
  2021-04-23 16:58 ` [PATCH v5 02/20] drm/bridge: ti-sn65dsi86: Rename the main driver data structure Douglas Anderson
@ 2021-04-23 16:58 ` Douglas Anderson
  2021-04-23 16:58 ` [PATCH v5 04/20] drm/bridge: ti-sn65dsi86: Use devm to do our runtime_disable Douglas Anderson
                   ` (16 subsequent siblings)
  19 siblings, 0 replies; 36+ messages in thread
From: Douglas Anderson @ 2021-04-23 16:58 UTC (permalink / raw)
  To: Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, Sam Ravnborg, Wolfram Sang
  Cc: linux-arm-msm, robdclark, Stanislav Lisovskiy, Stephen Boyd,
	Steev Klimaszewski, Linus W, Maarten Lankhorst, linux-i2c,
	Bjorn Andersson, dri-devel, Douglas Anderson, Daniel Vetter,
	David Airlie, Robert Foss, linux-kernel

Like the previous patch ("drm/bridge: ti-sn65dsi86: Rename the main
driver data structure") this is just a no-op rename in preparation for
splitting the driver up a bit.

Here I've attempted to rename functions / structures making sure that
anything applicable to the whole chip (instead of just the MIPI to eDP
bridge part) included "sn65dsi86" somewhere in the name instead of
just "ti_sn_bridge".

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

(no changes since v1)

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

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index f00ceb9dda29..57574132e200 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -164,30 +164,30 @@ struct ti_sn65dsi86 {
 #endif
 };
 
-static const struct regmap_range ti_sn_bridge_volatile_ranges[] = {
+static const struct regmap_range ti_sn65dsi86_volatile_ranges[] = {
 	{ .range_min = 0, .range_max = 0xFF },
 };
 
 static const struct regmap_access_table ti_sn_bridge_volatile_table = {
-	.yes_ranges = ti_sn_bridge_volatile_ranges,
-	.n_yes_ranges = ARRAY_SIZE(ti_sn_bridge_volatile_ranges),
+	.yes_ranges = ti_sn65dsi86_volatile_ranges,
+	.n_yes_ranges = ARRAY_SIZE(ti_sn65dsi86_volatile_ranges),
 };
 
-static const struct regmap_config ti_sn_bridge_regmap_config = {
+static const struct regmap_config ti_sn65dsi86_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
 	.volatile_table = &ti_sn_bridge_volatile_table,
 	.cache_type = REGCACHE_NONE,
 };
 
-static void ti_sn_bridge_write_u16(struct ti_sn65dsi86 *pdata,
+static void ti_sn65dsi86_write_u16(struct ti_sn65dsi86 *pdata,
 				   unsigned int reg, u16 val)
 {
 	regmap_write(pdata->regmap, reg, val & 0xFF);
 	regmap_write(pdata->regmap, reg + 1, val >> 8);
 }
 
-static int __maybe_unused ti_sn_bridge_resume(struct device *dev)
+static int __maybe_unused ti_sn65dsi86_resume(struct device *dev)
 {
 	struct ti_sn65dsi86 *pdata = dev_get_drvdata(dev);
 	int ret;
@@ -203,7 +203,7 @@ static int __maybe_unused ti_sn_bridge_resume(struct device *dev)
 	return ret;
 }
 
-static int __maybe_unused ti_sn_bridge_suspend(struct device *dev)
+static int __maybe_unused ti_sn65dsi86_suspend(struct device *dev)
 {
 	struct ti_sn65dsi86 *pdata = dev_get_drvdata(dev);
 	int ret;
@@ -217,8 +217,8 @@ static int __maybe_unused ti_sn_bridge_suspend(struct device *dev)
 	return ret;
 }
 
-static const struct dev_pm_ops ti_sn_bridge_pm_ops = {
-	SET_RUNTIME_PM_OPS(ti_sn_bridge_suspend, ti_sn_bridge_resume, NULL)
+static const struct dev_pm_ops ti_sn65dsi86_pm_ops = {
+	SET_RUNTIME_PM_OPS(ti_sn65dsi86_suspend, ti_sn65dsi86_resume, NULL)
 	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
 				pm_runtime_force_resume)
 };
@@ -245,7 +245,7 @@ static int status_show(struct seq_file *s, void *data)
 
 DEFINE_SHOW_ATTRIBUTE(status);
 
-static void ti_sn_debugfs_init(struct ti_sn65dsi86 *pdata)
+static void ti_sn65dsi86_debugfs_init(struct ti_sn65dsi86 *pdata)
 {
 	pdata->debugfs = debugfs_create_dir(dev_name(pdata->dev), NULL);
 
@@ -253,7 +253,7 @@ static void ti_sn_debugfs_init(struct ti_sn65dsi86 *pdata)
 			&status_fops);
 }
 
-static void ti_sn_debugfs_remove(struct ti_sn65dsi86 *pdata)
+static void ti_sn65dsi86_debugfs_remove(struct ti_sn65dsi86 *pdata)
 {
 	debugfs_remove_recursive(pdata->debugfs);
 	pdata->debugfs = NULL;
@@ -261,14 +261,14 @@ static void ti_sn_debugfs_remove(struct ti_sn65dsi86 *pdata)
 
 /* Connector funcs */
 static struct ti_sn65dsi86 *
-connector_to_ti_sn_bridge(struct drm_connector *connector)
+connector_to_ti_sn65dsi86(struct drm_connector *connector)
 {
 	return container_of(connector, struct ti_sn65dsi86, connector);
 }
 
 static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
 {
-	struct ti_sn65dsi86 *pdata = connector_to_ti_sn_bridge(connector);
+	struct ti_sn65dsi86 *pdata = connector_to_ti_sn65dsi86(connector);
 	struct edid *edid = pdata->edid;
 	int num, ret;
 
@@ -314,12 +314,12 @@ static const struct drm_connector_funcs ti_sn_bridge_connector_funcs = {
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
-static struct ti_sn65dsi86 *bridge_to_ti_sn_bridge(struct drm_bridge *bridge)
+static struct ti_sn65dsi86 *bridge_to_ti_sn65dsi86(struct drm_bridge *bridge)
 {
 	return container_of(bridge, struct ti_sn65dsi86, bridge);
 }
 
-static int ti_sn_bridge_parse_regulators(struct ti_sn65dsi86 *pdata)
+static int ti_sn65dsi86_parse_regulators(struct ti_sn65dsi86 *pdata)
 {
 	unsigned int i;
 	const char * const ti_sn_bridge_supply_names[] = {
@@ -337,7 +337,7 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
 			       enum drm_bridge_attach_flags flags)
 {
 	int ret, val;
-	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn_bridge(bridge);
+	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
 	struct mipi_dsi_host *host;
 	struct mipi_dsi_device *dsi;
 	const struct mipi_dsi_device_info info = { .type = "ti_sn_bridge",
@@ -425,12 +425,12 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
 
 static void ti_sn_bridge_detach(struct drm_bridge *bridge)
 {
-	drm_dp_aux_unregister(&bridge_to_ti_sn_bridge(bridge)->aux);
+	drm_dp_aux_unregister(&bridge_to_ti_sn65dsi86(bridge)->aux);
 }
 
 static void ti_sn_bridge_disable(struct drm_bridge *bridge)
 {
-	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn_bridge(bridge);
+	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
 
 	drm_panel_disable(pdata->panel);
 
@@ -648,9 +648,9 @@ static void ti_sn_bridge_set_video_timings(struct ti_sn65dsi86 *pdata)
 	if (mode->flags & DRM_MODE_FLAG_PVSYNC)
 		vsync_polarity = CHA_VSYNC_POLARITY;
 
-	ti_sn_bridge_write_u16(pdata, SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG,
+	ti_sn65dsi86_write_u16(pdata, SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG,
 			       mode->hdisplay);
-	ti_sn_bridge_write_u16(pdata, SN_CHA_VERTICAL_DISPLAY_SIZE_LOW_REG,
+	ti_sn65dsi86_write_u16(pdata, SN_CHA_VERTICAL_DISPLAY_SIZE_LOW_REG,
 			       mode->vdisplay);
 	regmap_write(pdata->regmap, SN_CHA_HSYNC_PULSE_WIDTH_LOW_REG,
 		     (mode->hsync_end - mode->hsync_start) & 0xFF);
@@ -751,7 +751,7 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
 
 static void ti_sn_bridge_enable(struct drm_bridge *bridge)
 {
-	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn_bridge(bridge);
+	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
 	bool rate_valid[ARRAY_SIZE(ti_sn_bridge_dp_rate_lut)] = { };
 	const char *last_err_str = "No supported DP rate";
 	int dp_rate_idx;
@@ -822,7 +822,7 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
 
 static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
 {
-	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn_bridge(bridge);
+	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
 
 	pm_runtime_get_sync(pdata->dev);
 
@@ -853,7 +853,7 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
 
 static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
 {
-	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn_bridge(bridge);
+	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
 
 	drm_panel_unprepare(pdata->panel);
 
@@ -871,7 +871,7 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
 	.post_disable = ti_sn_bridge_post_disable,
 };
 
-static struct ti_sn65dsi86 *aux_to_ti_sn_bridge(struct drm_dp_aux *aux)
+static struct ti_sn65dsi86 *aux_to_ti_sn65dsi86(struct drm_dp_aux *aux)
 {
 	return container_of(aux, struct ti_sn65dsi86, aux);
 }
@@ -879,7 +879,7 @@ static struct ti_sn65dsi86 *aux_to_ti_sn_bridge(struct drm_dp_aux *aux)
 static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
 				  struct drm_dp_aux_msg *msg)
 {
-	struct ti_sn65dsi86 *pdata = aux_to_ti_sn_bridge(aux);
+	struct ti_sn65dsi86 *pdata = aux_to_ti_sn65dsi86(aux);
 	u32 request = msg->request & ~(DP_AUX_I2C_MOT | DP_AUX_I2C_WRITE_STATUS_UPDATE);
 	u32 request_val = AUX_CMD_REQ(msg->request);
 	u8 *buf = msg->buffer;
@@ -1213,7 +1213,7 @@ static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,
 	pdata->ln_polrs = ln_polrs;
 }
 
-static int ti_sn_bridge_probe(struct i2c_client *client,
+static int ti_sn65dsi86_probe(struct i2c_client *client,
 			      const struct i2c_device_id *id)
 {
 	struct ti_sn65dsi86 *pdata;
@@ -1230,7 +1230,7 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
 		return -ENOMEM;
 
 	pdata->regmap = devm_regmap_init_i2c(client,
-					     &ti_sn_bridge_regmap_config);
+					     &ti_sn65dsi86_regmap_config);
 	if (IS_ERR(pdata->regmap)) {
 		DRM_ERROR("regmap i2c init failed\n");
 		return PTR_ERR(pdata->regmap);
@@ -1257,7 +1257,7 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
 
 	ti_sn_bridge_parse_lanes(pdata, client->dev.of_node);
 
-	ret = ti_sn_bridge_parse_regulators(pdata);
+	ret = ti_sn65dsi86_parse_regulators(pdata);
 	if (ret) {
 		DRM_ERROR("failed to parse regulators\n");
 		return ret;
@@ -1291,12 +1291,12 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
 
 	drm_bridge_add(&pdata->bridge);
 
-	ti_sn_debugfs_init(pdata);
+	ti_sn65dsi86_debugfs_init(pdata);
 
 	return 0;
 }
 
-static int ti_sn_bridge_remove(struct i2c_client *client)
+static int ti_sn65dsi86_remove(struct i2c_client *client)
 {
 	struct ti_sn65dsi86 *pdata = i2c_get_clientdata(client);
 
@@ -1310,7 +1310,7 @@ static int ti_sn_bridge_remove(struct i2c_client *client)
 
 	kfree(pdata->edid);
 
-	ti_sn_debugfs_remove(pdata);
+	ti_sn65dsi86_debugfs_remove(pdata);
 
 	drm_bridge_remove(&pdata->bridge);
 
@@ -1321,29 +1321,29 @@ static int ti_sn_bridge_remove(struct i2c_client *client)
 	return 0;
 }
 
-static struct i2c_device_id ti_sn_bridge_id[] = {
+static struct i2c_device_id ti_sn65dsi86_id[] = {
 	{ "ti,sn65dsi86", 0},
 	{},
 };
-MODULE_DEVICE_TABLE(i2c, ti_sn_bridge_id);
+MODULE_DEVICE_TABLE(i2c, ti_sn65dsi86_id);
 
-static const struct of_device_id ti_sn_bridge_match_table[] = {
+static const struct of_device_id ti_sn65dsi86_match_table[] = {
 	{.compatible = "ti,sn65dsi86"},
 	{},
 };
-MODULE_DEVICE_TABLE(of, ti_sn_bridge_match_table);
+MODULE_DEVICE_TABLE(of, ti_sn65dsi86_match_table);
 
-static struct i2c_driver ti_sn_bridge_driver = {
+static struct i2c_driver ti_sn65dsi86_driver = {
 	.driver = {
 		.name = "ti_sn65dsi86",
-		.of_match_table = ti_sn_bridge_match_table,
-		.pm = &ti_sn_bridge_pm_ops,
+		.of_match_table = ti_sn65dsi86_match_table,
+		.pm = &ti_sn65dsi86_pm_ops,
 	},
-	.probe = ti_sn_bridge_probe,
-	.remove = ti_sn_bridge_remove,
-	.id_table = ti_sn_bridge_id,
+	.probe = ti_sn65dsi86_probe,
+	.remove = ti_sn65dsi86_remove,
+	.id_table = ti_sn65dsi86_id,
 };
-module_i2c_driver(ti_sn_bridge_driver);
+module_i2c_driver(ti_sn65dsi86_driver);
 
 MODULE_AUTHOR("Sandeep Panda <spanda@codeaurora.org>");
 MODULE_DESCRIPTION("sn65dsi86 DSI to eDP bridge driver");
-- 
2.31.1.498.g6c1eba8ee3d-goog


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v5 04/20] drm/bridge: ti-sn65dsi86: Use devm to do our runtime_disable
  2021-04-23 16:58 [PATCH v5 00/20] drm: Fix EDID reading on ti-sn65dsi86; solve some chicken-and-egg problems Douglas Anderson
                   ` (2 preceding siblings ...)
  2021-04-23 16:58 ` [PATCH v5 03/20] drm/bridge: ti-sn65dsi86: More renames in prep for sub-devices Douglas Anderson
@ 2021-04-23 16:58 ` Douglas Anderson
  2021-04-23 16:58 ` [PATCH v5 05/20] drm/bridge: ti-sn65dsi86: Clean debugfs code Douglas Anderson
                   ` (15 subsequent siblings)
  19 siblings, 0 replies; 36+ messages in thread
From: Douglas Anderson @ 2021-04-23 16:58 UTC (permalink / raw)
  To: Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, Sam Ravnborg, Wolfram Sang
  Cc: linux-arm-msm, robdclark, Stanislav Lisovskiy, Stephen Boyd,
	Steev Klimaszewski, Linus W, Maarten Lankhorst, linux-i2c,
	Bjorn Andersson, dri-devel, Douglas Anderson, Daniel Vetter,
	David Airlie, Robert Foss, linux-kernel

There's no devm_runtime_enable(), but it's easy to use
devm_add_action_or_reset() and means we don't need to worry about the
disable in our remove() routine or in error paths.

No functional changes intended by this change.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes in v5:
- Reordered to debugfs change to avoid transient issue

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 57574132e200..44d8395505f0 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -1213,6 +1213,11 @@ static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,
 	pdata->ln_polrs = ln_polrs;
 }
 
+static void ti_sn65dsi86_runtime_disable(void *data)
+{
+	pm_runtime_disable(data);
+}
+
 static int ti_sn65dsi86_probe(struct i2c_client *client,
 			      const struct i2c_device_id *id)
 {
@@ -1272,12 +1277,13 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
 		return ret;
 
 	pm_runtime_enable(pdata->dev);
+	ret = devm_add_action_or_reset(pdata->dev, ti_sn65dsi86_runtime_disable, pdata->dev);
+	if (ret)
+		return ret;
 
 	ret = ti_sn_setup_gpio_controller(pdata);
-	if (ret) {
-		pm_runtime_disable(pdata->dev);
+	if (ret)
 		return ret;
-	}
 
 	i2c_set_clientdata(client, pdata);
 
@@ -1314,8 +1320,6 @@ static int ti_sn65dsi86_remove(struct i2c_client *client)
 
 	drm_bridge_remove(&pdata->bridge);
 
-	pm_runtime_disable(pdata->dev);
-
 	of_node_put(pdata->host_node);
 
 	return 0;
-- 
2.31.1.498.g6c1eba8ee3d-goog


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v5 05/20] drm/bridge: ti-sn65dsi86: Clean debugfs code
  2021-04-23 16:58 [PATCH v5 00/20] drm: Fix EDID reading on ti-sn65dsi86; solve some chicken-and-egg problems Douglas Anderson
                   ` (3 preceding siblings ...)
  2021-04-23 16:58 ` [PATCH v5 04/20] drm/bridge: ti-sn65dsi86: Use devm to do our runtime_disable Douglas Anderson
@ 2021-04-23 16:58 ` Douglas Anderson
  2021-04-23 16:58 ` [PATCH v5 06/20] drm/bridge: ti-sn65dsi86: Add local var for "dev" to simplify probe Douglas Anderson
                   ` (14 subsequent siblings)
  19 siblings, 0 replies; 36+ messages in thread
From: Douglas Anderson @ 2021-04-23 16:58 UTC (permalink / raw)
  To: Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, Sam Ravnborg, Wolfram Sang
  Cc: linux-arm-msm, robdclark, Stanislav Lisovskiy, Stephen Boyd,
	Steev Klimaszewski, Linus W, Maarten Lankhorst, linux-i2c,
	Bjorn Andersson, dri-devel, Douglas Anderson, Daniel Vetter,
	David Airlie, Robert Foss, linux-kernel

Let's cleanup the debugfs code to:
- Check for errors.
- Use devm to manage freeing, which also means we don't need to store
  a pointer in our structure.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
Bjorn: I left off your tag on this patch since I made changes compared
to v4. Can you re-add if it still looks OK? This is now ordered
_after_ the pm_runtime patch so I believe the ordering problem you
pointed out should be fixed as well?

Changes in v5:
- Don't print debugfs creation errors.
- Handle NULL from debugfs_create_dir() which is documented possible.

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 33 +++++++++++++++++----------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 44d8395505f0..8aa36074aab9 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -118,7 +118,6 @@
  * @aux:          Our aux channel.
  * @bridge:       Our bridge.
  * @connector:    Our connector.
- * @debugfs:      Used for managing our debugfs.
  * @host_node:    Remote DSI node.
  * @dsi:          Our MIPI DSI source.
  * @edid:         Detected EDID of eDP panel.
@@ -146,7 +145,6 @@ struct ti_sn65dsi86 {
 	struct drm_dp_aux		aux;
 	struct drm_bridge		bridge;
 	struct drm_connector		connector;
-	struct dentry			*debugfs;
 	struct edid			*edid;
 	struct device_node		*host_node;
 	struct mipi_dsi_device		*dsi;
@@ -245,18 +243,31 @@ static int status_show(struct seq_file *s, void *data)
 
 DEFINE_SHOW_ATTRIBUTE(status);
 
-static void ti_sn65dsi86_debugfs_init(struct ti_sn65dsi86 *pdata)
+static void ti_sn65dsi86_debugfs_remove(void *data)
 {
-	pdata->debugfs = debugfs_create_dir(dev_name(pdata->dev), NULL);
-
-	debugfs_create_file("status", 0600, pdata->debugfs, pdata,
-			&status_fops);
+	debugfs_remove_recursive(data);
 }
 
-static void ti_sn65dsi86_debugfs_remove(struct ti_sn65dsi86 *pdata)
+static void ti_sn65dsi86_debugfs_init(struct ti_sn65dsi86 *pdata)
 {
-	debugfs_remove_recursive(pdata->debugfs);
-	pdata->debugfs = NULL;
+	struct device *dev = pdata->dev;
+	struct dentry *debugfs;
+	int ret;
+
+	debugfs = debugfs_create_dir(dev_name(dev), NULL);
+
+	/*
+	 * We might get an error back if debugfs wasn't enabled in the kernel
+	 * so let's just silently return upon failure.
+	 */
+	if (IS_ERR_OR_NULL(debugfs))
+		return;
+
+	ret = devm_add_action_or_reset(dev, ti_sn65dsi86_debugfs_remove, debugfs);
+	if (ret)
+		return;
+
+	debugfs_create_file("status", 0600, debugfs, pdata, &status_fops);
 }
 
 /* Connector funcs */
@@ -1316,8 +1327,6 @@ static int ti_sn65dsi86_remove(struct i2c_client *client)
 
 	kfree(pdata->edid);
 
-	ti_sn65dsi86_debugfs_remove(pdata);
-
 	drm_bridge_remove(&pdata->bridge);
 
 	of_node_put(pdata->host_node);
-- 
2.31.1.498.g6c1eba8ee3d-goog


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v5 06/20] drm/bridge: ti-sn65dsi86: Add local var for "dev" to simplify probe
  2021-04-23 16:58 [PATCH v5 00/20] drm: Fix EDID reading on ti-sn65dsi86; solve some chicken-and-egg problems Douglas Anderson
                   ` (4 preceding siblings ...)
  2021-04-23 16:58 ` [PATCH v5 05/20] drm/bridge: ti-sn65dsi86: Clean debugfs code Douglas Anderson
@ 2021-04-23 16:58 ` Douglas Anderson
  2021-04-23 16:58 ` [PATCH v5 07/20] drm/bridge: ti-sn65dsi86: Cleanup managing of drvdata Douglas Anderson
                   ` (13 subsequent siblings)
  19 siblings, 0 replies; 36+ messages in thread
From: Douglas Anderson @ 2021-04-23 16:58 UTC (permalink / raw)
  To: Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, Sam Ravnborg, Wolfram Sang
  Cc: linux-arm-msm, robdclark, Stanislav Lisovskiy, Stephen Boyd,
	Steev Klimaszewski, Linus W, Maarten Lankhorst, linux-i2c,
	Bjorn Andersson, dri-devel, Douglas Anderson, Daniel Vetter,
	David Airlie, Robert Foss, linux-kernel

Tiny cleanup for probe so we don't keep having to specify
"&client->dev" or "pdata->dev". No functional changes intended.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes in v5:
- Rebased atop the pm_runtime patch, which got reordered.

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 8aa36074aab9..c868193f5b8f 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -1232,6 +1232,7 @@ static void ti_sn65dsi86_runtime_disable(void *data)
 static int ti_sn65dsi86_probe(struct i2c_client *client,
 			      const struct i2c_device_id *id)
 {
+	struct device *dev = &client->dev;
 	struct ti_sn65dsi86 *pdata;
 	int ret;
 
@@ -1240,8 +1241,7 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
 		return -ENODEV;
 	}
 
-	pdata = devm_kzalloc(&client->dev, sizeof(struct ti_sn65dsi86),
-			     GFP_KERNEL);
+	pdata = devm_kzalloc(dev, sizeof(struct ti_sn65dsi86), GFP_KERNEL);
 	if (!pdata)
 		return -ENOMEM;
 
@@ -1252,26 +1252,24 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
 		return PTR_ERR(pdata->regmap);
 	}
 
-	pdata->dev = &client->dev;
+	pdata->dev = dev;
 
-	ret = drm_of_find_panel_or_bridge(pdata->dev->of_node, 1, 0,
-					  &pdata->panel, NULL);
+	ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &pdata->panel, NULL);
 	if (ret) {
 		DRM_ERROR("could not find any panel node\n");
 		return ret;
 	}
 
-	dev_set_drvdata(&client->dev, pdata);
+	dev_set_drvdata(dev, pdata);
 
-	pdata->enable_gpio = devm_gpiod_get(pdata->dev, "enable",
-					    GPIOD_OUT_LOW);
+	pdata->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
 	if (IS_ERR(pdata->enable_gpio)) {
 		DRM_ERROR("failed to get enable gpio from DT\n");
 		ret = PTR_ERR(pdata->enable_gpio);
 		return ret;
 	}
 
-	ti_sn_bridge_parse_lanes(pdata, client->dev.of_node);
+	ti_sn_bridge_parse_lanes(pdata, dev->of_node);
 
 	ret = ti_sn65dsi86_parse_regulators(pdata);
 	if (ret) {
@@ -1279,7 +1277,7 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
 		return ret;
 	}
 
-	pdata->refclk = devm_clk_get_optional(pdata->dev, "refclk");
+	pdata->refclk = devm_clk_get_optional(dev, "refclk");
 	if (IS_ERR(pdata->refclk))
 		return PTR_ERR(pdata->refclk);
 
@@ -1287,8 +1285,8 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
 	if (ret)
 		return ret;
 
-	pm_runtime_enable(pdata->dev);
-	ret = devm_add_action_or_reset(pdata->dev, ti_sn65dsi86_runtime_disable, pdata->dev);
+	pm_runtime_enable(dev);
+	ret = devm_add_action_or_reset(dev, ti_sn65dsi86_runtime_disable, dev);
 	if (ret)
 		return ret;
 
@@ -1299,12 +1297,12 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
 	i2c_set_clientdata(client, pdata);
 
 	pdata->aux.name = "ti-sn65dsi86-aux";
-	pdata->aux.dev = pdata->dev;
+	pdata->aux.dev = dev;
 	pdata->aux.transfer = ti_sn_aux_transfer;
 	drm_dp_aux_init(&pdata->aux);
 
 	pdata->bridge.funcs = &ti_sn_bridge_funcs;
-	pdata->bridge.of_node = client->dev.of_node;
+	pdata->bridge.of_node = dev->of_node;
 
 	drm_bridge_add(&pdata->bridge);
 
-- 
2.31.1.498.g6c1eba8ee3d-goog


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v5 07/20] drm/bridge: ti-sn65dsi86: Cleanup managing of drvdata
  2021-04-23 16:58 [PATCH v5 00/20] drm: Fix EDID reading on ti-sn65dsi86; solve some chicken-and-egg problems Douglas Anderson
                   ` (5 preceding siblings ...)
  2021-04-23 16:58 ` [PATCH v5 06/20] drm/bridge: ti-sn65dsi86: Add local var for "dev" to simplify probe Douglas Anderson
@ 2021-04-23 16:58 ` Douglas Anderson
  2021-04-23 16:58 ` [PATCH v5 08/20] drm/bridge: ti-sn65dsi86: Move all the chip-related init to the start Douglas Anderson
                   ` (12 subsequent siblings)
  19 siblings, 0 replies; 36+ messages in thread
From: Douglas Anderson @ 2021-04-23 16:58 UTC (permalink / raw)
  To: Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, Sam Ravnborg, Wolfram Sang
  Cc: linux-arm-msm, robdclark, Stanislav Lisovskiy, Stephen Boyd,
	Steev Klimaszewski, Linus W, Maarten Lankhorst, linux-i2c,
	Bjorn Andersson, dri-devel, Douglas Anderson, Daniel Vetter,
	David Airlie, Robert Foss, linux-kernel

Let's:
- Set the drvdata as soon as it's allocated. This just sets up a
  pointer so there's no downside here.
- Remove the useless call to i2c_set_clientdata() which is literally
  the same thing as dev_set_drvdata().

No functional changes intended.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

(no changes since v1)

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index c868193f5b8f..75a41198993f 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -1244,6 +1244,8 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
 	pdata = devm_kzalloc(dev, sizeof(struct ti_sn65dsi86), GFP_KERNEL);
 	if (!pdata)
 		return -ENOMEM;
+	dev_set_drvdata(dev, pdata);
+	pdata->dev = dev;
 
 	pdata->regmap = devm_regmap_init_i2c(client,
 					     &ti_sn65dsi86_regmap_config);
@@ -1252,16 +1254,12 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
 		return PTR_ERR(pdata->regmap);
 	}
 
-	pdata->dev = dev;
-
 	ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &pdata->panel, NULL);
 	if (ret) {
 		DRM_ERROR("could not find any panel node\n");
 		return ret;
 	}
 
-	dev_set_drvdata(dev, pdata);
-
 	pdata->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
 	if (IS_ERR(pdata->enable_gpio)) {
 		DRM_ERROR("failed to get enable gpio from DT\n");
@@ -1294,8 +1292,6 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
 	if (ret)
 		return ret;
 
-	i2c_set_clientdata(client, pdata);
-
 	pdata->aux.name = "ti-sn65dsi86-aux";
 	pdata->aux.dev = dev;
 	pdata->aux.transfer = ti_sn_aux_transfer;
-- 
2.31.1.498.g6c1eba8ee3d-goog


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v5 08/20] drm/bridge: ti-sn65dsi86: Move all the chip-related init to the start
  2021-04-23 16:58 [PATCH v5 00/20] drm: Fix EDID reading on ti-sn65dsi86; solve some chicken-and-egg problems Douglas Anderson
                   ` (6 preceding siblings ...)
  2021-04-23 16:58 ` [PATCH v5 07/20] drm/bridge: ti-sn65dsi86: Cleanup managing of drvdata Douglas Anderson
@ 2021-04-23 16:58 ` Douglas Anderson
  2021-04-23 16:58 ` [PATCH v5 09/20] drm/bridge: ti-sn65dsi86: Break GPIO and MIPI-to-eDP bridge into sub-drivers Douglas Anderson
                   ` (11 subsequent siblings)
  19 siblings, 0 replies; 36+ messages in thread
From: Douglas Anderson @ 2021-04-23 16:58 UTC (permalink / raw)
  To: Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, Sam Ravnborg, Wolfram Sang
  Cc: linux-arm-msm, robdclark, Stanislav Lisovskiy, Stephen Boyd,
	Steev Klimaszewski, Linus W, Maarten Lankhorst, linux-i2c,
	Bjorn Andersson, dri-devel, Douglas Anderson, Daniel Vetter,
	David Airlie, Robert Foss, linux-kernel

This is just code motion of the probe routine to move all the things
that are for the "whole chip" (instead of the GPIO parts or the
MIPI-to-eDP parts) together at the start of probe. This is in
preparation for breaking the driver into sub-drivers.

Since we're using devm for all of the "whole chip" stuff this is
actually quite easy now.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

(no changes since v1)

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

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 75a41198993f..68673f736b23 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -1254,12 +1254,6 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
 		return PTR_ERR(pdata->regmap);
 	}
 
-	ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &pdata->panel, NULL);
-	if (ret) {
-		DRM_ERROR("could not find any panel node\n");
-		return ret;
-	}
-
 	pdata->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
 	if (IS_ERR(pdata->enable_gpio)) {
 		DRM_ERROR("failed to get enable gpio from DT\n");
@@ -1267,8 +1261,6 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
 		return ret;
 	}
 
-	ti_sn_bridge_parse_lanes(pdata, dev->of_node);
-
 	ret = ti_sn65dsi86_parse_regulators(pdata);
 	if (ret) {
 		DRM_ERROR("failed to parse regulators\n");
@@ -1279,12 +1271,22 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
 	if (IS_ERR(pdata->refclk))
 		return PTR_ERR(pdata->refclk);
 
-	ret = ti_sn_bridge_parse_dsi_host(pdata);
+	pm_runtime_enable(dev);
+	ret = devm_add_action_or_reset(dev, ti_sn65dsi86_runtime_disable, dev);
 	if (ret)
 		return ret;
 
-	pm_runtime_enable(dev);
-	ret = devm_add_action_or_reset(dev, ti_sn65dsi86_runtime_disable, dev);
+	ti_sn65dsi86_debugfs_init(pdata);
+
+	ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &pdata->panel, NULL);
+	if (ret) {
+		DRM_ERROR("could not find any panel node\n");
+		return ret;
+	}
+
+	ti_sn_bridge_parse_lanes(pdata, dev->of_node);
+
+	ret = ti_sn_bridge_parse_dsi_host(pdata);
 	if (ret)
 		return ret;
 
@@ -1302,8 +1304,6 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
 
 	drm_bridge_add(&pdata->bridge);
 
-	ti_sn65dsi86_debugfs_init(pdata);
-
 	return 0;
 }
 
-- 
2.31.1.498.g6c1eba8ee3d-goog


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v5 09/20] drm/bridge: ti-sn65dsi86: Break GPIO and MIPI-to-eDP bridge into sub-drivers
  2021-04-23 16:58 [PATCH v5 00/20] drm: Fix EDID reading on ti-sn65dsi86; solve some chicken-and-egg problems Douglas Anderson
                   ` (7 preceding siblings ...)
  2021-04-23 16:58 ` [PATCH v5 08/20] drm/bridge: ti-sn65dsi86: Move all the chip-related init to the start Douglas Anderson
@ 2021-04-23 16:58 ` Douglas Anderson
  2021-05-01 11:59   ` Linus Walleij
  2021-04-23 16:58 ` [PATCH v5 10/20] drm/panel: panel-simple: Get rid of hacky HPD chicken-and-egg code Douglas Anderson
                   ` (10 subsequent siblings)
  19 siblings, 1 reply; 36+ messages in thread
From: Douglas Anderson @ 2021-04-23 16:58 UTC (permalink / raw)
  To: Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, Sam Ravnborg, Wolfram Sang
  Cc: linux-arm-msm, robdclark, Stanislav Lisovskiy, Stephen Boyd,
	Steev Klimaszewski, Linus W, Maarten Lankhorst, linux-i2c,
	Bjorn Andersson, dri-devel, Douglas Anderson, Daniel Vetter,
	David Airlie, Robert Foss, linux-kernel

Let's use the newly minted aux bus to break up the driver into sub
drivers. We're not doing a full breakup here: all the code is still in
the same file and remains largely untouched. The big goal here of
using sub-drivers is to allow part of our code to finish probing even
if some other code needs to defer. This can solve some chicken-and-egg
problems. Specifically:
- In commit 48834e6084f1 ("drm/panel-simple: Support hpd-gpios for
  delaying prepare()") we had to add a bit of a hack to simpel-panel
  to support HPD showing up late. We can get rid of that hack now
  since the GPIO part of our driver can finish probing early.
- We have a desire to expose our DDC bus to simple-panel (and perhaps
  to a backlight driver?). That will end up with the same
  chicken-and-egg problem. A future patch to move this to a sub-driver
  will fix it.
- If/when we support the PWM functionality present in the bridge chip
  for a backlight we'll end up with another chicken-and-egg
  problem. If we allow the PWM to be a sub-driver too then it solves
  this problem.

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

Changes in v5:
- Fix module compile problems (Bjorn + kbuild bot)
- Remove useless MODULE_DEVICE_TABLE (Bjorn).

 drivers/gpu/drm/bridge/Kconfig        |   1 +
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 252 ++++++++++++++++++++------
 2 files changed, 200 insertions(+), 53 deletions(-)

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index d907a91a2ee8..bdec664f27ec 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -275,6 +275,7 @@ config DRM_TI_SN65DSI86
 	select REGMAP_I2C
 	select DRM_PANEL
 	select DRM_MIPI_DSI
+	select AUXILIARY_BUS
 	help
 	  Texas Instruments SN65DSI86 DSI to eDP Bridge driver
 
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 68673f736b23..0bd1a1d1453e 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -4,6 +4,7 @@
  * datasheet: https://www.ti.com/lit/ds/symlink/sn65dsi86.pdf
  */
 
+#include <linux/auxiliary_bus.h>
 #include <linux/bits.h>
 #include <linux/clk.h>
 #include <linux/debugfs.h>
@@ -113,7 +114,10 @@
 
 /**
  * struct ti_sn65dsi86 - Platform data for ti-sn65dsi86 driver.
- * @dev:          Pointer to our device.
+ * @bridge_aux:   AUX-bus sub device for MIPI-to-eDP bridge functionality.
+ * @gpio_aux:     AUX-bus sub device for GPIO controller functionality.
+ *
+ * @dev:          Pointer to the top level (i2c) device.
  * @regmap:       Regmap for accessing i2c.
  * @aux:          Our aux channel.
  * @bridge:       Our bridge.
@@ -140,6 +144,9 @@
  *                each other's read-modify-write.
  */
 struct ti_sn65dsi86 {
+	struct auxiliary_device		bridge_aux;
+	struct auxiliary_device		gpio_aux;
+
 	struct device			*dev;
 	struct regmap			*regmap;
 	struct drm_dp_aux		aux;
@@ -1136,8 +1143,10 @@ static const char * const ti_sn_bridge_gpio_names[SN_NUM_GPIOS] = {
 	"GPIO1", "GPIO2", "GPIO3", "GPIO4"
 };
 
-static int ti_sn_setup_gpio_controller(struct ti_sn65dsi86 *pdata)
+static int ti_sn_gpio_probe(struct auxiliary_device *adev,
+			    const struct auxiliary_device_id *id)
 {
+	struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent);
 	int ret;
 
 	/* Only init if someone is going to use us as a GPIO controller */
@@ -1159,20 +1168,41 @@ static int ti_sn_setup_gpio_controller(struct ti_sn65dsi86 *pdata)
 	pdata->gchip.names = ti_sn_bridge_gpio_names;
 	pdata->gchip.ngpio = SN_NUM_GPIOS;
 	pdata->gchip.base = -1;
-	ret = devm_gpiochip_add_data(pdata->dev, &pdata->gchip, pdata);
+	ret = devm_gpiochip_add_data(&adev->dev, &pdata->gchip, pdata);
 	if (ret)
 		dev_err(pdata->dev, "can't add gpio chip\n");
 
 	return ret;
 }
 
-#else
+static const struct auxiliary_device_id ti_sn_gpio_id_table[] = {
+	{ .name = "ti_sn65dsi86.gpio", },
+	{},
+};
 
-static inline int ti_sn_setup_gpio_controller(struct ti_sn65dsi86 *pdata)
+MODULE_DEVICE_TABLE(auxiliary, ti_sn_gpio_id_table);
+
+static struct auxiliary_driver ti_sn_gpio_driver = {
+	.name = "gpio",
+	.probe = ti_sn_gpio_probe,
+	.id_table = ti_sn_gpio_id_table,
+};
+
+static int __init ti_sn_gpio_register(void)
 {
-	return 0;
+	return auxiliary_driver_register(&ti_sn_gpio_driver);
 }
 
+static void __exit ti_sn_gpio_unregister(void)
+{
+	auxiliary_driver_unregister(&ti_sn_gpio_driver);
+}
+
+#else
+
+static inline int ti_sn_gpio_register(void) { return 0; }
+static inline void ti_sn_gpio_unregister(void) {}
+
 #endif
 
 static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,
@@ -1224,11 +1254,124 @@ static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,
 	pdata->ln_polrs = ln_polrs;
 }
 
+static int ti_sn_bridge_probe(struct auxiliary_device *adev,
+			      const struct auxiliary_device_id *id)
+{
+	struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent);
+	struct device_node *np = pdata->dev->of_node;
+	int ret;
+
+	ret = drm_of_find_panel_or_bridge(np, 1, 0, &pdata->panel, NULL);
+	if (ret) {
+		DRM_ERROR("could not find any panel node\n");
+		return ret;
+	}
+
+	ti_sn_bridge_parse_lanes(pdata, np);
+
+	ret = ti_sn_bridge_parse_dsi_host(pdata);
+	if (ret)
+		return ret;
+
+	pdata->aux.name = "ti-sn65dsi86-aux";
+	pdata->aux.dev = pdata->dev;
+	pdata->aux.transfer = ti_sn_aux_transfer;
+	drm_dp_aux_init(&pdata->aux);
+
+	pdata->bridge.funcs = &ti_sn_bridge_funcs;
+	pdata->bridge.of_node = np;
+
+	drm_bridge_add(&pdata->bridge);
+
+	return 0;
+}
+
+static void ti_sn_bridge_remove(struct auxiliary_device *adev)
+{
+	struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent);
+
+	if (!pdata)
+		return;
+
+	if (pdata->dsi) {
+		mipi_dsi_detach(pdata->dsi);
+		mipi_dsi_device_unregister(pdata->dsi);
+	}
+
+	kfree(pdata->edid);
+
+	drm_bridge_remove(&pdata->bridge);
+
+	of_node_put(pdata->host_node);
+}
+
+static const struct auxiliary_device_id ti_sn_bridge_id_table[] = {
+	{ .name = "ti_sn65dsi86.bridge", },
+	{},
+};
+
+static struct auxiliary_driver ti_sn_bridge_driver = {
+	.name = "bridge",
+	.probe = ti_sn_bridge_probe,
+	.remove = ti_sn_bridge_remove,
+	.id_table = ti_sn_bridge_id_table,
+};
+
 static void ti_sn65dsi86_runtime_disable(void *data)
 {
 	pm_runtime_disable(data);
 }
 
+static void ti_sn65dsi86_uninit_aux(void *data)
+{
+	auxiliary_device_uninit(data);
+}
+
+static void ti_sn65dsi86_delete_aux(void *data)
+{
+	auxiliary_device_delete(data);
+}
+
+/*
+ * AUX bus docs say that a non-NULL release is mandatory, but it makes no
+ * sense for the model used here where all of the aux devices are allocated
+ * in the single shared structure. We'll use this noop as a workaround.
+ */
+static void ti_sn65dsi86_noop(struct device *dev) {}
+
+static int ti_sn65dsi86_add_aux_device(struct ti_sn65dsi86 *pdata,
+				       struct auxiliary_device *aux,
+				       const char *name)
+{
+	struct device *dev = pdata->dev;
+	int ret;
+
+	/*
+	 * NOTE: It would be nice to set the "of_node" of our children to be
+	 * the same "of_node"" that the top-level component has. That doesn't
+	 * work, though, since pinctrl will try (and fail) to reserve the
+	 * pins again. Until that gets sorted out the children will just need
+	 * to look at the of_node of the main device.
+	 */
+
+	aux->name = name;
+	aux->dev.parent = dev;
+	aux->dev.release = ti_sn65dsi86_noop;
+	ret = auxiliary_device_init(aux);
+	if (ret)
+		return ret;
+	ret = devm_add_action_or_reset(dev, ti_sn65dsi86_uninit_aux, aux);
+	if (ret)
+		return ret;
+
+	ret = auxiliary_device_add(aux);
+	if (ret)
+		return ret;
+	ret = devm_add_action_or_reset(dev, ti_sn65dsi86_delete_aux, aux);
+
+	return ret;
+}
+
 static int ti_sn65dsi86_probe(struct i2c_client *client,
 			      const struct i2c_device_id *id)
 {
@@ -1278,54 +1421,24 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
 
 	ti_sn65dsi86_debugfs_init(pdata);
 
-	ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &pdata->panel, NULL);
-	if (ret) {
-		DRM_ERROR("could not find any panel node\n");
-		return ret;
-	}
-
-	ti_sn_bridge_parse_lanes(pdata, dev->of_node);
-
-	ret = ti_sn_bridge_parse_dsi_host(pdata);
-	if (ret)
-		return ret;
-
-	ret = ti_sn_setup_gpio_controller(pdata);
-	if (ret)
-		return ret;
-
-	pdata->aux.name = "ti-sn65dsi86-aux";
-	pdata->aux.dev = dev;
-	pdata->aux.transfer = ti_sn_aux_transfer;
-	drm_dp_aux_init(&pdata->aux);
-
-	pdata->bridge.funcs = &ti_sn_bridge_funcs;
-	pdata->bridge.of_node = dev->of_node;
-
-	drm_bridge_add(&pdata->bridge);
-
-	return 0;
-}
-
-static int ti_sn65dsi86_remove(struct i2c_client *client)
-{
-	struct ti_sn65dsi86 *pdata = i2c_get_clientdata(client);
-
-	if (!pdata)
-		return -EINVAL;
+	/*
+	 * Break ourselves up into a collection of aux devices. The only real
+	 * motiviation here is to solve the chicken-and-egg problem of probe
+	 * ordering. The bridge wants the panel to be there when it probes.
+	 * The panel wants its HPD GPIO (provided by sn65dsi86 on some boards)
+	 * when it probes. There will soon be other devices (DDC I2C bus, PWM)
+	 * that have the same problem. Having sub-devices allows the some sub
+	 * devices to finish probing even if others return -EPROBE_DEFER and
+	 * gets us around the problems.
+	 */
 
-	if (pdata->dsi) {
-		mipi_dsi_detach(pdata->dsi);
-		mipi_dsi_device_unregister(pdata->dsi);
+	if (IS_ENABLED(CONFIG_OF_GPIO)) {
+		ret = ti_sn65dsi86_add_aux_device(pdata, &pdata->gpio_aux, "gpio");
+		if (ret)
+			return ret;
 	}
 
-	kfree(pdata->edid);
-
-	drm_bridge_remove(&pdata->bridge);
-
-	of_node_put(pdata->host_node);
-
-	return 0;
+	return ti_sn65dsi86_add_aux_device(pdata, &pdata->bridge_aux, "bridge");
 }
 
 static struct i2c_device_id ti_sn65dsi86_id[] = {
@@ -1347,10 +1460,43 @@ static struct i2c_driver ti_sn65dsi86_driver = {
 		.pm = &ti_sn65dsi86_pm_ops,
 	},
 	.probe = ti_sn65dsi86_probe,
-	.remove = ti_sn65dsi86_remove,
 	.id_table = ti_sn65dsi86_id,
 };
-module_i2c_driver(ti_sn65dsi86_driver);
+
+static int __init ti_sn65dsi86_init(void)
+{
+	int ret;
+
+	ret = i2c_add_driver(&ti_sn65dsi86_driver);
+	if (ret)
+		return ret;
+
+	ret = ti_sn_gpio_register();
+	if (ret)
+		goto err_main_was_registered;
+
+	ret = auxiliary_driver_register(&ti_sn_bridge_driver);
+	if (ret)
+		goto err_gpio_was_registered;
+
+	return 0;
+
+err_gpio_was_registered:
+	ti_sn_gpio_unregister();
+err_main_was_registered:
+	i2c_del_driver(&ti_sn65dsi86_driver);
+
+	return ret;
+}
+module_init(ti_sn65dsi86_init);
+
+static void __exit ti_sn65dsi86_exit(void)
+{
+	auxiliary_driver_unregister(&ti_sn_bridge_driver);
+	ti_sn_gpio_unregister();
+	i2c_del_driver(&ti_sn65dsi86_driver);
+}
+module_exit(ti_sn65dsi86_exit);
 
 MODULE_AUTHOR("Sandeep Panda <spanda@codeaurora.org>");
 MODULE_DESCRIPTION("sn65dsi86 DSI to eDP bridge driver");
-- 
2.31.1.498.g6c1eba8ee3d-goog


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v5 10/20] drm/panel: panel-simple: Get rid of hacky HPD chicken-and-egg code
  2021-04-23 16:58 [PATCH v5 00/20] drm: Fix EDID reading on ti-sn65dsi86; solve some chicken-and-egg problems Douglas Anderson
                   ` (8 preceding siblings ...)
  2021-04-23 16:58 ` [PATCH v5 09/20] drm/bridge: ti-sn65dsi86: Break GPIO and MIPI-to-eDP bridge into sub-drivers Douglas Anderson
@ 2021-04-23 16:58 ` Douglas Anderson
  2021-04-28 16:35   ` Sean Paul
  2021-04-23 16:58 ` [PATCH v5 11/20] drm/bridge: ti-sn65dsi86: Use pm_runtime autosuspend Douglas Anderson
                   ` (9 subsequent siblings)
  19 siblings, 1 reply; 36+ messages in thread
From: Douglas Anderson @ 2021-04-23 16:58 UTC (permalink / raw)
  To: Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, Sam Ravnborg, Wolfram Sang
  Cc: linux-arm-msm, robdclark, Stanislav Lisovskiy, Stephen Boyd,
	Steev Klimaszewski, Linus W, Maarten Lankhorst, linux-i2c,
	Bjorn Andersson, dri-devel, Douglas Anderson, Daniel Vetter,
	David Airlie, Thierry Reding, linux-kernel

When I added support for the hpd-gpio to simple-panel in commit
48834e6084f1 ("drm/panel-simple: Support hpd-gpios for delaying
prepare()"), I added a special case to handle a circular dependency I
was running into on the ti-sn65dsi86 bridge chip. On my board the
hpd-gpio is actually provided by the bridge chip. That was causing
some circular dependency problems that I had to work around by getting
the hpd-gpio late.

I've now reorganized the ti-sn65dsi86 bridge chip driver to be a
collection of sub-drivers. Now the GPIO part can probe separately and
that breaks the chain. Let's get rid of the old code to clean things
up.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

(no changes since v1)

 drivers/gpu/drm/panel/panel-simple.c | 24 +++++-------------------
 1 file changed, 5 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 9746eda6f675..bd208abcbf07 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -366,8 +366,7 @@ static int panel_simple_unprepare(struct drm_panel *panel)
 	return 0;
 }
 
-static int panel_simple_get_hpd_gpio(struct device *dev,
-				     struct panel_simple *p, bool from_probe)
+static int panel_simple_get_hpd_gpio(struct device *dev, struct panel_simple *p)
 {
 	int err;
 
@@ -375,17 +374,10 @@ static int panel_simple_get_hpd_gpio(struct device *dev,
 	if (IS_ERR(p->hpd_gpio)) {
 		err = PTR_ERR(p->hpd_gpio);
 
-		/*
-		 * If we're called from probe we won't consider '-EPROBE_DEFER'
-		 * to be an error--we'll leave the error code in "hpd_gpio".
-		 * When we try to use it we'll try again.  This allows for
-		 * circular dependencies where the component providing the
-		 * hpd gpio needs the panel to init before probing.
-		 */
-		if (err != -EPROBE_DEFER || !from_probe) {
+		if (err != -EPROBE_DEFER)
 			dev_err(dev, "failed to get 'hpd' GPIO: %d\n", err);
-			return err;
-		}
+
+		return err;
 	}
 
 	return 0;
@@ -416,12 +408,6 @@ static int panel_simple_prepare_once(struct panel_simple *p)
 		msleep(delay);
 
 	if (p->hpd_gpio) {
-		if (IS_ERR(p->hpd_gpio)) {
-			err = panel_simple_get_hpd_gpio(dev, p, false);
-			if (err)
-				goto error;
-		}
-
 		if (p->desc->delay.hpd_absent_delay)
 			hpd_wait_us = p->desc->delay.hpd_absent_delay * 1000UL;
 		else
@@ -682,7 +668,7 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
 
 	panel->no_hpd = of_property_read_bool(dev->of_node, "no-hpd");
 	if (!panel->no_hpd) {
-		err = panel_simple_get_hpd_gpio(dev, panel, true);
+		err = panel_simple_get_hpd_gpio(dev, panel);
 		if (err)
 			return err;
 	}
-- 
2.31.1.498.g6c1eba8ee3d-goog


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v5 11/20] drm/bridge: ti-sn65dsi86: Use pm_runtime autosuspend
  2021-04-23 16:58 [PATCH v5 00/20] drm: Fix EDID reading on ti-sn65dsi86; solve some chicken-and-egg problems Douglas Anderson
                   ` (9 preceding siblings ...)
  2021-04-23 16:58 ` [PATCH v5 10/20] drm/panel: panel-simple: Get rid of hacky HPD chicken-and-egg code Douglas Anderson
@ 2021-04-23 16:58 ` Douglas Anderson
  2021-04-23 16:58 ` [PATCH v5 12/20] drm/bridge: ti-sn65dsi86: Code motion of refclk management functions Douglas Anderson
                   ` (8 subsequent siblings)
  19 siblings, 0 replies; 36+ messages in thread
From: Douglas Anderson @ 2021-04-23 16:58 UTC (permalink / raw)
  To: Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, Sam Ravnborg, Wolfram Sang
  Cc: linux-arm-msm, robdclark, Stanislav Lisovskiy, Stephen Boyd,
	Steev Klimaszewski, Linus W, Maarten Lankhorst, linux-i2c,
	Bjorn Andersson, dri-devel, Douglas Anderson, Daniel Vetter,
	David Airlie, Robert Foss, linux-kernel

Let's make the bridge use autosuspend with a 500ms delay. This is in
preparation for promoting DP AUX transfers to their own sub-driver so
that we're not constantly powering up and down the device as we
transfer all the chunks.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

(no changes since v1)

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 0bd1a1d1453e..49b76b2ffe25 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -243,7 +243,7 @@ static int status_show(struct seq_file *s, void *data)
 		seq_printf(s, "[0x%02x] = 0x%08x\n", reg, val);
 	}
 
-	pm_runtime_put(pdata->dev);
+	pm_runtime_put_autosuspend(pdata->dev);
 
 	return 0;
 }
@@ -293,7 +293,7 @@ static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
 	if (!edid) {
 		pm_runtime_get_sync(pdata->dev);
 		edid = pdata->edid = drm_get_edid(connector, &pdata->aux.ddc);
-		pm_runtime_put(pdata->dev);
+		pm_runtime_put_autosuspend(pdata->dev);
 	}
 
 	if (edid && drm_edid_is_valid(edid)) {
@@ -419,7 +419,7 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
 	/* check if continuous dsi clock is required or not */
 	pm_runtime_get_sync(pdata->dev);
 	regmap_read(pdata->regmap, SN_DPPLL_SRC_REG, &val);
-	pm_runtime_put(pdata->dev);
+	pm_runtime_put_autosuspend(pdata->dev);
 	if (!(val & DPPLL_CLK_SRC_DSICLK))
 		dsi->mode_flags |= MIPI_DSI_CLOCK_NON_CONTINUOUS;
 
@@ -1050,7 +1050,7 @@ static int ti_sn_bridge_gpio_get(struct gpio_chip *chip, unsigned int offset)
 	 */
 	pm_runtime_get_sync(pdata->dev);
 	ret = regmap_read(pdata->regmap, SN_GPIO_IO_REG, &val);
-	pm_runtime_put(pdata->dev);
+	pm_runtime_put_autosuspend(pdata->dev);
 
 	if (ret)
 		return ret;
@@ -1101,7 +1101,7 @@ static int ti_sn_bridge_gpio_direction_input(struct gpio_chip *chip,
 	 * it off and when it comes back it will have lost all state, but
 	 * that's OK because the default is input and we're now an input.
 	 */
-	pm_runtime_put(pdata->dev);
+	pm_runtime_put_autosuspend(pdata->dev);
 
 	return 0;
 }
@@ -1127,7 +1127,7 @@ static int ti_sn_bridge_gpio_direction_output(struct gpio_chip *chip,
 				 SN_GPIO_MUX_OUTPUT << shift);
 	if (ret) {
 		clear_bit(offset, pdata->gchip_output);
-		pm_runtime_put(pdata->dev);
+		pm_runtime_put_autosuspend(pdata->dev);
 	}
 
 	return ret;
@@ -1418,6 +1418,8 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
 	ret = devm_add_action_or_reset(dev, ti_sn65dsi86_runtime_disable, dev);
 	if (ret)
 		return ret;
+	pm_runtime_set_autosuspend_delay(pdata->dev, 500);
+	pm_runtime_use_autosuspend(pdata->dev);
 
 	ti_sn65dsi86_debugfs_init(pdata);
 
-- 
2.31.1.498.g6c1eba8ee3d-goog


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v5 12/20] drm/bridge: ti-sn65dsi86: Code motion of refclk management functions
  2021-04-23 16:58 [PATCH v5 00/20] drm: Fix EDID reading on ti-sn65dsi86; solve some chicken-and-egg problems Douglas Anderson
                   ` (10 preceding siblings ...)
  2021-04-23 16:58 ` [PATCH v5 11/20] drm/bridge: ti-sn65dsi86: Use pm_runtime autosuspend Douglas Anderson
@ 2021-04-23 16:58 ` Douglas Anderson
  2021-04-23 16:58 ` [PATCH v5 13/20] drm/bridge: ti-sn65dsi86: If refclk, DP AUX can happen w/out pre-enable Douglas Anderson
                   ` (7 subsequent siblings)
  19 siblings, 0 replies; 36+ messages in thread
From: Douglas Anderson @ 2021-04-23 16:58 UTC (permalink / raw)
  To: Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, Sam Ravnborg, Wolfram Sang
  Cc: linux-arm-msm, robdclark, Stanislav Lisovskiy, Stephen Boyd,
	Steev Klimaszewski, Linus W, Maarten Lankhorst, linux-i2c,
	Bjorn Andersson, dri-devel, Douglas Anderson, Daniel Vetter,
	David Airlie, Robert Foss, linux-kernel

No functional changes--this just makes the diffstat of a future change
easier to understand.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

(no changes since v1)

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

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 49b76b2ffe25..db367793cdff 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -192,6 +192,64 @@ static void ti_sn65dsi86_write_u16(struct ti_sn65dsi86 *pdata,
 	regmap_write(pdata->regmap, reg + 1, val >> 8);
 }
 
+static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn65dsi86 *pdata)
+{
+	u32 bit_rate_khz, clk_freq_khz;
+	struct drm_display_mode *mode =
+		&pdata->bridge.encoder->crtc->state->adjusted_mode;
+
+	bit_rate_khz = mode->clock *
+			mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
+	clk_freq_khz = bit_rate_khz / (pdata->dsi->lanes * 2);
+
+	return clk_freq_khz;
+}
+
+/* clk frequencies supported by bridge in Hz in case derived from REFCLK pin */
+static const u32 ti_sn_bridge_refclk_lut[] = {
+	12000000,
+	19200000,
+	26000000,
+	27000000,
+	38400000,
+};
+
+/* clk frequencies supported by bridge in Hz in case derived from DACP/N pin */
+static const u32 ti_sn_bridge_dsiclk_lut[] = {
+	468000000,
+	384000000,
+	416000000,
+	486000000,
+	460800000,
+};
+
+static void ti_sn_bridge_set_refclk_freq(struct ti_sn65dsi86 *pdata)
+{
+	int i;
+	u32 refclk_rate;
+	const u32 *refclk_lut;
+	size_t refclk_lut_size;
+
+	if (pdata->refclk) {
+		refclk_rate = clk_get_rate(pdata->refclk);
+		refclk_lut = ti_sn_bridge_refclk_lut;
+		refclk_lut_size = ARRAY_SIZE(ti_sn_bridge_refclk_lut);
+		clk_prepare_enable(pdata->refclk);
+	} else {
+		refclk_rate = ti_sn_bridge_get_dsi_freq(pdata) * 1000;
+		refclk_lut = ti_sn_bridge_dsiclk_lut;
+		refclk_lut_size = ARRAY_SIZE(ti_sn_bridge_dsiclk_lut);
+	}
+
+	/* for i equals to refclk_lut_size means default frequency */
+	for (i = 0; i < refclk_lut_size; i++)
+		if (refclk_lut[i] == refclk_rate)
+			break;
+
+	regmap_update_bits(pdata->regmap, SN_DPPLL_SRC_REG, REFCLK_FREQ_MASK,
+			   REFCLK_FREQ(i));
+}
+
 static int __maybe_unused ti_sn65dsi86_resume(struct device *dev)
 {
 	struct ti_sn65dsi86 *pdata = dev_get_drvdata(dev);
@@ -460,64 +518,6 @@ static void ti_sn_bridge_disable(struct drm_bridge *bridge)
 	regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 0);
 }
 
-static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn65dsi86 *pdata)
-{
-	u32 bit_rate_khz, clk_freq_khz;
-	struct drm_display_mode *mode =
-		&pdata->bridge.encoder->crtc->state->adjusted_mode;
-
-	bit_rate_khz = mode->clock *
-			mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
-	clk_freq_khz = bit_rate_khz / (pdata->dsi->lanes * 2);
-
-	return clk_freq_khz;
-}
-
-/* clk frequencies supported by bridge in Hz in case derived from REFCLK pin */
-static const u32 ti_sn_bridge_refclk_lut[] = {
-	12000000,
-	19200000,
-	26000000,
-	27000000,
-	38400000,
-};
-
-/* clk frequencies supported by bridge in Hz in case derived from DACP/N pin */
-static const u32 ti_sn_bridge_dsiclk_lut[] = {
-	468000000,
-	384000000,
-	416000000,
-	486000000,
-	460800000,
-};
-
-static void ti_sn_bridge_set_refclk_freq(struct ti_sn65dsi86 *pdata)
-{
-	int i;
-	u32 refclk_rate;
-	const u32 *refclk_lut;
-	size_t refclk_lut_size;
-
-	if (pdata->refclk) {
-		refclk_rate = clk_get_rate(pdata->refclk);
-		refclk_lut = ti_sn_bridge_refclk_lut;
-		refclk_lut_size = ARRAY_SIZE(ti_sn_bridge_refclk_lut);
-		clk_prepare_enable(pdata->refclk);
-	} else {
-		refclk_rate = ti_sn_bridge_get_dsi_freq(pdata) * 1000;
-		refclk_lut = ti_sn_bridge_dsiclk_lut;
-		refclk_lut_size = ARRAY_SIZE(ti_sn_bridge_dsiclk_lut);
-	}
-
-	/* for i equals to refclk_lut_size means default frequency */
-	for (i = 0; i < refclk_lut_size; i++)
-		if (refclk_lut[i] == refclk_rate)
-			break;
-
-	regmap_update_bits(pdata->regmap, SN_DPPLL_SRC_REG, REFCLK_FREQ_MASK,
-			   REFCLK_FREQ(i));
-}
-
 static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)
 {
 	unsigned int bit_rate_mhz, clk_freq_mhz;
-- 
2.31.1.498.g6c1eba8ee3d-goog


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v5 13/20] drm/bridge: ti-sn65dsi86: If refclk, DP AUX can happen w/out pre-enable
  2021-04-23 16:58 [PATCH v5 00/20] drm: Fix EDID reading on ti-sn65dsi86; solve some chicken-and-egg problems Douglas Anderson
                   ` (11 preceding siblings ...)
  2021-04-23 16:58 ` [PATCH v5 12/20] drm/bridge: ti-sn65dsi86: Code motion of refclk management functions Douglas Anderson
@ 2021-04-23 16:58 ` Douglas Anderson
  2021-04-23 16:59 ` [PATCH v5 14/20] drm/bridge: ti-sn65dsi86: Promote the AUX channel to its own sub-dev Douglas Anderson
                   ` (6 subsequent siblings)
  19 siblings, 0 replies; 36+ messages in thread
From: Douglas Anderson @ 2021-04-23 16:58 UTC (permalink / raw)
  To: Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, Sam Ravnborg, Wolfram Sang
  Cc: linux-arm-msm, robdclark, Stanislav Lisovskiy, Stephen Boyd,
	Steev Klimaszewski, Linus W, Maarten Lankhorst, linux-i2c,
	Bjorn Andersson, dri-devel, Douglas Anderson, Daniel Vetter,
	David Airlie, Robert Foss, linux-kernel

Let's reorganize how we init and turn on the reference clock in the
code to allow us to turn it on early (even before pre_enable()) so
that we can read the EDID early. This is handy for eDP because:
- We always assume that a panel is there.
- Once we report that a panel is there we get asked to read the EDID.
- Pre-enable isn't called until we know what pixel clock we want to
  use and we're ready to turn everything on. That's _after_ we get
  asked to read the EDID.

NOTE: the above only works out OK if we "refclk" is provided. Though I
don't have access to any hardware that uses ti-sn65dsi86 and _doesn't_
provide a "refclk", I believe that we'll have trouble reading the EDID
at bootup in that case. Specifically I believe that if there's no
"refclk" we need the MIPI source clock to be active before we can
successfully read the EDID. My evidence here is that, in testing, I
couldn't read the EDID until I turned on the DPPLL in the bridge chip
and that the DPPLL needs the input clock to be active.

Since this is hard to support, let's punt trying to handle this case
if there's no "refclk". In that case we'll enable comms in
pre_enable() like we always did.

I don't believe there are any users of the ti-sn65dsi86 bridge chip
that _don't_ use "refclk". The bridge chip is _very_ inflexible in
that mode. The only time I've seen that mode used was for some really
early prototype hardware that was thrown in the e-waste bin years ago
when we realized how inflexible it was.

Even if someone is using the bridge chip without the "refclk" they're
in no worse shape than they were before the (fairly recent) commit
58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over DDC").

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

(no changes since v1)

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 129 +++++++++++++++++++-------
 1 file changed, 94 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index db367793cdff..9dc3cd8e17df 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -132,6 +132,8 @@
  * @dp_lanes:     Count of dp_lanes we're using.
  * @ln_assign:    Value to program to the LN_ASSIGN register.
  * @ln_polrs:     Value for the 4-bit LN_POLRS field of SN_ENH_FRAME_REG.
+ * @comms_enabled: If true then communication over the aux channel is enabled.
+ * @comms_mutex:   Protects modification of comms_enabled.
  *
  * @gchip:        If we expose our GPIOs, this is used.
  * @gchip_output: A cache of whether we've set GPIOs to output.  This
@@ -162,6 +164,8 @@ struct ti_sn65dsi86 {
 	int				dp_lanes;
 	u8				ln_assign;
 	u8				ln_polrs;
+	bool				comms_enabled;
+	struct mutex			comms_mutex;
 
 #if defined(CONFIG_OF_GPIO)
 	struct gpio_chip		gchip;
@@ -250,6 +254,47 @@ static void ti_sn_bridge_set_refclk_freq(struct ti_sn65dsi86 *pdata)
 			   REFCLK_FREQ(i));
 }
 
+static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata)
+{
+	mutex_lock(&pdata->comms_mutex);
+
+	/* configure bridge ref_clk */
+	ti_sn_bridge_set_refclk_freq(pdata);
+
+	/*
+	 * 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);
+
+	pdata->comms_enabled = true;
+
+	mutex_unlock(&pdata->comms_mutex);
+}
+
+static void ti_sn65dsi86_disable_comms(struct ti_sn65dsi86 *pdata)
+{
+	mutex_lock(&pdata->comms_mutex);
+
+	pdata->comms_enabled = false;
+	clk_disable_unprepare(pdata->refclk);
+
+	mutex_unlock(&pdata->comms_mutex);
+}
+
 static int __maybe_unused ti_sn65dsi86_resume(struct device *dev)
 {
 	struct ti_sn65dsi86 *pdata = dev_get_drvdata(dev);
@@ -263,6 +308,16 @@ static int __maybe_unused ti_sn65dsi86_resume(struct device *dev)
 
 	gpiod_set_value(pdata->enable_gpio, 1);
 
+	/*
+	 * If we have a reference clock we can enable communication w/ the
+	 * panel (including the aux channel) w/out any need for an input clock
+	 * so we can do it in resume which lets us read the EDID before
+	 * pre_enable(). Without a reference clock we need the MIPI reference
+	 * clock so reading early doesn't work.
+	 */
+	if (pdata->refclk)
+		ti_sn65dsi86_enable_comms(pdata);
+
 	return ret;
 }
 
@@ -271,6 +326,9 @@ static int __maybe_unused ti_sn65dsi86_suspend(struct device *dev)
 	struct ti_sn65dsi86 *pdata = dev_get_drvdata(dev);
 	int ret;
 
+	if (pdata->refclk)
+		ti_sn65dsi86_disable_comms(pdata);
+
 	gpiod_set_value(pdata->enable_gpio, 0);
 
 	ret = regulator_bulk_disable(SN_REGULATOR_SUPPLY_NUM, pdata->supplies);
@@ -844,27 +902,8 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
 
 	pm_runtime_get_sync(pdata->dev);
 
-	/* configure bridge ref_clk */
-	ti_sn_bridge_set_refclk_freq(pdata);
-
-	/*
-	 * 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);
+	if (!pdata->refclk)
+		ti_sn65dsi86_enable_comms(pdata);
 
 	drm_panel_prepare(pdata->panel);
 }
@@ -875,7 +914,8 @@ static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
 
 	drm_panel_unprepare(pdata->panel);
 
-	clk_disable_unprepare(pdata->refclk);
+	if (!pdata->refclk)
+		ti_sn65dsi86_disable_comms(pdata);
 
 	pm_runtime_put_sync(pdata->dev);
 }
@@ -909,6 +949,20 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
 	if (len > SN_AUX_MAX_PAYLOAD_BYTES)
 		return -EINVAL;
 
+	pm_runtime_get_sync(pdata->dev);
+	mutex_lock(&pdata->comms_mutex);
+
+	/*
+	 * If someone tries to do a DDC over AUX transaction before pre_enable()
+	 * on a device without a dedicated reference clock then we just can't
+	 * do it. Fail right away. This prevents non-refclk users from reading
+	 * the EDID before enabling the panel but such is life.
+	 */
+	if (!pdata->comms_enabled) {
+		ret = -EIO;
+		goto exit;
+	}
+
 	switch (request) {
 	case DP_AUX_NATIVE_WRITE:
 	case DP_AUX_I2C_WRITE:
@@ -919,7 +973,8 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
 		msg->reply = 0;
 		break;
 	default:
-		return -EINVAL;
+		ret = -EINVAL;
+		goto exit;
 	}
 
 	BUILD_BUG_ON(sizeof(addr_len) != sizeof(__be32));
@@ -943,11 +998,11 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
 	ret = regmap_read_poll_timeout(pdata->regmap, SN_AUX_CMD_REG, val,
 				       !(val & AUX_CMD_SEND), 0, 50 * 1000);
 	if (ret)
-		return ret;
+		goto exit;
 
 	ret = regmap_read(pdata->regmap, SN_AUX_CMD_STATUS_REG, &val);
 	if (ret)
-		return ret;
+		goto exit;
 
 	if (val & AUX_IRQ_STATUS_AUX_RPLY_TOUT) {
 		/*
@@ -955,13 +1010,14 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
 		 * but it hit a timeout. We ignore defers here because they're
 		 * handled in hardware.
 		 */
-		return -ETIMEDOUT;
+		ret = -ETIMEDOUT;
+		goto exit;
 	}
 
 	if (val & AUX_IRQ_STATUS_AUX_SHORT) {
 		ret = regmap_read(pdata->regmap, SN_AUX_LENGTH_REG, &len);
 		if (ret)
-			return ret;
+			goto exit;
 	} else if (val & AUX_IRQ_STATUS_NAT_I2C_FAIL) {
 		switch (request) {
 		case DP_AUX_I2C_WRITE:
@@ -973,18 +1029,19 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
 			msg->reply |= DP_AUX_NATIVE_REPLY_NACK;
 			break;
 		}
-		return 0;
+		len = 0;
+		goto exit;
 	}
 
-	if (request == DP_AUX_NATIVE_WRITE || request == DP_AUX_I2C_WRITE ||
-	    len == 0)
-		return len;
+	if (request != DP_AUX_NATIVE_WRITE && request != DP_AUX_I2C_WRITE && len != 0)
+		ret = regmap_bulk_read(pdata->regmap, SN_AUX_RDATA_REG(0), buf, len);
 
-	ret = regmap_bulk_read(pdata->regmap, SN_AUX_RDATA_REG(0), buf, len);
-	if (ret)
-		return ret;
+exit:
+	mutex_unlock(&pdata->comms_mutex);
+	pm_runtime_mark_last_busy(pdata->dev);
+	pm_runtime_put_autosuspend(pdata->dev);
 
-	return len;
+	return ret ? ret : len;
 }
 
 static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
@@ -1390,6 +1447,8 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
 	dev_set_drvdata(dev, pdata);
 	pdata->dev = dev;
 
+	mutex_init(&pdata->comms_mutex);
+
 	pdata->regmap = devm_regmap_init_i2c(client,
 					     &ti_sn65dsi86_regmap_config);
 	if (IS_ERR(pdata->regmap)) {
-- 
2.31.1.498.g6c1eba8ee3d-goog


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v5 14/20] drm/bridge: ti-sn65dsi86: Promote the AUX channel to its own sub-dev
  2021-04-23 16:58 [PATCH v5 00/20] drm: Fix EDID reading on ti-sn65dsi86; solve some chicken-and-egg problems Douglas Anderson
                   ` (12 preceding siblings ...)
  2021-04-23 16:58 ` [PATCH v5 13/20] drm/bridge: ti-sn65dsi86: If refclk, DP AUX can happen w/out pre-enable Douglas Anderson
@ 2021-04-23 16:59 ` Douglas Anderson
  2021-05-03 20:27   ` Doug Anderson
  2021-04-23 16:59 ` [PATCH v5 15/20] i2c: i2c-core-of: Fix corner case of finding adapter by node Douglas Anderson
                   ` (5 subsequent siblings)
  19 siblings, 1 reply; 36+ messages in thread
From: Douglas Anderson @ 2021-04-23 16:59 UTC (permalink / raw)
  To: Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, Sam Ravnborg, Wolfram Sang
  Cc: linux-arm-msm, robdclark, Stanislav Lisovskiy, Stephen Boyd,
	Steev Klimaszewski, Linus W, Maarten Lankhorst, linux-i2c,
	Bjorn Andersson, dri-devel, Douglas Anderson, Daniel Vetter,
	David Airlie, Robert Foss, linux-kernel

We'd like to be able to expose the DDC-over-AUX channel bus to our
panel. This gets into a chicken-and-egg problem because:
- The panel wants to get its DDC at probe time.
- The ti-sn65dsi86 MIPI-to-eDP bridge code, which provides the DDC
  bus, wants to get the panel at probe time.

By using a sub device we can fully create the AUX channel bits so that
the panel can get them. Then the panel can finish probing and the
bridge can probe.

To accomplish this, we also move registering the AUX channel out of
the bridge's attach code and do it right at probe time. We use devm to
manage cleanup.

NOTE: there's a little bit of a trick here. Though the AUX channel can
run without the MIPI-to-eDP bits of the code, the MIPI-to-eDP bits
can't run without the AUX channel. We could come up a complicated
signaling scheme (have the MIPI-to-eDP bits return EPROBE_DEFER for a
while or wait on some sort of completion), but it seems simple enough
to just not even bother creating the bridge device until the AUX
channel probes. That's what we'll do.

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

Changes in v5:
- Fix module compile problems (Bjorn + kbuild bot)
- Remove useless MODULE_DEVICE_TABLE (Bjorn).

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 87 +++++++++++++++++++++------
 1 file changed, 67 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 9dc3cd8e17df..3539ddf9d109 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -116,6 +116,7 @@
  * struct ti_sn65dsi86 - Platform data for ti-sn65dsi86 driver.
  * @bridge_aux:   AUX-bus sub device for MIPI-to-eDP bridge functionality.
  * @gpio_aux:     AUX-bus sub device for GPIO controller functionality.
+ * @aux_aux:      AUX-bus sub device for eDP AUX channel functionality.
  *
  * @dev:          Pointer to the top level (i2c) device.
  * @regmap:       Regmap for accessing i2c.
@@ -148,6 +149,7 @@
 struct ti_sn65dsi86 {
 	struct auxiliary_device		bridge_aux;
 	struct auxiliary_device		gpio_aux;
+	struct auxiliary_device		aux_aux;
 
 	struct device			*dev;
 	struct regmap			*regmap;
@@ -484,18 +486,12 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
 		return -EINVAL;
 	}
 
-	ret = drm_dp_aux_register(&pdata->aux);
-	if (ret < 0) {
-		drm_err(bridge->dev, "Failed to register DP AUX channel: %d\n", ret);
-		return ret;
-	}
-
 	ret = drm_connector_init(bridge->dev, &pdata->connector,
 				 &ti_sn_bridge_connector_funcs,
 				 DRM_MODE_CONNECTOR_eDP);
 	if (ret) {
 		DRM_ERROR("Failed to initialize connector with drm\n");
-		goto err_conn_init;
+		return ret;
 	}
 
 	drm_connector_helper_add(&pdata->connector,
@@ -552,8 +548,6 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
 	mipi_dsi_device_unregister(dsi);
 err_dsi_host:
 	drm_connector_cleanup(&pdata->connector);
-err_conn_init:
-	drm_dp_aux_unregister(&pdata->aux);
 	return ret;
 }
 
@@ -1330,11 +1324,6 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
 	if (ret)
 		return ret;
 
-	pdata->aux.name = "ti-sn65dsi86-aux";
-	pdata->aux.dev = pdata->dev;
-	pdata->aux.transfer = ti_sn_aux_transfer;
-	drm_dp_aux_init(&pdata->aux);
-
 	pdata->bridge.funcs = &ti_sn_bridge_funcs;
 	pdata->bridge.of_node = np;
 
@@ -1429,6 +1418,50 @@ static int ti_sn65dsi86_add_aux_device(struct ti_sn65dsi86 *pdata,
 	return ret;
 }
 
+static void ti_sn65dsi86_unregister_dp_aux(void *data)
+{
+	drm_dp_aux_unregister(data);
+}
+
+static int ti_sn_aux_probe(struct auxiliary_device *adev,
+			   const struct auxiliary_device_id *id)
+{
+	struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent);
+	int ret;
+
+	pdata->aux.name = "ti-sn65dsi86-aux";
+	pdata->aux.dev = pdata->dev;
+	pdata->aux.transfer = ti_sn_aux_transfer;
+	drm_dp_aux_init(&pdata->aux);
+
+	ret = drm_dp_aux_register(&pdata->aux);
+	if (ret < 0) {
+		drm_err(pdata, "Failed to register DP AUX channel: %d\n", ret);
+		return ret;
+	}
+	ret = devm_add_action_or_reset(&adev->dev,
+				       ti_sn65dsi86_unregister_dp_aux, &pdata->aux);
+	if (ret)
+		return ret;
+
+	/*
+	 * The eDP to MIPI bridge parts don't work until the AUX channel is
+	 * setup so we don't add it in the main driver probe, we add it now.
+	 */
+	return ti_sn65dsi86_add_aux_device(pdata, &pdata->bridge_aux, "bridge");
+}
+
+static const struct auxiliary_device_id ti_sn_aux_id_table[] = {
+	{ .name = "ti_sn65dsi86.aux", },
+	{},
+};
+
+static struct auxiliary_driver ti_sn_aux_driver = {
+	.name = "aux",
+	.probe = ti_sn_aux_probe,
+	.id_table = ti_sn_aux_id_table,
+};
+
 static int ti_sn65dsi86_probe(struct i2c_client *client,
 			      const struct i2c_device_id *id)
 {
@@ -1487,10 +1520,11 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
 	 * motiviation here is to solve the chicken-and-egg problem of probe
 	 * ordering. The bridge wants the panel to be there when it probes.
 	 * The panel wants its HPD GPIO (provided by sn65dsi86 on some boards)
-	 * when it probes. There will soon be other devices (DDC I2C bus, PWM)
-	 * that have the same problem. Having sub-devices allows the some sub
-	 * devices to finish probing even if others return -EPROBE_DEFER and
-	 * gets us around the problems.
+	 * when it probes. The panel and maybe backlight might want the DDC
+	 * bus. Soon the PWM provided by the bridge chip will have the same
+	 * problem. Having sub-devices allows the some sub devices to finish
+	 * probing even if others return -EPROBE_DEFER and gets us around the
+	 * problems.
 	 */
 
 	if (IS_ENABLED(CONFIG_OF_GPIO)) {
@@ -1499,7 +1533,13 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
 			return ret;
 	}
 
-	return ti_sn65dsi86_add_aux_device(pdata, &pdata->bridge_aux, "bridge");
+	/*
+	 * NOTE: At the end of the AUX channel probe we'll add the aux device
+	 * for the bridge. This is because the bridge can't be used until the
+	 * AUX channel is there and this is a very simple solution to the
+	 * dependency problem.
+	 */
+	return ti_sn65dsi86_add_aux_device(pdata, &pdata->aux_aux, "aux");
 }
 
 static struct i2c_device_id ti_sn65dsi86_id[] = {
@@ -1536,12 +1576,18 @@ static int __init ti_sn65dsi86_init(void)
 	if (ret)
 		goto err_main_was_registered;
 
-	ret = auxiliary_driver_register(&ti_sn_bridge_driver);
+	ret = auxiliary_driver_register(&ti_sn_aux_driver);
 	if (ret)
 		goto err_gpio_was_registered;
 
+	ret = auxiliary_driver_register(&ti_sn_bridge_driver);
+	if (ret)
+		goto err_aux_was_registered;
+
 	return 0;
 
+err_aux_was_registered:
+	auxiliary_driver_unregister(&ti_sn_aux_driver);
 err_gpio_was_registered:
 	ti_sn_gpio_unregister();
 err_main_was_registered:
@@ -1554,6 +1600,7 @@ module_init(ti_sn65dsi86_init);
 static void __exit ti_sn65dsi86_exit(void)
 {
 	auxiliary_driver_unregister(&ti_sn_bridge_driver);
+	auxiliary_driver_unregister(&ti_sn_aux_driver);
 	ti_sn_gpio_unregister();
 	i2c_del_driver(&ti_sn65dsi86_driver);
 }
-- 
2.31.1.498.g6c1eba8ee3d-goog


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v5 15/20] i2c: i2c-core-of: Fix corner case of finding adapter by node
  2021-04-23 16:58 [PATCH v5 00/20] drm: Fix EDID reading on ti-sn65dsi86; solve some chicken-and-egg problems Douglas Anderson
                   ` (13 preceding siblings ...)
  2021-04-23 16:59 ` [PATCH v5 14/20] drm/bridge: ti-sn65dsi86: Promote the AUX channel to its own sub-dev Douglas Anderson
@ 2021-04-23 16:59 ` Douglas Anderson
  2021-04-23 16:59 ` [PATCH v5 16/20] drm/panel: panel-simple: Remove extra call: drm_connector_update_edid_property() Douglas Anderson
                   ` (4 subsequent siblings)
  19 siblings, 0 replies; 36+ messages in thread
From: Douglas Anderson @ 2021-04-23 16:59 UTC (permalink / raw)
  To: Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, Sam Ravnborg, Wolfram Sang
  Cc: linux-arm-msm, robdclark, Stanislav Lisovskiy, Stephen Boyd,
	Steev Klimaszewski, Linus W, Maarten Lankhorst, linux-i2c,
	Bjorn Andersson, dri-devel, Douglas Anderson, linux-kernel

The of_find_i2c_adapter_by_node() could end up failing to find an
adapter in certain conditions. Specifically it's possible that
of_dev_or_parent_node_match() could end up finding an I2C client in
the list and cause bus_find_device() to stop early even though an I2C
adapter was present later in the list.

Let's move the i2c_verify_adapter() into the predicate function to
prevent this. Now we'll properly skip over the I2C client and be able
to find the I2C adapter.

This issue has always been a potential problem if a single device tree
node could represent both an I2C client and an adapter. I believe this
is a sane thing to do if, for instance, an I2C-connected DP bridge
chip is present. The bridge chip is an I2C client but it can also
provide an I2C adapter (DDC tunneled over AUX channel). We don't want
to have to create a sub-node just so a panel can link to it with the
"ddc-i2c-bus" property.

I believe that this problem got worse, however, with commit
e814e688413a ("i2c: of: Try to find an I2C adapter matching the
parent"). Starting at that commit it would be even easier to
accidentally miss finding the adapter.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
This commit is sorta just jammed into the middle of my series. It has
no dependencies on the earlier patches in the series and I think it
can land independently in the i2c tree. Later patches in the series
won't work right without this one, but they won't crash. If we can't
find the i2c bus we'll just fall back to the hardcoded panel modes
which, at least today, all panels have.

I'll also note that part of me wonders if we should actually fix this
further to run two passes through everything: first look to see if we
find an exact match and only look at the parent pointer if there is no
match. I don't currently have a need for that and it's a slightly
bigger change, but it seems conceivable that it could affect someone?

(no changes since v1)

 drivers/i2c/i2c-core-of.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
index 3ed74aa4b44b..de0bf5fce3a2 100644
--- a/drivers/i2c/i2c-core-of.c
+++ b/drivers/i2c/i2c-core-of.c
@@ -124,6 +124,14 @@ static int of_dev_or_parent_node_match(struct device *dev, const void *data)
 	return 0;
 }
 
+static int of_i2c_adapter_match(struct device *dev, const void *data)
+{
+	if (!of_dev_or_parent_node_match(dev, data))
+		return 0;
+
+	return !!i2c_verify_adapter(dev);
+}
+
 /* must call put_device() when done with returned i2c_client device */
 struct i2c_client *of_find_i2c_device_by_node(struct device_node *node)
 {
@@ -146,18 +154,13 @@ EXPORT_SYMBOL(of_find_i2c_device_by_node);
 struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
 {
 	struct device *dev;
-	struct i2c_adapter *adapter;
 
 	dev = bus_find_device(&i2c_bus_type, NULL, node,
-			      of_dev_or_parent_node_match);
+			      of_i2c_adapter_match);
 	if (!dev)
 		return NULL;
 
-	adapter = i2c_verify_adapter(dev);
-	if (!adapter)
-		put_device(dev);
-
-	return adapter;
+	return to_i2c_adapter(dev);
 }
 EXPORT_SYMBOL(of_find_i2c_adapter_by_node);
 
-- 
2.31.1.498.g6c1eba8ee3d-goog


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v5 16/20] drm/panel: panel-simple: Remove extra call: drm_connector_update_edid_property()
  2021-04-23 16:58 [PATCH v5 00/20] drm: Fix EDID reading on ti-sn65dsi86; solve some chicken-and-egg problems Douglas Anderson
                   ` (14 preceding siblings ...)
  2021-04-23 16:59 ` [PATCH v5 15/20] i2c: i2c-core-of: Fix corner case of finding adapter by node Douglas Anderson
@ 2021-04-23 16:59 ` Douglas Anderson
  2021-04-28 16:38   ` Sean Paul
  2021-04-23 16:59 ` [PATCH v5 17/20] drm/panel: panel-simple: Power the panel when reading the EDID Douglas Anderson
                   ` (3 subsequent siblings)
  19 siblings, 1 reply; 36+ messages in thread
From: Douglas Anderson @ 2021-04-23 16:59 UTC (permalink / raw)
  To: Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, Sam Ravnborg, Wolfram Sang
  Cc: linux-arm-msm, robdclark, Stanislav Lisovskiy, Stephen Boyd,
	Steev Klimaszewski, Linus W, Maarten Lankhorst, linux-i2c,
	Bjorn Andersson, dri-devel, Douglas Anderson, Daniel Vetter,
	David Airlie, Thierry Reding, linux-kernel

As of commit 5186421cbfe2 ("drm: Introduce epoch counter to
drm_connector") the drm_get_edid() function calls
drm_connector_update_edid_property() for us. There's no reason for us
to call it again.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
As Laurent pointed out [1] this is actually a pretty common
problem. His suggestion to do this more broadly is a good idea but
this series is probably a bit ambitious already so I would suggest
that be taken up separately.

[1] https://lore.kernel.org/r/YGphgcESWsozCi1y@pendragon.ideasonboard.com

(no changes since v1)

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

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index bd208abcbf07..4de33c929a59 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -512,7 +512,6 @@ static int panel_simple_get_modes(struct drm_panel *panel,
 	if (p->ddc) {
 		struct edid *edid = drm_get_edid(connector, p->ddc);
 
-		drm_connector_update_edid_property(connector, edid);
 		if (edid) {
 			num += drm_add_edid_modes(connector, edid);
 			kfree(edid);
-- 
2.31.1.498.g6c1eba8ee3d-goog


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v5 17/20] drm/panel: panel-simple: Power the panel when reading the EDID
  2021-04-23 16:58 [PATCH v5 00/20] drm: Fix EDID reading on ti-sn65dsi86; solve some chicken-and-egg problems Douglas Anderson
                   ` (15 preceding siblings ...)
  2021-04-23 16:59 ` [PATCH v5 16/20] drm/panel: panel-simple: Remove extra call: drm_connector_update_edid_property() Douglas Anderson
@ 2021-04-23 16:59 ` Douglas Anderson
  2021-04-28 16:44   ` Sean Paul
  2021-04-23 16:59 ` [PATCH v5 18/20] drm/panel: panel-simple: Cache the EDID as long as we retain power Douglas Anderson
                   ` (2 subsequent siblings)
  19 siblings, 1 reply; 36+ messages in thread
From: Douglas Anderson @ 2021-04-23 16:59 UTC (permalink / raw)
  To: Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, Sam Ravnborg, Wolfram Sang
  Cc: linux-arm-msm, robdclark, Stanislav Lisovskiy, Stephen Boyd,
	Steev Klimaszewski, Linus W, Maarten Lankhorst, linux-i2c,
	Bjorn Andersson, dri-devel, Douglas Anderson, Daniel Vetter,
	David Airlie, Thierry Reding, linux-kernel

I don't believe that it ever makes sense to read the EDID when a panel
is not powered and the powering on of the panel is the job of
prepare(). Let's make sure that this happens before we try to read the
EDID. We use the pm_runtime functions directly rather than directly
calling the normal prepare() function because the pm_runtime functions
are definitely refcounted whereas it's less clear if the prepare() one
is.

NOTE: I'm not 100% sure how EDID reading was working for folks in the
past, but I can only assume that it was failing on the initial attempt
and then working only later. This patch, presumably, will fix that. If
some panel out there really can read the EDID without powering up and
it's a big advantage to preserve the old behavior we can add a
per-panel flag. It appears that providing the DDC bus to the panel in
the past was somewhat uncommon in any case.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

(no changes since v1)

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

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 4de33c929a59..a12dfe8b8d90 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -510,12 +510,18 @@ static int panel_simple_get_modes(struct drm_panel *panel,
 
 	/* probe EDID if a DDC bus is available */
 	if (p->ddc) {
-		struct edid *edid = drm_get_edid(connector, p->ddc);
+		struct edid *edid;
 
+		pm_runtime_get_sync(panel->dev);
+
+		edid = drm_get_edid(connector, p->ddc);
 		if (edid) {
 			num += drm_add_edid_modes(connector, edid);
 			kfree(edid);
 		}
+
+		pm_runtime_mark_last_busy(panel->dev);
+		pm_runtime_put_autosuspend(panel->dev);
 	}
 
 	/* add hard-coded panel modes */
-- 
2.31.1.498.g6c1eba8ee3d-goog


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v5 18/20] drm/panel: panel-simple: Cache the EDID as long as we retain power
  2021-04-23 16:58 [PATCH v5 00/20] drm: Fix EDID reading on ti-sn65dsi86; solve some chicken-and-egg problems Douglas Anderson
                   ` (16 preceding siblings ...)
  2021-04-23 16:59 ` [PATCH v5 17/20] drm/panel: panel-simple: Power the panel when reading the EDID Douglas Anderson
@ 2021-04-23 16:59 ` Douglas Anderson
  2021-04-28 16:50   ` Sean Paul
  2021-04-23 16:59 ` [PATCH v5 19/20] drm/bridge: ti-sn65dsi86: Don't read EDID blob over DDC Douglas Anderson
  2021-04-23 16:59 ` [PATCH v5 20/20] arm64: dts: qcom: Link the panel to the bridge's DDC bus Douglas Anderson
  19 siblings, 1 reply; 36+ messages in thread
From: Douglas Anderson @ 2021-04-23 16:59 UTC (permalink / raw)
  To: Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, Sam Ravnborg, Wolfram Sang
  Cc: linux-arm-msm, robdclark, Stanislav Lisovskiy, Stephen Boyd,
	Steev Klimaszewski, Linus W, Maarten Lankhorst, linux-i2c,
	Bjorn Andersson, dri-devel, Douglas Anderson, Daniel Vetter,
	David Airlie, Thierry Reding, linux-kernel

It doesn't make sense to go out to the bus and read the EDID over and
over again. Let's cache it and throw away the cache when we turn power
off from the panel. Autosuspend means that even if there are several
calls to read the EDID before we officially turn the power on then we
should get good use out of this cache.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

(no changes since v1)

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

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index a12dfe8b8d90..9be050ab372f 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -189,6 +189,8 @@ struct panel_simple {
 	struct gpio_desc *enable_gpio;
 	struct gpio_desc *hpd_gpio;
 
+	struct edid *edid;
+
 	struct drm_display_mode override_mode;
 
 	enum drm_panel_orientation orientation;
@@ -345,6 +347,9 @@ static int panel_simple_suspend(struct device *dev)
 	regulator_disable(p->supply);
 	p->unprepared_time = ktime_get();
 
+	kfree(p->edid);
+	p->edid = NULL;
+
 	return 0;
 }
 
@@ -510,15 +515,13 @@ static int panel_simple_get_modes(struct drm_panel *panel,
 
 	/* probe EDID if a DDC bus is available */
 	if (p->ddc) {
-		struct edid *edid;
-
 		pm_runtime_get_sync(panel->dev);
 
-		edid = drm_get_edid(connector, p->ddc);
-		if (edid) {
-			num += drm_add_edid_modes(connector, edid);
-			kfree(edid);
-		}
+		if (!p->edid)
+			p->edid = drm_get_edid(connector, p->ddc);
+
+		if (p->edid)
+			num += drm_add_edid_modes(connector, p->edid);
 
 		pm_runtime_mark_last_busy(panel->dev);
 		pm_runtime_put_autosuspend(panel->dev);
-- 
2.31.1.498.g6c1eba8ee3d-goog


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v5 19/20] drm/bridge: ti-sn65dsi86: Don't read EDID blob over DDC
  2021-04-23 16:58 [PATCH v5 00/20] drm: Fix EDID reading on ti-sn65dsi86; solve some chicken-and-egg problems Douglas Anderson
                   ` (17 preceding siblings ...)
  2021-04-23 16:59 ` [PATCH v5 18/20] drm/panel: panel-simple: Cache the EDID as long as we retain power Douglas Anderson
@ 2021-04-23 16:59 ` Douglas Anderson
  2021-04-23 16:59 ` [PATCH v5 20/20] arm64: dts: qcom: Link the panel to the bridge's DDC bus Douglas Anderson
  19 siblings, 0 replies; 36+ messages in thread
From: Douglas Anderson @ 2021-04-23 16:59 UTC (permalink / raw)
  To: Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, Sam Ravnborg, Wolfram Sang
  Cc: linux-arm-msm, robdclark, Stanislav Lisovskiy, Stephen Boyd,
	Steev Klimaszewski, Linus W, Maarten Lankhorst, linux-i2c,
	Bjorn Andersson, dri-devel, Douglas Anderson, Daniel Vetter,
	David Airlie, Robert Foss, linux-kernel

This is really just a revert of commit 58074b08c04a ("drm/bridge:
ti-sn65dsi86: Read EDID blob over DDC"), resolving conflicts.

The old code failed to read the EDID properly in a very important
case: before the bridge's pre_enable() was called. The way things need
to work:
1. Read the EDID.
2. Based on the EDID, decide on video settings and pixel clock.
3. Enable the bridge w/ the desired settings.

The way things were working:
1. Try to read the EDID but fail; fall back to hardcoded values.
2. Based on hardcoded values, decide on video settings and pixel clock.
3. Enable the bridge w/ the desired settings.
4. Try again to read the EDID, it works now!
5. Realize that the hardcoded settings weren't quite right.
6. Disable / reenable the bridge w/ the right settings.

The reasons for the failures were twofold:
a) Since we never ran the bridge chip's pre-enable then we never set
   the bit to ignore HPD. This meant the bridge chip didn't even _try_
   to go out on the bus and communicate with the panel.
b) Even if we fixed things to ignore HPD, the EDID still wouldn't read
   if the panel wasn't on.

Instead of reverting the code, we could fix it to set the HPD bit and
also power on the panel. However, it also works nicely to just let the
panel code read the EDID. Now that we've split the driver up we can
expose the DDC AUX channel bus to the panel node. The panel can take
charge of reading the EDID.

NOTE: in order for things to work, anyone that needs to read the EDID
will need to add something that looks like this to their panel in the
dts:
  ddc-i2c-bus = <&sn65dsi86_bridge>;

Presumably it's OK to land this without waiting for users to add the
dts property since the EDID reading was a bit broken anyway, was
"recently" added, and we know we must have the fallback mode to use
(since the EDID reading was a bit broken).

Suggested-by: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

(no changes since v1)

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 22 ----------------------
 1 file changed, 22 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 3539ddf9d109..26851119df96 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -125,7 +125,6 @@
  * @connector:    Our connector.
  * @host_node:    Remote DSI node.
  * @dsi:          Our MIPI DSI source.
- * @edid:         Detected EDID of eDP panel.
  * @refclk:       Our reference clock.
  * @panel:        Our panel.
  * @enable_gpio:  The GPIO we toggle to enable the bridge.
@@ -156,7 +155,6 @@ struct ti_sn65dsi86 {
 	struct drm_dp_aux		aux;
 	struct drm_bridge		bridge;
 	struct drm_connector		connector;
-	struct edid			*edid;
 	struct device_node		*host_node;
 	struct mipi_dsi_device		*dsi;
 	struct clk			*refclk;
@@ -405,24 +403,6 @@ connector_to_ti_sn65dsi86(struct drm_connector *connector)
 static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
 {
 	struct ti_sn65dsi86 *pdata = connector_to_ti_sn65dsi86(connector);
-	struct edid *edid = pdata->edid;
-	int num, ret;
-
-	if (!edid) {
-		pm_runtime_get_sync(pdata->dev);
-		edid = pdata->edid = drm_get_edid(connector, &pdata->aux.ddc);
-		pm_runtime_put_autosuspend(pdata->dev);
-	}
-
-	if (edid && drm_edid_is_valid(edid)) {
-		ret = drm_connector_update_edid_property(connector, edid);
-		if (!ret) {
-			num = drm_add_edid_modes(connector, edid);
-			if (num)
-				return num;
-		}
-	}
-
 	return drm_panel_get_modes(pdata->panel, connector);
 }
 
@@ -1344,8 +1324,6 @@ static void ti_sn_bridge_remove(struct auxiliary_device *adev)
 		mipi_dsi_device_unregister(pdata->dsi);
 	}
 
-	kfree(pdata->edid);
-
 	drm_bridge_remove(&pdata->bridge);
 
 	of_node_put(pdata->host_node);
-- 
2.31.1.498.g6c1eba8ee3d-goog


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v5 20/20] arm64: dts: qcom: Link the panel to the bridge's DDC bus
  2021-04-23 16:58 [PATCH v5 00/20] drm: Fix EDID reading on ti-sn65dsi86; solve some chicken-and-egg problems Douglas Anderson
                   ` (18 preceding siblings ...)
  2021-04-23 16:59 ` [PATCH v5 19/20] drm/bridge: ti-sn65dsi86: Don't read EDID blob over DDC Douglas Anderson
@ 2021-04-23 16:59 ` Douglas Anderson
  19 siblings, 0 replies; 36+ messages in thread
From: Douglas Anderson @ 2021-04-23 16:59 UTC (permalink / raw)
  To: Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, Sam Ravnborg, Wolfram Sang
  Cc: linux-arm-msm, robdclark, Stanislav Lisovskiy, Stephen Boyd,
	Steev Klimaszewski, Linus W, Maarten Lankhorst, linux-i2c,
	Bjorn Andersson, dri-devel, Douglas Anderson, Andy Gross,
	Rob Herring, devicetree, linux-kernel

Adding this link allows the panel code to do things like read the
EDID.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

(no changes since v1)

 arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
index 24d293ef56d7..96e530594509 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
@@ -265,6 +265,7 @@ panel: panel {
 		power-supply = <&pp3300_dx_edp>;
 		backlight = <&backlight>;
 		hpd-gpios = <&sn65dsi86_bridge 2 GPIO_ACTIVE_HIGH>;
+		ddc-i2c-bus = <&sn65dsi86_bridge>;
 
 		ports {
 			port {
-- 
2.31.1.498.g6c1eba8ee3d-goog


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* Re: [PATCH v5 01/20] drm/panel: panel-simple: Add missing pm_runtime_disable() calls
  2021-04-23 16:58 ` [PATCH v5 01/20] drm/panel: panel-simple: Add missing pm_runtime_disable() calls Douglas Anderson
@ 2021-04-28 16:32   ` Sean Paul
  2021-04-30  0:58   ` Linus Walleij
  1 sibling, 0 replies; 36+ messages in thread
From: Sean Paul @ 2021-04-28 16:32 UTC (permalink / raw)
  To: Douglas Anderson
  Cc: Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, Sam Ravnborg, Wolfram Sang, robdclark, dri-devel,
	David Airlie, linux-arm-msm, Steev Klimaszewski, Stephen Boyd,
	Stanislav Lisovskiy, Thierry Reding, linux-i2c, Bjorn Andersson,
	Linux Kernel Mailing List

On Fri, Apr 23, 2021 at 12:59 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> In commit 3235b0f20a0a ("drm/panel: panel-simple: Use runtime pm to
> avoid excessive unprepare / prepare") we started using pm_runtime, but
> my patch neglected to add the proper pm_runtime_disable(). Doh! Add
> them now.
>
> Fixes: 3235b0f20a0a ("drm/panel: panel-simple: Use runtime pm to avoid excessive unprepare / prepare")
> Reported-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
> Changes in v5:
> - Missing pm_runtime_disable() patch new for v5.
>
>  drivers/gpu/drm/panel/panel-simple.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index 6b22872b3281..9746eda6f675 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -797,12 +797,14 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
>
>         err = drm_panel_of_backlight(&panel->base);
>         if (err)
> -               goto free_ddc;
> +               goto disable_pm_runtime;
>
>         drm_panel_add(&panel->base);
>
>         return 0;
>
> +disable_pm_runtime:
> +       pm_runtime_disable(dev);
>  free_ddc:
>         if (panel->ddc)
>                 put_device(&panel->ddc->dev);
> @@ -818,6 +820,7 @@ static int panel_simple_remove(struct device *dev)
>         drm_panel_disable(&panel->base);
>         drm_panel_unprepare(&panel->base);
>
> +       pm_runtime_disable(dev);
>         if (panel->ddc)
>                 put_device(&panel->ddc->dev);
>
> --
> 2.31.1.498.g6c1eba8ee3d-goog
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v5 10/20] drm/panel: panel-simple: Get rid of hacky HPD chicken-and-egg code
  2021-04-23 16:58 ` [PATCH v5 10/20] drm/panel: panel-simple: Get rid of hacky HPD chicken-and-egg code Douglas Anderson
@ 2021-04-28 16:35   ` Sean Paul
  0 siblings, 0 replies; 36+ messages in thread
From: Sean Paul @ 2021-04-28 16:35 UTC (permalink / raw)
  To: Douglas Anderson
  Cc: Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, Sam Ravnborg, Wolfram Sang, robdclark, dri-devel,
	David Airlie, linux-arm-msm, Steev Klimaszewski, Stephen Boyd,
	Stanislav Lisovskiy, Thierry Reding, linux-i2c, Bjorn Andersson,
	Linux Kernel Mailing List

On Fri, Apr 23, 2021 at 1:00 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> When I added support for the hpd-gpio to simple-panel in commit
> 48834e6084f1 ("drm/panel-simple: Support hpd-gpios for delaying
> prepare()"), I added a special case to handle a circular dependency I
> was running into on the ti-sn65dsi86 bridge chip. On my board the
> hpd-gpio is actually provided by the bridge chip. That was causing
> some circular dependency problems that I had to work around by getting
> the hpd-gpio late.
>
> I've now reorganized the ti-sn65dsi86 bridge chip driver to be a
> collection of sub-drivers. Now the GPIO part can probe separately and
> that breaks the chain. Let's get rid of the old code to clean things
> up.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> ---
>
> (no changes since v1)
>
>  drivers/gpu/drm/panel/panel-simple.c | 24 +++++-------------------
>  1 file changed, 5 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index 9746eda6f675..bd208abcbf07 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -366,8 +366,7 @@ static int panel_simple_unprepare(struct drm_panel *panel)
>         return 0;
>  }
>
> -static int panel_simple_get_hpd_gpio(struct device *dev,
> -                                    struct panel_simple *p, bool from_probe)
> +static int panel_simple_get_hpd_gpio(struct device *dev, struct panel_simple *p)
>  {
>         int err;
>
> @@ -375,17 +374,10 @@ static int panel_simple_get_hpd_gpio(struct device *dev,
>         if (IS_ERR(p->hpd_gpio)) {
>                 err = PTR_ERR(p->hpd_gpio);
>
> -               /*
> -                * If we're called from probe we won't consider '-EPROBE_DEFER'
> -                * to be an error--we'll leave the error code in "hpd_gpio".
> -                * When we try to use it we'll try again.  This allows for
> -                * circular dependencies where the component providing the
> -                * hpd gpio needs the panel to init before probing.
> -                */
> -               if (err != -EPROBE_DEFER || !from_probe) {
> +               if (err != -EPROBE_DEFER)
>                         dev_err(dev, "failed to get 'hpd' GPIO: %d\n", err);
> -                       return err;
> -               }
> +
> +               return err;
>         }
>
>         return 0;
> @@ -416,12 +408,6 @@ static int panel_simple_prepare_once(struct panel_simple *p)
>                 msleep(delay);
>
>         if (p->hpd_gpio) {
> -               if (IS_ERR(p->hpd_gpio)) {
> -                       err = panel_simple_get_hpd_gpio(dev, p, false);
> -                       if (err)
> -                               goto error;
> -               }
> -
>                 if (p->desc->delay.hpd_absent_delay)
>                         hpd_wait_us = p->desc->delay.hpd_absent_delay * 1000UL;
>                 else
> @@ -682,7 +668,7 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
>
>         panel->no_hpd = of_property_read_bool(dev->of_node, "no-hpd");
>         if (!panel->no_hpd) {
> -               err = panel_simple_get_hpd_gpio(dev, panel, true);
> +               err = panel_simple_get_hpd_gpio(dev, panel);
>                 if (err)
>                         return err;
>         }
> --
> 2.31.1.498.g6c1eba8ee3d-goog
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v5 16/20] drm/panel: panel-simple: Remove extra call: drm_connector_update_edid_property()
  2021-04-23 16:59 ` [PATCH v5 16/20] drm/panel: panel-simple: Remove extra call: drm_connector_update_edid_property() Douglas Anderson
@ 2021-04-28 16:38   ` Sean Paul
  0 siblings, 0 replies; 36+ messages in thread
From: Sean Paul @ 2021-04-28 16:38 UTC (permalink / raw)
  To: Douglas Anderson
  Cc: Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, Sam Ravnborg, Wolfram Sang, robdclark, dri-devel,
	David Airlie, linux-arm-msm, Steev Klimaszewski, Stephen Boyd,
	Stanislav Lisovskiy, Thierry Reding, linux-i2c, Bjorn Andersson,
	Linux Kernel Mailing List

On Fri, Apr 23, 2021 at 1:00 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> As of commit 5186421cbfe2 ("drm: Introduce epoch counter to
> drm_connector") the drm_get_edid() function calls
> drm_connector_update_edid_property() for us. There's no reason for us
> to call it again.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> ---
> As Laurent pointed out [1] this is actually a pretty common
> problem. His suggestion to do this more broadly is a good idea but
> this series is probably a bit ambitious already so I would suggest
> that be taken up separately.
>
> [1] https://lore.kernel.org/r/YGphgcESWsozCi1y@pendragon.ideasonboard.com
>
> (no changes since v1)
>
>  drivers/gpu/drm/panel/panel-simple.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index bd208abcbf07..4de33c929a59 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -512,7 +512,6 @@ static int panel_simple_get_modes(struct drm_panel *panel,
>         if (p->ddc) {
>                 struct edid *edid = drm_get_edid(connector, p->ddc);
>
> -               drm_connector_update_edid_property(connector, edid);
>                 if (edid) {
>                         num += drm_add_edid_modes(connector, edid);
>                         kfree(edid);
> --
> 2.31.1.498.g6c1eba8ee3d-goog
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v5 17/20] drm/panel: panel-simple: Power the panel when reading the EDID
  2021-04-23 16:59 ` [PATCH v5 17/20] drm/panel: panel-simple: Power the panel when reading the EDID Douglas Anderson
@ 2021-04-28 16:44   ` Sean Paul
  0 siblings, 0 replies; 36+ messages in thread
From: Sean Paul @ 2021-04-28 16:44 UTC (permalink / raw)
  To: Douglas Anderson
  Cc: Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, Sam Ravnborg, Wolfram Sang, robdclark, dri-devel,
	David Airlie, linux-arm-msm, Steev Klimaszewski, Stephen Boyd,
	Stanislav Lisovskiy, Thierry Reding, linux-i2c, Bjorn Andersson,
	Linux Kernel Mailing List

On Fri, Apr 23, 2021 at 1:00 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> I don't believe that it ever makes sense to read the EDID when a panel
> is not powered and the powering on of the panel is the job of
> prepare(). Let's make sure that this happens before we try to read the
> EDID. We use the pm_runtime functions directly rather than directly
> calling the normal prepare() function because the pm_runtime functions
> are definitely refcounted whereas it's less clear if the prepare() one
> is.
>
> NOTE: I'm not 100% sure how EDID reading was working for folks in the
> past, but I can only assume that it was failing on the initial attempt
> and then working only later. This patch, presumably, will fix that. If
> some panel out there really can read the EDID without powering up and
> it's a big advantage to preserve the old behavior we can add a
> per-panel flag. It appears that providing the DDC bus to the panel in
> the past was somewhat uncommon in any case.
>

Maybe some combination of drivers caching the EDID for panels while
they're already powered and overly broad pm runtime references?

At any rate, this makes sense to me,

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>
> (no changes since v1)
>
>  drivers/gpu/drm/panel/panel-simple.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index 4de33c929a59..a12dfe8b8d90 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -510,12 +510,18 @@ static int panel_simple_get_modes(struct drm_panel *panel,
>
>         /* probe EDID if a DDC bus is available */
>         if (p->ddc) {
> -               struct edid *edid = drm_get_edid(connector, p->ddc);
> +               struct edid *edid;
>
> +               pm_runtime_get_sync(panel->dev);
> +
> +               edid = drm_get_edid(connector, p->ddc);
>                 if (edid) {
>                         num += drm_add_edid_modes(connector, edid);
>                         kfree(edid);
>                 }
> +
> +               pm_runtime_mark_last_busy(panel->dev);
> +               pm_runtime_put_autosuspend(panel->dev);
>         }
>
>         /* add hard-coded panel modes */
> --
> 2.31.1.498.g6c1eba8ee3d-goog
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v5 18/20] drm/panel: panel-simple: Cache the EDID as long as we retain power
  2021-04-23 16:59 ` [PATCH v5 18/20] drm/panel: panel-simple: Cache the EDID as long as we retain power Douglas Anderson
@ 2021-04-28 16:50   ` Sean Paul
  0 siblings, 0 replies; 36+ messages in thread
From: Sean Paul @ 2021-04-28 16:50 UTC (permalink / raw)
  To: Douglas Anderson
  Cc: Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, Sam Ravnborg, Wolfram Sang, robdclark, dri-devel,
	David Airlie, linux-arm-msm, Steev Klimaszewski, Stephen Boyd,
	Stanislav Lisovskiy, Thierry Reding, linux-i2c, Bjorn Andersson,
	Linux Kernel Mailing List

On Fri, Apr 23, 2021 at 1:00 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> It doesn't make sense to go out to the bus and read the EDID over and
> over again. Let's cache it and throw away the cache when we turn power
> off from the panel. Autosuspend means that even if there are several
> calls to read the EDID before we officially turn the power on then we
> should get good use out of this cache.
>

I think i915 caches the edid once on init and never refreshes it
(assuming no hotplugs). That said, I think it makes sense for a more
conservative approach in panel-simple.


> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>
> (no changes since v1)
>
>  drivers/gpu/drm/panel/panel-simple.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index a12dfe8b8d90..9be050ab372f 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -189,6 +189,8 @@ struct panel_simple {
>         struct gpio_desc *enable_gpio;
>         struct gpio_desc *hpd_gpio;
>
> +       struct edid *edid;
> +
>         struct drm_display_mode override_mode;
>
>         enum drm_panel_orientation orientation;
> @@ -345,6 +347,9 @@ static int panel_simple_suspend(struct device *dev)
>         regulator_disable(p->supply);
>         p->unprepared_time = ktime_get();
>
> +       kfree(p->edid);
> +       p->edid = NULL;
> +
>         return 0;
>  }
>
> @@ -510,15 +515,13 @@ static int panel_simple_get_modes(struct drm_panel *panel,
>
>         /* probe EDID if a DDC bus is available */
>         if (p->ddc) {
> -               struct edid *edid;
> -
>                 pm_runtime_get_sync(panel->dev);
>
> -               edid = drm_get_edid(connector, p->ddc);
> -               if (edid) {
> -                       num += drm_add_edid_modes(connector, edid);
> -                       kfree(edid);
> -               }
> +               if (!p->edid)
> +                       p->edid = drm_get_edid(connector, p->ddc);
> +
> +               if (p->edid)
> +                       num += drm_add_edid_modes(connector, p->edid);

I suppose this would keep banging on the ddc if drm_get_edid()
continuously returns NULL, but maybe that's desireable (it'll succeed
sometime in the future)? At any rate, this is an improvement on
current behavior so it has my vote.

Reviewed-by: Sean Paul <seanpaul@chromium.org>

>
>                 pm_runtime_mark_last_busy(panel->dev);
>                 pm_runtime_put_autosuspend(panel->dev);
> --
> 2.31.1.498.g6c1eba8ee3d-goog
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v5 01/20] drm/panel: panel-simple: Add missing pm_runtime_disable() calls
  2021-04-23 16:58 ` [PATCH v5 01/20] drm/panel: panel-simple: Add missing pm_runtime_disable() calls Douglas Anderson
  2021-04-28 16:32   ` Sean Paul
@ 2021-04-30  0:58   ` Linus Walleij
  2021-04-30  1:24     ` Doug Anderson
  1 sibling, 1 reply; 36+ messages in thread
From: Linus Walleij @ 2021-04-30  0:58 UTC (permalink / raw)
  To: Douglas Anderson
  Cc: Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, Sam Ravnborg, Wolfram Sang, MSM, Rob Clark,
	Stanislav Lisovskiy, Stephen Boyd, Steev Klimaszewski,
	Maarten Lankhorst, linux-i2c, Bjorn Andersson,
	open list:DRM PANEL DRIVERS, Daniel Vetter, David Airlie,
	Thierry Reding, linux-kernel

On Fri, Apr 23, 2021 at 6:59 PM Douglas Anderson <dianders@chromium.org> wrote:

> In commit 3235b0f20a0a ("drm/panel: panel-simple: Use runtime pm to
> avoid excessive unprepare / prepare") we started using pm_runtime, but
> my patch neglected to add the proper pm_runtime_disable(). Doh! Add
> them now.
>
> Fixes: 3235b0f20a0a ("drm/panel: panel-simple: Use runtime pm to avoid excessive unprepare / prepare")
> Reported-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

This patch as such:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Notice however: you turn on pm runtime pm_runtime_enable()
in panel_simple_probe() but are you ever turning it off in
panel_simple_remove()?

I think pm_runtime_disable(); need to be added there?

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v5 01/20] drm/panel: panel-simple: Add missing pm_runtime_disable() calls
  2021-04-30  0:58   ` Linus Walleij
@ 2021-04-30  1:24     ` Doug Anderson
  2021-04-30  1:27       ` Linus Walleij
  0 siblings, 1 reply; 36+ messages in thread
From: Doug Anderson @ 2021-04-30  1:24 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, Sam Ravnborg, Wolfram Sang, MSM, Rob Clark,
	Stanislav Lisovskiy, Stephen Boyd, Steev Klimaszewski,
	Maarten Lankhorst, linux-i2c, Bjorn Andersson,
	open list:DRM PANEL DRIVERS, Daniel Vetter, David Airlie,
	Thierry Reding, linux-kernel

Hi,

On Thu, Apr 29, 2021 at 5:58 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Fri, Apr 23, 2021 at 6:59 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> > In commit 3235b0f20a0a ("drm/panel: panel-simple: Use runtime pm to
> > avoid excessive unprepare / prepare") we started using pm_runtime, but
> > my patch neglected to add the proper pm_runtime_disable(). Doh! Add
> > them now.
> >
> > Fixes: 3235b0f20a0a ("drm/panel: panel-simple: Use runtime pm to avoid excessive unprepare / prepare")
> > Reported-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
>
> This patch as such:
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>
> Notice however: you turn on pm runtime pm_runtime_enable()
> in panel_simple_probe() but are you ever turning it off in
> panel_simple_remove()?
>
> I think pm_runtime_disable(); need to be added there?

I'm a bit confused. You're saying that I need to add
pm_runtime_disable() to panel_simple_remove()? Doesn't this patch do
that? This patch adds two calls to pm_runtime_disable(). One of those
is in the probe error path and the other one is in
panel_simple_remove().

-Doug

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v5 01/20] drm/panel: panel-simple: Add missing pm_runtime_disable() calls
  2021-04-30  1:24     ` Doug Anderson
@ 2021-04-30  1:27       ` Linus Walleij
  2021-04-30 21:04         ` Doug Anderson
  0 siblings, 1 reply; 36+ messages in thread
From: Linus Walleij @ 2021-04-30  1:27 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, Sam Ravnborg, Wolfram Sang, MSM, Rob Clark,
	Stanislav Lisovskiy, Stephen Boyd, Steev Klimaszewski,
	Maarten Lankhorst, linux-i2c, Bjorn Andersson,
	open list:DRM PANEL DRIVERS, Daniel Vetter, David Airlie,
	Thierry Reding, linux-kernel

On Fri, Apr 30, 2021 at 3:25 AM Doug Anderson <dianders@chromium.org> wrote:

> > I think pm_runtime_disable(); need to be added there?
>
> I'm a bit confused. You're saying that I need to add
> pm_runtime_disable() to panel_simple_remove()? Doesn't this patch do
> that?

It does, sorry, too late at night :D

I was looking at the previous patch and mixed up which was the
patch and the patch to the patch...

Thanks, apply this!
Linus Walleij

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v5 01/20] drm/panel: panel-simple: Add missing pm_runtime_disable() calls
  2021-04-30  1:27       ` Linus Walleij
@ 2021-04-30 21:04         ` Doug Anderson
  2021-05-01 12:07           ` Linus Walleij
  0 siblings, 1 reply; 36+ messages in thread
From: Doug Anderson @ 2021-04-30 21:04 UTC (permalink / raw)
  To: Linus Walleij, Andrzej Hajda, Laurent Pinchart, Bjorn Andersson
  Cc: Neil Armstrong, Jonas Karlman, Jernej Skrabec, Sam Ravnborg,
	Wolfram Sang, MSM, Rob Clark, Stanislav Lisovskiy, Stephen Boyd,
	Steev Klimaszewski, Maarten Lankhorst, linux-i2c,
	open list:DRM PANEL DRIVERS, Daniel Vetter, David Airlie,
	Thierry Reding, linux-kernel

Hi,

On Thu, Apr 29, 2021 at 6:28 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Fri, Apr 30, 2021 at 3:25 AM Doug Anderson <dianders@chromium.org> wrote:
>
> > > I think pm_runtime_disable(); need to be added there?
> >
> > I'm a bit confused. You're saying that I need to add
> > pm_runtime_disable() to panel_simple_remove()? Doesn't this patch do
> > that?
>
> It does, sorry, too late at night :D
>
> I was looking at the previous patch and mixed up which was the
> patch and the patch to the patch...
>
> Thanks, apply this!

Pushed this one patch. Rest of the series is pending adult
supervision. Overall summary:

1. I could probably push some of the early sn65dsi86 cleanup patches
in this series since they have Bjorn's review and are pretty much
no-ops / simple cleanups, but there's probably not tons gained for
shoving those in early.

2. The whole concept of breaking up the patch into sub-drivers has no
official Reviewed-by tags yet. Presumably Bjorn will give those a
re-review when he has time again. Assuming nobody is really upset
about it, I could land those which might unblock some of Bjorn's
future PWM work. It would probably be good to get an extra set of eyes
on them, though, just so someone else agrees that they're not "too
hacky" or anything. IMO it's actually a pretty nice solution, but I'm
biased!

3. Laurent and I had a big discussion on #dri-devel yesterday about
the EDID reading. He's not totally convinced with the idea of doing
this in the panel when the bridge could just do it by itself, but it
sounded like he might be coming around. Right now this is waiting on
Laurent to have time to get back to this.

My summary of the IRC discussion with Laurent (please correct if I got
this wrong):

a) In general I argued that it was important to be able to provide the
EDID and the DDC bus to the panel driver. Providing the EDID to the
panel driver allows the panel driver is one of the prerequisites for
my proposal for solving the "panel second sourcing" problem [1]. Being
able to provide the DDC bus to the panel will likely be important in
the eventual solution to Rajeev's problem [2].

b) Today, if we provide the DDC bus to simple-panel then simple-panel
will assume it's in charge of reading the EDID.

c) Having the panel driver involved in reading the EDID feels like it
makes sense to me. The panel driver knows how to power the panel on
enough to read the EDID. It also might know extra quirks needed to
read the EDID on a given panel. This feels a little cleaner (to me)
than just re-using the panel's "prepare" and assuming that a prepared
panel was ready for EDID read, though I can see that both may have
their advantages.

d) Laurent proposed that some eDP controllers might have special ways
to read an EDID but might not be able to provide a DDC bus or an i2c
bus. If we run into controllers like this then we would be painted
into a corner and we'd have to come up with a new solution. This is
definitely a good point, though it remains to be seen if this is
common with eDP (like Laurent says it is for HDMI). Some eDP panels
need custom DDC commands in order to be configured and so hopefully
all eDP bridges out there at least provide a DDC bus. It does feel
like this could be solved later, though. My patch series is leveraging
the existing concept that the panel driver is in charge of reading the
EDID if it's given the DDC bus, so it's not creating a new mechanism
but instead just continuing to use the existing mechanism. If the
existing mechanism doesn't work then it can be improved when there is
a need.

e) Laurent worried about circular dependencies and wanted to see my
solution to the problem before deciding if it was too big of a hack.
Hopefully it looks OK since it solves not only this problem but also
the HPD GPIO problem and will be important for when Bjorn exports the
PWM from the bridge chip.

[1] https://lore.kernel.org/lkml/CAD=FV=VZYOMPwQZzWdhJGh5cjJWw_EcM-wQVEivZ-bdGXjPrEQ@mail.gmail.com/
[2] https://lore.kernel.org/r/78c4bd291bd4a17ae2a1d02d0217de43@codeaurora.org

OK, I'll shut up now. ;-)

-Doug

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v5 09/20] drm/bridge: ti-sn65dsi86: Break GPIO and MIPI-to-eDP bridge into sub-drivers
  2021-04-23 16:58 ` [PATCH v5 09/20] drm/bridge: ti-sn65dsi86: Break GPIO and MIPI-to-eDP bridge into sub-drivers Douglas Anderson
@ 2021-05-01 11:59   ` Linus Walleij
  2021-05-03 16:55     ` Doug Anderson
  0 siblings, 1 reply; 36+ messages in thread
From: Linus Walleij @ 2021-05-01 11:59 UTC (permalink / raw)
  To: Douglas Anderson
  Cc: Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, Sam Ravnborg, Wolfram Sang, MSM, Rob Clark,
	Stanislav Lisovskiy, Stephen Boyd, Steev Klimaszewski,
	Maarten Lankhorst, linux-i2c, Bjorn Andersson,
	open list:DRM PANEL DRIVERS, Daniel Vetter, David Airlie,
	Robert Foss, linux-kernel

On Fri, Apr 23, 2021 at 6:59 PM Douglas Anderson <dianders@chromium.org> wrote:

> Let's use the newly minted aux bus to break up the driver into sub
> drivers. We're not doing a full breakup here: all the code is still in
> the same file and remains largely untouched. The big goal here of
> using sub-drivers is to allow part of our code to finish probing even
> if some other code needs to defer. This can solve some chicken-and-egg
> problems. Specifically:
> - In commit 48834e6084f1 ("drm/panel-simple: Support hpd-gpios for
>   delaying prepare()") we had to add a bit of a hack to simpel-panel
>   to support HPD showing up late. We can get rid of that hack now
>   since the GPIO part of our driver can finish probing early.
> - We have a desire to expose our DDC bus to simple-panel (and perhaps
>   to a backlight driver?). That will end up with the same
>   chicken-and-egg problem. A future patch to move this to a sub-driver
>   will fix it.
> - If/when we support the PWM functionality present in the bridge chip
>   for a backlight we'll end up with another chicken-and-egg
>   problem. If we allow the PWM to be a sub-driver too then it solves
>   this problem.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
> Changes in v5:
> - Fix module compile problems (Bjorn + kbuild bot)
> - Remove useless MODULE_DEVICE_TABLE (Bjorn).

This is generally a good idea. I have no idea when to use
auxbus or MFD but I trust that you researched that so:
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v5 01/20] drm/panel: panel-simple: Add missing pm_runtime_disable() calls
  2021-04-30 21:04         ` Doug Anderson
@ 2021-05-01 12:07           ` Linus Walleij
  2021-05-03 20:41             ` Doug Anderson
  0 siblings, 1 reply; 36+ messages in thread
From: Linus Walleij @ 2021-05-01 12:07 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Andrzej Hajda, Laurent Pinchart, Bjorn Andersson, Neil Armstrong,
	Jonas Karlman, Jernej Skrabec, Sam Ravnborg, Wolfram Sang, MSM,
	Rob Clark, Stanislav Lisovskiy, Stephen Boyd, Steev Klimaszewski,
	Maarten Lankhorst, linux-i2c, open list:DRM PANEL DRIVERS,
	Daniel Vetter, David Airlie, Thierry Reding, linux-kernel

Hi Doug,

On Fri, Apr 30, 2021 at 11:04 PM Doug Anderson <dianders@chromium.org> wrote:

> Pushed this one patch. Rest of the series is pending adult
> supervision. Overall summary:
>
> 1. I could probably push some of the early sn65dsi86 cleanup patches
> in this series since they have Bjorn's review and are pretty much
> no-ops / simple cleanups, but there's probably not tons gained for
> shoving those in early.

Those look good to me as well. I'd say just apply them.

To me it looks like up until and including patch 18?
Feel free to add my
Acked-by: Linus Walleij <linus.walleij@linaro.org>

On these.

> 2. The whole concept of breaking up the patch into sub-drivers has no
> official Reviewed-by tags yet. Presumably Bjorn will give those a
> re-review when he has time again.

It looks good to me so I sent an explicit ACK on that patch.

> 3. Laurent and I had a big discussion on #dri-devel yesterday about
> the EDID reading. He's not totally convinced with the idea of doing
> this in the panel when the bridge could just do it by itself, but it
> sounded like he might be coming around. Right now this is waiting on
> Laurent to have time to get back to this.

I dare not speak of this. Laurent has the long and tedious experience
with panels and pretty much anything related so if Laurent is
hesitant then I am hesitant too. His buy-in is absolutely required.

But IIUC that is just for patch 19+20?

It'd be good to apply the rest and get down the stack.

Just to keep you busy and make sure you don't run out of work
(haha) I noticed that the gpio_chip in this driver can use
the new GPIO_REGMAP helper library with the fixes just
landed in Torvald's tree.

At your convenience and when you think there is too little
stuff in your sn65dsi86 TODO, check out
pinctrl-bcm63xx.c for an example of select GPIO_REGMAP
made very simple (this works fine as long as they are bit
offsets starting from 0).

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v5 09/20] drm/bridge: ti-sn65dsi86: Break GPIO and MIPI-to-eDP bridge into sub-drivers
  2021-05-01 11:59   ` Linus Walleij
@ 2021-05-03 16:55     ` Doug Anderson
  0 siblings, 0 replies; 36+ messages in thread
From: Doug Anderson @ 2021-05-03 16:55 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, Sam Ravnborg, Wolfram Sang, MSM, Rob Clark,
	Stanislav Lisovskiy, Stephen Boyd, Steev Klimaszewski,
	Maarten Lankhorst, linux-i2c, Bjorn Andersson,
	open list:DRM PANEL DRIVERS, Daniel Vetter, David Airlie,
	Robert Foss, linux-kernel

Hi,

On Sat, May 1, 2021 at 4:59 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Fri, Apr 23, 2021 at 6:59 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> > Let's use the newly minted aux bus to break up the driver into sub
> > drivers. We're not doing a full breakup here: all the code is still in
> > the same file and remains largely untouched. The big goal here of
> > using sub-drivers is to allow part of our code to finish probing even
> > if some other code needs to defer. This can solve some chicken-and-egg
> > problems. Specifically:
> > - In commit 48834e6084f1 ("drm/panel-simple: Support hpd-gpios for
> >   delaying prepare()") we had to add a bit of a hack to simpel-panel
> >   to support HPD showing up late. We can get rid of that hack now
> >   since the GPIO part of our driver can finish probing early.
> > - We have a desire to expose our DDC bus to simple-panel (and perhaps
> >   to a backlight driver?). That will end up with the same
> >   chicken-and-egg problem. A future patch to move this to a sub-driver
> >   will fix it.
> > - If/when we support the PWM functionality present in the bridge chip
> >   for a backlight we'll end up with another chicken-and-egg
> >   problem. If we allow the PWM to be a sub-driver too then it solves
> >   this problem.
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> >
> > Changes in v5:
> > - Fix module compile problems (Bjorn + kbuild bot)
> > - Remove useless MODULE_DEVICE_TABLE (Bjorn).
>
> This is generally a good idea. I have no idea when to use
> auxbus or MFD

It was a bit hard for me to figure out too. I think historically this
could have been implemented by MFD but I believe that the point of
introducing the AUX bus was that MFD wasn't a great fit for things
like this. It's talked about a bit in
"Documentation/driver-api/auxiliary_bus.rst". For me the important
thing here is that we think of the bridge chip as one device, not a
collection of IP blocks glued together in one package. As some
evidence, the DT bindings don't have sub-nodes for this. There's a
single DT node that says that this one device is the bridge, is a GPIO
controller, and can provide a PWM.


> but I trust that you researched that so:
> Acked-by: Linus Walleij <linus.walleij@linaro.org>

Thanks! I'll land it then to whittle the patch stack down to just the
controversial EDID one.

-Doug

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v5 14/20] drm/bridge: ti-sn65dsi86: Promote the AUX channel to its own sub-dev
  2021-04-23 16:59 ` [PATCH v5 14/20] drm/bridge: ti-sn65dsi86: Promote the AUX channel to its own sub-dev Douglas Anderson
@ 2021-05-03 20:27   ` Doug Anderson
  0 siblings, 0 replies; 36+ messages in thread
From: Doug Anderson @ 2021-05-03 20:27 UTC (permalink / raw)
  To: Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, Sam Ravnborg, Lyude Paul, Thierry Reding,
	Linus W
  Cc: linux-arm-msm, Rob Clark, Stanislav Lisovskiy, Stephen Boyd,
	Steev Klimaszewski, Maarten Lankhorst, linux-i2c,
	Bjorn Andersson, dri-devel, Daniel Vetter, David Airlie,
	Robert Foss, LKML, Wolfram Sang

Hi,

On Fri, Apr 23, 2021 at 10:00 AM Douglas Anderson <dianders@chromium.org> wrote:
>
> We'd like to be able to expose the DDC-over-AUX channel bus to our
> panel. This gets into a chicken-and-egg problem because:
> - The panel wants to get its DDC at probe time.
> - The ti-sn65dsi86 MIPI-to-eDP bridge code, which provides the DDC
>   bus, wants to get the panel at probe time.
>
> By using a sub device we can fully create the AUX channel bits so that
> the panel can get them. Then the panel can finish probing and the
> bridge can probe.
>
> To accomplish this, we also move registering the AUX channel out of
> the bridge's attach code and do it right at probe time. We use devm to
> manage cleanup.
>
> NOTE: there's a little bit of a trick here. Though the AUX channel can
> run without the MIPI-to-eDP bits of the code, the MIPI-to-eDP bits
> can't run without the AUX channel. We could come up a complicated
> signaling scheme (have the MIPI-to-eDP bits return EPROBE_DEFER for a
> while or wait on some sort of completion), but it seems simple enough
> to just not even bother creating the bridge device until the AUX
> channel probes. That's what we'll do.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
> Changes in v5:
> - Fix module compile problems (Bjorn + kbuild bot)
> - Remove useless MODULE_DEVICE_TABLE (Bjorn).
>
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 87 +++++++++++++++++++++------
>  1 file changed, 67 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 9dc3cd8e17df..3539ddf9d109 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -116,6 +116,7 @@
>   * struct ti_sn65dsi86 - Platform data for ti-sn65dsi86 driver.
>   * @bridge_aux:   AUX-bus sub device for MIPI-to-eDP bridge functionality.
>   * @gpio_aux:     AUX-bus sub device for GPIO controller functionality.
> + * @aux_aux:      AUX-bus sub device for eDP AUX channel functionality.
>   *
>   * @dev:          Pointer to the top level (i2c) device.
>   * @regmap:       Regmap for accessing i2c.
> @@ -148,6 +149,7 @@
>  struct ti_sn65dsi86 {
>         struct auxiliary_device         bridge_aux;
>         struct auxiliary_device         gpio_aux;
> +       struct auxiliary_device         aux_aux;
>
>         struct device                   *dev;
>         struct regmap                   *regmap;
> @@ -484,18 +486,12 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
>                 return -EINVAL;
>         }
>
> -       ret = drm_dp_aux_register(&pdata->aux);
> -       if (ret < 0) {
> -               drm_err(bridge->dev, "Failed to register DP AUX channel: %d\n", ret);
> -               return ret;
> -       }
> -
>         ret = drm_connector_init(bridge->dev, &pdata->connector,
>                                  &ti_sn_bridge_connector_funcs,
>                                  DRM_MODE_CONNECTOR_eDP);
>         if (ret) {
>                 DRM_ERROR("Failed to initialize connector with drm\n");
> -               goto err_conn_init;
> +               return ret;
>         }
>
>         drm_connector_helper_add(&pdata->connector,
> @@ -552,8 +548,6 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
>         mipi_dsi_device_unregister(dsi);
>  err_dsi_host:
>         drm_connector_cleanup(&pdata->connector);
> -err_conn_init:
> -       drm_dp_aux_unregister(&pdata->aux);
>         return ret;
>  }
>
> @@ -1330,11 +1324,6 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
>         if (ret)
>                 return ret;
>
> -       pdata->aux.name = "ti-sn65dsi86-aux";
> -       pdata->aux.dev = pdata->dev;
> -       pdata->aux.transfer = ti_sn_aux_transfer;
> -       drm_dp_aux_init(&pdata->aux);
> -
>         pdata->bridge.funcs = &ti_sn_bridge_funcs;
>         pdata->bridge.of_node = np;
>
> @@ -1429,6 +1418,50 @@ static int ti_sn65dsi86_add_aux_device(struct ti_sn65dsi86 *pdata,
>         return ret;
>  }
>
> +static void ti_sn65dsi86_unregister_dp_aux(void *data)
> +{
> +       drm_dp_aux_unregister(data);
> +}
> +
> +static int ti_sn_aux_probe(struct auxiliary_device *adev,
> +                          const struct auxiliary_device_id *id)
> +{
> +       struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent);
> +       int ret;
> +
> +       pdata->aux.name = "ti-sn65dsi86-aux";
> +       pdata->aux.dev = pdata->dev;
> +       pdata->aux.transfer = ti_sn_aux_transfer;
> +       drm_dp_aux_init(&pdata->aux);
> +
> +       ret = drm_dp_aux_register(&pdata->aux);
> +       if (ret < 0) {
> +               drm_err(pdata, "Failed to register DP AUX channel: %d\n", ret);
> +               return ret;
> +       }
> +       ret = devm_add_action_or_reset(&adev->dev,
> +                                      ti_sn65dsi86_unregister_dp_aux, &pdata->aux);
> +       if (ret)
> +               return ret;
> +
> +       /*
> +        * The eDP to MIPI bridge parts don't work until the AUX channel is
> +        * setup so we don't add it in the main driver probe, we add it now.
> +        */
> +       return ti_sn65dsi86_add_aux_device(pdata, &pdata->bridge_aux, "bridge");
> +}
> +
> +static const struct auxiliary_device_id ti_sn_aux_id_table[] = {
> +       { .name = "ti_sn65dsi86.aux", },
> +       {},
> +};
> +
> +static struct auxiliary_driver ti_sn_aux_driver = {
> +       .name = "aux",
> +       .probe = ti_sn_aux_probe,
> +       .id_table = ti_sn_aux_id_table,
> +};
> +
>  static int ti_sn65dsi86_probe(struct i2c_client *client,
>                               const struct i2c_device_id *id)
>  {
> @@ -1487,10 +1520,11 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
>          * motiviation here is to solve the chicken-and-egg problem of probe
>          * ordering. The bridge wants the panel to be there when it probes.
>          * The panel wants its HPD GPIO (provided by sn65dsi86 on some boards)
> -        * when it probes. There will soon be other devices (DDC I2C bus, PWM)
> -        * that have the same problem. Having sub-devices allows the some sub
> -        * devices to finish probing even if others return -EPROBE_DEFER and
> -        * gets us around the problems.
> +        * when it probes. The panel and maybe backlight might want the DDC
> +        * bus. Soon the PWM provided by the bridge chip will have the same
> +        * problem. Having sub-devices allows the some sub devices to finish
> +        * probing even if others return -EPROBE_DEFER and gets us around the
> +        * problems.
>          */
>
>         if (IS_ENABLED(CONFIG_OF_GPIO)) {
> @@ -1499,7 +1533,13 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
>                         return ret;
>         }
>
> -       return ti_sn65dsi86_add_aux_device(pdata, &pdata->bridge_aux, "bridge");
> +       /*
> +        * NOTE: At the end of the AUX channel probe we'll add the aux device
> +        * for the bridge. This is because the bridge can't be used until the
> +        * AUX channel is there and this is a very simple solution to the
> +        * dependency problem.
> +        */
> +       return ti_sn65dsi86_add_aux_device(pdata, &pdata->aux_aux, "aux");
>  }
>
>  static struct i2c_device_id ti_sn65dsi86_id[] = {
> @@ -1536,12 +1576,18 @@ static int __init ti_sn65dsi86_init(void)
>         if (ret)
>                 goto err_main_was_registered;
>
> -       ret = auxiliary_driver_register(&ti_sn_bridge_driver);
> +       ret = auxiliary_driver_register(&ti_sn_aux_driver);
>         if (ret)
>                 goto err_gpio_was_registered;
>
> +       ret = auxiliary_driver_register(&ti_sn_bridge_driver);
> +       if (ret)
> +               goto err_aux_was_registered;
> +
>         return 0;
>
> +err_aux_was_registered:
> +       auxiliary_driver_unregister(&ti_sn_aux_driver);
>  err_gpio_was_registered:
>         ti_sn_gpio_unregister();
>  err_main_was_registered:
> @@ -1554,6 +1600,7 @@ module_init(ti_sn65dsi86_init);
>  static void __exit ti_sn65dsi86_exit(void)
>  {
>         auxiliary_driver_unregister(&ti_sn_bridge_driver);
> +       auxiliary_driver_unregister(&ti_sn_aux_driver);
>         ti_sn_gpio_unregister();
>         i2c_del_driver(&ti_sn65dsi86_driver);
>  }

Ugh, more fun! :(

I tried rebasing this to the latest drm-misc-next and I found commit
6cba3fe43341 ("drm/dp: Add backpointer to drm_device in drm_dp_aux").
That commit makes it pretty explicit that we shouldn't call
drm_dp_aux_register() until we actually have a "drm_dev" for the
bridge.

I'm applying several of the other patches in this series but I won't
apply this one or anything based on it. I'll do some digging and send
out a proposed fix shortly.

-Doug

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v5 01/20] drm/panel: panel-simple: Add missing pm_runtime_disable() calls
  2021-05-01 12:07           ` Linus Walleij
@ 2021-05-03 20:41             ` Doug Anderson
  2021-05-05 12:51               ` Linus Walleij
  0 siblings, 1 reply; 36+ messages in thread
From: Doug Anderson @ 2021-05-03 20:41 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Andrzej Hajda, Laurent Pinchart, Bjorn Andersson, Neil Armstrong,
	Jonas Karlman, Jernej Skrabec, Sam Ravnborg, Wolfram Sang, MSM,
	Rob Clark, Stanislav Lisovskiy, Stephen Boyd, Steev Klimaszewski,
	Maarten Lankhorst, linux-i2c, open list:DRM PANEL DRIVERS,
	Daniel Vetter, David Airlie, Thierry Reding, linux-kernel

Hi,

On Sat, May 1, 2021 at 5:07 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> Hi Doug,
>
> On Fri, Apr 30, 2021 at 11:04 PM Doug Anderson <dianders@chromium.org> wrote:
>
> > Pushed this one patch. Rest of the series is pending adult
> > supervision. Overall summary:
> >
> > 1. I could probably push some of the early sn65dsi86 cleanup patches
> > in this series since they have Bjorn's review and are pretty much
> > no-ops / simple cleanups, but there's probably not tons gained for
> > shoving those in early.
>
> Those look good to me as well. I'd say just apply them.
>
> To me it looks like up until and including patch 18?
> Feel free to add my
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
>
> On these.

OK, thanks! I've just pushed these patches to drm-misc-next with your Ack:

63358e24ee79 drm/panel: panel-simple: Cache the EDID as long as we retain power
31e25395d8b7 drm/panel: panel-simple: Power the panel when reading the EDID
4318ea406e02 drm/panel: panel-simple: Remove extra call:
drm_connector_update_edid_property()
b137406d9679 drm/bridge: ti-sn65dsi86: If refclk, DP AUX can happen
w/out pre-enable
f7a5ee2cd3e2 drm/bridge: ti-sn65dsi86: Code motion of refclk
management functions
9bede63127c6 drm/bridge: ti-sn65dsi86: Use pm_runtime autosuspend
5c4381eeb709 drm/panel: panel-simple: Get rid of hacky HPD chicken-and-egg code
bf73537f411b drm/bridge: ti-sn65dsi86: Break GPIO and MIPI-to-eDP
bridge into sub-drivers
bef236a5206c drm/bridge: ti-sn65dsi86: Move all the chip-related init
to the start
f94eb8a32863 drm/bridge: ti-sn65dsi86: Cleanup managing of drvdata
3636fc25f760 drm/bridge: ti-sn65dsi86: Add local var for "dev" to simplify probe
52d54819c8ae drm/bridge: ti-sn65dsi86: Clean debugfs code
dea2500a820c drm/bridge: ti-sn65dsi86: Use devm to do our runtime_disable
905d66d08d0f drm/bridge: ti-sn65dsi86: More renames in prep for sub-devices
db0036db4851 drm/bridge: ti-sn65dsi86: Rename the main driver data structure

Things not pushed:

[v5,15/20] i2c: i2c-core-of: Fix corner case of finding adapter by node
-> Can't push i2c things

[v5,14/20] drm/bridge: ti-sn65dsi86: Promote the AUX channel to its own sub-dev
-> Won't work without rework. See [1]

[v5,19/20] drm/bridge: ti-sn65dsi86: Don't read EDID blob over DDC
-> Needs Laurent and also patch 14/20 to be resolved.

[v5,20/20] arm64: dts: qcom: Link the panel to the bridge's DDC bus
-> Needs all the rest resolved.

Let me see if I can find a way to work around the AUX channel stuff
and then I'll push a v6 of just what's left.

> Just to keep you busy and make sure you don't run out of work
> (haha) I noticed that the gpio_chip in this driver can use
> the new GPIO_REGMAP helper library with the fixes just
> landed in Torvald's tree.
>
> At your convenience and when you think there is too little
> stuff in your sn65dsi86 TODO, check out
> pinctrl-bcm63xx.c for an example of select GPIO_REGMAP
> made very simple (this works fine as long as they are bit
> offsets starting from 0).

I seem to recall you mentioning something like this. When I looked at
it in the past I wasn't convinced it would be easy. See my response
[2]. The rough summary is that I didn't think the helpers were happy
with the pm_runtime() model that I'm using. Did I get that wrong?

[1] https://lore.kernel.org/dri-devel/CAD=FV=UTmOP8LDaf-Tyx17OORQK6pJH6O_w3cP0Bu-KRYaHkYw@mail.gmail.com/
[2] https://lore.kernel.org/r/CAD=FV=VqD-dY=v23KYuTqy8aRNQJJzJ7h_UOcdEBYuK9X51MQQ@mail.gmail.com

-Doug

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v5 01/20] drm/panel: panel-simple: Add missing pm_runtime_disable() calls
  2021-05-03 20:41             ` Doug Anderson
@ 2021-05-05 12:51               ` Linus Walleij
  0 siblings, 0 replies; 36+ messages in thread
From: Linus Walleij @ 2021-05-05 12:51 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Andrzej Hajda, Laurent Pinchart, Bjorn Andersson, Neil Armstrong,
	Jonas Karlman, Jernej Skrabec, Sam Ravnborg, Wolfram Sang, MSM,
	Rob Clark, Stanislav Lisovskiy, Stephen Boyd, Steev Klimaszewski,
	Maarten Lankhorst, linux-i2c, open list:DRM PANEL DRIVERS,
	Daniel Vetter, David Airlie, Thierry Reding, linux-kernel

On Mon, May 3, 2021 at 10:41 PM Doug Anderson <dianders@chromium.org> wrote:

> > At your convenience and when you think there is too little
> > stuff in your sn65dsi86 TODO, check out
> > pinctrl-bcm63xx.c for an example of select GPIO_REGMAP
> > made very simple (this works fine as long as they are bit
> > offsets starting from 0).
>
> I seem to recall you mentioning something like this. When I looked at
> it in the past I wasn't convinced it would be easy. See my response
> [2]. The rough summary is that I didn't think the helpers were happy
> with the pm_runtime() model that I'm using. Did I get that wrong?

Yeah good point. It does seem a bit too complex for that.
Sorry for not remembering.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 36+ messages in thread

end of thread, other threads:[~2021-05-05 12:51 UTC | newest]

Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-23 16:58 [PATCH v5 00/20] drm: Fix EDID reading on ti-sn65dsi86; solve some chicken-and-egg problems Douglas Anderson
2021-04-23 16:58 ` [PATCH v5 01/20] drm/panel: panel-simple: Add missing pm_runtime_disable() calls Douglas Anderson
2021-04-28 16:32   ` Sean Paul
2021-04-30  0:58   ` Linus Walleij
2021-04-30  1:24     ` Doug Anderson
2021-04-30  1:27       ` Linus Walleij
2021-04-30 21:04         ` Doug Anderson
2021-05-01 12:07           ` Linus Walleij
2021-05-03 20:41             ` Doug Anderson
2021-05-05 12:51               ` Linus Walleij
2021-04-23 16:58 ` [PATCH v5 02/20] drm/bridge: ti-sn65dsi86: Rename the main driver data structure Douglas Anderson
2021-04-23 16:58 ` [PATCH v5 03/20] drm/bridge: ti-sn65dsi86: More renames in prep for sub-devices Douglas Anderson
2021-04-23 16:58 ` [PATCH v5 04/20] drm/bridge: ti-sn65dsi86: Use devm to do our runtime_disable Douglas Anderson
2021-04-23 16:58 ` [PATCH v5 05/20] drm/bridge: ti-sn65dsi86: Clean debugfs code Douglas Anderson
2021-04-23 16:58 ` [PATCH v5 06/20] drm/bridge: ti-sn65dsi86: Add local var for "dev" to simplify probe Douglas Anderson
2021-04-23 16:58 ` [PATCH v5 07/20] drm/bridge: ti-sn65dsi86: Cleanup managing of drvdata Douglas Anderson
2021-04-23 16:58 ` [PATCH v5 08/20] drm/bridge: ti-sn65dsi86: Move all the chip-related init to the start Douglas Anderson
2021-04-23 16:58 ` [PATCH v5 09/20] drm/bridge: ti-sn65dsi86: Break GPIO and MIPI-to-eDP bridge into sub-drivers Douglas Anderson
2021-05-01 11:59   ` Linus Walleij
2021-05-03 16:55     ` Doug Anderson
2021-04-23 16:58 ` [PATCH v5 10/20] drm/panel: panel-simple: Get rid of hacky HPD chicken-and-egg code Douglas Anderson
2021-04-28 16:35   ` Sean Paul
2021-04-23 16:58 ` [PATCH v5 11/20] drm/bridge: ti-sn65dsi86: Use pm_runtime autosuspend Douglas Anderson
2021-04-23 16:58 ` [PATCH v5 12/20] drm/bridge: ti-sn65dsi86: Code motion of refclk management functions Douglas Anderson
2021-04-23 16:58 ` [PATCH v5 13/20] drm/bridge: ti-sn65dsi86: If refclk, DP AUX can happen w/out pre-enable Douglas Anderson
2021-04-23 16:59 ` [PATCH v5 14/20] drm/bridge: ti-sn65dsi86: Promote the AUX channel to its own sub-dev Douglas Anderson
2021-05-03 20:27   ` Doug Anderson
2021-04-23 16:59 ` [PATCH v5 15/20] i2c: i2c-core-of: Fix corner case of finding adapter by node Douglas Anderson
2021-04-23 16:59 ` [PATCH v5 16/20] drm/panel: panel-simple: Remove extra call: drm_connector_update_edid_property() Douglas Anderson
2021-04-28 16:38   ` Sean Paul
2021-04-23 16:59 ` [PATCH v5 17/20] drm/panel: panel-simple: Power the panel when reading the EDID Douglas Anderson
2021-04-28 16:44   ` Sean Paul
2021-04-23 16:59 ` [PATCH v5 18/20] drm/panel: panel-simple: Cache the EDID as long as we retain power Douglas Anderson
2021-04-28 16:50   ` Sean Paul
2021-04-23 16:59 ` [PATCH v5 19/20] drm/bridge: ti-sn65dsi86: Don't read EDID blob over DDC Douglas Anderson
2021-04-23 16:59 ` [PATCH v5 20/20] arm64: dts: qcom: Link the panel to the bridge's DDC bus Douglas Anderson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).