Re: [PATCH v6 1/4] usb: typec: Add QCOM PMIC typec detection driver
On 7/29/2020 1:33 AM, Stephen Boyd wrote: > Quoting Wesley Cheng (2020-07-29 00:13:37) >> diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig >> index 559dd06117e7..3e375f82849d 100644 >> --- a/drivers/usb/typec/Kconfig >> +++ b/drivers/usb/typec/Kconfig >> @@ -73,6 +73,18 @@ config TYPEC_TPS6598X >> If you choose to build this driver as a dynamically linked module, >> the >> module will be called tps6598x.ko. >> >> +config TYPEC_QCOM_PMIC >> + tristate "Qualcomm PMIC USB Type-C driver" >> + depends on ARCH_QCOM > > Can you add || COMPILE_TEST here? > Sure, will do. >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include > > Is this include used? > >> +#include >> +#include > > Is this include used? > Reviewed which includes I used, and removed the ones that were not needed. >> +static void qcom_pmic_typec_enable_vbus_regulator(struct qcom_pmic_typec >> + *qcom_usb, bool >> enable) >> +{ >> + int ret = 0; > > Please don't assign and then reassign before testing this variable. > I will just remove the assignment here. >> + if (stat & CC_ATTACHED) { >> + orientation = ((stat & CC_ORIENTATION) >> 1) ? > > Do we really need to shift >> by 1? Seems useless for a test. > Agreed, we can remove the shift. >> + ret = of_property_read_u32(dev->of_node, "reg", ); >> + if (ret < 0) { >> + dev_err(dev, "missing base address"); > > Please add newlines at the end of printk messages. > Done. >> + irq = platform_get_irq(pdev, 0); >> + if (irq < 0) { >> + dev_err(dev, "Failed to get CC irq\n"); > > platform_get_irq() already prints an error message. Please remove this. > Got it. >> +static const struct of_device_id qcom_pmic_typec_table[] = { >> + { .compatible = "qcom,pm8150b-usb-typec" }, >> + { }, > > Nitpick: Drop the comma here so nothing can come after without causing a > compile error. > Sure. >> +static struct platform_driver qcom_pmic_typec = { >> + .driver = { >> + .name = "qcom,pmic-typec", >> + .of_match_table = qcom_pmic_typec_table, >> + }, >> + .probe = qcom_pmic_typec_probe, >> + .remove = qcom_pmic_typec_remove, >> +}; >> + > > Another nitpick: Drop the newline and make module_platform_driver() > follow directly after the driver. > Ok, will do. Thanks for the review/feedback, Stephen. -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v6 1/4] usb: typec: Add QCOM PMIC typec detection driver
Quoting Wesley Cheng (2020-07-29 00:13:37) > diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig > index 559dd06117e7..3e375f82849d 100644 > --- a/drivers/usb/typec/Kconfig > +++ b/drivers/usb/typec/Kconfig > @@ -73,6 +73,18 @@ config TYPEC_TPS6598X > If you choose to build this driver as a dynamically linked module, > the > module will be called tps6598x.ko. > > +config TYPEC_QCOM_PMIC > + tristate "Qualcomm PMIC USB Type-C driver" > + depends on ARCH_QCOM Can you add || COMPILE_TEST here? > + help > + Driver for supporting role switch over the Qualcomm PMIC. This will > + handle the USB Type-C role and orientation detection reported by the > + QCOM PMIC if the PMIC has the capability to handle USB Type-C > + detection. > + > + It will also enable the VBUS output to connected devices when a > + DFP connection is made. > + > source "drivers/usb/typec/mux/Kconfig" > > source "drivers/usb/typec/altmodes/Kconfig" > diff --git a/drivers/usb/typec/qcom-pmic-typec.c > b/drivers/usb/typec/qcom-pmic-typec.c > new file mode 100644 > index ..5ae3af03c638 > --- /dev/null > +++ b/drivers/usb/typec/qcom-pmic-typec.c > @@ -0,0 +1,275 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2020, The Linux Foundation. All rights reserved. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include Is this include used? > +#include > +#include Is this include used? > +#include > +#include > +#include > + > +#define TYPEC_MISC_STATUS 0xb > +#define CC_ATTACHEDBIT(0) > +#define CC_ORIENTATION BIT(1) > +#define SNK_SRC_MODE BIT(6) > +#define TYPEC_MODE_CFG 0x44 > +#define TYPEC_DISABLE_CMD BIT(0) > +#define EN_SNK_ONLYBIT(1) > +#define EN_SRC_ONLYBIT(2) > +#define TYPEC_VCONN_CONTROL0x46 > +#define VCONN_EN_SRC BIT(0) > +#define VCONN_EN_VAL BIT(1) > +#define TYPEC_EXIT_STATE_CFG 0x50 > +#define SEL_SRC_UPPER_REF BIT(2) > +#define TYPEC_INTR_EN_CFG_10x5e > +#define TYPEC_INTR_EN_CFG_1_MASK GENMASK(7, 0) > + > +struct qcom_pmic_typec { > + struct device *dev; > + struct fwnode_handle*fwnode; > + struct regmap *regmap; > + u32 base; > + > + struct typec_capability *cap; > + struct typec_port *port; > + struct usb_role_switch *role_sw; > + > + struct regulator*vbus_reg; > + boolvbus_enabled; > +}; > + > +static void qcom_pmic_typec_enable_vbus_regulator(struct qcom_pmic_typec > + *qcom_usb, bool > enable) > +{ > + int ret = 0; Please don't assign and then reassign before testing this variable. > + > + if (enable == qcom_usb->vbus_enabled) > + return; > + > + if (!qcom_usb->vbus_reg) { > + qcom_usb->vbus_reg = devm_regulator_get(qcom_usb->dev, > + "usb_vbus"); > + if (IS_ERR(qcom_usb->vbus_reg)) { > + qcom_usb->vbus_reg = NULL; > + return; > + } > + } > + > + if (enable) { > + ret = regulator_enable(qcom_usb->vbus_reg); > + if (ret) > + return; > + } else { > + ret = regulator_disable(qcom_usb->vbus_reg); > + if (ret) > + return; > + } > + qcom_usb->vbus_enabled = enable; > +} > + > +static void qcom_pmic_typec_check_connection(struct qcom_pmic_typec > *qcom_usb) > +{ > + enum typec_orientation orientation; > + enum usb_role role; > + unsigned int stat; > + bool enable_vbus; > + > + regmap_read(qcom_usb->regmap, qcom_usb->base + TYPEC_MISC_STATUS, > + ); > + > + if (stat & CC_ATTACHED) { > + orientation = ((stat & CC_ORIENTATION) >> 1) ? Do we really need to shift >> by 1? Seems useless for a test. > + TYPEC_ORIENTATION_REVERSE : > + TYPEC_ORIENTATION_NORMAL; > + typec_set_orientation(qcom_usb->port, orientation); > + > + role = (stat & SNK_SRC_MODE) ? USB_ROLE_HOST : > USB_ROLE_DEVICE; > + if (role == USB_ROLE_HOST) > + enable_vbus = true; > + else > + enable_vbus = false; > + } else { > + role = USB_ROLE_NONE; > + enable_vbus = false; > + } > + > + qcom_pmic_typec_enable_vbus_regulator(qcom_usb, enable_vbus); > + usb_role_switch_set_role(qcom_usb->role_sw, role);
[PATCH v6 1/4] usb: typec: Add QCOM PMIC typec detection driver
The QCOM SPMI typec driver handles the role and orientation detection, and notifies client drivers using the USB role switch framework. It registers as a typec port, so orientation can be communicated using the typec switch APIs. The driver also attains a handle to the VBUS output regulator, so it can enable/disable the VBUS source when acting as a host/device. Signed-off-by: Wesley Cheng Acked-by: Heikki Krogerus --- drivers/usb/typec/Kconfig | 12 ++ drivers/usb/typec/Makefile | 1 + drivers/usb/typec/qcom-pmic-typec.c | 275 3 files changed, 288 insertions(+) create mode 100644 drivers/usb/typec/qcom-pmic-typec.c diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig index 559dd06117e7..3e375f82849d 100644 --- a/drivers/usb/typec/Kconfig +++ b/drivers/usb/typec/Kconfig @@ -73,6 +73,18 @@ config TYPEC_TPS6598X If you choose to build this driver as a dynamically linked module, the module will be called tps6598x.ko. +config TYPEC_QCOM_PMIC + tristate "Qualcomm PMIC USB Type-C driver" + depends on ARCH_QCOM + help + Driver for supporting role switch over the Qualcomm PMIC. This will + handle the USB Type-C role and orientation detection reported by the + QCOM PMIC if the PMIC has the capability to handle USB Type-C + detection. + + It will also enable the VBUS output to connected devices when a + DFP connection is made. + source "drivers/usb/typec/mux/Kconfig" source "drivers/usb/typec/altmodes/Kconfig" diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile index 7753a5c3cd46..cceffd987643 100644 --- a/drivers/usb/typec/Makefile +++ b/drivers/usb/typec/Makefile @@ -6,4 +6,5 @@ obj-$(CONFIG_TYPEC_TCPM)+= tcpm/ obj-$(CONFIG_TYPEC_UCSI) += ucsi/ obj-$(CONFIG_TYPEC_HD3SS3220) += hd3ss3220.o obj-$(CONFIG_TYPEC_TPS6598X) += tps6598x.o +obj-$(CONFIG_TYPEC_QCOM_PMIC) += qcom-pmic-typec.o obj-$(CONFIG_TYPEC)+= mux/ diff --git a/drivers/usb/typec/qcom-pmic-typec.c b/drivers/usb/typec/qcom-pmic-typec.c new file mode 100644 index ..5ae3af03c638 --- /dev/null +++ b/drivers/usb/typec/qcom-pmic-typec.c @@ -0,0 +1,275 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (c) 2020, The Linux Foundation. All rights reserved. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#define TYPEC_MISC_STATUS 0xb +#define CC_ATTACHEDBIT(0) +#define CC_ORIENTATION BIT(1) +#define SNK_SRC_MODE BIT(6) +#define TYPEC_MODE_CFG 0x44 +#define TYPEC_DISABLE_CMD BIT(0) +#define EN_SNK_ONLYBIT(1) +#define EN_SRC_ONLYBIT(2) +#define TYPEC_VCONN_CONTROL0x46 +#define VCONN_EN_SRC BIT(0) +#define VCONN_EN_VAL BIT(1) +#define TYPEC_EXIT_STATE_CFG 0x50 +#define SEL_SRC_UPPER_REF BIT(2) +#define TYPEC_INTR_EN_CFG_10x5e +#define TYPEC_INTR_EN_CFG_1_MASK GENMASK(7, 0) + +struct qcom_pmic_typec { + struct device *dev; + struct fwnode_handle*fwnode; + struct regmap *regmap; + u32 base; + + struct typec_capability *cap; + struct typec_port *port; + struct usb_role_switch *role_sw; + + struct regulator*vbus_reg; + boolvbus_enabled; +}; + +static void qcom_pmic_typec_enable_vbus_regulator(struct qcom_pmic_typec + *qcom_usb, bool enable) +{ + int ret = 0; + + if (enable == qcom_usb->vbus_enabled) + return; + + if (!qcom_usb->vbus_reg) { + qcom_usb->vbus_reg = devm_regulator_get(qcom_usb->dev, + "usb_vbus"); + if (IS_ERR(qcom_usb->vbus_reg)) { + qcom_usb->vbus_reg = NULL; + return; + } + } + + if (enable) { + ret = regulator_enable(qcom_usb->vbus_reg); + if (ret) + return; + } else { + ret = regulator_disable(qcom_usb->vbus_reg); + if (ret) + return; + } + qcom_usb->vbus_enabled = enable; +} + +static void qcom_pmic_typec_check_connection(struct qcom_pmic_typec *qcom_usb) +{ + enum typec_orientation orientation; + enum usb_role role; + unsigned int stat; + bool enable_vbus; + + regmap_read(qcom_usb->regmap, qcom_usb->base + TYPEC_MISC_STATUS, + ); + + if (stat & CC_ATTACHED) { + orientation = ((stat & CC_ORIENTATION) >> 1) ? +