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