On Tue, Dec 19, 2023 at 2:34 PM Neil Armstrong
<neil.armstr...@linaro.org> wrote:
>
> On 18/12/2023 20:10, Jagan Teki wrote:
> > From: Jagan Teki <ja...@edgeble.ai>
> >
> > DW HDMI support Vendor PHY like Rockchip RK3328 Inno HDMI PHY.
> >
> > Extend the vendor phy handling by adding platform phy hooks.
> >
> > Signed-off-by: Jagan Teki <ja...@edgeble.ai>
> > ---
> > Changes for v2:
> > - fix meson cfg
> >
> > drivers/video/dw_hdmi.c | 29 +++++++++++++++++++++++++++-
> > drivers/video/meson/meson_dw_hdmi.c | 11 ++++++++++-
> > drivers/video/rockchip/rk3399_hdmi.c | 8 +++++++-
> > drivers/video/rockchip/rk_hdmi.c | 2 +-
> > drivers/video/sunxi/sunxi_dw_hdmi.c | 11 ++++++++++-
> > include/dw_hdmi.h | 14 +++++++++++++-
> > 6 files changed, 69 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/video/dw_hdmi.c b/drivers/video/dw_hdmi.c
> > index c4fbb18294..ea12a09407 100644
> > --- a/drivers/video/dw_hdmi.c
> > +++ b/drivers/video/dw_hdmi.c
> > @@ -988,7 +988,7 @@ int dw_hdmi_enable(struct dw_hdmi *hdmi, const struct
> > display_timing *edid)
> >
> > hdmi_av_composer(hdmi, edid);
> >
> > - ret = hdmi->phy_set(hdmi, edid->pixelclock.typ);
> > + ret = hdmi->ops->phy_set(hdmi, edid->pixelclock.typ);
> > if (ret)
> > return ret;
> >
> > @@ -1009,10 +1009,37 @@ int dw_hdmi_enable(struct dw_hdmi *hdmi, const
> > struct display_timing *edid)
> > return 0;
> > }
> >
> > +static const struct dw_hdmi_phy_ops dw_hdmi_synopsys_phy_ops = {
> > + .phy_set = dw_hdmi_phy_cfg,
> > +};
> > +
> > +static void dw_hdmi_detect_phy(struct dw_hdmi *hdmi)
> > +{
> > + if (!hdmi->data)
> > + return;
> > +
> > + /* hook Synopsys PHYs ops */
> > + if (!hdmi->data->phy_force_vendor) {
> > + hdmi->ops = &dw_hdmi_synopsys_phy_ops;
> > + return;
> > + }
> > +
> > + /* Vendor HDMI PHYs must assign phy_ops in plat_data */
> > + if (!hdmi->data->phy_ops) {
> > + printf("Unsupported Vendor HDMI phy_ops\n");
> > + return;
> > + }
> > +
> > + /* hook Vendor HDMI PHYs ops */
> > + hdmi->ops = hdmi->data->phy_ops;
>
> Sorry but I still don't understand why you need phy_force_vendor & phy_ops,
> this code clearly fails if you have phy_force_vendor=true && phy_ops=NULL,
> so drop phy_force_vendor and simply use phy_ops if != NULL, and since it's
> the only element of dw_hdmi_plat_data, drop dw_hdmi_plat_data and pass
> dw_hdmi_phy_ops directly in the dw_hdmi struct.
>
> So in dw_hdmi_detect_phy(), if hdmi->ops is NULL, set it to
> dw_hdmi_synopsys_phy_ops.
Let me elaborate more.
DW HDMI IP must have phy ops. It never be NULL. Either it uses
1. Internal PHY via DW called them Synopsys PHYs ops - for example, rk3399
2. Vendor PHY via vendor phy meson, sunxi, rk3328
For case 1) phy_force_vendor is false so it uses dw_hdmi_synopsys_phy_ops
For case 2) phy_force_vendor is true so it uses dw_hdmi_plat_data phy ops
dw_hdmi_detect_phy assigns internal phy ops first and then vendor phy
ops based on phy_force_vendor flag.
If we remove dw_hdmi_plat_data how can we assign or differentiate two
types of phy ops hooks? can you explain?
Thanks,
Jagan.
