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

[Liu Ying <victor.liu@nxp.com>]

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);
}

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