From: Doug Anderson <dianders@chromium.org>
To: Andrzej Hajda <a.hajda@samsung.com>,
Neil Armstrong <narmstrong@baylibre.com>,
Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
Jonas Karlman <jonas@kwiboo.se>,
Jernej Skrabec <jernej.skrabec@siol.net>,
Sam Ravnborg <sam@ravnborg.org>, Lyude Paul <lyude@redhat.com>,
Thierry Reding <treding@nvidia.com>,
Linus W <linus.walleij@linaro.org>
Cc: linux-arm-msm <linux-arm-msm@vger.kernel.org>,
Rob Clark <robdclark@chromium.org>,
Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>,
Stephen Boyd <swboyd@chromium.org>,
Steev Klimaszewski <steev@kali.org>,
Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
linux-i2c <linux-i2c@vger.kernel.org>,
Bjorn Andersson <bjorn.andersson@linaro.org>,
dri-devel <dri-devel@lists.freedesktop.org>,
Daniel Vetter <daniel@ffwll.ch>, David Airlie <airlied@linux.ie>,
Robert Foss <robert.foss@linaro.org>,
LKML <linux-kernel@vger.kernel.org>,
Wolfram Sang <wsa@kernel.org>
Subject: Re: [PATCH v5 14/20] drm/bridge: ti-sn65dsi86: Promote the AUX channel to its own sub-dev
Date: Mon, 3 May 2021 13:27:15 -0700 [thread overview]
Message-ID: <CAD=FV=UTmOP8LDaf-Tyx17OORQK6pJH6O_w3cP0Bu-KRYaHkYw@mail.gmail.com> (raw)
In-Reply-To: <20210423095743.v5.14.Ie9daa320d907fff73f893f74b898197e399cce59@changeid>
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
next prev parent reply other threads:[~2021-05-03 20:27 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-23 16:58 [PATCH v5 00/20] drm: Fix EDID reading on ti-sn65dsi86; solve some chicken-and-egg problems Douglas Anderson
2021-04-23 16:58 ` [PATCH v5 01/20] drm/panel: panel-simple: Add missing pm_runtime_disable() calls Douglas Anderson
2021-04-28 16:32 ` Sean Paul
2021-04-30 0:58 ` Linus Walleij
2021-04-30 1:24 ` Doug Anderson
2021-04-30 1:27 ` Linus Walleij
2021-04-30 21:04 ` Doug Anderson
2021-05-01 12:07 ` Linus Walleij
2021-05-03 20:41 ` Doug Anderson
2021-05-05 12:51 ` Linus Walleij
2021-04-23 16:58 ` [PATCH v5 02/20] drm/bridge: ti-sn65dsi86: Rename the main driver data structure Douglas Anderson
2021-04-23 16:58 ` [PATCH v5 03/20] drm/bridge: ti-sn65dsi86: More renames in prep for sub-devices Douglas Anderson
2021-04-23 16:58 ` [PATCH v5 04/20] drm/bridge: ti-sn65dsi86: Use devm to do our runtime_disable Douglas Anderson
2021-04-23 16:58 ` [PATCH v5 05/20] drm/bridge: ti-sn65dsi86: Clean debugfs code Douglas Anderson
2021-04-23 16:58 ` [PATCH v5 06/20] drm/bridge: ti-sn65dsi86: Add local var for "dev" to simplify probe Douglas Anderson
2021-04-23 16:58 ` [PATCH v5 07/20] drm/bridge: ti-sn65dsi86: Cleanup managing of drvdata Douglas Anderson
2021-04-23 16:58 ` [PATCH v5 08/20] drm/bridge: ti-sn65dsi86: Move all the chip-related init to the start Douglas Anderson
2021-04-23 16:58 ` [PATCH v5 09/20] drm/bridge: ti-sn65dsi86: Break GPIO and MIPI-to-eDP bridge into sub-drivers Douglas Anderson
2021-05-01 11:59 ` Linus Walleij
2021-05-03 16:55 ` Doug Anderson
2021-04-23 16:58 ` [PATCH v5 10/20] drm/panel: panel-simple: Get rid of hacky HPD chicken-and-egg code Douglas Anderson
2021-04-28 16:35 ` Sean Paul
2021-04-23 16:58 ` [PATCH v5 11/20] drm/bridge: ti-sn65dsi86: Use pm_runtime autosuspend Douglas Anderson
2021-04-23 16:58 ` [PATCH v5 12/20] drm/bridge: ti-sn65dsi86: Code motion of refclk management functions Douglas Anderson
2021-04-23 16:58 ` [PATCH v5 13/20] drm/bridge: ti-sn65dsi86: If refclk, DP AUX can happen w/out pre-enable Douglas Anderson
2021-04-23 16:59 ` [PATCH v5 14/20] drm/bridge: ti-sn65dsi86: Promote the AUX channel to its own sub-dev Douglas Anderson
2021-05-03 20:27 ` Doug Anderson [this message]
2021-04-23 16:59 ` [PATCH v5 15/20] i2c: i2c-core-of: Fix corner case of finding adapter by node Douglas Anderson
2021-04-23 16:59 ` [PATCH v5 16/20] drm/panel: panel-simple: Remove extra call: drm_connector_update_edid_property() Douglas Anderson
2021-04-28 16:38 ` Sean Paul
2021-04-23 16:59 ` [PATCH v5 17/20] drm/panel: panel-simple: Power the panel when reading the EDID Douglas Anderson
2021-04-28 16:44 ` Sean Paul
2021-04-23 16:59 ` [PATCH v5 18/20] drm/panel: panel-simple: Cache the EDID as long as we retain power Douglas Anderson
2021-04-28 16:50 ` Sean Paul
2021-04-23 16:59 ` [PATCH v5 19/20] drm/bridge: ti-sn65dsi86: Don't read EDID blob over DDC Douglas Anderson
2021-04-23 16:59 ` [PATCH v5 20/20] arm64: dts: qcom: Link the panel to the bridge's DDC bus Douglas Anderson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAD=FV=UTmOP8LDaf-Tyx17OORQK6pJH6O_w3cP0Bu-KRYaHkYw@mail.gmail.com' \
--to=dianders@chromium.org \
--cc=Laurent.pinchart@ideasonboard.com \
--cc=a.hajda@samsung.com \
--cc=airlied@linux.ie \
--cc=bjorn.andersson@linaro.org \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=jernej.skrabec@siol.net \
--cc=jonas@kwiboo.se \
--cc=linus.walleij@linaro.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-i2c@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=lyude@redhat.com \
--cc=maarten.lankhorst@linux.intel.com \
--cc=narmstrong@baylibre.com \
--cc=robdclark@chromium.org \
--cc=robert.foss@linaro.org \
--cc=sam@ravnborg.org \
--cc=stanislav.lisovskiy@intel.com \
--cc=steev@kali.org \
--cc=swboyd@chromium.org \
--cc=treding@nvidia.com \
--cc=wsa@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).