Re: [PATCH v4 3/8] drm/bridge/synopsys: dsi: add ability to have glue-specific attach and detach

[Heiko Stuebner <heiko@sntech.de>]

Hi,

Am Montag, 20. August 2018, 12:34:37 CEST schrieb Andrzej Hajda:
> On 14.08.2018 12:26, Heiko Stuebner wrote:
> > With the regular means of adding the dsi-component in probe it creates
> > a race condition with the panel probing, as the panel device only gets
> > created after the dsi-bus got created.
> >
> > When the panel-driver is build as a module it currently fails hard as the
> > panel cannot be probed directly:
> >
> > dw_mipi_dsi_bind()
> >   __dw_mipi_dsi_probe()
> >     creates dsi bus
> >     creates panel device
> >     triggers panel module load
> >     panel not probed (module not loaded or panel probe slow)
> >   drm_bridge_attach
> >     fails with -EINVAL due to empty panel_bridge
> >
> > Additionally the panel probing can run concurrently with dsi bringup
> > making it possible that the panel can already be found but dsi-attach
> > hasn't finished running.
> >
> > To solve that cleanly we may want to only create the component after
> > the panel has finished probing, by calling component_add from the
> > host-attach dsi callback.
> >
> > As that is specific to glue drivers, add a new struct for host_ops
> > so that glue drivers can tell the bridge to call specific functions
> > after the common host-attach and before the common host-detach run.
> 
> Sometimes I have an impression that core/glue driver architecture with
> callbacks to glue drivers is quite complicated, and smells mid-layer
> mistake :), I wonder if simple bunch of helpers with some base object
> wouldn't be better, but this is subject for other discussion.
> 
> >
> > Suggested-by: Andrzej Hajda <a.hajda@samsung.com>
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>

> > @@ -300,6 +301,12 @@ static int dw_mipi_dsi_host_attach(struct mipi_dsi_host *host,
> >  
> >  	drm_bridge_add(&dsi->bridge);
> >  
> > +	if (pdata->host_ops && pdata->host_ops->attach) {
> > +		ret = pdata->host_ops->attach(pdata->priv_data, device);
> > +		if (ret < 0)
> > +			return ret;
> 
> It could be replaced by:
>     return pdata->host_ops->attach(pdata->priv_data, device);
> 
> But no strong feelings. With or without the change:

I'll keep it the way it is then ;-) .

My rationale is that the "return 0" below is the last line of this
function and returns the success.

The specialized attach is only called in a fraction of instances, so
having a return on success in there, might be surprising if later on
additional code gets added between the attach call and the return below
and might be missed to modify in that case, possibly causing breakage.


> > +	}
> > +
> >  	return 0;
> >  }


Heiko


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel