On 08/06/2025, Shengjiu Wang wrote:
> On Tue, Aug 5, 2025 at 4:55 PM Liu Ying <victor....@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
