Re: [PATCH 5/7] phy: qcom-qmp-combo: Introduce drm_bridge

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

On 27/04/2023 16:11, Neil Armstrong wrote:
> On 26/04/2023 12:33, Bryan O'Donoghue wrote:
>> On Tue, Apr 25, 2023 at 4:40 AM Bjorn Andersson
>> <quic_bjorande@quicinc.com> wrote:
>>>
>>> The QMP combo PHY sits in an of_graph connected between the DisplayPort
>>> controller and a USB Type-C connector (or possibly a redriver).
>>>
>>> The TCPM needs to be able to convey the HPD signal to the DisplayPort
>>> controller, but no directly link is provided by DeviceTree so the signal
>>> needs to "pass through" the QMP combo phy.
>>>
>>> Handle this by introducing a drm_bridge which upon initialization finds
>>> the next bridge (i.e. the usb-c-connector) and chain this together. This
>>> way HPD changes in the connector will propagate to the DisplayPort
>>> driver.
>>>
>>> The connector bridge is resolved lazily, as the TCPM is expected to be
>>> able to resolve the typec mux and switch at probe time, so the QMP combo
>>> phy will probe before the TCPM.
>>>
>>> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
>>> ---
>>>   drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 36 +++++++++++++++++++++++
>>>   1 file changed, 36 insertions(+)
>>>
>>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c 
>>> b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
>>> index 5d6d6ef3944b..84bc08002537 100644
>>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
>>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
>>> @@ -22,6 +22,8 @@
>>>   #include <linux/usb/typec.h>
>>>   #include <linux/usb/typec_mux.h>
>>>
>>> +#include <drm/drm_bridge.h>
>>> +
>>>   #include <dt-bindings/phy/phy-qcom-qmp.h>
>>>
>>>   #include "phy-qcom-qmp.h"
>>> @@ -1332,6 +1334,8 @@ struct qmp_combo {
>>>          struct clk_hw dp_link_hw;
>>>          struct clk_hw dp_pixel_hw;
>>>
>>> +       struct drm_bridge bridge;
>>> +
>>>          struct typec_switch_dev *sw;
>>>          enum typec_orientation orientation;
>>>   };
>>> @@ -3196,6 +3200,34 @@ static int qmp_combo_register_clocks(struct 
>>> qmp_combo *qmp, struct device_node *
>>>          return devm_add_action_or_reset(qmp->dev, 
>>> phy_clk_release_provider, dp_np);
>>>   }
>>>
>>> +static int qmp_combo_bridge_attach(struct drm_bridge *bridge,
>>> +                                  enum drm_bridge_attach_flags flags)
>>> +{
>>> +       struct qmp_combo *qmp = container_of(bridge, struct 
>>> qmp_combo, bridge);
>>> +       struct drm_bridge *next_bridge;
>>> +
>>> +       if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
>>> +               return -EINVAL;
>>> +
>>> +       next_bridge = devm_drm_of_get_bridge(qmp->dev, 
>>> qmp->dev->of_node, 0, 0);
>>> +       if (IS_ERR(next_bridge))
>>> +               return dev_err_probe(qmp->dev, PTR_ERR(next_bridge), 
>>> "failed to acquire drm_bridge\n");
>>> +
>>> +       return drm_bridge_attach(bridge->encoder, next_bridge, 
>>> bridge, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>>> +}
>>> +
>>> +static const struct drm_bridge_funcs qmp_combo_bridge_funcs = {
>>> +       .attach = qmp_combo_bridge_attach,
>>> +};
>>> +
>>> +static int qmp_combo_dp_register_bridge(struct qmp_combo *qmp)
>>> +{
>>> +       qmp->bridge.funcs = &qmp_combo_bridge_funcs;
>>> +       qmp->bridge.of_node = qmp->dev->of_node;
>>> +
>>> +       return devm_drm_bridge_add(qmp->dev, &qmp->bridge);
>>> +}
>>> +
>>>   static int qmp_combo_parse_dt_lecacy_dp(struct qmp_combo *qmp, 
>>> struct device_node *np)
>>>   {
>>>          struct device *dev = qmp->dev;
>>> @@ -3459,6 +3491,10 @@ static int qmp_combo_probe(struct 
>>> platform_device *pdev)
>>>          if (ret)
>>>                  return ret;
>>>
>>> +       ret = qmp_combo_dp_register_bridge(qmp);
>>> +       if (ret)
>>> +               return ret;
> 
> I think the DRM part should be only built if CONFIG_DRM is enabled, I don't
> have a strong opinion on this, I think Vinod could help here.
> 
>>> +
>>>          /* Check for legacy binding with child nodes. */
>>>          usb_np = of_get_child_by_name(dev->of_node, "usb3-phy");
>>>          if (usb_np) {
>>> -- 
>>> 2.39.2
>>>
>>
>> You need to add some or all of these
>>         select DRM_DISPLAY_DP_HELPER
>>         select DRM_DISPLAY_HELPER
>>         select DRM_DP_AUX_BUS
>>         select DRM_KMS_HELPER
>>         select DRM_MIPI_DSI
>>         select DRM_PANEL
>>
>>
>> /opt/linaro/gcc-linaro-7.5.0-2019.12-x86_64_aarch64-linux-gnu/bin/aarch64-linux-gnu-ld:
>> Unexpected GOT/PLT entries detected!
>> /opt/linaro/gcc-linaro-7.5.0-2019.12-x86_64_aarch64-linux-gnu/bin/aarch64-linux-gnu-ld:
>> Unexpected run-time procedure linkages detected!
>> drivers/phy/qualcomm/phy-qcom-qmp-combo.o: In function
>> `qmp_combo_bridge_attach':
>> phy-qcom-qmp-combo.c:(.text+0xb50): undefined reference to
>> `devm_drm_of_get_bridge'
>> phy-qcom-qmp-combo.c:(.text+0xb6c): undefined reference to 
>> `drm_bridge_attach'
>> drivers/phy/qualcomm/phy-qcom-qmp-combo.o: In function `qmp_combo_probe':
>> phy-qcom-qmp-combo.c:(.text+0x13fc): undefined reference to
>> `devm_drm_bridge_add'
> 
> I think CONFIG_DRM_PANEL_BRIDGE in addition to CONFIG_DRM. should be 
> enough.
> 

I'd say DRM_PANEL_BRIDGE || !DRM_PANEL_BRIDGE in addition to DRM. And we 
probably should fix the  devm_drm_of_get_bridge() stub to use 
drm_of_find_panel_or_bridge() with panel = NULL.

> With this config added and my drm-bridge hat:
> 
> Acked-by: Neil Armstrong <neil.armstrong@linaro.org>
> 
> Neil
> 
> 
>>
>> ---
>> bod
> 

-- 
With best wishes
Dmitry