RE: [PATCH v7] staging: typec: handle vendor defined part and modify drp toggling flow

2018-03-12 Thread Jun Li 

> -Original Message-
> From: 李書帆 [mailto:leechu...@gmail.com]
> Sent: 2018年3月12日 14:57
> To: Jun Li <jun...@nxp.com>
> Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>;
> heikki.kroge...@linux.intel.com; li...@roeck-us.net; g...@kroah.com;
> shufan_...@richtek.com; cy_hu...@richtek.com;
> linux-kernel@vger.kernel.org; linux-...@vger.kernel.org
> Subject: Re: [PATCH v7] staging: typec: handle vendor defined part and modify
> drp toggling flow
> 
> Hi Jun,
> 
> 2018-03-12 13:58 GMT+08:00 Jun Li <jun...@nxp.com>:
> > Hi
> >> -Original Message-
> >> From: 李書帆 [mailto:leechu...@gmail.com]
> >> Sent: 2018年3月12日 13:22
> >> To: Jun Li <jun...@nxp.com>
> >> Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>;
> >> heikki.kroge...@linux.intel.com; li...@roeck-us.net; g...@kroah.com;
> >> shufan_...@richtek.com; cy_hu...@richtek.com;
> >> linux-kernel@vger.kernel.org; linux-...@vger.kernel.org
> >> Subject: Re: [PATCH v7] staging: typec: handle vendor defined part
> >> and modify drp toggling flow
> >>
> >> Hi Jun,
> >>
> >>   Thank you.
> >>
> >> 2018-03-12 12:33 GMT+08:00 Jun Li <jun...@nxp.com>:
> >> > Hi,
> >> >
> >> >> +static irqreturn_t _tcpci_irq(int irq, void *dev_id) {
> >> >> + struct tcpci *tcpci = dev_id;
> >> >> +
> >> >> + return tcpci_irq(tcpci);
> >> >> +}
> >> >>
> >> > ...
> >> >
> >> >> + err = devm_request_threaded_irq(>dev, client->irq, NULL,
> >> >> + _tcpci_irq,
> >> >>   IRQF_ONESHOT |
> >> IRQF_TRIGGER_LOW,
> >> >> - dev_name(tcpci->dev),
> tcpci);
> >> >> + dev_name(>dev),
> >> >> + chip);
> >> >
> >> > - dev_name(>dev), chip);
> >> > + dev_name(>dev), chip->tcpci);
> >> >
> >> > Did you ever test this patch?
> >> I've tested this patch with tcpci_rt1711h.c that will be sent out for
> >> reviewing in the next patch after tcpci's modification is passed.
> >> Because interrupt handler is registered in tcpci_rt1711h.c, here is
> >> the place I didn't notice.
> >
> > Understood.
> >
> >> The interrupt handler for tcpci.c should be modified as following:
> >>  static irqreturn_t _tcpci_irq(int irq, void *dev_id)  {
> >> -   struct tcpci *tcpci = dev_id;
> >> +   struct tcpci_chip *chip = dev_id;
> >>
> >> -   return tcpci_irq(tcpci);
> >> +   return tcpci_irq(chip->tcpci);
> >>  }
> >>
> >
> > Either way is OK to fix it.
> > You may send out your v8 and notify Greg to drop your v7 version.
> >
> > Jun Li
> 
> May I add you in the Reported-by list?

I just gave a run with your patch on my HW, so for your new version,
you can directly add:

Reviewed-by: Li Jun <jun...@nxp.com>
Tested-by: Li Jun <jun...@nxp.com>

> 
> --
> Best Regards,
> 書帆

RE: [PATCH v7] staging: typec: handle vendor defined part and modify drp toggling flow

2018-03-12 Thread Jun Li 

