[RESEND,v11,13/18] drm: exynos: dsi: Add Exynos based host irq hooks

Message ID 20230123151212.269082-14-jagan@amarulasolutions.com
State New
Headers show
Series
  • drm: Add Samsung MIPI DSIM bridge
Related show

Commit Message

Jagan Teki Jan. 23, 2023, 3:12 p.m. UTC
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(-)

Comments

Marek Vasut Jan. 24, 2023, 8:48 p.m. UTC | #1
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.
Jagan Teki Jan. 24, 2023, 9:01 p.m. UTC | #2
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.
Marek Vasut Jan. 24, 2023, 9:12 p.m. UTC | #3
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 .
Jagan Teki Jan. 24, 2023, 9:24 p.m. UTC | #4
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 .
Jagan Teki Jan. 24, 2023, 9:24 p.m. UTC | #5
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.
Jagan Teki Jan. 25, 2023, 6:54 a.m. UTC | #6
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.
Marek Vasut Jan. 25, 2023, 1:53 p.m. UTC | #7
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.
Jagan Teki Jan. 25, 2023, 2:04 p.m. UTC | #8
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.
Jagan Teki Jan. 25, 2023, 4:02 p.m. UTC | #9
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.
Marek Vasut Jan. 25, 2023, 4:46 p.m. UTC | #10
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 .
Jagan Teki Jan. 25, 2023, 5:12 p.m. UTC | #11
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.
Marek Vasut Jan. 25, 2023, 5:27 p.m. UTC | #12
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 ?
Jagan Teki Jan. 25, 2023, 5:35 p.m. UTC | #13
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.
Marek Vasut Jan. 25, 2023, 6:03 p.m. UTC | #14
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.
Jagan Teki Jan. 25, 2023, 7:24 p.m. UTC | #15
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.
Marek Vasut Jan. 25, 2023, 9:53 p.m. UTC | #16
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 .

Patch

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[] = {