Re: [PATCH v6] staging: typec: handle vendor defined part and modify drp toggling flow

[李書帆]

Hi,

2018-03-05 22:49 GMT+08:00 Guenter Roeck :
> On 03/04/2018 08:16 PM, ShuFan Lee wrote:
>>
>> From: ShuFan Lee 
>>
>> Handle vendor defined behavior in tcpci_init, tcpci_set_vconn,
>> tcpci_start_drp_toggling
>> 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:
>
>
> s/chagned/changed/
Sorry for the type error, I'll correct it in v7.
>
>
>>- Write DRP = 1, Rp level and RC.CCx to Rd/Rd or Rp/Rp
>>- Set LOOK4CONNECTION command
>>
>> Signed-off-by: ShuFan Lee 
>> ---
>>   drivers/staging/typec/tcpci.c | 134
>> ++
>>   drivers/staging/typec/tcpci.h |  15 +
>>   2 files changed, 123 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
>>
>>   patch changelogs between v3 & v4
>>   - Directly return regmap_write at the end of drp_toggling function
>>   - Because tcpci_irq is called in _tcpci_irq, move _tcpci_irq to the
>> place after tcpci_irq
>>
>>   patch changelogs between v4 & v5
>>   - Add a space between my first & last name, from ShuFanLee to ShuFan
>> Lee.
>>
>>   patch changelogs between v5 & v6
>>   - Add start_drp_toggling in tcpci_data and call it at the beginning of
>> tcpci_start_drp_toggling
>>   - Modify the flow of tcpci_start_drp_toggling as following
>>  - Set Rp level, RC.CCx to Rd/Rd or Rp/Rp and DRP = 1
>>  - Set LOOK4CONNECTION command
>>
>> diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
>> index 9bd4412..9e600f7 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,9 +102,19 @@ 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;
>>   + if (tcpci->data) {
>> +   if (tcpci->data->start_drp_toggling) {
>
>
> From the code flow it is guaranteed that ->data is set. It should therefore
> be unnecessary to check for it (we don't check if ->reg is set either).
Ok, I'll modify it in v7, thank you.
>
>
>> +   ret = tcpci->data->start_drp_toggling(tcpci,
>> + tcpci->data,
>> cc);
>> +   if (ret < 0)
>> +   return ret;
>> +   }
>> +   }
>> +
>> switch (cc) {
>> default:
>> case TYPEC_CC_RP_DEF:
>> @@ -117,7 +131,17 @@ static int tcpci_start_drp_toggling(struct tcpc_dev
>> *tcpc,
>> break;
>> }
>>   - return regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
>> +   if (cc == TYPEC_CC_RD)
>> +   reg |= (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC1_SHIFT)
>> |
>> +  (TCPC_ROLE_CTRL_CC_RD <<
>> TCPC_ROLE_CTRL_CC2_SHIFT);
>> +   else
>> +   reg |= (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC1_SHIFT)
>> |
>> +  (TCPC_ROLE_CTRL_CC_RP <<
>> TCPC_ROLE_CTRL_CC2_SHIFT);
>> +   ret = regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
>> +   if (ret < 0)
>> +   return 

Re: [PATCH v6] staging: typec: handle vendor defined part and modify drp toggling flow

[Guenter Roeck]

On 03/04/2018 08:16 PM, ShuFan Lee wrote:

From: ShuFan Lee 

Handle vendor defined behavior in tcpci_init, tcpci_set_vconn, 
tcpci_start_drp_toggling
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:


s/chagned/changed/


   - Write DRP = 1, Rp level and RC.CCx to Rd/Rd or Rp/Rp
   - Set LOOK4CONNECTION command

Signed-off-by: ShuFan Lee 
---
  drivers/staging/typec/tcpci.c | 134 ++
  drivers/staging/typec/tcpci.h |  15 +
  2 files changed, 123 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

  patch changelogs between v3 & v4
  - Directly return regmap_write at the end of drp_toggling function
  - Because tcpci_irq is called in _tcpci_irq, move _tcpci_irq to the place 
after tcpci_irq

  patch changelogs between v4 & v5
  - Add a space between my first & last name, from ShuFanLee to ShuFan Lee.

  patch changelogs between v5 & v6
  - Add start_drp_toggling in tcpci_data and call it at the beginning of 
tcpci_start_drp_toggling
  - Modify the flow of tcpci_start_drp_toggling as following
 - Set Rp level, RC.CCx to Rd/Rd or Rp/Rp and DRP = 1
 - Set LOOK4CONNECTION command

diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
index 9bd4412..9e600f7 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,9 +102,19 @@ 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;
  
+	if (tcpci->data) {

+   if (tcpci->data->start_drp_toggling) {


From the code flow it is guaranteed that ->data is set. It should therefore
be unnecessary to check for it (we don't check if ->reg is set either).


+   ret = tcpci->data->start_drp_toggling(tcpci,
+ tcpci->data, cc);
+   if (ret < 0)
+   return ret;
+   }
+   }
+
switch (cc) {
default:
case TYPEC_CC_RP_DEF:
@@ -117,7 +131,17 @@ static int tcpci_start_drp_toggling(struct tcpc_dev *tcpc,
break;
}
  
-	return regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);

+   if (cc == TYPEC_CC_RD)
+   reg |= (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC1_SHIFT) |
+  (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC2_SHIFT);
+   else
+   reg |= (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC1_SHIFT) |
+  (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC2_SHIFT);
+   ret = regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
+   if (ret < 0)
+   return ret;
+   return regmap_write(tcpci->regmap, TCPC_COMMAND,
+   TCPC_CMD_LOOK4CONNECTION);
  }
  
  static enum typec_cc_status tcpci_to_typec_cc(unsigned int cc, bool sink)

@@ -178,6 +202,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 =