> -Original Message-
> From: 李書帆 [mailto:leechu...@gmail.com]
> Sent: 2018年3月12日 14:57
> To: Jun Li 
> Cc: Greg Kroah-Hartman ;
> heikki.kroge...@linux.intel.com; li...@roeck-us.net; g...@kroah.com;
> shufan_...@richtek.com; cy_hu...@richtek.com;
> linux-kernel@vger.kernel.org; linux-...@vger.kernel.org
> Subject: Re: [PATCH v7] staging: typec: handle vendor defined part and modify
> drp toggling flow
> 
> Hi Jun,
> 
> 2018-03-12 13:58 GMT+08:00 Jun Li :
> > Hi
> >> -Original Message-
> >> From: 李書帆 [mailto:leechu...@gmail.com]
> >> Sent: 2018年3月12日 13:22
> >> To: Jun Li 
> >> Cc: Greg Kroah-Hartman ;
> >> heikki.kroge...@linux.intel.com; li...@roeck-us.net; g...@kroah.com;
> >> shufan_...@richtek.com; cy_hu...@richtek.com;
> >> linux-kernel@vger.kernel.org; linux-...@vger.kernel.org
> >> Subject: Re: [PATCH v7] staging: typec: handle vendor defined part
> >> and modify drp toggling flow
> >>
> >> Hi Jun,
> >>
> >>   Thank you.
> >>
> >> 2018-03-12 12:33 GMT+08:00 Jun Li :
> >> > Hi,
> >> >
> >> >> +static irqreturn_t _tcpci_irq(int irq, void *dev_id) {
> >> >> + struct tcpci *tcpci = dev_id;
> >> >> +
> >> >> + return tcpci_irq(tcpci);
> >> >> +}
> >> >>
> >> > ...
> >> >
> >> >> + err = devm_request_threaded_irq(>dev, client->irq, NULL,
> >> >> + _tcpci_irq,
> >> >>   IRQF_ONESHOT |
> >> IRQF_TRIGGER_LOW,
> >> >> - dev_name(tcpci->dev),
> tcpci);
> >> >> + dev_name(>dev),
> >> >> + chip);
> >> >
> >> > - dev_name(>dev), chip);
> >> > + dev_name(>dev), chip->tcpci);
> >> >
> >> > Did you ever test this patch?
> >> I've tested this patch with tcpci_rt1711h.c that will be sent out for
> >> reviewing in the next patch after tcpci's modification is passed.
> >> Because interrupt handler is registered in tcpci_rt1711h.c, here is
> >> the place I didn't notice.
> >
> > Understood.
> >
> >> The interrupt handler for tcpci.c should be modified as following:
> >>  static irqreturn_t _tcpci_irq(int irq, void *dev_id)  {
> >> -   struct tcpci *tcpci = dev_id;
> >> +   struct tcpci_chip *chip = dev_id;
> >>
> >> -   return tcpci_irq(tcpci);
> >> +   return tcpci_irq(chip->tcpci);
> >>  }
> >>
> >
> > Either way is OK to fix it.
> > You may send out your v8 and notify Greg to drop your v7 version.
> >
> > Jun Li
> 
> May I add you in the Reported-by list?

I just gave a run with your patch on my HW, so for your new version,
you can directly add:

Reviewed-by: Li Jun 
Tested-by: Li Jun 

> 
> --
> Best Regards,
> 書帆

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

2018-03-12 Thread 李書帆 
Hi Jun,

2018-03-12 13:58 GMT+08:00 Jun Li <jun...@nxp.com>:
> Hi
>> -Original Message-
>> From: 李書帆 [mailto:leechu...@gmail.com]
>> Sent: 2018年3月12日 13:22
>> To: Jun Li <jun...@nxp.com>
>> Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>;
>> heikki.kroge...@linux.intel.com; li...@roeck-us.net; g...@kroah.com;
>> shufan_...@richtek.com; cy_hu...@richtek.com;
>> linux-kernel@vger.kernel.org; linux-...@vger.kernel.org
>> Subject: Re: [PATCH v7] staging: typec: handle vendor defined part and modify
>> drp toggling flow
>>
>> Hi Jun,
>>
>>   Thank you.
>>
>> 2018-03-12 12:33 GMT+08:00 Jun Li <jun...@nxp.com>:
>> > Hi,
>> >
>> >> +static irqreturn_t _tcpci_irq(int irq, void *dev_id) {
>> >> + struct tcpci *tcpci = dev_id;
>> >> +
>> >> + return tcpci_irq(tcpci);
>> >> +}
>> >>
>> > ...
>> >
>> >> + err = devm_request_threaded_irq(>dev, client->irq, NULL,
>> >> + _tcpci_irq,
>> >>   IRQF_ONESHOT |
>> IRQF_TRIGGER_LOW,
>> >> - dev_name(tcpci->dev), tcpci);
>> >> + dev_name(>dev), chip);
>> >
>> > - dev_name(>dev), chip);
>> > + dev_name(>dev), chip->tcpci);
>> >
>> > Did you ever test this patch?
>> I've tested this patch with tcpci_rt1711h.c that will be sent out for 
>> reviewing in
>> the next patch after tcpci's modification is passed.
>> Because interrupt handler is registered in tcpci_rt1711h.c, here is the 
>> place I
>> didn't notice.
>
> Understood.
>
>> The interrupt handler for tcpci.c should be modified as following:
>>  static irqreturn_t _tcpci_irq(int irq, void *dev_id)  {
>> -   struct tcpci *tcpci = dev_id;
>> +   struct tcpci_chip *chip = dev_id;
>>
>> -   return tcpci_irq(tcpci);
>> +   return tcpci_irq(chip->tcpci);
>>  }
>>
>
> Either way is OK to fix it.
> You may send out your v8 and notify Greg to drop your v7 version.
>
> Jun Li

