| Message ID | 20170627195839.3338-7-eric@anholt.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. Thanks, Archit > > Signed-off-by: Eric Anholt <eric@anholt.net> > --- > drivers/gpu/drm/drm_mipi_dsi.c | 49 ++++++++++++++++++++++++++++++++---------- > include/drm/drm_mipi_dsi.h | 3 +++ > 2 files changed, 41 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c > index 1160a579e0dc..9cdd68a7dc0d 100644 > --- a/drivers/gpu/drm/drm_mipi_dsi.c > +++ b/drivers/gpu/drm/drm_mipi_dsi.c > @@ -45,6 +45,13 @@ > * subset of the MIPI DCS command set. > */ > > +static DEFINE_MUTEX(host_lock); > +static LIST_HEAD(host_list); > +/* List of struct mipi_dsi_device which were registered while no host > + * was available. > + */ > +static LIST_HEAD(unattached_device_list); > + > static int mipi_dsi_device_match(struct device *dev, struct device_driver *drv) > { > struct mipi_dsi_device *dsi = to_mipi_dsi_device(dev); > @@ -138,10 +145,12 @@ static struct mipi_dsi_device *mipi_dsi_device_alloc(struct mipi_dsi_host *host) > > dsi->host = host; > dsi->dev.bus = &mipi_dsi_bus_type; > - dsi->dev.parent = host->dev; > dsi->dev.type = &mipi_dsi_device_type; > > - device_initialize(&dsi->dev); > + if (dsi->host) { > + dsi->dev.parent = host->dev; > + device_initialize(&dsi->dev); > + } > > return dsi; > } > @@ -206,7 +215,7 @@ mipi_dsi_device_register_full(struct mipi_dsi_host *host, > const struct mipi_dsi_device_info *info) > { > struct mipi_dsi_device *dsi; > - struct device *dev = host->dev; > + struct device *dev = host ? host->dev : NULL; > int ret; > > if (!info) { > @@ -230,11 +239,17 @@ mipi_dsi_device_register_full(struct mipi_dsi_host *host, > dsi->channel = info->channel; > strlcpy(dsi->name, info->type, sizeof(dsi->name)); > > - ret = mipi_dsi_device_add(dsi); > - if (ret) { > - dev_err(dev, "failed to add DSI device %d\n", ret); > - kfree(dsi); > - return ERR_PTR(ret); > + if (!dsi->host) { > + mutex_lock(&host_lock); > + list_add(&dsi->list, &unattached_device_list); > + mutex_unlock(&host_lock); > + } else { > + ret = mipi_dsi_device_add(dsi); > + if (ret) { > + dev_err(dev, "failed to add DSI device %d\n", ret); > + kfree(dsi); > + return ERR_PTR(ret); > + } > } > > return dsi; > @@ -251,9 +266,6 @@ void mipi_dsi_device_unregister(struct mipi_dsi_device *dsi) > } > EXPORT_SYMBOL(mipi_dsi_device_unregister); > > -static DEFINE_MUTEX(host_lock); > -static LIST_HEAD(host_list); > - > /** > * of_find_mipi_dsi_host_by_node() - find the MIPI DSI host matching a > * device tree node > @@ -285,6 +297,7 @@ EXPORT_SYMBOL(of_find_mipi_dsi_host_by_node); > int mipi_dsi_host_register(struct mipi_dsi_host *host) > { > struct device_node *node; > + struct mipi_dsi_device *dsi, *temp; > > for_each_available_child_of_node(host->dev->of_node, node) { > /* skip nodes without reg property */ > @@ -295,6 +308,20 @@ int mipi_dsi_host_register(struct mipi_dsi_host *host) > > mutex_lock(&host_lock); > list_add_tail(&host->list, &host_list); > + > + /* If any DSI devices were registered under our OF node, then > + * 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) { > + dsi->host = host; > + dsi->dev.parent = host->dev; > + device_initialize(&dsi->dev); > + > + mipi_dsi_device_add(dsi); > + list_del_init(&dsi->list); > + } > + } > mutex_unlock(&host_lock); > > return 0; > diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h > index 4fef19064b0f..699ea4acd5b6 100644 > --- a/include/drm/drm_mipi_dsi.h > +++ b/include/drm/drm_mipi_dsi.h > @@ -178,6 +178,9 @@ struct mipi_dsi_device { > unsigned int lanes; > enum mipi_dsi_pixel_format format; > unsigned long mode_flags; > + > + /* Entry on the unattached_device_list */ > + struct list_head list; > }; > > #define MIPI_DSI_MODULE_PREFIX "mipi-dsi:" >
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? I did not spent much time on this idea, so I cannot guarantee it has not fundamental issues :) Regards Andrzej > > Thanks, > Archit > >> Signed-off-by: Eric Anholt <eric@anholt.net> >> --- >> drivers/gpu/drm/drm_mipi_dsi.c | 49 ++++++++++++++++++++++++++++++++---------- >> include/drm/drm_mipi_dsi.h | 3 +++ >> 2 files changed, 41 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c >> index 1160a579e0dc..9cdd68a7dc0d 100644 >> --- a/drivers/gpu/drm/drm_mipi_dsi.c >> +++ b/drivers/gpu/drm/drm_mipi_dsi.c >> @@ -45,6 +45,13 @@ >> * subset of the MIPI DCS command set. >> */ >> >> +static DEFINE_MUTEX(host_lock); >> +static LIST_HEAD(host_list); >> +/* List of struct mipi_dsi_device which were registered while no host >> + * was available. >> + */ >> +static LIST_HEAD(unattached_device_list); >> + >> static int mipi_dsi_device_match(struct device *dev, struct device_driver *drv) >> { >> struct mipi_dsi_device *dsi = to_mipi_dsi_device(dev); >> @@ -138,10 +145,12 @@ static struct mipi_dsi_device *mipi_dsi_device_alloc(struct mipi_dsi_host *host) >> >> dsi->host = host; >> dsi->dev.bus = &mipi_dsi_bus_type; >> - dsi->dev.parent = host->dev; >> dsi->dev.type = &mipi_dsi_device_type; >> >> - device_initialize(&dsi->dev); >> + if (dsi->host) { >> + dsi->dev.parent = host->dev; >> + device_initialize(&dsi->dev); >> + } >> >> return dsi; >> } >> @@ -206,7 +215,7 @@ mipi_dsi_device_register_full(struct mipi_dsi_host *host, >> const struct mipi_dsi_device_info *info) >> { >> struct mipi_dsi_device *dsi; >> - struct device *dev = host->dev; >> + struct device *dev = host ? host->dev : NULL; >> int ret; >> >> if (!info) { >> @@ -230,11 +239,17 @@ mipi_dsi_device_register_full(struct mipi_dsi_host *host, >> dsi->channel = info->channel; >> strlcpy(dsi->name, info->type, sizeof(dsi->name)); >> >> - ret = mipi_dsi_device_add(dsi); >> - if (ret) { >> - dev_err(dev, "failed to add DSI device %d\n", ret); >> - kfree(dsi); >> - return ERR_PTR(ret); >> + if (!dsi->host) { >> + mutex_lock(&host_lock); >> + list_add(&dsi->list, &unattached_device_list); >> + mutex_unlock(&host_lock); >> + } else { >> + ret = mipi_dsi_device_add(dsi); >> + if (ret) { >> + dev_err(dev, "failed to add DSI device %d\n", ret); >> + kfree(dsi); >> + return ERR_PTR(ret); >> + } >> } >> >> return dsi; >> @@ -251,9 +266,6 @@ void mipi_dsi_device_unregister(struct mipi_dsi_device *dsi) >> } >> EXPORT_SYMBOL(mipi_dsi_device_unregister); >> >> -static DEFINE_MUTEX(host_lock); >> -static LIST_HEAD(host_list); >> - >> /** >> * of_find_mipi_dsi_host_by_node() - find the MIPI DSI host matching a >> * device tree node >> @@ -285,6 +297,7 @@ EXPORT_SYMBOL(of_find_mipi_dsi_host_by_node); >> int mipi_dsi_host_register(struct mipi_dsi_host *host) >> { >> struct device_node *node; >> + struct mipi_dsi_device *dsi, *temp; >> >> for_each_available_child_of_node(host->dev->of_node, node) { >> /* skip nodes without reg property */ >> @@ -295,6 +308,20 @@ int mipi_dsi_host_register(struct mipi_dsi_host *host) >> >> mutex_lock(&host_lock); >> list_add_tail(&host->list, &host_list); >> + >> + /* If any DSI devices were registered under our OF node, then >> + * 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) { >> + dsi->host = host; >> + dsi->dev.parent = host->dev; >> + device_initialize(&dsi->dev); >> + >> + mipi_dsi_device_add(dsi); >> + list_del_init(&dsi->list); >> + } >> + } >> mutex_unlock(&host_lock); >> >> return 0; >> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h >> index 4fef19064b0f..699ea4acd5b6 100644 >> --- a/include/drm/drm_mipi_dsi.h >> +++ b/include/drm/drm_mipi_dsi.h >> @@ -178,6 +178,9 @@ struct mipi_dsi_device { >> unsigned int lanes; >> enum mipi_dsi_pixel_format format; >> unsigned long mode_flags; >> + >> + /* Entry on the unattached_device_list */ >> + struct list_head list; >> }; >> >> #define MIPI_DSI_MODULE_PREFIX "mipi-dsi:" >>
Andrzej Hajda <a.hajda@samsung.com> writes: > 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. It only changes the semantic if you past in a NULL host, from "oops" to "do something useful". > 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? > > I did not spent much time on this idea, so I cannot guarantee it has not > fundamental issues :) If component_add() isn't called during probe, then DSI would just get skipped during bind as far as I know. I *think* what you're thinking is moving DSI host register to probe, and then panel lookup stays in bind. That seems much more risky to me -- do we know for sure that no mipi_dsi_device will do any DSI transactions during its probe? I would expect some of them to.
On 29.06.2017 17:22, Eric Anholt wrote: > Andrzej Hajda <a.hajda@samsung.com> writes: > >> 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. > It only changes the semantic if you past in a NULL host, from "oops" to > "do something useful". > >> 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? >> >> I did not spent much time on this idea, so I cannot guarantee it has not >> fundamental issues :) > If component_add() isn't called during probe, then DSI would just get > skipped during bind as far as I know. No, drm master will wait till all components are present. > > I *think* what you're thinking is moving DSI host register to probe, yes, this way you will allow to create dsi devices. > and > then panel lookup stays in bind. no, panel lookup will be performed in dsi_host attach callback, and component_add will be called also there, if all required panels/bridges are get. > That seems much more risky to me -- do > we know for sure that no mipi_dsi_device will do any DSI transactions > during its probe? I would expect some of them to. I think it is irrelevant, with the current design only transactions between prepare/unprepare callbacks have chances to succeed. Andrzej
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. About the solution that you and Boris discussed in the other thread, I'll give this a try on the msm driver and see how it does. Thanks, Archit > > I did not spent much time on this idea, so I cannot guarantee it has not > fundamental issues :) > > Regards > Andrzej > >> >> Thanks, >> Archit >> >>> Signed-off-by: Eric Anholt <eric@anholt.net> >>> --- >>> drivers/gpu/drm/drm_mipi_dsi.c | 49 ++++++++++++++++++++++++++++++++---------- >>> include/drm/drm_mipi_dsi.h | 3 +++ >>> 2 files changed, 41 insertions(+), 11 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c >>> index 1160a579e0dc..9cdd68a7dc0d 100644 >>> --- a/drivers/gpu/drm/drm_mipi_dsi.c >>> +++ b/drivers/gpu/drm/drm_mipi_dsi.c >>> @@ -45,6 +45,13 @@ >>> * subset of the MIPI DCS command set. >>> */ >>> >>> +static DEFINE_MUTEX(host_lock); >>> +static LIST_HEAD(host_list); >>> +/* List of struct mipi_dsi_device which were registered while no host >>> + * was available. >>> + */ >>> +static LIST_HEAD(unattached_device_list); >>> + >>> static int mipi_dsi_device_match(struct device *dev, struct device_driver *drv) >>> { >>> struct mipi_dsi_device *dsi = to_mipi_dsi_device(dev); >>> @@ -138,10 +145,12 @@ static struct mipi_dsi_device *mipi_dsi_device_alloc(struct mipi_dsi_host *host) >>> >>> dsi->host = host; >>> dsi->dev.bus = &mipi_dsi_bus_type; >>> - dsi->dev.parent = host->dev; >>> dsi->dev.type = &mipi_dsi_device_type; >>> >>> - device_initialize(&dsi->dev); >>> + if (dsi->host) { >>> + dsi->dev.parent = host->dev; >>> + device_initialize(&dsi->dev); >>> + } >>> >>> return dsi; >>> } >>> @@ -206,7 +215,7 @@ mipi_dsi_device_register_full(struct mipi_dsi_host *host, >>> const struct mipi_dsi_device_info *info) >>> { >>> struct mipi_dsi_device *dsi; >>> - struct device *dev = host->dev; >>> + struct device *dev = host ? host->dev : NULL; >>> int ret; >>> >>> if (!info) { >>> @@ -230,11 +239,17 @@ mipi_dsi_device_register_full(struct mipi_dsi_host *host, >>> dsi->channel = info->channel; >>> strlcpy(dsi->name, info->type, sizeof(dsi->name)); >>> >>> - ret = mipi_dsi_device_add(dsi); >>> - if (ret) { >>> - dev_err(dev, "failed to add DSI device %d\n", ret); >>> - kfree(dsi); >>> - return ERR_PTR(ret); >>> + if (!dsi->host) { >>> + mutex_lock(&host_lock); >>> + list_add(&dsi->list, &unattached_device_list); >>> + mutex_unlock(&host_lock); >>> + } else { >>> + ret = mipi_dsi_device_add(dsi); >>> + if (ret) { >>> + dev_err(dev, "failed to add DSI device %d\n", ret); >>> + kfree(dsi); >>> + return ERR_PTR(ret); >>> + } >>> } >>> >>> return dsi; >>> @@ -251,9 +266,6 @@ void mipi_dsi_device_unregister(struct mipi_dsi_device *dsi) >>> } >>> EXPORT_SYMBOL(mipi_dsi_device_unregister); >>> >>> -static DEFINE_MUTEX(host_lock); >>> -static LIST_HEAD(host_list); >>> - >>> /** >>> * of_find_mipi_dsi_host_by_node() - find the MIPI DSI host matching a >>> * device tree node >>> @@ -285,6 +297,7 @@ EXPORT_SYMBOL(of_find_mipi_dsi_host_by_node); >>> int mipi_dsi_host_register(struct mipi_dsi_host *host) >>> { >>> struct device_node *node; >>> + struct mipi_dsi_device *dsi, *temp; >>> >>> for_each_available_child_of_node(host->dev->of_node, node) { >>> /* skip nodes without reg property */ >>> @@ -295,6 +308,20 @@ int mipi_dsi_host_register(struct mipi_dsi_host *host) >>> >>> mutex_lock(&host_lock); >>> list_add_tail(&host->list, &host_list); >>> + >>> + /* If any DSI devices were registered under our OF node, then >>> + * 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) { >>> + dsi->host = host; >>> + dsi->dev.parent = host->dev; >>> + device_initialize(&dsi->dev); >>> + >>> + mipi_dsi_device_add(dsi); >>> + list_del_init(&dsi->list); >>> + } >>> + } >>> mutex_unlock(&host_lock); >>> >>> return 0; >>> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h >>> index 4fef19064b0f..699ea4acd5b6 100644 >>> --- a/include/drm/drm_mipi_dsi.h >>> +++ b/include/drm/drm_mipi_dsi.h >>> @@ -178,6 +178,9 @@ struct mipi_dsi_device { >>> unsigned int lanes; >>> enum mipi_dsi_pixel_format format; >>> unsigned long mode_flags; >>> + >>> + /* Entry on the unattached_device_list */ >>> + struct list_head list; >>> }; >>> >>> #define MIPI_DSI_MODULE_PREFIX "mipi-dsi:" >>> >
Archit Taneja <architt@codeaurora.org> 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().
Archit Taneja <architt@codeaurora.org> 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"?
On 07/15/2017 04:28 AM, Eric Anholt wrote: > Archit Taneja <architt@codeaurora.org> 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? 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. Ideally, a dev's 'of_node' should be left to NULL if we don't have a corresponding OF node. 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. Our check in mipi_dsi_host_register() could then be something like: if (dsi->host_node) == host->dev->of_node) { ... ... } Since Thierry also reviews drm_mipi_dsi.c changes, it would nice to get some comments from him too. Thanks, Archit
On 07/15/2017 04:31 AM, Eric Anholt wrote: > Archit Taneja <architt@codeaurora.org> 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 >
diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c index 1160a579e0dc..9cdd68a7dc0d 100644 --- a/drivers/gpu/drm/drm_mipi_dsi.c +++ b/drivers/gpu/drm/drm_mipi_dsi.c @@ -45,6 +45,13 @@ * subset of the MIPI DCS command set. */ +static DEFINE_MUTEX(host_lock); +static LIST_HEAD(host_list); +/* List of struct mipi_dsi_device which were registered while no host + * was available. + */ +static LIST_HEAD(unattached_device_list); + static int mipi_dsi_device_match(struct device *dev, struct device_driver *drv) { struct mipi_dsi_device *dsi = to_mipi_dsi_device(dev); @@ -138,10 +145,12 @@ static struct mipi_dsi_device *mipi_dsi_device_alloc(struct mipi_dsi_host *host) dsi->host = host; dsi->dev.bus = &mipi_dsi_bus_type; - dsi->dev.parent = host->dev; dsi->dev.type = &mipi_dsi_device_type; - device_initialize(&dsi->dev); + if (dsi->host) { + dsi->dev.parent = host->dev; + device_initialize(&dsi->dev); + } return dsi; } @@ -206,7 +215,7 @@ mipi_dsi_device_register_full(struct mipi_dsi_host *host, const struct mipi_dsi_device_info *info) { struct mipi_dsi_device *dsi; - struct device *dev = host->dev; + struct device *dev = host ? host->dev : NULL; int ret; if (!info) { @@ -230,11 +239,17 @@ mipi_dsi_device_register_full(struct mipi_dsi_host *host, dsi->channel = info->channel; strlcpy(dsi->name, info->type, sizeof(dsi->name)); - ret = mipi_dsi_device_add(dsi); - if (ret) { - dev_err(dev, "failed to add DSI device %d\n", ret); - kfree(dsi); - return ERR_PTR(ret); + if (!dsi->host) { + mutex_lock(&host_lock); + list_add(&dsi->list, &unattached_device_list); + mutex_unlock(&host_lock); + } else { + ret = mipi_dsi_device_add(dsi); + if (ret) { + dev_err(dev, "failed to add DSI device %d\n", ret); + kfree(dsi); + return ERR_PTR(ret); + } } return dsi; @@ -251,9 +266,6 @@ void mipi_dsi_device_unregister(struct mipi_dsi_device *dsi) } EXPORT_SYMBOL(mipi_dsi_device_unregister); -static DEFINE_MUTEX(host_lock); -static LIST_HEAD(host_list); - /** * of_find_mipi_dsi_host_by_node() - find the MIPI DSI host matching a * device tree node @@ -285,6 +297,7 @@ EXPORT_SYMBOL(of_find_mipi_dsi_host_by_node); int mipi_dsi_host_register(struct mipi_dsi_host *host) { struct device_node *node; + struct mipi_dsi_device *dsi, *temp; for_each_available_child_of_node(host->dev->of_node, node) { /* skip nodes without reg property */ @@ -295,6 +308,20 @@ int mipi_dsi_host_register(struct mipi_dsi_host *host) mutex_lock(&host_lock); list_add_tail(&host->list, &host_list); + + /* If any DSI devices were registered under our OF node, then + * 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) { + dsi->host = host; + dsi->dev.parent = host->dev; + device_initialize(&dsi->dev); + + mipi_dsi_device_add(dsi); + list_del_init(&dsi->list); + } + } mutex_unlock(&host_lock); return 0; diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h index 4fef19064b0f..699ea4acd5b6 100644 --- a/include/drm/drm_mipi_dsi.h +++ b/include/drm/drm_mipi_dsi.h @@ -178,6 +178,9 @@ struct mipi_dsi_device { unsigned int lanes; enum mipi_dsi_pixel_format format; unsigned long mode_flags; + + /* Entry on the unattached_device_list */ + struct list_head list; }; #define MIPI_DSI_MODULE_PREFIX "mipi-dsi:"
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. Signed-off-by: Eric Anholt <eric@anholt.net> --- drivers/gpu/drm/drm_mipi_dsi.c | 49 ++++++++++++++++++++++++++++++++---------- include/drm/drm_mipi_dsi.h | 3 +++ 2 files changed, 41 insertions(+), 11 deletions(-)