Re: [PATCH v3 5/6] drm/bridge: imx: add driver for HDMI TX Parallel Audio Interface

[Shengjiu Wang <shengjiu.wang@gmail.com>]

On Fri, Aug 8, 2025 at 3:49 PM Liu Ying <victor.liu@nxp.com> wrote:
>
> On 08/08/2025, Shengjiu Wang wrote:
> > On Fri, Aug 8, 2025 at 2:32 PM Liu Ying <victor.liu@nxp.com> wrote:
> >>
> >> On 08/07/2025, Shengjiu Wang wrote:
> >>> On Wed, Aug 6, 2025 at 2:52 PM Liu Ying <victor.liu@nxp.com> wrote:
> >>>>
> >>>> On 08/06/2025, Shengjiu Wang wrote:
> >>>>> On Tue, Aug 5, 2025 at 4:55 PM Liu Ying <victor.liu@nxp.com> wrote:
> >>>>>>
> >>>>>> On 08/04/2025, Shengjiu Wang wrote:
> >>>>
> >>>> [...]
> >>>>
> >>>>>>> +static int imx8mp_hdmi_pai_bind(struct device *dev, struct device *master, void *data)
> >>>>>>> +{
> >>>>>>> +     struct dw_hdmi_plat_data *plat_data = (struct dw_hdmi_plat_data *)data;
> >>>>>>> +     struct imx8mp_hdmi_pai *hdmi_pai;
> >>>>>>> +
> >>>>>>> +     hdmi_pai = dev_get_drvdata(dev);
> >>>>>>> +
> >>>>>>> +     plat_data->enable_audio = imx8mp_hdmi_pai_enable;
> >>>>>>> +     plat_data->disable_audio = imx8mp_hdmi_pai_disable;
> >>>>>>> +     plat_data->priv_audio = hdmi_pai;
> >>>>>>> +
> >>>>>>> +     return 0;
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static void imx8mp_hdmi_pai_unbind(struct device *dev, struct device *master, void *data)
> >>>>>>> +{
> >>>>>>> +     struct dw_hdmi_plat_data *plat_data = (struct dw_hdmi_plat_data *)data;
> >>>>>>> +
> >>>>>>> +     plat_data->enable_audio = NULL;
> >>>>>>> +     plat_data->disable_audio = NULL;
> >>>>>>> +     plat_data->priv_audio = NULL;
> >>>>>>
> >>>>>> Do you really need to set these ptrs to NULL?
> >>>>>
> >>>>> yes.  below code in dw-hdmi.c use the pdata->enable_audio as condition.
> >>>>
> >>>> Note that this is all about tearing down components.
> >>>> If this is done properly as the below snippet of pseudo-code, then
> >>>> hdmi->{enable,disable}_audio() and pdata->{enable,disable}_audio() won't be
> >>>> called after audio device is removed by dw_hdmi_remove().  So, it's unnecessary
> >>>> to set these pointers to NULL here.
> >>>>
> >>>> imx8mp_dw_hdmi_unbind()
> >>>> {
> >>>>    dw_hdmi_remove(); // platform_device_unregister(hdmi->audio);
> >>>>    component_unbind_all(); //imx8mp_hdmi_pai_unbind()
> >>>> }
> >>>>
> >>>> BTW, I suggest the below snippet[1] to bind components.
> >>>>
> >>>> imx8mp_dw_hdmi_bind()
> >>>> {
> >>>>    component_bind_all(); // imx8mp_hdmi_pai_bind()
> >>>>                          //   set pdata->{enable,disable}_audio
> >>>>    dw_hdmi_probe(); // hdmi->audio = platform_device_register_full(&pdevinfo);
> >>>> }
> >>>
> >>> Looks like we should use dw_hdmi_bind() here to make unbind -> bind work.
> >>
> >> I don't get your idea here.
> >>
> >> What are you trying to make work?
> >> Why dw_hdmi_probe() can't be used?
> >> How does dw_hdmi_bind() help here?
> >
> > bind() is ok.  but unbind(),  then bind() there is an issue.
> >
> >>
> >>> But can't get the encoder pointer.  the encoder pointer is from lcdif_drv.c,
> >>> the probe sequence of lcdif, pvi, dw_hdmi should be dw_hdmi first, then pvi,
> >>> then lcdif, because current implementation in lcdif and pvi driver.
> >>
> >> We use deferral probe to make sure the probe sequence is
> >> DW_HDMI -> PVI -> LCDIF.
> >>
> >> LCDIF driver would call devm_drm_of_get_bridge() to get the next bridge PVI
> >> and it defers probe if devm_drm_of_get_bridge() returns ERR_PTR(-EPROBE_DEFER).
> >> Same to PVI driver, it would call of_drm_find_bridge() to get the next bridge
> >> DW_HDMI and defers probe if needed.
> >
> > right, probe is no problem,  but after probe,  if unbind pai, hdmi_tx,
> >  then bind
> > them again,  there is a problem,  because no one call the
> > drm_bridge_attach() again.
>
> In my mind, this is a common issue as DRM bridges are not properly detached
> and attached again.
> For now, only drm_encoder_cleanup() calls drm_bridge_detach().
>
> Anyway, this issue is not introduced by this patch series, i.e. it's already
> there.

Ok,  thanks for clarification.

best regards
shengjiu Wang
>
> >
> >>
> >>>
> >>> Should the lcdif and pvi driver be modified to use component helper?
> >>
> >> Why should they use component helper?
> >>
> >> BTW, I've tried testing the snippets suggested by me on i.MX8MP EVK and
> >> the components bind successfully:
> >
> > right, probe is no problem. but if try to unbind() then bind, there is issue.
>
> I don't think the DRM bridge detach/attach issue would be addressed by
> using component helper.
>
> >
> > best regards
> > shengjiu Wang
> >>
> >> cat /sys/kernel/debug/device_component/32fd8000.hdmi
> >> aggregate_device name                                            status
> >> -----------------------------------------------------------------------
> >> 32fd8000.hdmi                                                     bound
> >>
> >> device name                                                      status
> >> -----------------------------------------------------------------------
> >> 32fc4800.audio-bridge                                             bound
> >>
> >>> This seems out of the scope of this patch set.
> >>>
> >>> Best regards
> >>> Shengjiu Wang
> >>
> >> [...]
> >>
> >> --
> >> Regards,
> >> Liu Ying
>
>
> --
> Regards,
> Liu Ying