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

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

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

Should the lcdif and pvi driver be modified to use component helper?
This seems out of the scope of this patch set.

Best regards
Shengjiu Wang
>
> >
> >         if (pdata->enable_audio)
> >                 pdata->enable_audio(hdmi,
> >                                     hdmi->channels,
> >                                     hdmi->sample_width,
> >                                     hdmi->sample_rate,
> >                                     hdmi->sample_non_pcm,
> >                                     hdmi->sample_iec958);
> >
> >
> >>
>
> [...]
>
> >>> +     return component_add(dev, &imx8mp_hdmi_pai_ops);
> >>
> >> Imagine that users could enable this driver without enabling imx8mp-hdmi-tx
> >> driver, you may add the component in this probe() callback only and move all
> >> the other stuff to bind() callback to avoid unnecessary things being done here.
> >
> > component helper functions don't have such dependency that the aggregate
> > driver or component driver must be probed or not.  if imx8mp-hdmi-tx is not
> > enabled, there is no problem, just the bind() callback is not called.
>
> I meant I'd write imx8mp_hdmi_pai_probe() as below snippet and do all the
> other stuff in imx8mp_hdmi_pai_bind().  This ensures minimum things are done
> in imx8mp_hdmi_pai_probe() if imx8mp-hdmi-tx doesn't probe.
>
> static int imx8mp_hdmi_pai_probe(struct platform_device *pdev)
> {
>         return component_add(&pdev->dev, &imx8mp_hdmi_pai_ops);
> }
>
> >
> >>
> >>> +}
> >>> +
> >>> +static void imx8mp_hdmi_pai_remove(struct platform_device *pdev)
> >>> +{
> >>> +     component_del(&pdev->dev, &imx8mp_hdmi_pai_ops);
> >>> +}
> >>> +
> >>> +static const struct of_device_id imx8mp_hdmi_pai_of_table[] = {
> >>> +     { .compatible = "fsl,imx8mp-hdmi-pai" },
> >>> +     { /* Sentinel */ }
> >>> +};
> >>> +MODULE_DEVICE_TABLE(of, imx8mp_hdmi_pai_of_table);
> >>> +
> >>> +static struct platform_driver imx8mp_hdmi_pai_platform_driver = {
> >>> +     .probe          = imx8mp_hdmi_pai_probe,
> >>> +     .remove         = imx8mp_hdmi_pai_remove,
> >>> +     .driver         = {
> >>> +             .name   = "imx8mp-hdmi-pai",
> >>> +             .of_match_table = imx8mp_hdmi_pai_of_table,
> >>> +     },
> >>> +};
> >>> +module_platform_driver(imx8mp_hdmi_pai_platform_driver);
> >>> +
> >>> +MODULE_DESCRIPTION("i.MX8MP HDMI PAI driver");
> >>> +MODULE_LICENSE("GPL");
> >>> diff --git a/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
> >>> index 1e7a789ec289..ee08084d2394 100644
> >>> --- a/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
> >>> +++ b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
> >>> @@ -5,11 +5,13 @@
> >>>   */
> >>>
> >>>  #include <linux/clk.h>
> >>> +#include <linux/component.h>
> >>>  #include <linux/mod_devicetable.h>
> >>>  #include <linux/module.h>
> >>>  #include <linux/platform_device.h>
> >>>  #include <drm/bridge/dw_hdmi.h>
> >>>  #include <drm/drm_modes.h>
> >>> +#include <drm/drm_of.h>
> >>>
> >>>  struct imx8mp_hdmi {
> >>>       struct dw_hdmi_plat_data plat_data;
> >>> @@ -79,11 +81,46 @@ static const struct dw_hdmi_phy_ops imx8mp_hdmi_phy_ops = {
> >>>       .update_hpd     = dw_hdmi_phy_update_hpd,
> >>>  };
> >>>
> >>> +static int imx8mp_dw_hdmi_bind(struct device *dev)
> >>> +{
> >>> +     struct dw_hdmi_plat_data *plat_data;
> >>> +     struct imx8mp_hdmi *hdmi;
> >>> +     int ret;
> >>> +
> >>> +     hdmi = dev_get_drvdata(dev);
> >>> +     plat_data = &hdmi->plat_data;
> >>> +
> >>> +     ret = component_bind_all(dev, plat_data);
> >>> +     if (ret)
> >>> +             return dev_err_probe(dev, ret, "component_bind_all failed!\n");
> >>
> >> As component_bind_all() would bind imx8mp-hdmi-pai and hence set
> >> {enable,disable}_audio callbacks, you need to call dw_hdmi_probe() after
> >> component_bind_all() instead of too early in probe() callback.
> >
> > There is no such dependency.
> > Maybe you mixed the hdmi->enable_audio() with pdata->enable_audio().
>
> As the above snippet[1] shows, once dw_hdmi_probe() registers audio device,
> the audio device could be functional soon after audio driver probes, hence
> hdmi->enable_audio() would be called and hence pdata->enable_audio() would
> be called. So, you need to set pdata->enable_audio() before dw_hdmi_probe()
> is called, otherwise pdata->enable_audio could be NULL when is called by
> audio driver.
>
> [...]
>
> >>> +     remote = of_graph_get_remote_node(pdev->dev.of_node, 2, 0);
> >>> +     if (remote && of_device_is_available(remote)) {
> >>
> >> Doesn't of_graph_get_remote_node() ensure that remote is avaiable?
> >
> > No.  'remote' is the node,  not the 'device'.
>
> See of_device_is_available() is called by of_graph_get_remote_node():
>
> struct device_node *of_graph_get_remote_node(const struct device_node *node,
>                                              u32 port, u32 endpoint)
> {
>         struct device_node *endpoint_node, *remote;
>
>         endpoint_node = of_graph_get_endpoint_by_regs(node, port, endpoint);
>         if (!endpoint_node) {
>                 pr_debug("no valid endpoint (%d, %d) for node %pOF\n",
>                          port, endpoint, node);
>                 return NULL;
>         }
>
>         remote = of_graph_get_remote_port_parent(endpoint_node);
>         of_node_put(endpoint_node);
>         if (!remote) {
>                 pr_debug("no valid remote node\n");
>                 return NULL;
>         }
>
>         if (!of_device_is_available(remote)) {
>              ^~~~~~~~~~~~~~~~~~~~~~
>                 pr_debug("not available for remote node\n");
>                 of_node_put(remote);
>                 return NULL;
>         }
>
>         return remote;
> }
> EXPORT_SYMBOL(of_graph_get_remote_node);
>
> >
> >>
> >>> +             drm_of_component_match_add(dev, &match, component_compare_of, remote);
> >>> +
> >>> +             of_node_put(remote);
> >>> +
> >>> +             ret = component_master_add_with_match(dev, &imx8mp_dw_hdmi_ops, match);
> >>> +             if (ret)
> >>> +                     dev_warn(dev, "Unable to register aggregate driver\n");
> >>> +             /*
> >>> +              * This audio function is optional for avoid blocking display.
> >>> +              * So just print warning message and no error is returned.
> >>
> >> No, since PAI node is available here, it has to be bound.  Yet you still need
> >> to properly handle the case where PAI node is inavailable.
> >
> > This is for aggregate driver registration,  not for bind()
> >
> > The bind() is called after both drivers have been registered.  again there is no
> > dependency for both aggregate driver and component driver should be
> > registered or probed.
>
> Sorry for not being clear about my previous wording.  I meant since PAI node is
> available here, component_master_add_with_match() must be called to register
> the master and if it fails to register it, imx8mp_dw_hdmi_probe() should return
> proper error code, not 0.
>
> --
> Regards,
> Liu Ying