Hi, > -----Original Message----- > From: linux-usb-owner@xxxxxxxxxxxxxxx > [mailto:linux-usb-owner@xxxxxxxxxxxxxxx] On Behalf Of ShuFanLee > Sent: 2018年2月21日 23:02 > To: heikki.krogerus@xxxxxxxxxxxxxxx; linux@xxxxxxxxxxxx; greg@xxxxxxxxx > Cc: shufan_lee@xxxxxxxxxxx; cy_huang@xxxxxxxxxxx; > linux-kernel@xxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx > Subject: [PATCH] staging: typec: handle vendor defined part and modify drp > toggling flow > > From: ShuFanLee <shufan_lee@xxxxxxxxxxx> > > Handle vendor defined behavior in tcpci_init, tcpci_set_vconn and export > tcpci_irq. > More operations can be extended in tcpci_data if needed. > According to TCPCI specification, 4.4.5.2 ROLE_CONTROL, TCPC shall not > start DRP toggling until subsequently the TCPM writes to the COMMAND > register to start DRP toggling. My understanding of above statement is TCPM(this Linux driver) can enable DRP and CC1/CC2 in one shot, but TCPC(your typec chip internal firmware) needs wait until TCPM writes to COMMAND register to start drp toggling. > DRP toggling flow is chagned as following: > - Write DRP = 0 & Rd/Rd Why fixed to be Rd/Rd? in this case, it means the starting value: Tcpci 4.4.5.2: "When initiating autonomous DRP toggling, the TCPM shall write B6 (DRP) =1b and the starting value of Rp/Rd to B3..0 (CC1/CC2) to indicate DRP autonomous toggling mode to the TCPC." > - Write DRP = 1 What's your problem if combine above 2 in one shot? > - Set LOOK4CONNECTION command > > Signed-off-by: ShuFanLee <shufan_lee@xxxxxxxxxxx> > --- > drivers/staging/typec/tcpci.c | 128 > +++++++++++++++++++++++++++++++++--------- > drivers/staging/typec/tcpci.h | 13 +++++ > 2 files changed, 115 insertions(+), 26 deletions(-) > > patch changelogs between v1 & v2 > - Remove unnecessary i2c_client in the structure of tcpci > - Rename structure of tcpci_vendor_data to tcpci_data > - Not exporting tcpci read/write wrappers but register i2c regmap in glue > driver > - Add set_vconn ops in tcpci_data > (It is necessary for RT1711H to enable/disable idle mode before > disabling/enabling vconn) > - Export tcpci_irq so that vendor can call it in their own IRQ handler > > patch changelogs between v2 & v3 > - Change the return type of tcpci_irq from int to irqreturn_t > > diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c > index 9bd4412..4959c69 100644 > --- a/drivers/staging/typec/tcpci.c > +++ b/drivers/staging/typec/tcpci.c > @@ -21,7 +21,6 @@ > > struct tcpci { > struct device *dev; > - struct i2c_client *client; > > struct tcpm_port *port; > > @@ -30,6 +29,12 @@ struct tcpci { > bool controls_vbus; > > struct tcpc_dev tcpc; > + struct tcpci_data *data; > +}; > + > +struct tcpci_chip { > + struct tcpci *tcpci; > + struct tcpci_data data; > }; > > static inline struct tcpci *tcpc_to_tcpci(struct tcpc_dev *tcpc) @@ -37,8 > +42,7 @@ static inline struct tcpci *tcpc_to_tcpci(struct tcpc_dev *tcpc) > return container_of(tcpc, struct tcpci, tcpc); } > > -static int tcpci_read16(struct tcpci *tcpci, unsigned int reg, > - u16 *val) > +static int tcpci_read16(struct tcpci *tcpci, unsigned int reg, u16 > +*val) > { > return regmap_raw_read(tcpci->regmap, reg, val, sizeof(u16)); } @@ > -98,8 +102,10 @@ static int tcpci_set_cc(struct tcpc_dev *tcpc, enum > typec_cc_status cc) static int tcpci_start_drp_toggling(struct tcpc_dev > *tcpc, > enum typec_cc_status cc) > { > + int ret; > struct tcpci *tcpci = tcpc_to_tcpci(tcpc); > - unsigned int reg = TCPC_ROLE_CTRL_DRP; > + unsigned int reg = (TCPC_ROLE_CTRL_CC_RD << > TCPC_ROLE_CTRL_CC1_SHIFT) | > + (TCPC_ROLE_CTRL_CC_RD << > TCPC_ROLE_CTRL_CC2_SHIFT); > > switch (cc) { > default: > @@ -117,7 +123,19 @@ static int tcpci_start_drp_toggling(struct tcpc_dev > *tcpc, > break; > } > > - return regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg); > + ret = regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg); > + if (ret < 0) > + return ret; > + usleep_range(500, 1000); Why need a wait here? It seems you actually don't want to do autonomously toggling from the very beginning, instead, you begin with a directly control on CC, keep it(Rd) for some time, then switch to use autonomously toggling, why you need such kind of sequence? I think it's special and not following tcpci spec. > + reg |= TCPC_ROLE_CTRL_DRP; > + ret = regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg); > + if (ret < 0) > + return ret; > + ret = regmap_write(tcpci->regmap, TCPC_COMMAND, > + TCPC_CMD_LOOK4CONNECTION); > + if (ret < 0) > + return ret; > + return 0; > } > > static enum typec_cc_status tcpci_to_typec_cc(unsigned int cc, bool sink) > @@ -178,6 +196,16 @@ static int tcpci_set_vconn(struct tcpc_dev *tcpc, > bool enable) > struct tcpci *tcpci = tcpc_to_tcpci(tcpc); > int ret; > > + /* Handle vendor set vconn */ > + if (tcpci->data) { > + if (tcpci->data->set_vconn) { > + ret = tcpci->data->set_vconn(tcpci, tcpci->data, > + enable); > + if (ret < 0) > + return ret; > + } > + } > + > ret = regmap_write(tcpci->regmap, TCPC_POWER_CTRL, > enable ? TCPC_POWER_CTRL_VCONN_ENABLE : 0); > if (ret < 0) > @@ -323,6 +351,15 @@ static int tcpci_init(struct tcpc_dev *tcpc) > if (time_after(jiffies, timeout)) > return -ETIMEDOUT; > > + /* Handle vendor init */ > + if (tcpci->data) { > + if (tcpci->data->init) { > + ret = tcpci->data->init(tcpci, tcpci->data); > + if (ret < 0) > + return ret; > + } > + } > + > /* Clear all events */ > ret = tcpci_write16(tcpci, TCPC_ALERT, 0xffff); > if (ret < 0) > @@ -344,9 +381,15 @@ static int tcpci_init(struct tcpc_dev *tcpc) > return tcpci_write16(tcpci, TCPC_ALERT_MASK, reg); } > > -static irqreturn_t tcpci_irq(int irq, void *dev_id) > +static irqreturn_t _tcpci_irq(int irq, void *dev_id) > { > struct tcpci *tcpci = dev_id; > + > + return tcpci_irq(tcpci); > +} > + > +irqreturn_t tcpci_irq(struct tcpci *tcpci) { > u16 status; > > tcpci_read16(tcpci, TCPC_ALERT, &status); @@ -412,6 +455,7 @@ > static irqreturn_t tcpci_irq(int irq, void *dev_id) > > return IRQ_HANDLED; > } > +EXPORT_SYMBOL_GPL(tcpci_irq); I don't catch the point of how you handle private events by above change, maybe post your RT1711H part as a user in one series can make it clear, I assume this can be done in existing tcpci_irq like above vender specific handling as well: tcpci->data->priv_events(tcpci, tcpci->data); Li Jun > > static const struct regmap_config tcpci_regmap_config = { > .reg_bits = 8, > @@ -435,22 +479,18 @@ static int tcpci_parse_config(struct tcpci *tcpci) > return 0; > } > > -static int tcpci_probe(struct i2c_client *client, > - const struct i2c_device_id *i2c_id) > +struct tcpci *tcpci_register_port(struct device *dev, struct tcpci_data > +*data) > { > struct tcpci *tcpci; > int err; > > - tcpci = devm_kzalloc(&client->dev, sizeof(*tcpci), GFP_KERNEL); > + tcpci = devm_kzalloc(dev, sizeof(*tcpci), GFP_KERNEL); > if (!tcpci) > - return -ENOMEM; > + return ERR_PTR(-ENOMEM); > > - tcpci->client = client; > - tcpci->dev = &client->dev; > - i2c_set_clientdata(client, tcpci); > - tcpci->regmap = devm_regmap_init_i2c(client, &tcpci_regmap_config); > - if (IS_ERR(tcpci->regmap)) > - return PTR_ERR(tcpci->regmap); > + tcpci->dev = dev; > + tcpci->data = data; > + tcpci->regmap = data->regmap; > > tcpci->tcpc.init = tcpci_init; > tcpci->tcpc.get_vbus = tcpci_get_vbus; @@ -467,27 +507,63 @@ static > int tcpci_probe(struct i2c_client *client, > > err = tcpci_parse_config(tcpci); > if (err < 0) > - return err; > + return ERR_PTR(err); > + > + tcpci->port = tcpm_register_port(tcpci->dev, &tcpci->tcpc); > + if (PTR_ERR_OR_ZERO(tcpci->port)) > + return ERR_CAST(tcpci->port); > > - /* Disable chip interrupts */ > - tcpci_write16(tcpci, TCPC_ALERT_MASK, 0); > + return tcpci; > +} > +EXPORT_SYMBOL_GPL(tcpci_register_port); > + > +void tcpci_unregister_port(struct tcpci *tcpci) { > + tcpm_unregister_port(tcpci->port); > +} > +EXPORT_SYMBOL_GPL(tcpci_unregister_port); > > - err = devm_request_threaded_irq(tcpci->dev, client->irq, NULL, > - tcpci_irq, > +static int tcpci_probe(struct i2c_client *client, > + const struct i2c_device_id *i2c_id) { > + struct tcpci_chip *chip; > + int err; > + u16 val = 0; > + > + chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL); > + if (!chip) > + return -ENOMEM; > + > + chip->data.regmap = devm_regmap_init_i2c(client, > &tcpci_regmap_config); > + if (IS_ERR(chip->data.regmap)) > + return PTR_ERR(chip->data.regmap); > + > + /* Disable chip interrupts before requesting irq */ > + err = regmap_raw_write(chip->data.regmap, TCPC_ALERT_MASK, &val, > + sizeof(u16)); > + if (err < 0) > + return err; > + > + err = devm_request_threaded_irq(&client->dev, client->irq, NULL, > + _tcpci_irq, > IRQF_ONESHOT | IRQF_TRIGGER_LOW, > - dev_name(tcpci->dev), tcpci); > + dev_name(&client->dev), chip); > if (err < 0) > return err; > > - tcpci->port = tcpm_register_port(tcpci->dev, &tcpci->tcpc); > - return PTR_ERR_OR_ZERO(tcpci->port); > + chip->tcpci = tcpci_register_port(&client->dev, &chip->data); > + if (PTR_ERR_OR_ZERO(chip->tcpci)) > + return PTR_ERR(chip->tcpci); > + > + i2c_set_clientdata(client, chip); > + return 0; > } > > static int tcpci_remove(struct i2c_client *client) { > - struct tcpci *tcpci = i2c_get_clientdata(client); > + struct tcpci_chip *chip = i2c_get_clientdata(client); > > - tcpm_unregister_port(tcpci->port); > + tcpci_unregister_port(chip->tcpci); > > return 0; > } > diff --git a/drivers/staging/typec/tcpci.h b/drivers/staging/typec/tcpci.h > index fdfb06c..40025b2 100644 > --- a/drivers/staging/typec/tcpci.h > +++ b/drivers/staging/typec/tcpci.h > @@ -59,6 +59,7 @@ > #define TCPC_POWER_CTRL_VCONN_ENABLE BIT(0) > > #define TCPC_CC_STATUS 0x1d > +#define TCPC_CC_STATUS_DRPRST BIT(5) > #define TCPC_CC_STATUS_TERM BIT(4) > #define TCPC_CC_STATUS_CC2_SHIFT 2 > #define TCPC_CC_STATUS_CC2_MASK 0x3 > @@ -121,4 +122,16 @@ > #define TCPC_VBUS_VOLTAGE_ALARM_HI_CFG 0x76 > #define TCPC_VBUS_VOLTAGE_ALARM_LO_CFG 0x78 > > +struct tcpci; > +struct tcpci_data { > + struct regmap *regmap; > + int (*init)(struct tcpci *tcpci, struct tcpci_data *data); > + int (*set_vconn)(struct tcpci *tcpci, struct tcpci_data *data, > + bool enable); > +}; > + > +struct tcpci *tcpci_register_port(struct device *dev, struct tcpci_data > +*data); void tcpci_unregister_port(struct tcpci *tcpci); irqreturn_t > +tcpci_irq(struct tcpci *tcpci); > + > #endif /* __LINUX_USB_TCPCI_H */ > -- > 1.9.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" in the > body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at > https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvger > .kernel.org%2Fmajordomo-info.html&data=02%7C01%7Cjun.li%40nxp.com > %7C44d11af27e0244fa4d1108d5793c3dc4%7C686ea1d3bc2b4c6fa92cd99c > 5c301635%7C0%7C1%7C636548222008905206&sdata=zn2ougKRvs%2BbmH > Qg2c8Rjj7vNe0ZiiVKdar7GdPa%2B3o%3D&reserved=0 ?韬{.n?????%??檩??w?{.n???{炳???骅w* jg????????G??⒏ ⒎?:+v???????? ????＂??????
