All of lore.kernel.org
 help / color / mirror / Atom feed
* staging: typec: handle vendor defined part and modify drp toggling flow
  2018-02-21 22:15   ` [PATCH] " Guenter Roeck
@ 2018-02-28  2:14 ` shufan_lee(李書帆)
  -1 siblings, 0 replies; 24+ messages in thread
From: shufan_lee(李書帆) @ 2018-02-28  2:14 UTC (permalink / raw)
  To: Guenter Roeck, ShuFanLee
  Cc: heikki.krogerus@linux.intel.com, greg@kroah.com,
	cy_huang(黃啟原), linux-kernel@vger.kernel.org,
	linux-usb@vger.kernel.org

Hi Guenter,

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

  Ok, I'll modify it in v4

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.

  Do you mean it maybe better to call vendor's set_vconn after normal set_vconn is finished?
  If I understand correctly, I can also modify it in v4. For RT1711H, it also works by enabling/disabling idle mode after set_vconn.

Best Regards,
*****************************************
Shu-Fan Lee
Richtek Technology Corporation
TEL: +886-3-5526789 #2359
FAX: +886-3-5526612
*****************************************

________________________________________
寄件者: Guenter Roeck <groeck7@gmail.com> 代表 Guenter Roeck <linux@roeck-us.net>
寄件日期: 2018年2月22日 上午 06:15
收件者: ShuFanLee
副本: heikki.krogerus@linux.intel.com; greg@kroah.com; shufan_lee(李書帆); cy_huang(黃啟原); linux-kernel@vger.kernel.org; linux-usb@vger.kernel.org
主旨: Re: [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
>
************* Email Confidentiality Notice ********************

The information contained in this e-mail message (including any attachments) may be confidential, proprietary, privileged, or otherwise exempt from disclosure under applicable laws. It is intended to be conveyed only to the designated recipient(s). Any use, dissemination, distribution, printing, retaining or copying of this e-mail (including its attachments) by unintended recipient(s) is strictly prohibited and may be unlawful. If you are not an intended recipient of this e-mail, or believe that you have received this e-mail in error, please notify the sender immediately (by replying to this e-mail), delete any and all copies of this e-mail (including any attachments) from your system, and do not disclose the content of this e-mail to any other person. Thank you!

^ permalink raw reply	[flat|nested] 24+ messages in thread
* staging: typec: handle vendor defined part and modify drp toggling flow
  2018-03-02 18:39               ` 李書帆
@ 2018-03-02 18:43 ` 李書帆
  -1 siblings, 0 replies; 24+ messages in thread
From: ShuFanLee @ 2018-03-02 18:43 UTC (permalink / raw)
  To: Jun Li
  Cc: shufan_lee(李書帆),
	heikki.krogerus@linux.intel.com, linux@roeck-us.net,
	greg@kroah.com, cy_huang(黃啟原),
	linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org,
	dl-linux-imx

Hi,

  Sorry, I modify it to plain text mode and send again

2018-03-03 2:39 GMT+08:00 李書帆 <leechu729@gmail.com>:
>
> Hi Jun,
>
>   I think this operation should be moved to vendor's operation after rechecking the specification.
>
> 2018-03-02 22:38 GMT+08:00 Jun Li <jun.li@nxp.com>:
>>
>> Hi
>> > -----Original Message-----
>> > From: shufan_lee(李書帆) [mailto:shufan_lee@richtek.com]
>> > Sent: 2018年3月1日 19:54
>> > To: Jun Li <jun.li@nxp.com>; ShuFanLee <leechu729@gmail.com>;
>> > heikki.krogerus@linux.intel.com; linux@roeck-us.net; greg@kroah.com
>> > Cc: cy_huang(黃啟原) <cy_huang@richtek.com>;
>> > linux-kernel@vger.kernel.org; linux-usb@vger.kernel.org; dl-linux-imx
>> > <linux-imx@nxp.com>
>> > Subject: RE: [PATCH] staging: typec: handle vendor defined part and modify
>> > drp toggling flow
>> >
>> > Hi Jun,
>> >
>> > > -----Original Message-----
>> > > From: Jun Li [mailto:jun.li@nxp.com]
>> > > Sent: Thursday, March 01, 2018 6:06 PM
>> > > To: shufan_lee(李書帆); ShuFanLee; heikki.krogerus@linux.intel.com;
>> > > linux@roeck-us.net; greg@kroah.com
>> > > Cc: cy_huang(黃啟原); linux-kernel@vger.kernel.org;
>> > > linux-usb@vger.kernel.org; dl-linux-imx
>> > > Subject: RE: [PATCH] staging: typec: handle vendor defined part and
>> > > modify drp toggling flow
>> > >
>> > > Hi Shufan
>> > >
>> > > Please don't top posting
>> > >
>> > > -----Original Message-----
>> > > From: shufan_lee(李書帆) [mailto:shufan_lee@richtek.com]
>> > > Sent: 2018年3月1日 16:49
>> > > To: Jun Li <jun.li@nxp.com>; ShuFanLee <leechu729@gmail.com>;
>> > > heikki.krogerus@linux.intel.com; linux@roeck-us.net; greg@kroah.com
>> > > Cc: cy_huang(黃啟原) <cy_huang@richtek.com>;
>> > > linux-kernel@vger.kernel.org; linux-usb@vger.kernel.org; dl-linux-imx
>> > > <linux-imx@nxp.com>
>> > > Subject: RE: [PATCH] staging: typec: handle vendor defined part and
>> > > modify drp toggling flow
>> > >
>> > > Hi Jun,
>> > >
>> > >   The attachment is waveform of the condition we met but I'm not sure
>> > > whether you can download the attachment.
>> > >   I add log in RT1711H it shows as following:
>> > >
>> > > You don't need add log by your own.
>> > > There is tcpm(./drivers/usb/typec/tcpm.c) log for debug already, which can
>> > show all the events and state transitions, you can get it by below command
>> > as I commented:
>> > >
>> > > cat /sys/kernel/debug/tcpm/xxxxx(your tcpc i2c device)
>> > >
>> > After applying your patch[2], TCPM log is as following:
>>
>> I assume you also applied my patch [1].
>> [1] https://www.spinics.net/lists/devicetree/msg216851.html
>>
> Yes, this patch is also applied.
>>
>> >
>> > [   53.050602] CC1: 0 -> 2, CC2: 0 -> 0 [state DRP_TOGGLING, polarity 0,
>> > connected]
>> > [   53.050613] state change DRP_TOGGLING -> SRC_ATTACH_WAIT
>> > [   53.050678] pending state change SRC_ATTACH_WAIT -> SNK_TRY @
>> > 200 ms
>> > => Rd is plugged out
>> > [   53.178804] CC1: 2 -> 0, CC2: 0 -> 0 [state SRC_ATTACH_WAIT, polarity 0,
>> > disconnected]
>> > [   53.178815] state change SRC_ATTACH_WAIT -> SRC_UNATTACHED
>> > => Rd is plugged in
>> > [   53.178874] Start DRP toggling
>> > [   53.188472] CC1: 0 -> 0, CC2: 0 -> 0 [state DRP_TOGGLING, polarity 0,
>> > disconnected]
>>
>> 1. The plug out and then plug in happens in 10ms? (53.188472 - 53.178804)
>> Was this done manually? Or by some special test method?
>
> It's done manually. If you can download the waveform attached in previous email, you can see Rd is plugged out/in within ms level.
> We connect a bridge board with test points of CC1, CC2 and GND to our platform's Typec receptacle. Then we can simulate a device plugging in/out by connecting/disconnecting a 5.1k resistor between CC1 and GND.
>
>>
>> 2. This is all tcpm log if you finally keep the Rd-device connected on typec
>> port? There is no more tcpm log after 53.188472 if you plug in Rd-device
>>
>> and don't remove it?
>
> Yes, RT1711H reports open/open to TCPM in drp_toggling state and stops toggling. Currently, TCPM does not restart drp_toggling if it receives open/open in drp_toggling state.
> I remember the specification of TypeC does not depict what TCPM should do if it receives open/open in drp_toggling state.
> Maybe restart toggling is an option.
>
>> 3. If the answer of Q2 is yes, then I must ask why you tcpc chip+internal firmware
>> can't report further cc change event after your drp toggling starts to present Rp(I know
>> it firstly present Rd after you write to LOOK4CONNECTION, but then it should change
>> to present Rp, so it should be able to detect the Rd-device finally)
>
> Because RT1711H's internal firmware changes to attached state when Rd is re-plugged in(cc level is default Rp (around 400mV) now).
> Then, LOOK4CONNECTION is set and RT1711H changes CCx to Rd that makes it reports open/open(because cc level changes from default Rp(around 400mV) to 0mV)
>>
>>
>> >
>> > If TCPM does not enter SRC_ATTACHED state, RC.DRP will not be cleared.
>>
>> In this case, you don’t need clear RC.DRP, see TCPCI spec:
>> "Figure 4-18. Sink Disconnect"
>> TCPM sink doesn't clear it in whole sequence, just directly set it:
>> Restart DRP Toggling
>> PC.AutoDischargeDisconnect=0b
>> Set RC.DRP=1b (DRP)
>> Set RC.CC1=10b (Rd)
>> Set RC.CC2=10b (Rd)
>> COMMAND.Look4Connection (DRP toggle)
>>
>>
>> > When TCPM writes Rd/Rd or Rp/Rp in the drp_toggling function, it does not
>> > take effect until LOOK4CONNECTION command is set.
>> > The above condition causes RT1711H reports open/open at [53.188472]
>> >
>> > > [ 914.937340] typec_rt1711h 2-004e: __rt1711h_irq_handler [
>> > > 914.943838] typec_rt1711h 2-004e: __tcpm_get_cc cc1 = Open, cc2 =
>> > Open
>> > > => Device(Rd) is plugged out
>> > >
>> > > [ 914.958041] typec_rt1711h 2-004e: tcpm_set_pd_rx 0 [ 914.963011]
>> > > typec_rt1711h 2-004e: tcpm_set_vbus vbus = 0 [ 914.968407]
>> > > typec_rt1711h
>> > > 2-004e: tcpm_set_vbus chg is already 0 [ 914.974541] typec_rt1711h
>> > 2-004e:
>> > > tcpm_set_vconn vconn is already 0 [ 914.980921] typec_rt1711h 2-004e:
>> > > tcpm_set_current_limit 0 ma, 0 mv (not implemented) [ 914.988894]
>> > > typec_rt1711h 2-004e: tcpm_set_polarity Polarity_CC1 [ 915.003201]
>> > > typec_rt1711h 2-004e: tcpm_set_roles Source Host [ 915.009264]
>> > > typec_rt1711h 2-004e: tcpm_start_drp_toggling => state_machine_work
>> > of
>> > > TCPM calls start_drp_toggling function but does not set
>> > > LOOK4CONNECTION command yet => (Note1) Device(Rd) is plugged in
>> > > (RT1711H's internal firmware detects Rd plugged in. cc_change is
>> > > triggered and it will be vRd-connected at this moment) => TCPM writes
>> > > LOOK4CONNECTION command
>> > > - Because RC.DRP is still 1, RT1711H will not pull cc1/cc2 to Rd while
>> > > writing Rd/Rd to RC.CC1/RC.CC2.
>> > > - (Note2) Right after LOOK4CONNECTION command is written, RT1711H
>> > > pulls CC to Rd's level (because RC.Role is Rd/Rd), stop toggling
>> > > (because
>> > > device(Rd) is already connected) and CC status will be open/open now
>> > > (because RT1711H presents Rd and device is connected(Rd))
>> > >
>> > > [ 915.037263] typec_rt1711h 2-004e: __tcpm_get_cc cc1 = Open, cc2 =
>> > > Open => Enter RT1711H's irq handler and it reports open/open
>> > >
>> > > I think the point is to write RC.DRP = 0 at the beginning of
>> > > drp_toggling so that RT1711H will pull cc1/cc2 to Rd while writing
>> > > Rd/Rd to RC.CC1/RC.CC2 This operation will make RT1711H's internal
>> > > firmware restarts from disconnected state and toggles correctly.
>> > >
>> > > According to TCPCI spec (4.4.5.2):
>> > > It is recommended the TCPM write ROLE_CONTROL.DRP=0 before writing
>> > to
>> > > POWER_CONTROL.AutoDischargeDisconnect and starting the DRP toggling
>> > > using COMMAND.Look4Connection Restart DRP Toggling => It is
>> > > recommended the TCPM write ROLE_CONTROL.DRP=0 here Set
>> > >
>> > > This statement is_not_ recommend you do this(RC.DRP=0) for start drp
>> > toggling, Please carefully check the whole sentence:
>> > > "... as shown in Figure 4-16, "
>> > > If you look at "Figure 4-16. DRP Initialization and Connection Detection"
>> > > It gives clear drp toggling start operations:
>> > >
>> > > Set TCPC to DRP
>> > > - Read PS.TCPCInitializationStatus
>> > > - Write ROLE_CONTROL
>> > > - RC.DRP = 1b
>> > > - RC.CC2=01b or 10b
>> > > - RC.CC1=01b or 10b
>> > > - RC.CC1=RC.CC2
>> > > - Write COMMAND.Look4ConnectionPS.
>> > >
>> > > Above is all operations required to start drp toggling. You also can see
>> > RC.CCx = 01b or 10b, not fixed to be Rd, right?
>> > Yes, this one should be like your patch[07/12]. Write Rd or Rp to RC.CCx
>> > according to the cc parameter of drp_toggling function.
>> > >
>> > > Go on to check the Figure 4-16
>> > > After debounce, we need do following:
>> > >
>> > > ConnectionDetermine CC & VCONN
>> > > - Write RC.CC1 & RC.CC2 per decision
>> > > - Write RC.DRP=0
>> > > - Write TCPC_CONTROl.PlugOrientation
>> > > - Write PC.AutoDischargeDisconnect=1
>> > >  & PC.EnableVconnConnection
>> > >
>> > > Current existing tcpm+tcpci will not clear RC.DRP after attach, That means
>> > RC.DRP clear may be done after attach, not in start_drp_toggling.
>> > > I am not sure if this can resolve your problem, but I think it deserve a try,
>> > you can follow above operation sequence while entering attach state, refer
>> > to my patch[2]:
>> > >
>> > > [2]
>> > >
>> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fw
>> > ww
>> > > .spinics.net%2Flists%2Fdevicetree%2Fmsg216852.html&data=02%7C01%7
>> > Cjun.
>> > >
>> > li%40nxp.com%7C9117425550d24ddc86d808d57f6b1b4e%7C686ea1d3bc2b
>> > 4c6fa92c
>> > >
>> > d99c5c301635%7C0%7C0%7C636555020366483456&sdata=9%2BywYl%2BR
>> > PYtk60Wg6p
>> > > R63cCW2AnRXs%2BrINvvqUpqL18%3D&reserved=0
>> > >
>> > > diff --git a/drivers/staging/typec/tcpci.c
>> > > b/drivers/staging/typec/tcpci.c index 530a5d7..7145771 100644
>> > > --- a/drivers/staging/typec/tcpci.c
>> > > +++ b/drivers/staging/typec/tcpci.c
>> > > @@ -184,6 +184,7 @@ static int tcpci_set_polarity(struct tcpc_dev *tcpc,
>> > >                               enum typec_cc_polarity polarity)  {
>> > >         struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
>> > > +       unsigned int reg;
>> > >         int ret;
>> > >
>> > >         ret = regmap_write(tcpci->regmap, TCPC_TCPC_CTRL, @@
>> > -192,6 +193,20 @@ static int tcpci_set_polarity(struct tcpc_dev *tcpc,
>> > >         if (ret < 0)
>> > >                 return ret;
>> > >
>> > > +       ret = regmap_read(tcpci->regmap, TCPC_ROLE_CTRL, &reg);
>> > > +       if (ret < 0)
>> > > +               return ret;
>> > > +
>> > > +       if (polarity == TYPEC_POLARITY_CC2)
>> > > +               ret = TCPC_ROLE_CTRL_CC1_SHIFT;
>> > > +       else
>> > > +               ret = TCPC_ROLE_CTRL_CC2_SHIFT;
>> > > +       reg |= TCPC_ROLE_CTRL_CC_OPEN << ret;
>> > > +       reg &= ~TCPC_ROLE_CTRL_DRP;
>> > > +       ret = regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
>> > > +       if (ret < 0)
>> > > +               return ret;
>> > > +
>> > >         return 0;
>> > >  }
>> > >
>> > > PC.AutoDischargeDisconnect=0b Set RC.DRP=1b (DRP) Set RC.RpValue=00b
>> > > (smallest Rp to save power) Set RC.CC1=01b Set
>> > > RC.CC2=01bCOMMAND.Look4ConnectionNo
>> > >
>> > > It seems like it is not a general case here (because it is only
>> > > recommended but not necessary), we can move it to vendor_ops in the
>> > next patch.
>> > >
>> > > The TCPM should be able to cover all cases, and we should follow the
>> > recommended sequence(if what you are trying to do is really as spec says).
>> > Agree!
>> > My understanding is that we need to set RC.DRP to 0 every time before
>> > TCPM restarts toggling but not just for attached.
>>
>> Actually I have no objection on adding a RC.DRP clear, the question is
>> where is the right place to do this, till now I don’t see how this DRP bit keep set
>> while start drp toggling is causing problem, You want to start present Rd after write
>> CC1/CC2 to be Rd/Rd and before write to LOOK4CONNECTION, this is not required
>> per tcpci spec.
>
> According to TCPCI's specification, it is recommended the TCPM write RC.DRP = 0 before changing AutoDischargeDisconnecd.
> Maybe, an interface for controlling AutoDischargeDisconnecd can be added. Then clear RC.DRP in the beginning of that interface.
>>
>>
>> -Behavior after your patch:
>> //begin with Open as RC.DRP = 1
>> RC.CCx=Rd & RC.DRP = 0; //Start present Rd
>> Wait 1ms; // Still Rd
>> RC.CCx=Rd & RC.DRP = 1; //Open?
>> Look4CONNECTION = 1; //DRP toggling continue preset Rd
>>
>> Is my above CC state description correct? Is this what you want?
>
> Correct. But the purpose of setting RC.CCx = Rd & RC.DRP = 0 is to make RT1711H's internal firmware leave attached state.
> Because RT1711H is still presenting Rp after the device is plugged out, it's internal firmware enters attached state when the device re-plugged in.
> When we write RC.CCx = Rd & RC.DRP = 0, RT1711H's internal firmware leaves attached state. So it will keep toggling from Rd to Rp but not report open/open when LOOK4CONNECTION is set.
> After rechecking the specification of TCPCI, I think TCPC's internal firmware should start toggling from detached state when LOOK4CONNECTION is set even if RC.DRP is not cleared before
> I've discussed with my colleagues, we think this should be in vendor ops. I'll update it in the patch v6.
> Could I also apply your patch [1] in the patch v6?
> Thank you for your time to clarify this condition.
>>
>>
>> -Only apply my patches:
>> //begin with Open as RC.DRP = 1
>> RC.CCx=Rd & RC.DRP = 1; //still Open
>> Look4Connection = 1; //Start preset Rd
>>
>> > For RT1711H, it follows above flow. If it is not correct, this operation should
>> > be moved to vendor_ops.
>> > >
>> > > For your question:
>> > > Why RT1711H reports open/open after drp_toggling is enabled?
>> > > => See Note2 above.
>> > > This open/open is for you plug out the device, right?
>> > > => No, see Note2 above.
>> > > Why RT1711H can't report new cc change events after you plug in the
>> > > device?
>> > > => RT1711H can generate new cc change events after plugging in the
>> > device.
>> > > What cc change event tcpc will report on step 4?
>> > > => See Note1 above
>> > > Did you verify your change can pass the typec compliance test?
>> > > => We didn't test it yet but try to make all functions work correctly first.
>> > >
>> > > Best Regards,
>> > > *****************************
>> > > Shu-Fan Lee
>> > > Richtek Technology Corporation
>> > > TEL: +886-3-5526789 #2359
>> > > FAX: +886-3-5526612
>> > > *****************************
>> > >
>> >
>> > ************* Email Confidentiality Notice ********************
>> >
>> > The information contained in this e-mail message (including any attachments)
>> > may be confidential, proprietary, privileged, or otherwise exempt from
>> > disclosure under applicable laws. It is intended to be conveyed only to the
>> > designated recipient(s). Any use, dissemination, distribution, printing,
>> > retaining or copying of this e-mail (including its attachments) by unintended
>> > recipient(s) is strictly prohibited and may be unlawful. If you are not an
>> > intended recipient of this e-mail, or believe that you have received this
>> > e-mail in error, please notify the sender immediately (by replying to this
>> > e-mail), delete any and all copies of this e-mail (including any attachments)
>> > from your system, and do not disclose the content of this e-mail to any other
>> > person. Thank you!
>
>
>
>
> --
> Best Regards,
> 書帆

^ permalink raw reply	[flat|nested] 24+ messages in thread
* staging: typec: handle vendor defined part and modify drp toggling flow
@ 2018-02-21 15:02 ` ShuFanLee
  0 siblings, 0 replies; 24+ messages in thread
From: ShuFanLee @ 2018-02-21 15:02 UTC (permalink / raw)
  To: heikki.krogerus, linux, greg
  Cc: shufan_lee, cy_huang, linux-kernel, linux-usb

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>
---
 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;
 }
 
 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);
 
 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 */

^ permalink raw reply related	[flat|nested] 24+ messages in thread
* staging: typec: handle vendor defined part and modify drp toggling flow
@ 2018-02-14 10:56 Heikki Krogerus
  0 siblings, 0 replies; 24+ messages in thread
From: Heikki Krogerus @ 2018-02-14 10:56 UTC (permalink / raw)
  To: ShuFanLee; +Cc: linux, linux-kernel, linux-usb, cy_huang, shufan_lee

On Wed, Feb 14, 2018 at 05:24:04PM +0800, ShuFanLee wrote:
> From: ShuFanLee <shufan_lee@richtek.com>
> 
> Handle vendor defined behavior in tcpci_init and tcpci_irq.
> More operations can be extended in tcpci_vendor_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>
> ---
>  drivers/staging/typec/tcpci.c | 98 ++++++++++++++++++++++++++++++++++++-------
>  drivers/staging/typec/tcpci.h | 15 +++++++
>  2 files changed, 97 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
> index 9bd4412..b3a97b3 100644
> --- a/drivers/staging/typec/tcpci.c
> +++ b/drivers/staging/typec/tcpci.c
> @@ -30,6 +30,7 @@ struct tcpci {
>  	bool controls_vbus;
>  
>  	struct tcpc_dev tcpc;
> +	struct tcpci_vendor_data *vdata;
>  };
>  
>  static inline struct tcpci *tcpc_to_tcpci(struct tcpc_dev *tcpc)
> @@ -37,16 +38,29 @@ 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)
> +int tcpci_read16(struct tcpci *tcpci, unsigned int reg, u16 *val)
>  {
>  	return regmap_raw_read(tcpci->regmap, reg, val, sizeof(u16));
>  }
> +EXPORT_SYMBOL_GPL(tcpci_read16);
>  
> -static int tcpci_write16(struct tcpci *tcpci, unsigned int reg, u16 val)
> +int tcpci_write16(struct tcpci *tcpci, unsigned int reg, u16 val)
>  {
>  	return regmap_raw_write(tcpci->regmap, reg, &val, sizeof(u16));
>  }
> +EXPORT_SYMBOL_GPL(tcpci_write16);
> +
> +int tcpci_read8(struct tcpci *tcpci, unsigned int reg, u8 *val)
> +{
> +	return regmap_raw_read(tcpci->regmap, reg, val, sizeof(u8));
> +}
> +EXPORT_SYMBOL_GPL(tcpci_read8);
> +
> +int tcpci_write8(struct tcpci *tcpci, unsigned int reg, u8 val)
> +{
> +	return regmap_raw_write(tcpci->regmap, reg, &val, sizeof(u8));
> +}
> +EXPORT_SYMBOL_GPL(tcpci_write8);

I don't think there is any need to export those wrappers. You can
always expect the glue driver to supply the regmap as member of that
struct tcpci_vendor_data. See below..

>  static int tcpci_set_cc(struct tcpc_dev *tcpc, enum typec_cc_status cc)
>  {
> @@ -98,8 +112,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:
> @@ -116,8 +132,19 @@ static int tcpci_start_drp_toggling(struct tcpc_dev *tcpc,
>  			TCPC_ROLE_CTRL_RP_VAL_SHIFT);
>  		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;
>  }
>  
>  static enum typec_cc_status tcpci_to_typec_cc(unsigned int cc, bool sink)
> @@ -323,6 +350,15 @@ static int tcpci_init(struct tcpc_dev *tcpc)
>  	if (time_after(jiffies, timeout))
>  		return -ETIMEDOUT;
>  
> +	/* Handle vendor init */
> +	if (tcpci->vdata) {
> +		if (tcpci->vdata->init) {
> +			ret = (*tcpci->vdata->init)(tcpci, tcpci->vdata);

ret = tcpci->vdata->init(...

> +			if (ret < 0)
> +				return ret;
> +		}
> +	}
> +
>  	/* Clear all events */
>  	ret = tcpci_write16(tcpci, TCPC_ALERT, 0xffff);
>  	if (ret < 0)
> @@ -351,6 +387,13 @@ static irqreturn_t tcpci_irq(int irq, void *dev_id)
>  
>  	tcpci_read16(tcpci, TCPC_ALERT, &status);
>  
> +	/* Handle vendor defined interrupt */
> +	if (tcpci->vdata) {
> +		if (tcpci->vdata->irq_handler)
> +			(*tcpci->vdata->irq_handler)(tcpci, tcpci->vdata,
> +						     &status);

Ditto.

> +	}
> +
>  	/*
>  	 * Clear alert status for everything except RX_STATUS, which shouldn't
>  	 * be cleared until we have successfully retrieved message.
> @@ -417,7 +460,7 @@ static irqreturn_t tcpci_irq(int irq, void *dev_id)
>  	.reg_bits = 8,
>  	.val_bits = 8,
>  
> -	.max_register = 0x7F, /* 0x80 .. 0xFF are vendor defined */
> +	.max_register = 0xFF, /* 0x80 .. 0xFF are vendor defined */
>  };
>  
>  static const struct tcpc_config tcpci_tcpc_config = {
> @@ -435,22 +478,22 @@ 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 i2c_client *client,
> +				  struct tcpci_vendor_data *vdata)

struct tcpci *tcpci_register_port(struct device *dev, struct tcpci_...)
{
...

>  {
>  	struct tcpci *tcpci;
>  	int err;
>  
>  	tcpci = devm_kzalloc(&client->dev, sizeof(*tcpci), GFP_KERNEL);
>  	if (!tcpci)
> -		return -ENOMEM;
> +		return ERR_PTR(-ENOMEM);
>  
>  	tcpci->client = client;

struct tcpci does not need that "client" member.

>  	tcpci->dev = &client->dev;

tcpci->dev = dev;

> -	i2c_set_clientdata(client, tcpci);
> +	tcpci->vdata = vdata;
>  	tcpci->regmap = devm_regmap_init_i2c(client, &tcpci_regmap_config);

You would now do that in your glue driver, and here just:

tcpci->regmap = data->regmap;

>  	tcpci->tcpc.init = tcpci_init;
>  	tcpci->tcpc.get_vbus = tcpci_get_vbus;
> @@ -467,7 +510,7 @@ static int tcpci_probe(struct i2c_client *client,
>  
>  	err = tcpci_parse_config(tcpci);
>  	if (err < 0)
> -		return err;
> +		return ERR_PTR(err);
>  
>  	/* Disable chip interrupts */
>  	tcpci_write16(tcpci, TCPC_ALERT_MASK, 0);
> @@ -477,17 +520,40 @@ static int tcpci_probe(struct i2c_client *client,
>  					IRQF_ONESHOT | IRQF_TRIGGER_LOW,
>  					dev_name(tcpci->dev), tcpci);

The interrupt could also be requested in the glue driver. Then from
the irq routine in your glue driver, you call tcpci_irq(). That would
mean you export tcpci_irq() of course.

>  	if (err < 0)
> -		return err;
> +		return ERR_PTR(err);
>  
>  	tcpci->port = tcpm_register_port(tcpci->dev, &tcpci->tcpc);
> -	return PTR_ERR_OR_ZERO(tcpci->port);
> +	if (PTR_ERR_OR_ZERO(tcpci->port))
> +		return ERR_CAST(tcpci->port);
> +
> +	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);
> +
> +static int tcpci_probe(struct i2c_client *client,
> +		       const struct i2c_device_id *i2c_id)
> +{
> +	struct tcpci *tcpci;
> +
> +	tcpci = tcpci_register_port(client, NULL);
> +	if (PTR_ERR_OR_ZERO(tcpci))
> +		return PTR_ERR(tcpci);
> +
> +	i2c_set_clientdata(client, tcpci);
> +	return 0;
>  }

That would then look something like this:

static int tcpci_probe(struct i2c_client *client...
{
        struct tcpci_data data; /* s/tcpci_vendor_data/tcpci_data/ */
        struct tcpci *tcpci;

        data.regmap = devm_regmap_init_i2c(client, &tcpci_regmap_config);
        if (IS_ERR(tcpci->regmap))
                return PTR_ERR(tcpci->regmap);

        /*
         * NOTE: everything in struct tcpci_data should be copied in
         * tcpci_register_port().
         */
        tcpci = tcpci_register_port(&client->dev, &data);
        if (IS_ERR(tcpci))
                return PTR_ERR(tcpci);

        devm_request_threaded_irq(&client->dev, client->irq, NULL,
                                  tcpci_irq,
                                  ...

        i2c_set_clientdata(client, tcpci);

        return 0;
}


Br,

^ permalink raw reply	[flat|nested] 24+ messages in thread
* staging: typec: handle vendor defined part and modify drp toggling flow
@ 2018-02-14  9:24 ShuFanLee
  0 siblings, 0 replies; 24+ messages in thread
From: ShuFanLee @ 2018-02-14  9:24 UTC (permalink / raw)
  To: heikki.krogerus, linux; +Cc: linux-kernel, linux-usb, cy_huang, shufan_lee

From: ShuFanLee <shufan_lee@richtek.com>

Handle vendor defined behavior in tcpci_init and tcpci_irq.
More operations can be extended in tcpci_vendor_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>
---
 drivers/staging/typec/tcpci.c | 98 ++++++++++++++++++++++++++++++++++++-------
 drivers/staging/typec/tcpci.h | 15 +++++++
 2 files changed, 97 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
index 9bd4412..b3a97b3 100644
--- a/drivers/staging/typec/tcpci.c
+++ b/drivers/staging/typec/tcpci.c
@@ -30,6 +30,7 @@ struct tcpci {
 	bool controls_vbus;
 
 	struct tcpc_dev tcpc;
+	struct tcpci_vendor_data *vdata;
 };
 
 static inline struct tcpci *tcpc_to_tcpci(struct tcpc_dev *tcpc)
@@ -37,16 +38,29 @@ 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)
+int tcpci_read16(struct tcpci *tcpci, unsigned int reg, u16 *val)
 {
 	return regmap_raw_read(tcpci->regmap, reg, val, sizeof(u16));
 }
+EXPORT_SYMBOL_GPL(tcpci_read16);
 
-static int tcpci_write16(struct tcpci *tcpci, unsigned int reg, u16 val)
+int tcpci_write16(struct tcpci *tcpci, unsigned int reg, u16 val)
 {
 	return regmap_raw_write(tcpci->regmap, reg, &val, sizeof(u16));
 }
+EXPORT_SYMBOL_GPL(tcpci_write16);
+
+int tcpci_read8(struct tcpci *tcpci, unsigned int reg, u8 *val)
+{
+	return regmap_raw_read(tcpci->regmap, reg, val, sizeof(u8));
+}
+EXPORT_SYMBOL_GPL(tcpci_read8);
+
+int tcpci_write8(struct tcpci *tcpci, unsigned int reg, u8 val)
+{
+	return regmap_raw_write(tcpci->regmap, reg, &val, sizeof(u8));
+}
+EXPORT_SYMBOL_GPL(tcpci_write8);
 
 static int tcpci_set_cc(struct tcpc_dev *tcpc, enum typec_cc_status cc)
 {
@@ -98,8 +112,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:
@@ -116,8 +132,19 @@ static int tcpci_start_drp_toggling(struct tcpc_dev *tcpc,
 			TCPC_ROLE_CTRL_RP_VAL_SHIFT);
 		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;
 }
 
 static enum typec_cc_status tcpci_to_typec_cc(unsigned int cc, bool sink)
@@ -323,6 +350,15 @@ static int tcpci_init(struct tcpc_dev *tcpc)
 	if (time_after(jiffies, timeout))
 		return -ETIMEDOUT;
 
+	/* Handle vendor init */
+	if (tcpci->vdata) {
+		if (tcpci->vdata->init) {
+			ret = (*tcpci->vdata->init)(tcpci, tcpci->vdata);
+			if (ret < 0)
+				return ret;
+		}
+	}
+
 	/* Clear all events */
 	ret = tcpci_write16(tcpci, TCPC_ALERT, 0xffff);
 	if (ret < 0)
@@ -351,6 +387,13 @@ static irqreturn_t tcpci_irq(int irq, void *dev_id)
 
 	tcpci_read16(tcpci, TCPC_ALERT, &status);
 
+	/* Handle vendor defined interrupt */
+	if (tcpci->vdata) {
+		if (tcpci->vdata->irq_handler)
+			(*tcpci->vdata->irq_handler)(tcpci, tcpci->vdata,
+						     &status);
+	}
+
 	/*
 	 * Clear alert status for everything except RX_STATUS, which shouldn't
 	 * be cleared until we have successfully retrieved message.
@@ -417,7 +460,7 @@ static irqreturn_t tcpci_irq(int irq, void *dev_id)
 	.reg_bits = 8,
 	.val_bits = 8,
 
-	.max_register = 0x7F, /* 0x80 .. 0xFF are vendor defined */
+	.max_register = 0xFF, /* 0x80 .. 0xFF are vendor defined */
 };
 
 static const struct tcpc_config tcpci_tcpc_config = {
@@ -435,22 +478,22 @@ 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 i2c_client *client,
+				  struct tcpci_vendor_data *vdata)
 {
 	struct tcpci *tcpci;
 	int err;
 
 	tcpci = devm_kzalloc(&client->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->vdata = vdata;
 	tcpci->regmap = devm_regmap_init_i2c(client, &tcpci_regmap_config);
 	if (IS_ERR(tcpci->regmap))
-		return PTR_ERR(tcpci->regmap);
+		return ERR_CAST(tcpci->regmap);
 
 	tcpci->tcpc.init = tcpci_init;
 	tcpci->tcpc.get_vbus = tcpci_get_vbus;
@@ -467,7 +510,7 @@ static int tcpci_probe(struct i2c_client *client,
 
 	err = tcpci_parse_config(tcpci);
 	if (err < 0)
-		return err;
+		return ERR_PTR(err);
 
 	/* Disable chip interrupts */
 	tcpci_write16(tcpci, TCPC_ALERT_MASK, 0);
@@ -477,17 +520,40 @@ static int tcpci_probe(struct i2c_client *client,
 					IRQF_ONESHOT | IRQF_TRIGGER_LOW,
 					dev_name(tcpci->dev), tcpci);
 	if (err < 0)
-		return err;
+		return ERR_PTR(err);
 
 	tcpci->port = tcpm_register_port(tcpci->dev, &tcpci->tcpc);
-	return PTR_ERR_OR_ZERO(tcpci->port);
+	if (PTR_ERR_OR_ZERO(tcpci->port))
+		return ERR_CAST(tcpci->port);
+
+	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);
+
+static int tcpci_probe(struct i2c_client *client,
+		       const struct i2c_device_id *i2c_id)
+{
+	struct tcpci *tcpci;
+
+	tcpci = tcpci_register_port(client, NULL);
+	if (PTR_ERR_OR_ZERO(tcpci))
+		return PTR_ERR(tcpci);
+
+	i2c_set_clientdata(client, tcpci);
+	return 0;
 }
 
 static int tcpci_remove(struct i2c_client *client)
 {
 	struct tcpci *tcpci = i2c_get_clientdata(client);
 
-	tcpm_unregister_port(tcpci->port);
+	tcpci_unregister_port(tcpci);
 
 	return 0;
 }
diff --git a/drivers/staging/typec/tcpci.h b/drivers/staging/typec/tcpci.h
index fdfb06c..b1fcb4a 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,18 @@
 #define TCPC_VBUS_VOLTAGE_ALARM_HI_CFG		0x76
 #define TCPC_VBUS_VOLTAGE_ALARM_LO_CFG		0x78
 
+struct tcpci;
+struct tcpci_vendor_data {
+	int (*init)(struct tcpci *tcpci, struct tcpci_vendor_data *data);
+	int (*irq_handler)(struct tcpci *tcpci, struct tcpci_vendor_data *data,
+			   u16 *status);
+};
+
+int tcpci_read16(struct tcpci *tcpci, unsigned int reg, u16 *val);
+int tcpci_write16(struct tcpci *tcpci, unsigned int reg, u16 val);
+int tcpci_read8(struct tcpci *tcpci, unsigned int reg, u8 *val);
+int tcpci_write8(struct tcpci *tcpci, unsigned int reg, u8 val);
+struct tcpci *tcpci_register_port(struct i2c_client *client,
+				  struct tcpci_vendor_data *data);
+void tcpci_unregister_port(struct tcpci *tcpci);
 #endif /* __LINUX_USB_TCPCI_H */

^ permalink raw reply related	[flat|nested] 24+ messages in thread
end of thread, other threads:[~2018-03-02 18:43 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-02-28  2:14 staging: typec: handle vendor defined part and modify drp toggling flow shufan_lee(李書帆)
2018-02-28  2:14 ` 回覆: [PATCH] " shufan_lee(李書帆)
  -- strict thread matches above, loose matches on Subject: below --
2018-03-02 18:43 ShuFanLee
2018-03-02 18:43 ` [PATCH] " 李書帆
2018-02-21 15:02 ShuFanLee
2018-02-21 15:02 ` [PATCH] " ShuFanLee
2018-02-21 22:15 ` Guenter Roeck
2018-02-21 22:15   ` [PATCH] " Guenter Roeck
2018-02-28  2:09   ` 李書帆
2018-02-22 10:16 ` Jun Li
2018-02-22 10:16   ` [PATCH] " Jun Li
2018-02-28  3:40   ` shufan_lee(李書帆)
2018-02-28  3:40     ` 回覆: [PATCH] " shufan_lee(李書帆)
2018-03-01  5:34     ` Jun Li
2018-03-01  8:49       ` shufan_lee(李��帆)
2018-03-01 10:06         ` Jun Li
2018-03-01 10:06           ` [PATCH] " Jun Li
2018-03-01 11:53           ` shufan_lee(李書帆)
2018-03-01 11:53             ` [PATCH] " shufan_lee(李書帆)
2018-03-02 14:38             ` Jun Li
2018-03-02 14:38               ` [PATCH] " Jun Li
2018-03-02 18:39               ` 李書帆
2018-02-14 10:56 Heikki Krogerus
2018-02-14  9:24 ShuFanLee

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.