May I add you in the Reported-by list?

-- 
Best Regards,
書帆

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

2018-03-12 Thread 李書帆 
Hi Jun,

2018-03-12 13:58 GMT+08:00 Jun Li :
> Hi
>> -Original Message-
>> From: 李書帆 [mailto:leechu...@gmail.com]
>> Sent: 2018年3月12日 13:22
>> To: Jun Li 
>> Cc: Greg Kroah-Hartman ;
>> heikki.kroge...@linux.intel.com; li...@roeck-us.net; g...@kroah.com;
>> shufan_...@richtek.com; cy_hu...@richtek.com;
>> linux-kernel@vger.kernel.org; linux-...@vger.kernel.org
>> Subject: Re: [PATCH v7] staging: typec: handle vendor defined part and modify
>> drp toggling flow
>>
>> Hi Jun,
>>
>>   Thank you.
>>
>> 2018-03-12 12:33 GMT+08:00 Jun Li :
>> > Hi,
>> >
>> >> +static irqreturn_t _tcpci_irq(int irq, void *dev_id) {
>> >> + struct tcpci *tcpci = dev_id;
>> >> +
>> >> + return tcpci_irq(tcpci);
>> >> +}
>> >>
>> > ...
>> >
>> >> + err = devm_request_threaded_irq(>dev, client->irq, NULL,
>> >> + _tcpci_irq,
>> >>   IRQF_ONESHOT |
>> IRQF_TRIGGER_LOW,
>> >> - dev_name(tcpci->dev), tcpci);
>> >> + dev_name(>dev), chip);
>> >
>> > - dev_name(>dev), chip);
>> > + dev_name(>dev), chip->tcpci);
>> >
>> > Did you ever test this patch?
>> I've tested this patch with tcpci_rt1711h.c that will be sent out for 
>> reviewing in
>> the next patch after tcpci's modification is passed.
>> Because interrupt handler is registered in tcpci_rt1711h.c, here is the 
>> place I
>> didn't notice.
>
> Understood.
>
>> The interrupt handler for tcpci.c should be modified as following:
>>  static irqreturn_t _tcpci_irq(int irq, void *dev_id)  {
>> -   struct tcpci *tcpci = dev_id;
>> +   struct tcpci_chip *chip = dev_id;
>>
>> -   return tcpci_irq(tcpci);
>> +   return tcpci_irq(chip->tcpci);
>>  }
>>
>
> Either way is OK to fix it.
> You may send out your v8 and notify Greg to drop your v7 version.
>
> Jun Li

May I add you in the Reported-by list?

-- 
Best Regards,
書帆

RE: [PATCH v7] staging: typec: handle vendor defined part and modify drp toggling flow

