[PATCH v4 00/27] drm: Fix EDID reading on ti-sn65dsi86; solve some chicken-and-egg problems
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) 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.
I apologize in advance for the length of this series. I'm currently working through getting commit access to drm-misc [3] so I can land the first several patches which are already reviewed. There are still a lot of patches even after the first few, but hopefully you can see that there are only so many because they're broken up into nice and reviewable bite-sized-chunks. :-)
[1] https://lore.kernel.org/r/20210304155144.3.I60a7fb23ce4589006bc95c64ab8d15c7... [2] https://lore.kernel.org/r/161706912161.3012082.17313817257247946143@swboyd.m... [3] https://gitlab.freedesktop.org/freedesktop/freedesktop/-/issues/348
Changes in v4: - Reword commit mesage slightly.
Changes in v3: - Removed "NOTES" from commit message.
Changes in v2: - Removed 2nd paragraph in commit message.
Douglas Anderson (27): drm/bridge: Fix the stop condition of drm_bridge_chain_pre_enable() drm/bridge: ti-sn65dsi86: Simplify refclk handling drm/bridge: ti-sn65dsi86: Remove incorrectly tagged kerneldoc comment drm/bridge: ti-sn65dsi86: Reorder remove() drm/bridge: ti-sn65dsi86: Move drm_panel_unprepare() to post_disable() drm/bridge: ti-sn65dsi86: Get rid of the useless detect() function drm/panel: panel-simple: Use runtime pm to avoid excessive unprepare / prepare 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: 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: Use devm to do our runtime_disable 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 drm/panel: panel-simple: Prepare/unprepare are refcounted, not forced
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 1 + drivers/gpu/drm/bridge/Kconfig | 1 + drivers/gpu/drm/bridge/ti-sn65dsi86.c | 748 ++++++++++++------- drivers/gpu/drm/drm_bridge.c | 3 + drivers/gpu/drm/panel/Kconfig | 1 + drivers/gpu/drm/panel/panel-simple.c | 123 +-- drivers/i2c/i2c-core-of.c | 17 +- 7 files changed, 555 insertions(+), 339 deletions(-)
The drm_bridge_chain_pre_enable() is not the proper opposite of drm_bridge_chain_post_disable(). It continues along the chain to _before_ the starting bridge. Let's fix that.
Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list") Signed-off-by: Douglas Anderson dianders@chromium.org Reviewed-by: Andrzej Hajda a.hajda@samsung.com Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com ---
(no changes since v1)
drivers/gpu/drm/drm_bridge.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index 64f0effb52ac..044acd07c153 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -522,6 +522,9 @@ void drm_bridge_chain_pre_enable(struct drm_bridge *bridge) list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) { if (iter->funcs->pre_enable) iter->funcs->pre_enable(iter); + + if (iter == bridge) + break; } } EXPORT_SYMBOL(drm_bridge_chain_pre_enable);
The clock framework makes it simple to deal with an optional clock. You can call clk_get_optional() and if the clock isn't specified it'll just return NULL without complaint. It's valid to pass NULL to enable/disable/prepare/unprepare. Let's make use of this to simplify things a tiny bit.
Signed-off-by: Douglas Anderson dianders@chromium.org Reviewed-by: Robert Foss robert.foss@linaro.org Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org Reviewed-by: Stephen Boyd swboyd@chromium.org Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Reviewed-by: Andrzej Hajda a.hajda@samsung.com ---
(no changes since v2)
Changes in v2: - Removed 2nd paragraph in commit message.
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 88df4dd0f39d..96fe8f2c0ea9 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -1275,14 +1275,9 @@ static int ti_sn_bridge_probe(struct i2c_client *client, return ret; }
- pdata->refclk = devm_clk_get(pdata->dev, "refclk"); - if (IS_ERR(pdata->refclk)) { - ret = PTR_ERR(pdata->refclk); - if (ret == -EPROBE_DEFER) - return ret; - DRM_DEBUG_KMS("refclk not found\n"); - pdata->refclk = NULL; - } + pdata->refclk = devm_clk_get_optional(pdata->dev, "refclk"); + if (IS_ERR(pdata->refclk)) + return PTR_ERR(pdata->refclk);
ret = ti_sn_bridge_parse_dsi_host(pdata); if (ret)
A random comment inside a function had "/**" in front of it. That doesn't make sense. Remove.
Signed-off-by: Douglas Anderson dianders@chromium.org Reviewed-by: Andrzej Hajda a.hajda@samsung.com Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com ---
(no changes since v1)
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 96fe8f2c0ea9..76f43af6735d 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -788,7 +788,7 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge) /* set dsi clk frequency value */ ti_sn_bridge_set_dsi_rate(pdata);
- /** + /* * The SN65DSI86 only supports ASSR Display Authentication method and * this method is enabled by default. An eDP panel must support this * authentication method. We need to enable this method in the eDP panel
Let's make the remove() function strictly the reverse of the probe() function so it's easier to reason about.
This patch was created by code inspection and should move us closer to a proper remove.
Signed-off-by: Douglas Anderson dianders@chromium.org Reviewed-by: Andrzej Hajda a.hajda@samsung.com Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com ---
(no changes since v3)
Changes in v3: - Removed "NOTES" from commit message.
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 76f43af6735d..c006678c9921 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -1315,20 +1315,21 @@ static int ti_sn_bridge_remove(struct i2c_client *client) if (!pdata) return -EINVAL;
- kfree(pdata->edid); - ti_sn_debugfs_remove(pdata); - - of_node_put(pdata->host_node); - - pm_runtime_disable(pdata->dev); - if (pdata->dsi) { mipi_dsi_detach(pdata->dsi); mipi_dsi_device_unregister(pdata->dsi); }
+ kfree(pdata->edid); + + ti_sn_debugfs_remove(pdata); + drm_bridge_remove(&pdata->bridge);
+ pm_runtime_disable(pdata->dev); + + of_node_put(pdata->host_node); + return 0; }
We prepared the panel in pre_enable() so we should unprepare it in post_disable() to match.
Signed-off-by: Douglas Anderson dianders@chromium.org Reviewed-by: Andrzej Hajda a.hajda@samsung.com Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com ---
Changes in v4: - Reword commit mesage slightly.
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index c006678c9921..e30460002c48 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -452,8 +452,6 @@ static void ti_sn_bridge_disable(struct drm_bridge *bridge) regmap_write(pdata->regmap, SN_ML_TX_MODE_REG, 0); /* disable DP PLL */ regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 0); - - drm_panel_unprepare(pdata->panel); }
static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn_bridge *pdata) @@ -869,6 +867,8 @@ static void ti_sn_bridge_post_disable(struct drm_bridge *bridge) { struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+ drm_panel_unprepare(pdata->panel); + clk_disable_unprepare(pdata->refclk);
pm_runtime_put_sync(pdata->dev);
If we just leave the detect() function as NULL then the upper layers assume we're always connected. There's no reason for a stub.
Signed-off-by: Douglas Anderson dianders@chromium.org Reviewed-by: Andrzej Hajda a.hajda@samsung.com Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com ---
(no changes since v1)
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 12 ------------ 1 file changed, 12 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index e30460002c48..51db30d573c1 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -306,20 +306,8 @@ static struct drm_connector_helper_funcs ti_sn_bridge_connector_helper_funcs = { .mode_valid = ti_sn_bridge_connector_mode_valid, };
-static enum drm_connector_status -ti_sn_bridge_connector_detect(struct drm_connector *connector, bool force) -{ - /** - * TODO: Currently if drm_panel is present, then always - * return the status as connected. Need to add support to detect - * device state for hot pluggable scenarios. - */ - return connector_status_connected; -} - static const struct drm_connector_funcs ti_sn_bridge_connector_funcs = { .fill_modes = drm_helper_probe_single_connector_modes, - .detect = ti_sn_bridge_connector_detect, .destroy = drm_connector_cleanup, .reset = drm_atomic_helper_connector_reset, .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
Unpreparing and re-preparing a panel can be a really heavy operation. Panels datasheets often specify something on the order of 500ms as the delay you should insert after turning off the panel before turning it on again. In addition, turning on a panel can have delays on the order of 100ms - 200ms before the panel will assert HPD (AKA "panel ready"). The above means that we should avoid turning a panel off if we're going to turn it on again shortly.
The above becomes a problem when we want to read the EDID of a panel. The way that ordering works is that userspace wants to read the EDID of the panel _before_ fully enabling it so that it can set the initial mode correctly. However, we can't read the EDID until we power it up. This leads to code that does this dance (like ps8640_bridge_get_edid()):
1. When userspace requests EDID / the panel modes (through an ioctl), we power on the panel just enough to read the EDID and then power it off. 2. Userspace then turns the panel on.
There's likely not much time between step #1 and #2 and so we want to avoid powering the panel off and on again between those two steps.
Let's use Runtime PM to help us. We'll move the existing prepare() and unprepare() to be runtime resume() and runtime suspend(). Now when we want to prepare() or unprepare() we just increment or decrement the refcount. We'll default to a 1 second autosuspend delay which seems sane given the typical delays we see for panels.
A few notes: - It seems the existing unprepare() and prepare() are defined to be no-ops if called extra times. We'll preserve that behavior but may try to remove it in a future patch. - This is a slight change in the ABI of simple panel. If something was absolutely relying on the unprepare() to happen instantly that simply won't be the case anymore. I'm not aware of anyone relying on that behavior, but if there is someone then we'll need to figure out how to enable (or disable) this new delayed behavior selectively. - In order for this to work we now have a hard dependency on "PM". From memory this is a legit thing to assume these days and we don't have to find some fallback to keep working if someone wants to build their system without "PM".
Signed-off-by: Douglas Anderson dianders@chromium.org Link: https://lore.kernel.org/r/20210402152701.v3.12.I9e8bd33b49c496745bfac58ea9ab... Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- Laurent pointed out that perhaps we don't need to make unprepare() and prepare() no-ops if called extra times [1]. Since I worry that will break someone out there, I have left it as a separate change at the end of this series. See ("drm/panel: panel-simple: Prepare/unprepare are refcounted, not forced")
[1] https://lore.kernel.org/r/YHePsQgqOau1V5lD@pendragon.ideasonboard.com/
(no changes since v1)
drivers/gpu/drm/panel/Kconfig | 1 + drivers/gpu/drm/panel/panel-simple.c | 93 +++++++++++++++++++++------- 2 files changed, 73 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig index 4894913936e9..ef87d92cdf49 100644 --- a/drivers/gpu/drm/panel/Kconfig +++ b/drivers/gpu/drm/panel/Kconfig @@ -80,6 +80,7 @@ config DRM_PANEL_SIMPLE tristate "support for simple panels" depends on OF depends on BACKLIGHT_CLASS_DEVICE + depends on PM select VIDEOMODE_HELPERS help DRM panel driver for dumb panels that need at most a regulator and diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index be312b5c04dd..6b22872b3281 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -27,6 +27,7 @@ #include <linux/module.h> #include <linux/of_platform.h> #include <linux/platform_device.h> +#include <linux/pm_runtime.h> #include <linux/regulator/consumer.h>
#include <video/display_timing.h> @@ -175,6 +176,8 @@ struct panel_simple { bool enabled; bool no_hpd;
+ bool prepared; + ktime_t prepared_time; ktime_t unprepared_time;
@@ -334,19 +337,31 @@ static int panel_simple_disable(struct drm_panel *panel) return 0; }
+static int panel_simple_suspend(struct device *dev) +{ + struct panel_simple *p = dev_get_drvdata(dev); + + gpiod_set_value_cansleep(p->enable_gpio, 0); + regulator_disable(p->supply); + p->unprepared_time = ktime_get(); + + return 0; +} + static int panel_simple_unprepare(struct drm_panel *panel) { struct panel_simple *p = to_panel_simple(panel); + int ret;
- if (p->prepared_time == 0) + /* Unpreparing when already unprepared is a no-op */ + if (!p->prepared) return 0;
- gpiod_set_value_cansleep(p->enable_gpio, 0); - - regulator_disable(p->supply); - - p->prepared_time = 0; - p->unprepared_time = ktime_get(); + pm_runtime_mark_last_busy(panel->dev); + ret = pm_runtime_put_autosuspend(panel->dev); + if (ret < 0) + return ret; + p->prepared = false;
return 0; } @@ -376,22 +391,19 @@ static int panel_simple_get_hpd_gpio(struct device *dev, return 0; }
-static int panel_simple_prepare_once(struct drm_panel *panel) +static int panel_simple_prepare_once(struct panel_simple *p) { - struct panel_simple *p = to_panel_simple(panel); + struct device *dev = p->base.dev; unsigned int delay; int err; int hpd_asserted; unsigned long hpd_wait_us;
- if (p->prepared_time != 0) - return 0; - panel_simple_wait(p->unprepared_time, p->desc->delay.unprepare);
err = regulator_enable(p->supply); if (err < 0) { - dev_err(panel->dev, "failed to enable supply: %d\n", err); + dev_err(dev, "failed to enable supply: %d\n", err); return err; }
@@ -405,7 +417,7 @@ static int panel_simple_prepare_once(struct drm_panel *panel)
if (p->hpd_gpio) { if (IS_ERR(p->hpd_gpio)) { - err = panel_simple_get_hpd_gpio(panel->dev, p, false); + err = panel_simple_get_hpd_gpio(dev, p, false); if (err) goto error; } @@ -423,7 +435,7 @@ static int panel_simple_prepare_once(struct drm_panel *panel)
if (err) { if (err != -ETIMEDOUT) - dev_err(panel->dev, + dev_err(dev, "error waiting for hpd GPIO: %d\n", err); goto error; } @@ -447,25 +459,46 @@ static int panel_simple_prepare_once(struct drm_panel *panel) */ #define MAX_PANEL_PREPARE_TRIES 5
-static int panel_simple_prepare(struct drm_panel *panel) +static int panel_simple_resume(struct device *dev) { + struct panel_simple *p = dev_get_drvdata(dev); int ret; int try;
for (try = 0; try < MAX_PANEL_PREPARE_TRIES; try++) { - ret = panel_simple_prepare_once(panel); + ret = panel_simple_prepare_once(p); if (ret != -ETIMEDOUT) break; }
if (ret == -ETIMEDOUT) - dev_err(panel->dev, "Prepare timeout after %d tries\n", try); + dev_err(dev, "Prepare timeout after %d tries\n", try); else if (try) - dev_warn(panel->dev, "Prepare needed %d retries\n", try); + dev_warn(dev, "Prepare needed %d retries\n", try);
return ret; }
+static int panel_simple_prepare(struct drm_panel *panel) +{ + struct panel_simple *p = to_panel_simple(panel); + int ret; + + /* Preparing when already prepared is a no-op */ + if (p->prepared) + return 0; + + ret = pm_runtime_get_sync(panel->dev); + if (ret < 0) { + pm_runtime_put_autosuspend(panel->dev); + return ret; + } + + p->prepared = true; + + return 0; +} + static int panel_simple_enable(struct drm_panel *panel) { struct panel_simple *p = to_panel_simple(panel); @@ -748,6 +781,18 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) break; }
+ dev_set_drvdata(dev, panel); + + /* + * We use runtime PM for prepare / unprepare since those power the panel + * on and off and those can be very slow operations. This is important + * to optimize powering the panel on briefly to read the EDID before + * fully enabling the panel. + */ + pm_runtime_enable(dev); + pm_runtime_set_autosuspend_delay(dev, 1000); + pm_runtime_use_autosuspend(dev); + drm_panel_init(&panel->base, dev, &panel_simple_funcs, connector_type);
err = drm_panel_of_backlight(&panel->base); @@ -756,8 +801,6 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
drm_panel_add(&panel->base);
- dev_set_drvdata(dev, panel); - return 0;
free_ddc: @@ -4603,10 +4646,17 @@ static void panel_simple_platform_shutdown(struct platform_device *pdev) panel_simple_shutdown(&pdev->dev); }
+static const struct dev_pm_ops panel_simple_pm_ops = { + SET_RUNTIME_PM_OPS(panel_simple_suspend, panel_simple_resume, NULL) + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, + pm_runtime_force_resume) +}; + static struct platform_driver panel_simple_platform_driver = { .driver = { .name = "panel-simple", .of_match_table = platform_of_match, + .pm = &panel_simple_pm_ops, }, .probe = panel_simple_platform_probe, .remove = panel_simple_platform_remove, @@ -4901,6 +4951,7 @@ static struct mipi_dsi_driver panel_simple_dsi_driver = { .driver = { .name = "panel-simple-dsi", .of_match_table = dsi_of_match, + .pm = &panel_simple_pm_ops, }, .probe = panel_simple_dsi_probe, .remove = panel_simple_dsi_remove,
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 ---
(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;
On Fri 16 Apr 17:39 CDT 2021, Douglas Anderson wrote:
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.
Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org
Regards, Bjorn
Signed-off-by: Douglas Anderson dianders@chromium.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 oneach 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.368.gbe11c130af-goog
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 ---
(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");
On Fri 16 Apr 17:39 CDT 2021, Douglas Anderson wrote:
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".
Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org
Regards, Bjorn
Signed-off-by: Douglas Anderson dianders@chromium.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);@@ -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.368.gbe11c130af-goog
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 ---
(no changes since v1)
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 32 +++++++++++++++++---------- 1 file changed, 20 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 57574132e200..0c6aa99ddc99 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,30 @@ 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); + if (IS_ERR(debugfs)) + ret = PTR_ERR(debugfs); + else + ret = devm_add_action_or_reset(dev, ti_sn65dsi86_debugfs_remove, + debugfs); + + if (ret) { + dev_warn(dev, "Failed to create debugfs (%d), skipping\n", ret); + return; + } + + debugfs_create_file("status", 0600, debugfs, pdata, &status_fops); }
/* Connector funcs */ @@ -1310,8 +1320,6 @@ static int ti_sn65dsi86_remove(struct i2c_client *client)
kfree(pdata->edid);
- ti_sn65dsi86_debugfs_remove(pdata); - drm_bridge_remove(&pdata->bridge);
pm_runtime_disable(pdata->dev);
On Fri 16 Apr 17:39 CDT 2021, Douglas Anderson wrote:
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
(no changes since v1)
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 32 +++++++++++++++++---------- 1 file changed, 20 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 57574132e200..0c6aa99ddc99 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,30 @@ 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);
- if (IS_ERR(debugfs))
ret = PTR_ERR(debugfs);- else
ret = devm_add_action_or_reset(dev, ti_sn65dsi86_debugfs_remove,debugfs);- if (ret) {
You're not supposed to handle errors from debugfs_create_dir(), but I like what you're doing with devm here and that needs a check.
Also worth mentioning is that at this point in the patch stack the debugfs "status" file will outlive the activation of pm_runtime, this is however taken care of in a later patch. Given that it's unlikely to cause a problem I don't mind this transient issue - but wanted to mention that I reviewed the end result in this regard.
Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org
Regards, Bjorn
dev_warn(dev, "Failed to create debugfs (%d), skipping\n", ret);return;- }
- debugfs_create_file("status", 0600, debugfs, pdata, &status_fops);
}
/* Connector funcs */ @@ -1310,8 +1320,6 @@ static int ti_sn65dsi86_remove(struct i2c_client *client)
kfree(pdata->edid);
ti_sn65dsi86_debugfs_remove(pdata);
drm_bridge_remove(&pdata->bridge);
pm_runtime_disable(pdata->dev);
-- 2.31.1.368.gbe11c130af-goog
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 ---
(no changes since v1)
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 0c6aa99ddc99..2cbf619fbd27 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -1226,6 +1226,7 @@ static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata, 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;
@@ -1234,8 +1235,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;
@@ -1246,26 +1246,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) { @@ -1273,7 +1271,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);
@@ -1281,23 +1279,23 @@ static int ti_sn65dsi86_probe(struct i2c_client *client, if (ret) return ret;
- pm_runtime_enable(pdata->dev); + pm_runtime_enable(dev);
ret = ti_sn_setup_gpio_controller(pdata); if (ret) { - pm_runtime_disable(pdata->dev); + pm_runtime_disable(dev); return ret; }
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);
On Fri 16 Apr 17:39 CDT 2021, Douglas Anderson wrote:
Tiny cleanup for probe so we don't keep having to specify "&client->dev" or "pdata->dev". No functional changes intended.
Nice
Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org
Regards, Bjorn
Signed-off-by: Douglas Anderson dianders@chromium.org
(no changes since v1)
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 0c6aa99ddc99..2cbf619fbd27 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -1226,6 +1226,7 @@ static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata, 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;
@@ -1234,8 +1235,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;
@@ -1246,26 +1246,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) {
@@ -1273,7 +1271,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);
@@ -1281,23 +1279,23 @@ static int ti_sn65dsi86_probe(struct i2c_client *client, if (ret) return ret;
- pm_runtime_enable(pdata->dev);
pm_runtime_enable(dev);
ret = ti_sn_setup_gpio_controller(pdata); if (ret) {
pm_runtime_disable(pdata->dev);
pm_runtime_disable(dev);return ret; }
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.368.gbe11c130af-goog
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 ---
(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 2cbf619fbd27..a200e88fd006 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -1238,6 +1238,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); @@ -1246,16 +1248,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"); @@ -1287,8 +1285,6 @@ static int ti_sn65dsi86_probe(struct i2c_client *client, return ret; }
- i2c_set_clientdata(client, pdata); - pdata->aux.name = "ti-sn65dsi86-aux"; pdata->aux.dev = dev; pdata->aux.transfer = ti_sn_aux_transfer;
On Fri 16 Apr 17:39 CDT 2021, Douglas Anderson wrote:
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.
Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org
Regards, Bjorn
Signed-off-by: Douglas Anderson dianders@chromium.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 2cbf619fbd27..a200e88fd006 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -1238,6 +1238,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);
@@ -1246,16 +1248,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");
@@ -1287,8 +1285,6 @@ static int ti_sn65dsi86_probe(struct i2c_client *client, 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.368.gbe11c130af-goog
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 ---
(no changes since v1)
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 a200e88fd006..53f1f7b3022f 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -1223,6 +1223,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) { @@ -1278,12 +1283,13 @@ static int ti_sn65dsi86_probe(struct i2c_client *client, return ret;
pm_runtime_enable(dev); + ret = devm_add_action_or_reset(dev, ti_sn65dsi86_runtime_disable, dev); + if (ret) + return ret;
ret = ti_sn_setup_gpio_controller(pdata); - if (ret) { - pm_runtime_disable(dev); + if (ret) return ret; - }
pdata->aux.name = "ti-sn65dsi86-aux"; pdata->aux.dev = dev; @@ -1316,8 +1322,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;
On Fri 16 Apr 17:39 CDT 2021, Douglas Anderson wrote:
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.
Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org
Regards, Bjorn
Signed-off-by: Douglas Anderson dianders@chromium.org
(no changes since v1)
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 a200e88fd006..53f1f7b3022f 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -1223,6 +1223,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) { @@ -1278,12 +1283,13 @@ static int ti_sn65dsi86_probe(struct i2c_client *client, return ret;
pm_runtime_enable(dev);
ret = devm_add_action_or_reset(dev, ti_sn65dsi86_runtime_disable, dev);
if (ret)
return ret;ret = ti_sn_setup_gpio_controller(pdata);
- if (ret) {
pm_runtime_disable(dev);
- if (ret) return ret;
}
pdata->aux.name = "ti-sn65dsi86-aux"; pdata->aux.dev = dev;
@@ -1316,8 +1322,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.368.gbe11c130af-goog
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 ---
(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 53f1f7b3022f..57bc489a0412 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -1253,12 +1253,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"); @@ -1266,8 +1260,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"); @@ -1278,12 +1270,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;
@@ -1301,8 +1303,6 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
drm_bridge_add(&pdata->bridge);
- ti_sn65dsi86_debugfs_init(pdata); - return 0; }
On Fri 16 Apr 17:39 CDT 2021, Douglas Anderson wrote:
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.
Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org
Regards, Bjorn
Signed-off-by: Douglas Anderson dianders@chromium.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 53f1f7b3022f..57bc489a0412 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -1253,12 +1253,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");
@@ -1266,8 +1260,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");
@@ -1278,12 +1270,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;
@@ -1301,8 +1303,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.368.gbe11c130af-goog
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 ---
(no changes since v1)
drivers/gpu/drm/bridge/Kconfig | 1 + drivers/gpu/drm/bridge/ti-sn65dsi86.c | 211 +++++++++++++++++++------- 2 files changed, 158 insertions(+), 54 deletions(-)
diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig index 22a467abd3e9..6868050961a2 100644 --- a/drivers/gpu/drm/bridge/Kconfig +++ b/drivers/gpu/drm/bridge/Kconfig @@ -267,6 +267,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 57bc489a0412..44edcf6f5744 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; @@ -1135,8 +1142,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 */ @@ -1158,19 +1167,27 @@ 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", }, + {}, +}; + +MODULE_DEVICE_TABLE(auxiliary, ti_sn_gpio_id_table);
-static inline int ti_sn_setup_gpio_controller(struct ti_sn65dsi86 *pdata) -{ - return 0; -} +static struct auxiliary_driver ti_sn_gpio_driver = { + .name = "gpio", + .probe = ti_sn_gpio_probe, + .id_table = ti_sn_gpio_id_table, +}; + +module_auxiliary_driver(ti_sn_gpio_driver);
#endif
@@ -1223,11 +1240,128 @@ 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", }, + {}, +}; + +MODULE_DEVICE_TABLE(auxiliary, ti_sn_bridge_id_table); + +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, +}; + +module_auxiliary_driver(ti_sn_bridge_driver); + 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) { @@ -1277,54 +1411,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[] = { @@ -1346,7 +1450,6 @@ 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);
Hi Douglas,
I love your patch! Yet something to improve:
[auto build test ERROR on next-20210416] [cannot apply to wsa/i2c/for-next robh/for-next linus/master v5.12-rc7 v5.12-rc6 v5.12-rc5 v5.12-rc7] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Douglas-Anderson/drm-Fix-EDID-readi... base: 18250b538735142307082e4e99e3ae5c12d44013 config: x86_64-randconfig-a002-20210416 (attached as .config) compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project f549176ad976caa3e19edd036df9a7e12770af7c) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install x86_64 cross compiling tool for clang build # apt-get install binutils-x86-64-linux-gnu # https://github.com/0day-ci/linux/commit/a870b6e38fac3e5453e4b74fdfe6eb05c8be... git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Douglas-Anderson/drm-Fix-EDID-reading-on-ti-sn65dsi86-solve-some-chicken-and-egg-problems/20210417-064243 git checkout a870b6e38fac3e5453e4b74fdfe6eb05c8be7ea7 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All errors (new ones prefixed by >>):
drivers/gpu/drm/bridge/ti-sn65dsi86.c:1308:1: error: redefinition of '__inittest'
module_auxiliary_driver(ti_sn_bridge_driver); ^ include/linux/auxiliary_bus.h:71:2: note: expanded from macro 'module_auxiliary_driver' module_driver(__auxiliary_driver, auxiliary_driver_register, auxiliary_driver_unregister) ^ include/linux/device/driver.h:262:3: note: expanded from macro 'module_driver' } \ ^ include/linux/module.h:130:42: note: expanded from macro '\ module_init' static inline initcall_t __maybe_unused __inittest(void) \ ^ drivers/gpu/drm/bridge/ti-sn65dsi86.c:1190:1: note: previous definition is here module_auxiliary_driver(ti_sn_gpio_driver); ^ include/linux/auxiliary_bus.h:71:2: note: expanded from macro 'module_auxiliary_driver' module_driver(__auxiliary_driver, auxiliary_driver_register, auxiliary_driver_unregister) ^ include/linux/device/driver.h:262:3: note: expanded from macro 'module_driver' } \ ^ include/linux/module.h:130:42: note: expanded from macro '\ module_init' static inline initcall_t __maybe_unused __inittest(void) \ ^
drivers/gpu/drm/bridge/ti-sn65dsi86.c:1308:1: error: redefinition of 'init_module'
module_auxiliary_driver(ti_sn_bridge_driver); ^ include/linux/auxiliary_bus.h:71:2: note: expanded from macro 'module_auxiliary_driver' module_driver(__auxiliary_driver, auxiliary_driver_register, auxiliary_driver_unregister) ^ include/linux/device/driver.h:262:3: note: expanded from macro 'module_driver' } \ ^ include/linux/module.h:132:6: note: expanded from macro '\ module_init' int init_module(void) __copy(initfn) \ ^ drivers/gpu/drm/bridge/ti-sn65dsi86.c:1190:1: note: previous definition is here module_auxiliary_driver(ti_sn_gpio_driver); ^ include/linux/auxiliary_bus.h:71:2: note: expanded from macro 'module_auxiliary_driver' module_driver(__auxiliary_driver, auxiliary_driver_register, auxiliary_driver_unregister) ^ include/linux/device/driver.h:262:3: note: expanded from macro 'module_driver' } \ ^ include/linux/module.h:132:6: note: expanded from macro '\ module_init' int init_module(void) __copy(initfn) \ ^
drivers/gpu/drm/bridge/ti-sn65dsi86.c:1308:1: error: redefinition of '__exittest'
module_auxiliary_driver(ti_sn_bridge_driver); ^ include/linux/auxiliary_bus.h:71:2: note: expanded from macro 'module_auxiliary_driver' module_driver(__auxiliary_driver, auxiliary_driver_register, auxiliary_driver_unregister) ^ include/linux/device/driver.h:267:3: note: expanded from macro 'module_driver' } \ ^ include/linux/module.h:138:42: note: expanded from macro '\ module_exit' static inline exitcall_t __maybe_unused __exittest(void) \ ^ drivers/gpu/drm/bridge/ti-sn65dsi86.c:1190:1: note: previous definition is here module_auxiliary_driver(ti_sn_gpio_driver); ^ include/linux/auxiliary_bus.h:71:2: note: expanded from macro 'module_auxiliary_driver' module_driver(__auxiliary_driver, auxiliary_driver_register, auxiliary_driver_unregister) ^ include/linux/device/driver.h:267:3: note: expanded from macro 'module_driver' } \ ^ include/linux/module.h:138:42: note: expanded from macro '\ module_exit' static inline exitcall_t __maybe_unused __exittest(void) \ ^
drivers/gpu/drm/bridge/ti-sn65dsi86.c:1308:1: error: redefinition of 'cleanup_module'
module_auxiliary_driver(ti_sn_bridge_driver); ^ include/linux/auxiliary_bus.h:71:2: note: expanded from macro 'module_auxiliary_driver' module_driver(__auxiliary_driver, auxiliary_driver_register, auxiliary_driver_unregister) ^ include/linux/device/driver.h:267:3: note: expanded from macro 'module_driver' } \ ^ include/linux/module.h:140:7: note: expanded from macro '\ module_exit' void cleanup_module(void) __copy(exitfn) \ ^ drivers/gpu/drm/bridge/ti-sn65dsi86.c:1190:1: note: previous definition is here module_auxiliary_driver(ti_sn_gpio_driver); ^ include/linux/auxiliary_bus.h:71:2: note: expanded from macro 'module_auxiliary_driver' module_driver(__auxiliary_driver, auxiliary_driver_register, auxiliary_driver_unregister) ^ include/linux/device/driver.h:267:3: note: expanded from macro 'module_driver' } \ ^ include/linux/module.h:140:7: note: expanded from macro '\ module_exit' void cleanup_module(void) __copy(exitfn) \ ^ 4 errors generated.
vim +/__inittest +1308 drivers/gpu/drm/bridge/ti-sn65dsi86.c
1307
1308 module_auxiliary_driver(ti_sn_bridge_driver);
1309
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi,
On Fri, Apr 16, 2021 at 7:32 PM kernel test robot lkp@intel.com wrote:
Hi Douglas,
I love your patch! Yet something to improve:
[auto build test ERROR on next-20210416] [cannot apply to wsa/i2c/for-next robh/for-next linus/master v5.12-rc7 v5.12-rc6 v5.12-rc5 v5.12-rc7] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Douglas-Anderson/drm-Fix-EDID-readi... base: 18250b538735142307082e4e99e3ae5c12d44013 config: x86_64-randconfig-a002-20210416 (attached as .config) compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project f549176ad976caa3e19edd036df9a7e12770af7c) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install x86_64 cross compiling tool for clang build # apt-get install binutils-x86-64-linux-gnu # https://github.com/0day-ci/linux/commit/a870b6e38fac3e5453e4b74fdfe6eb05c8be... git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Douglas-Anderson/drm-Fix-EDID-reading-on-ti-sn65dsi86-solve-some-chicken-and-egg-problems/20210417-064243 git checkout a870b6e38fac3e5453e4b74fdfe6eb05c8be7ea7 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All errors (new ones prefixed by >>):
drivers/gpu/drm/bridge/ti-sn65dsi86.c:1308:1: error: redefinition of '__inittest'
module_auxiliary_driver(ti_sn_bridge_driver); ^ include/linux/auxiliary_bus.h:71:2: note: expanded from macro 'module_auxiliary_driver' module_driver(__auxiliary_driver, auxiliary_driver_register, auxiliary_driver_unregister) ^ include/linux/device/driver.h:262:3: note: expanded from macro 'module_driver' } \ ^ include/linux/module.h:130:42: note: expanded from macro '\ module_init' static inline initcall_t __maybe_unused __inittest(void) \ ^ drivers/gpu/drm/bridge/ti-sn65dsi86.c:1190:1: note: previous definition is here module_auxiliary_driver(ti_sn_gpio_driver); ^ include/linux/auxiliary_bus.h:71:2: note: expanded from macro 'module_auxiliary_driver' module_driver(__auxiliary_driver, auxiliary_driver_register, auxiliary_driver_unregister) ^ include/linux/device/driver.h:262:3: note: expanded from macro 'module_driver' } \ ^ include/linux/module.h:130:42: note: expanded from macro '\ module_init' static inline initcall_t __maybe_unused __inittest(void) \
Ah, my mistake in individually using these in the same module:
module_auxiliary_driver(ti_sn_gpio_driver); module_auxiliary_driver(ti_sn_bridge_driver); module_auxiliary_driver(ti_sn_aux_driver); module_i2c_driver(ti_sn65dsi86_driver);
What I had worked fine because I wasn't building as a module. I've fixed this to have a manual init mechanism that will look something like this at the end of the series:
---
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_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: 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); auxiliary_driver_unregister(&ti_sn_aux_driver); ti_sn_gpio_unregister(); i2c_del_driver(&ti_sn65dsi86_driver); } module_exit(ti_sn65dsi86_exit);
---
With that I can compile as a module and everything works fine with this builtin. I'll plan to spin a v5 with that fix but I'll wait a little bit to see if I get any feedback. If I happen to get drm-misc commit access or can convince someone, I'll try to get the early patches in the series landed so v5 isn't so giant.
NOTE: on my system sn65dsi86 doesn't actually work currently when running as a module. That's a pre-existing problem and not one introduced by my patch. Or perhaps, more appropriately, a pre-existing pile of problems that I'm not going to try to tackle. A quick summary:
* Part of the problem of making this a module is that I run into the looping I spent a little bit of time looking at in the past [1]. I believe the main MSM graphics driver can't handle itself being builtin but some of the things it needs being in modules.
* Part of the problem is fw_devlink. I don't think it understands the circularness of the panel and HPD lines and it seems to get upset unless in permissive mode.
* If I try permissive mode and move the whole MSM graphics to a module, I get an error about 'disp_cc_mdss_mdp_clk_src: RCG did not turn on'. Again, this is without my patch series.
Those are not small problems and not new, so I'll settle for making sure I continue to compile as a module.
[1] https://lore.kernel.org/lkml/20200710230224.2265647-1-dianders@chromium.org/
On Fri 16 Apr 17:39 CDT 2021, Douglas Anderson 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.
I rebased my pwm_chip patch ontop of this and it works like a charm!
Signed-off-by: Douglas Anderson dianders@chromium.org
(no changes since v1)
drivers/gpu/drm/bridge/Kconfig | 1 + drivers/gpu/drm/bridge/ti-sn65dsi86.c | 211 +++++++++++++++++++------- 2 files changed, 158 insertions(+), 54 deletions(-)
diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig index 22a467abd3e9..6868050961a2 100644 --- a/drivers/gpu/drm/bridge/Kconfig +++ b/drivers/gpu/drm/bridge/Kconfig @@ -267,6 +267,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 57bc489a0412..44edcf6f5744 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -4,6 +4,7 @@
*/
+#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;
@@ -1135,8 +1142,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 */
@@ -1158,19 +1167,27 @@ 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", },
- {},
+};
+MODULE_DEVICE_TABLE(auxiliary, ti_sn_gpio_id_table);
The MODULE_DEVICE_TABLE will ensure that the driver will be autoloaded if this auxiliary compatible is requested, but it's only ever going to be triggered by the already loaded driver.
So you can omit this.
-static inline int ti_sn_setup_gpio_controller(struct ti_sn65dsi86 *pdata) -{
- return 0;
-} +static struct auxiliary_driver ti_sn_gpio_driver = {
- .name = "gpio",
- .probe = ti_sn_gpio_probe,
- .id_table = ti_sn_gpio_id_table,
+};
+module_auxiliary_driver(ti_sn_gpio_driver);
You may only have a single module_driver() per driver. Compiling the driver as a module will result in a spew of errors because init_module() and cleanup_module() are defined multiple times.
As such I believe you have to roll your own init/exit. I did the following for my testing:
====8<-----------------
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 62904dfdee0a..fe3317bc85be 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -1218,8 +1218,6 @@ static struct auxiliary_driver ti_sn_gpio_driver = { .id_table = ti_sn_gpio_id_table, };
-module_auxiliary_driver(ti_sn_gpio_driver); - #endif
static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata, @@ -1329,8 +1327,6 @@ static struct auxiliary_driver ti_sn_bridge_driver = { .id_table = ti_sn_bridge_id_table, };
-module_auxiliary_driver(ti_sn_bridge_driver); - static void ti_sn65dsi86_runtime_disable(void *data) { pm_runtime_disable(data); @@ -1432,8 +1428,6 @@ static struct auxiliary_driver ti_sn_aux_driver = { .id_table = ti_sn_aux_id_table, };
-module_auxiliary_driver(ti_sn_aux_driver); - static int ti_sn65dsi86_probe(struct i2c_client *client, const struct i2c_device_id *id) { @@ -1535,7 +1529,32 @@ static struct i2c_driver ti_sn65dsi86_driver = { .probe = ti_sn65dsi86_probe, .id_table = ti_sn65dsi86_id, }; -module_i2c_driver(ti_sn65dsi86_driver); + +static int ti_sn65dsi86_init(void) +{ +#if defined(CONFIG_OF_GPIO) + auxiliary_driver_register(&ti_sn_gpio_driver); +#endif + auxiliary_driver_register(&ti_sn_bridge_driver); + auxiliary_driver_register(&ti_sn_aux_driver); + + i2c_add_driver(&ti_sn65dsi86_driver); + + return 0; +} +module_init(ti_sn65dsi86_init); + +static void ti_sn65dsi86_exit(void) +{ + i2c_del_driver(&ti_sn65dsi86_driver); + +#if defined(CONFIG_OF_GPIO) + auxiliary_driver_unregister(&ti_sn_gpio_driver); +#endif + auxiliary_driver_unregister(&ti_sn_bridge_driver); + auxiliary_driver_unregister(&ti_sn_aux_driver); +} +module_exit(ti_sn65dsi86_exit);
MODULE_AUTHOR("Sandeep Panda spanda@codeaurora.org"); MODULE_DESCRIPTION("sn65dsi86 DSI to eDP bridge driver");
8<-----------------
The rest looks good!
Regards, Bjorn
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 ---
(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 6b22872b3281..90a17ca79d06 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; }
On Fri 16 Apr 17:39 CDT 2021, Douglas Anderson 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.
Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org
Regards, Bjorn
Signed-off-by: Douglas Anderson dianders@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 6b22872b3281..90a17ca79d06 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);-- 2.31.1.368.gbe11c130af-goog
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 ---
(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 44edcf6f5744..a98abf496190 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; } @@ -292,7 +292,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)) { @@ -418,7 +418,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;
@@ -1049,7 +1049,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; @@ -1100,7 +1100,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; } @@ -1126,7 +1126,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; @@ -1408,6 +1408,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);
On Fri 16 Apr 17:39 CDT 2021, Douglas Anderson wrote:
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.
Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org
Regards, Bjorn
Signed-off-by: Douglas Anderson dianders@chromium.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 44edcf6f5744..a98abf496190 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;
} @@ -292,7 +292,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)) {
@@ -418,7 +418,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;
@@ -1049,7 +1049,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;
@@ -1100,7 +1100,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;
} @@ -1126,7 +1126,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;
@@ -1408,6 +1408,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.368.gbe11c130af-goog
No functional changes--this just makes the diffstat of a future change easier to understand.
Signed-off-by: Douglas Anderson dianders@chromium.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 a98abf496190..b3c699da7724 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); @@ -459,64 +517,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;
On Fri 16 Apr 17:39 CDT 2021, Douglas Anderson wrote:
No functional changes--this just makes the diffstat of a future change easier to understand.
Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org
Regards, Bjorn
Signed-off-by: Douglas Anderson dianders@chromium.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 a98abf496190..b3c699da7724 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); @@ -459,64 +517,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.368.gbe11c130af-goog
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 ---
(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 b3c699da7724..875e5dbe6594 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); @@ -843,27 +901,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); } @@ -874,7 +913,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); } @@ -908,6 +948,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: @@ -918,7 +972,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)); @@ -942,11 +997,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) { /* @@ -954,13 +1009,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: @@ -972,18 +1028,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) @@ -1380,6 +1437,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)) {
On Fri 16 Apr 17:39 CDT 2021, Douglas Anderson wrote:
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").
Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org
Regards, Bjorn
Signed-off-by: Douglas Anderson dianders@chromium.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 b3c699da7724..875e5dbe6594 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);
@@ -843,27 +901,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);
} @@ -874,7 +913,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);
} @@ -908,6 +948,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:
@@ -918,7 +972,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));
@@ -942,11 +997,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) { /*
@@ -954,13 +1009,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;@@ -972,18 +1028,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) @@ -1380,6 +1437,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.368.gbe11c130af-goog
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 ---
(no changes since v1)
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 82 ++++++++++++++++++++------- 1 file changed, 63 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 875e5dbe6594..8253098bcdbf 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; @@ -483,18 +485,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, @@ -551,8 +547,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; }
@@ -1316,11 +1310,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;
@@ -1419,6 +1408,54 @@ 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", }, + {}, +}; + +MODULE_DEVICE_TABLE(auxiliary, ti_sn_aux_id_table); + +static struct auxiliary_driver ti_sn_aux_driver = { + .name = "aux", + .probe = ti_sn_aux_probe, + .id_table = ti_sn_aux_id_table, +}; + +module_auxiliary_driver(ti_sn_aux_driver); + static int ti_sn65dsi86_probe(struct i2c_client *client, const struct i2c_device_id *id) { @@ -1477,10 +1514,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)) { @@ -1489,7 +1527,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[] = {
On Fri 16 Apr 17:39 CDT 2021, Douglas Anderson 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
(no changes since v1)
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 82 ++++++++++++++++++++------- 1 file changed, 63 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 875e5dbe6594..8253098bcdbf 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;
@@ -483,18 +485,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,
@@ -551,8 +547,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;
}
@@ -1316,11 +1310,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;
@@ -1419,6 +1408,54 @@ 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", },
- {},
+};
+MODULE_DEVICE_TABLE(auxiliary, ti_sn_aux_id_table);
+static struct auxiliary_driver ti_sn_aux_driver = {
- .name = "aux",
- .probe = ti_sn_aux_probe,
- .id_table = ti_sn_aux_id_table,
+};
+module_auxiliary_driver(ti_sn_aux_driver);
As with the earlier patch, please drop MODULE_DEVICE_TABLE() and rework module_driver().
Regards, Bjorn
static int ti_sn65dsi86_probe(struct i2c_client *client, const struct i2c_device_id *id) { @@ -1477,10 +1514,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)) {
@@ -1489,7 +1527,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[] = {
2.31.1.368.gbe11c130af-goog
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 --- 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 90a17ca79d06..c91e8aa108f7 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);
On Fri 16 Apr 17:39 CDT 2021, Douglas Anderson 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.
Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org
Signed-off-by: Douglas Anderson dianders@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 90a17ca79d06..c91e8aa108f7 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);-- 2.31.1.368.gbe11c130af-goog
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 ---
(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 c91e8aa108f7..40382c1be692 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 */
On Fri 16 Apr 17:39 CDT 2021, Douglas Anderson 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.
Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org
Regards, Bjorn
Signed-off-by: Douglas Anderson dianders@chromium.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 c91e8aa108f7..40382c1be692 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.368.gbe11c130af-goog
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 ---
(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 40382c1be692..5a2953c4ca44 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);
On Fri 16 Apr 17:39 CDT 2021, Douglas Anderson 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.
Signed-off-by: Douglas Anderson dianders@chromium.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 40382c1be692..5a2953c4ca44 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;
Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org
But separate of this, shouldn't the driver have a pm_runtime_disable() in the remove path to synchronize the autosleep? Or is that not how that works?
Regards, Bjorn
- 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.368.gbe11c130af-goog
Hi,
On Fri, Apr 23, 2021 at 9:12 AM Bjorn Andersson bjorn.andersson@linaro.org wrote:
On Fri 16 Apr 17:39 CDT 2021, Douglas Anderson 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.
Signed-off-by: Douglas Anderson dianders@chromium.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 40382c1be692..5a2953c4ca44 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;Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org
But separate of this, shouldn't the driver have a pm_runtime_disable() in the remove path to synchronize the autosleep? Or is that not how that works?
Indeed! I'll add a patch to the start of my v5 (coming shortly) that fixes this. Thanks for catching!
-Doug
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 ---
(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 8253098bcdbf..62904dfdee0a 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; @@ -404,24 +402,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); }
@@ -1330,8 +1310,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);
On Fri 16 Apr 17:39 CDT 2021, Douglas Anderson wrote:
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:
- Read the EDID.
- Based on the EDID, decide on video settings and pixel clock.
- Enable the bridge w/ the desired settings.
The way things were working:
- Try to read the EDID but fail; fall back to hardcoded values.
- Based on hardcoded values, decide on video settings and pixel clock.
- Enable the bridge w/ the desired settings.
- Try again to read the EDID, it works now!
- Realize that the hardcoded settings weren't quite right.
- 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
Regards, Bjorn
(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 8253098bcdbf..62904dfdee0a 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;
@@ -404,24 +402,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);
}
@@ -1330,8 +1310,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.368.gbe11c130af-goog
Historically simple-panel had code to make sure that if prepare() was called on an already-prepared panel that it was a no-op. Similarly if unprepare() was called on an already-unprepared panel it was also a no-op. Essentially it means that these functions always "forced" the value to be whatever the caller wanted it to be. You can see that the forcing behavior dates back at least as far as 2014 by looking at commit 613a633e7a56 ("drm/panel: simple: Add proper definition for prepare and unprepare").
Apparently the code supporting the historical behavior may not be needed [1] and prepare() / unprepare() are supposed to be balanced. Let's try removing it and see if anyone breaks! If they do then we can have a debate about whether we should change that code or revert this patch. :-) If nobody breaks then we've nicely saved a few lines of code and some complexity.
[1] https://lore.kernel.org/r/YHePsQgqOau1V5lD@pendragon.ideasonboard.com
Suggested-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Douglas Anderson dianders@chromium.org ---
(no changes since v1)
drivers/gpu/drm/panel/panel-simple.c | 14 -------------- 1 file changed, 14 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index 5a2953c4ca44..a2c3008af7e5 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -176,8 +176,6 @@ struct panel_simple { bool enabled; bool no_hpd;
- bool prepared; - ktime_t prepared_time; ktime_t unprepared_time;
@@ -355,18 +353,12 @@ static int panel_simple_suspend(struct device *dev)
static int panel_simple_unprepare(struct drm_panel *panel) { - struct panel_simple *p = to_panel_simple(panel); int ret;
- /* Unpreparing when already unprepared is a no-op */ - if (!p->prepared) - return 0; - pm_runtime_mark_last_busy(panel->dev); ret = pm_runtime_put_autosuspend(panel->dev); if (ret < 0) return ret; - p->prepared = false;
return 0; } @@ -475,18 +467,12 @@ static int panel_simple_prepare(struct drm_panel *panel) struct panel_simple *p = to_panel_simple(panel); int ret;
- /* Preparing when already prepared is a no-op */ - if (p->prepared) - return 0; - ret = pm_runtime_get_sync(panel->dev); if (ret < 0) { pm_runtime_put_autosuspend(panel->dev); return ret; }
- p->prepared = true; - return 0; }
On Fri 16 Apr 17:39 CDT 2021, Douglas Anderson wrote:
Historically simple-panel had code to make sure that if prepare() was called on an already-prepared panel that it was a no-op. Similarly if unprepare() was called on an already-unprepared panel it was also a no-op. Essentially it means that these functions always "forced" the value to be whatever the caller wanted it to be. You can see that the forcing behavior dates back at least as far as 2014 by looking at commit 613a633e7a56 ("drm/panel: simple: Add proper definition for prepare and unprepare").
Apparently the code supporting the historical behavior may not be needed [1] and prepare() / unprepare() are supposed to be balanced. Let's try removing it and see if anyone breaks! If they do then we can have a debate about whether we should change that code or revert this patch. :-) If nobody breaks then we've nicely saved a few lines of code and some complexity.
[1] https://lore.kernel.org/r/YHePsQgqOau1V5lD@pendragon.ideasonboard.com
Acked-by: Bjorn Andersson bjorn.andersson@linaro.org
Regards, Bjorn
Suggested-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Douglas Anderson dianders@chromium.org
(no changes since v1)
drivers/gpu/drm/panel/panel-simple.c | 14 -------------- 1 file changed, 14 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index 5a2953c4ca44..a2c3008af7e5 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -176,8 +176,6 @@ struct panel_simple { bool enabled; bool no_hpd;
- bool prepared;
- ktime_t prepared_time; ktime_t unprepared_time;
@@ -355,18 +353,12 @@ static int panel_simple_suspend(struct device *dev)
static int panel_simple_unprepare(struct drm_panel *panel) {
struct panel_simple *p = to_panel_simple(panel); int ret;
/* Unpreparing when already unprepared is a no-op */
if (!p->prepared)
return 0;pm_runtime_mark_last_busy(panel->dev); ret = pm_runtime_put_autosuspend(panel->dev); if (ret < 0) return ret;
p->prepared = false;
return 0;
} @@ -475,18 +467,12 @@ static int panel_simple_prepare(struct drm_panel *panel) struct panel_simple *p = to_panel_simple(panel); int ret;
/* Preparing when already prepared is a no-op */
if (p->prepared)
return 0;ret = pm_runtime_get_sync(panel->dev); if (ret < 0) { pm_runtime_put_autosuspend(panel->dev); return ret; }
p->prepared = true;
return 0;
}
-- 2.31.1.368.gbe11c130af-goog
Hi,
On Fri, Apr 16, 2021 at 3:41 PM Douglas Anderson dianders@chromium.org wrote:
Historically simple-panel had code to make sure that if prepare() was called on an already-prepared panel that it was a no-op. Similarly if unprepare() was called on an already-unprepared panel it was also a no-op. Essentially it means that these functions always "forced" the value to be whatever the caller wanted it to be. You can see that the forcing behavior dates back at least as far as 2014 by looking at commit 613a633e7a56 ("drm/panel: simple: Add proper definition for prepare and unprepare").
Apparently the code supporting the historical behavior may not be needed [1] and prepare() / unprepare() are supposed to be balanced. Let's try removing it and see if anyone breaks! If they do then we can have a debate about whether we should change that code or revert this patch. :-) If nobody breaks then we've nicely saved a few lines of code and some complexity.
[1] https://lore.kernel.org/r/YHePsQgqOau1V5lD@pendragon.ideasonboard.com
Suggested-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Douglas Anderson dianders@chromium.org
(no changes since v1)
drivers/gpu/drm/panel/panel-simple.c | 14 -------------- 1 file changed, 14 deletions(-)
So it turns out that when looking at panel_simple_remove() I already found a case that's not necessarily refcounting. There I see drm_panel_unprepare() just simply hardcoded, but (as I understand it) there's no reason to believe that the panel has been prepared at remove() time. Yes, I could fix panel_simple_remove() but instead, for now, I think I'm going to drop this patch from the series. If someone wants to pick it up then I certainly won't object, but for now keeping the way things are seems the best way to keep from getting shouted at.
-Doug
Hi,
On Fri, Apr 16, 2021 at 3:40 PM Douglas Anderson dianders@chromium.org wrote:
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) 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.
I apologize in advance for the length of this series. I'm currently working through getting commit access to drm-misc [3] so I can land the first several patches which are already reviewed. There are still a lot of patches even after the first few, but hopefully you can see that there are only so many because they're broken up into nice and reviewable bite-sized-chunks. :-)
[1] https://lore.kernel.org/r/20210304155144.3.I60a7fb23ce4589006bc95c64ab8d15c7... [2] https://lore.kernel.org/r/161706912161.3012082.17313817257247946143@swboyd.m... [3] https://gitlab.freedesktop.org/freedesktop/freedesktop/-/issues/348
Changes in v4:
- Reword commit mesage slightly.
Changes in v3:
- Removed "NOTES" from commit message.
Changes in v2:
- Removed 2nd paragraph in commit message.
Douglas Anderson (27): drm/bridge: Fix the stop condition of drm_bridge_chain_pre_enable() drm/bridge: ti-sn65dsi86: Simplify refclk handling drm/bridge: ti-sn65dsi86: Remove incorrectly tagged kerneldoc comment drm/bridge: ti-sn65dsi86: Reorder remove() drm/bridge: ti-sn65dsi86: Move drm_panel_unprepare() to post_disable() drm/bridge: ti-sn65dsi86: Get rid of the useless detect() function drm/panel: panel-simple: Use runtime pm to avoid excessive unprepare / prepare
I have pushed the above 7 patches to drm-misc-next now so I don't have to spam everyone with them for v5. The first patch is technically a "fix" but I'm not aware of it affecting anyone in mainline and so I didn't try to direct it to a fixes branch. The next 5 are trivial / reviewed plenty. The last one is a bigger change but has Laurent's review on it and it's been up on the lists for a while, so I sent it in too.
I'll wait a few more days to see if any of the other "trivial" patches early in the series get reviews and see if there is any other feedback on the rest of the series. If I get reviews for some of the early patches I'll likely try to push them before the v5.
-Doug
dri-devel@lists.freedesktop.org
-
Bjorn Andersson -
Doug Anderson -
Douglas Anderson
-
kernel test robot