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

From: Archit Taneja
Date: Mon Jul 17 2017 - 09:39:17 EST




On 07/15/2017 04:31 AM, Eric Anholt wrote:
Archit Taneja <architt@xxxxxxxxxxxxxx> writes:

On 06/29/2017 04:09 PM, Andrzej Hajda wrote:
On 29.06.2017 07:03, Archit Taneja wrote:

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.

Field abuse is not a biggest issue.

This patch changes totally semantic of mipi_dsi_device_register_full.
Currently semantic of *_device_register* function is to create and add
device to existing bus, ie after return we have device attached to bus,
so it can be instantly used. With this change function can return some
unattached device, without warranty it will be ever attached - kind of
hidden deferring. Such change looks for me quite dangerous, even if it
looks convenient in this case.

As discussed in other thread more appealing solution for me would be:
1. host creates dsi bus, but doesn't call component_add as it does not
have all required resources.
2. host waits for all required dsi devs attached, gets registered panels
or bridges and calls component_add after that.
3. in bind phase it has all it needs, hasn't it?

This seems like it would work, but would require KMS drivers to restructure
themselves around this approach. For KMS drivers that don't even use the
component stuff, it might be asking too much.

We could maybe consider Eric's patch as an intermediate solution, we should
definitely put WARN_ON(!dsi->host) like checks for all the existing
mipi_dsi_*() API.

Could you clarify which entrypoints you'd like a warning on? Is it just
"everything that gets the host ops"?

Sorry, all API was a bad suggestion.

I think a warning and early bail in mipi_dsi_attach() should be sufficient.
A panel/bridge DSI device shouldn't use any other API if it hasn't called
mipi_dsi_attach().

Thanks,
Archit



--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project