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@xxxxxxxxxxxx>
> Reviewed-by: Lyude Paul <lyude@xxxxxxxxxx>
> ---
>
> (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
