| Message ID | 20180725154644.25412-3-a.hajda@samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Add TOSHIBA TC358764 DSI/LVDS bridge driver | expand |
2018년 07월 26일 00:46에 Andrzej Hajda 이(가) 쓴 글: > From: Maciej Purski <m.purski@samsung.com> > > The current implementation assumes that the only possible peripheral > device for DSIM is a panel. Using an output bridge child device > should also be possible. > > If an output bridge is available, don't create a new connector. > Instead, call drm_bridge_attach() and set encoder's bridge to NULL > in order to avoid an out bridge from being visible by the framework, as > the DSI bus needs control on enabling its child output bridge. > > Such sequence is required by Toshiba TC358764 bridge, which is a DSI > peripheral bridge device. > > changed in v5: > - detach bridge in mipi_dsi detach callback > > Signed-off-by: Maciej Purski <m.purski@samsung.com> > [ a.hajda@samsung.com: v5 ] > Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> > --- > drivers/gpu/drm/exynos/exynos_drm_dsi.c | 50 ++++++++++++++++--------- > 1 file changed, 32 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c > index 351403f9d245..f5f51f584fa0 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c > @@ -255,6 +255,7 @@ struct exynos_dsi { > struct mipi_dsi_host dsi_host; > struct drm_connector connector; > struct drm_panel *panel; > + struct drm_bridge *out_bridge; > struct device *dev; > > void __iomem *reg_base; > @@ -1499,7 +1500,30 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host, > struct mipi_dsi_device *device) > { > struct exynos_dsi *dsi = host_to_dsi(host); > - struct drm_device *drm = dsi->connector.dev; > + struct drm_encoder *encoder = &dsi->encoder; > + struct drm_device *drm = encoder->dev; > + struct drm_bridge *out_bridge; > + > + out_bridge = of_drm_find_bridge(device->dev.of_node); Is there any reason to find out_bridge without considering device tree graph binding? Then, you embedded bridge device node in dsi device node[1] but the binding document below doesn't mention about this, Documentation/devicetree/bindings/display/exynos/exynos_dsim.txt [1] https://lkml.org/lkml/2018/6/19/237 Thanks, Inki Dae > + if (out_bridge) { > + drm_bridge_attach(encoder, out_bridge, NULL); > + dsi->out_bridge = out_bridge; > + encoder->bridge = NULL; > + } else { > + int ret = exynos_dsi_create_connector(encoder); > + > + if (ret) { > + DRM_ERROR("failed to create connector ret = %d\n", ret); > + drm_encoder_cleanup(encoder); > + return ret; > + } > + > + dsi->panel = of_drm_find_panel(device->dev.of_node); > + if (dsi->panel) { > + drm_panel_attach(dsi->panel, &dsi->connector); > + dsi->connector.status = connector_status_connected; > + } > + } > > /* > * This is a temporary solution and should be made by more generic way. > @@ -1518,11 +1542,6 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host, > dsi->lanes = device->lanes; > dsi->format = device->format; > dsi->mode_flags = device->mode_flags; > - dsi->panel = of_drm_find_panel(device->dev.of_node); > - if (dsi->panel) { > - drm_panel_attach(dsi->panel, &dsi->connector); > - dsi->connector.status = connector_status_connected; > - } > exynos_drm_crtc_get_by_type(drm, EXYNOS_DISPLAY_TYPE_LCD)->i80_mode = > !(dsi->mode_flags & MIPI_DSI_MODE_VIDEO); > > @@ -1538,19 +1557,21 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host, > struct mipi_dsi_device *device) > { > struct exynos_dsi *dsi = host_to_dsi(host); > - struct drm_device *drm = dsi->connector.dev; > - > - mutex_lock(&drm->mode_config.mutex); > + struct drm_device *drm = dsi->encoder.dev; > > if (dsi->panel) { > + mutex_lock(&drm->mode_config.mutex); > exynos_dsi_disable(&dsi->encoder); > drm_panel_detach(dsi->panel); > dsi->panel = NULL; > dsi->connector.status = connector_status_disconnected; > + mutex_unlock(&drm->mode_config.mutex); > + } else { > + if (dsi->out_bridge->funcs->detach) > + dsi->out_bridge->funcs->detach(dsi->out_bridge); > + dsi->out_bridge = NULL; > } > > - mutex_unlock(&drm->mode_config.mutex); > - > if (drm->mode_config.poll_enabled) > drm_kms_helper_hotplug_event(drm); > > @@ -1654,13 +1675,6 @@ static int exynos_dsi_bind(struct device *dev, struct device *master, > if (ret < 0) > return ret; > > - ret = exynos_dsi_create_connector(encoder); > - if (ret) { > - DRM_ERROR("failed to create connector ret = %d\n", ret); > - drm_encoder_cleanup(encoder); > - return ret; > - } > - > if (dsi->in_bridge_node) { > in_bridge = of_drm_find_bridge(dsi->in_bridge_node); > if (in_bridge) > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07.08.2018 10:53, Inki Dae wrote: > > 2018년 07월 26일 00:46에 Andrzej Hajda 이(가) 쓴 글: >> From: Maciej Purski <m.purski@samsung.com> >> >> The current implementation assumes that the only possible peripheral >> device for DSIM is a panel. Using an output bridge child device >> should also be possible. >> >> If an output bridge is available, don't create a new connector. >> Instead, call drm_bridge_attach() and set encoder's bridge to NULL >> in order to avoid an out bridge from being visible by the framework, as >> the DSI bus needs control on enabling its child output bridge. >> >> Such sequence is required by Toshiba TC358764 bridge, which is a DSI >> peripheral bridge device. >> >> changed in v5: >> - detach bridge in mipi_dsi detach callback >> >> Signed-off-by: Maciej Purski <m.purski@samsung.com> >> [ a.hajda@samsung.com: v5 ] >> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> >> --- >> drivers/gpu/drm/exynos/exynos_drm_dsi.c | 50 ++++++++++++++++--------- >> 1 file changed, 32 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c >> index 351403f9d245..f5f51f584fa0 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c >> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c >> @@ -255,6 +255,7 @@ struct exynos_dsi { >> struct mipi_dsi_host dsi_host; >> struct drm_connector connector; >> struct drm_panel *panel; >> + struct drm_bridge *out_bridge; >> struct device *dev; >> >> void __iomem *reg_base; >> @@ -1499,7 +1500,30 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host, >> struct mipi_dsi_device *device) >> { >> struct exynos_dsi *dsi = host_to_dsi(host); >> - struct drm_device *drm = dsi->connector.dev; >> + struct drm_encoder *encoder = &dsi->encoder; >> + struct drm_device *drm = encoder->dev; >> + struct drm_bridge *out_bridge; >> + >> + out_bridge = of_drm_find_bridge(device->dev.of_node); > Is there any reason to find out_bridge without considering device tree graph binding? If the sink is a child MIPI-DSI device, the bindings can be omitted, as in case of all DSI panels in Exynos platforms. In case bindings are not present you cannot use graph functions. > > Then, you embedded bridge device node in dsi device node[1] but the binding document below doesn't mention about this, > Documentation/devicetree/bindings/display/exynos/exynos_dsim.txt But this document says about child nodes, and Toshiba bridge is MIPI-DSI child [1]. Regards Andrzej > > [1] https://lkml.org/lkml/2018/6/19/237 > > Thanks, > Inki Dae > >> + if (out_bridge) { >> + drm_bridge_attach(encoder, out_bridge, NULL); >> + dsi->out_bridge = out_bridge; >> + encoder->bridge = NULL; >> + } else { >> + int ret = exynos_dsi_create_connector(encoder); >> + >> + if (ret) { >> + DRM_ERROR("failed to create connector ret = %d\n", ret); >> + drm_encoder_cleanup(encoder); >> + return ret; >> + } >> + >> + dsi->panel = of_drm_find_panel(device->dev.of_node); >> + if (dsi->panel) { >> + drm_panel_attach(dsi->panel, &dsi->connector); >> + dsi->connector.status = connector_status_connected; >> + } >> + } >> >> /* >> * This is a temporary solution and should be made by more generic way. >> @@ -1518,11 +1542,6 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host, >> dsi->lanes = device->lanes; >> dsi->format = device->format; >> dsi->mode_flags = device->mode_flags; >> - dsi->panel = of_drm_find_panel(device->dev.of_node); >> - if (dsi->panel) { >> - drm_panel_attach(dsi->panel, &dsi->connector); >> - dsi->connector.status = connector_status_connected; >> - } >> exynos_drm_crtc_get_by_type(drm, EXYNOS_DISPLAY_TYPE_LCD)->i80_mode = >> !(dsi->mode_flags & MIPI_DSI_MODE_VIDEO); >> >> @@ -1538,19 +1557,21 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host, >> struct mipi_dsi_device *device) >> { >> struct exynos_dsi *dsi = host_to_dsi(host); >> - struct drm_device *drm = dsi->connector.dev; >> - >> - mutex_lock(&drm->mode_config.mutex); >> + struct drm_device *drm = dsi->encoder.dev; >> >> if (dsi->panel) { >> + mutex_lock(&drm->mode_config.mutex); >> exynos_dsi_disable(&dsi->encoder); >> drm_panel_detach(dsi->panel); >> dsi->panel = NULL; >> dsi->connector.status = connector_status_disconnected; >> + mutex_unlock(&drm->mode_config.mutex); >> + } else { >> + if (dsi->out_bridge->funcs->detach) >> + dsi->out_bridge->funcs->detach(dsi->out_bridge); >> + dsi->out_bridge = NULL; >> } >> >> - mutex_unlock(&drm->mode_config.mutex); >> - >> if (drm->mode_config.poll_enabled) >> drm_kms_helper_hotplug_event(drm); >> >> @@ -1654,13 +1675,6 @@ static int exynos_dsi_bind(struct device *dev, struct device *master, >> if (ret < 0) >> return ret; >> >> - ret = exynos_dsi_create_connector(encoder); >> - if (ret) { >> - DRM_ERROR("failed to create connector ret = %d\n", ret); >> - drm_encoder_cleanup(encoder); >> - return ret; >> - } >> - >> if (dsi->in_bridge_node) { >> in_bridge = of_drm_find_bridge(dsi->in_bridge_node); >> if (in_bridge) >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > >
2018년 08월 13일 20:17에 Andrzej Hajda 이(가) 쓴 글: > On 07.08.2018 10:53, Inki Dae wrote: >> >> 2018년 07월 26일 00:46에 Andrzej Hajda 이(가) 쓴 글: >>> From: Maciej Purski <m.purski@samsung.com> >>> >>> The current implementation assumes that the only possible peripheral >>> device for DSIM is a panel. Using an output bridge child device >>> should also be possible. >>> >>> If an output bridge is available, don't create a new connector. >>> Instead, call drm_bridge_attach() and set encoder's bridge to NULL >>> in order to avoid an out bridge from being visible by the framework, as >>> the DSI bus needs control on enabling its child output bridge. >>> >>> Such sequence is required by Toshiba TC358764 bridge, which is a DSI >>> peripheral bridge device. >>> >>> changed in v5: >>> - detach bridge in mipi_dsi detach callback >>> >>> Signed-off-by: Maciej Purski <m.purski@samsung.com> >>> [ a.hajda@samsung.com: v5 ] >>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> >>> --- >>> drivers/gpu/drm/exynos/exynos_drm_dsi.c | 50 ++++++++++++++++--------- >>> 1 file changed, 32 insertions(+), 18 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c >>> index 351403f9d245..f5f51f584fa0 100644 >>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c >>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c >>> @@ -255,6 +255,7 @@ struct exynos_dsi { >>> struct mipi_dsi_host dsi_host; >>> struct drm_connector connector; >>> struct drm_panel *panel; >>> + struct drm_bridge *out_bridge; >>> struct device *dev; >>> >>> void __iomem *reg_base; >>> @@ -1499,7 +1500,30 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host, >>> struct mipi_dsi_device *device) >>> { >>> struct exynos_dsi *dsi = host_to_dsi(host); >>> - struct drm_device *drm = dsi->connector.dev; >>> + struct drm_encoder *encoder = &dsi->encoder; >>> + struct drm_device *drm = encoder->dev; >>> + struct drm_bridge *out_bridge; >>> + >>> + out_bridge = of_drm_find_bridge(device->dev.of_node); >> Is there any reason to find out_bridge without considering device tree graph binding? > > If the sink is a child MIPI-DSI device, the bindings can be omitted, as > in case of all DSI panels in Exynos platforms. > In case bindings are not present you cannot use graph functions. In other words, this means that this patch doesn't allow to use the device tree graph binding. I.e., if other people wrote the graph things in his board dts file for the use of bridge device then with this patch he cannot use the bride device. So I think it would be better to allow to use both of them, as a child device and graph binding. How about trying to bind the graph things using drm_of_find_panel_or_bridge funtion first and then for child device way? And I'm not clear about what kinds of devices could be a child device of DSI, which isn't required for the graph binding. Is there any document I can refer to? Thanks, Inki Dae > >> >> Then, you embedded bridge device node in dsi device node[1] but the binding document below doesn't mention about this, >> Documentation/devicetree/bindings/display/exynos/exynos_dsim.txt > > But this document says about child nodes, and Toshiba bridge is MIPI-DSI > child [1]. > > Regards > Andrzej > >> >> [1] https://lkml.org/lkml/2018/6/19/237 >> >> Thanks, >> Inki Dae >> >>> + if (out_bridge) { >>> + drm_bridge_attach(encoder, out_bridge, NULL); >>> + dsi->out_bridge = out_bridge; >>> + encoder->bridge = NULL; >>> + } else { >>> + int ret = exynos_dsi_create_connector(encoder); >>> + >>> + if (ret) { >>> + DRM_ERROR("failed to create connector ret = %d\n", ret); >>> + drm_encoder_cleanup(encoder); >>> + return ret; >>> + } >>> + >>> + dsi->panel = of_drm_find_panel(device->dev.of_node); >>> + if (dsi->panel) { >>> + drm_panel_attach(dsi->panel, &dsi->connector); >>> + dsi->connector.status = connector_status_connected; >>> + } >>> + } >>> >>> /* >>> * This is a temporary solution and should be made by more generic way. >>> @@ -1518,11 +1542,6 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host, >>> dsi->lanes = device->lanes; >>> dsi->format = device->format; >>> dsi->mode_flags = device->mode_flags; >>> - dsi->panel = of_drm_find_panel(device->dev.of_node); >>> - if (dsi->panel) { >>> - drm_panel_attach(dsi->panel, &dsi->connector); >>> - dsi->connector.status = connector_status_connected; >>> - } >>> exynos_drm_crtc_get_by_type(drm, EXYNOS_DISPLAY_TYPE_LCD)->i80_mode = >>> !(dsi->mode_flags & MIPI_DSI_MODE_VIDEO); >>> >>> @@ -1538,19 +1557,21 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host, >>> struct mipi_dsi_device *device) >>> { >>> struct exynos_dsi *dsi = host_to_dsi(host); >>> - struct drm_device *drm = dsi->connector.dev; >>> - >>> - mutex_lock(&drm->mode_config.mutex); >>> + struct drm_device *drm = dsi->encoder.dev; >>> >>> if (dsi->panel) { >>> + mutex_lock(&drm->mode_config.mutex); >>> exynos_dsi_disable(&dsi->encoder); >>> drm_panel_detach(dsi->panel); >>> dsi->panel = NULL; >>> dsi->connector.status = connector_status_disconnected; >>> + mutex_unlock(&drm->mode_config.mutex); >>> + } else { >>> + if (dsi->out_bridge->funcs->detach) >>> + dsi->out_bridge->funcs->detach(dsi->out_bridge); >>> + dsi->out_bridge = NULL; >>> } >>> >>> - mutex_unlock(&drm->mode_config.mutex); >>> - >>> if (drm->mode_config.poll_enabled) >>> drm_kms_helper_hotplug_event(drm); >>> >>> @@ -1654,13 +1675,6 @@ static int exynos_dsi_bind(struct device *dev, struct device *master, >>> if (ret < 0) >>> return ret; >>> >>> - ret = exynos_dsi_create_connector(encoder); >>> - if (ret) { >>> - DRM_ERROR("failed to create connector ret = %d\n", ret); >>> - drm_encoder_cleanup(encoder); >>> - return ret; >>> - } >>> - >>> if (dsi->in_bridge_node) { >>> in_bridge = of_drm_find_bridge(dsi->in_bridge_node); >>> if (in_bridge) >>> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> > > >
On 17.08.2018 03:56, Inki Dae wrote: > > 2018년 08월 13일 20:17에 Andrzej Hajda 이(가) 쓴 글: >> On 07.08.2018 10:53, Inki Dae wrote: >>> 2018년 07월 26일 00:46에 Andrzej Hajda 이(가) 쓴 글: >>>> From: Maciej Purski <m.purski@samsung.com> >>>> >>>> The current implementation assumes that the only possible peripheral >>>> device for DSIM is a panel. Using an output bridge child device >>>> should also be possible. >>>> >>>> If an output bridge is available, don't create a new connector. >>>> Instead, call drm_bridge_attach() and set encoder's bridge to NULL >>>> in order to avoid an out bridge from being visible by the framework, as >>>> the DSI bus needs control on enabling its child output bridge. >>>> >>>> Such sequence is required by Toshiba TC358764 bridge, which is a DSI >>>> peripheral bridge device. >>>> >>>> changed in v5: >>>> - detach bridge in mipi_dsi detach callback >>>> >>>> Signed-off-by: Maciej Purski <m.purski@samsung.com> >>>> [ a.hajda@samsung.com: v5 ] >>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> >>>> --- >>>> drivers/gpu/drm/exynos/exynos_drm_dsi.c | 50 ++++++++++++++++--------- >>>> 1 file changed, 32 insertions(+), 18 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c >>>> index 351403f9d245..f5f51f584fa0 100644 >>>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c >>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c >>>> @@ -255,6 +255,7 @@ struct exynos_dsi { >>>> struct mipi_dsi_host dsi_host; >>>> struct drm_connector connector; >>>> struct drm_panel *panel; >>>> + struct drm_bridge *out_bridge; >>>> struct device *dev; >>>> >>>> void __iomem *reg_base; >>>> @@ -1499,7 +1500,30 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host, >>>> struct mipi_dsi_device *device) >>>> { >>>> struct exynos_dsi *dsi = host_to_dsi(host); >>>> - struct drm_device *drm = dsi->connector.dev; >>>> + struct drm_encoder *encoder = &dsi->encoder; >>>> + struct drm_device *drm = encoder->dev; >>>> + struct drm_bridge *out_bridge; >>>> + >>>> + out_bridge = of_drm_find_bridge(device->dev.of_node); >>> Is there any reason to find out_bridge without considering device tree graph binding? >> If the sink is a child MIPI-DSI device, the bindings can be omitted, as >> in case of all DSI panels in Exynos platforms. >> In case bindings are not present you cannot use graph functions. > In other words, this means that this patch doesn't allow to use the device tree graph binding. > I.e., if other people wrote the graph things in his board dts file for the use of bridge device then with this patch he cannot use the bride device. > > So I think it would be better to allow to use both of them, as a child device and graph binding. > > How about trying to bind the graph things using drm_of_find_panel_or_bridge funtion first and then for child device way? It could be done this way, but it should be done for panels and for bridges, and it should be rather generic helper, not Exynos specific. So it is something which require additional investigation, discussion and separate patchset. On the other side this patch is enough to correctly handle all DSI bridges in existing Exynos boards. > > And I'm not clear about what kinds of devices could be a child device of DSI, which isn't required for the graph binding. > Is there any document I can refer to? DSI devices (peripherals) should be described as child nodes of DSI host node in DT bindings, it is described in [1]. And you can find all dsi panels in exynos based boards are modeled this way. And the graph documentation [2] says that graphs should be used for connections that "can not be inferred from device tree parent-child relationships", it was emphasised multiple times by Rob in discussions regarding bindings of DSI devices. [1]: Documentation/devicetree/bindings/display/mipi-dsi-bus.txt [2]: Documentation/devicetree/bindings/graph.txt Regards Andrzej > > Thanks, > Inki Dae > >>> Then, you embedded bridge device node in dsi device node[1] but the binding document below doesn't mention about this, >>> Documentation/devicetree/bindings/display/exynos/exynos_dsim.txt >> But this document says about child nodes, and Toshiba bridge is MIPI-DSI >> child [1]. >> >> Regards >> Andrzej >> >>> [1] https://lkml.org/lkml/2018/6/19/237 >>> >>> Thanks, >>> Inki Dae >>> >>>> + if (out_bridge) { >>>> + drm_bridge_attach(encoder, out_bridge, NULL); >>>> + dsi->out_bridge = out_bridge; >>>> + encoder->bridge = NULL; >>>> + } else { >>>> + int ret = exynos_dsi_create_connector(encoder); >>>> + >>>> + if (ret) { >>>> + DRM_ERROR("failed to create connector ret = %d\n", ret); >>>> + drm_encoder_cleanup(encoder); >>>> + return ret; >>>> + } >>>> + >>>> + dsi->panel = of_drm_find_panel(device->dev.of_node); >>>> + if (dsi->panel) { >>>> + drm_panel_attach(dsi->panel, &dsi->connector); >>>> + dsi->connector.status = connector_status_connected; >>>> + } >>>> + } >>>> >>>> /* >>>> * This is a temporary solution and should be made by more generic way. >>>> @@ -1518,11 +1542,6 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host, >>>> dsi->lanes = device->lanes; >>>> dsi->format = device->format; >>>> dsi->mode_flags = device->mode_flags; >>>> - dsi->panel = of_drm_find_panel(device->dev.of_node); >>>> - if (dsi->panel) { >>>> - drm_panel_attach(dsi->panel, &dsi->connector); >>>> - dsi->connector.status = connector_status_connected; >>>> - } >>>> exynos_drm_crtc_get_by_type(drm, EXYNOS_DISPLAY_TYPE_LCD)->i80_mode = >>>> !(dsi->mode_flags & MIPI_DSI_MODE_VIDEO); >>>> >>>> @@ -1538,19 +1557,21 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host, >>>> struct mipi_dsi_device *device) >>>> { >>>> struct exynos_dsi *dsi = host_to_dsi(host); >>>> - struct drm_device *drm = dsi->connector.dev; >>>> - >>>> - mutex_lock(&drm->mode_config.mutex); >>>> + struct drm_device *drm = dsi->encoder.dev; >>>> >>>> if (dsi->panel) { >>>> + mutex_lock(&drm->mode_config.mutex); >>>> exynos_dsi_disable(&dsi->encoder); >>>> drm_panel_detach(dsi->panel); >>>> dsi->panel = NULL; >>>> dsi->connector.status = connector_status_disconnected; >>>> + mutex_unlock(&drm->mode_config.mutex); >>>> + } else { >>>> + if (dsi->out_bridge->funcs->detach) >>>> + dsi->out_bridge->funcs->detach(dsi->out_bridge); >>>> + dsi->out_bridge = NULL; >>>> } >>>> >>>> - mutex_unlock(&drm->mode_config.mutex); >>>> - >>>> if (drm->mode_config.poll_enabled) >>>> drm_kms_helper_hotplug_event(drm); >>>> >>>> @@ -1654,13 +1675,6 @@ static int exynos_dsi_bind(struct device *dev, struct device *master, >>>> if (ret < 0) >>>> return ret; >>>> >>>> - ret = exynos_dsi_create_connector(encoder); >>>> - if (ret) { >>>> - DRM_ERROR("failed to create connector ret = %d\n", ret); >>>> - drm_encoder_cleanup(encoder); >>>> - return ret; >>>> - } >>>> - >>>> if (dsi->in_bridge_node) { >>>> in_bridge = of_drm_find_bridge(dsi->in_bridge_node); >>>> if (in_bridge) >>>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> >>> >> >> >
2018년 08월 20일 18:00에 Andrzej Hajda 이(가) 쓴 글: > On 17.08.2018 03:56, Inki Dae wrote: >> >> 2018년 08월 13일 20:17에 Andrzej Hajda 이(가) 쓴 글: >>> On 07.08.2018 10:53, Inki Dae wrote: >>>> 2018년 07월 26일 00:46에 Andrzej Hajda 이(가) 쓴 글: >>>>> From: Maciej Purski <m.purski@samsung.com> >>>>> >>>>> The current implementation assumes that the only possible peripheral >>>>> device for DSIM is a panel. Using an output bridge child device >>>>> should also be possible. >>>>> >>>>> If an output bridge is available, don't create a new connector. >>>>> Instead, call drm_bridge_attach() and set encoder's bridge to NULL >>>>> in order to avoid an out bridge from being visible by the framework, as >>>>> the DSI bus needs control on enabling its child output bridge. >>>>> >>>>> Such sequence is required by Toshiba TC358764 bridge, which is a DSI >>>>> peripheral bridge device. >>>>> >>>>> changed in v5: >>>>> - detach bridge in mipi_dsi detach callback >>>>> >>>>> Signed-off-by: Maciej Purski <m.purski@samsung.com> >>>>> [ a.hajda@samsung.com: v5 ] >>>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> >>>>> --- >>>>> drivers/gpu/drm/exynos/exynos_drm_dsi.c | 50 ++++++++++++++++--------- >>>>> 1 file changed, 32 insertions(+), 18 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c >>>>> index 351403f9d245..f5f51f584fa0 100644 >>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c >>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c >>>>> @@ -255,6 +255,7 @@ struct exynos_dsi { >>>>> struct mipi_dsi_host dsi_host; >>>>> struct drm_connector connector; >>>>> struct drm_panel *panel; >>>>> + struct drm_bridge *out_bridge; >>>>> struct device *dev; >>>>> >>>>> void __iomem *reg_base; >>>>> @@ -1499,7 +1500,30 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host, >>>>> struct mipi_dsi_device *device) >>>>> { >>>>> struct exynos_dsi *dsi = host_to_dsi(host); >>>>> - struct drm_device *drm = dsi->connector.dev; >>>>> + struct drm_encoder *encoder = &dsi->encoder; >>>>> + struct drm_device *drm = encoder->dev; >>>>> + struct drm_bridge *out_bridge; >>>>> + >>>>> + out_bridge = of_drm_find_bridge(device->dev.of_node); >>>> Is there any reason to find out_bridge without considering device tree graph binding? >>> If the sink is a child MIPI-DSI device, the bindings can be omitted, as >>> in case of all DSI panels in Exynos platforms. >>> In case bindings are not present you cannot use graph functions. >> In other words, this means that this patch doesn't allow to use the device tree graph binding. >> I.e., if other people wrote the graph things in his board dts file for the use of bridge device then with this patch he cannot use the bride device. >> >> So I think it would be better to allow to use both of them, as a child device and graph binding. >> >> How about trying to bind the graph things using drm_of_find_panel_or_bridge funtion first and then for child device way? > > It could be done this way, but it should be done for panels and for > bridges, and it should be rather generic helper, not Exynos specific. So As the function name says, drm_of_find_panel_or_bridge function will return panel, bridge or both of them according to given arguments. > it is something which require additional investigation, discussion and > separate patchset. So I think we wouldn't need additional discussion at all becuase we don't touch panel but add only bridge device as output, and the use of this function for bridge looks more generic way. Of course, as a separate patch, we could consider using this function for panel device later. > On the other side this patch is enough to correctly handle all DSI > bridges in existing Exynos boards. > >> >> And I'm not clear about what kinds of devices could be a child device of DSI, which isn't required for the graph binding. >> Is there any document I can refer to? > > DSI devices (peripherals) should be described as child nodes of DSI host > node in DT bindings, it is described in [1]. And you can find all dsi > panels in exynos based boards are modeled this way. > And the graph documentation [2] says that graphs should be used for > connections that "can not be inferred from device tree parent-child > relationships", it was emphasised multiple times by Rob in discussions > regarding bindings of DSI devices. Hm... I don't see why Rob introduced drm_of_find_panel_or_bridge fucntion[1] if DSI devices should be described as child nodes. And some DSI drivers of other ARM DRM - vc4, mediatek, and kirin - use only drm_of_find_panel_or_bridge funtion. [1] commit 1f2db3034c9f16ed918e34809167546f21d0fcb4 Author: Rob Herring <robh@kernel.org> Date: Wed Mar 22 08:26:05 2017 -0500 drm: of: introduce drm_of_find_panel_or_bridge 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. Signed-off-by: Rob Herring <robh@kernel.org> Acked-by: Philipp Zabel <p.zabel@pengutronix.de> [seanpaul dropped extern from drm_of.h] Signed-off-by: Sean Paul <seanpaul@chromium.org> Thanks, Inki Dae > > [1]: Documentation/devicetree/bindings/display/mipi-dsi-bus.txt > [2]: Documentation/devicetree/bindings/graph.txt > > Regards > Andrzej > >> >> Thanks, >> Inki Dae >> >>>> Then, you embedded bridge device node in dsi device node[1] but the binding document below doesn't mention about this, >>>> Documentation/devicetree/bindings/display/exynos/exynos_dsim.txt >>> But this document says about child nodes, and Toshiba bridge is MIPI-DSI >>> child [1]. >>> >>> Regards >>> Andrzej >>> >>>> [1] https://lkml.org/lkml/2018/6/19/237 >>>> >>>> Thanks, >>>> Inki Dae >>>> >>>>> + if (out_bridge) { >>>>> + drm_bridge_attach(encoder, out_bridge, NULL); >>>>> + dsi->out_bridge = out_bridge; >>>>> + encoder->bridge = NULL; >>>>> + } else { >>>>> + int ret = exynos_dsi_create_connector(encoder); >>>>> + >>>>> + if (ret) { >>>>> + DRM_ERROR("failed to create connector ret = %d\n", ret); >>>>> + drm_encoder_cleanup(encoder); >>>>> + return ret; >>>>> + } >>>>> + >>>>> + dsi->panel = of_drm_find_panel(device->dev.of_node); >>>>> + if (dsi->panel) { >>>>> + drm_panel_attach(dsi->panel, &dsi->connector); >>>>> + dsi->connector.status = connector_status_connected; >>>>> + } >>>>> + } >>>>> >>>>> /* >>>>> * This is a temporary solution and should be made by more generic way. >>>>> @@ -1518,11 +1542,6 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host, >>>>> dsi->lanes = device->lanes; >>>>> dsi->format = device->format; >>>>> dsi->mode_flags = device->mode_flags; >>>>> - dsi->panel = of_drm_find_panel(device->dev.of_node); >>>>> - if (dsi->panel) { >>>>> - drm_panel_attach(dsi->panel, &dsi->connector); >>>>> - dsi->connector.status = connector_status_connected; >>>>> - } >>>>> exynos_drm_crtc_get_by_type(drm, EXYNOS_DISPLAY_TYPE_LCD)->i80_mode = >>>>> !(dsi->mode_flags & MIPI_DSI_MODE_VIDEO); >>>>> >>>>> @@ -1538,19 +1557,21 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host, >>>>> struct mipi_dsi_device *device) >>>>> { >>>>> struct exynos_dsi *dsi = host_to_dsi(host); >>>>> - struct drm_device *drm = dsi->connector.dev; >>>>> - >>>>> - mutex_lock(&drm->mode_config.mutex); >>>>> + struct drm_device *drm = dsi->encoder.dev; >>>>> >>>>> if (dsi->panel) { >>>>> + mutex_lock(&drm->mode_config.mutex); >>>>> exynos_dsi_disable(&dsi->encoder); >>>>> drm_panel_detach(dsi->panel); >>>>> dsi->panel = NULL; >>>>> dsi->connector.status = connector_status_disconnected; >>>>> + mutex_unlock(&drm->mode_config.mutex); >>>>> + } else { >>>>> + if (dsi->out_bridge->funcs->detach) >>>>> + dsi->out_bridge->funcs->detach(dsi->out_bridge); >>>>> + dsi->out_bridge = NULL; >>>>> } >>>>> >>>>> - mutex_unlock(&drm->mode_config.mutex); >>>>> - >>>>> if (drm->mode_config.poll_enabled) >>>>> drm_kms_helper_hotplug_event(drm); >>>>> >>>>> @@ -1654,13 +1675,6 @@ static int exynos_dsi_bind(struct device *dev, struct device *master, >>>>> if (ret < 0) >>>>> return ret; >>>>> >>>>> - ret = exynos_dsi_create_connector(encoder); >>>>> - if (ret) { >>>>> - DRM_ERROR("failed to create connector ret = %d\n", ret); >>>>> - drm_encoder_cleanup(encoder); >>>>> - return ret; >>>>> - } >>>>> - >>>>> if (dsi->in_bridge_node) { >>>>> in_bridge = of_drm_find_bridge(dsi->in_bridge_node); >>>>> if (in_bridge) >>>>> >>>> -- >>>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in >>>> the body of a message to majordomo@vger.kernel.org >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>> >>>> >>> >>> >> > > >
On 21.08.2018 07:27, Inki Dae wrote: > > 2018년 08월 20일 18:00에 Andrzej Hajda 이(가) 쓴 글: >> On 17.08.2018 03:56, Inki Dae wrote: >>> 2018년 08월 13일 20:17에 Andrzej Hajda 이(가) 쓴 글: >>>> On 07.08.2018 10:53, Inki Dae wrote: >>>>> 2018년 07월 26일 00:46에 Andrzej Hajda 이(가) 쓴 글: >>>>>> From: Maciej Purski <m.purski@samsung.com> >>>>>> >>>>>> The current implementation assumes that the only possible peripheral >>>>>> device for DSIM is a panel. Using an output bridge child device >>>>>> should also be possible. >>>>>> >>>>>> If an output bridge is available, don't create a new connector. >>>>>> Instead, call drm_bridge_attach() and set encoder's bridge to NULL >>>>>> in order to avoid an out bridge from being visible by the framework, as >>>>>> the DSI bus needs control on enabling its child output bridge. >>>>>> >>>>>> Such sequence is required by Toshiba TC358764 bridge, which is a DSI >>>>>> peripheral bridge device. >>>>>> >>>>>> changed in v5: >>>>>> - detach bridge in mipi_dsi detach callback >>>>>> >>>>>> Signed-off-by: Maciej Purski <m.purski@samsung.com> >>>>>> [ a.hajda@samsung.com: v5 ] >>>>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> >>>>>> --- >>>>>> drivers/gpu/drm/exynos/exynos_drm_dsi.c | 50 ++++++++++++++++--------- >>>>>> 1 file changed, 32 insertions(+), 18 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c >>>>>> index 351403f9d245..f5f51f584fa0 100644 >>>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c >>>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c >>>>>> @@ -255,6 +255,7 @@ struct exynos_dsi { >>>>>> struct mipi_dsi_host dsi_host; >>>>>> struct drm_connector connector; >>>>>> struct drm_panel *panel; >>>>>> + struct drm_bridge *out_bridge; >>>>>> struct device *dev; >>>>>> >>>>>> void __iomem *reg_base; >>>>>> @@ -1499,7 +1500,30 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host, >>>>>> struct mipi_dsi_device *device) >>>>>> { >>>>>> struct exynos_dsi *dsi = host_to_dsi(host); >>>>>> - struct drm_device *drm = dsi->connector.dev; >>>>>> + struct drm_encoder *encoder = &dsi->encoder; >>>>>> + struct drm_device *drm = encoder->dev; >>>>>> + struct drm_bridge *out_bridge; >>>>>> + >>>>>> + out_bridge = of_drm_find_bridge(device->dev.of_node); >>>>> Is there any reason to find out_bridge without considering device tree graph binding? >>>> If the sink is a child MIPI-DSI device, the bindings can be omitted, as >>>> in case of all DSI panels in Exynos platforms. >>>> In case bindings are not present you cannot use graph functions. >>> In other words, this means that this patch doesn't allow to use the device tree graph binding. >>> I.e., if other people wrote the graph things in his board dts file for the use of bridge device then with this patch he cannot use the bride device. >>> >>> So I think it would be better to allow to use both of them, as a child device and graph binding. >>> >>> How about trying to bind the graph things using drm_of_find_panel_or_bridge funtion first and then for child device way? >> It could be done this way, but it should be done for panels and for >> bridges, and it should be rather generic helper, not Exynos specific. So > As the function name says, drm_of_find_panel_or_bridge function will return panel, bridge or both of them according to given arguments. > > >> it is something which require additional investigation, discussion and >> separate patchset. > So I think we wouldn't need additional discussion at all becuase we don't touch panel but add only bridge device as output, and the use of this function for bridge looks more generic way. But drm_of_find_panel_or_bridge is only for looking for non-dsi devices, or more specifically for looking for devices connected in DTS via DT-graph. This is not case of panels and bridges used in Exynos boards. So this function is currently useless with our boards. Maybe some day we will have bridge/panel controlled via I2C and then it will become useful, but for now it serves for nothing. > Of course, as a separate patch, we could consider using this function for panel device later. As you said drm_of_find_panel_or_bridge looks for panel and/or bridge, and it was created to merge similar code in panel and bridge lookup, using it only for bridges and not for panels seems against it's purpose. > >> On the other side this patch is enough to correctly handle all DSI >> bridges in existing Exynos boards. >> >>> And I'm not clear about what kinds of devices could be a child device of DSI, which isn't required for the graph binding. >>> Is there any document I can refer to? >> DSI devices (peripherals) should be described as child nodes of DSI host >> node in DT bindings, it is described in [1]. And you can find all dsi >> panels in exynos based boards are modeled this way. >> And the graph documentation [2] says that graphs should be used for >> connections that "can not be inferred from device tree parent-child >> relationships", it was emphasised multiple times by Rob in discussions >> regarding bindings of DSI devices. > Hm... I don't see why Rob introduced drm_of_find_panel_or_bridge fucntion[1] if DSI devices should be described as child nodes. > And some DSI drivers of other ARM DRM - vc4, mediatek, and kirin - use only drm_of_find_panel_or_bridge funtion. Because this function was created for i2c/spi controlled bridges/panels and probably these boards use only such devices. Regards Andrzej
2018년 08월 21일 20:21에 Andrzej Hajda 이(가) 쓴 글: > On 21.08.2018 07:27, Inki Dae wrote: >> >> 2018년 08월 20일 18:00에 Andrzej Hajda 이(가) 쓴 글: >>> On 17.08.2018 03:56, Inki Dae wrote: >>>> 2018년 08월 13일 20:17에 Andrzej Hajda 이(가) 쓴 글: >>>>> On 07.08.2018 10:53, Inki Dae wrote: >>>>>> 2018년 07월 26일 00:46에 Andrzej Hajda 이(가) 쓴 글: >>>>>>> From: Maciej Purski <m.purski@samsung.com> >>>>>>> >>>>>>> The current implementation assumes that the only possible peripheral >>>>>>> device for DSIM is a panel. Using an output bridge child device >>>>>>> should also be possible. >>>>>>> >>>>>>> If an output bridge is available, don't create a new connector. >>>>>>> Instead, call drm_bridge_attach() and set encoder's bridge to NULL >>>>>>> in order to avoid an out bridge from being visible by the framework, as >>>>>>> the DSI bus needs control on enabling its child output bridge. >>>>>>> >>>>>>> Such sequence is required by Toshiba TC358764 bridge, which is a DSI >>>>>>> peripheral bridge device. >>>>>>> >>>>>>> changed in v5: >>>>>>> - detach bridge in mipi_dsi detach callback >>>>>>> >>>>>>> Signed-off-by: Maciej Purski <m.purski@samsung.com> >>>>>>> [ a.hajda@samsung.com: v5 ] >>>>>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> >>>>>>> --- >>>>>>> drivers/gpu/drm/exynos/exynos_drm_dsi.c | 50 ++++++++++++++++--------- >>>>>>> 1 file changed, 32 insertions(+), 18 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c >>>>>>> index 351403f9d245..f5f51f584fa0 100644 >>>>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c >>>>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c >>>>>>> @@ -255,6 +255,7 @@ struct exynos_dsi { >>>>>>> struct mipi_dsi_host dsi_host; >>>>>>> struct drm_connector connector; >>>>>>> struct drm_panel *panel; >>>>>>> + struct drm_bridge *out_bridge; >>>>>>> struct device *dev; >>>>>>> >>>>>>> void __iomem *reg_base; >>>>>>> @@ -1499,7 +1500,30 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host, >>>>>>> struct mipi_dsi_device *device) >>>>>>> { >>>>>>> struct exynos_dsi *dsi = host_to_dsi(host); >>>>>>> - struct drm_device *drm = dsi->connector.dev; >>>>>>> + struct drm_encoder *encoder = &dsi->encoder; >>>>>>> + struct drm_device *drm = encoder->dev; >>>>>>> + struct drm_bridge *out_bridge; >>>>>>> + >>>>>>> + out_bridge = of_drm_find_bridge(device->dev.of_node); >>>>>> Is there any reason to find out_bridge without considering device tree graph binding? >>>>> If the sink is a child MIPI-DSI device, the bindings can be omitted, as >>>>> in case of all DSI panels in Exynos platforms. >>>>> In case bindings are not present you cannot use graph functions. >>>> In other words, this means that this patch doesn't allow to use the device tree graph binding. >>>> I.e., if other people wrote the graph things in his board dts file for the use of bridge device then with this patch he cannot use the bride device. >>>> >>>> So I think it would be better to allow to use both of them, as a child device and graph binding. >>>> >>>> How about trying to bind the graph things using drm_of_find_panel_or_bridge funtion first and then for child device way? >>> It could be done this way, but it should be done for panels and for >>> bridges, and it should be rather generic helper, not Exynos specific. So >> As the function name says, drm_of_find_panel_or_bridge function will return panel, bridge or both of them according to given arguments. >> >> >>> it is something which require additional investigation, discussion and >>> separate patchset. >> So I think we wouldn't need additional discussion at all becuase we don't touch panel but add only bridge device as output, and the use of this function for bridge looks more generic way. > > But drm_of_find_panel_or_bridge is only for looking for non-dsi devices, > or more specifically for looking for devices connected in DTS via > DT-graph. This is not case of panels and bridges used in Exynos boards. > So this function is currently useless with our boards. Maybe some day we > will have bridge/panel controlled via I2C and then it will become > useful, but for now it serves for nothing. > >> Of course, as a separate patch, we could consider using this function for panel device later. > > As you said drm_of_find_panel_or_bridge looks for panel and/or bridge, > and it was created to merge similar code in panel and bridge lookup, > using it only for bridges and not for panels seems against it's purpose. > >> >>> On the other side this patch is enough to correctly handle all DSI >>> bridges in existing Exynos boards. >>> >>>> And I'm not clear about what kinds of devices could be a child device of DSI, which isn't required for the graph binding. >>>> Is there any document I can refer to? >>> DSI devices (peripherals) should be described as child nodes of DSI host >>> node in DT bindings, it is described in [1]. And you can find all dsi >>> panels in exynos based boards are modeled this way. >>> And the graph documentation [2] says that graphs should be used for >>> connections that "can not be inferred from device tree parent-child >>> relationships", it was emphasised multiple times by Rob in discussions >>> regarding bindings of DSI devices. >> Hm... I don't see why Rob introduced drm_of_find_panel_or_bridge fucntion[1] if DSI devices should be described as child nodes. >> And some DSI drivers of other ARM DRM - vc4, mediatek, and kirin - use only drm_of_find_panel_or_bridge funtion. > > Because this function was created for i2c/spi controlled bridges/panels > and probably these boards use only such devices. Clear. I will pick them up. BTW, the bridge and panelI mentioned above are child devices of DSI driver. So I'm not sure but the child devices would be controlled by MIPI DSI protocol not by i2c/spi. If you are right then the devices - bridge or panel - are controlled by i2c or spi and only image data are transferred from DSI to bridge or panel by MIPI lanes? Thanks, Inki Dae > > Regards > Andrzej > > >
On 22.08.2018 04:59, Inki Dae wrote: > > 2018년 08월 21일 20:21에 Andrzej Hajda 이(가) 쓴 글: >> On 21.08.2018 07:27, Inki Dae wrote: >>> 2018년 08월 20일 18:00에 Andrzej Hajda 이(가) 쓴 글: >>>> On 17.08.2018 03:56, Inki Dae wrote: >>>>> 2018년 08월 13일 20:17에 Andrzej Hajda 이(가) 쓴 글: >>>>>> On 07.08.2018 10:53, Inki Dae wrote: >>>>>>> 2018년 07월 26일 00:46에 Andrzej Hajda 이(가) 쓴 글: >>>>>>>> From: Maciej Purski <m.purski@samsung.com> >>>>>>>> >>>>>>>> The current implementation assumes that the only possible peripheral >>>>>>>> device for DSIM is a panel. Using an output bridge child device >>>>>>>> should also be possible. >>>>>>>> >>>>>>>> If an output bridge is available, don't create a new connector. >>>>>>>> Instead, call drm_bridge_attach() and set encoder's bridge to NULL >>>>>>>> in order to avoid an out bridge from being visible by the framework, as >>>>>>>> the DSI bus needs control on enabling its child output bridge. >>>>>>>> >>>>>>>> Such sequence is required by Toshiba TC358764 bridge, which is a DSI >>>>>>>> peripheral bridge device. >>>>>>>> >>>>>>>> changed in v5: >>>>>>>> - detach bridge in mipi_dsi detach callback >>>>>>>> >>>>>>>> Signed-off-by: Maciej Purski <m.purski@samsung.com> >>>>>>>> [ a.hajda@samsung.com: v5 ] >>>>>>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> >>>>>>>> --- >>>>>>>> drivers/gpu/drm/exynos/exynos_drm_dsi.c | 50 ++++++++++++++++--------- >>>>>>>> 1 file changed, 32 insertions(+), 18 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c >>>>>>>> index 351403f9d245..f5f51f584fa0 100644 >>>>>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c >>>>>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c >>>>>>>> @@ -255,6 +255,7 @@ struct exynos_dsi { >>>>>>>> struct mipi_dsi_host dsi_host; >>>>>>>> struct drm_connector connector; >>>>>>>> struct drm_panel *panel; >>>>>>>> + struct drm_bridge *out_bridge; >>>>>>>> struct device *dev; >>>>>>>> >>>>>>>> void __iomem *reg_base; >>>>>>>> @@ -1499,7 +1500,30 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host, >>>>>>>> struct mipi_dsi_device *device) >>>>>>>> { >>>>>>>> struct exynos_dsi *dsi = host_to_dsi(host); >>>>>>>> - struct drm_device *drm = dsi->connector.dev; >>>>>>>> + struct drm_encoder *encoder = &dsi->encoder; >>>>>>>> + struct drm_device *drm = encoder->dev; >>>>>>>> + struct drm_bridge *out_bridge; >>>>>>>> + >>>>>>>> + out_bridge = of_drm_find_bridge(device->dev.of_node); >>>>>>> Is there any reason to find out_bridge without considering device tree graph binding? >>>>>> If the sink is a child MIPI-DSI device, the bindings can be omitted, as >>>>>> in case of all DSI panels in Exynos platforms. >>>>>> In case bindings are not present you cannot use graph functions. >>>>> In other words, this means that this patch doesn't allow to use the device tree graph binding. >>>>> I.e., if other people wrote the graph things in his board dts file for the use of bridge device then with this patch he cannot use the bride device. >>>>> >>>>> So I think it would be better to allow to use both of them, as a child device and graph binding. >>>>> >>>>> How about trying to bind the graph things using drm_of_find_panel_or_bridge funtion first and then for child device way? >>>> It could be done this way, but it should be done for panels and for >>>> bridges, and it should be rather generic helper, not Exynos specific. So >>> As the function name says, drm_of_find_panel_or_bridge function will return panel, bridge or both of them according to given arguments. >>> >>> >>>> it is something which require additional investigation, discussion and >>>> separate patchset. >>> So I think we wouldn't need additional discussion at all becuase we don't touch panel but add only bridge device as output, and the use of this function for bridge looks more generic way. >> But drm_of_find_panel_or_bridge is only for looking for non-dsi devices, >> or more specifically for looking for devices connected in DTS via >> DT-graph. This is not case of panels and bridges used in Exynos boards. >> So this function is currently useless with our boards. Maybe some day we >> will have bridge/panel controlled via I2C and then it will become >> useful, but for now it serves for nothing. >> >>> Of course, as a separate patch, we could consider using this function for panel device later. >> As you said drm_of_find_panel_or_bridge looks for panel and/or bridge, >> and it was created to merge similar code in panel and bridge lookup, >> using it only for bridges and not for panels seems against it's purpose. >> >>>> On the other side this patch is enough to correctly handle all DSI >>>> bridges in existing Exynos boards. >>>> >>>>> And I'm not clear about what kinds of devices could be a child device of DSI, which isn't required for the graph binding. >>>>> Is there any document I can refer to? >>>> DSI devices (peripherals) should be described as child nodes of DSI host >>>> node in DT bindings, it is described in [1]. And you can find all dsi >>>> panels in exynos based boards are modeled this way. >>>> And the graph documentation [2] says that graphs should be used for >>>> connections that "can not be inferred from device tree parent-child >>>> relationships", it was emphasised multiple times by Rob in discussions >>>> regarding bindings of DSI devices. >>> Hm... I don't see why Rob introduced drm_of_find_panel_or_bridge fucntion[1] if DSI devices should be described as child nodes. >>> And some DSI drivers of other ARM DRM - vc4, mediatek, and kirin - use only drm_of_find_panel_or_bridge funtion. >> Because this function was created for i2c/spi controlled bridges/panels >> and probably these boards use only such devices. > Clear. I will pick them up. > > BTW, the bridge and panelI mentioned above are child devices of DSI driver. So I'm not sure but the child devices would be controlled by MIPI DSI protocol not by i2c/spi. Currently the only pure DSI bridge is tc358764, adv7511 registers as both i2c and dsi bridge driver, but in dts files it is always as i2c device, so it is never a child of DSI host. In case of DSI panels I have found 2 which uses obsolete bindings (orisetech,otm8009a, jdi,lt070me05000) - ie they are child nodes of dsi host and have graph links- in their case drm_of_find_panel_or_bridge will work. The rest is either not present in dts, either follows current bindings guidelines (all samsung panels) - no graph bindings if it is a child of dsi host. I gave up with checking panel-simple, as it has too many compatibles. > If you are right then the devices - bridge or panel - are controlled by i2c or spi and only image data are transferred from DSI to bridge or panel by MIPI lanes? Yes, or the control is mixed: power-up initialization/link-setup only via i2c, other commands and data via dsi. Regards Andrzej > > Thanks, > Inki Dae > >> Regards >> Andrzej >> >> >> >
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index 351403f9d245..f5f51f584fa0 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -255,6 +255,7 @@ struct exynos_dsi { struct mipi_dsi_host dsi_host; struct drm_connector connector; struct drm_panel *panel; + struct drm_bridge *out_bridge; struct device *dev; void __iomem *reg_base; @@ -1499,7 +1500,30 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host, struct mipi_dsi_device *device) { struct exynos_dsi *dsi = host_to_dsi(host); - struct drm_device *drm = dsi->connector.dev; + struct drm_encoder *encoder = &dsi->encoder; + struct drm_device *drm = encoder->dev; + struct drm_bridge *out_bridge; + + out_bridge = of_drm_find_bridge(device->dev.of_node); + if (out_bridge) { + drm_bridge_attach(encoder, out_bridge, NULL); + dsi->out_bridge = out_bridge; + encoder->bridge = NULL; + } else { + int ret = exynos_dsi_create_connector(encoder); + + if (ret) { + DRM_ERROR("failed to create connector ret = %d\n", ret); + drm_encoder_cleanup(encoder); + return ret; + } + + dsi->panel = of_drm_find_panel(device->dev.of_node); + if (dsi->panel) { + drm_panel_attach(dsi->panel, &dsi->connector); + dsi->connector.status = connector_status_connected; + } + } /* * This is a temporary solution and should be made by more generic way. @@ -1518,11 +1542,6 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host, dsi->lanes = device->lanes; dsi->format = device->format; dsi->mode_flags = device->mode_flags; - dsi->panel = of_drm_find_panel(device->dev.of_node); - if (dsi->panel) { - drm_panel_attach(dsi->panel, &dsi->connector); - dsi->connector.status = connector_status_connected; - } exynos_drm_crtc_get_by_type(drm, EXYNOS_DISPLAY_TYPE_LCD)->i80_mode = !(dsi->mode_flags & MIPI_DSI_MODE_VIDEO); @@ -1538,19 +1557,21 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host, struct mipi_dsi_device *device) { struct exynos_dsi *dsi = host_to_dsi(host); - struct drm_device *drm = dsi->connector.dev; - - mutex_lock(&drm->mode_config.mutex); + struct drm_device *drm = dsi->encoder.dev; if (dsi->panel) { + mutex_lock(&drm->mode_config.mutex); exynos_dsi_disable(&dsi->encoder); drm_panel_detach(dsi->panel); dsi->panel = NULL; dsi->connector.status = connector_status_disconnected; + mutex_unlock(&drm->mode_config.mutex); + } else { + if (dsi->out_bridge->funcs->detach) + dsi->out_bridge->funcs->detach(dsi->out_bridge); + dsi->out_bridge = NULL; } - mutex_unlock(&drm->mode_config.mutex); - if (drm->mode_config.poll_enabled) drm_kms_helper_hotplug_event(drm); @@ -1654,13 +1675,6 @@ static int exynos_dsi_bind(struct device *dev, struct device *master, if (ret < 0) return ret; - ret = exynos_dsi_create_connector(encoder); - if (ret) { - DRM_ERROR("failed to create connector ret = %d\n", ret); - drm_encoder_cleanup(encoder); - return ret; - } - if (dsi->in_bridge_node) { in_bridge = of_drm_find_bridge(dsi->in_bridge_node); if (in_bridge)