dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
From: Bjorn Andersson <bjorn.andersson@linaro.org>
To: Douglas Anderson <dianders@chromium.org>
Cc: robdclark@chromium.org, Jernej Skrabec <jernej.skrabec@gmail.com>,
	dri-devel@lists.freedesktop.org, Jonas Karlman <jonas@kwiboo.se>,
	David Airlie <airlied@linux.ie>,
	linux-arm-msm@vger.kernel.org,
	Neil Armstrong <narmstrong@baylibre.com>,
	linux-kernel@vger.kernel.org, Steev Klimaszewski <steev@kali.org>,
	Stephen Boyd <swboyd@chromium.org>,
	Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>,
	Andrzej Hajda <a.hajda@samsung.com>,
	Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
	Thierry Reding <treding@nvidia.com>,
	Sam Ravnborg <sam@ravnborg.org>,
	Robert Foss <robert.foss@linaro.org>
Subject: Re: [PATCH v8 08/11] drm/bridge: ti-sn65dsi86: Promote the AUX channel to its own sub-dev
Date: Sat, 5 Jun 2021 23:40:40 -0500	[thread overview]
Message-ID: <YLxRyC9eNlg+Uex3@builder.lan> (raw)
In-Reply-To: <20210524165920.v8.8.If89144992cb9d900f8c91a8d1817dbe00f543720@changeid>

On Mon 24 May 19:01 CDT 2021, Douglas Anderson wrote:

> On its own, this change looks a little strange and doesn't do too much
> useful. To understand why we're doing this we need to look forward to
> future patches where we're going to probe our panel using the new DP
> AUX bus. See the patch ("drm/bridge: ti-sn65dsi86: Add support for the
> DP AUX bus").
> 
> Let's think about the set of steps we'll want to happen when we have
> the DP AUX bus:
> 
> 1. We'll create the DP AUX bus.
> 2. We'll populate the devices on the DP AUX bus (AKA our panel).
> 3. For setting up the bridge-related functions of ti-sn65dsi86 we'll
>    need to get a reference to the panel.
> 
> If we do #1 - #3 in a single probe call things _mostly_ will work, but
> it won't be massively robust. Let's explore.
> 
> First let's think of the easy case of no -EPROBE_DEFER. In that case
> in step #2 when we populate the devices on the DP AUX bus it will
> actually try probing the panel right away. Since the panel probe
> doesn't defer then in step #3 we'll get a reference to the panel and
> we're golden.
> 
> Second, let's think of the case when the panel returns
> -EPROBE_DEFER. In that case step #2 won't synchronously create the
> panel (it'll just add the device to the defer list to do it
> later). Step #3 will fail to get the panel and the bridge sub-device
> will return -EPROBE_DEFER. We'll depopulate the DP AUX bus. Later
> we'll try the whole sequence again. Presumably the panel will
> eventually stop returning -EPROBE_DEFER and we'll go back to the first
> case where things were golden. So this case is OK too even if it's a
> bit ugly that we have to keep creating / deleting the AUX bus over and
> over.
> 
> So where is the problem? As I said, it's mostly about robustness. I
> don't believe that step #2 (creating the sub-devices) is really
> guaranteed to be synchronous. This is evidenced by the fact that it's
> allowed to "succeed" by just sticking the device on the deferred
> list. If anything about the process changes in Linux as a whole and
> step #2 just kicks off the probe of the DP AUX endpoints (our panel)
> in the background then we'd be in trouble because we might never get
> the panel in step #3.
> 
> Adding an extra sub-device means we just don't need to worry about
> it. We'll create the sub-device for the DP AUX bus and it won't go
> away until the whole ti-sn65dsi86 driver goes away. If the bridge
> sub-device defers (maybe because it can't find the panel) that won't
> depopulate the DP AUX bus and so we don't need to worry about it.
> 
> 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>
> Reviewed-by: Lyude Paul <lyude@redhat.com>
> ---
> 
> (no changes since v7)
> 
> Changes in v7:
> - Beefed up commit message in context of the DP AUX bus.
> - Remove use of now-dropped drm_dp_aux_register_ddc() call.
> - Set the proper sub-device "dev" pointer in the AUX structure.
> 
> Changes in v6:
> - Use new drm_dp_aux_register_ddc() calls.
> 
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 80 +++++++++++++++++++++++----
>  1 file changed, 69 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 45a2969afb2b..1ea07d704705 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;
> @@ -1333,11 +1335,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;
>  
> @@ -1432,6 +1429,53 @@ static int ti_sn65dsi86_add_aux_device(struct ti_sn65dsi86 *pdata,
>  	return ret;
>  }
>  
> +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;
> +
> +	/*
> +	 * We couldn't do this pre-probe because it would confuse pinctrl.
> +	 * It would have tried to grab the same pins that the main device had.
> +	 * Set it now so that we can put the proper (sub) device in the aux
> +	 * structure and it will have the right node.
> +	 */
> +	adev->dev.of_node = pdata->dev->of_node;

I suspect the refcount of the of_node will be wrong here and upon
removing the aux device things will be off...

Instead, I think you're looking for device_set_of_node_from_dev(), which
also sets of_node_reused, which in turn causes pinctrl_bind_pins() to be
skipped - i.e. it seems to deal with the problem your comment describes.

Regards,
Bjorn

  reply	other threads:[~2021-06-06  4:40 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-25  0:01 [PATCH v8 00/11] drm: Fix EDID reading on ti-sn65dsi86 by introducing the DP AUX bus Douglas Anderson
2021-05-25  0:01 ` [PATCH v8 01/11] dt-bindings: display: simple: List hpd properties in panel-simple Douglas Anderson
2021-06-02 18:00   ` Rob Herring
2021-05-25  0:01 ` [PATCH v8 02/11] dt-bindings: drm: Introduce the DP AUX bus Douglas Anderson
2021-06-02 18:08   ` Rob Herring
2021-05-25  0:01 ` [PATCH v8 03/11] dt-bindings: drm/bridge: ti-sn65dsi86: Add aux-bus child Douglas Anderson
2021-05-28  0:29   ` Linus Walleij
2021-06-02 18:06     ` Rob Herring
2021-06-02 18:09       ` Rob Herring
2021-06-02 18:10   ` Rob Herring
2021-05-25  0:01 ` [PATCH v8 04/11] dt-bindings: drm/aux-bus: Add an example Douglas Anderson
2021-05-28  0:31   ` Linus Walleij
2021-06-02 18:16   ` Rob Herring
2021-06-02 19:55     ` Doug Anderson
2021-05-25  0:01 ` [PATCH v8 05/11] drm: Introduce the DP AUX bus Douglas Anderson
2021-05-25  0:01 ` [PATCH v8 06/11] drm/panel: panel-simple: Allow panel-simple be a DP AUX endpoint device Douglas Anderson
2021-05-25  0:01 ` [PATCH v8 07/11] drm/panel: panel-simple: Stash DP AUX bus; allow using it for DDC Douglas Anderson
2021-06-04 16:10   ` rajeevny
2021-06-07 17:07     ` Doug Anderson
2021-05-25  0:01 ` [PATCH v8 08/11] drm/bridge: ti-sn65dsi86: Promote the AUX channel to its own sub-dev Douglas Anderson
2021-06-06  4:40   ` Bjorn Andersson [this message]
2021-06-07 17:07     ` Doug Anderson
2021-05-25  0:01 ` [PATCH v8 09/11] drm/bridge: ti-sn65dsi86: Add support for the DP AUX bus Douglas Anderson
2021-05-25  0:01 ` [PATCH v8 10/11] drm/bridge: ti-sn65dsi86: Don't read EDID blob over DDC Douglas Anderson
2021-05-25  0:01 ` [PATCH v8 11/11] arm64: dts: qcom: sc7180-trogdor: Move panel under the bridge chip 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=YLxRyC9eNlg+Uex3@builder.lan \
    --to=bjorn.andersson@linaro.org \
    --cc=Laurent.pinchart@ideasonboard.com \
    --cc=a.hajda@samsung.com \
    --cc=airlied@linux.ie \
    --cc=dianders@chromium.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=jernej.skrabec@gmail.com \
    --cc=jonas@kwiboo.se \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --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 \
    --subject='Re: [PATCH v8 08/11] drm/bridge: ti-sn65dsi86: Promote the AUX channel to its own sub-dev' \
    /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

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).