Hi, Thanks for the feedback. > Subject: Re: [PATCH V3 3/7] usb: typec: driver for TI HD3SS3220 USB Type-C > DRP port controller > > Hi, > On Fri, 2019-04-12 at 11:16 +0100, Biju Das wrote: > > Driver for TI HD3SS3220 USB Type-C DRP port controller. > > > > The driver currently registers the port and supports data role swapping. > > > > Signed-off-by: Biju Das <biju.das@xxxxxxxxxxxxxx> > > --- > > Note: This patch has compilation dependency on > > https://patchwork.kernel.org/patch/10882555/ > > > > V2-->V3 > > * Used the new api "usb_role_switch by node" for getting > > remote endpoint associated with Type-C USB DRP port > > controller devices. > > V1-->V2 > > * Driver uses usb role class instead of extcon for dual role switch > > and also handles connect/disconnect events. > > --- > > drivers/usb/typec/Kconfig | 10 ++ > > drivers/usb/typec/Makefile | 1 + > > drivers/usb/typec/hd3ss3220.c | 278 > > ++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 289 insertions(+) > > create mode 100644 drivers/usb/typec/hd3ss3220.c > > > > diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig > > index 89d9193..92a3717 100644 > > --- a/drivers/usb/typec/Kconfig > > +++ b/drivers/usb/typec/Kconfig > > @@ -50,6 +50,16 @@ source "drivers/usb/typec/tcpm/Kconfig" > > > > source "drivers/usb/typec/ucsi/Kconfig" > > > > +config TYPEC_HD3SS3220 > > + tristate "TI HD3SS3220 Type-C DRP Port controller driver" > > + depends on I2C > > + help > > + Say Y or M here if your system has TI HD3SS3220 Type-C DRP Port > > + controller driver. > > + > > + If you choose to build this driver as a dynamically linked module, the > > + module will be called hd3ss3220.ko. > > + > > config TYPEC_TPS6598X > > tristate "TI TPS6598x USB Power Delivery controller driver" > > depends on I2C > > diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile > > index 6696b72..7753a5c3 100644 > > --- a/drivers/usb/typec/Makefile > > +++ b/drivers/usb/typec/Makefile > > @@ -4,5 +4,6 @@ typec-y := class.o mux.o bus.o > > obj-$(CONFIG_TYPEC) += altmodes/ > > 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) += mux/ > > diff --git a/drivers/usb/typec/hd3ss3220.c > > b/drivers/usb/typec/hd3ss3220.c new file mode 100644 index > > 0000000..b5cb42c > > --- /dev/null > > +++ b/drivers/usb/typec/hd3ss3220.c > > @@ -0,0 +1,278 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * TI HD3SS3220 Type-C DRP Port Controller Driver > > + * > > + * Copyright (C) 2019 Renesas Electronics Corp. > > + */ > > + > > +#include <linux/module.h> > > +#include <linux/i2c.h> > > +#include <linux/usb/role.h> > > +#include <linux/irqreturn.h> > > +#include <linux/interrupt.h> > > +#include <linux/module.h> > > +#include <linux/of_graph.h> > > +#include <linux/regmap.h> > > +#include <linux/slab.h> > > +#include <linux/usb/typec.h> > > +#include <linux/delay.h> > > + > > +#define HD3SS3220_REG_CN_STAT_CTRL 0x09 > > +#define HD3SS3220_REG_GEN_CTRL 0x0A > > +#define HD3SS3220_REG_DEV_REV 0xA0 > > + > > +/* Register HD3SS3220_REG_CN_STAT_CTRL*/ > > +#define HD3SS3220_REG_CN_STAT_CTRL_ATTACHED_STATE_MASK > (BIT(7) | BIT(6)) > > +#define HD3SS3220_REG_CN_STAT_CTRL_AS_DFP BIT(6) > > +#define HD3SS3220_REG_CN_STAT_CTRL_AS_UFP BIT(7) > > +#define HD3SS3220_REG_CN_STAT_CTRL_TO_ACCESSORY > (BIT(7) | BIT(6)) > > +#define HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS BIT(4) > > + > > +/* Register HD3SS3220_REG_GEN_CTRL*/ > > +#define HD3SS3220_REG_GEN_CTRL_SRC_PREF_MASK > (BIT(2) | BIT(1)) > > +#define HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_DEFAULT 0x00 > > +#define HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SNK BIT(1) > > +#define HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SRC > (BIT(2) | BIT(1)) > > + > > +struct hd3ss3220 { > > + struct device *dev; > > + struct regmap *regmap; > > + struct usb_role_switch *role_sw; > > + struct typec_port *port; > > + struct typec_capability typec_cap; > > +}; > > + > > +static int hd3ss3220_set_source_pref(struct hd3ss3220 *hd3ss3220, int > > +src_pref) { > > + unsigned int reg_val; > > + int ret; > > + > > + ret = regmap_read(hd3ss3220->regmap, > HD3SS3220_REG_GEN_CTRL, ®_val); > > + if (ret < 0) > > + return ret; > > + > > + reg_val &= ~HD3SS3220_REG_GEN_CTRL_SRC_PREF_MASK; > > + reg_val |= src_pref; > > + > > + return regmap_write(hd3ss3220->regmap, > HD3SS3220_REG_GEN_CTRL, > > +reg_val); > use regmap_update_bits()? Yes, As an optimization will change it to regmap_update_bits. > > +} > > + > > +static enum usb_role hd3ss3220_get_attached_state(struct hd3ss3220 > > +*hd3ss3220) { > > + unsigned int reg_val; > > + enum usb_role attached_state; > > + int ret; > > + > > + ret = regmap_read(hd3ss3220->regmap, > HD3SS3220_REG_CN_STAT_CTRL, > > + ®_val); > > + if (ret < 0) > > + return ret; > > + > > + switch (reg_val & > HD3SS3220_REG_CN_STAT_CTRL_ATTACHED_STATE_MASK) { > > + case HD3SS3220_REG_CN_STAT_CTRL_AS_DFP: > > + attached_state = USB_ROLE_HOST; > > + break; > > + case HD3SS3220_REG_CN_STAT_CTRL_AS_UFP: > > + attached_state = USB_ROLE_DEVICE; > > + break; > > + default: > > + attached_state = USB_ROLE_NONE; > > + } > > + > > + return attached_state; > > +} > > + > > +static int hd3ss3220_dr_set(const struct typec_capability *cap, > > + enum typec_data_role role) > > +{ > > + struct hd3ss3220 *hd3ss3220 = container_of(cap, struct hd3ss3220, > > + typec_cap); > > + enum usb_role role_val; > > + int pref, ret = 0; > > + > > + if (role == TYPEC_HOST) { > > + role_val = USB_ROLE_HOST; > > + pref = > HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SRC; > > + } else { > > + role_val = USB_ROLE_DEVICE; > > + pref = > HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SNK; > > + } > > + > > + ret = hd3ss3220_set_source_pref(hd3ss3220, pref); > > + usleep_range(10, 100); > > + > > + usb_role_switch_set_role(hd3ss3220->role_sw, role_val); > > + typec_set_data_role(hd3ss3220->port, role); > > + > > + return ret; > > +} > > + > > +static void hd3ss3220_set_role(struct hd3ss3220 *hd3ss3220) { > > + enum usb_role role_state = > hd3ss3220_get_attached_state(hd3ss3220); > > + > > + usb_role_switch_set_role(hd3ss3220->role_sw, role_state); > > + if (role_state == USB_ROLE_NONE) > > + hd3ss3220_set_source_pref(hd3ss3220, > > + > HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_DEFAULT); > > + > > + switch (role_state) { > > + case USB_ROLE_HOST: > > + typec_set_data_role(hd3ss3220->port, TYPEC_HOST); > > + break; > > + case USB_ROLE_DEVICE: > > + typec_set_data_role(hd3ss3220->port, TYPEC_DEVICE); > > + break; > > + default: > > + break; > > + } > > +} > > + > > +irqreturn_t hd3ss3220_irq(struct hd3ss3220 *hd3ss3220) { > > + int err; > > + unsigned int data; > > + > > + hd3ss3220_set_role(hd3ss3220); > > + > > + err = regmap_read(hd3ss3220->regmap, > HD3SS3220_REG_CN_STAT_CTRL, &data); > > + if (err < 0) > > + return IRQ_NONE; > > + > > + err = regmap_write(hd3ss3220->regmap, > HD3SS3220_REG_CN_STAT_CTRL, > > + data | > HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS); > > + if (err < 0) > > + return IRQ_NONE; > use regmap_xxx_bits() if possible? OK , yes as an optimization, in this case I need to use "regmap_update_bits_base" api with force update to clear the interrupt status bit. I will send V4 with this changes, Please let me know, if you think otherwise. The new code will look like err = regmap_update_bits_base(hd3ss3220->regmap, HD3SS3220_REG_CN_STAT_CTRL, HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS, HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS, NULL, false, true); Regards, Biju
