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

[neil.armstrong at linaro.org]

On 09/01/2024 21:04, Jagan Teki wrote:
> 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.

Sure, I understand your point, but there's a limited number of users and it can
be greatly simplified, no need to keep the current structure, there's no external
users of the driver.

Just set hdmi->ops with the vendor phy ops from the glue driver if needed,
or let it to NULL if the platform uses the synopsys PHY, it's as simple as that.
If hdmi->ops is NULL it would be replaced with dw_hdmi_synopsys_phy_ops in the
dw_hdmi_detect_phy() function.

Neil

> 
> Thanks,
> Jagan.