2018-03-11 Thread Jun Li 
Hi
> -Original Message-
> From: 李書帆 [mailto:leechu...@gmail.com]
> Sent: 2018年3月12日 13:22
> To: Jun Li <jun...@nxp.com>
> Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>;
> heikki.kroge...@linux.intel.com; li...@roeck-us.net; g...@kroah.com;
> shufan_...@richtek.com; cy_hu...@richtek.com;
> linux-kernel@vger.kernel.org; linux-...@vger.kernel.org
> Subject: Re: [PATCH v7] staging: typec: handle vendor defined part and modify
> drp toggling flow
> 
> Hi Jun,
> 
>   Thank you.
> 
> 2018-03-12 12:33 GMT+08:00 Jun Li <jun...@nxp.com>:
> > Hi,
> >
> >> +static irqreturn_t _tcpci_irq(int irq, void *dev_id) {
> >> + struct tcpci *tcpci = dev_id;
> >> +
> >> + return tcpci_irq(tcpci);
> >> +}
> >>
> > ...
> >
> >> + err = devm_request_threaded_irq(>dev, client->irq, NULL,
> >> + _tcpci_irq,
> >>   IRQF_ONESHOT |
> IRQF_TRIGGER_LOW,
> >> - dev_name(tcpci->dev), tcpci);
> >> + dev_name(>dev), chip);
> >
> > - dev_name(>dev), chip);
> > + dev_name(>dev), chip->tcpci);
> >
> > Did you ever test this patch?
> I've tested this patch with tcpci_rt1711h.c that will be sent out for 
> reviewing in
> the next patch after tcpci's modification is passed.
> Because interrupt handler is registered in tcpci_rt1711h.c, here is the place 
> I
> didn't notice.

Understood.

> The interrupt handler for tcpci.c should be modified as following:
>  static irqreturn_t _tcpci_irq(int irq, void *dev_id)  {
> -   struct tcpci *tcpci = dev_id;
> +   struct tcpci_chip *chip = dev_id;
> 
> -   return tcpci_irq(tcpci);
> +   return tcpci_irq(chip->tcpci);
>  }
> 

Either way is OK to fix it.
You may send out your v8 and notify Greg to drop your v7 version.

Jun Li

RE: [PATCH v7] staging: typec: handle vendor defined part and modify drp toggling flow

2018-03-11 Thread Jun Li 
Hi
> -Original Message-
> From: 李書帆 [mailto:leechu...@gmail.com]
> Sent: 2018年3月12日 13:22
> To: Jun Li 
> Cc: Greg Kroah-Hartman ;
> heikki.kroge...@linux.intel.com; li...@roeck-us.net; g...@kroah.com;
> shufan_...@richtek.com; cy_hu...@richtek.com;
> linux-kernel@vger.kernel.org; linux-...@vger.kernel.org
> Subject: Re: [PATCH v7] staging: typec: handle vendor defined part and modify
> drp toggling flow
> 
> Hi Jun,
> 
>   Thank you.
> 
> 2018-03-12 12:33 GMT+08:00 Jun Li :
> > Hi,
> >
> >> +static irqreturn_t _tcpci_irq(int irq, void *dev_id) {
> >> + struct tcpci *tcpci = dev_id;
> >> +
> >> + return tcpci_irq(tcpci);
> >> +}
> >>
> > ...
> >
> >> + err = devm_request_threaded_irq(>dev, client->irq, NULL,
> >> + _tcpci_irq,
> >>   IRQF_ONESHOT |
> IRQF_TRIGGER_LOW,
> >> - dev_name(tcpci->dev), tcpci);
> >> + dev_name(>dev), chip);
> >
> > - dev_name(>dev), chip);
> > + dev_name(>dev), chip->tcpci);
> >
> > Did you ever test this patch?
> I've tested this patch with tcpci_rt1711h.c that will be sent out for 
> reviewing in
> the next patch after tcpci's modification is passed.
> Because interrupt handler is registered in tcpci_rt1711h.c, here is the place 
> I
> didn't notice.

Understood.

> The interrupt handler for tcpci.c should be modified as following:
>  static irqreturn_t _tcpci_irq(int irq, void *dev_id)  {
> -   struct tcpci *tcpci = dev_id;
> +   struct tcpci_chip *chip = dev_id;
> 
> -   return tcpci_irq(tcpci);
> +   return tcpci_irq(chip->tcpci);
>  }
> 

Either way is OK to fix it.
You may send out your v8 and notify Greg to drop your v7 version.

Jun Li

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

2018-03-11 Thread 李書帆 
Hi Jun,

  Thank you.

