| Message ID | 20230123151212.269082-14-jagan@amarulasolutions.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm: Add Samsung MIPI DSIM bridge | expand |
On 1/23/23 16:12, Jagan Teki wrote: > Enable and disable of te_gpio's are Exynos platform specific > irq handling, so add the exynos based irq operations and hook > them for exynos plat_data. If this is just an optional generic GPIO IRQ, why not keep it in the core code ? TE (tearing enable?) should be available on MX8M too.
On Wed, Jan 25, 2023 at 2:18 AM Marek Vasut <marex@denx.de> wrote: > > On 1/23/23 16:12, Jagan Teki wrote: > > Enable and disable of te_gpio's are Exynos platform specific > > irq handling, so add the exynos based irq operations and hook > > them for exynos plat_data. > > If this is just an optional generic GPIO IRQ, why not keep it in the > core code ? TE (tearing enable?) should be available on MX8M too. So far the discussion (since from initial versions) with Marek Szyprowski, seems to be available in Exynos. So, I keep it separate from the DSIM core. Jagan.
On 1/24/23 22:01, Jagan Teki wrote: > On Wed, Jan 25, 2023 at 2:18 AM Marek Vasut <marex@denx.de> wrote: >> >> On 1/23/23 16:12, Jagan Teki wrote: >>> Enable and disable of te_gpio's are Exynos platform specific >>> irq handling, so add the exynos based irq operations and hook >>> them for exynos plat_data. >> >> If this is just an optional generic GPIO IRQ, why not keep it in the >> core code ? TE (tearing enable?) should be available on MX8M too. > > So far the discussion (since from initial versions) with Marek > Szyprowski, seems to be available in Exynos. So, I keep it separate > from the DSIM core. Isn't TE a generic GPIO IRQ ? If so, it is available also on i.MX8M .
On Wed, Jan 25, 2023 at 2:42 AM Marek Vasut <marex@denx.de> wrote: > > On 1/24/23 22:01, Jagan Teki wrote: > > On Wed, Jan 25, 2023 at 2:18 AM Marek Vasut <marex@denx.de> wrote: > >> > >> On 1/23/23 16:12, Jagan Teki wrote: > >>> Enable and disable of te_gpio's are Exynos platform specific > >>> irq handling, so add the exynos based irq operations and hook > >>> them for exynos plat_data. > >> > >> If this is just an optional generic GPIO IRQ, why not keep it in the > >> core code ? TE (tearing enable?) should be available on MX8M too. > > > > So far the discussion (since from initial versions) with Marek > > Szyprowski, seems to be available in Exynos. So, I keep it separate > > from the DSIM core. > > Isn't TE a generic GPIO IRQ ? If so, it is available also on i.MX8M .
On Wed, Jan 25, 2023 at 2:54 AM Jagan Teki <jagan@amarulasolutions.com> wrote: > > On Wed, Jan 25, 2023 at 2:42 AM Marek Vasut <marex@denx.de> wrote: > > > > On 1/24/23 22:01, Jagan Teki wrote: > > > On Wed, Jan 25, 2023 at 2:18 AM Marek Vasut <marex@denx.de> wrote: > > >> > > >> On 1/23/23 16:12, Jagan Teki wrote: > > >>> Enable and disable of te_gpio's are Exynos platform specific > > >>> irq handling, so add the exynos based irq operations and hook > > >>> them for exynos plat_data. > > >> > > >> If this is just an optional generic GPIO IRQ, why not keep it in the > > >> core code ? TE (tearing enable?) should be available on MX8M too. > > > > > > So far the discussion (since from initial versions) with Marek > > > Szyprowski, seems to be available in Exynos. So, I keep it separate > > > from the DSIM core. > > > > Isn't TE a generic GPIO IRQ ? If so, it is available also on i.MX8M . I will check this. Jagan.
On Wed, Jan 25, 2023 at 2:54 AM Jagan Teki <jagan@amarulasolutions.com> wrote: > > On Wed, Jan 25, 2023 at 2:54 AM Jagan Teki <jagan@amarulasolutions.com> wrote: > > > > On Wed, Jan 25, 2023 at 2:42 AM Marek Vasut <marex@denx.de> wrote: > > > > > > On 1/24/23 22:01, Jagan Teki wrote: > > > > On Wed, Jan 25, 2023 at 2:18 AM Marek Vasut <marex@denx.de> wrote: > > > >> > > > >> On 1/23/23 16:12, Jagan Teki wrote: > > > >>> Enable and disable of te_gpio's are Exynos platform specific > > > >>> irq handling, so add the exynos based irq operations and hook > > > >>> them for exynos plat_data. > > > >> > > > >> If this is just an optional generic GPIO IRQ, why not keep it in the > > > >> core code ? TE (tearing enable?) should be available on MX8M too. > > > > > > > > So far the discussion (since from initial versions) with Marek > > > > Szyprowski, seems to be available in Exynos. So, I keep it separate > > > > from the DSIM core. > > > > > > Isn't TE a generic GPIO IRQ ? If so, it is available also on i.MX8M . > > I will check this. In order to use TE_GPIO we need te handler implementation, right now Exynos CRTC DRM drivers have implementation for this. That is the main reason to keep the TE_GPIO handling in exynos, maybe if we handle that generically then it is a viable option to move TE_GPIO to the DSIM core. Jagan.
On 1/25/23 07:54, Jagan Teki wrote: > On Wed, Jan 25, 2023 at 2:54 AM Jagan Teki <jagan@amarulasolutions.com> wrote: >> >> On Wed, Jan 25, 2023 at 2:54 AM Jagan Teki <jagan@amarulasolutions.com> wrote: >>> >>> On Wed, Jan 25, 2023 at 2:42 AM Marek Vasut <marex@denx.de> wrote: >>>> >>>> On 1/24/23 22:01, Jagan Teki wrote: >>>>> On Wed, Jan 25, 2023 at 2:18 AM Marek Vasut <marex@denx.de> wrote: >>>>>> >>>>>> On 1/23/23 16:12, Jagan Teki wrote: >>>>>>> Enable and disable of te_gpio's are Exynos platform specific >>>>>>> irq handling, so add the exynos based irq operations and hook >>>>>>> them for exynos plat_data. >>>>>> >>>>>> If this is just an optional generic GPIO IRQ, why not keep it in the >>>>>> core code ? TE (tearing enable?) should be available on MX8M too. >>>>> >>>>> So far the discussion (since from initial versions) with Marek >>>>> Szyprowski, seems to be available in Exynos. So, I keep it separate >>>>> from the DSIM core. >>>> >>>> Isn't TE a generic GPIO IRQ ? If so, it is available also on i.MX8M . >> >> I will check this. > > In order to use TE_GPIO we need te handler implementation, right now > Exynos CRTC DRM drivers have implementation for this. That is the main > reason to keep the TE_GPIO handling in exynos, maybe if we handle that > generically then it is a viable option to move TE_GPIO to the DSIM > core. I think you can do this exactly the same way exynos does it -- check whether te_handler() callback is implemented by the glue code (the one you already have for various exynos and imx8mm/imx8mm SoCs) and if so, call it. If it is not implemented, do not call anything in the TE IRQ handler.
On Wed, Jan 25, 2023 at 7:23 PM Marek Vasut <marex@denx.de> wrote: > > On 1/25/23 07:54, Jagan Teki wrote: > > On Wed, Jan 25, 2023 at 2:54 AM Jagan Teki <jagan@amarulasolutions.com> wrote: > >> > >> On Wed, Jan 25, 2023 at 2:54 AM Jagan Teki <jagan@amarulasolutions.com> wrote: > >>> > >>> On Wed, Jan 25, 2023 at 2:42 AM Marek Vasut <marex@denx.de> wrote: > >>>> > >>>> On 1/24/23 22:01, Jagan Teki wrote: > >>>>> On Wed, Jan 25, 2023 at 2:18 AM Marek Vasut <marex@denx.de> wrote: > >>>>>> > >>>>>> On 1/23/23 16:12, Jagan Teki wrote: > >>>>>>> Enable and disable of te_gpio's are Exynos platform specific > >>>>>>> irq handling, so add the exynos based irq operations and hook > >>>>>>> them for exynos plat_data. > >>>>>> > >>>>>> If this is just an optional generic GPIO IRQ, why not keep it in the > >>>>>> core code ? TE (tearing enable?) should be available on MX8M too. > >>>>> > >>>>> So far the discussion (since from initial versions) with Marek > >>>>> Szyprowski, seems to be available in Exynos. So, I keep it separate > >>>>> from the DSIM core. > >>>> > >>>> Isn't TE a generic GPIO IRQ ? If so, it is available also on i.MX8M . > >> > >> I will check this. > > > > In order to use TE_GPIO we need te handler implementation, right now > > Exynos CRTC DRM drivers have implementation for this. That is the main > > reason to keep the TE_GPIO handling in exynos, maybe if we handle that > > generically then it is a viable option to move TE_GPIO to the DSIM > > core. > > I think you can do this exactly the same way exynos does it -- check > whether te_handler() callback is implemented by the glue code (the one > you already have for various exynos and imx8mm/imx8mm SoCs) and if so, > call it. If it is not implemented, do not call anything in the TE IRQ > handler. I need to understand how i.MX8MM handles this on TE IRQ in the DSIM host side, Can I do this in future patch set as it might involve bindings changes as well if it's part of DSIM? Jagan.
On Wed, Jan 25, 2023 at 2:42 AM Marek Vasut <marex@denx.de> wrote: > > On 1/24/23 22:01, Jagan Teki wrote: > > On Wed, Jan 25, 2023 at 2:18 AM Marek Vasut <marex@denx.de> wrote: > >> > >> On 1/23/23 16:12, Jagan Teki wrote: > >>> Enable and disable of te_gpio's are Exynos platform specific > >>> irq handling, so add the exynos based irq operations and hook > >>> them for exynos plat_data. > >> > >> If this is just an optional generic GPIO IRQ, why not keep it in the > >> core code ? TE (tearing enable?) should be available on MX8M too. > > > > So far the discussion (since from initial versions) with Marek > > Szyprowski, seems to be available in Exynos. So, I keep it separate > > from the DSIM core. > > Isn't TE a generic GPIO IRQ ? If so, it is available also on i.MX8M . I didn't find this in the DSIM part in i.MX8M Manual nor in the i.MX 8/RT MIPI DSI/CSI-2 or bsp kernel [1], did you find anywhere in i.MX8M part? Look like TE GPIO means tearing effect signal handle on the panel side. from, Documentation/devicetree/bindings/display/panel/panel-common.yaml te-gpios: maxItems: 1 description: GPIO spec for the tearing effect synchronization signal. The tearing effect signal is active high. Active low signals can be supported by inverting the GPIO specifier polarity flag. Maybe Exynos hack this gpio on the host side instead of on the panel side for some reason, not sure about it - Marek Szypeowski any comments please? [1] https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/gpu/drm/bridge/sec-dsim.c?h=imx_5.4.47_2.2.0 Thanks, Jagan.
On 1/25/23 15:04, Jagan Teki wrote: > On Wed, Jan 25, 2023 at 7:23 PM Marek Vasut <marex@denx.de> wrote: >> >> On 1/25/23 07:54, Jagan Teki wrote: >>> On Wed, Jan 25, 2023 at 2:54 AM Jagan Teki <jagan@amarulasolutions.com> wrote: >>>> >>>> On Wed, Jan 25, 2023 at 2:54 AM Jagan Teki <jagan@amarulasolutions.com> wrote: >>>>> >>>>> On Wed, Jan 25, 2023 at 2:42 AM Marek Vasut <marex@denx.de> wrote: >>>>>> >>>>>> On 1/24/23 22:01, Jagan Teki wrote: >>>>>>> On Wed, Jan 25, 2023 at 2:18 AM Marek Vasut <marex@denx.de> wrote: >>>>>>>> >>>>>>>> On 1/23/23 16:12, Jagan Teki wrote: >>>>>>>>> Enable and disable of te_gpio's are Exynos platform specific >>>>>>>>> irq handling, so add the exynos based irq operations and hook >>>>>>>>> them for exynos plat_data. >>>>>>>> >>>>>>>> If this is just an optional generic GPIO IRQ, why not keep it in the >>>>>>>> core code ? TE (tearing enable?) should be available on MX8M too. >>>>>>> >>>>>>> So far the discussion (since from initial versions) with Marek >>>>>>> Szyprowski, seems to be available in Exynos. So, I keep it separate >>>>>>> from the DSIM core. >>>>>> >>>>>> Isn't TE a generic GPIO IRQ ? If so, it is available also on i.MX8M . >>>> >>>> I will check this. >>> >>> In order to use TE_GPIO we need te handler implementation, right now >>> Exynos CRTC DRM drivers have implementation for this. That is the main >>> reason to keep the TE_GPIO handling in exynos, maybe if we handle that >>> generically then it is a viable option to move TE_GPIO to the DSIM >>> core. >> >> I think you can do this exactly the same way exynos does it -- check >> whether te_handler() callback is implemented by the glue code (the one >> you already have for various exynos and imx8mm/imx8mm SoCs) and if so, >> call it. If it is not implemented, do not call anything in the TE IRQ >> handler. > > I need to understand how i.MX8MM handles this on TE IRQ in the DSIM > host side, Can I do this in future patch set as it might involve > bindings changes as well if it's part of DSIM? Why not leave an empty te_handler implementation on MX8M for now ? You can fill that implementation in future patchset, but the generic part of the code would be in place .
On Wed, Jan 25, 2023 at 10:16 PM Marek Vasut <marex@denx.de> wrote: > > On 1/25/23 15:04, Jagan Teki wrote: > > On Wed, Jan 25, 2023 at 7:23 PM Marek Vasut <marex@denx.de> wrote: > >> > >> On 1/25/23 07:54, Jagan Teki wrote: > >>> On Wed, Jan 25, 2023 at 2:54 AM Jagan Teki <jagan@amarulasolutions.com> wrote: > >>>> > >>>> On Wed, Jan 25, 2023 at 2:54 AM Jagan Teki <jagan@amarulasolutions.com> wrote: > >>>>> > >>>>> On Wed, Jan 25, 2023 at 2:42 AM Marek Vasut <marex@denx.de> wrote: > >>>>>> > >>>>>> On 1/24/23 22:01, Jagan Teki wrote: > >>>>>>> On Wed, Jan 25, 2023 at 2:18 AM Marek Vasut <marex@denx.de> wrote: > >>>>>>>> > >>>>>>>> On 1/23/23 16:12, Jagan Teki wrote: > >>>>>>>>> Enable and disable of te_gpio's are Exynos platform specific > >>>>>>>>> irq handling, so add the exynos based irq operations and hook > >>>>>>>>> them for exynos plat_data. > >>>>>>>> > >>>>>>>> If this is just an optional generic GPIO IRQ, why not keep it in the > >>>>>>>> core code ? TE (tearing enable?) should be available on MX8M too. > >>>>>>> > >>>>>>> So far the discussion (since from initial versions) with Marek > >>>>>>> Szyprowski, seems to be available in Exynos. So, I keep it separate > >>>>>>> from the DSIM core. > >>>>>> > >>>>>> Isn't TE a generic GPIO IRQ ? If so, it is available also on i.MX8M . > >>>> > >>>> I will check this. > >>> > >>> In order to use TE_GPIO we need te handler implementation, right now > >>> Exynos CRTC DRM drivers have implementation for this. That is the main > >>> reason to keep the TE_GPIO handling in exynos, maybe if we handle that > >>> generically then it is a viable option to move TE_GPIO to the DSIM > >>> core. > >> > >> I think you can do this exactly the same way exynos does it -- check > >> whether te_handler() callback is implemented by the glue code (the one > >> you already have for various exynos and imx8mm/imx8mm SoCs) and if so, > >> call it. If it is not implemented, do not call anything in the TE IRQ > >> handler. > > > > I need to understand how i.MX8MM handles this on TE IRQ in the DSIM > > host side, Can I do this in future patch set as it might involve > > bindings changes as well if it's part of DSIM? > > Why not leave an empty te_handler implementation on MX8M for now ? > You can fill that implementation in future patchset, but the generic > part of the code would be in place . Look like we have one issue to move regster te_irq into samsung-dsim. exynos_dsi_register_te_irq is done after the bridge attach is done in Exynos, here bridge attach is triggered in the component ops bind call, since samsung-dsim is a pure bridge w/o any component ops. https://github.com/openedev/kernel/blob/imx8mm-dsi-v12/drivers/gpu/drm/bridge/samsung-dsim.c#L1527 https://github.com/openedev/kernel/blob/imx8mm-dsi-v12/drivers/gpu/drm/exynos/exynos_drm_dsi.c#L112 Any suggestion on how to handle this? Thanks, Jagan.
On 1/25/23 18:12, Jagan Teki wrote: > On Wed, Jan 25, 2023 at 10:16 PM Marek Vasut <marex@denx.de> wrote: >> >> On 1/25/23 15:04, Jagan Teki wrote: >>> On Wed, Jan 25, 2023 at 7:23 PM Marek Vasut <marex@denx.de> wrote: >>>> >>>> On 1/25/23 07:54, Jagan Teki wrote: >>>>> On Wed, Jan 25, 2023 at 2:54 AM Jagan Teki <jagan@amarulasolutions.com> wrote: >>>>>> >>>>>> On Wed, Jan 25, 2023 at 2:54 AM Jagan Teki <jagan@amarulasolutions.com> wrote: >>>>>>> >>>>>>> On Wed, Jan 25, 2023 at 2:42 AM Marek Vasut <marex@denx.de> wrote: >>>>>>>> >>>>>>>> On 1/24/23 22:01, Jagan Teki wrote: >>>>>>>>> On Wed, Jan 25, 2023 at 2:18 AM Marek Vasut <marex@denx.de> wrote: >>>>>>>>>> >>>>>>>>>> On 1/23/23 16:12, Jagan Teki wrote: >>>>>>>>>>> Enable and disable of te_gpio's are Exynos platform specific >>>>>>>>>>> irq handling, so add the exynos based irq operations and hook >>>>>>>>>>> them for exynos plat_data. >>>>>>>>>> >>>>>>>>>> If this is just an optional generic GPIO IRQ, why not keep it in the >>>>>>>>>> core code ? TE (tearing enable?) should be available on MX8M too. >>>>>>>>> >>>>>>>>> So far the discussion (since from initial versions) with Marek >>>>>>>>> Szyprowski, seems to be available in Exynos. So, I keep it separate >>>>>>>>> from the DSIM core. >>>>>>>> >>>>>>>> Isn't TE a generic GPIO IRQ ? If so, it is available also on i.MX8M . >>>>>> >>>>>> I will check this. >>>>> >>>>> In order to use TE_GPIO we need te handler implementation, right now >>>>> Exynos CRTC DRM drivers have implementation for this. That is the main >>>>> reason to keep the TE_GPIO handling in exynos, maybe if we handle that >>>>> generically then it is a viable option to move TE_GPIO to the DSIM >>>>> core. >>>> >>>> I think you can do this exactly the same way exynos does it -- check >>>> whether te_handler() callback is implemented by the glue code (the one >>>> you already have for various exynos and imx8mm/imx8mm SoCs) and if so, >>>> call it. If it is not implemented, do not call anything in the TE IRQ >>>> handler. >>> >>> I need to understand how i.MX8MM handles this on TE IRQ in the DSIM >>> host side, Can I do this in future patch set as it might involve >>> bindings changes as well if it's part of DSIM? >> >> Why not leave an empty te_handler implementation on MX8M for now ? >> You can fill that implementation in future patchset, but the generic >> part of the code would be in place . > > Look like we have one issue to move regster te_irq into samsung-dsim. > > exynos_dsi_register_te_irq is done after the bridge attach is done in > Exynos, here bridge attach is triggered in the component ops bind > call, since samsung-dsim is a pure bridge w/o any component ops. > https://github.com/openedev/kernel/blob/imx8mm-dsi-v12/drivers/gpu/drm/bridge/samsung-dsim.c#L1527 > https://github.com/openedev/kernel/blob/imx8mm-dsi-v12/drivers/gpu/drm/exynos/exynos_drm_dsi.c#L112 > > Any suggestion on how to handle this? Why isn't the generic code calling drm_bridge_attach() in samsung_dsim_host_attach(), like the exynos one ?
On Wed, Jan 25, 2023 at 10:57 PM Marek Vasut <marex@denx.de> wrote: > > On 1/25/23 18:12, Jagan Teki wrote: > > On Wed, Jan 25, 2023 at 10:16 PM Marek Vasut <marex@denx.de> wrote: > >> > >> On 1/25/23 15:04, Jagan Teki wrote: > >>> On Wed, Jan 25, 2023 at 7:23 PM Marek Vasut <marex@denx.de> wrote: > >>>> > >>>> On 1/25/23 07:54, Jagan Teki wrote: > >>>>> On Wed, Jan 25, 2023 at 2:54 AM Jagan Teki <jagan@amarulasolutions.com> wrote: > >>>>>> > >>>>>> On Wed, Jan 25, 2023 at 2:54 AM Jagan Teki <jagan@amarulasolutions.com> wrote: > >>>>>>> > >>>>>>> On Wed, Jan 25, 2023 at 2:42 AM Marek Vasut <marex@denx.de> wrote: > >>>>>>>> > >>>>>>>> On 1/24/23 22:01, Jagan Teki wrote: > >>>>>>>>> On Wed, Jan 25, 2023 at 2:18 AM Marek Vasut <marex@denx.de> wrote: > >>>>>>>>>> > >>>>>>>>>> On 1/23/23 16:12, Jagan Teki wrote: > >>>>>>>>>>> Enable and disable of te_gpio's are Exynos platform specific > >>>>>>>>>>> irq handling, so add the exynos based irq operations and hook > >>>>>>>>>>> them for exynos plat_data. > >>>>>>>>>> > >>>>>>>>>> If this is just an optional generic GPIO IRQ, why not keep it in the > >>>>>>>>>> core code ? TE (tearing enable?) should be available on MX8M too. > >>>>>>>>> > >>>>>>>>> So far the discussion (since from initial versions) with Marek > >>>>>>>>> Szyprowski, seems to be available in Exynos. So, I keep it separate > >>>>>>>>> from the DSIM core. > >>>>>>>> > >>>>>>>> Isn't TE a generic GPIO IRQ ? If so, it is available also on i.MX8M . > >>>>>> > >>>>>> I will check this. > >>>>> > >>>>> In order to use TE_GPIO we need te handler implementation, right now > >>>>> Exynos CRTC DRM drivers have implementation for this. That is the main > >>>>> reason to keep the TE_GPIO handling in exynos, maybe if we handle that > >>>>> generically then it is a viable option to move TE_GPIO to the DSIM > >>>>> core. > >>>> > >>>> I think you can do this exactly the same way exynos does it -- check > >>>> whether te_handler() callback is implemented by the glue code (the one > >>>> you already have for various exynos and imx8mm/imx8mm SoCs) and if so, > >>>> call it. If it is not implemented, do not call anything in the TE IRQ > >>>> handler. > >>> > >>> I need to understand how i.MX8MM handles this on TE IRQ in the DSIM > >>> host side, Can I do this in future patch set as it might involve > >>> bindings changes as well if it's part of DSIM? > >> > >> Why not leave an empty te_handler implementation on MX8M for now ? > >> You can fill that implementation in future patchset, but the generic > >> part of the code would be in place . > > > > Look like we have one issue to move regster te_irq into samsung-dsim. > > > > exynos_dsi_register_te_irq is done after the bridge attach is done in > > Exynos, here bridge attach is triggered in the component ops bind > > call, since samsung-dsim is a pure bridge w/o any component ops. > > https://github.com/openedev/kernel/blob/imx8mm-dsi-v12/drivers/gpu/drm/bridge/samsung-dsim.c#L1527 > > https://github.com/openedev/kernel/blob/imx8mm-dsi-v12/drivers/gpu/drm/exynos/exynos_drm_dsi.c#L112 > > > > Any suggestion on how to handle this? > > Why isn't the generic code calling drm_bridge_attach() in > samsung_dsim_host_attach(), like the exynos one ? Exynos drm drivers follow component ops and generic dsim is a pure drm bridge whose downstream bridge will attach in bridge ops attach and the component-based drivers require an initial bridge attach (whose previous is NULL) call in the component bind hook for establishing the bridge chain. Jagan.
On 1/25/23 18:35, Jagan Teki wrote: [...] >>> exynos_dsi_register_te_irq is done after the bridge attach is done in >>> Exynos, here bridge attach is triggered in the component ops bind >>> call, since samsung-dsim is a pure bridge w/o any component ops. >>> https://github.com/openedev/kernel/blob/imx8mm-dsi-v12/drivers/gpu/drm/bridge/samsung-dsim.c#L1527 >>> https://github.com/openedev/kernel/blob/imx8mm-dsi-v12/drivers/gpu/drm/exynos/exynos_drm_dsi.c#L112 >>> >>> Any suggestion on how to handle this? >> >> Why isn't the generic code calling drm_bridge_attach() in >> samsung_dsim_host_attach(), like the exynos one ? > > Exynos drm drivers follow component ops and generic dsim is a pure drm > bridge whose downstream bridge will attach in bridge ops attach and > the component-based drivers require an initial bridge attach (whose > previous is NULL) call in the component bind hook for establishing the > bridge chain. Well in that case, call the exynos optional host_attach and register the TE IRQ handler at the end, that should work just fine too, right ? If so, then you can also move the IRQ handler registration into the generic part of the driver.
On Wed, Jan 25, 2023 at 11:33 PM Marek Vasut <marex@denx.de> wrote: > > On 1/25/23 18:35, Jagan Teki wrote: > > [...] > > >>> exynos_dsi_register_te_irq is done after the bridge attach is done in > >>> Exynos, here bridge attach is triggered in the component ops bind > >>> call, since samsung-dsim is a pure bridge w/o any component ops. > >>> https://github.com/openedev/kernel/blob/imx8mm-dsi-v12/drivers/gpu/drm/bridge/samsung-dsim.c#L1527 > >>> https://github.com/openedev/kernel/blob/imx8mm-dsi-v12/drivers/gpu/drm/exynos/exynos_drm_dsi.c#L112 > >>> > >>> Any suggestion on how to handle this? > >> > >> Why isn't the generic code calling drm_bridge_attach() in > >> samsung_dsim_host_attach(), like the exynos one ? > > > > Exynos drm drivers follow component ops and generic dsim is a pure drm > > bridge whose downstream bridge will attach in bridge ops attach and > > the component-based drivers require an initial bridge attach (whose > > previous is NULL) call in the component bind hook for establishing the > > bridge chain. > > Well in that case, call the exynos optional host_attach and register the > TE IRQ handler at the end, that should work just fine too, right ? If > so, then you can also move the IRQ handler registration into the generic > part of the driver. Something like this? samsung_dsim_host_attach() { drm_bridge_add(&dsi->bridge); if (pdata->host_ops && pdata->host_ops->attach) pdata->host_ops->attach(dsi, device); exynos_dsi_register_te_irq dsi->lanes = device->lanes; dsi->format = device->format; dsi->mode_flags = device->mode_flags; } Jagan.
On 1/25/23 20:24, Jagan Teki wrote: > On Wed, Jan 25, 2023 at 11:33 PM Marek Vasut <marex@denx.de> wrote: >> >> On 1/25/23 18:35, Jagan Teki wrote: >> >> [...] >> >>>>> exynos_dsi_register_te_irq is done after the bridge attach is done in >>>>> Exynos, here bridge attach is triggered in the component ops bind >>>>> call, since samsung-dsim is a pure bridge w/o any component ops. >>>>> https://github.com/openedev/kernel/blob/imx8mm-dsi-v12/drivers/gpu/drm/bridge/samsung-dsim.c#L1527 >>>>> https://github.com/openedev/kernel/blob/imx8mm-dsi-v12/drivers/gpu/drm/exynos/exynos_drm_dsi.c#L112 >>>>> >>>>> Any suggestion on how to handle this? >>>> >>>> Why isn't the generic code calling drm_bridge_attach() in >>>> samsung_dsim_host_attach(), like the exynos one ? >>> >>> Exynos drm drivers follow component ops and generic dsim is a pure drm >>> bridge whose downstream bridge will attach in bridge ops attach and >>> the component-based drivers require an initial bridge attach (whose >>> previous is NULL) call in the component bind hook for establishing the >>> bridge chain. >> >> Well in that case, call the exynos optional host_attach and register the >> TE IRQ handler at the end, that should work just fine too, right ? If >> so, then you can also move the IRQ handler registration into the generic >> part of the driver. > > Something like this? > > samsung_dsim_host_attach() > { > drm_bridge_add(&dsi->bridge); > > if (pdata->host_ops && pdata->host_ops->attach) > pdata->host_ops->attach(dsi, device); > > exynos_dsi_register_te_irq > > dsi->lanes = device->lanes; > dsi->format = device->format; > dsi->mode_flags = device->mode_flags; > } Yes, I think that should work .
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index fc7f00ab01b4..5e672209ed5f 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -290,9 +290,15 @@ struct exynos_dsim_host_ops { int (*detach)(struct exynos_dsi *dsim, struct mipi_dsi_device *device); }; +struct exynos_dsim_irq_ops { + void (*enable)(struct exynos_dsi *dsim); + void (*disable)(struct exynos_dsi *dsim); +}; + struct exynos_dsi_plat_data { enum exynos_dsi_type hw_type; const struct exynos_dsim_host_ops *host_ops; + const struct exynos_dsim_irq_ops *irq_ops; }; struct exynos_dsi { @@ -307,7 +313,6 @@ struct exynos_dsi { struct clk **clks; struct regulator_bulk_data supplies[2]; int irq; - struct gpio_desc *te_gpio; u32 pll_clk_rate; u32 burst_clk_rate; @@ -331,6 +336,7 @@ struct exynos_dsi { struct exynos_dsi_enc { struct drm_encoder encoder; + struct gpio_desc *te_gpio; }; #define host_to_dsi(host) container_of(host, struct exynos_dsi, dsi_host) @@ -1344,18 +1350,38 @@ static irqreturn_t exynos_dsi_te_irq_handler(int irq, void *dev_id) return IRQ_HANDLED; } -static void exynos_dsi_enable_irq(struct exynos_dsi *dsi) +static void _exynos_dsi_enable_irq(struct exynos_dsi *dsim) { - enable_irq(dsi->irq); + struct _exynos_dsi *dsi = dsim->priv; if (dsi->te_gpio) enable_irq(gpiod_to_irq(dsi->te_gpio)); } -static void exynos_dsi_disable_irq(struct exynos_dsi *dsi) +static void _exynos_dsi_disable_irq(struct exynos_dsi *dsim) { + struct _exynos_dsi *dsi = dsim->priv; + if (dsi->te_gpio) disable_irq(gpiod_to_irq(dsi->te_gpio)); +} + +static void exynos_dsi_enable_irq(struct exynos_dsi *dsi) +{ + const struct exynos_dsi_plat_data *pdata = dsi->plat_data; + + enable_irq(dsi->irq); + + if (pdata->irq_ops && pdata->irq_ops->enable) + pdata->irq_ops->enable(dsi); +} + +static void exynos_dsi_disable_irq(struct exynos_dsi *dsi) +{ + const struct exynos_dsi_plat_data *pdata = dsi->plat_data; + + if (pdata->irq_ops && pdata->irq_ops->disable) + pdata->irq_ops->disable(dsi); disable_irq(dsi->irq); } @@ -1384,9 +1410,10 @@ static int exynos_dsi_init(struct exynos_dsi *dsi) return 0; } -static int exynos_dsi_register_te_irq(struct exynos_dsi *dsi, +static int exynos_dsi_register_te_irq(struct exynos_dsi *dsim, struct device *panel) { + struct _exynos_dsi *dsi = dsim->priv; int ret; int te_gpio_irq; @@ -1394,7 +1421,7 @@ static int exynos_dsi_register_te_irq(struct exynos_dsi *dsi, if (!dsi->te_gpio) { return 0; } else if (IS_ERR(dsi->te_gpio)) { - dev_err(dsi->dev, "gpio request failed with %ld\n", + dev_err(dsim->dev, "gpio request failed with %ld\n", PTR_ERR(dsi->te_gpio)); return PTR_ERR(dsi->te_gpio); } @@ -1404,7 +1431,7 @@ static int exynos_dsi_register_te_irq(struct exynos_dsi *dsi, ret = request_threaded_irq(te_gpio_irq, exynos_dsi_te_irq_handler, NULL, IRQF_TRIGGER_RISING | IRQF_NO_AUTOEN, "TE", dsi); if (ret) { - dev_err(dsi->dev, "request interrupt failed with %d\n", ret); + dev_err(dsim->dev, "request interrupt failed with %d\n", ret); gpiod_put(dsi->te_gpio); return ret; } @@ -1412,8 +1439,10 @@ static int exynos_dsi_register_te_irq(struct exynos_dsi *dsi, return 0; } -static void exynos_dsi_unregister_te_irq(struct exynos_dsi *dsi) +static void exynos_dsi_unregister_te_irq(struct exynos_dsi *dsim) { + struct _exynos_dsi *dsi = dsim->priv; + if (dsi->te_gpio) { free_irq(gpiod_to_irq(dsi->te_gpio), dsi); gpiod_put(dsi->te_gpio); @@ -2033,6 +2062,11 @@ static const struct dev_pm_ops exynos_dsi_pm_ops = { pm_runtime_force_resume) }; +static const struct exynos_dsim_irq_ops exynos_dsi_irq_ops = { + .enable = _exynos_dsi_enable_irq, + .disable = _exynos_dsi_disable_irq, +}; + static const struct exynos_dsim_host_ops exynos_dsi_host_ops = { .register_host = exynos_dsi_register_host, .unregister_host = exynos_dsi_unregister_host, @@ -2043,26 +2077,31 @@ static const struct exynos_dsim_host_ops exynos_dsi_host_ops = { static const struct exynos_dsi_plat_data exynos3250_dsi_pdata = { .hw_type = DSIM_TYPE_EXYNOS3250, .host_ops = &exynos_dsi_host_ops, + .irq_ops = &exynos_dsi_irq_ops, }; static const struct exynos_dsi_plat_data exynos4210_dsi_pdata = { .hw_type = DSIM_TYPE_EXYNOS4210, .host_ops = &exynos_dsi_host_ops, + .irq_ops = &exynos_dsi_irq_ops, }; static const struct exynos_dsi_plat_data exynos5410_dsi_pdata = { .hw_type = DSIM_TYPE_EXYNOS5410, .host_ops = &exynos_dsi_host_ops, + .irq_ops = &exynos_dsi_irq_ops, }; static const struct exynos_dsi_plat_data exynos5422_dsi_pdata = { .hw_type = DSIM_TYPE_EXYNOS5422, .host_ops = &exynos_dsi_host_ops, + .irq_ops = &exynos_dsi_irq_ops, }; static const struct exynos_dsi_plat_data exynos5433_dsi_pdata = { .hw_type = DSIM_TYPE_EXYNOS5433, .host_ops = &exynos_dsi_host_ops, + .irq_ops = &exynos_dsi_irq_ops, }; static const struct of_device_id exynos_dsi_of_match[] = {
Enable and disable of te_gpio's are Exynos platform specific irq handling, so add the exynos based irq operations and hook them for exynos plat_data. Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> --- Changes for v11: - none Changes for v10: - split from previous series patch "drm: bridge: Generalize Exynos-DSI driver into a Samsung DSIM bridge" drivers/gpu/drm/exynos/exynos_drm_dsi.c | 55 +++++++++++++++++++++---- 1 file changed, 47 insertions(+), 8 deletions(-)