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

[Jagan Teki]

Hi Neil,

On Tue, Dec 19, 2023 at 5:21 PM Jagan Teki <jagan at amarulasolutions.com> wrote:
>
> 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?

Let me know if you have any comments on this. I'm sending V3 would
probably hit in MW.

Thanks,
Jagan.