2018-03-12 12:33 GMT+08:00 Jun Li :
> Hi,
>
>> +static irqreturn_t _tcpci_irq(int irq, void *dev_id) {
>> + struct tcpci *tcpci = dev_id;
>> +
>> + return tcpci_irq(tcpci);
>> +}
>>
> ...
>
>> + err = devm_request_threaded_irq(>dev, client->irq, NULL,
>> + _tcpci_irq,
>>   IRQF_ONESHOT | IRQF_TRIGGER_LOW,
>> - dev_name(tcpci->dev), tcpci);
>> + dev_name(>dev), chip);
>
> - dev_name(>dev), chip);
> + dev_name(>dev), chip->tcpci);
>
> Did you ever test this patch?
I've tested this patch with tcpci_rt1711h.c that will be sent out for
reviewing in the next patch after tcpci's modification is passed.
Because interrupt handler is registered in tcpci_rt1711h.c, here is
the place I didn't notice.
The interrupt handler for tcpci.c should be modified as following:
 static irqreturn_t _tcpci_irq(int irq, void *dev_id)
 {
-   struct tcpci *tcpci = dev_id;
+   struct tcpci_chip *chip = dev_id;

-   return tcpci_irq(tcpci);
+   return tcpci_irq(chip->tcpci);
 }

>
> I noticed Greg already picked this patch[1]:
> [1] 
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/commit/?h=usb-testing=8f94390226487bcb86c554ddc12eef0d27bdec3b
>
> One more minor comment below.
>
> Jun Li
>
>> diff --git a/drivers/staging/typec/tcpci.h b/drivers/staging/typec/tcpci.h 
>> index
>> fdfb06c..a2c1754 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_DRPRSTBIT(5)
>
> Defined but not used.
This definition can be removed for now.
>
>>  #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_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);
>> + int (*start_drp_toggling)(struct tcpci *tcpci, struct tcpci_data *data,
>> +   enum typec_cc_status cc);
>> +};
>> +
>> +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
>



-- 
Best Regards,
書帆

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

2018-03-11 Thread 李書帆 
Hi Jun,

  Thank you.

2018-03-12 12:33 GMT+08:00 Jun Li :
> Hi,
>
>> +static irqreturn_t _tcpci_irq(int irq, void *dev_id) {
>> + struct tcpci *tcpci = dev_id;
>> +
>> + return tcpci_irq(tcpci);
>> +}
>>
> ...
>
>> + err = devm_request_threaded_irq(>dev, client->irq, NULL,
>> + _tcpci_irq,
>>   IRQF_ONESHOT | IRQF_TRIGGER_LOW,
>> - dev_name(tcpci->dev), tcpci);
>> + dev_name(>dev), chip);
>
> - dev_name(>dev), chip);
> + dev_name(>dev), chip->tcpci);
>
> Did you ever test this patch?
I've tested this patch with tcpci_rt1711h.c that will be sent out for
reviewing in the next patch after tcpci's modification is passed.
Because interrupt handler is registered in tcpci_rt1711h.c, here is
the place I didn't notice.
The interrupt handler for tcpci.c should be modified as following:
 static irqreturn_t _tcpci_irq(int irq, void *dev_id)
 {
-   struct tcpci *tcpci = dev_id;
+   struct tcpci_chip *chip = dev_id;

-   return tcpci_irq(tcpci);
+   return tcpci_irq(chip->tcpci);
 }

>
> I noticed Greg already picked this patch[1]:
> [1] 
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/commit/?h=usb-testing=8f94390226487bcb86c554ddc12eef0d27bdec3b
>
> One more minor comment below.
>
> Jun Li
>
>> diff --git a/drivers/staging/typec/tcpci.h b/drivers/staging/typec/tcpci.h 
>> index
>> fdfb06c..a2c1754 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_DRPRSTBIT(5)
>
> Defined but not used.
This definition can be removed for now.
>
>>  #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_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);
>> + int (*start_drp_toggling)(struct tcpci *tcpci, struct tcpci_data *data,
>> +   enum typec_cc_status cc);
>> +};
>> +
>> +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
>



-- 
Best Regards,
書帆

RE: [PATCH v7] staging: typec: handle vendor defined part and modify drp toggling flow

2018-03-11 Thread Jun Li 
Hi,

> +static irqreturn_t _tcpci_irq(int irq, void *dev_id) {
> + struct tcpci *tcpci = dev_id;
> +
> + return tcpci_irq(tcpci);
> +}
> 
...

> + err = devm_request_threaded_irq(>dev, client->irq, NULL,
> + _tcpci_irq,
>   IRQF_ONESHOT | IRQF_TRIGGER_LOW,
> - dev_name(tcpci->dev), tcpci);
> + dev_name(>dev), chip);

