On Fri, Aug 8, 2025 at 3:49 PM Liu Ying <victor....@nxp.com> wrote:
>
> 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.
Ok, thanks for clarification.
best regards
shengjiu Wang
>
> >
> >>
> >>>
> >>> 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
