[PATCH v5 00/20] 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) with drm-misc-next (as of 20210423) merged in on a sc7180-trogdor-lazor device. To get things booting for me, I had to use Stephen's patch [2] to keep from crashing but otherwise all the patches I needed were here.
Primary change between v2 and v3 is to stop doing the EDID caching in the core. I also added Andrzej's review tags.
Between v3 and v4 this series grew a whole lot. I changed it so that the EDID reading is actually driven by the panel driver now as was suggested by Andrzej. While I still believe that the old approach wasn't too bad I'm still switching. Why?
The main reason is that I think it's useful in general for the panel code to have access to the DDC bus and to be able to read the EDID. This may allow us to more easily have the panel code support multiple sources of panels--it can read the EDID and possibly adjust timings based on the model ID. It also allows the panel code (or perhaps backlight code?) to send DDC commands if they are need for a particular panel.
At the moment, once the panel is provided the DDC bus then existing code will assume that it should be in charge of reading the EDID. While it doesn't have to work that way, it seems sane to build on what's already there.
In order to expose the DDC bus to the panel, I had to solve a bunch of chicken-and-egg problems in terms of probe ordering between the bridge and the panel. I've broken the bridge driver into several sub drivers to make this happen. At the moment the sub-drivers are just there to solve the probe problem, but conceivably someone could use them to break the driver up in the future if need be.
Between v4 and v5, high-level view of changes. - Some of the early patches landed, so dropped from series. - New pm_runtime_disable() fix (fixed a patch that already landed). - Added Bjorn's tags to most patches - Fixed problems when building as a module. - Reordered debugfs patch and fixed error handling there. - Dropped last patch. I'm not convinced it's safe w/out more work.
I apologize in advance for the length of this series. Hopefully you can see that there are only so many because they're broken up into nice and reviewable bite-sized-chunks. :-)
*** NOTE ***: my hope is to actually land patches that have been reviewed sometime mid-to-late next week. Please shout if you think this is too much of a rush and you know of someone who is planning to review that needs more time.
[1] https://lore.kernel.org/r/20210304155144.3.I60a7fb23ce4589006bc95c64ab8d15c7... [2] https://lore.kernel.org/r/161706912161.3012082.17313817257247946143@swboyd.m...
Changes in v5: - Missing pm_runtime_disable() patch new for v5. - Reordered to debugfs change to avoid transient issue - Don't print debugfs creation errors. - Handle NULL from debugfs_create_dir() which is documented possible. - Rebased atop the pm_runtime patch, which got reordered. - Fix module compile problems (Bjorn + kbuild bot) - Remove useless MODULE_DEVICE_TABLE (Bjorn). - Fix module compile problems (Bjorn + kbuild bot) - Remove useless MODULE_DEVICE_TABLE (Bjorn).
Douglas Anderson (20): drm/panel: panel-simple: Add missing pm_runtime_disable() calls drm/bridge: ti-sn65dsi86: Rename the main driver data structure drm/bridge: ti-sn65dsi86: More renames in prep for sub-devices drm/bridge: ti-sn65dsi86: Use devm to do our runtime_disable drm/bridge: ti-sn65dsi86: Clean debugfs code drm/bridge: ti-sn65dsi86: Add local var for "dev" to simplify probe drm/bridge: ti-sn65dsi86: Cleanup managing of drvdata drm/bridge: ti-sn65dsi86: Move all the chip-related init to the start drm/bridge: ti-sn65dsi86: Break GPIO and MIPI-to-eDP bridge into sub-drivers drm/panel: panel-simple: Get rid of hacky HPD chicken-and-egg code drm/bridge: ti-sn65dsi86: Use pm_runtime autosuspend drm/bridge: ti-sn65dsi86: Code motion of refclk management functions drm/bridge: ti-sn65dsi86: If refclk, DP AUX can happen w/out pre-enable drm/bridge: ti-sn65dsi86: Promote the AUX channel to its own sub-dev i2c: i2c-core-of: Fix corner case of finding adapter by node drm/panel: panel-simple: Remove extra call: drm_connector_update_edid_property() drm/panel: panel-simple: Power the panel when reading the EDID drm/panel: panel-simple: Cache the EDID as long as we retain power drm/bridge: ti-sn65dsi86: Don't read EDID blob over DDC arm64: dts: qcom: Link the panel to the bridge's DDC bus
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 1 + drivers/gpu/drm/bridge/Kconfig | 1 + drivers/gpu/drm/bridge/ti-sn65dsi86.c | 767 ++++++++++++------- drivers/gpu/drm/panel/panel-simple.c | 49 +- drivers/i2c/i2c-core-of.c | 17 +- 5 files changed, 538 insertions(+), 297 deletions(-)
In commit 3235b0f20a0a ("drm/panel: panel-simple: Use runtime pm to avoid excessive unprepare / prepare") we started using pm_runtime, but my patch neglected to add the proper pm_runtime_disable(). Doh! Add them now.
Fixes: 3235b0f20a0a ("drm/panel: panel-simple: Use runtime pm to avoid excessive unprepare / prepare") Reported-by: Bjorn Andersson bjorn.andersson@linaro.org Signed-off-by: Douglas Anderson dianders@chromium.org ---
Changes in v5: - Missing pm_runtime_disable() patch new for v5.
drivers/gpu/drm/panel/panel-simple.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index 6b22872b3281..9746eda6f675 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -797,12 +797,14 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
err = drm_panel_of_backlight(&panel->base); if (err) - goto free_ddc; + goto disable_pm_runtime;
drm_panel_add(&panel->base);
return 0;
+disable_pm_runtime: + pm_runtime_disable(dev); free_ddc: if (panel->ddc) put_device(&panel->ddc->dev); @@ -818,6 +820,7 @@ static int panel_simple_remove(struct device *dev) drm_panel_disable(&panel->base); drm_panel_unprepare(&panel->base);
+ pm_runtime_disable(dev); if (panel->ddc) put_device(&panel->ddc->dev);
On Fri, Apr 23, 2021 at 12:59 PM Douglas Anderson dianders@chromium.org wrote:
In commit 3235b0f20a0a ("drm/panel: panel-simple: Use runtime pm to avoid excessive unprepare / prepare") we started using pm_runtime, but my patch neglected to add the proper pm_runtime_disable(). Doh! Add them now.
Fixes: 3235b0f20a0a ("drm/panel: panel-simple: Use runtime pm to avoid excessive unprepare / prepare") Reported-by: Bjorn Andersson bjorn.andersson@linaro.org
Reviewed-by: Sean Paul seanpaul@chromium.org
Signed-off-by: Douglas Anderson dianders@chromium.org
Changes in v5:
- Missing pm_runtime_disable() patch new for v5.
drivers/gpu/drm/panel/panel-simple.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index 6b22872b3281..9746eda6f675 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -797,12 +797,14 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
err = drm_panel_of_backlight(&panel->base); if (err)
goto free_ddc;
goto disable_pm_runtime; drm_panel_add(&panel->base); return 0;+disable_pm_runtime:
pm_runtime_disable(dev);free_ddc: if (panel->ddc) put_device(&panel->ddc->dev); @@ -818,6 +820,7 @@ static int panel_simple_remove(struct device *dev) drm_panel_disable(&panel->base); drm_panel_unprepare(&panel->base);
pm_runtime_disable(dev); if (panel->ddc) put_device(&panel->ddc->dev);-- 2.31.1.498.g6c1eba8ee3d-goog
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Apr 23, 2021 at 6:59 PM Douglas Anderson dianders@chromium.org wrote:
In commit 3235b0f20a0a ("drm/panel: panel-simple: Use runtime pm to avoid excessive unprepare / prepare") we started using pm_runtime, but my patch neglected to add the proper pm_runtime_disable(). Doh! Add them now.
Fixes: 3235b0f20a0a ("drm/panel: panel-simple: Use runtime pm to avoid excessive unprepare / prepare") Reported-by: Bjorn Andersson bjorn.andersson@linaro.org Signed-off-by: Douglas Anderson dianders@chromium.org
This patch as such: Reviewed-by: Linus Walleij linus.walleij@linaro.org
Notice however: you turn on pm runtime pm_runtime_enable() in panel_simple_probe() but are you ever turning it off in panel_simple_remove()?
I think pm_runtime_disable(); need to be added there?
Yours, Linus Walleij
Hi,
On Thu, Apr 29, 2021 at 5:58 PM Linus Walleij linus.walleij@linaro.org wrote:
On Fri, Apr 23, 2021 at 6:59 PM Douglas Anderson dianders@chromium.org wrote:
In commit 3235b0f20a0a ("drm/panel: panel-simple: Use runtime pm to avoid excessive unprepare / prepare") we started using pm_runtime, but my patch neglected to add the proper pm_runtime_disable(). Doh! Add them now.
Fixes: 3235b0f20a0a ("drm/panel: panel-simple: Use runtime pm to avoid excessive unprepare / prepare") Reported-by: Bjorn Andersson bjorn.andersson@linaro.org Signed-off-by: Douglas Anderson dianders@chromium.org
This patch as such: Reviewed-by: Linus Walleij linus.walleij@linaro.org
Notice however: you turn on pm runtime pm_runtime_enable() in panel_simple_probe() but are you ever turning it off in panel_simple_remove()?
I think pm_runtime_disable(); need to be added there?
I'm a bit confused. You're saying that I need to add pm_runtime_disable() to panel_simple_remove()? Doesn't this patch do that? This patch adds two calls to pm_runtime_disable(). One of those is in the probe error path and the other one is in panel_simple_remove().
-Doug
On Fri, Apr 30, 2021 at 3:25 AM Doug Anderson dianders@chromium.org wrote:
I think pm_runtime_disable(); need to be added there?
I'm a bit confused. You're saying that I need to add pm_runtime_disable() to panel_simple_remove()? Doesn't this patch do that?
It does, sorry, too late at night :D
I was looking at the previous patch and mixed up which was the patch and the patch to the patch...
Thanks, apply this! Linus Walleij
Hi,
On Thu, Apr 29, 2021 at 6:28 PM Linus Walleij linus.walleij@linaro.org wrote:
On Fri, Apr 30, 2021 at 3:25 AM Doug Anderson dianders@chromium.org wrote:
I think pm_runtime_disable(); need to be added there?
I'm a bit confused. You're saying that I need to add pm_runtime_disable() to panel_simple_remove()? Doesn't this patch do that?
It does, sorry, too late at night :D
I was looking at the previous patch and mixed up which was the patch and the patch to the patch...
Thanks, apply this!
Pushed this one patch. Rest of the series is pending adult supervision. Overall summary:
1. I could probably push some of the early sn65dsi86 cleanup patches in this series since they have Bjorn's review and are pretty much no-ops / simple cleanups, but there's probably not tons gained for shoving those in early.
2. The whole concept of breaking up the patch into sub-drivers has no official Reviewed-by tags yet. Presumably Bjorn will give those a re-review when he has time again. Assuming nobody is really upset about it, I could land those which might unblock some of Bjorn's future PWM work. It would probably be good to get an extra set of eyes on them, though, just so someone else agrees that they're not "too hacky" or anything. IMO it's actually a pretty nice solution, but I'm biased!
3. Laurent and I had a big discussion on #dri-devel yesterday about the EDID reading. He's not totally convinced with the idea of doing this in the panel when the bridge could just do it by itself, but it sounded like he might be coming around. Right now this is waiting on Laurent to have time to get back to this.
My summary of the IRC discussion with Laurent (please correct if I got this wrong):
a) In general I argued that it was important to be able to provide the EDID and the DDC bus to the panel driver. Providing the EDID to the panel driver allows the panel driver is one of the prerequisites for my proposal for solving the "panel second sourcing" problem [1]. Being able to provide the DDC bus to the panel will likely be important in the eventual solution to Rajeev's problem [2].
b) Today, if we provide the DDC bus to simple-panel then simple-panel will assume it's in charge of reading the EDID.
c) Having the panel driver involved in reading the EDID feels like it makes sense to me. The panel driver knows how to power the panel on enough to read the EDID. It also might know extra quirks needed to read the EDID on a given panel. This feels a little cleaner (to me) than just re-using the panel's "prepare" and assuming that a prepared panel was ready for EDID read, though I can see that both may have their advantages.
d) Laurent proposed that some eDP controllers might have special ways to read an EDID but might not be able to provide a DDC bus or an i2c bus. If we run into controllers like this then we would be painted into a corner and we'd have to come up with a new solution. This is definitely a good point, though it remains to be seen if this is common with eDP (like Laurent says it is for HDMI). Some eDP panels need custom DDC commands in order to be configured and so hopefully all eDP bridges out there at least provide a DDC bus. It does feel like this could be solved later, though. My patch series is leveraging the existing concept that the panel driver is in charge of reading the EDID if it's given the DDC bus, so it's not creating a new mechanism but instead just continuing to use the existing mechanism. If the existing mechanism doesn't work then it can be improved when there is a need.
e) Laurent worried about circular dependencies and wanted to see my solution to the problem before deciding if it was too big of a hack. Hopefully it looks OK since it solves not only this problem but also the HPD GPIO problem and will be important for when Bjorn exports the PWM from the bridge chip.
[1] https://lore.kernel.org/lkml/CAD=FV=VZYOMPwQZzWdhJGh5cjJWw_EcM-wQVEivZ-bdGXj... [2] https://lore.kernel.org/r/78c4bd291bd4a17ae2a1d02d0217de43@codeaurora.org
OK, I'll shut up now. ;-)
-Doug
Hi Doug,
On Fri, Apr 30, 2021 at 11:04 PM Doug Anderson dianders@chromium.org wrote:
Pushed this one patch. Rest of the series is pending adult supervision. Overall summary:
- I could probably push some of the early sn65dsi86 cleanup patches
in this series since they have Bjorn's review and are pretty much no-ops / simple cleanups, but there's probably not tons gained for shoving those in early.
Those look good to me as well. I'd say just apply them.
To me it looks like up until and including patch 18? Feel free to add my Acked-by: Linus Walleij linus.walleij@linaro.org
On these.
- The whole concept of breaking up the patch into sub-drivers has no
official Reviewed-by tags yet. Presumably Bjorn will give those a re-review when he has time again.
It looks good to me so I sent an explicit ACK on that patch.
- Laurent and I had a big discussion on #dri-devel yesterday about
the EDID reading. He's not totally convinced with the idea of doing this in the panel when the bridge could just do it by itself, but it sounded like he might be coming around. Right now this is waiting on Laurent to have time to get back to this.
I dare not speak of this. Laurent has the long and tedious experience with panels and pretty much anything related so if Laurent is hesitant then I am hesitant too. His buy-in is absolutely required.
But IIUC that is just for patch 19+20?
It'd be good to apply the rest and get down the stack.
Just to keep you busy and make sure you don't run out of work (haha) I noticed that the gpio_chip in this driver can use the new GPIO_REGMAP helper library with the fixes just landed in Torvald's tree.
At your convenience and when you think there is too little stuff in your sn65dsi86 TODO, check out pinctrl-bcm63xx.c for an example of select GPIO_REGMAP made very simple (this works fine as long as they are bit offsets starting from 0).
Yours, Linus Walleij
Hi,
On Sat, May 1, 2021 at 5:07 AM Linus Walleij linus.walleij@linaro.org wrote:
Hi Doug,
On Fri, Apr 30, 2021 at 11:04 PM Doug Anderson dianders@chromium.org wrote:
Pushed this one patch. Rest of the series is pending adult supervision. Overall summary:
- I could probably push some of the early sn65dsi86 cleanup patches
in this series since they have Bjorn's review and are pretty much no-ops / simple cleanups, but there's probably not tons gained for shoving those in early.
Those look good to me as well. I'd say just apply them.
To me it looks like up until and including patch 18? Feel free to add my Acked-by: Linus Walleij linus.walleij@linaro.org
On these.
OK, thanks! I've just pushed these patches to drm-misc-next with your Ack:
63358e24ee79 drm/panel: panel-simple: Cache the EDID as long as we retain power 31e25395d8b7 drm/panel: panel-simple: Power the panel when reading the EDID 4318ea406e02 drm/panel: panel-simple: Remove extra call: drm_connector_update_edid_property() b137406d9679 drm/bridge: ti-sn65dsi86: If refclk, DP AUX can happen w/out pre-enable f7a5ee2cd3e2 drm/bridge: ti-sn65dsi86: Code motion of refclk management functions 9bede63127c6 drm/bridge: ti-sn65dsi86: Use pm_runtime autosuspend 5c4381eeb709 drm/panel: panel-simple: Get rid of hacky HPD chicken-and-egg code bf73537f411b drm/bridge: ti-sn65dsi86: Break GPIO and MIPI-to-eDP bridge into sub-drivers bef236a5206c drm/bridge: ti-sn65dsi86: Move all the chip-related init to the start f94eb8a32863 drm/bridge: ti-sn65dsi86: Cleanup managing of drvdata 3636fc25f760 drm/bridge: ti-sn65dsi86: Add local var for "dev" to simplify probe 52d54819c8ae drm/bridge: ti-sn65dsi86: Clean debugfs code dea2500a820c drm/bridge: ti-sn65dsi86: Use devm to do our runtime_disable 905d66d08d0f drm/bridge: ti-sn65dsi86: More renames in prep for sub-devices db0036db4851 drm/bridge: ti-sn65dsi86: Rename the main driver data structure
Things not pushed:
[v5,15/20] i2c: i2c-core-of: Fix corner case of finding adapter by node -> Can't push i2c things
[v5,14/20] drm/bridge: ti-sn65dsi86: Promote the AUX channel to its own sub-dev -> Won't work without rework. See [1]
[v5,19/20] drm/bridge: ti-sn65dsi86: Don't read EDID blob over DDC -> Needs Laurent and also patch 14/20 to be resolved.
[v5,20/20] arm64: dts: qcom: Link the panel to the bridge's DDC bus -> Needs all the rest resolved.
Let me see if I can find a way to work around the AUX channel stuff and then I'll push a v6 of just what's left.
Just to keep you busy and make sure you don't run out of work (haha) I noticed that the gpio_chip in this driver can use the new GPIO_REGMAP helper library with the fixes just landed in Torvald's tree.
At your convenience and when you think there is too little stuff in your sn65dsi86 TODO, check out pinctrl-bcm63xx.c for an example of select GPIO_REGMAP made very simple (this works fine as long as they are bit offsets starting from 0).
I seem to recall you mentioning something like this. When I looked at it in the past I wasn't convinced it would be easy. See my response [2]. The rough summary is that I didn't think the helpers were happy with the pm_runtime() model that I'm using. Did I get that wrong?
[1] https://lore.kernel.org/dri-devel/CAD=FV=UTmOP8LDaf-Tyx17OORQK6pJH6O_w3cP0Bu... [2] https://lore.kernel.org/r/CAD=FV=VqD-dY=v23KYuTqy8aRNQJJzJ7h_UOcdEBYuK9X51MQ...
-Doug
On Mon, May 3, 2021 at 10:41 PM Doug Anderson dianders@chromium.org wrote:
At your convenience and when you think there is too little stuff in your sn65dsi86 TODO, check out pinctrl-bcm63xx.c for an example of select GPIO_REGMAP made very simple (this works fine as long as they are bit offsets starting from 0).
I seem to recall you mentioning something like this. When I looked at it in the past I wasn't convinced it would be easy. See my response [2]. The rough summary is that I didn't think the helpers were happy with the pm_runtime() model that I'm using. Did I get that wrong?
Yeah good point. It does seem a bit too complex for that. Sorry for not remembering.
Yours, Linus Walleij
In preparation for splitting this driver into sub-drivers, let's rename the main data structure so it's clear that it's holding data for the whole device and not just the MIPI-eDP bridge part.
This is a no-op change.
Signed-off-by: Douglas Anderson dianders@chromium.org Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org ---
(no changes since v1)
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 86 +++++++++++++-------------- 1 file changed, 43 insertions(+), 43 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 51db30d573c1..f00ceb9dda29 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -112,7 +112,7 @@ #define SN_LINK_TRAINING_TRIES 10
/** - * struct ti_sn_bridge - Platform data for ti-sn65dsi86 driver. + * struct ti_sn65dsi86 - Platform data for ti-sn65dsi86 driver. * @dev: Pointer to our device. * @regmap: Regmap for accessing i2c. * @aux: Our aux channel. @@ -140,7 +140,7 @@ * lock so concurrent users of our 4 GPIOs don't stomp on * each other's read-modify-write. */ -struct ti_sn_bridge { +struct ti_sn65dsi86 { struct device *dev; struct regmap *regmap; struct drm_dp_aux aux; @@ -180,7 +180,7 @@ static const struct regmap_config ti_sn_bridge_regmap_config = { .cache_type = REGCACHE_NONE, };
-static void ti_sn_bridge_write_u16(struct ti_sn_bridge *pdata, +static void ti_sn_bridge_write_u16(struct ti_sn65dsi86 *pdata, unsigned int reg, u16 val) { regmap_write(pdata->regmap, reg, val & 0xFF); @@ -189,7 +189,7 @@ static void ti_sn_bridge_write_u16(struct ti_sn_bridge *pdata,
static int __maybe_unused ti_sn_bridge_resume(struct device *dev) { - struct ti_sn_bridge *pdata = dev_get_drvdata(dev); + struct ti_sn65dsi86 *pdata = dev_get_drvdata(dev); int ret;
ret = regulator_bulk_enable(SN_REGULATOR_SUPPLY_NUM, pdata->supplies); @@ -205,7 +205,7 @@ static int __maybe_unused ti_sn_bridge_resume(struct device *dev)
static int __maybe_unused ti_sn_bridge_suspend(struct device *dev) { - struct ti_sn_bridge *pdata = dev_get_drvdata(dev); + struct ti_sn65dsi86 *pdata = dev_get_drvdata(dev); int ret;
gpiod_set_value(pdata->enable_gpio, 0); @@ -225,7 +225,7 @@ static const struct dev_pm_ops ti_sn_bridge_pm_ops = {
static int status_show(struct seq_file *s, void *data) { - struct ti_sn_bridge *pdata = s->private; + struct ti_sn65dsi86 *pdata = s->private; unsigned int reg, val;
seq_puts(s, "STATUS REGISTERS:\n"); @@ -245,7 +245,7 @@ static int status_show(struct seq_file *s, void *data)
DEFINE_SHOW_ATTRIBUTE(status);
-static void ti_sn_debugfs_init(struct ti_sn_bridge *pdata) +static void ti_sn_debugfs_init(struct ti_sn65dsi86 *pdata) { pdata->debugfs = debugfs_create_dir(dev_name(pdata->dev), NULL);
@@ -253,22 +253,22 @@ static void ti_sn_debugfs_init(struct ti_sn_bridge *pdata) &status_fops); }
-static void ti_sn_debugfs_remove(struct ti_sn_bridge *pdata) +static void ti_sn_debugfs_remove(struct ti_sn65dsi86 *pdata) { debugfs_remove_recursive(pdata->debugfs); pdata->debugfs = NULL; }
/* Connector funcs */ -static struct ti_sn_bridge * +static struct ti_sn65dsi86 * connector_to_ti_sn_bridge(struct drm_connector *connector) { - return container_of(connector, struct ti_sn_bridge, connector); + return container_of(connector, struct ti_sn65dsi86, connector); }
static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector) { - struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector); + struct ti_sn65dsi86 *pdata = connector_to_ti_sn_bridge(connector); struct edid *edid = pdata->edid; int num, ret;
@@ -314,12 +314,12 @@ static const struct drm_connector_funcs ti_sn_bridge_connector_funcs = { .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, };
-static struct ti_sn_bridge *bridge_to_ti_sn_bridge(struct drm_bridge *bridge) +static struct ti_sn65dsi86 *bridge_to_ti_sn_bridge(struct drm_bridge *bridge) { - return container_of(bridge, struct ti_sn_bridge, bridge); + return container_of(bridge, struct ti_sn65dsi86, bridge); }
-static int ti_sn_bridge_parse_regulators(struct ti_sn_bridge *pdata) +static int ti_sn_bridge_parse_regulators(struct ti_sn65dsi86 *pdata) { unsigned int i; const char * const ti_sn_bridge_supply_names[] = { @@ -337,7 +337,7 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flags flags) { int ret, val; - struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge); + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn_bridge(bridge); struct mipi_dsi_host *host; struct mipi_dsi_device *dsi; const struct mipi_dsi_device_info info = { .type = "ti_sn_bridge", @@ -430,7 +430,7 @@ static void ti_sn_bridge_detach(struct drm_bridge *bridge)
static void ti_sn_bridge_disable(struct drm_bridge *bridge) { - struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge); + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn_bridge(bridge);
drm_panel_disable(pdata->panel);
@@ -442,7 +442,7 @@ static void ti_sn_bridge_disable(struct drm_bridge *bridge) regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 0); }
-static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn_bridge *pdata) +static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn65dsi86 *pdata) { u32 bit_rate_khz, clk_freq_khz; struct drm_display_mode *mode = @@ -473,7 +473,7 @@ static const u32 ti_sn_bridge_dsiclk_lut[] = { 460800000, };
-static void ti_sn_bridge_set_refclk_freq(struct ti_sn_bridge *pdata) +static void ti_sn_bridge_set_refclk_freq(struct ti_sn65dsi86 *pdata) { int i; u32 refclk_rate; @@ -500,7 +500,7 @@ static void ti_sn_bridge_set_refclk_freq(struct ti_sn_bridge *pdata) REFCLK_FREQ(i)); }
-static void ti_sn_bridge_set_dsi_rate(struct ti_sn_bridge *pdata) +static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata) { unsigned int bit_rate_mhz, clk_freq_mhz; unsigned int val; @@ -518,7 +518,7 @@ static void ti_sn_bridge_set_dsi_rate(struct ti_sn_bridge *pdata) regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val); }
-static unsigned int ti_sn_bridge_get_bpp(struct ti_sn_bridge *pdata) +static unsigned int ti_sn_bridge_get_bpp(struct ti_sn65dsi86 *pdata) { if (pdata->connector.display_info.bpc <= 6) return 18; @@ -535,7 +535,7 @@ static const unsigned int ti_sn_bridge_dp_rate_lut[] = { 0, 1620, 2160, 2430, 2700, 3240, 4320, 5400 };
-static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn_bridge *pdata) +static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata) { unsigned int bit_rate_khz, dp_rate_mhz; unsigned int i; @@ -556,7 +556,7 @@ static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn_bridge *pdata) return i; }
-static void ti_sn_bridge_read_valid_rates(struct ti_sn_bridge *pdata, +static void ti_sn_bridge_read_valid_rates(struct ti_sn65dsi86 *pdata, bool rate_valid[]) { unsigned int rate_per_200khz; @@ -637,7 +637,7 @@ static void ti_sn_bridge_read_valid_rates(struct ti_sn_bridge *pdata, } }
-static void ti_sn_bridge_set_video_timings(struct ti_sn_bridge *pdata) +static void ti_sn_bridge_set_video_timings(struct ti_sn65dsi86 *pdata) { struct drm_display_mode *mode = &pdata->bridge.encoder->crtc->state->adjusted_mode; @@ -676,7 +676,7 @@ static void ti_sn_bridge_set_video_timings(struct ti_sn_bridge *pdata) usleep_range(10000, 10500); /* 10ms delay recommended by spec */ }
-static unsigned int ti_sn_get_max_lanes(struct ti_sn_bridge *pdata) +static unsigned int ti_sn_get_max_lanes(struct ti_sn65dsi86 *pdata) { u8 data; int ret; @@ -691,7 +691,7 @@ static unsigned int ti_sn_get_max_lanes(struct ti_sn_bridge *pdata) return data & DP_LANE_COUNT_MASK; }
-static int ti_sn_link_training(struct ti_sn_bridge *pdata, int dp_rate_idx, +static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx, const char **last_err_str) { unsigned int val; @@ -751,7 +751,7 @@ static int ti_sn_link_training(struct ti_sn_bridge *pdata, int dp_rate_idx,
static void ti_sn_bridge_enable(struct drm_bridge *bridge) { - struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge); + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn_bridge(bridge); bool rate_valid[ARRAY_SIZE(ti_sn_bridge_dp_rate_lut)] = { }; const char *last_err_str = "No supported DP rate"; int dp_rate_idx; @@ -822,7 +822,7 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge) { - struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge); + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn_bridge(bridge);
pm_runtime_get_sync(pdata->dev);
@@ -853,7 +853,7 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
static void ti_sn_bridge_post_disable(struct drm_bridge *bridge) { - struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge); + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn_bridge(bridge);
drm_panel_unprepare(pdata->panel);
@@ -871,15 +871,15 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = { .post_disable = ti_sn_bridge_post_disable, };
-static struct ti_sn_bridge *aux_to_ti_sn_bridge(struct drm_dp_aux *aux) +static struct ti_sn65dsi86 *aux_to_ti_sn_bridge(struct drm_dp_aux *aux) { - return container_of(aux, struct ti_sn_bridge, aux); + return container_of(aux, struct ti_sn65dsi86, aux); }
static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) { - struct ti_sn_bridge *pdata = aux_to_ti_sn_bridge(aux); + struct ti_sn65dsi86 *pdata = aux_to_ti_sn_bridge(aux); u32 request = msg->request & ~(DP_AUX_I2C_MOT | DP_AUX_I2C_WRITE_STATUS_UPDATE); u32 request_val = AUX_CMD_REQ(msg->request); u8 *buf = msg->buffer; @@ -969,7 +969,7 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux, return len; }
-static int ti_sn_bridge_parse_dsi_host(struct ti_sn_bridge *pdata) +static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata) { struct device_node *np = pdata->dev->of_node;
@@ -1004,7 +1004,7 @@ static int tn_sn_bridge_of_xlate(struct gpio_chip *chip, static int ti_sn_bridge_gpio_get_direction(struct gpio_chip *chip, unsigned int offset) { - struct ti_sn_bridge *pdata = gpiochip_get_data(chip); + struct ti_sn65dsi86 *pdata = gpiochip_get_data(chip);
/* * We already have to keep track of the direction because we use @@ -1018,7 +1018,7 @@ static int ti_sn_bridge_gpio_get_direction(struct gpio_chip *chip,
static int ti_sn_bridge_gpio_get(struct gpio_chip *chip, unsigned int offset) { - struct ti_sn_bridge *pdata = gpiochip_get_data(chip); + struct ti_sn65dsi86 *pdata = gpiochip_get_data(chip); unsigned int val; int ret;
@@ -1043,7 +1043,7 @@ static int ti_sn_bridge_gpio_get(struct gpio_chip *chip, unsigned int offset) static void ti_sn_bridge_gpio_set(struct gpio_chip *chip, unsigned int offset, int val) { - struct ti_sn_bridge *pdata = gpiochip_get_data(chip); + struct ti_sn65dsi86 *pdata = gpiochip_get_data(chip); int ret;
if (!test_bit(offset, pdata->gchip_output)) { @@ -1063,7 +1063,7 @@ static void ti_sn_bridge_gpio_set(struct gpio_chip *chip, unsigned int offset, static int ti_sn_bridge_gpio_direction_input(struct gpio_chip *chip, unsigned int offset) { - struct ti_sn_bridge *pdata = gpiochip_get_data(chip); + struct ti_sn65dsi86 *pdata = gpiochip_get_data(chip); int shift = offset * 2; int ret;
@@ -1091,7 +1091,7 @@ static int ti_sn_bridge_gpio_direction_input(struct gpio_chip *chip, static int ti_sn_bridge_gpio_direction_output(struct gpio_chip *chip, unsigned int offset, int val) { - struct ti_sn_bridge *pdata = gpiochip_get_data(chip); + struct ti_sn65dsi86 *pdata = gpiochip_get_data(chip); int shift = offset * 2; int ret;
@@ -1125,7 +1125,7 @@ static const char * const ti_sn_bridge_gpio_names[SN_NUM_GPIOS] = { "GPIO1", "GPIO2", "GPIO3", "GPIO4" };
-static int ti_sn_setup_gpio_controller(struct ti_sn_bridge *pdata) +static int ti_sn_setup_gpio_controller(struct ti_sn65dsi86 *pdata) { int ret;
@@ -1157,14 +1157,14 @@ static int ti_sn_setup_gpio_controller(struct ti_sn_bridge *pdata)
#else
-static inline int ti_sn_setup_gpio_controller(struct ti_sn_bridge *pdata) +static inline int ti_sn_setup_gpio_controller(struct ti_sn65dsi86 *pdata) { return 0; }
#endif
-static void ti_sn_bridge_parse_lanes(struct ti_sn_bridge *pdata, +static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata, struct device_node *np) { u32 lane_assignments[SN_MAX_DP_LANES] = { 0, 1, 2, 3 }; @@ -1216,7 +1216,7 @@ static void ti_sn_bridge_parse_lanes(struct ti_sn_bridge *pdata, static int ti_sn_bridge_probe(struct i2c_client *client, const struct i2c_device_id *id) { - struct ti_sn_bridge *pdata; + struct ti_sn65dsi86 *pdata; int ret;
if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) { @@ -1224,7 +1224,7 @@ static int ti_sn_bridge_probe(struct i2c_client *client, return -ENODEV; }
- pdata = devm_kzalloc(&client->dev, sizeof(struct ti_sn_bridge), + pdata = devm_kzalloc(&client->dev, sizeof(struct ti_sn65dsi86), GFP_KERNEL); if (!pdata) return -ENOMEM; @@ -1298,7 +1298,7 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
static int ti_sn_bridge_remove(struct i2c_client *client) { - struct ti_sn_bridge *pdata = i2c_get_clientdata(client); + struct ti_sn65dsi86 *pdata = i2c_get_clientdata(client);
if (!pdata) return -EINVAL;
Like the previous patch ("drm/bridge: ti-sn65dsi86: Rename the main driver data structure") this is just a no-op rename in preparation for splitting the driver up a bit.
Here I've attempted to rename functions / structures making sure that anything applicable to the whole chip (instead of just the MIPI to eDP bridge part) included "sn65dsi86" somewhere in the name instead of just "ti_sn_bridge".
Signed-off-by: Douglas Anderson dianders@chromium.org Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org ---
(no changes since v1)
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 84 +++++++++++++-------------- 1 file changed, 42 insertions(+), 42 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index f00ceb9dda29..57574132e200 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -164,30 +164,30 @@ struct ti_sn65dsi86 { #endif };
-static const struct regmap_range ti_sn_bridge_volatile_ranges[] = { +static const struct regmap_range ti_sn65dsi86_volatile_ranges[] = { { .range_min = 0, .range_max = 0xFF }, };
static const struct regmap_access_table ti_sn_bridge_volatile_table = { - .yes_ranges = ti_sn_bridge_volatile_ranges, - .n_yes_ranges = ARRAY_SIZE(ti_sn_bridge_volatile_ranges), + .yes_ranges = ti_sn65dsi86_volatile_ranges, + .n_yes_ranges = ARRAY_SIZE(ti_sn65dsi86_volatile_ranges), };
-static const struct regmap_config ti_sn_bridge_regmap_config = { +static const struct regmap_config ti_sn65dsi86_regmap_config = { .reg_bits = 8, .val_bits = 8, .volatile_table = &ti_sn_bridge_volatile_table, .cache_type = REGCACHE_NONE, };
-static void ti_sn_bridge_write_u16(struct ti_sn65dsi86 *pdata, +static void ti_sn65dsi86_write_u16(struct ti_sn65dsi86 *pdata, unsigned int reg, u16 val) { regmap_write(pdata->regmap, reg, val & 0xFF); regmap_write(pdata->regmap, reg + 1, val >> 8); }
-static int __maybe_unused ti_sn_bridge_resume(struct device *dev) +static int __maybe_unused ti_sn65dsi86_resume(struct device *dev) { struct ti_sn65dsi86 *pdata = dev_get_drvdata(dev); int ret; @@ -203,7 +203,7 @@ static int __maybe_unused ti_sn_bridge_resume(struct device *dev) return ret; }
-static int __maybe_unused ti_sn_bridge_suspend(struct device *dev) +static int __maybe_unused ti_sn65dsi86_suspend(struct device *dev) { struct ti_sn65dsi86 *pdata = dev_get_drvdata(dev); int ret; @@ -217,8 +217,8 @@ static int __maybe_unused ti_sn_bridge_suspend(struct device *dev) return ret; }
-static const struct dev_pm_ops ti_sn_bridge_pm_ops = { - SET_RUNTIME_PM_OPS(ti_sn_bridge_suspend, ti_sn_bridge_resume, NULL) +static const struct dev_pm_ops ti_sn65dsi86_pm_ops = { + SET_RUNTIME_PM_OPS(ti_sn65dsi86_suspend, ti_sn65dsi86_resume, NULL) SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume) }; @@ -245,7 +245,7 @@ static int status_show(struct seq_file *s, void *data)
DEFINE_SHOW_ATTRIBUTE(status);
-static void ti_sn_debugfs_init(struct ti_sn65dsi86 *pdata) +static void ti_sn65dsi86_debugfs_init(struct ti_sn65dsi86 *pdata) { pdata->debugfs = debugfs_create_dir(dev_name(pdata->dev), NULL);
@@ -253,7 +253,7 @@ static void ti_sn_debugfs_init(struct ti_sn65dsi86 *pdata) &status_fops); }
-static void ti_sn_debugfs_remove(struct ti_sn65dsi86 *pdata) +static void ti_sn65dsi86_debugfs_remove(struct ti_sn65dsi86 *pdata) { debugfs_remove_recursive(pdata->debugfs); pdata->debugfs = NULL; @@ -261,14 +261,14 @@ static void ti_sn_debugfs_remove(struct ti_sn65dsi86 *pdata)
/* Connector funcs */ static struct ti_sn65dsi86 * -connector_to_ti_sn_bridge(struct drm_connector *connector) +connector_to_ti_sn65dsi86(struct drm_connector *connector) { return container_of(connector, struct ti_sn65dsi86, connector); }
static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector) { - struct ti_sn65dsi86 *pdata = connector_to_ti_sn_bridge(connector); + struct ti_sn65dsi86 *pdata = connector_to_ti_sn65dsi86(connector); struct edid *edid = pdata->edid; int num, ret;
@@ -314,12 +314,12 @@ static const struct drm_connector_funcs ti_sn_bridge_connector_funcs = { .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, };
-static struct ti_sn65dsi86 *bridge_to_ti_sn_bridge(struct drm_bridge *bridge) +static struct ti_sn65dsi86 *bridge_to_ti_sn65dsi86(struct drm_bridge *bridge) { return container_of(bridge, struct ti_sn65dsi86, bridge); }
-static int ti_sn_bridge_parse_regulators(struct ti_sn65dsi86 *pdata) +static int ti_sn65dsi86_parse_regulators(struct ti_sn65dsi86 *pdata) { unsigned int i; const char * const ti_sn_bridge_supply_names[] = { @@ -337,7 +337,7 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flags flags) { int ret, val; - struct ti_sn65dsi86 *pdata = bridge_to_ti_sn_bridge(bridge); + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge); struct mipi_dsi_host *host; struct mipi_dsi_device *dsi; const struct mipi_dsi_device_info info = { .type = "ti_sn_bridge", @@ -425,12 +425,12 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
static void ti_sn_bridge_detach(struct drm_bridge *bridge) { - drm_dp_aux_unregister(&bridge_to_ti_sn_bridge(bridge)->aux); + drm_dp_aux_unregister(&bridge_to_ti_sn65dsi86(bridge)->aux); }
static void ti_sn_bridge_disable(struct drm_bridge *bridge) { - struct ti_sn65dsi86 *pdata = bridge_to_ti_sn_bridge(bridge); + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
drm_panel_disable(pdata->panel);
@@ -648,9 +648,9 @@ static void ti_sn_bridge_set_video_timings(struct ti_sn65dsi86 *pdata) if (mode->flags & DRM_MODE_FLAG_PVSYNC) vsync_polarity = CHA_VSYNC_POLARITY;
- ti_sn_bridge_write_u16(pdata, SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG, + ti_sn65dsi86_write_u16(pdata, SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG, mode->hdisplay); - ti_sn_bridge_write_u16(pdata, SN_CHA_VERTICAL_DISPLAY_SIZE_LOW_REG, + ti_sn65dsi86_write_u16(pdata, SN_CHA_VERTICAL_DISPLAY_SIZE_LOW_REG, mode->vdisplay); regmap_write(pdata->regmap, SN_CHA_HSYNC_PULSE_WIDTH_LOW_REG, (mode->hsync_end - mode->hsync_start) & 0xFF); @@ -751,7 +751,7 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
static void ti_sn_bridge_enable(struct drm_bridge *bridge) { - struct ti_sn65dsi86 *pdata = bridge_to_ti_sn_bridge(bridge); + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge); bool rate_valid[ARRAY_SIZE(ti_sn_bridge_dp_rate_lut)] = { }; const char *last_err_str = "No supported DP rate"; int dp_rate_idx; @@ -822,7 +822,7 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge) { - struct ti_sn65dsi86 *pdata = bridge_to_ti_sn_bridge(bridge); + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
pm_runtime_get_sync(pdata->dev);
@@ -853,7 +853,7 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
static void ti_sn_bridge_post_disable(struct drm_bridge *bridge) { - struct ti_sn65dsi86 *pdata = bridge_to_ti_sn_bridge(bridge); + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
drm_panel_unprepare(pdata->panel);
@@ -871,7 +871,7 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = { .post_disable = ti_sn_bridge_post_disable, };
-static struct ti_sn65dsi86 *aux_to_ti_sn_bridge(struct drm_dp_aux *aux) +static struct ti_sn65dsi86 *aux_to_ti_sn65dsi86(struct drm_dp_aux *aux) { return container_of(aux, struct ti_sn65dsi86, aux); } @@ -879,7 +879,7 @@ static struct ti_sn65dsi86 *aux_to_ti_sn_bridge(struct drm_dp_aux *aux) static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) { - struct ti_sn65dsi86 *pdata = aux_to_ti_sn_bridge(aux); + struct ti_sn65dsi86 *pdata = aux_to_ti_sn65dsi86(aux); u32 request = msg->request & ~(DP_AUX_I2C_MOT | DP_AUX_I2C_WRITE_STATUS_UPDATE); u32 request_val = AUX_CMD_REQ(msg->request); u8 *buf = msg->buffer; @@ -1213,7 +1213,7 @@ static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata, pdata->ln_polrs = ln_polrs; }
-static int ti_sn_bridge_probe(struct i2c_client *client, +static int ti_sn65dsi86_probe(struct i2c_client *client, const struct i2c_device_id *id) { struct ti_sn65dsi86 *pdata; @@ -1230,7 +1230,7 @@ static int ti_sn_bridge_probe(struct i2c_client *client, return -ENOMEM;
pdata->regmap = devm_regmap_init_i2c(client, - &ti_sn_bridge_regmap_config); + &ti_sn65dsi86_regmap_config); if (IS_ERR(pdata->regmap)) { DRM_ERROR("regmap i2c init failed\n"); return PTR_ERR(pdata->regmap); @@ -1257,7 +1257,7 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
ti_sn_bridge_parse_lanes(pdata, client->dev.of_node);
- ret = ti_sn_bridge_parse_regulators(pdata); + ret = ti_sn65dsi86_parse_regulators(pdata); if (ret) { DRM_ERROR("failed to parse regulators\n"); return ret; @@ -1291,12 +1291,12 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
drm_bridge_add(&pdata->bridge);
- ti_sn_debugfs_init(pdata); + ti_sn65dsi86_debugfs_init(pdata);
return 0; }
-static int ti_sn_bridge_remove(struct i2c_client *client) +static int ti_sn65dsi86_remove(struct i2c_client *client) { struct ti_sn65dsi86 *pdata = i2c_get_clientdata(client);
@@ -1310,7 +1310,7 @@ static int ti_sn_bridge_remove(struct i2c_client *client)
kfree(pdata->edid);
- ti_sn_debugfs_remove(pdata); + ti_sn65dsi86_debugfs_remove(pdata);
drm_bridge_remove(&pdata->bridge);
@@ -1321,29 +1321,29 @@ static int ti_sn_bridge_remove(struct i2c_client *client) return 0; }
-static struct i2c_device_id ti_sn_bridge_id[] = { +static struct i2c_device_id ti_sn65dsi86_id[] = { { "ti,sn65dsi86", 0}, {}, }; -MODULE_DEVICE_TABLE(i2c, ti_sn_bridge_id); +MODULE_DEVICE_TABLE(i2c, ti_sn65dsi86_id);
-static const struct of_device_id ti_sn_bridge_match_table[] = { +static const struct of_device_id ti_sn65dsi86_match_table[] = { {.compatible = "ti,sn65dsi86"}, {}, }; -MODULE_DEVICE_TABLE(of, ti_sn_bridge_match_table); +MODULE_DEVICE_TABLE(of, ti_sn65dsi86_match_table);
-static struct i2c_driver ti_sn_bridge_driver = { +static struct i2c_driver ti_sn65dsi86_driver = { .driver = { .name = "ti_sn65dsi86", - .of_match_table = ti_sn_bridge_match_table, - .pm = &ti_sn_bridge_pm_ops, + .of_match_table = ti_sn65dsi86_match_table, + .pm = &ti_sn65dsi86_pm_ops, }, - .probe = ti_sn_bridge_probe, - .remove = ti_sn_bridge_remove, - .id_table = ti_sn_bridge_id, + .probe = ti_sn65dsi86_probe, + .remove = ti_sn65dsi86_remove, + .id_table = ti_sn65dsi86_id, }; -module_i2c_driver(ti_sn_bridge_driver); +module_i2c_driver(ti_sn65dsi86_driver);
MODULE_AUTHOR("Sandeep Panda spanda@codeaurora.org"); MODULE_DESCRIPTION("sn65dsi86 DSI to eDP bridge driver");
There's no devm_runtime_enable(), but it's easy to use devm_add_action_or_reset() and means we don't need to worry about the disable in our remove() routine or in error paths.
No functional changes intended by this change.
Signed-off-by: Douglas Anderson dianders@chromium.org Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org ---
Changes in v5: - Reordered to debugfs change to avoid transient issue
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 57574132e200..44d8395505f0 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -1213,6 +1213,11 @@ static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata, pdata->ln_polrs = ln_polrs; }
+static void ti_sn65dsi86_runtime_disable(void *data) +{ + pm_runtime_disable(data); +} + static int ti_sn65dsi86_probe(struct i2c_client *client, const struct i2c_device_id *id) { @@ -1272,12 +1277,13 @@ static int ti_sn65dsi86_probe(struct i2c_client *client, return ret;
pm_runtime_enable(pdata->dev); + ret = devm_add_action_or_reset(pdata->dev, ti_sn65dsi86_runtime_disable, pdata->dev); + if (ret) + return ret;
ret = ti_sn_setup_gpio_controller(pdata); - if (ret) { - pm_runtime_disable(pdata->dev); + if (ret) return ret; - }
i2c_set_clientdata(client, pdata);
@@ -1314,8 +1320,6 @@ static int ti_sn65dsi86_remove(struct i2c_client *client)
drm_bridge_remove(&pdata->bridge);
- pm_runtime_disable(pdata->dev); - of_node_put(pdata->host_node);
return 0;
Let's cleanup the debugfs code to: - Check for errors. - Use devm to manage freeing, which also means we don't need to store a pointer in our structure.
Signed-off-by: Douglas Anderson dianders@chromium.org --- Bjorn: I left off your tag on this patch since I made changes compared to v4. Can you re-add if it still looks OK? This is now ordered _after_ the pm_runtime patch so I believe the ordering problem you pointed out should be fixed as well?
Changes in v5: - Don't print debugfs creation errors. - Handle NULL from debugfs_create_dir() which is documented possible.
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 33 +++++++++++++++++---------- 1 file changed, 21 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 44d8395505f0..8aa36074aab9 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -118,7 +118,6 @@ * @aux: Our aux channel. * @bridge: Our bridge. * @connector: Our connector. - * @debugfs: Used for managing our debugfs. * @host_node: Remote DSI node. * @dsi: Our MIPI DSI source. * @edid: Detected EDID of eDP panel. @@ -146,7 +145,6 @@ struct ti_sn65dsi86 { struct drm_dp_aux aux; struct drm_bridge bridge; struct drm_connector connector; - struct dentry *debugfs; struct edid *edid; struct device_node *host_node; struct mipi_dsi_device *dsi; @@ -245,18 +243,31 @@ static int status_show(struct seq_file *s, void *data)
DEFINE_SHOW_ATTRIBUTE(status);
-static void ti_sn65dsi86_debugfs_init(struct ti_sn65dsi86 *pdata) +static void ti_sn65dsi86_debugfs_remove(void *data) { - pdata->debugfs = debugfs_create_dir(dev_name(pdata->dev), NULL); - - debugfs_create_file("status", 0600, pdata->debugfs, pdata, - &status_fops); + debugfs_remove_recursive(data); }
-static void ti_sn65dsi86_debugfs_remove(struct ti_sn65dsi86 *pdata) +static void ti_sn65dsi86_debugfs_init(struct ti_sn65dsi86 *pdata) { - debugfs_remove_recursive(pdata->debugfs); - pdata->debugfs = NULL; + struct device *dev = pdata->dev; + struct dentry *debugfs; + int ret; + + debugfs = debugfs_create_dir(dev_name(dev), NULL); + + /* + * We might get an error back if debugfs wasn't enabled in the kernel + * so let's just silently return upon failure. + */ + if (IS_ERR_OR_NULL(debugfs)) + return; + + ret = devm_add_action_or_reset(dev, ti_sn65dsi86_debugfs_remove, debugfs); + if (ret) + return; + + debugfs_create_file("status", 0600, debugfs, pdata, &status_fops); }
/* Connector funcs */ @@ -1316,8 +1327,6 @@ static int ti_sn65dsi86_remove(struct i2c_client *client)
kfree(pdata->edid);
- ti_sn65dsi86_debugfs_remove(pdata); - drm_bridge_remove(&pdata->bridge);
of_node_put(pdata->host_node);
Tiny cleanup for probe so we don't keep having to specify "&client->dev" or "pdata->dev". No functional changes intended.
Signed-off-by: Douglas Anderson dianders@chromium.org Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org ---
Changes in v5: - Rebased atop the pm_runtime patch, which got reordered.
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 8aa36074aab9..c868193f5b8f 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -1232,6 +1232,7 @@ static void ti_sn65dsi86_runtime_disable(void *data) static int ti_sn65dsi86_probe(struct i2c_client *client, const struct i2c_device_id *id) { + struct device *dev = &client->dev; struct ti_sn65dsi86 *pdata; int ret;
@@ -1240,8 +1241,7 @@ static int ti_sn65dsi86_probe(struct i2c_client *client, return -ENODEV; }
- pdata = devm_kzalloc(&client->dev, sizeof(struct ti_sn65dsi86), - GFP_KERNEL); + pdata = devm_kzalloc(dev, sizeof(struct ti_sn65dsi86), GFP_KERNEL); if (!pdata) return -ENOMEM;
@@ -1252,26 +1252,24 @@ static int ti_sn65dsi86_probe(struct i2c_client *client, return PTR_ERR(pdata->regmap); }
- pdata->dev = &client->dev; + pdata->dev = dev;
- ret = drm_of_find_panel_or_bridge(pdata->dev->of_node, 1, 0, - &pdata->panel, NULL); + ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &pdata->panel, NULL); if (ret) { DRM_ERROR("could not find any panel node\n"); return ret; }
- dev_set_drvdata(&client->dev, pdata); + dev_set_drvdata(dev, pdata);
- pdata->enable_gpio = devm_gpiod_get(pdata->dev, "enable", - GPIOD_OUT_LOW); + pdata->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW); if (IS_ERR(pdata->enable_gpio)) { DRM_ERROR("failed to get enable gpio from DT\n"); ret = PTR_ERR(pdata->enable_gpio); return ret; }
- ti_sn_bridge_parse_lanes(pdata, client->dev.of_node); + ti_sn_bridge_parse_lanes(pdata, dev->of_node);
ret = ti_sn65dsi86_parse_regulators(pdata); if (ret) { @@ -1279,7 +1277,7 @@ static int ti_sn65dsi86_probe(struct i2c_client *client, return ret; }
- pdata->refclk = devm_clk_get_optional(pdata->dev, "refclk"); + pdata->refclk = devm_clk_get_optional(dev, "refclk"); if (IS_ERR(pdata->refclk)) return PTR_ERR(pdata->refclk);
@@ -1287,8 +1285,8 @@ static int ti_sn65dsi86_probe(struct i2c_client *client, if (ret) return ret;
- pm_runtime_enable(pdata->dev); - ret = devm_add_action_or_reset(pdata->dev, ti_sn65dsi86_runtime_disable, pdata->dev); + pm_runtime_enable(dev); + ret = devm_add_action_or_reset(dev, ti_sn65dsi86_runtime_disable, dev); if (ret) return ret;
@@ -1299,12 +1297,12 @@ static int ti_sn65dsi86_probe(struct i2c_client *client, i2c_set_clientdata(client, pdata);
pdata->aux.name = "ti-sn65dsi86-aux"; - pdata->aux.dev = pdata->dev; + pdata->aux.dev = dev; pdata->aux.transfer = ti_sn_aux_transfer; drm_dp_aux_init(&pdata->aux);
pdata->bridge.funcs = &ti_sn_bridge_funcs; - pdata->bridge.of_node = client->dev.of_node; + pdata->bridge.of_node = dev->of_node;
drm_bridge_add(&pdata->bridge);
Let's: - Set the drvdata as soon as it's allocated. This just sets up a pointer so there's no downside here. - Remove the useless call to i2c_set_clientdata() which is literally the same thing as dev_set_drvdata().
No functional changes intended.
Signed-off-by: Douglas Anderson dianders@chromium.org Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org ---
(no changes since v1)
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index c868193f5b8f..75a41198993f 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -1244,6 +1244,8 @@ static int ti_sn65dsi86_probe(struct i2c_client *client, pdata = devm_kzalloc(dev, sizeof(struct ti_sn65dsi86), GFP_KERNEL); if (!pdata) return -ENOMEM; + dev_set_drvdata(dev, pdata); + pdata->dev = dev;
pdata->regmap = devm_regmap_init_i2c(client, &ti_sn65dsi86_regmap_config); @@ -1252,16 +1254,12 @@ static int ti_sn65dsi86_probe(struct i2c_client *client, return PTR_ERR(pdata->regmap); }
- pdata->dev = dev; - ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &pdata->panel, NULL); if (ret) { DRM_ERROR("could not find any panel node\n"); return ret; }
- dev_set_drvdata(dev, pdata); - pdata->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW); if (IS_ERR(pdata->enable_gpio)) { DRM_ERROR("failed to get enable gpio from DT\n"); @@ -1294,8 +1292,6 @@ static int ti_sn65dsi86_probe(struct i2c_client *client, if (ret) return ret;
- i2c_set_clientdata(client, pdata); - pdata->aux.name = "ti-sn65dsi86-aux"; pdata->aux.dev = dev; pdata->aux.transfer = ti_sn_aux_transfer;
This is just code motion of the probe routine to move all the things that are for the "whole chip" (instead of the GPIO parts or the MIPI-to-eDP parts) together at the start of probe. This is in preparation for breaking the driver into sub-drivers.
Since we're using devm for all of the "whole chip" stuff this is actually quite easy now.
Signed-off-by: Douglas Anderson dianders@chromium.org Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org ---
(no changes since v1)
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 75a41198993f..68673f736b23 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -1254,12 +1254,6 @@ static int ti_sn65dsi86_probe(struct i2c_client *client, return PTR_ERR(pdata->regmap); }
- ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &pdata->panel, NULL); - if (ret) { - DRM_ERROR("could not find any panel node\n"); - return ret; - } - pdata->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW); if (IS_ERR(pdata->enable_gpio)) { DRM_ERROR("failed to get enable gpio from DT\n"); @@ -1267,8 +1261,6 @@ static int ti_sn65dsi86_probe(struct i2c_client *client, return ret; }
- ti_sn_bridge_parse_lanes(pdata, dev->of_node); - ret = ti_sn65dsi86_parse_regulators(pdata); if (ret) { DRM_ERROR("failed to parse regulators\n"); @@ -1279,12 +1271,22 @@ static int ti_sn65dsi86_probe(struct i2c_client *client, if (IS_ERR(pdata->refclk)) return PTR_ERR(pdata->refclk);
- ret = ti_sn_bridge_parse_dsi_host(pdata); + pm_runtime_enable(dev); + ret = devm_add_action_or_reset(dev, ti_sn65dsi86_runtime_disable, dev); if (ret) return ret;
- pm_runtime_enable(dev); - ret = devm_add_action_or_reset(dev, ti_sn65dsi86_runtime_disable, dev); + ti_sn65dsi86_debugfs_init(pdata); + + ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &pdata->panel, NULL); + if (ret) { + DRM_ERROR("could not find any panel node\n"); + return ret; + } + + ti_sn_bridge_parse_lanes(pdata, dev->of_node); + + ret = ti_sn_bridge_parse_dsi_host(pdata); if (ret) return ret;
@@ -1302,8 +1304,6 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
drm_bridge_add(&pdata->bridge);
- ti_sn65dsi86_debugfs_init(pdata); - return 0; }
Let's use the newly minted aux bus to break up the driver into sub drivers. We're not doing a full breakup here: all the code is still in the same file and remains largely untouched. The big goal here of using sub-drivers is to allow part of our code to finish probing even if some other code needs to defer. This can solve some chicken-and-egg problems. Specifically: - In commit 48834e6084f1 ("drm/panel-simple: Support hpd-gpios for delaying prepare()") we had to add a bit of a hack to simpel-panel to support HPD showing up late. We can get rid of that hack now since the GPIO part of our driver can finish probing early. - We have a desire to expose our DDC bus to simple-panel (and perhaps to a backlight driver?). That will end up with the same chicken-and-egg problem. A future patch to move this to a sub-driver will fix it. - If/when we support the PWM functionality present in the bridge chip for a backlight we'll end up with another chicken-and-egg problem. If we allow the PWM to be a sub-driver too then it solves this problem.
Signed-off-by: Douglas Anderson dianders@chromium.org ---
Changes in v5: - Fix module compile problems (Bjorn + kbuild bot) - Remove useless MODULE_DEVICE_TABLE (Bjorn).
drivers/gpu/drm/bridge/Kconfig | 1 + drivers/gpu/drm/bridge/ti-sn65dsi86.c | 252 ++++++++++++++++++++------ 2 files changed, 200 insertions(+), 53 deletions(-)
diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig index d907a91a2ee8..bdec664f27ec 100644 --- a/drivers/gpu/drm/bridge/Kconfig +++ b/drivers/gpu/drm/bridge/Kconfig @@ -275,6 +275,7 @@ config DRM_TI_SN65DSI86 select REGMAP_I2C select DRM_PANEL select DRM_MIPI_DSI + select AUXILIARY_BUS help Texas Instruments SN65DSI86 DSI to eDP Bridge driver
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 68673f736b23..0bd1a1d1453e 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -4,6 +4,7 @@ * datasheet: https://www.ti.com/lit/ds/symlink/sn65dsi86.pdf */
+#include <linux/auxiliary_bus.h> #include <linux/bits.h> #include <linux/clk.h> #include <linux/debugfs.h> @@ -113,7 +114,10 @@
/** * struct ti_sn65dsi86 - Platform data for ti-sn65dsi86 driver. - * @dev: Pointer to our device. + * @bridge_aux: AUX-bus sub device for MIPI-to-eDP bridge functionality. + * @gpio_aux: AUX-bus sub device for GPIO controller functionality. + * + * @dev: Pointer to the top level (i2c) device. * @regmap: Regmap for accessing i2c. * @aux: Our aux channel. * @bridge: Our bridge. @@ -140,6 +144,9 @@ * each other's read-modify-write. */ struct ti_sn65dsi86 { + struct auxiliary_device bridge_aux; + struct auxiliary_device gpio_aux; + struct device *dev; struct regmap *regmap; struct drm_dp_aux aux; @@ -1136,8 +1143,10 @@ static const char * const ti_sn_bridge_gpio_names[SN_NUM_GPIOS] = { "GPIO1", "GPIO2", "GPIO3", "GPIO4" };
-static int ti_sn_setup_gpio_controller(struct ti_sn65dsi86 *pdata) +static int ti_sn_gpio_probe(struct auxiliary_device *adev, + const struct auxiliary_device_id *id) { + struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent); int ret;
/* Only init if someone is going to use us as a GPIO controller */ @@ -1159,20 +1168,41 @@ static int ti_sn_setup_gpio_controller(struct ti_sn65dsi86 *pdata) pdata->gchip.names = ti_sn_bridge_gpio_names; pdata->gchip.ngpio = SN_NUM_GPIOS; pdata->gchip.base = -1; - ret = devm_gpiochip_add_data(pdata->dev, &pdata->gchip, pdata); + ret = devm_gpiochip_add_data(&adev->dev, &pdata->gchip, pdata); if (ret) dev_err(pdata->dev, "can't add gpio chip\n");
return ret; }
-#else +static const struct auxiliary_device_id ti_sn_gpio_id_table[] = { + { .name = "ti_sn65dsi86.gpio", }, + {}, +};
-static inline int ti_sn_setup_gpio_controller(struct ti_sn65dsi86 *pdata) +MODULE_DEVICE_TABLE(auxiliary, ti_sn_gpio_id_table); + +static struct auxiliary_driver ti_sn_gpio_driver = { + .name = "gpio", + .probe = ti_sn_gpio_probe, + .id_table = ti_sn_gpio_id_table, +}; + +static int __init ti_sn_gpio_register(void) { - return 0; + return auxiliary_driver_register(&ti_sn_gpio_driver); }
+static void __exit ti_sn_gpio_unregister(void) +{ + auxiliary_driver_unregister(&ti_sn_gpio_driver); +} + +#else + +static inline int ti_sn_gpio_register(void) { return 0; } +static inline void ti_sn_gpio_unregister(void) {} + #endif
static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata, @@ -1224,11 +1254,124 @@ static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata, pdata->ln_polrs = ln_polrs; }
+static int ti_sn_bridge_probe(struct auxiliary_device *adev, + const struct auxiliary_device_id *id) +{ + struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent); + struct device_node *np = pdata->dev->of_node; + int ret; + + ret = drm_of_find_panel_or_bridge(np, 1, 0, &pdata->panel, NULL); + if (ret) { + DRM_ERROR("could not find any panel node\n"); + return ret; + } + + ti_sn_bridge_parse_lanes(pdata, np); + + ret = ti_sn_bridge_parse_dsi_host(pdata); + if (ret) + return ret; + + pdata->aux.name = "ti-sn65dsi86-aux"; + pdata->aux.dev = pdata->dev; + pdata->aux.transfer = ti_sn_aux_transfer; + drm_dp_aux_init(&pdata->aux); + + pdata->bridge.funcs = &ti_sn_bridge_funcs; + pdata->bridge.of_node = np; + + drm_bridge_add(&pdata->bridge); + + return 0; +} + +static void ti_sn_bridge_remove(struct auxiliary_device *adev) +{ + struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent); + + if (!pdata) + return; + + if (pdata->dsi) { + mipi_dsi_detach(pdata->dsi); + mipi_dsi_device_unregister(pdata->dsi); + } + + kfree(pdata->edid); + + drm_bridge_remove(&pdata->bridge); + + of_node_put(pdata->host_node); +} + +static const struct auxiliary_device_id ti_sn_bridge_id_table[] = { + { .name = "ti_sn65dsi86.bridge", }, + {}, +}; + +static struct auxiliary_driver ti_sn_bridge_driver = { + .name = "bridge", + .probe = ti_sn_bridge_probe, + .remove = ti_sn_bridge_remove, + .id_table = ti_sn_bridge_id_table, +}; + static void ti_sn65dsi86_runtime_disable(void *data) { pm_runtime_disable(data); }
+static void ti_sn65dsi86_uninit_aux(void *data) +{ + auxiliary_device_uninit(data); +} + +static void ti_sn65dsi86_delete_aux(void *data) +{ + auxiliary_device_delete(data); +} + +/* + * AUX bus docs say that a non-NULL release is mandatory, but it makes no + * sense for the model used here where all of the aux devices are allocated + * in the single shared structure. We'll use this noop as a workaround. + */ +static void ti_sn65dsi86_noop(struct device *dev) {} + +static int ti_sn65dsi86_add_aux_device(struct ti_sn65dsi86 *pdata, + struct auxiliary_device *aux, + const char *name) +{ + struct device *dev = pdata->dev; + int ret; + + /* + * NOTE: It would be nice to set the "of_node" of our children to be + * the same "of_node"" that the top-level component has. That doesn't + * work, though, since pinctrl will try (and fail) to reserve the + * pins again. Until that gets sorted out the children will just need + * to look at the of_node of the main device. + */ + + aux->name = name; + aux->dev.parent = dev; + aux->dev.release = ti_sn65dsi86_noop; + ret = auxiliary_device_init(aux); + if (ret) + return ret; + ret = devm_add_action_or_reset(dev, ti_sn65dsi86_uninit_aux, aux); + if (ret) + return ret; + + ret = auxiliary_device_add(aux); + if (ret) + return ret; + ret = devm_add_action_or_reset(dev, ti_sn65dsi86_delete_aux, aux); + + return ret; +} + static int ti_sn65dsi86_probe(struct i2c_client *client, const struct i2c_device_id *id) { @@ -1278,54 +1421,24 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
ti_sn65dsi86_debugfs_init(pdata);
- ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &pdata->panel, NULL); - if (ret) { - DRM_ERROR("could not find any panel node\n"); - return ret; - } - - ti_sn_bridge_parse_lanes(pdata, dev->of_node); - - ret = ti_sn_bridge_parse_dsi_host(pdata); - if (ret) - return ret; - - ret = ti_sn_setup_gpio_controller(pdata); - if (ret) - return ret; - - pdata->aux.name = "ti-sn65dsi86-aux"; - pdata->aux.dev = dev; - pdata->aux.transfer = ti_sn_aux_transfer; - drm_dp_aux_init(&pdata->aux); - - pdata->bridge.funcs = &ti_sn_bridge_funcs; - pdata->bridge.of_node = dev->of_node; - - drm_bridge_add(&pdata->bridge); - - return 0; -} - -static int ti_sn65dsi86_remove(struct i2c_client *client) -{ - struct ti_sn65dsi86 *pdata = i2c_get_clientdata(client); - - if (!pdata) - return -EINVAL; + /* + * Break ourselves up into a collection of aux devices. The only real + * motiviation here is to solve the chicken-and-egg problem of probe + * ordering. The bridge wants the panel to be there when it probes. + * The panel wants its HPD GPIO (provided by sn65dsi86 on some boards) + * when it probes. There will soon be other devices (DDC I2C bus, PWM) + * that have the same problem. Having sub-devices allows the some sub + * devices to finish probing even if others return -EPROBE_DEFER and + * gets us around the problems. + */
- if (pdata->dsi) { - mipi_dsi_detach(pdata->dsi); - mipi_dsi_device_unregister(pdata->dsi); + if (IS_ENABLED(CONFIG_OF_GPIO)) { + ret = ti_sn65dsi86_add_aux_device(pdata, &pdata->gpio_aux, "gpio"); + if (ret) + return ret; }
- kfree(pdata->edid); - - drm_bridge_remove(&pdata->bridge); - - of_node_put(pdata->host_node); - - return 0; + return ti_sn65dsi86_add_aux_device(pdata, &pdata->bridge_aux, "bridge"); }
static struct i2c_device_id ti_sn65dsi86_id[] = { @@ -1347,10 +1460,43 @@ static struct i2c_driver ti_sn65dsi86_driver = { .pm = &ti_sn65dsi86_pm_ops, }, .probe = ti_sn65dsi86_probe, - .remove = ti_sn65dsi86_remove, .id_table = ti_sn65dsi86_id, }; -module_i2c_driver(ti_sn65dsi86_driver); + +static int __init ti_sn65dsi86_init(void) +{ + int ret; + + ret = i2c_add_driver(&ti_sn65dsi86_driver); + if (ret) + return ret; + + ret = ti_sn_gpio_register(); + if (ret) + goto err_main_was_registered; + + ret = auxiliary_driver_register(&ti_sn_bridge_driver); + if (ret) + goto err_gpio_was_registered; + + return 0; + +err_gpio_was_registered: + ti_sn_gpio_unregister(); +err_main_was_registered: + i2c_del_driver(&ti_sn65dsi86_driver); + + return ret; +} +module_init(ti_sn65dsi86_init); + +static void __exit ti_sn65dsi86_exit(void) +{ + auxiliary_driver_unregister(&ti_sn_bridge_driver); + ti_sn_gpio_unregister(); + i2c_del_driver(&ti_sn65dsi86_driver); +} +module_exit(ti_sn65dsi86_exit);
MODULE_AUTHOR("Sandeep Panda spanda@codeaurora.org"); MODULE_DESCRIPTION("sn65dsi86 DSI to eDP bridge driver");
On Fri, Apr 23, 2021 at 6:59 PM Douglas Anderson dianders@chromium.org wrote:
Let's use the newly minted aux bus to break up the driver into sub drivers. We're not doing a full breakup here: all the code is still in the same file and remains largely untouched. The big goal here of using sub-drivers is to allow part of our code to finish probing even if some other code needs to defer. This can solve some chicken-and-egg problems. Specifically:
- In commit 48834e6084f1 ("drm/panel-simple: Support hpd-gpios for delaying prepare()") we had to add a bit of a hack to simpel-panel to support HPD showing up late. We can get rid of that hack now since the GPIO part of our driver can finish probing early.
- We have a desire to expose our DDC bus to simple-panel (and perhaps to a backlight driver?). That will end up with the same chicken-and-egg problem. A future patch to move this to a sub-driver will fix it.
- If/when we support the PWM functionality present in the bridge chip for a backlight we'll end up with another chicken-and-egg problem. If we allow the PWM to be a sub-driver too then it solves this problem.
Signed-off-by: Douglas Anderson dianders@chromium.org
Changes in v5:
- Fix module compile problems (Bjorn + kbuild bot)
- Remove useless MODULE_DEVICE_TABLE (Bjorn).
This is generally a good idea. I have no idea when to use auxbus or MFD but I trust that you researched that so: Acked-by: Linus Walleij linus.walleij@linaro.org
Yours, Linus Walleij
Hi,
On Sat, May 1, 2021 at 4:59 AM Linus Walleij linus.walleij@linaro.org wrote:
On Fri, Apr 23, 2021 at 6:59 PM Douglas Anderson dianders@chromium.org wrote:
Let's use the newly minted aux bus to break up the driver into sub drivers. We're not doing a full breakup here: all the code is still in the same file and remains largely untouched. The big goal here of using sub-drivers is to allow part of our code to finish probing even if some other code needs to defer. This can solve some chicken-and-egg problems. Specifically:
- In commit 48834e6084f1 ("drm/panel-simple: Support hpd-gpios for delaying prepare()") we had to add a bit of a hack to simpel-panel to support HPD showing up late. We can get rid of that hack now since the GPIO part of our driver can finish probing early.
- We have a desire to expose our DDC bus to simple-panel (and perhaps to a backlight driver?). That will end up with the same chicken-and-egg problem. A future patch to move this to a sub-driver will fix it.
- If/when we support the PWM functionality present in the bridge chip for a backlight we'll end up with another chicken-and-egg problem. If we allow the PWM to be a sub-driver too then it solves this problem.
Signed-off-by: Douglas Anderson dianders@chromium.org
Changes in v5:
- Fix module compile problems (Bjorn + kbuild bot)
- Remove useless MODULE_DEVICE_TABLE (Bjorn).
This is generally a good idea. I have no idea when to use auxbus or MFD
It was a bit hard for me to figure out too. I think historically this could have been implemented by MFD but I believe that the point of introducing the AUX bus was that MFD wasn't a great fit for things like this. It's talked about a bit in "Documentation/driver-api/auxiliary_bus.rst". For me the important thing here is that we think of the bridge chip as one device, not a collection of IP blocks glued together in one package. As some evidence, the DT bindings don't have sub-nodes for this. There's a single DT node that says that this one device is the bridge, is a GPIO controller, and can provide a PWM.
but I trust that you researched that so: Acked-by: Linus Walleij linus.walleij@linaro.org
Thanks! I'll land it then to whittle the patch stack down to just the controversial EDID one.
-Doug
When I added support for the hpd-gpio to simple-panel in commit 48834e6084f1 ("drm/panel-simple: Support hpd-gpios for delaying prepare()"), I added a special case to handle a circular dependency I was running into on the ti-sn65dsi86 bridge chip. On my board the hpd-gpio is actually provided by the bridge chip. That was causing some circular dependency problems that I had to work around by getting the hpd-gpio late.
I've now reorganized the ti-sn65dsi86 bridge chip driver to be a collection of sub-drivers. Now the GPIO part can probe separately and that breaks the chain. Let's get rid of the old code to clean things up.
Signed-off-by: Douglas Anderson dianders@chromium.org Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org ---
(no changes since v1)
drivers/gpu/drm/panel/panel-simple.c | 24 +++++------------------- 1 file changed, 5 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index 9746eda6f675..bd208abcbf07 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -366,8 +366,7 @@ static int panel_simple_unprepare(struct drm_panel *panel) return 0; }
-static int panel_simple_get_hpd_gpio(struct device *dev, - struct panel_simple *p, bool from_probe) +static int panel_simple_get_hpd_gpio(struct device *dev, struct panel_simple *p) { int err;
@@ -375,17 +374,10 @@ static int panel_simple_get_hpd_gpio(struct device *dev, if (IS_ERR(p->hpd_gpio)) { err = PTR_ERR(p->hpd_gpio);
- /* - * If we're called from probe we won't consider '-EPROBE_DEFER' - * to be an error--we'll leave the error code in "hpd_gpio". - * When we try to use it we'll try again. This allows for - * circular dependencies where the component providing the - * hpd gpio needs the panel to init before probing. - */ - if (err != -EPROBE_DEFER || !from_probe) { + if (err != -EPROBE_DEFER) dev_err(dev, "failed to get 'hpd' GPIO: %d\n", err); - return err; - } + + return err; }
return 0; @@ -416,12 +408,6 @@ static int panel_simple_prepare_once(struct panel_simple *p) msleep(delay);
if (p->hpd_gpio) { - if (IS_ERR(p->hpd_gpio)) { - err = panel_simple_get_hpd_gpio(dev, p, false); - if (err) - goto error; - } - if (p->desc->delay.hpd_absent_delay) hpd_wait_us = p->desc->delay.hpd_absent_delay * 1000UL; else @@ -682,7 +668,7 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
panel->no_hpd = of_property_read_bool(dev->of_node, "no-hpd"); if (!panel->no_hpd) { - err = panel_simple_get_hpd_gpio(dev, panel, true); + err = panel_simple_get_hpd_gpio(dev, panel); if (err) return err; }
On Fri, Apr 23, 2021 at 1:00 PM Douglas Anderson dianders@chromium.org wrote:
When I added support for the hpd-gpio to simple-panel in commit 48834e6084f1 ("drm/panel-simple: Support hpd-gpios for delaying prepare()"), I added a special case to handle a circular dependency I was running into on the ti-sn65dsi86 bridge chip. On my board the hpd-gpio is actually provided by the bridge chip. That was causing some circular dependency problems that I had to work around by getting the hpd-gpio late.
I've now reorganized the ti-sn65dsi86 bridge chip driver to be a collection of sub-drivers. Now the GPIO part can probe separately and that breaks the chain. Let's get rid of the old code to clean things up.
Signed-off-by: Douglas Anderson dianders@chromium.org Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org
Reviewed-by: Sean Paul seanpaul@chromium.org
(no changes since v1)
drivers/gpu/drm/panel/panel-simple.c | 24 +++++------------------- 1 file changed, 5 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index 9746eda6f675..bd208abcbf07 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -366,8 +366,7 @@ static int panel_simple_unprepare(struct drm_panel *panel) return 0; }
-static int panel_simple_get_hpd_gpio(struct device *dev,
struct panel_simple *p, bool from_probe)+static int panel_simple_get_hpd_gpio(struct device *dev, struct panel_simple *p) { int err;
@@ -375,17 +374,10 @@ static int panel_simple_get_hpd_gpio(struct device *dev, if (IS_ERR(p->hpd_gpio)) { err = PTR_ERR(p->hpd_gpio);
/** If we're called from probe we won't consider '-EPROBE_DEFER'* to be an error--we'll leave the error code in "hpd_gpio".* When we try to use it we'll try again. This allows for* circular dependencies where the component providing the* hpd gpio needs the panel to init before probing.*/if (err != -EPROBE_DEFER || !from_probe) {
if (err != -EPROBE_DEFER) dev_err(dev, "failed to get 'hpd' GPIO: %d\n", err);
return err;}
return err; } return 0;@@ -416,12 +408,6 @@ static int panel_simple_prepare_once(struct panel_simple *p) msleep(delay);
if (p->hpd_gpio) {
if (IS_ERR(p->hpd_gpio)) {err = panel_simple_get_hpd_gpio(dev, p, false);if (err)goto error;}if (p->desc->delay.hpd_absent_delay) hpd_wait_us = p->desc->delay.hpd_absent_delay * 1000UL; else@@ -682,7 +668,7 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
panel->no_hpd = of_property_read_bool(dev->of_node, "no-hpd"); if (!panel->no_hpd) {
err = panel_simple_get_hpd_gpio(dev, panel, true);
err = panel_simple_get_hpd_gpio(dev, panel); if (err) return err; }-- 2.31.1.498.g6c1eba8ee3d-goog
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Let's make the bridge use autosuspend with a 500ms delay. This is in preparation for promoting DP AUX transfers to their own sub-driver so that we're not constantly powering up and down the device as we transfer all the chunks.
Signed-off-by: Douglas Anderson dianders@chromium.org Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org ---
(no changes since v1)
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 0bd1a1d1453e..49b76b2ffe25 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -243,7 +243,7 @@ static int status_show(struct seq_file *s, void *data) seq_printf(s, "[0x%02x] = 0x%08x\n", reg, val); }
- pm_runtime_put(pdata->dev); + pm_runtime_put_autosuspend(pdata->dev);
return 0; } @@ -293,7 +293,7 @@ static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector) if (!edid) { pm_runtime_get_sync(pdata->dev); edid = pdata->edid = drm_get_edid(connector, &pdata->aux.ddc); - pm_runtime_put(pdata->dev); + pm_runtime_put_autosuspend(pdata->dev); }
if (edid && drm_edid_is_valid(edid)) { @@ -419,7 +419,7 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge, /* check if continuous dsi clock is required or not */ pm_runtime_get_sync(pdata->dev); regmap_read(pdata->regmap, SN_DPPLL_SRC_REG, &val); - pm_runtime_put(pdata->dev); + pm_runtime_put_autosuspend(pdata->dev); if (!(val & DPPLL_CLK_SRC_DSICLK)) dsi->mode_flags |= MIPI_DSI_CLOCK_NON_CONTINUOUS;
@@ -1050,7 +1050,7 @@ static int ti_sn_bridge_gpio_get(struct gpio_chip *chip, unsigned int offset) */ pm_runtime_get_sync(pdata->dev); ret = regmap_read(pdata->regmap, SN_GPIO_IO_REG, &val); - pm_runtime_put(pdata->dev); + pm_runtime_put_autosuspend(pdata->dev);
if (ret) return ret; @@ -1101,7 +1101,7 @@ static int ti_sn_bridge_gpio_direction_input(struct gpio_chip *chip, * it off and when it comes back it will have lost all state, but * that's OK because the default is input and we're now an input. */ - pm_runtime_put(pdata->dev); + pm_runtime_put_autosuspend(pdata->dev);
return 0; } @@ -1127,7 +1127,7 @@ static int ti_sn_bridge_gpio_direction_output(struct gpio_chip *chip, SN_GPIO_MUX_OUTPUT << shift); if (ret) { clear_bit(offset, pdata->gchip_output); - pm_runtime_put(pdata->dev); + pm_runtime_put_autosuspend(pdata->dev); }
return ret; @@ -1418,6 +1418,8 @@ static int ti_sn65dsi86_probe(struct i2c_client *client, ret = devm_add_action_or_reset(dev, ti_sn65dsi86_runtime_disable, dev); if (ret) return ret; + pm_runtime_set_autosuspend_delay(pdata->dev, 500); + pm_runtime_use_autosuspend(pdata->dev);
ti_sn65dsi86_debugfs_init(pdata);
No functional changes--this just makes the diffstat of a future change easier to understand.
Signed-off-by: Douglas Anderson dianders@chromium.org Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org ---
(no changes since v1)
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 116 +++++++++++++------------- 1 file changed, 58 insertions(+), 58 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 49b76b2ffe25..db367793cdff 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -192,6 +192,64 @@ static void ti_sn65dsi86_write_u16(struct ti_sn65dsi86 *pdata, regmap_write(pdata->regmap, reg + 1, val >> 8); }
+static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn65dsi86 *pdata) +{ + u32 bit_rate_khz, clk_freq_khz; + struct drm_display_mode *mode = + &pdata->bridge.encoder->crtc->state->adjusted_mode; + + bit_rate_khz = mode->clock * + mipi_dsi_pixel_format_to_bpp(pdata->dsi->format); + clk_freq_khz = bit_rate_khz / (pdata->dsi->lanes * 2); + + return clk_freq_khz; +} + +/* clk frequencies supported by bridge in Hz in case derived from REFCLK pin */ +static const u32 ti_sn_bridge_refclk_lut[] = { + 12000000, + 19200000, + 26000000, + 27000000, + 38400000, +}; + +/* clk frequencies supported by bridge in Hz in case derived from DACP/N pin */ +static const u32 ti_sn_bridge_dsiclk_lut[] = { + 468000000, + 384000000, + 416000000, + 486000000, + 460800000, +}; + +static void ti_sn_bridge_set_refclk_freq(struct ti_sn65dsi86 *pdata) +{ + int i; + u32 refclk_rate; + const u32 *refclk_lut; + size_t refclk_lut_size; + + if (pdata->refclk) { + refclk_rate = clk_get_rate(pdata->refclk); + refclk_lut = ti_sn_bridge_refclk_lut; + refclk_lut_size = ARRAY_SIZE(ti_sn_bridge_refclk_lut); + clk_prepare_enable(pdata->refclk); + } else { + refclk_rate = ti_sn_bridge_get_dsi_freq(pdata) * 1000; + refclk_lut = ti_sn_bridge_dsiclk_lut; + refclk_lut_size = ARRAY_SIZE(ti_sn_bridge_dsiclk_lut); + } + + /* for i equals to refclk_lut_size means default frequency */ + for (i = 0; i < refclk_lut_size; i++) + if (refclk_lut[i] == refclk_rate) + break; + + regmap_update_bits(pdata->regmap, SN_DPPLL_SRC_REG, REFCLK_FREQ_MASK, + REFCLK_FREQ(i)); +} + static int __maybe_unused ti_sn65dsi86_resume(struct device *dev) { struct ti_sn65dsi86 *pdata = dev_get_drvdata(dev); @@ -460,64 +518,6 @@ static void ti_sn_bridge_disable(struct drm_bridge *bridge) regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 0); }
-static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn65dsi86 *pdata) -{ - u32 bit_rate_khz, clk_freq_khz; - struct drm_display_mode *mode = - &pdata->bridge.encoder->crtc->state->adjusted_mode; - - bit_rate_khz = mode->clock * - mipi_dsi_pixel_format_to_bpp(pdata->dsi->format); - clk_freq_khz = bit_rate_khz / (pdata->dsi->lanes * 2); - - return clk_freq_khz; -} - -/* clk frequencies supported by bridge in Hz in case derived from REFCLK pin */ -static const u32 ti_sn_bridge_refclk_lut[] = { - 12000000, - 19200000, - 26000000, - 27000000, - 38400000, -}; - -/* clk frequencies supported by bridge in Hz in case derived from DACP/N pin */ -static const u32 ti_sn_bridge_dsiclk_lut[] = { - 468000000, - 384000000, - 416000000, - 486000000, - 460800000, -}; - -static void ti_sn_bridge_set_refclk_freq(struct ti_sn65dsi86 *pdata) -{ - int i; - u32 refclk_rate; - const u32 *refclk_lut; - size_t refclk_lut_size; - - if (pdata->refclk) { - refclk_rate = clk_get_rate(pdata->refclk); - refclk_lut = ti_sn_bridge_refclk_lut; - refclk_lut_size = ARRAY_SIZE(ti_sn_bridge_refclk_lut); - clk_prepare_enable(pdata->refclk); - } else { - refclk_rate = ti_sn_bridge_get_dsi_freq(pdata) * 1000; - refclk_lut = ti_sn_bridge_dsiclk_lut; - refclk_lut_size = ARRAY_SIZE(ti_sn_bridge_dsiclk_lut); - } - - /* for i equals to refclk_lut_size means default frequency */ - for (i = 0; i < refclk_lut_size; i++) - if (refclk_lut[i] == refclk_rate) - break; - - regmap_update_bits(pdata->regmap, SN_DPPLL_SRC_REG, REFCLK_FREQ_MASK, - REFCLK_FREQ(i)); -} - static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata) { unsigned int bit_rate_mhz, clk_freq_mhz;
Let's reorganize how we init and turn on the reference clock in the code to allow us to turn it on early (even before pre_enable()) so that we can read the EDID early. This is handy for eDP because: - We always assume that a panel is there. - Once we report that a panel is there we get asked to read the EDID. - Pre-enable isn't called until we know what pixel clock we want to use and we're ready to turn everything on. That's _after_ we get asked to read the EDID.
NOTE: the above only works out OK if we "refclk" is provided. Though I don't have access to any hardware that uses ti-sn65dsi86 and _doesn't_ provide a "refclk", I believe that we'll have trouble reading the EDID at bootup in that case. Specifically I believe that if there's no "refclk" we need the MIPI source clock to be active before we can successfully read the EDID. My evidence here is that, in testing, I couldn't read the EDID until I turned on the DPPLL in the bridge chip and that the DPPLL needs the input clock to be active.
Since this is hard to support, let's punt trying to handle this case if there's no "refclk". In that case we'll enable comms in pre_enable() like we always did.
I don't believe there are any users of the ti-sn65dsi86 bridge chip that _don't_ use "refclk". The bridge chip is _very_ inflexible in that mode. The only time I've seen that mode used was for some really early prototype hardware that was thrown in the e-waste bin years ago when we realized how inflexible it was.
Even if someone is using the bridge chip without the "refclk" they're in no worse shape than they were before the (fairly recent) commit 58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over DDC").
Signed-off-by: Douglas Anderson dianders@chromium.org Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org ---
(no changes since v1)
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 129 +++++++++++++++++++------- 1 file changed, 94 insertions(+), 35 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index db367793cdff..9dc3cd8e17df 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -132,6 +132,8 @@ * @dp_lanes: Count of dp_lanes we're using. * @ln_assign: Value to program to the LN_ASSIGN register. * @ln_polrs: Value for the 4-bit LN_POLRS field of SN_ENH_FRAME_REG. + * @comms_enabled: If true then communication over the aux channel is enabled. + * @comms_mutex: Protects modification of comms_enabled. * * @gchip: If we expose our GPIOs, this is used. * @gchip_output: A cache of whether we've set GPIOs to output. This @@ -162,6 +164,8 @@ struct ti_sn65dsi86 { int dp_lanes; u8 ln_assign; u8 ln_polrs; + bool comms_enabled; + struct mutex comms_mutex;
#if defined(CONFIG_OF_GPIO) struct gpio_chip gchip; @@ -250,6 +254,47 @@ static void ti_sn_bridge_set_refclk_freq(struct ti_sn65dsi86 *pdata) REFCLK_FREQ(i)); }
+static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata) +{ + mutex_lock(&pdata->comms_mutex); + + /* configure bridge ref_clk */ + ti_sn_bridge_set_refclk_freq(pdata); + + /* + * HPD on this bridge chip is a bit useless. This is an eDP bridge + * so the HPD is an internal signal that's only there to signal that + * the panel is done powering up. ...but the bridge chip debounces + * this signal by between 100 ms and 400 ms (depending on process, + * voltage, and temperate--I measured it at about 200 ms). One + * particular panel asserted HPD 84 ms after it was powered on meaning + * that we saw HPD 284 ms after power on. ...but the same panel said + * that instead of looking at HPD you could just hardcode a delay of + * 200 ms. We'll assume that the panel driver will have the hardcoded + * delay in its prepare and always disable HPD. + * + * If HPD somehow makes sense on some future panel we'll have to + * change this to be conditional on someone specifying that HPD should + * be used. + */ + regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE, + HPD_DISABLE); + + pdata->comms_enabled = true; + + mutex_unlock(&pdata->comms_mutex); +} + +static void ti_sn65dsi86_disable_comms(struct ti_sn65dsi86 *pdata) +{ + mutex_lock(&pdata->comms_mutex); + + pdata->comms_enabled = false; + clk_disable_unprepare(pdata->refclk); + + mutex_unlock(&pdata->comms_mutex); +} + static int __maybe_unused ti_sn65dsi86_resume(struct device *dev) { struct ti_sn65dsi86 *pdata = dev_get_drvdata(dev); @@ -263,6 +308,16 @@ static int __maybe_unused ti_sn65dsi86_resume(struct device *dev)
gpiod_set_value(pdata->enable_gpio, 1);
+ /* + * If we have a reference clock we can enable communication w/ the + * panel (including the aux channel) w/out any need for an input clock + * so we can do it in resume which lets us read the EDID before + * pre_enable(). Without a reference clock we need the MIPI reference + * clock so reading early doesn't work. + */ + if (pdata->refclk) + ti_sn65dsi86_enable_comms(pdata); + return ret; }
@@ -271,6 +326,9 @@ static int __maybe_unused ti_sn65dsi86_suspend(struct device *dev) struct ti_sn65dsi86 *pdata = dev_get_drvdata(dev); int ret;
+ if (pdata->refclk) + ti_sn65dsi86_disable_comms(pdata); + gpiod_set_value(pdata->enable_gpio, 0);
ret = regulator_bulk_disable(SN_REGULATOR_SUPPLY_NUM, pdata->supplies); @@ -844,27 +902,8 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
pm_runtime_get_sync(pdata->dev);
- /* configure bridge ref_clk */ - ti_sn_bridge_set_refclk_freq(pdata); - - /* - * HPD on this bridge chip is a bit useless. This is an eDP bridge - * so the HPD is an internal signal that's only there to signal that - * the panel is done powering up. ...but the bridge chip debounces - * this signal by between 100 ms and 400 ms (depending on process, - * voltage, and temperate--I measured it at about 200 ms). One - * particular panel asserted HPD 84 ms after it was powered on meaning - * that we saw HPD 284 ms after power on. ...but the same panel said - * that instead of looking at HPD you could just hardcode a delay of - * 200 ms. We'll assume that the panel driver will have the hardcoded - * delay in its prepare and always disable HPD. - * - * If HPD somehow makes sense on some future panel we'll have to - * change this to be conditional on someone specifying that HPD should - * be used. - */ - regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE, - HPD_DISABLE); + if (!pdata->refclk) + ti_sn65dsi86_enable_comms(pdata);
drm_panel_prepare(pdata->panel); } @@ -875,7 +914,8 @@ static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
drm_panel_unprepare(pdata->panel);
- clk_disable_unprepare(pdata->refclk); + if (!pdata->refclk) + ti_sn65dsi86_disable_comms(pdata);
pm_runtime_put_sync(pdata->dev); } @@ -909,6 +949,20 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux, if (len > SN_AUX_MAX_PAYLOAD_BYTES) return -EINVAL;
+ pm_runtime_get_sync(pdata->dev); + mutex_lock(&pdata->comms_mutex); + + /* + * If someone tries to do a DDC over AUX transaction before pre_enable() + * on a device without a dedicated reference clock then we just can't + * do it. Fail right away. This prevents non-refclk users from reading + * the EDID before enabling the panel but such is life. + */ + if (!pdata->comms_enabled) { + ret = -EIO; + goto exit; + } + switch (request) { case DP_AUX_NATIVE_WRITE: case DP_AUX_I2C_WRITE: @@ -919,7 +973,8 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux, msg->reply = 0; break; default: - return -EINVAL; + ret = -EINVAL; + goto exit; }
BUILD_BUG_ON(sizeof(addr_len) != sizeof(__be32)); @@ -943,11 +998,11 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux, ret = regmap_read_poll_timeout(pdata->regmap, SN_AUX_CMD_REG, val, !(val & AUX_CMD_SEND), 0, 50 * 1000); if (ret) - return ret; + goto exit;
ret = regmap_read(pdata->regmap, SN_AUX_CMD_STATUS_REG, &val); if (ret) - return ret; + goto exit;
if (val & AUX_IRQ_STATUS_AUX_RPLY_TOUT) { /* @@ -955,13 +1010,14 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux, * but it hit a timeout. We ignore defers here because they're * handled in hardware. */ - return -ETIMEDOUT; + ret = -ETIMEDOUT; + goto exit; }
if (val & AUX_IRQ_STATUS_AUX_SHORT) { ret = regmap_read(pdata->regmap, SN_AUX_LENGTH_REG, &len); if (ret) - return ret; + goto exit; } else if (val & AUX_IRQ_STATUS_NAT_I2C_FAIL) { switch (request) { case DP_AUX_I2C_WRITE: @@ -973,18 +1029,19 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux, msg->reply |= DP_AUX_NATIVE_REPLY_NACK; break; } - return 0; + len = 0; + goto exit; }
- if (request == DP_AUX_NATIVE_WRITE || request == DP_AUX_I2C_WRITE || - len == 0) - return len; + if (request != DP_AUX_NATIVE_WRITE && request != DP_AUX_I2C_WRITE && len != 0) + ret = regmap_bulk_read(pdata->regmap, SN_AUX_RDATA_REG(0), buf, len);
- ret = regmap_bulk_read(pdata->regmap, SN_AUX_RDATA_REG(0), buf, len); - if (ret) - return ret; +exit: + mutex_unlock(&pdata->comms_mutex); + pm_runtime_mark_last_busy(pdata->dev); + pm_runtime_put_autosuspend(pdata->dev);
- return len; + return ret ? ret : len; }
static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata) @@ -1390,6 +1447,8 @@ static int ti_sn65dsi86_probe(struct i2c_client *client, dev_set_drvdata(dev, pdata); pdata->dev = dev;
+ mutex_init(&pdata->comms_mutex); + pdata->regmap = devm_regmap_init_i2c(client, &ti_sn65dsi86_regmap_config); if (IS_ERR(pdata->regmap)) {
We'd like to be able to expose the DDC-over-AUX channel bus to our panel. This gets into a chicken-and-egg problem because: - The panel wants to get its DDC at probe time. - The ti-sn65dsi86 MIPI-to-eDP bridge code, which provides the DDC bus, wants to get the panel at probe time.
By using a sub device we can fully create the AUX channel bits so that the panel can get them. Then the panel can finish probing and the bridge can probe.
To accomplish this, we also move registering the AUX channel out of the bridge's attach code and do it right at probe time. We use devm to manage cleanup.
NOTE: there's a little bit of a trick here. Though the AUX channel can run without the MIPI-to-eDP bits of the code, the MIPI-to-eDP bits can't run without the AUX channel. We could come up a complicated signaling scheme (have the MIPI-to-eDP bits return EPROBE_DEFER for a while or wait on some sort of completion), but it seems simple enough to just not even bother creating the bridge device until the AUX channel probes. That's what we'll do.
Signed-off-by: Douglas Anderson dianders@chromium.org ---
Changes in v5: - Fix module compile problems (Bjorn + kbuild bot) - Remove useless MODULE_DEVICE_TABLE (Bjorn).
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 87 +++++++++++++++++++++------ 1 file changed, 67 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 9dc3cd8e17df..3539ddf9d109 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -116,6 +116,7 @@ * struct ti_sn65dsi86 - Platform data for ti-sn65dsi86 driver. * @bridge_aux: AUX-bus sub device for MIPI-to-eDP bridge functionality. * @gpio_aux: AUX-bus sub device for GPIO controller functionality. + * @aux_aux: AUX-bus sub device for eDP AUX channel functionality. * * @dev: Pointer to the top level (i2c) device. * @regmap: Regmap for accessing i2c. @@ -148,6 +149,7 @@ struct ti_sn65dsi86 { struct auxiliary_device bridge_aux; struct auxiliary_device gpio_aux; + struct auxiliary_device aux_aux;
struct device *dev; struct regmap *regmap; @@ -484,18 +486,12 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge, return -EINVAL; }
- ret = drm_dp_aux_register(&pdata->aux); - if (ret < 0) { - drm_err(bridge->dev, "Failed to register DP AUX channel: %d\n", ret); - return ret; - } - ret = drm_connector_init(bridge->dev, &pdata->connector, &ti_sn_bridge_connector_funcs, DRM_MODE_CONNECTOR_eDP); if (ret) { DRM_ERROR("Failed to initialize connector with drm\n"); - goto err_conn_init; + return ret; }
drm_connector_helper_add(&pdata->connector, @@ -552,8 +548,6 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge, mipi_dsi_device_unregister(dsi); err_dsi_host: drm_connector_cleanup(&pdata->connector); -err_conn_init: - drm_dp_aux_unregister(&pdata->aux); return ret; }
@@ -1330,11 +1324,6 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev, if (ret) return ret;
- pdata->aux.name = "ti-sn65dsi86-aux"; - pdata->aux.dev = pdata->dev; - pdata->aux.transfer = ti_sn_aux_transfer; - drm_dp_aux_init(&pdata->aux); - pdata->bridge.funcs = &ti_sn_bridge_funcs; pdata->bridge.of_node = np;
@@ -1429,6 +1418,50 @@ static int ti_sn65dsi86_add_aux_device(struct ti_sn65dsi86 *pdata, return ret; }
+static void ti_sn65dsi86_unregister_dp_aux(void *data) +{ + drm_dp_aux_unregister(data); +} + +static int ti_sn_aux_probe(struct auxiliary_device *adev, + const struct auxiliary_device_id *id) +{ + struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent); + int ret; + + pdata->aux.name = "ti-sn65dsi86-aux"; + pdata->aux.dev = pdata->dev; + pdata->aux.transfer = ti_sn_aux_transfer; + drm_dp_aux_init(&pdata->aux); + + ret = drm_dp_aux_register(&pdata->aux); + if (ret < 0) { + drm_err(pdata, "Failed to register DP AUX channel: %d\n", ret); + return ret; + } + ret = devm_add_action_or_reset(&adev->dev, + ti_sn65dsi86_unregister_dp_aux, &pdata->aux); + if (ret) + return ret; + + /* + * The eDP to MIPI bridge parts don't work until the AUX channel is + * setup so we don't add it in the main driver probe, we add it now. + */ + return ti_sn65dsi86_add_aux_device(pdata, &pdata->bridge_aux, "bridge"); +} + +static const struct auxiliary_device_id ti_sn_aux_id_table[] = { + { .name = "ti_sn65dsi86.aux", }, + {}, +}; + +static struct auxiliary_driver ti_sn_aux_driver = { + .name = "aux", + .probe = ti_sn_aux_probe, + .id_table = ti_sn_aux_id_table, +}; + static int ti_sn65dsi86_probe(struct i2c_client *client, const struct i2c_device_id *id) { @@ -1487,10 +1520,11 @@ static int ti_sn65dsi86_probe(struct i2c_client *client, * motiviation here is to solve the chicken-and-egg problem of probe * ordering. The bridge wants the panel to be there when it probes. * The panel wants its HPD GPIO (provided by sn65dsi86 on some boards) - * when it probes. There will soon be other devices (DDC I2C bus, PWM) - * that have the same problem. Having sub-devices allows the some sub - * devices to finish probing even if others return -EPROBE_DEFER and - * gets us around the problems. + * when it probes. The panel and maybe backlight might want the DDC + * bus. Soon the PWM provided by the bridge chip will have the same + * problem. Having sub-devices allows the some sub devices to finish + * probing even if others return -EPROBE_DEFER and gets us around the + * problems. */
if (IS_ENABLED(CONFIG_OF_GPIO)) { @@ -1499,7 +1533,13 @@ static int ti_sn65dsi86_probe(struct i2c_client *client, return ret; }
- return ti_sn65dsi86_add_aux_device(pdata, &pdata->bridge_aux, "bridge"); + /* + * NOTE: At the end of the AUX channel probe we'll add the aux device + * for the bridge. This is because the bridge can't be used until the + * AUX channel is there and this is a very simple solution to the + * dependency problem. + */ + return ti_sn65dsi86_add_aux_device(pdata, &pdata->aux_aux, "aux"); }
static struct i2c_device_id ti_sn65dsi86_id[] = { @@ -1536,12 +1576,18 @@ static int __init ti_sn65dsi86_init(void) if (ret) goto err_main_was_registered;
- ret = auxiliary_driver_register(&ti_sn_bridge_driver); + ret = auxiliary_driver_register(&ti_sn_aux_driver); if (ret) goto err_gpio_was_registered;
+ ret = auxiliary_driver_register(&ti_sn_bridge_driver); + if (ret) + goto err_aux_was_registered; + return 0;
+err_aux_was_registered: + auxiliary_driver_unregister(&ti_sn_aux_driver); err_gpio_was_registered: ti_sn_gpio_unregister(); err_main_was_registered: @@ -1554,6 +1600,7 @@ module_init(ti_sn65dsi86_init); static void __exit ti_sn65dsi86_exit(void) { auxiliary_driver_unregister(&ti_sn_bridge_driver); + auxiliary_driver_unregister(&ti_sn_aux_driver); ti_sn_gpio_unregister(); i2c_del_driver(&ti_sn65dsi86_driver); }
Hi,
On Fri, Apr 23, 2021 at 10:00 AM Douglas Anderson dianders@chromium.org wrote:
We'd like to be able to expose the DDC-over-AUX channel bus to our panel. This gets into a chicken-and-egg problem because:
- The panel wants to get its DDC at probe time.
- The ti-sn65dsi86 MIPI-to-eDP bridge code, which provides the DDC bus, wants to get the panel at probe time.
By using a sub device we can fully create the AUX channel bits so that the panel can get them. Then the panel can finish probing and the bridge can probe.
To accomplish this, we also move registering the AUX channel out of the bridge's attach code and do it right at probe time. We use devm to manage cleanup.
NOTE: there's a little bit of a trick here. Though the AUX channel can run without the MIPI-to-eDP bits of the code, the MIPI-to-eDP bits can't run without the AUX channel. We could come up a complicated signaling scheme (have the MIPI-to-eDP bits return EPROBE_DEFER for a while or wait on some sort of completion), but it seems simple enough to just not even bother creating the bridge device until the AUX channel probes. That's what we'll do.
Signed-off-by: Douglas Anderson dianders@chromium.org
Changes in v5:
- Fix module compile problems (Bjorn + kbuild bot)
- Remove useless MODULE_DEVICE_TABLE (Bjorn).
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 87 +++++++++++++++++++++------ 1 file changed, 67 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 9dc3cd8e17df..3539ddf9d109 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -116,6 +116,7 @@
- struct ti_sn65dsi86 - Platform data for ti-sn65dsi86 driver.
- @bridge_aux: AUX-bus sub device for MIPI-to-eDP bridge functionality.
- @gpio_aux: AUX-bus sub device for GPIO controller functionality.
- @aux_aux: AUX-bus sub device for eDP AUX channel functionality.
- @dev: Pointer to the top level (i2c) device.
- @regmap: Regmap for accessing i2c.
@@ -148,6 +149,7 @@ struct ti_sn65dsi86 { struct auxiliary_device bridge_aux; struct auxiliary_device gpio_aux;
struct auxiliary_device aux_aux; struct device *dev; struct regmap *regmap;@@ -484,18 +486,12 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge, return -EINVAL; }
ret = drm_dp_aux_register(&pdata->aux);if (ret < 0) {drm_err(bridge->dev, "Failed to register DP AUX channel: %d\n", ret);return ret;}ret = drm_connector_init(bridge->dev, &pdata->connector, &ti_sn_bridge_connector_funcs, DRM_MODE_CONNECTOR_eDP); if (ret) { DRM_ERROR("Failed to initialize connector with drm\n");goto err_conn_init;
return ret; } drm_connector_helper_add(&pdata->connector,@@ -552,8 +548,6 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge, mipi_dsi_device_unregister(dsi); err_dsi_host: drm_connector_cleanup(&pdata->connector); -err_conn_init:
drm_dp_aux_unregister(&pdata->aux); return ret;}
@@ -1330,11 +1324,6 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev, if (ret) return ret;
pdata->aux.name = "ti-sn65dsi86-aux";pdata->aux.dev = pdata->dev;pdata->aux.transfer = ti_sn_aux_transfer;drm_dp_aux_init(&pdata->aux);pdata->bridge.funcs = &ti_sn_bridge_funcs; pdata->bridge.of_node = np;@@ -1429,6 +1418,50 @@ static int ti_sn65dsi86_add_aux_device(struct ti_sn65dsi86 *pdata, return ret; }
+static void ti_sn65dsi86_unregister_dp_aux(void *data) +{
drm_dp_aux_unregister(data);+}
+static int ti_sn_aux_probe(struct auxiliary_device *adev,
const struct auxiliary_device_id *id)+{
struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent);int ret;pdata->aux.name = "ti-sn65dsi86-aux";pdata->aux.dev = pdata->dev;pdata->aux.transfer = ti_sn_aux_transfer;drm_dp_aux_init(&pdata->aux);ret = drm_dp_aux_register(&pdata->aux);if (ret < 0) {drm_err(pdata, "Failed to register DP AUX channel: %d\n", ret);return ret;}ret = devm_add_action_or_reset(&adev->dev,ti_sn65dsi86_unregister_dp_aux, &pdata->aux);if (ret)return ret;/** The eDP to MIPI bridge parts don't work until the AUX channel is* setup so we don't add it in the main driver probe, we add it now.*/return ti_sn65dsi86_add_aux_device(pdata, &pdata->bridge_aux, "bridge");+}
+static const struct auxiliary_device_id ti_sn_aux_id_table[] = {
{ .name = "ti_sn65dsi86.aux", },{},+};
+static struct auxiliary_driver ti_sn_aux_driver = {
.name = "aux",.probe = ti_sn_aux_probe,.id_table = ti_sn_aux_id_table,+};
static int ti_sn65dsi86_probe(struct i2c_client *client, const struct i2c_device_id *id) { @@ -1487,10 +1520,11 @@ static int ti_sn65dsi86_probe(struct i2c_client *client, * motiviation here is to solve the chicken-and-egg problem of probe * ordering. The bridge wants the panel to be there when it probes. * The panel wants its HPD GPIO (provided by sn65dsi86 on some boards)
* when it probes. There will soon be other devices (DDC I2C bus, PWM)* that have the same problem. Having sub-devices allows the some sub* devices to finish probing even if others return -EPROBE_DEFER and* gets us around the problems.
* when it probes. The panel and maybe backlight might want the DDC* bus. Soon the PWM provided by the bridge chip will have the same* problem. Having sub-devices allows the some sub devices to finish* probing even if others return -EPROBE_DEFER and gets us around the* problems. */ if (IS_ENABLED(CONFIG_OF_GPIO)) {@@ -1499,7 +1533,13 @@ static int ti_sn65dsi86_probe(struct i2c_client *client, return ret; }
return ti_sn65dsi86_add_aux_device(pdata, &pdata->bridge_aux, "bridge");
/** NOTE: At the end of the AUX channel probe we'll add the aux device* for the bridge. This is because the bridge can't be used until the* AUX channel is there and this is a very simple solution to the* dependency problem.*/return ti_sn65dsi86_add_aux_device(pdata, &pdata->aux_aux, "aux");}
static struct i2c_device_id ti_sn65dsi86_id[] = { @@ -1536,12 +1576,18 @@ static int __init ti_sn65dsi86_init(void) if (ret) goto err_main_was_registered;
ret = auxiliary_driver_register(&ti_sn_bridge_driver);
ret = auxiliary_driver_register(&ti_sn_aux_driver); if (ret) goto err_gpio_was_registered;ret = auxiliary_driver_register(&ti_sn_bridge_driver);if (ret)goto err_aux_was_registered;return 0;+err_aux_was_registered:
auxiliary_driver_unregister(&ti_sn_aux_driver);err_gpio_was_registered: ti_sn_gpio_unregister(); err_main_was_registered: @@ -1554,6 +1600,7 @@ module_init(ti_sn65dsi86_init); static void __exit ti_sn65dsi86_exit(void) { auxiliary_driver_unregister(&ti_sn_bridge_driver);
auxiliary_driver_unregister(&ti_sn_aux_driver); ti_sn_gpio_unregister(); i2c_del_driver(&ti_sn65dsi86_driver);}
Ugh, more fun! :(
I tried rebasing this to the latest drm-misc-next and I found commit 6cba3fe43341 ("drm/dp: Add backpointer to drm_device in drm_dp_aux"). That commit makes it pretty explicit that we shouldn't call drm_dp_aux_register() until we actually have a "drm_dev" for the bridge.
I'm applying several of the other patches in this series but I won't apply this one or anything based on it. I'll do some digging and send out a proposed fix shortly.
-Doug
The of_find_i2c_adapter_by_node() could end up failing to find an adapter in certain conditions. Specifically it's possible that of_dev_or_parent_node_match() could end up finding an I2C client in the list and cause bus_find_device() to stop early even though an I2C adapter was present later in the list.
Let's move the i2c_verify_adapter() into the predicate function to prevent this. Now we'll properly skip over the I2C client and be able to find the I2C adapter.
This issue has always been a potential problem if a single device tree node could represent both an I2C client and an adapter. I believe this is a sane thing to do if, for instance, an I2C-connected DP bridge chip is present. The bridge chip is an I2C client but it can also provide an I2C adapter (DDC tunneled over AUX channel). We don't want to have to create a sub-node just so a panel can link to it with the "ddc-i2c-bus" property.
I believe that this problem got worse, however, with commit e814e688413a ("i2c: of: Try to find an I2C adapter matching the parent"). Starting at that commit it would be even easier to accidentally miss finding the adapter.
Signed-off-by: Douglas Anderson dianders@chromium.org Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org --- This commit is sorta just jammed into the middle of my series. It has no dependencies on the earlier patches in the series and I think it can land independently in the i2c tree. Later patches in the series won't work right without this one, but they won't crash. If we can't find the i2c bus we'll just fall back to the hardcoded panel modes which, at least today, all panels have.
I'll also note that part of me wonders if we should actually fix this further to run two passes through everything: first look to see if we find an exact match and only look at the parent pointer if there is no match. I don't currently have a need for that and it's a slightly bigger change, but it seems conceivable that it could affect someone?
(no changes since v1)
drivers/i2c/i2c-core-of.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c index 3ed74aa4b44b..de0bf5fce3a2 100644 --- a/drivers/i2c/i2c-core-of.c +++ b/drivers/i2c/i2c-core-of.c @@ -124,6 +124,14 @@ static int of_dev_or_parent_node_match(struct device *dev, const void *data) return 0; }
+static int of_i2c_adapter_match(struct device *dev, const void *data) +{ + if (!of_dev_or_parent_node_match(dev, data)) + return 0; + + return !!i2c_verify_adapter(dev); +} + /* must call put_device() when done with returned i2c_client device */ struct i2c_client *of_find_i2c_device_by_node(struct device_node *node) { @@ -146,18 +154,13 @@ EXPORT_SYMBOL(of_find_i2c_device_by_node); struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node) { struct device *dev; - struct i2c_adapter *adapter;
dev = bus_find_device(&i2c_bus_type, NULL, node, - of_dev_or_parent_node_match); + of_i2c_adapter_match); if (!dev) return NULL;
- adapter = i2c_verify_adapter(dev); - if (!adapter) - put_device(dev); - - return adapter; + return to_i2c_adapter(dev); } EXPORT_SYMBOL(of_find_i2c_adapter_by_node);
As of commit 5186421cbfe2 ("drm: Introduce epoch counter to drm_connector") the drm_get_edid() function calls drm_connector_update_edid_property() for us. There's no reason for us to call it again.
Signed-off-by: Douglas Anderson dianders@chromium.org Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org --- As Laurent pointed out [1] this is actually a pretty common problem. His suggestion to do this more broadly is a good idea but this series is probably a bit ambitious already so I would suggest that be taken up separately.
[1] https://lore.kernel.org/r/YGphgcESWsozCi1y@pendragon.ideasonboard.com
(no changes since v1)
drivers/gpu/drm/panel/panel-simple.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index bd208abcbf07..4de33c929a59 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -512,7 +512,6 @@ static int panel_simple_get_modes(struct drm_panel *panel, if (p->ddc) { struct edid *edid = drm_get_edid(connector, p->ddc);
- drm_connector_update_edid_property(connector, edid); if (edid) { num += drm_add_edid_modes(connector, edid); kfree(edid);
On Fri, Apr 23, 2021 at 1:00 PM Douglas Anderson dianders@chromium.org wrote:
As of commit 5186421cbfe2 ("drm: Introduce epoch counter to drm_connector") the drm_get_edid() function calls drm_connector_update_edid_property() for us. There's no reason for us to call it again.
Signed-off-by: Douglas Anderson dianders@chromium.org Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org
Reviewed-by: Sean Paul seanpaul@chromium.org
As Laurent pointed out [1] this is actually a pretty common problem. His suggestion to do this more broadly is a good idea but this series is probably a bit ambitious already so I would suggest that be taken up separately.
[1] https://lore.kernel.org/r/YGphgcESWsozCi1y@pendragon.ideasonboard.com
(no changes since v1)
drivers/gpu/drm/panel/panel-simple.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index bd208abcbf07..4de33c929a59 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -512,7 +512,6 @@ static int panel_simple_get_modes(struct drm_panel *panel, if (p->ddc) { struct edid *edid = drm_get_edid(connector, p->ddc);
drm_connector_update_edid_property(connector, edid); if (edid) { num += drm_add_edid_modes(connector, edid); kfree(edid);-- 2.31.1.498.g6c1eba8ee3d-goog
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
I don't believe that it ever makes sense to read the EDID when a panel is not powered and the powering on of the panel is the job of prepare(). Let's make sure that this happens before we try to read the EDID. We use the pm_runtime functions directly rather than directly calling the normal prepare() function because the pm_runtime functions are definitely refcounted whereas it's less clear if the prepare() one is.
NOTE: I'm not 100% sure how EDID reading was working for folks in the past, but I can only assume that it was failing on the initial attempt and then working only later. This patch, presumably, will fix that. If some panel out there really can read the EDID without powering up and it's a big advantage to preserve the old behavior we can add a per-panel flag. It appears that providing the DDC bus to the panel in the past was somewhat uncommon in any case.
Signed-off-by: Douglas Anderson dianders@chromium.org Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org ---
(no changes since v1)
drivers/gpu/drm/panel/panel-simple.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index 4de33c929a59..a12dfe8b8d90 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -510,12 +510,18 @@ static int panel_simple_get_modes(struct drm_panel *panel,
/* probe EDID if a DDC bus is available */ if (p->ddc) { - struct edid *edid = drm_get_edid(connector, p->ddc); + struct edid *edid;
+ pm_runtime_get_sync(panel->dev); + + edid = drm_get_edid(connector, p->ddc); if (edid) { num += drm_add_edid_modes(connector, edid); kfree(edid); } + + pm_runtime_mark_last_busy(panel->dev); + pm_runtime_put_autosuspend(panel->dev); }
/* add hard-coded panel modes */
On Fri, Apr 23, 2021 at 1:00 PM Douglas Anderson dianders@chromium.org wrote:
I don't believe that it ever makes sense to read the EDID when a panel is not powered and the powering on of the panel is the job of prepare(). Let's make sure that this happens before we try to read the EDID. We use the pm_runtime functions directly rather than directly calling the normal prepare() function because the pm_runtime functions are definitely refcounted whereas it's less clear if the prepare() one is.
NOTE: I'm not 100% sure how EDID reading was working for folks in the past, but I can only assume that it was failing on the initial attempt and then working only later. This patch, presumably, will fix that. If some panel out there really can read the EDID without powering up and it's a big advantage to preserve the old behavior we can add a per-panel flag. It appears that providing the DDC bus to the panel in the past was somewhat uncommon in any case.
Maybe some combination of drivers caching the EDID for panels while they're already powered and overly broad pm runtime references?
At any rate, this makes sense to me,
Reviewed-by: Sean Paul seanpaul@chromium.org
Signed-off-by: Douglas Anderson dianders@chromium.org Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org
(no changes since v1)
drivers/gpu/drm/panel/panel-simple.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index 4de33c929a59..a12dfe8b8d90 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -510,12 +510,18 @@ static int panel_simple_get_modes(struct drm_panel *panel,
/* probe EDID if a DDC bus is available */ if (p->ddc) {
struct edid *edid = drm_get_edid(connector, p->ddc);
struct edid *edid;pm_runtime_get_sync(panel->dev);edid = drm_get_edid(connector, p->ddc); if (edid) { num += drm_add_edid_modes(connector, edid); kfree(edid); }pm_runtime_mark_last_busy(panel->dev);pm_runtime_put_autosuspend(panel->dev); } /* add hard-coded panel modes */-- 2.31.1.498.g6c1eba8ee3d-goog
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
It doesn't make sense to go out to the bus and read the EDID over and over again. Let's cache it and throw away the cache when we turn power off from the panel. Autosuspend means that even if there are several calls to read the EDID before we officially turn the power on then we should get good use out of this cache.
Signed-off-by: Douglas Anderson dianders@chromium.org Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org ---
(no changes since v1)
drivers/gpu/drm/panel/panel-simple.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index a12dfe8b8d90..9be050ab372f 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -189,6 +189,8 @@ struct panel_simple { struct gpio_desc *enable_gpio; struct gpio_desc *hpd_gpio;
+ struct edid *edid; + struct drm_display_mode override_mode;
enum drm_panel_orientation orientation; @@ -345,6 +347,9 @@ static int panel_simple_suspend(struct device *dev) regulator_disable(p->supply); p->unprepared_time = ktime_get();
+ kfree(p->edid); + p->edid = NULL; + return 0; }
@@ -510,15 +515,13 @@ static int panel_simple_get_modes(struct drm_panel *panel,
/* probe EDID if a DDC bus is available */ if (p->ddc) { - struct edid *edid; - pm_runtime_get_sync(panel->dev);
- edid = drm_get_edid(connector, p->ddc); - if (edid) { - num += drm_add_edid_modes(connector, edid); - kfree(edid); - } + if (!p->edid) + p->edid = drm_get_edid(connector, p->ddc); + + if (p->edid) + num += drm_add_edid_modes(connector, p->edid);
pm_runtime_mark_last_busy(panel->dev); pm_runtime_put_autosuspend(panel->dev);
On Fri, Apr 23, 2021 at 1:00 PM Douglas Anderson dianders@chromium.org wrote:
It doesn't make sense to go out to the bus and read the EDID over and over again. Let's cache it and throw away the cache when we turn power off from the panel. Autosuspend means that even if there are several calls to read the EDID before we officially turn the power on then we should get good use out of this cache.
I think i915 caches the edid once on init and never refreshes it (assuming no hotplugs). That said, I think it makes sense for a more conservative approach in panel-simple.
Signed-off-by: Douglas Anderson dianders@chromium.org Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org
(no changes since v1)
drivers/gpu/drm/panel/panel-simple.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index a12dfe8b8d90..9be050ab372f 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -189,6 +189,8 @@ struct panel_simple { struct gpio_desc *enable_gpio; struct gpio_desc *hpd_gpio;
struct edid *edid;struct drm_display_mode override_mode; enum drm_panel_orientation orientation;@@ -345,6 +347,9 @@ static int panel_simple_suspend(struct device *dev) regulator_disable(p->supply); p->unprepared_time = ktime_get();
kfree(p->edid);p->edid = NULL;return 0;}
@@ -510,15 +515,13 @@ static int panel_simple_get_modes(struct drm_panel *panel,
/* probe EDID if a DDC bus is available */ if (p->ddc) {
struct edid *edid;pm_runtime_get_sync(panel->dev);edid = drm_get_edid(connector, p->ddc);if (edid) {num += drm_add_edid_modes(connector, edid);kfree(edid);}
if (!p->edid)p->edid = drm_get_edid(connector, p->ddc);if (p->edid)num += drm_add_edid_modes(connector, p->edid);
I suppose this would keep banging on the ddc if drm_get_edid() continuously returns NULL, but maybe that's desireable (it'll succeed sometime in the future)? At any rate, this is an improvement on current behavior so it has my vote.
Reviewed-by: Sean Paul seanpaul@chromium.org
pm_runtime_mark_last_busy(panel->dev); pm_runtime_put_autosuspend(panel->dev);-- 2.31.1.498.g6c1eba8ee3d-goog
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
This is really just a revert of commit 58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over DDC"), resolving conflicts.
The old code failed to read the EDID properly in a very important case: before the bridge's pre_enable() was called. The way things need to work: 1. Read the EDID. 2. Based on the EDID, decide on video settings and pixel clock. 3. Enable the bridge w/ the desired settings.
The way things were working: 1. Try to read the EDID but fail; fall back to hardcoded values. 2. Based on hardcoded values, decide on video settings and pixel clock. 3. Enable the bridge w/ the desired settings. 4. Try again to read the EDID, it works now! 5. Realize that the hardcoded settings weren't quite right. 6. Disable / reenable the bridge w/ the right settings.
The reasons for the failures were twofold: a) Since we never ran the bridge chip's pre-enable then we never set the bit to ignore HPD. This meant the bridge chip didn't even _try_ to go out on the bus and communicate with the panel. b) Even if we fixed things to ignore HPD, the EDID still wouldn't read if the panel wasn't on.
Instead of reverting the code, we could fix it to set the HPD bit and also power on the panel. However, it also works nicely to just let the panel code read the EDID. Now that we've split the driver up we can expose the DDC AUX channel bus to the panel node. The panel can take charge of reading the EDID.
NOTE: in order for things to work, anyone that needs to read the EDID will need to add something that looks like this to their panel in the dts: ddc-i2c-bus = <&sn65dsi86_bridge>;
Presumably it's OK to land this without waiting for users to add the dts property since the EDID reading was a bit broken anyway, was "recently" added, and we know we must have the fallback mode to use (since the EDID reading was a bit broken).
Suggested-by: Andrzej Hajda a.hajda@samsung.com Signed-off-by: Douglas Anderson dianders@chromium.org Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org ---
(no changes since v1)
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 22 ---------------------- 1 file changed, 22 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 3539ddf9d109..26851119df96 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -125,7 +125,6 @@ * @connector: Our connector. * @host_node: Remote DSI node. * @dsi: Our MIPI DSI source. - * @edid: Detected EDID of eDP panel. * @refclk: Our reference clock. * @panel: Our panel. * @enable_gpio: The GPIO we toggle to enable the bridge. @@ -156,7 +155,6 @@ struct ti_sn65dsi86 { struct drm_dp_aux aux; struct drm_bridge bridge; struct drm_connector connector; - struct edid *edid; struct device_node *host_node; struct mipi_dsi_device *dsi; struct clk *refclk; @@ -405,24 +403,6 @@ connector_to_ti_sn65dsi86(struct drm_connector *connector) static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector) { struct ti_sn65dsi86 *pdata = connector_to_ti_sn65dsi86(connector); - struct edid *edid = pdata->edid; - int num, ret; - - if (!edid) { - pm_runtime_get_sync(pdata->dev); - edid = pdata->edid = drm_get_edid(connector, &pdata->aux.ddc); - pm_runtime_put_autosuspend(pdata->dev); - } - - if (edid && drm_edid_is_valid(edid)) { - ret = drm_connector_update_edid_property(connector, edid); - if (!ret) { - num = drm_add_edid_modes(connector, edid); - if (num) - return num; - } - } - return drm_panel_get_modes(pdata->panel, connector); }
@@ -1344,8 +1324,6 @@ static void ti_sn_bridge_remove(struct auxiliary_device *adev) mipi_dsi_device_unregister(pdata->dsi); }
- kfree(pdata->edid); - drm_bridge_remove(&pdata->bridge);
of_node_put(pdata->host_node);
Adding this link allows the panel code to do things like read the EDID.
Signed-off-by: Douglas Anderson dianders@chromium.org Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org ---
(no changes since v1)
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 1 + 1 file changed, 1 insertion(+)
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi index 24d293ef56d7..96e530594509 100644 --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi @@ -265,6 +265,7 @@ panel: panel { power-supply = <&pp3300_dx_edp>; backlight = <&backlight>; hpd-gpios = <&sn65dsi86_bridge 2 GPIO_ACTIVE_HIGH>; + ddc-i2c-bus = <&sn65dsi86_bridge>;
ports { port {
dri-devel@lists.freedesktop.org
-
Doug Anderson -
Douglas Anderson
-
Linus Walleij -
Sean Paul