- dev_name(>dev), chip);
+ dev_name(>dev), chip->tcpci); 

Did you ever test this patch?

I noticed Greg already picked this patch[1]:
[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/commit/?h=usb-testing=8f94390226487bcb86c554ddc12eef0d27bdec3b

One more minor comment below.

Jun Li

> diff --git a/drivers/staging/typec/tcpci.h b/drivers/staging/typec/tcpci.h 
> index
> fdfb06c..a2c1754 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_DRPRSTBIT(5)

Defined but not used.

>  #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_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);
> + int (*start_drp_toggling)(struct tcpci *tcpci, struct tcpci_data *data,
> +   enum typec_cc_status cc);
> +};
> +
> +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

RE: [PATCH v7] staging: typec: handle vendor defined part and modify drp toggling flow

2018-03-11 Thread Jun Li 
Hi,

> +static irqreturn_t _tcpci_irq(int irq, void *dev_id) {
> + struct tcpci *tcpci = dev_id;
> +
> + return tcpci_irq(tcpci);
> +}
> 
...

> + err = devm_request_threaded_irq(>dev, client->irq, NULL,
> + _tcpci_irq,
>   IRQF_ONESHOT | IRQF_TRIGGER_LOW,
> - dev_name(tcpci->dev), tcpci);
> + dev_name(>dev), chip);

- dev_name(>dev), chip);
+ dev_name(>dev), chip->tcpci); 

Did you ever test this patch?

I noticed Greg already picked this patch[1]:
[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/commit/?h=usb-testing=8f94390226487bcb86c554ddc12eef0d27bdec3b

One more minor comment below.

Jun Li

> diff --git a/drivers/staging/typec/tcpci.h b/drivers/staging/typec/tcpci.h 
> index
> fdfb06c..a2c1754 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_DRPRSTBIT(5)

Defined but not used.

>  #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_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);
> + int (*start_drp_toggling)(struct tcpci *tcpci, struct tcpci_data *data,
> +   enum typec_cc_status cc);
> +};
> +
> +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

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

2018-03-09 Thread Guenter Roeck 
On Tue, Mar 06, 2018 at 09:50:32AM +0800, 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 changed as following:
>   - Write DRP = 1, Rp level and RC.CCx to Rd/Rd or Rp/Rp
>   - Set LOOK4CONNECTION command
> 
> Signed-off-by: ShuFan Lee 

I am fine with this version.

Reviewed-by: Guenter Roeck 

> ---
>  drivers/staging/typec/tcpci.c | 127 
> +-
>  drivers/staging/typec/tcpci.h |  15 +
>  2 files changed, 116 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
> 
>  patch changelogs between v6 & v7
>  - Remove checking whether tcpci->data is NULL because it is guaranteed to be 
> available.
> 
> diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
> index 9bd4412..8043740 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,17 @@ 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;
>  
> + /* Handle vendor drp toggling */
> + if (tcpci->data->start_drp_toggling) {
> + 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 +129,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 +200,13 @@ static int tcpci_set_vconn(struct tcpc_dev *tcpc, bool 
> enable)
>   struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
>   int ret;
>  
> + /* Handle vendor set vconn */
>

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

2018-03-09 Thread Guenter Roeck 
On Tue, Mar 06, 2018 at 09:50:32AM +0800, 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 changed as following:
>   - Write DRP = 1, Rp level and RC.CCx to Rd/Rd or Rp/Rp
>   - Set LOOK4CONNECTION command
> 
> Signed-off-by: ShuFan Lee 

I am fine with this version.

Reviewed-by: Guenter Roeck 

> ---
>  drivers/staging/typec/tcpci.c | 127 
> +-
>  drivers/staging/typec/tcpci.h |  15 +
>  2 files changed, 116 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
> 
>  patch changelogs between v6 & v7
>  - Remove checking whether tcpci->data is NULL because it is guaranteed to be 
> available.
> 
> diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
> index 9bd4412..8043740 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,17 @@ 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;
>  
> + /* Handle vendor drp toggling */
> + if (tcpci->data->start_drp_toggling) {
> + 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 +129,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 +200,13 @@ 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->set_vconn) {
> + ret =