[PATCH v2 02/17] video: dw_hdmi: Add Vendor PHY handling

[Jagan Teki]

On Tue, Dec 19, 2023 at 2:34 PM Neil Armstrong
<neil.armstrong at linaro.org> wrote:
>
> On 18/12/2023 20:10, Jagan Teki wrote:
> > From: Jagan Teki <jagan at 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 <jagan at 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.