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 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.

>
> >
> > 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.

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