On 08/08/2025, Shengjiu Wang wrote:
> On Fri, Aug 8, 2025 at 2:32 PM Liu Ying <victor....@nxp.com> wrote:
>>
>> On 08/07/2025, Shengjiu Wang wrote:
>>> On Wed, Aug 6, 2025 at 2:52 PM Liu Ying <victor....@nxp.com> wrote:
>>>>
>>>> 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);
>>>> }
>>>
>>> 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.
In my mind, this is a common issue as DRM bridges are not properly detached
and attached again.
For now, only drm_encoder_cleanup() calls drm_bridge_detach().
Anyway, this issue is not introduced by this patch series, i.e. it's already
there.
>
>>
>>>
>>> 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.
I don't think the DRM bridge detach/attach issue would be addressed by
using component helper.
>
> 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
--
Regards,
Liu Ying
