On Mon, 2017-02-06 at 10:53 -0600, Rob Herring wrote:
> On Mon, Feb 06, 2017 at 11:42:48AM +0100, Philipp Zabel wrote:
> > On Fri, 2017-02-03 at 21:36 -0600, Rob Herring wrote:
> > > Many drivers have a common pattern of searching the OF graph for either an
> > > attached panel or bridge and then finding the DRM struct for the panel
> > > or bridge. Also, most drivers need to handle deferred probing when the
> > > DRM device is not yet instantiated. Create a common function,
> > > drm_of_find_panel_or_bridge, to find the connected node and the
> > > associated DRM panel or bridge device.
>
> [...]
>
> > > +int drm_of_find_panel_or_bridge(const struct device_node *np,
> > > + int port, int endpoint,
> > > + struct drm_panel **panel,
> > > + struct drm_bridge **bridge)
> > > +{
> > > + int ret = -ENODEV;
> >
> > This is only returned if !panel && !bridge. I'd consider this invalid
> > usage of this function, so maybe use -EINVAL?
>
> Yes.
>
> > > + struct device_node *remote;
> > > +
> > > + remote = of_graph_get_remote_node(np, port, endpoint);
> > > + if (!remote)
> > > + return -ENODEV;
> > > +
> > > + if (bridge)
> > > + *bridge = NULL;
> >
> > I would move this ^ ...
> >
> > > + if (panel) {
> > > + *panel = of_drm_find_panel(remote);
> > > + if (*panel) {
> >
> > ... here.
>
> Okay.
>
> > > + ret = 0;
> > > + goto out_put;
> > > + }
> > > + ret = -EPROBE_DEFER;
> > > + }
> > > +
> > > + if (bridge) {
> > > + *bridge = of_drm_find_bridge(remote);
> > > + if (*bridge)
> > > + ret = 0;
> > > + else
> > > + ret = -EPROBE_DEFER;
> > > + }
> > > +out_put:
> > > + of_node_put(remote);
> > > + return ret;
> > > +}
>
> I've ended up re-writing things a bit getting rid of the goto and the
> result looks like this:
Looks good to me.
> int drm_of_find_panel_or_bridge(const struct device_node *np,
> int port, int endpoint,
> struct drm_panel **panel,
> struct drm_bridge **bridge)
> {
> int ret = -EPROBE_DEFER;
> struct device_node *remote;
>
> if (!panel && !bridge)
> return -EINVAL;
>
> remote = of_graph_get_remote_node(np, port, endpoint);
> if (!remote)
> return -ENODEV;
>
> if (panel) {
> *panel = of_drm_find_panel(remote);
> if (*panel) {
> if (bridge)
> *bridge = NULL;
With the goto out_put gone, I'm conflicted whether I find this clearer
here, or ...
> ret = 0;
> }
> }
>
> /* No panel found yet, check for a bridge next. */
> if (ret && bridge) {
> *bridge = of_drm_find_bridge(remote);
> if (*bridge)
> ret = 0;
> }
... even down here:
if (bridge) {
if (ret) {
/* No panel found yet, check for a bridge next. */
*bridge = of_drm_find_bridge(remote)
if (*bridge)
ret = 0;
} else {
*bridge = NULL;
}
}
That way bridge doesn't have to be checked twice and all the
modification of *bridge is in the same block.
>
> of_node_put(remote);
> return ret;
> }
Either way,
Acked-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
regards
Philipp
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
