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

[Liu Ying]

On 08/07/2025, Shengjiu Wang wrote:
> On Wed, Aug 6, 2025 at 2:52 PM Liu Ying <victor.liu at nxp.com> wrote:
>>
>> On 08/06/2025, Shengjiu Wang wrote:
>>> On Tue, Aug 5, 2025 at 4:55 PM Liu Ying <victor.liu at 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?

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

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

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