lkml.org 
[lkml]   [2018]   [Feb]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] staging: typec: handle vendor defined part and modify drp toggling flow
    On Wed, Feb 21, 2018 at 11:02:23PM +0800, ShuFanLee wrote:
    > From: ShuFanLee <shufan_lee@richtek.com>
    >
    > 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.
    > DRP toggling flow is chagned as following:
    > - Write DRP = 0 & Rd/Rd
    > - Write DRP = 1
    > - Set LOOK4CONNECTION command
    >
    > Signed-off-by: ShuFanLee <shufan_lee@richtek.com>

    Mostly loooks good to me. Couple of nitpicks below.

    Guenter

    > ---
    > 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);
    > + 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;

    regmap_write returns a negative return code or 0, thus this can be
    simplified to
    return regmap_write(...);

    > }
    >
    > 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);
    > +}

    Hmm, normally I'd expect this function _after_ the function it calls.
    Guess that doesn't matter much here, so I am fine with it as long
    as Greg is ok with it as well.

    > +
    > +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);
    >
    > 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
    >

    \
     
     \ /
      Last update: 2018-02-21 23:16    [W:4.601 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site