Hi, > -----Original Message----- > From: Guenter Roeck [mailto:groeck@xxxxxxxxxx] > Sent: Saturday, October 01, 2016 2:45 AM > To: Jun Li <jun.li@xxxxxxx> > Cc: Guenter Roeck <groeck@xxxxxxxxxxxx>; Felipe Balbi > <felipe.balbi@xxxxxxxxxxxxxxx>; Chandra Sekhar Anagani > <chandra.sekhar.anagani@xxxxxxxxx>; Bruce Ashfield > <bruce.ashfield@xxxxxxxxxxxxx>; Bin Gao <bin.gao@xxxxxxxxx>; Pranav Tipnis > <pranav.tipnis@xxxxxxxxx>; Heikki Krogerus > <heikki.krogerus@xxxxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; linux- > usb@xxxxxxxxxxxxxxx > Subject: Re: [RFC PATCH v3 2/2] usb: typec: Type-C Port Controller > Interface driver (tcpci) > > On Thu, Sep 29, 2016 at 11:24 PM, Jun Li <jun.li@xxxxxxx> wrote: > > Hi Guenter, > > > >> -----Original Message----- > >> From: linux-usb-owner@xxxxxxxxxxxxxxx [mailto:linux-usb- > >> owner@xxxxxxxxxxxxxxx] On Behalf Of Guenter Roeck > >> Sent: Wednesday, August 24, 2016 5:11 AM > >> To: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx> > >> Cc: Chandra Sekhar Anagani <chandra.sekhar.anagani@xxxxxxxxx>; Bruce > >> Ashfield <bruce.ashfield@xxxxxxxxxxxxx>; Bin Gao <bin.gao@xxxxxxxxx>; > >> Pranav Tipnis <pranav.tipnis@xxxxxxxxx>; Heikki Krogerus > >> <heikki.krogerus@xxxxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; > >> linux- usb@xxxxxxxxxxxxxxx; Guenter Roeck <groeck@xxxxxxxxxxxx> > >> Subject: [RFC PATCH v3 2/2] usb: typec: Type-C Port Controller > >> Interface driver (tcpci) > >> > >> The port controller interface driver interconnects the Type-C Port > >> Manager with a Type-C Port Controller Interface (TCPCI) compliant port > controller. > >> > >> Signed-off-by: Guenter Roeck <groeck@xxxxxxxxxxxx> > >> --- > >> v3: > >> - No change > >> > >> v2: > >> - Adjust to modified callbacks into tcpm code > >> > >> drivers/usb/typec/Kconfig | 9 + > >> drivers/usb/typec/Makefile | 1 + > >> drivers/usb/typec/tcpci.c | 487 > >> +++++++++++++++++++++++++++++++++++++++++++++ > >> drivers/usb/typec/tcpci.h | 133 +++++++++++++ > >> 4 files changed, 630 insertions(+) > >> create mode 100644 drivers/usb/typec/tcpci.c create mode 100644 > >> drivers/usb/typec/tcpci.h > >> > >> diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig > >> index > >> 113bb1b3589c..a92c9d1a3e00 100644 > >> --- a/drivers/usb/typec/Kconfig > >> +++ b/drivers/usb/typec/Kconfig > >> @@ -25,4 +25,13 @@ config TYPEC_TCPM > >> The Type-C Port Controller Manager provides a USB PD and USB > Type-C > >> state machine for use with Type-C Port Controllers. > >> > >> +if TYPEC_TCPM > >> + > >> +config TYPEC_TCPCI > >> + tristate "Type-C Port Controller Interface driver" > >> + help > >> + Type-C Port Controller driver for TCPCI-compliant controller. > >> + > >> +endif > >> + > >> endmenu > >> diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile > >> index bbe45721cf52..7dbaf8c3911d 100644 > >> --- a/drivers/usb/typec/Makefile > >> +++ b/drivers/usb/typec/Makefile > >> @@ -1,3 +1,4 @@ > >> obj-$(CONFIG_TYPEC) += typec.o > >> obj-$(CONFIG_TYPEC_WCOVE) += typec_wcove.o > >> obj-$(CONFIG_TYPEC_TCPM) += tcpm.o > >> +obj-$(CONFIG_TYPEC_TCPCI) += tcpci.o > >> diff --git a/drivers/usb/typec/tcpci.c b/drivers/usb/typec/tcpci.c > >> new file mode 100644 index 000000000000..af338218a1f3 > >> --- /dev/null > >> +++ b/drivers/usb/typec/tcpci.c > >> @@ -0,0 +1,487 @@ > >> +/* > >> + * Copyright 2015-2016 Google, Inc > >> + * > >> + * This program is free software; you can redistribute it and/or > >> +modify > >> + * it under the terms of the GNU General Public License as published > >> +by > >> + * the Free Software Foundation; either version 2 of the License, or > >> + * (at your option) any later version. > >> + * > >> + * This program is distributed in the hope that it will be useful, > >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of > >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > >> + * GNU General Public License for more details. > >> + * > >> + * USB Type-C Port Controller Interface. > >> + */ > >> + > >> +#include <linux/delay.h> > >> +#include <linux/kernel.h> > >> +#include <linux/module.h> > >> +#include <linux/i2c.h> > >> +#include <linux/interrupt.h> > >> +#include <linux/regmap.h> > >> +#include <linux/usb/pd.h> > >> +#include <linux/usb/typec.h> > >> + > >> +#include "tcpci.h" > >> +#include "tcpm.h" > >> + > >> +#define PD_RETRY_COUNT 3 > >> + > >> +struct tcpci { > >> + struct device *dev; > >> + struct i2c_client *client; > >> + > >> + struct tcpm_port *port; > >> + > >> + struct regmap *regmap; > >> + > >> + bool controls_vbus; > >> + > >> + struct tcpc_dev tcpc; > >> +}; > >> + > >> +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, > >> + unsigned int *val) { > >> + return regmap_raw_read(tcpci->regmap, reg, val, sizeof(u16)); } > >> + > >> +static int tcpci_write16(struct tcpci *tcpci, unsigned int reg, u16 > >> +val) { > >> + return regmap_raw_write(tcpci->regmap, reg, &val, sizeof(u16)); > >> +} > >> + > >> +static int tcpci_set_cc(struct tcpc_dev *tcpc, enum typec_cc_status > >> +cc) { > >> + struct tcpci *tcpci = tcpc_to_tcpci(tcpc); > >> + unsigned int reg; > >> + int ret; > >> + > >> + switch (cc) { > >> + case TYPEC_CC_RA: > >> + reg = (TCPC_ROLE_CTRL_CC_RA << TCPC_ROLE_CTRL_CC1_SHIFT) > | > >> + (TCPC_ROLE_CTRL_CC_RA << > TCPC_ROLE_CTRL_CC2_SHIFT); > >> + break; > >> + case TYPEC_CC_RD: > >> + reg = (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC1_SHIFT) > | > >> + (TCPC_ROLE_CTRL_CC_RD << > TCPC_ROLE_CTRL_CC2_SHIFT); > >> + break; > >> + case TYPEC_CC_RP_DEF: > >> + reg = (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC1_SHIFT) > | > >> + (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC2_SHIFT) > | > >> + (TCPC_ROLE_CTRL_RP_VAL_DEF << > >> + TCPC_ROLE_CTRL_RP_VAL_SHIFT); > >> + break; > >> + case TYPEC_CC_RP_1_5: > >> + reg = (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC1_SHIFT) > | > >> + (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC2_SHIFT) > | > >> + (TCPC_ROLE_CTRL_RP_VAL_1_5 << > >> + TCPC_ROLE_CTRL_RP_VAL_SHIFT); > >> + break; > >> + case TYPEC_CC_RP_3_0: > >> + reg = (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC1_SHIFT) > | > >> + (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC2_SHIFT) > | > >> + (TCPC_ROLE_CTRL_RP_VAL_1_5 << > >> + TCPC_ROLE_CTRL_RP_VAL_SHIFT); > >> + break; > >> + case TYPEC_CC_OPEN: > >> + default: > >> + reg = (TCPC_ROLE_CTRL_CC_OPEN << TCPC_ROLE_CTRL_CC1_SHIFT) > | > >> + (TCPC_ROLE_CTRL_CC_OPEN << > TCPC_ROLE_CTRL_CC2_SHIFT); > >> + break; > >> + } > >> + > >> + ret = regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg); > >> + if (ret < 0) > >> + return ret; > >> + > >> + return 0; > >> +} > >> + > >> +static enum typec_cc_status tcpci_to_typec_cc(unsigned int cc, bool > >> +sink) { > >> + switch (cc) { > >> + case 0x1: > >> + return sink ? TYPEC_CC_RP_DEF : TYPEC_CC_RA; > >> + case 0x2: > >> + return sink ? TYPEC_CC_RP_1_5 : TYPEC_CC_RD; > >> + case 0x3: > >> + if (sink) > >> + return TYPEC_CC_RP_3_0; > >> + case 0x0: > >> + default: > >> + return TYPEC_CC_OPEN; > >> + } > >> +} > >> + > >> +static int tcpci_get_cc(struct tcpc_dev *tcpc, > >> + enum typec_cc_status *cc1, enum typec_cc_status > *cc2) { > >> + struct tcpci *tcpci = tcpc_to_tcpci(tcpc); > >> + unsigned int reg; > >> + int ret; > >> + > >> + ret = regmap_read(tcpci->regmap, TCPC_CC_STATUS, ®); > >> + if (ret < 0) > >> + return ret; > >> + > >> + *cc1 = tcpci_to_typec_cc((reg >> TCPC_CC_STATUS_CC1_SHIFT) & > >> + TCPC_CC_STATUS_CC1_MASK, > >> + reg & TCPC_CC_STATUS_TERM); > >> + *cc2 = tcpci_to_typec_cc((reg >> TCPC_CC_STATUS_CC2_SHIFT) & > >> + TCPC_CC_STATUS_CC2_MASK, > >> + reg & TCPC_CC_STATUS_TERM); > >> + > >> + return 0; > >> +} > >> + > >> +static int tcpci_set_polarity(struct tcpc_dev *tcpc, > >> + enum typec_cc_polarity polarity) { > >> + struct tcpci *tcpci = tcpc_to_tcpci(tcpc); > >> + int ret; > >> + > >> + ret = regmap_write(tcpci->regmap, TCPC_TCPC_CTRL, > >> + (polarity == TYPEC_POLARITY_CC2) ? > >> + TCPC_TCPC_CTRL_ORIENTATION : 0); > >> + if (ret < 0) > >> + return ret; > >> + > >> + return 0; > >> +} > >> + > >> +static int tcpci_set_vconn(struct tcpc_dev *tcpc, bool enable) { > >> + struct tcpci *tcpci = tcpc_to_tcpci(tcpc); > >> + int ret; > >> + > >> + ret = regmap_write(tcpci->regmap, TCPC_POWER_CTRL, > >> + enable ? TCPC_POWER_CTRL_VCONN_ENABLE : 0); > >> + if (ret < 0) > >> + return ret; > >> + > >> + return 0; > >> +} > >> + > >> +static int tcpci_set_pd_header(struct tcpc_dev *tcpc, enum > >> +typec_role > >> role, > >> + enum typec_data_role data) { > >> + struct tcpci *tcpci = tcpc_to_tcpci(tcpc); > >> + unsigned int reg; > >> + int ret; > >> + > >> + reg = PD_REV20 << TCPC_MSG_HDR_INFO_REV_SHIFT; > >> + if (role == TYPEC_SOURCE) > >> + reg |= TCPC_MSG_HDR_INFO_PWR_ROLE; > >> + if (data == TYPEC_HOST) > >> + reg |= TCPC_MSG_HDR_INFO_DATA_ROLE; > >> + ret = regmap_write(tcpci->regmap, TCPC_MSG_HDR_INFO, reg); > >> + if (ret < 0) > >> + return ret; > >> + > >> + return 0; > >> +} > >> + > >> +static int tcpci_set_pd_rx(struct tcpc_dev *tcpc, bool enable) { > >> + struct tcpci *tcpci = tcpc_to_tcpci(tcpc); > >> + unsigned int reg = 0; > >> + int ret; > >> + > >> + if (enable) > >> + reg = TCPC_RX_DETECT_SOP | TCPC_RX_DETECT_HARD_RESET; > >> + ret = regmap_write(tcpci->regmap, TCPC_RX_DETECT, reg); > >> + if (ret < 0) > >> + return ret; > >> + > >> + return 0; > >> +} > >> + > >> +static int tcpci_get_vbus(struct tcpc_dev *tcpc) { > >> + struct tcpci *tcpci = tcpc_to_tcpci(tcpc); > >> + unsigned int reg; > >> + int ret; > >> + > >> + ret = regmap_read(tcpci->regmap, TCPC_POWER_STATUS, ®); > >> + if (ret < 0) > >> + return ret; > >> + > >> + return !!(reg & TCPC_POWER_STATUS_VBUS_PRES); } > >> + > >> +static int tcpci_set_vbus(struct tcpc_dev *tcpc, bool source, bool > >> +sink) { > >> + struct tcpci *tcpci = tcpc_to_tcpci(tcpc); > >> + int ret; > >> + > >> + if (source) { > >> + ret = regmap_write(tcpci->regmap, TCPC_COMMAND, > >> + TCPC_CMD_SRC_VBUS_DEFAULT); > >> + if (ret < 0) > >> + return ret; > >> + } else { > >> + ret = regmap_write(tcpci->regmap, TCPC_COMMAND, > >> + TCPC_CMD_DISABLE_SRC_VBUS); > >> + if (ret < 0) > >> + return ret; > >> + } > >> + > >> + if (sink) { > >> + ret = regmap_write(tcpci->regmap, TCPC_COMMAND, > >> + TCPC_CMD_SINK_VBUS); > >> + if (ret < 0) > >> + return ret; > >> + } else { > >> + ret = regmap_write(tcpci->regmap, TCPC_COMMAND, > >> + TCPC_CMD_DISABLE_SINK_VBUS); > >> + if (ret < 0) > >> + return ret; > >> + } > >> + > > > > Per tcpc spec, disable should be firstly done before enable, so should > > it be done like this? > > if(!source) {}; > > if(!sink) {}; > > if(source) {}; > > if(sink) {}; > > > Makes sense. Changed accordingly. > > >> + return 0; > >> +} > >> + > >> +static int tcpci_pd_transmit(struct tcpc_dev *tcpc, > >> + enum tcpm_transmit_type type, > >> + const struct pd_message *msg) { > >> + struct tcpci *tcpci = tcpc_to_tcpci(tcpc); > >> + unsigned int reg, cnt, header; > >> + int ret; > >> + > >> + cnt = msg ? pd_header_cnt(msg->header) * 4 : 0; > >> + ret = regmap_write(tcpci->regmap, TCPC_TX_BYTE_CNT, cnt); > > > > Here miss the 2 bytes' header, so cnt should be cnt += 2; > > > > Tcpc 4.4.13 > > TRANSMIT_BYTE_COUNT: > > The number of bytes the TCPM will write This is the number of bytes in > > the TX_BUFFER_DATA_OBJECTS plus two (for the TX_BUF_HEADER) > > > Yes, you are correct. Fixed. > > >> + if (ret < 0) > >> + return ret; > >> + > >> + header = msg ? msg->header : 0; > >> + ret = tcpci_write16(tcpci, TCPC_TX_HDR, header); > >> + if (ret < 0) > >> + return ret; > >> + > >> + if (cnt > 0) { > >> + ret = regmap_raw_write(tcpci->regmap, TCPC_TX_DATA, > >> + &msg->payload, cnt); > >> + if (ret < 0) > >> + return ret; > >> + } > >> + > >> + reg = (PD_RETRY_COUNT << TCPC_TRANSMIT_RETRY_SHIFT) | > >> + (type << TCPC_TRANSMIT_TYPE_SHIFT); > >> + ret = regmap_write(tcpci->regmap, TCPC_TRANSMIT, reg); > >> + if (ret < 0) > >> + return ret; > >> + > >> + return 0; > >> +} > >> + > > > > ... > > > >> + > >> +static int tcpci_probe(struct i2c_client *client, > >> + const struct i2c_device_id *i2c_id) { > >> + struct tcpci *tcpci; > >> + int err; > >> + > >> + tcpci = devm_kzalloc(&client->dev, sizeof(*tcpci), GFP_KERNEL); > >> + if (!tcpci) > >> + return -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->tcpc.init = tcpci_init; > >> + tcpci->tcpc.get_vbus = tcpci_get_vbus; > >> + tcpci->tcpc.set_vbus = tcpci_set_vbus; > >> + tcpci->tcpc.set_cc = tcpci_set_cc; > >> + tcpci->tcpc.get_cc = tcpci_get_cc; > >> + tcpci->tcpc.set_polarity = tcpci_set_polarity; > >> + tcpci->tcpc.set_vconn = tcpci_set_vconn; > >> + > >> + tcpci->tcpc.set_pd_rx = tcpci_set_pd_rx; > >> + tcpci->tcpc.set_pd_header = tcpci_set_pd_header; > >> + tcpci->tcpc.pd_transmit = tcpci_pd_transmit; > >> + > >> + err = tcpci_parse_config(tcpci); > >> + if (err < 0) > >> + return err; > >> + > > > > As the alert mask reg is defaultly to be unmasked after power on, It's > > better clear and mask all before request irq. > > > Agreed and done. > > >> + err = devm_request_threaded_irq(tcpci->dev, client->irq, NULL, > >> + tcpci_irq, > >> + IRQF_TRIGGER_FALLING, > > > > IRQF_ONESHOT is required, and do we need the irq trigger to be > > IRQF_TRIGGER_LOW? > > > > Good question; I don't really know. IRQF_TRIGGER_FALLING definitely seems > to be wrong (and I don't recall where I got it from). Not sure about > IRQF_TRIGGER_LOW. Can you test on real hardware, by any chance ? > I tested on real hardware, IRQF_TRIGGER_LOW can work fine, so I think the right flag should be IRQF_TRIGGER_LOW | IRQF_ONESHOT. Li Jun > Thanks, > Guenter > > > > Li Jun ��.n��������+%������w��{.n�����{���)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
