Re: [PATCH 6/8] drm: Allow DSI devices to be registered before the host registers.

[Boris Brezillon]

Hi all,

Sorry to enter the discussion so late in the review process.

Eric, I'm replying here because this is where the initial discussion
happened, but I actually reviewed v5 of your patchset.

On Tue, 18 Jul 2017 13:13:04 -0700
Eric Anholt <eric@xxxxxxxxxx> wrote:

> Archit Taneja <architt@xxxxxxxxxxxxxx> writes:
> 
> > On 07/15/2017 04:28 AM, Eric Anholt wrote:  
> >> Archit Taneja <architt@xxxxxxxxxxxxxx> writes:
> >>   
> >>> On 06/28/2017 01:28 AM, Eric Anholt wrote:  
> >>>> When a mipi_dsi_host is registered, the DT is walked to find any child
> >>>> nodes with compatible strings.  Those get registered as DSI devices,
> >>>> and most DSI panel drivers are mipi_dsi_drivers that attach to those nodes.
> >>>>
> >>>> There is one special case currently, the adv7533 bridge, where the
> >>>> bridge probes on I2C, and during the bridge attach step it looks up
> >>>> the mipi_dsi_host and registers the mipi_dsi_device (for its own stub
> >>>> mipi_dsi_driver).
> >>>>
> >>>> For the Raspberry Pi panel, though, we also need to attach on I2C (our
> >>>> control bus), but don't have a bridge driver.  The lack of a bridge's
> >>>> attach() step like adv7533 uses means that we aren't able to delay the
> >>>> mipi_dsi_device creation until the mipi_dsi_host is present.
> >>>>
> >>>> To fix this, we extend mipi_dsi_device_register_full() to allow being
> >>>> called with a NULL host, which puts the device on a queue waiting for
> >>>> a host to appear.  When a new host is registered, we fill in the host
> >>>> value and finish the device creation process.  
> >>>
> >>> This is quite a nice idea. The only bothering thing is the info.of_node usage
> >>> varies between child nodes (mipi_dsi_devs) and non-child nodes (i2c control
> >>> bus).
> >>>
> >>> For DSI children expressed in DT, the of_node in info holds the DT node
> >>> corresponding to the DSI child itself. For non-DT ones, this patch assumes
> >>> that info.of_node stores the DSI host DT node. I think it should be okay as
> >>> long as we mention the usage in a comment somewhere. The other option is to
> >>> have a new info.host_node field to keep a track of the host DT node.  
> >> 
> >> I think maybe you misread the patch?  We're using
> >> of_get_parent(dsi->dev.node), which came from info->node, to compare to
> >> host->dev->of_node().  
> >
> > I think I did misread it.
> >
> > Although, I'm not entirely clear what we should be setting info.node to.
> > In patch #8, info.node is set by:
> >
> > 	endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
> > 	info.node = of_graph_get_remote_port(endpoint);
> >
> > Looking at the dt bindings in patch #7, it looks like info.node is set
> > to the 'port' device node in dsi@7e700000, is that right?  
> 
> Yeah.
> 
> 
> > I suppose 'port' here seems like a reasonable representation of
> > dsi->dev.node, I wonder how it would work if the dsi host had multiple
> > ports underneath it. I.e:
> >
> > dsi@7e700000 {
> > 	...
> > 	...
> > 	ports {
> > 		port@0 {
> > 			...
> > 			dsi_out_port: endpoint {
> > 				remote-endpoint = <&panel_dsi_port>;
> > 			};
> > 		};
> > 		port@1 {
> > 			...
> > 			...
> > 		};
> > 	};
> > };
> >
> > Here, we would need to set info.node to the 'ports' node, so that
> > of_get_parent(dsi->dev.of_node) equals host->dev->of_node. That doesn't
> > seem correct.

I agree. I think we're abusing dev->of_node here, which makes the whole
thing even harder to understand.

> >
> > Ideally, a dev's 'of_node' should be left to NULL if we don't have a
> > corresponding OF node.

Well, there are cases where the device actually has a valid OF node,
like the adv7533, but this device is defined under a different control
bus (i2c in this case). So, to be accurate with the DT representation,
dsi->dev.of_node should be set to the adv7533 OF node (the one under
the I2C bus), right?

> > We're sort of overriding it here since we don't
> > have any other place to store this information in the mipi_dsi_device
> > struct.
> >
> > Maybe we could add a 'host_node' entry in mipi_dsi_device itself, which
> > is exclusively used cases where the DSI device doesn't have a DT node.

I think that would be clearer.

> > Our check in mipi_dsi_host_register() could then be something like:  
> 
> I think instead of extending the struct, we can just walk to the parent
> similarly to how of_graph_get_remove_port_parent() does.

It's working as long as dsi->dev.of_node points to one of the port node
defined under the DSI host, but is this node really representing the DSI
device?

Sure, your solution works, but it makes the whole DSI registration even
harder to follow.

> And fix some
> node refcounting at the same time:
> 
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> index ed3d505dc203..77d439254da6 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> @@ -313,7 +313,12 @@ int mipi_dsi_host_register(struct mipi_dsi_host *host)
>          * connect our host to it and probe them now.
>          */
>         list_for_each_entry_safe(dsi, temp, &unattached_device_list, list) {
> -               if (of_get_parent(dsi->dev.of_node) == host->dev->of_node) {
> +               struct device_node *host_node = of_get_parent(dsi->dev.of_node);
> +
> +               if (!of_node_cmp(host_node->name, "ports"))
> +                       host_node = of_get_next_parent(host_node);
> +
> +               if (host_node == host->dev->of_node) {
>                         dsi->host = host;
>                         dsi->dev.parent = host->dev;
>                         device_initialize(&dsi->dev);
> @@ -321,6 +326,8 @@ int mipi_dsi_host_register(struct mipi_dsi_host *host)
>                         mipi_dsi_device_add(dsi);
>                         list_del_init(&dsi->list);
>                 }
> +
> +               of_node_put(host_node);
>         }
>         mutex_unlock(&host_lock);
>