| Message ID | 20231103230414.1483428-7-dmitry.baryshkov@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm: simplify support for transparent DRM bridges | expand |
On 04/11/2023 01:03, Dmitry Baryshkov wrote: > Use the freshly defined DRM_AUX_HPD_BRIDGE instead of open-coding the > same functionality for the DRM bridge chain termination. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Greg, Bryan, could you pease ack merging this patch through the drm-misc tree together with the rest of the series? You have acked patch 3, but since that time I added this one. > --- > drivers/usb/typec/tcpm/Kconfig | 1 + > drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c | 41 +++---------------- > 2 files changed, 7 insertions(+), 35 deletions(-) > > diff --git a/drivers/usb/typec/tcpm/Kconfig b/drivers/usb/typec/tcpm/Kconfig > index 0b2993fef564..64d5421c69e6 100644 > --- a/drivers/usb/typec/tcpm/Kconfig > +++ b/drivers/usb/typec/tcpm/Kconfig > @@ -80,6 +80,7 @@ config TYPEC_QCOM_PMIC > tristate "Qualcomm PMIC USB Type-C Port Controller Manager driver" > depends on ARCH_QCOM || COMPILE_TEST > depends on DRM || DRM=n > + select DRM_AUX_HPD_BRIDGE if DRM_BRIDGE > help > A Type-C port and Power Delivery driver which aggregates two > discrete pieces of silicon in the PM8150b PMIC block: the > diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c > index 581199d37b49..1a2b4bddaa97 100644 > --- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c > +++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c > @@ -18,7 +18,7 @@ > #include <linux/usb/tcpm.h> > #include <linux/usb/typec_mux.h> > > -#include <drm/drm_bridge.h> > +#include <drm/bridge/aux-bridge.h> > > #include "qcom_pmic_typec_pdphy.h" > #include "qcom_pmic_typec_port.h" > @@ -36,7 +36,6 @@ struct pmic_typec { > struct pmic_typec_port *pmic_typec_port; > bool vbus_enabled; > struct mutex lock; /* VBUS state serialization */ > - struct drm_bridge bridge; > }; > > #define tcpc_to_tcpm(_tcpc_) container_of(_tcpc_, struct pmic_typec, tcpc) > @@ -150,35 +149,6 @@ static int qcom_pmic_typec_init(struct tcpc_dev *tcpc) > return 0; > } > > -#if IS_ENABLED(CONFIG_DRM) > -static int qcom_pmic_typec_attach(struct drm_bridge *bridge, > - enum drm_bridge_attach_flags flags) > -{ > - return flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR ? 0 : -EINVAL; > -} > - > -static const struct drm_bridge_funcs qcom_pmic_typec_bridge_funcs = { > - .attach = qcom_pmic_typec_attach, > -}; > - > -static int qcom_pmic_typec_init_drm(struct pmic_typec *tcpm) > -{ > - tcpm->bridge.funcs = &qcom_pmic_typec_bridge_funcs; > -#ifdef CONFIG_OF > - tcpm->bridge.of_node = of_get_child_by_name(tcpm->dev->of_node, "connector"); > -#endif > - tcpm->bridge.ops = DRM_BRIDGE_OP_HPD; > - tcpm->bridge.type = DRM_MODE_CONNECTOR_DisplayPort; > - > - return devm_drm_bridge_add(tcpm->dev, &tcpm->bridge); > -} > -#else > -static int qcom_pmic_typec_init_drm(struct pmic_typec *tcpm) > -{ > - return 0; > -} > -#endif > - > static int qcom_pmic_typec_probe(struct platform_device *pdev) > { > struct pmic_typec *tcpm; > @@ -186,6 +156,7 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev) > struct device_node *np = dev->of_node; > const struct pmic_typec_resources *res; > struct regmap *regmap; > + struct device *bridge_dev; > u32 base[2]; > int ret; > > @@ -241,14 +212,14 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev) > mutex_init(&tcpm->lock); > platform_set_drvdata(pdev, tcpm); > > - ret = qcom_pmic_typec_init_drm(tcpm); > - if (ret) > - return ret; > - > tcpm->tcpc.fwnode = device_get_named_child_node(tcpm->dev, "connector"); > if (!tcpm->tcpc.fwnode) > return -EINVAL; > > + bridge_dev = drm_dp_hpd_bridge_register(tcpm->dev, to_of_node(tcpm->tcpc.fwnode)); > + if (IS_ERR(bridge_dev)) > + return PTR_ERR(bridge_dev); > + > tcpm->tcpm_port = tcpm_register_port(tcpm->dev, &tcpm->tcpc); > if (IS_ERR(tcpm->tcpm_port)) { > ret = PTR_ERR(tcpm->tcpm_port);
On 03/11/2023 23:03, Dmitry Baryshkov wrote: > Use the freshly defined DRM_AUX_HPD_BRIDGE instead of open-coding the > same functionality for the DRM bridge chain termination. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > + bridge_dev = drm_dp_hpd_bridge_register(tcpm->dev, to_of_node(tcpm->tcpc.fwnode)); > + if (IS_ERR(bridge_dev)) > + return PTR_ERR(bridge_dev); > + What is the effect if we never attach any bridged devices ? We make an aux device that just hangs around and eventually get cleaned up on release ? That's the way I read this code anyway. Acked-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
On Tue, 21 Nov 2023 at 12:46, Bryan O'Donoghue <bryan.odonoghue@linaro.org> wrote: > > On 03/11/2023 23:03, Dmitry Baryshkov wrote: > > Use the freshly defined DRM_AUX_HPD_BRIDGE instead of open-coding the > > same functionality for the DRM bridge chain termination. > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > + bridge_dev = drm_dp_hpd_bridge_register(tcpm->dev, to_of_node(tcpm->tcpc.fwnode)); > > + if (IS_ERR(bridge_dev)) > > + return PTR_ERR(bridge_dev); > > + > > What is the effect if we never attach any bridged devices ? > > We make an aux device that just hangs around and eventually get cleaned > up on release ? That's the way I read this code anyway. Yes. That's the point, to untangle the USB code and the DRM bridge. > Acked-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
diff --git a/drivers/usb/typec/tcpm/Kconfig b/drivers/usb/typec/tcpm/Kconfig index 0b2993fef564..64d5421c69e6 100644 --- a/drivers/usb/typec/tcpm/Kconfig +++ b/drivers/usb/typec/tcpm/Kconfig @@ -80,6 +80,7 @@ config TYPEC_QCOM_PMIC tristate "Qualcomm PMIC USB Type-C Port Controller Manager driver" depends on ARCH_QCOM || COMPILE_TEST depends on DRM || DRM=n + select DRM_AUX_HPD_BRIDGE if DRM_BRIDGE help A Type-C port and Power Delivery driver which aggregates two discrete pieces of silicon in the PM8150b PMIC block: the diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c index 581199d37b49..1a2b4bddaa97 100644 --- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c +++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c @@ -18,7 +18,7 @@ #include <linux/usb/tcpm.h> #include <linux/usb/typec_mux.h> -#include <drm/drm_bridge.h> +#include <drm/bridge/aux-bridge.h> #include "qcom_pmic_typec_pdphy.h" #include "qcom_pmic_typec_port.h" @@ -36,7 +36,6 @@ struct pmic_typec { struct pmic_typec_port *pmic_typec_port; bool vbus_enabled; struct mutex lock; /* VBUS state serialization */ - struct drm_bridge bridge; }; #define tcpc_to_tcpm(_tcpc_) container_of(_tcpc_, struct pmic_typec, tcpc) @@ -150,35 +149,6 @@ static int qcom_pmic_typec_init(struct tcpc_dev *tcpc) return 0; } -#if IS_ENABLED(CONFIG_DRM) -static int qcom_pmic_typec_attach(struct drm_bridge *bridge, - enum drm_bridge_attach_flags flags) -{ - return flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR ? 0 : -EINVAL; -} - -static const struct drm_bridge_funcs qcom_pmic_typec_bridge_funcs = { - .attach = qcom_pmic_typec_attach, -}; - -static int qcom_pmic_typec_init_drm(struct pmic_typec *tcpm) -{ - tcpm->bridge.funcs = &qcom_pmic_typec_bridge_funcs; -#ifdef CONFIG_OF - tcpm->bridge.of_node = of_get_child_by_name(tcpm->dev->of_node, "connector"); -#endif - tcpm->bridge.ops = DRM_BRIDGE_OP_HPD; - tcpm->bridge.type = DRM_MODE_CONNECTOR_DisplayPort; - - return devm_drm_bridge_add(tcpm->dev, &tcpm->bridge); -} -#else -static int qcom_pmic_typec_init_drm(struct pmic_typec *tcpm) -{ - return 0; -} -#endif - static int qcom_pmic_typec_probe(struct platform_device *pdev) { struct pmic_typec *tcpm; @@ -186,6 +156,7 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev) struct device_node *np = dev->of_node; const struct pmic_typec_resources *res; struct regmap *regmap; + struct device *bridge_dev; u32 base[2]; int ret; @@ -241,14 +212,14 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev) mutex_init(&tcpm->lock); platform_set_drvdata(pdev, tcpm); - ret = qcom_pmic_typec_init_drm(tcpm); - if (ret) - return ret; - tcpm->tcpc.fwnode = device_get_named_child_node(tcpm->dev, "connector"); if (!tcpm->tcpc.fwnode) return -EINVAL; + bridge_dev = drm_dp_hpd_bridge_register(tcpm->dev, to_of_node(tcpm->tcpc.fwnode)); + if (IS_ERR(bridge_dev)) + return PTR_ERR(bridge_dev); + tcpm->tcpm_port = tcpm_register_port(tcpm->dev, &tcpm->tcpc); if (IS_ERR(tcpm->tcpm_port)) { ret = PTR_ERR(tcpm->tcpm_port);
Use the freshly defined DRM_AUX_HPD_BRIDGE instead of open-coding the same functionality for the DRM bridge chain termination. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/usb/typec/tcpm/Kconfig | 1 + drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c | 41 +++---------------- 2 files changed, 7 insertions(+), 35 deletions(-)