Re: [PATCH v4 2/2] dt-bindings: usb: rt1711h device tree binding document

[李書帆]

Hi Rob,

2018-03-27 6:23 GMT+08:00 Rob Herring :
> On Tue, Mar 20, 2018 at 05:15:04PM +0800, ShuFan Lee wrote:
>> From: ShuFan Lee 
>>
>> Add device tree binding document for Richtek RT1711H Type-C chip driver
>>
>> Signed-off-by: ShuFan Lee 
>> ---
>>  .../devicetree/bindings/usb/richtek,rt1711h.txt| 30 
>> ++
>>  1 file changed, 30 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
>>
>>  changelogs between v2 & v3
>>  - add dt-bindings for rt1711h typec driver
>>
>> diff --git a/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt 
>> b/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
>> new file mode 100644
>> index ..7da4dac78ea7
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
>> @@ -0,0 +1,30 @@
>> +Richtek RT1711H TypeC PD Controller.
>> +
>> +Required properties:
>> + - compatible : Must be "richtek,rt1711h".
>> + - reg : Must be 0x4e, it's slave address of RT1711H.
>> +
>> +Recommended properties :
>> + - interrupt-parent : the phandle for the interrupt controller that
>> +   provides interrupts for this device.
>> + - interrupts :  where a is the interrupt number and b represents an
>> +   encoding of the sense and level information for the interrupt.
>> +
>> +Optional properties :
>> + - rt,intr-gpios : IRQ GPIO pin that's connected to RT1711H interrupt.
>> +   if interrupt-parent & interrupts are not defined, use this property 
>> instead.
>
> Drop this. You should simply always have interrupts property.
Does this also imply that we could always assume client->irq is ready
for request?
Therefore, there's no need to check client->irq and get gpio through
rt,intr-gpios.
>
> Rob



-- 
Best Regards,
書帆

Re: [PATCH v4 2/2] dt-bindings: usb: rt1711h device tree binding document

[李書帆]

Hi Rob,

2018-03-27 6:23 GMT+08:00 Rob Herring :
> On Tue, Mar 20, 2018 at 05:15:04PM +0800, ShuFan Lee wrote:
>> From: ShuFan Lee 
>>
>> Add device tree binding document for Richtek RT1711H Type-C chip driver
>>
>> Signed-off-by: ShuFan Lee 
>> ---
>>  .../devicetree/bindings/usb/richtek,rt1711h.txt| 30 
>> ++
>>  1 file changed, 30 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
>>
>>  changelogs between v2 & v3
>>  - add dt-bindings for rt1711h typec driver
>>
>> diff --git a/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt 
>> b/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
>> new file mode 100644
>> index ..7da4dac78ea7
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
>> @@ -0,0 +1,30 @@
>> +Richtek RT1711H TypeC PD Controller.
>> +
>> +Required properties:
>> + - compatible : Must be "richtek,rt1711h".
>> + - reg : Must be 0x4e, it's slave address of RT1711H.
>> +
>> +Recommended properties :
>> + - interrupt-parent : the phandle for the interrupt controller that
>> +   provides interrupts for this device.
>> + - interrupts :  where a is the interrupt number and b represents an
>> +   encoding of the sense and level information for the interrupt.
>> +
>> +Optional properties :
>> + - rt,intr-gpios : IRQ GPIO pin that's connected to RT1711H interrupt.
>> +   if interrupt-parent & interrupts are not defined, use this property 
>> instead.
>
> Drop this. You should simply always have interrupts property.
Does this also imply that we could always assume client->irq is ready
for request?
Therefore, there's no need to check client->irq and get gpio through
rt,intr-gpios.
>
> Rob



-- 
Best Regards,
書帆

Re: [PATCH 2/2] dt-bindings: usb: rt1711h device tree binding document

[李書帆]

Hi Guenter,

2018-03-20 0:29 GMT+08:00 Guenter Roeck :
> On Mon, Mar 19, 2018 at 11:49:35AM +0800, ShuFan Lee wrote:
>> From: ShuFan Lee 
>>
>> Add device tree binding document for Richtek RT1711H Type-C chip driver
>
> Cc: Rob Herring , devicet...@vger.kernel.org
> is missing. Added here but it might make sense to resend.
Thank you for reminding, I'll resend it.

>
>>
>> Signed-off-by: ShuFan Lee 
>> ---
>>  .../devicetree/bindings/usb/richtek,rt1711h.txt| 30 
>> ++
>>  1 file changed, 30 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
>>
>>  changelogs between v2 & v3
>>  - add dt-bindings document for rt1711h typec chip driver
>>
>> diff --git a/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt 
>> b/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
>> new file mode 100644
>> index ..7da4dac78ea7
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
>> @@ -0,0 +1,30 @@
>> +Richtek RT1711H TypeC PD Controller.
>> +
>> +Required properties:
>> + - compatible : Must be "richtek,rt1711h".
>> + - reg : Must be 0x4e, it's slave address of RT1711H.
>> +
>> +Recommended properties :
>> + - interrupt-parent : the phandle for the interrupt controller that
>> +   provides interrupts for this device.
>> + - interrupts :  where a is the interrupt number and b represents an
>> +   encoding of the sense and level information for the interrupt.
>> +
>> +Optional properties :
>> + - rt,intr-gpios : IRQ GPIO pin that's connected to RT1711H interrupt.
>> +   if interrupt-parent & interrupts are not defined, use this property 
>> instead.
>> +
>> +Example :
>> +rt1711h@4e {
>> + compatible = "richtek,rt1711h";
>> + reg = <0x4e>;
>> + interrupt-parent = <>;
>> + interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
>> +};
>> +
>> +Example using rt,intr-gpios :
>> +rt1711h@4e {
>> + compatible = "richtek,rt1711h";
>> + reg = <0x4e>;
>> + rt,intr-gpios = < 0 0x0>;
>> +};
>> --
>> 1.9.1
>>



-- 
Best Regards,
書帆

Re: [PATCH 2/2] dt-bindings: usb: rt1711h device tree binding document

[李書帆]

Hi Guenter,

2018-03-20 0:29 GMT+08:00 Guenter Roeck :
> On Mon, Mar 19, 2018 at 11:49:35AM +0800, ShuFan Lee wrote:
>> From: ShuFan Lee 
>>
>> Add device tree binding document for Richtek RT1711H Type-C chip driver
>
> Cc: Rob Herring , devicet...@vger.kernel.org
> is missing. Added here but it might make sense to resend.
Thank you for reminding, I'll resend it.

>
>>
>> Signed-off-by: ShuFan Lee 
>> ---
>>  .../devicetree/bindings/usb/richtek,rt1711h.txt| 30 
>> ++
>>  1 file changed, 30 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
>>
>>  changelogs between v2 & v3
>>  - add dt-bindings document for rt1711h typec chip driver
>>
>> diff --git a/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt 
>> b/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
>> new file mode 100644
>> index ..7da4dac78ea7
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
>> @@ -0,0 +1,30 @@
>> +Richtek RT1711H TypeC PD Controller.
>> +
>> +Required properties:
>> + - compatible : Must be "richtek,rt1711h".
>> + - reg : Must be 0x4e, it's slave address of RT1711H.
>> +
>> +Recommended properties :
>> + - interrupt-parent : the phandle for the interrupt controller that
>> +   provides interrupts for this device.
>> + - interrupts :  where a is the interrupt number and b represents an
>> +   encoding of the sense and level information for the interrupt.
>> +
>> +Optional properties :
>> + - rt,intr-gpios : IRQ GPIO pin that's connected to RT1711H interrupt.
>> +   if interrupt-parent & interrupts are not defined, use this property 
>> instead.
>> +
>> +Example :
>> +rt1711h@4e {
>> + compatible = "richtek,rt1711h";
>> + reg = <0x4e>;
>> + interrupt-parent = <>;
>> + interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
>> +};
>> +
>> +Example using rt,intr-gpios :
>> +rt1711h@4e {
>> + compatible = "richtek,rt1711h";
>> + reg = <0x4e>;
>> + rt,intr-gpios = < 0 0x0>;
>> +};
>> --
>> 1.9.1
>>



-- 
Best Regards,
書帆

Re: [PATCH v2] staging: typec: rt1711h typec chip driver

[李書帆]

Hi Guenter,

2018-03-17 10:16 GMT+08:00 Guenter Roeck :
> On 03/16/2018 10:40 AM, ShuFan Lee wrote:
>>
>> From: ShuFan Lee 
>>
>> Richtek RT1711H Type-C chip driver that works with
>> Type-C Port Controller Manager to provide USB PD and
>> USB Type-C functionalities.
>> Add definition of TCPC_CC_STATUS_TOGGLING.
>>
>> Signed-off-by: ShuFan Lee 
>
>
> Does this patch require DT review for the bindings ?
I see Jun is also working on the properties of DTS for TCPCI/TCPM in
his patches.
So, I plan to upload the devicetree binding file when it is ready to
move tcpci_rt1711h out of staging.
Is it OK or should I upload a version first?
>
> Gueter
>
>
>> ---
>>   drivers/staging/typec/Kconfig |   8 +
>>   drivers/staging/typec/Makefile|   1 +
>>   drivers/staging/typec/tcpci.h |   1 +
>>   drivers/staging/typec/tcpci_rt1711h.c | 329
>> ++
>>   4 files changed, 339 insertions(+)
>>   create mode 100644 drivers/staging/typec/tcpci_rt1711h.c
>>
>>   changelogs between v1 and v2
>>   - use gpiod_* instead of gpio_*
>>
>> diff --git a/drivers/staging/typec/Kconfig b/drivers/staging/typec/Kconfig
>> index 5359f556d203..3aa981fbc8f5 100644
>> --- a/drivers/staging/typec/Kconfig
>> +++ b/drivers/staging/typec/Kconfig
>> @@ -9,6 +9,14 @@ config TYPEC_TCPCI
>> help
>>   Type-C Port Controller driver for TCPCI-compliant controller.
>>   +config TYPEC_RT1711H
>> +   tristate "Richtek RT1711H Type-C chip driver"
>> +   select TYPEC_TCPCI
>> +   help
>> + Richtek RT1711H Type-C chip driver that works with
>> + Type-C Port Controller Manager to provide USB PD and USB
>> + Type-C functionalities.
>> +
>>   endif
>> endmenu
>> diff --git a/drivers/staging/typec/Makefile
>> b/drivers/staging/typec/Makefile
>> index 53d649abcb53..7803d485e1b3 100644
>> --- a/drivers/staging/typec/Makefile
>> +++ b/drivers/staging/typec/Makefile
>> @@ -1 +1,2 @@
>>   obj-$(CONFIG_TYPEC_TCPCI) += tcpci.o
>> +obj-$(CONFIG_TYPEC_RT1711H)+= tcpci_rt1711h.o
>> diff --git a/drivers/staging/typec/tcpci.h b/drivers/staging/typec/tcpci.h
>> index 34c865f0dcf6..303ebde26546 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_TOGGLINGBIT(5)
>>   #define TCPC_CC_STATUS_TERM   BIT(4)
>>   #define TCPC_CC_STATUS_CC2_SHIFT  2
>>   #define TCPC_CC_STATUS_CC2_MASK   0x3
>> diff --git a/drivers/staging/typec/tcpci_rt1711h.c
>> b/drivers/staging/typec/tcpci_rt1711h.c
>> new file mode 100644
>> index ..12afac363d6d
>> --- /dev/null
>> +++ b/drivers/staging/typec/tcpci_rt1711h.c
>> @@ -0,0 +1,329 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Copyright (C) 2018, Richtek Technology Corporation
>> + *
>> + * Richtek RT1711H Type-C Chip Driver
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include "tcpci.h"
>> +
>> +#define RT1711H_RTCTRL80x9B
>> +
>> +/* Autoidle timeout = (tout * 2 + 1) * 6.4ms */
>> +#define RT1711H_RTCTRL8_SET(ck300, ship_off, auto_idle, tout) \
>> +   (((ck300) << 7) | ((ship_off) << 5) | \
>> +   ((auto_idle) << 3) | ((tout) & 0x07))
>> +
>> +#define RT1711H_RTCTRL11   0x9E
>> +
>> +/* I2C timeout = (tout + 1) * 12.5ms */
>> +#define RT1711H_RTCTRL11_SET(en, tout) \
>> +(((en) << 7) | ((tout) & 0x0F))
>> +
>> +#define RT1711H_RTCTRL13   0xA0
>> +#define RT1711H_RTCTRL14   0xA1
>> +#define RT1711H_RTCTRL15   0xA2
>> +#define RT1711H_RTCTRL16   0xA3
>> +
>> +struct rt1711h_chip {
>> +   struct tcpci_data data;
>> +   struct tcpci *tcpci;
>> +   struct device *dev;
>> +   int irq;
>> +};
>> +
>> +static int rt1711h_read16(struct rt1711h_chip *chip, unsigned int reg,
>> u16 *val)
>> +{
>> +   return regmap_raw_read(chip->data.regmap, reg, val, sizeof(u16));
>> +}
>> +
>> +static int rt1711h_write16(struct rt1711h_chip *chip, unsigned int reg,
>> u16 val)
>> +{
>> +   return regmap_raw_write(chip->data.regmap, reg, ,
>> sizeof(u16));
>> +}
>> +
>> +static int rt1711h_read8(struct rt1711h_chip *chip, unsigned int reg, u8
>> *val)
>> +{
>> +   return regmap_raw_read(chip->data.regmap, reg, val, sizeof(u8));
>> +}
>> +
>> +static int rt1711h_write8(struct rt1711h_chip *chip, unsigned int reg, u8
>> val)
>> +{
>> +   return regmap_raw_write(chip->data.regmap, reg, , sizeof(u8));
>> +}
>> +
>> +static const struct regmap_config rt1711h_regmap_config = {
>> +   .reg_bits = 8,
>> +   .val_bits = 8,
>> +
>> +   .max_register = 0xFF, /* 0x80 .. 0xFF are vendor defined */
>> +};
>> +
>> +static struct rt1711h_chip 

Re: [PATCH v2] staging: typec: rt1711h typec chip driver

[李書帆]

Hi Guenter,

2018-03-17 10:16 GMT+08:00 Guenter Roeck :
> On 03/16/2018 10:40 AM, ShuFan Lee wrote:
>>
>> From: ShuFan Lee 
>>
>> Richtek RT1711H Type-C chip driver that works with
>> Type-C Port Controller Manager to provide USB PD and
>> USB Type-C functionalities.
>> Add definition of TCPC_CC_STATUS_TOGGLING.
>>
>> Signed-off-by: ShuFan Lee 
>
>
> Does this patch require DT review for the bindings ?
I see Jun is also working on the properties of DTS for TCPCI/TCPM in
his patches.
So, I plan to upload the devicetree binding file when it is ready to
move tcpci_rt1711h out of staging.
Is it OK or should I upload a version first?
>
> Gueter
>
>
>> ---
>>   drivers/staging/typec/Kconfig |   8 +
>>   drivers/staging/typec/Makefile|   1 +
>>   drivers/staging/typec/tcpci.h |   1 +
>>   drivers/staging/typec/tcpci_rt1711h.c | 329
>> ++
>>   4 files changed, 339 insertions(+)
>>   create mode 100644 drivers/staging/typec/tcpci_rt1711h.c
>>
>>   changelogs between v1 and v2
>>   - use gpiod_* instead of gpio_*
>>
>> diff --git a/drivers/staging/typec/Kconfig b/drivers/staging/typec/Kconfig
>> index 5359f556d203..3aa981fbc8f5 100644
>> --- a/drivers/staging/typec/Kconfig
>> +++ b/drivers/staging/typec/Kconfig
>> @@ -9,6 +9,14 @@ config TYPEC_TCPCI
>> help
>>   Type-C Port Controller driver for TCPCI-compliant controller.
>>   +config TYPEC_RT1711H
>> +   tristate "Richtek RT1711H Type-C chip driver"
>> +   select TYPEC_TCPCI
>> +   help
>> + Richtek RT1711H Type-C chip driver that works with
>> + Type-C Port Controller Manager to provide USB PD and USB
>> + Type-C functionalities.
>> +
>>   endif
>> endmenu
>> diff --git a/drivers/staging/typec/Makefile
>> b/drivers/staging/typec/Makefile
>> index 53d649abcb53..7803d485e1b3 100644
>> --- a/drivers/staging/typec/Makefile
>> +++ b/drivers/staging/typec/Makefile
>> @@ -1 +1,2 @@
>>   obj-$(CONFIG_TYPEC_TCPCI) += tcpci.o
>> +obj-$(CONFIG_TYPEC_RT1711H)+= tcpci_rt1711h.o
>> diff --git a/drivers/staging/typec/tcpci.h b/drivers/staging/typec/tcpci.h
>> index 34c865f0dcf6..303ebde26546 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_TOGGLINGBIT(5)
>>   #define TCPC_CC_STATUS_TERM   BIT(4)
>>   #define TCPC_CC_STATUS_CC2_SHIFT  2
>>   #define TCPC_CC_STATUS_CC2_MASK   0x3
>> diff --git a/drivers/staging/typec/tcpci_rt1711h.c
>> b/drivers/staging/typec/tcpci_rt1711h.c
>> new file mode 100644
>> index ..12afac363d6d
>> --- /dev/null
>> +++ b/drivers/staging/typec/tcpci_rt1711h.c
>> @@ -0,0 +1,329 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Copyright (C) 2018, Richtek Technology Corporation
>> + *
>> + * Richtek RT1711H Type-C Chip Driver
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include "tcpci.h"
>> +
>> +#define RT1711H_RTCTRL80x9B
>> +
>> +/* Autoidle timeout = (tout * 2 + 1) * 6.4ms */
>> +#define RT1711H_RTCTRL8_SET(ck300, ship_off, auto_idle, tout) \
>> +   (((ck300) << 7) | ((ship_off) << 5) | \
>> +   ((auto_idle) << 3) | ((tout) & 0x07))
>> +
>> +#define RT1711H_RTCTRL11   0x9E
>> +
>> +/* I2C timeout = (tout + 1) * 12.5ms */
>> +#define RT1711H_RTCTRL11_SET(en, tout) \
>> +(((en) << 7) | ((tout) & 0x0F))
>> +
>> +#define RT1711H_RTCTRL13   0xA0
>> +#define RT1711H_RTCTRL14   0xA1
>> +#define RT1711H_RTCTRL15   0xA2
>> +#define RT1711H_RTCTRL16   0xA3
>> +
>> +struct rt1711h_chip {
>> +   struct tcpci_data data;
>> +   struct tcpci *tcpci;
>> +   struct device *dev;
>> +   int irq;
>> +};
>> +
>> +static int rt1711h_read16(struct rt1711h_chip *chip, unsigned int reg,
>> u16 *val)
>> +{
>> +   return regmap_raw_read(chip->data.regmap, reg, val, sizeof(u16));
>> +}
>> +
>> +static int rt1711h_write16(struct rt1711h_chip *chip, unsigned int reg,
>> u16 val)
>> +{
>> +   return regmap_raw_write(chip->data.regmap, reg, ,
>> sizeof(u16));
>> +}
>> +
>> +static int rt1711h_read8(struct rt1711h_chip *chip, unsigned int reg, u8
>> *val)
>> +{
>> +   return regmap_raw_read(chip->data.regmap, reg, val, sizeof(u8));
>> +}
>> +
>> +static int rt1711h_write8(struct rt1711h_chip *chip, unsigned int reg, u8
>> val)
>> +{
>> +   return regmap_raw_write(chip->data.regmap, reg, , sizeof(u8));
>> +}
>> +
>> +static const struct regmap_config rt1711h_regmap_config = {
>> +   .reg_bits = 8,
>> +   .val_bits = 8,
>> +
>> +   .max_register = 0xFF, /* 0x80 .. 0xFF are vendor defined */
>> +};
>> +
>> +static struct rt1711h_chip *tdata_to_rt1711h(struct tcpci_data *tdata)
>> +{
>> +   return 

Re: [PATCH] staging: typec: rt1711h typec chip driver

[李書帆]

Hi Heikki,

2018-03-16 23:05 GMT+08:00 Heikki Krogerus :
> Hi ShuFan,
>
> On Fri, Mar 16, 2018 at 05:12:49PM +0800, ShuFan Lee wrote:
>> +static int rt1711h_init_gpio(struct rt1711h_chip *chip)
>> +{
>> + int ret;
>> + struct device_node *np = chip->dev->of_node;
>> +
>> + ret = of_get_named_gpio(np, "rt,intr_gpio", 0);
>> + if (ret < 0) {
>> + dev_err(chip->dev, "%s get int gpio fail(%d)\n", __func__, 
>> ret);
>> + return ret;
>> + }
>> + chip->irq_gpio = ret;
>> +
>> + ret = devm_gpio_request_one(chip->dev, chip->irq_gpio, GPIOF_IN,
>> + dev_name(chip->dev));
>> + if (ret < 0) {
>> + dev_err(chip->dev, "%s request gpio fail(%d)\n", __func__, 
>> ret);
>> + return ret;
>> + }
>> +
>> + chip->irq = gpio_to_irq(chip->irq_gpio);
>> + if (chip->irq <= 0) {
>> + dev_err(chip->dev, "%s gpio2irq fail(%d)\n", __func__,
>> + chip->irq);
>> + return -EINVAL;
>> + }
>> + return 0;
>
> "rt,intr_gpio" should probable be "rt,intr-gpio". Then this function
> can be prepared for all types of platforms:
>
> static int rt1711h_init_gpio(struct rt1711h_chip *chip)
> {
> struct gpio_desc *gpio;
>
> gpio = devm_gpiod_get(chip->dev, "rt,intr", GFP_KERNEL);
> if (IS_ERR(gpio))
> return PTR_ERR(gpio);
>
> chip->irq = gpiod_to_irq(gpio);
> if (chip->irq < 0)
> return chip->irq;
>
> return 0;
> }
>
>
> Thanks,
>
> --
> heikki

  Thank you, I've changed it in PATCH v2.

  May I add you to Suggested-by list?

-- 
Best Regards,
書帆

Re: [PATCH] staging: typec: rt1711h typec chip driver

[李書帆]

Hi Heikki,

2018-03-16 23:05 GMT+08:00 Heikki Krogerus :
> Hi ShuFan,
>
> On Fri, Mar 16, 2018 at 05:12:49PM +0800, ShuFan Lee wrote:
>> +static int rt1711h_init_gpio(struct rt1711h_chip *chip)
>> +{
>> + int ret;
>> + struct device_node *np = chip->dev->of_node;
>> +
>> + ret = of_get_named_gpio(np, "rt,intr_gpio", 0);
>> + if (ret < 0) {
>> + dev_err(chip->dev, "%s get int gpio fail(%d)\n", __func__, 
>> ret);
>> + return ret;
>> + }
>> + chip->irq_gpio = ret;
>> +
>> + ret = devm_gpio_request_one(chip->dev, chip->irq_gpio, GPIOF_IN,
>> + dev_name(chip->dev));
>> + if (ret < 0) {
>> + dev_err(chip->dev, "%s request gpio fail(%d)\n", __func__, 
>> ret);
>> + return ret;
>> + }
>> +
>> + chip->irq = gpio_to_irq(chip->irq_gpio);
>> + if (chip->irq <= 0) {
>> + dev_err(chip->dev, "%s gpio2irq fail(%d)\n", __func__,
>> + chip->irq);
>> + return -EINVAL;
>> + }
>> + return 0;
>
> "rt,intr_gpio" should probable be "rt,intr-gpio". Then this function
> can be prepared for all types of platforms:
>
> static int rt1711h_init_gpio(struct rt1711h_chip *chip)
> {
> struct gpio_desc *gpio;
>
> gpio = devm_gpiod_get(chip->dev, "rt,intr", GFP_KERNEL);
> if (IS_ERR(gpio))
> return PTR_ERR(gpio);
>
> chip->irq = gpiod_to_irq(gpio);
> if (chip->irq < 0)
> return chip->irq;
>
> return 0;
> }
>
>
> Thanks,
>
> --
> heikki

  Thank you, I've changed it in PATCH v2.

  May I add you to Suggested-by list?

-- 
Best Regards,
書帆

Re: [PATCH] staging: typec: rt1711h typec chip driver

[李書帆]

Hi Greg,

2018-03-16 21:29 GMT+08:00 Mats Karrman :
> Hi,
>
> On 2018-03-16 13:58, Greg KH wrote:
>
>> On Fri, Mar 16, 2018 at 05:12:49PM +0800, ShuFan Lee wrote:
>>>
>>> From: ShuFan Lee 
>>>
>>> Richtek RT1711H Type-C chip driver that works with
>>> Type-C Port Controller Manager to provide USB PD and
>>> USB Type-C functionalities.
>>> Add definition of TCPC_CC_STATUS_TOGGLING.
>>>
>>> Signed-off-by: ShuFan Lee 
>>> ---
>>>   drivers/staging/typec/Kconfig |   8 +
>>>   drivers/staging/typec/Makefile|   1 +
>>>   drivers/staging/typec/tcpci.h |   1 +
>>>   drivers/staging/typec/tcpci_rt1711h.c | 344
>>> ++
>>>   4 files changed, 354 insertions(+)
>>>   create mode 100644 drivers/staging/typec/tcpci_rt1711h.c
>>
>> This looks nice and simple to me, any reason it has to live in
>> drivers/staging/ and we can't just move it to drivers/usb/typec/ now?
>>
>> Or are there still api things that need to be resolved with the existing
>> drivers/staging/typec/ code?
>
>
> For one thing it depends on the tcpci driver that is still in staging until
> Jun Li's patches, [1], comes through.
>
> // Mats
>
> [1] https://marc.info/?l=linux-usb=152093458605648=2
>
>> thanks,
>>
>> greg k-h
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>

Just like Mats said. Because tcpci_rt1711h.c depends on tcpci driver.
I'm not sure if I can put it to drivers/usb/typec/. So, I put it
staging first and move out after Jun's patches.

-- 
Best Regards,
書帆

Re: [PATCH] staging: typec: rt1711h typec chip driver

[李書帆]

Hi Greg,

2018-03-16 21:29 GMT+08:00 Mats Karrman :
> Hi,
>
> On 2018-03-16 13:58, Greg KH wrote:
>
>> On Fri, Mar 16, 2018 at 05:12:49PM +0800, ShuFan Lee wrote:
>>>
>>> From: ShuFan Lee 
>>>
>>> Richtek RT1711H Type-C chip driver that works with
>>> Type-C Port Controller Manager to provide USB PD and
>>> USB Type-C functionalities.
>>> Add definition of TCPC_CC_STATUS_TOGGLING.
>>>
>>> Signed-off-by: ShuFan Lee 
>>> ---
>>>   drivers/staging/typec/Kconfig |   8 +
>>>   drivers/staging/typec/Makefile|   1 +
>>>   drivers/staging/typec/tcpci.h |   1 +
>>>   drivers/staging/typec/tcpci_rt1711h.c | 344
>>> ++
>>>   4 files changed, 354 insertions(+)
>>>   create mode 100644 drivers/staging/typec/tcpci_rt1711h.c
>>
>> This looks nice and simple to me, any reason it has to live in
>> drivers/staging/ and we can't just move it to drivers/usb/typec/ now?
>>
>> Or are there still api things that need to be resolved with the existing
>> drivers/staging/typec/ code?
>
>
> For one thing it depends on the tcpci driver that is still in staging until
> Jun Li's patches, [1], comes through.
>
> // Mats
>
> [1] https://marc.info/?l=linux-usb=152093458605648=2
>
>> thanks,
>>
>> greg k-h
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>

Just like Mats said. Because tcpci_rt1711h.c depends on tcpci driver.
I'm not sure if I can put it to drivers/usb/typec/. So, I put it
staging first and move out after Jun's patches.

-- 
Best Regards,
書帆

Re: [PATCH] Fixes: 8f9439022648("staging: typec: modify parameter of tcpci_irq")

[李書帆]

Hi Greg,

2018-03-12 19:07 GMT+08:00 Greg KH :
> On Mon, Mar 12, 2018 at 05:46:42PM +0800, ShuFan Lee wrote:
>> From: ShuFan Lee 
>
> Your subject is odd, that line should be below, in the signed-off-by:
> area, not as the subject of the patch :(
>
> Can you fix this up and resend?
Yes.
If I understand correctly, the title should be like
staging: typec: modify parameter of tcpci_irq
and the "Fixes" tag should be put in the signed-off-by area.

Should it be in the beginning of the area:
Fixes: 8f9439022648("staging: typec: handle vendor defined part and
modify drp toggling flow")
Signed-off-by: xxx

or the end of the area?
Signed-off-by: xxx
Fixes: 8f9439022648("staging: typec: handle vendor defined part and
modify drp toggling flow")

>
> thanks,
>
> greg k-h



-- 
Best Regards,
書帆

Re: [PATCH] Fixes: 8f9439022648("staging: typec: modify parameter of tcpci_irq")

[李書帆]

Hi Greg,

2018-03-12 19:07 GMT+08:00 Greg KH :
> On Mon, Mar 12, 2018 at 05:46:42PM +0800, ShuFan Lee wrote:
>> From: ShuFan Lee 
>
> Your subject is odd, that line should be below, in the signed-off-by:
> area, not as the subject of the patch :(
>
> Can you fix this up and resend?
Yes.
If I understand correctly, the title should be like
staging: typec: modify parameter of tcpci_irq
and the "Fixes" tag should be put in the signed-off-by area.

Should it be in the beginning of the area:
Fixes: 8f9439022648("staging: typec: handle vendor defined part and
modify drp toggling flow")
Signed-off-by: xxx

or the end of the area?
Signed-off-by: xxx
Fixes: 8f9439022648("staging: typec: handle vendor defined part and
modify drp toggling flow")

>
> thanks,
>
> greg k-h



-- 
Best Regards,
書帆

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?

-- 
Best Regards,
書帆

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?

-- 
Best Regards,
書帆

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.
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

[李書帆]

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 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

[李書帆]

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 ret;
>> +   return regmap_write(tcpci->regmap, TCPC_COMMAND,
>> 

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

[李書帆]

Hi,

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

2018-03-03 2:39 GMT+08:00 李書帆 <leechu...@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...@nxp.com>:
>>
>> Hi
>> > -Original Message-
>> > From: shufan_lee(李書帆) [mailto:shufan_...@richtek.com]
>> > Sent: 2018年3月1日 19:54
>> > To: Jun Li <jun...@nxp.com>; ShuFanLee <leechu...@gmail.com>;
>> > heikki.kroge...@linux.intel.com; li...@roeck-us.net; g...@kroah.com
>> > Cc: cy_huang(黃啟原) <cy_hu...@richtek.com>;
>> > linux-kernel@vger.kernel.org; linux-...@vger.kernel.org; dl-linux-imx
>> > <linux-...@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...@nxp.com]
>> > > Sent: Thursday, March 01, 2018 6:06 PM
>> > > To: shufan_lee(李書帆); ShuFanLee; heikki.kroge...@linux.intel.com;
>> > > li...@roeck-us.net; g...@kroah.com
>> > > Cc: cy_huang(黃啟原); linux-kernel@vger.kernel.org;
>> > > linux-...@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_...@richtek.com]
>> > > Sent: 2018年3月1日 16:49
>> > > To: Jun Li <jun...@nxp.com>; ShuFanLee <leechu...@gmail.com>;
>> > > heikki.kroge...@linux.intel.com; li...@roeck-us.net; g...@kroah.com
>> > > Cc: cy_huang(黃啟原) <cy_hu...@richtek.com>;
>> > > linux-kernel@vger.kernel.org; linux-...@vger.kernel.org; dl-linux-imx
>> > > <linux-...@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/x(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

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

[李書帆]

Hi,

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

2018-03-03 2:39 GMT+08:00 李書帆 :
>
> 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 :
>>
>> Hi
>> > -Original Message-
>> > From: shufan_lee(李書帆) [mailto:shufan_...@richtek.com]
>> > Sent: 2018年3月1日 19:54
>> > To: Jun Li ; ShuFanLee ;
>> > heikki.kroge...@linux.intel.com; li...@roeck-us.net; g...@kroah.com
>> > Cc: cy_huang(黃啟原) ;
>> > linux-kernel@vger.kernel.org; linux-...@vger.kernel.org; dl-linux-imx
>> > 
>> > Subject: RE: [PATCH] staging: typec: handle vendor defined part and modify
>> > drp toggling flow
>> >
>> > Hi Jun,
>> >
>> > > -Original Message-
>> > > From: Jun Li [mailto:jun...@nxp.com]
>> > > Sent: Thursday, March 01, 2018 6:06 PM
>> > > To: shufan_lee(李書帆); ShuFanLee; heikki.kroge...@linux.intel.com;
>> > > li...@roeck-us.net; g...@kroah.com
>> > > Cc: cy_huang(黃啟原); linux-kernel@vger.kernel.org;
>> > > linux-...@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_...@richtek.com]
>> > > Sent: 2018年3月1日 16:49
>> > > To: Jun Li ; ShuFanLee ;
>> > > heikki.kroge...@linux.intel.com; li...@roeck-us.net; g...@kroah.com
>> > > Cc: cy_huang(黃啟原) ;
>> > > linux-kernel@vger.kernel.org; linux-...@vger.kernel.org; dl-linux-imx
>> > > 
>> > > 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/x(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 answ

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

[李書帆]

Hi Jun,

> -Original Message-
> From: Jun Li [mailto:jun...@nxp.com]
> Sent: Thursday, March 01, 2018 6:06 PM
> To: shufan_lee(李書帆); ShuFanLee; heikki.kroge...@linux.intel.com; 
> li...@roeck-us.net; g...@kroah.com
> Cc: cy_huang(黃啟原); linux-kernel@vger.kernel.org; linux-...@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_...@richtek.com]
> Sent: 2018年3月1日 16:49
> To: Jun Li <jun...@nxp.com>; ShuFanLee <leechu...@gmail.com>;
> heikki.kroge...@linux.intel.com; li...@roeck-us.net; g...@kroah.com
> Cc: cy_huang(黃啟原) <cy_hu...@richtek.com>;
> linux-kernel@vger.kernel.org; linux-...@vger.kernel.org; dl-linux-imx
> <linux-...@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/x(your tcpc i2c device)
>
After applying your patch[2], TCPM log is as following:

[   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]

If TCPM does not enter SRC_ATTACHED state, RC.DRP will not be cleared.
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:

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

[李書帆]

Hi Jun,

> -Original Message-
> From: Jun Li [mailto:jun...@nxp.com]
> Sent: Thursday, March 01, 2018 6:06 PM
> To: shufan_lee(李書帆); ShuFanLee; heikki.kroge...@linux.intel.com; 
> li...@roeck-us.net; g...@kroah.com
> Cc: cy_huang(黃啟原); linux-kernel@vger.kernel.org; linux-...@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_...@richtek.com]
> Sent: 2018年3月1日 16:49
> To: Jun Li ; ShuFanLee ;
> heikki.kroge...@linux.intel.com; li...@roeck-us.net; g...@kroah.com
> Cc: cy_huang(黃啟原) ;
> linux-kernel@vger.kernel.org; linux-...@vger.kernel.org; dl-linux-imx
> 
> 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/x(your tcpc i2c device)
>
After applying your patch[2], TCPM log is as following:

[   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]

If TCPM does not enter SRC_ATTACHED state, RC.DRP will not be cleared.
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 

回覆: [PATCH] staging: typec: handle vendor defined part and modify drp toggling flow

[李書帆]

Hi Jun,

  For the questions of drp_toggling, our test is as following:

  According to TCPCI 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.

  We've encounter a situation while testing with RT1711H as following:
  We repeatedly plug in/out a device (with Rd), and not to provide VBUS(5V) 
while plugging in.
  If we plug out the device right after TCPC detects it and stops toggling, 
TCPM will try to restart drp_toggling.
  Here, we re-plug in the device before TCPM calls drp_toggling. Under this 
circumstance, RT1711H reports open/open after drp_toggling is called.
  For current TCPM, it stops if open/open is reported at drp_toggling state.

  The detailed flow that causes RT1711H reporting open/open is described as 
following:
  1. RT1711H detects the device, stops toggling and reports to TCPM.
  2. Rd is plugged out before set_cc is called. So, ROLE_CONTROL.DRP is still 1.
  3. Plug in the device before TCPM restarts drp_toggling
  4. The device is detected by RT1711H's internal firmware again(TCPC chip's 
internal firmware).
  5. TCPM calls drp_toggling before cc_change triggered by step 4 is handled.
  6. TCPM sets ROLE_CONTROL.DRP = 1, Rd/Rd and start drp_toggling.
According to TCPCI 4.4.5.2
The TCPM shall write B6 (DRP) = 0b and B3..0 (CC1/CC2) if it wishes to control 
the Rp/Rd
directly instead of having the TCPC perform DRP toggling autonomousl.
So, Rd/Rd set in step 6 will not work. (Because ROLE_CONTROL.DRP is 1 since the 
first step.)
  7. RT1711H will stop toggling immediately (Because it's internal firmware 
already detects device at step 4) and report open/open (Because CC1/CC2 will be 
changed to Rd by RT1711H after LOOK4CONNECTION is set. RT1711H sets to Rd and 
device is Rd so open is reported)
  8. TCPCI reports open/open to TCPM at drp_toggling state

  That's why we always set ROLE_CONTROL.DRP to 0 while setting Rd/Rd.
  If this circumstance is not a general case, we can also use a vendor ops to 
replace it.


I don't catch the point of how you handle private events by above change,
maybe post your RT1711H part as a user in one series can make it clear,
I assume this can be done in existing tcpci_irq like above vender specific
handling as well:

For every glue driver, it registers its own irq handler and calls tcpci_irq in 
the handler.

Take RT1711H as an example:
It registers its own irq handler in probe function and handle vendor defined 
interrupts before calling general tcpci_irq:

static irqreturn_t _tcpci_irq(int irq, void *dev_id)
{
struct rt1711h_chip *chip = dev_id;

/* handle vendor defined interrupts here */

/* call general tcpci_irq */
return tcpci_irq(chip->tcpci);
}

static int rt1711h_probe(struct i2c_client *client, const struct i2c_device_id 
*i2c_id)
{

ret = devm_request_threaded_irq(chip->dev, client->irq, NULL,
  _tcpci_irq,
  IRQF_ONESHOT | IRQF_TRIGGER_LOW,
  dev_name(>dev), chip);
}


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


寄件者: Jun Li <jun...@nxp.com>
寄件日期: 2018年2月22日 下午 06:16
收件者: ShuFanLee; heikki.kroge...@linux.intel.com; li...@roeck-us.net; 
g...@kroah.com
副本: shufan_lee(李書帆); cy_huang(黃啟原); linux-kernel@vger.kernel.org; 
linux-...@vger.kernel.org; dl-linux-imx
主旨: RE: [PATCH] staging: typec: handle vendor defined part and modify drp 
toggling flow

Hi,

> -Original Message-
> From: linux-usb-ow...@vger.kernel.org
> [mailto:linux-usb-ow...@vger.kernel.org] On Behalf Of ShuFanLee
> Sent: 2018年2月21日 23:02
> To: heikki.kroge...@linux.intel.com; li...@roeck-us.net; g...@kroah.com
> Cc: shufan_...@richtek.com; cy_hu...@richtek.com;
> linux-kernel@vger.kernel.org; linux-...@vger.kernel.org
> Subject: [PATCH] staging: typec: handle vendor defined part and modify drp
> toggling flow
>
> From: ShuFanLee <shufan_...@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.

My understanding of above statement is TCPM(this Linux driver) can enable
DRP and CC1/CC2 in one shot, but TCPC(your typec chip internal firmware)
needs wait until TCPM writes to COMMAND register to start drp toggling.

> DRP toggling flow is chagned as following:
>   - Wri

回覆: [PATCH] staging: typec: handle vendor defined part and modify drp toggling flow

[李書帆]

Hi Jun,

  For the questions of drp_toggling, our test is as following:

  According to TCPCI 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.

  We've encounter a situation while testing with RT1711H as following:
  We repeatedly plug in/out a device (with Rd), and not to provide VBUS(5V) 
while plugging in.
  If we plug out the device right after TCPC detects it and stops toggling, 
TCPM will try to restart drp_toggling.
  Here, we re-plug in the device before TCPM calls drp_toggling. Under this 
circumstance, RT1711H reports open/open after drp_toggling is called.
  For current TCPM, it stops if open/open is reported at drp_toggling state.

  The detailed flow that causes RT1711H reporting open/open is described as 
following:
  1. RT1711H detects the device, stops toggling and reports to TCPM.
  2. Rd is plugged out before set_cc is called. So, ROLE_CONTROL.DRP is still 1.
  3. Plug in the device before TCPM restarts drp_toggling
  4. The device is detected by RT1711H's internal firmware again(TCPC chip's 
internal firmware).
  5. TCPM calls drp_toggling before cc_change triggered by step 4 is handled.
  6. TCPM sets ROLE_CONTROL.DRP = 1, Rd/Rd and start drp_toggling.
According to TCPCI 4.4.5.2
The TCPM shall write B6 (DRP) = 0b and B3..0 (CC1/CC2) if it wishes to control 
the Rp/Rd
directly instead of having the TCPC perform DRP toggling autonomousl.
So, Rd/Rd set in step 6 will not work. (Because ROLE_CONTROL.DRP is 1 since the 
first step.)
  7. RT1711H will stop toggling immediately (Because it's internal firmware 
already detects device at step 4) and report open/open (Because CC1/CC2 will be 
changed to Rd by RT1711H after LOOK4CONNECTION is set. RT1711H sets to Rd and 
device is Rd so open is reported)
  8. TCPCI reports open/open to TCPM at drp_toggling state

  That's why we always set ROLE_CONTROL.DRP to 0 while setting Rd/Rd.
  If this circumstance is not a general case, we can also use a vendor ops to 
replace it.


I don't catch the point of how you handle private events by above change,
maybe post your RT1711H part as a user in one series can make it clear,
I assume this can be done in existing tcpci_irq like above vender specific
handling as well:

For every glue driver, it registers its own irq handler and calls tcpci_irq in 
the handler.

Take RT1711H as an example:
It registers its own irq handler in probe function and handle vendor defined 
interrupts before calling general tcpci_irq:

static irqreturn_t _tcpci_irq(int irq, void *dev_id)
{
struct rt1711h_chip *chip = dev_id;

/* handle vendor defined interrupts here */

/* call general tcpci_irq */
return tcpci_irq(chip->tcpci);
}

static int rt1711h_probe(struct i2c_client *client, const struct i2c_device_id 
*i2c_id)
{

ret = devm_request_threaded_irq(chip->dev, client->irq, NULL,
  _tcpci_irq,
  IRQF_ONESHOT | IRQF_TRIGGER_LOW,
  dev_name(>dev), chip);
}


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


寄件者: Jun Li 
寄件日期: 2018年2月22日 下午 06:16
收件者: ShuFanLee; heikki.kroge...@linux.intel.com; li...@roeck-us.net; 
g...@kroah.com
副本: shufan_lee(李書帆); cy_huang(黃啟原); linux-kernel@vger.kernel.org; 
linux-...@vger.kernel.org; dl-linux-imx
主旨: RE: [PATCH] staging: typec: handle vendor defined part and modify drp 
toggling flow

Hi,

> -Original Message-
> From: linux-usb-ow...@vger.kernel.org
> [mailto:linux-usb-ow...@vger.kernel.org] On Behalf Of ShuFanLee
> Sent: 2018年2月21日 23:02
> To: heikki.kroge...@linux.intel.com; li...@roeck-us.net; g...@kroah.com
> Cc: shufan_...@richtek.com; cy_hu...@richtek.com;
> linux-kernel@vger.kernel.org; linux-...@vger.kernel.org
> Subject: [PATCH] staging: typec: handle vendor defined part and modify drp
> toggling flow
>
> From: ShuFanLee 
>
> 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.

My understanding of above statement is TCPM(this Linux driver) can enable
DRP and CC1/CC2 in one shot, but TCPC(your typec chip internal firmware)
needs wait until TCPM writes to COMMAND register to start drp toggling.

> DRP toggling flow is chagned as following:
>   - Write DRP = 0 & Rd/Rd

Why fixed to be Rd/Rd?

回覆: [PATCH] staging: typec: handle vendor defined part and modify drp toggling flow

[李書帆]

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 <groe...@gmail.com> 代表 Guenter Roeck <li...@roeck-us.net>
寄件日期: 2018年2月22日 上午 06:15
收件者: ShuFanLee
副本: heikki.kroge...@linux.intel.com; g...@kroah.com; shufan_lee(李書帆); 
cy_huang(黃啟原); linux-kernel@vger.kernel.org; linux-...@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_...@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_...@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

回覆: [PATCH] staging: typec: handle vendor defined part and modify drp toggling flow

[李書帆]

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  代表 Guenter Roeck 
寄件日期: 2018年2月22日 上午 06:15
收件者: ShuFanLee
副本: heikki.kroge...@linux.intel.com; g...@kroah.com; shufan_lee(李書帆); 
cy_huang(黃啟原); linux-kernel@vger.kernel.org; linux-...@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 
>
> 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 

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 (r

回覆: [PATCH v3] 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 RO

[李書帆]

Hi Gerg,

  Thank you!

  I've fixed it and sent another email.

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


寄件者: Greg KH <g...@kroah.com>
寄件日期: 2018年2月21日 下午 10:39
收件者: ShuFanLee
副本: heikki.kroge...@linux.intel.com; li...@roeck-us.net; shufan_lee(李書帆); 
cy_huang(黃啟原); linux-kernel@vger.kernel.org; linux-...@vger.kernel.org
主旨: Re: [PATCH v3] 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 ...

On Wed, Feb 21, 2018 at 10:30:34PM +0800, ShuFanLee wrote:
> From: ShuFanLee <shufan_...@richtek.com>
>
> Signed-off-by: ShuFanLee <shufan_...@richtek.com>

Something went really wrong with your subject line :(

Please fix and try again.

thanks,

greg k-h
* 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!

回覆: [PATCH v3] 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 RO

[李書帆]

Hi Gerg,

  Thank you!

  I've fixed it and sent another email.

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


寄件者: Greg KH 
寄件日期: 2018年2月21日 下午 10:39
收件者: ShuFanLee
副本: heikki.kroge...@linux.intel.com; li...@roeck-us.net; shufan_lee(李書帆); 
cy_huang(黃啟原); linux-kernel@vger.kernel.org; linux-...@vger.kernel.org
主旨: Re: [PATCH v3] 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 ...

On Wed, Feb 21, 2018 at 10:30:34PM +0800, ShuFanLee wrote:
> From: ShuFanLee 
>
> Signed-off-by: ShuFanLee 

Something went really wrong with your subject line :(

Please fix and try again.

thanks,

greg k-h
* 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!

RE: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

[李書帆]

Hi Guenter,

  For now, it looks like there are two ways to implement vendor data. It would 
be nice to hear your suggestion.

  1. Set vendor data in the data field of of_device_id.
If I understand correctly, this would be the one more like you mentioned before.
In this case, tcpci_rt1711h_data needs to be defined inside tcpci.c or defined 
by other file(tcpci_rt1711h.c) but extern in tcpci.c.

For example:
static struct tcpci_vendor_data tcpci_rt1711h_data = {
.init = rt1711h_init;
.irq_handler = rt1711h_irq_handler
};
OR
extern struct tcpci_vendor_data tcpci_rt1711h_data;

Then, put this structure here
static const struct of_device_id tcpci_of_match[] = {
{ .compatible = "usb,tcpci", },
{ .compatible = "richtek,rt1711h", .data = (void *)_rt1711h_data },
{},
};

For other vendors who want to handle vendor data also need to add these code 
inside tcpci.c.
We are not sure that's what you expect or not.


2. In tcpci.c, create a static list_head used to maintain vendor data.
TCPCI driver provides an API for those vendors to add their vendor data in the 
list.
Then, we could find vendor data in the list according to compatible string.

For example:
In tcpci.h
struct tcpci_vendor_data {
const char *compatible;
int (*init)(...);
int (*irq_handler)(...);
struct list_head list;
};
/* This function adds tcpci_vendor_data to the list */
extern int tcpci_register_vendor_data(struct tcpci_vendor_data *data);

In tcpci.c
static LIST_HEAD(tcpci_vendor_data_list);
int tcpci_register_vendor_data(...)
{
...
list_add(...);
...
}

tcpci_init()
{
...
/* Find correct vendor data */
list_for_each(...)
...
}

In this case, vendor needs to guarantee that vendor data is added to the list 
before tcpci starts to work.

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

-Original Message-
From: Guenter Roeck [mailto:groe...@gmail.com] On Behalf Of Guenter Roeck
Sent: Tuesday, January 30, 2018 3:58 AM
To: shufan_lee(李書帆)
Cc: Heikki Krogerus; 'Jun Li'; ShuFanLee; cy_huang(黃啟原); 
linux-kernel@vger.kernel.org; linux-...@vger.kernel.org
Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

On Mon, Jan 29, 2018 at 07:19:06AM +, shufan_lee(李書帆) wrote:
> Hi Guenter,
>
>   We try to use the TCPCI driver on RT1711H and here are some questions.
>
>   Q1. Is current TCPCI driver written according to TypeC Port Controller 
> Interface Specification Revision 1.0 & Version 1.2?

Revision 1.0. Note that I did not find revision 1.2, only Revision 1.0 and 
Revision 2.0 version 1.0.

>   Q2. Because 0x80~0xFF are vendor defined registers. Some of them are needed 
> to be initialized in tcpci_init for RT1711H (or other chips also).
> In the future TCPCI driver, will an initial interface that is called in 
> tcpci_init be released for different vendors to implement.

My suggestion would be to provide an API for vendor specific code.

> Or, we should directly copy tcpci.c to tcpci_rt1711h.c and add the different 
> parts?

That would defeat the purpose. It would be better to implement vendor specific 
code in tcpci_rt1711h.c and call it from tcpci.c

Possible example:

struct tcpci_vendor_data {
int (*init)(...);
int (*irq_handler)(...);
...
}

static irqreturn_t tcpci_irq(...)
{
struct tcpci *tcpci = dev_id;

...
if (tcpci->vendor_data->irq_handler) {
ret = (*tcpci->vendor_data->irq_handler)(...);
...
}
...
}

tcpci_init()
{
struct tcpci_vendor_data *vendor_data = _rt1711h_data;
// eg from devicetree compatible property

...
if (vendor_data->init) {
ret = (*vendor_data->init)(...);
if (ret)
return ret;
}
...
}

>   Q3. If there are IRQs defined in vendor defined registers, will an 
> interface that is called in tcpci_irq be released for different vendors to 
> implement.
> So that they can handle their own IRQs first?

If there are vendor specific interrupts, I would assume that vendor specific 
code will have to be called. Either the generic interrupt handler can call 
vendor specific code, or the vendor specific code handles the interrupt and 
calls the generic handler. I don't know at this point which one is better.

> If the suggestion of Q2 is to copy tcpci.c to tcpci_rt1711h.c, then Q3 will 
> not be a problem.
>   Q4. According to TCPCI Specification Revision 1.0, we should set DRP = 1 
> and role to Rp/Rp or Rd/Rd and set LOOK4CONNECTION command to start toggle.
> So we modify the tcpci_start_drp_toggling in TCPCI driver as following. Here 
> we write Rd/Rd and DRP = 0 simultaneously so that Rd/Rd takes effect.
> Then we write DRP = 1 and set LOOK4CONNECTION command to start toggling.
>
SGTM.

> static int tcpci_start_drp_toggling(struct tcpc_dev *tcpc,
>  enum typec_cc_status cc)
>  {
> +int ret = 0;
>  struct tcpci *tcpci = tcpc_to_tcpci(tcpc); -unsigned int reg =
> TCPC_R

RE: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

[李書帆]

Hi Guenter,

  For now, it looks like there are two ways to implement vendor data. It would 
be nice to hear your suggestion.

  1. Set vendor data in the data field of of_device_id.
If I understand correctly, this would be the one more like you mentioned before.
In this case, tcpci_rt1711h_data needs to be defined inside tcpci.c or defined 
by other file(tcpci_rt1711h.c) but extern in tcpci.c.

For example:
static struct tcpci_vendor_data tcpci_rt1711h_data = {
.init = rt1711h_init;
.irq_handler = rt1711h_irq_handler
};
OR
extern struct tcpci_vendor_data tcpci_rt1711h_data;

Then, put this structure here
static const struct of_device_id tcpci_of_match[] = {
{ .compatible = "usb,tcpci", },
{ .compatible = "richtek,rt1711h", .data = (void *)_rt1711h_data },
{},
};

For other vendors who want to handle vendor data also need to add these code 
inside tcpci.c.
We are not sure that's what you expect or not.


2. In tcpci.c, create a static list_head used to maintain vendor data.
TCPCI driver provides an API for those vendors to add their vendor data in the 
list.
Then, we could find vendor data in the list according to compatible string.

For example:
In tcpci.h
struct tcpci_vendor_data {
const char *compatible;
int (*init)(...);
int (*irq_handler)(...);
struct list_head list;
};
/* This function adds tcpci_vendor_data to the list */
extern int tcpci_register_vendor_data(struct tcpci_vendor_data *data);

In tcpci.c
static LIST_HEAD(tcpci_vendor_data_list);
int tcpci_register_vendor_data(...)
{
...
list_add(...);
...
}

tcpci_init()
{
...
/* Find correct vendor data */
list_for_each(...)
...
}

In this case, vendor needs to guarantee that vendor data is added to the list 
before tcpci starts to work.

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

-Original Message-
From: Guenter Roeck [mailto:groe...@gmail.com] On Behalf Of Guenter Roeck
Sent: Tuesday, January 30, 2018 3:58 AM
To: shufan_lee(李書帆)
Cc: Heikki Krogerus; 'Jun Li'; ShuFanLee; cy_huang(黃啟原); 
linux-kernel@vger.kernel.org; linux-...@vger.kernel.org
Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

On Mon, Jan 29, 2018 at 07:19:06AM +, shufan_lee(李書帆) wrote:
> Hi Guenter,
>
>   We try to use the TCPCI driver on RT1711H and here are some questions.
>
>   Q1. Is current TCPCI driver written according to TypeC Port Controller 
> Interface Specification Revision 1.0 & Version 1.2?

Revision 1.0. Note that I did not find revision 1.2, only Revision 1.0 and 
Revision 2.0 version 1.0.

>   Q2. Because 0x80~0xFF are vendor defined registers. Some of them are needed 
> to be initialized in tcpci_init for RT1711H (or other chips also).
> In the future TCPCI driver, will an initial interface that is called in 
> tcpci_init be released for different vendors to implement.

My suggestion would be to provide an API for vendor specific code.

> Or, we should directly copy tcpci.c to tcpci_rt1711h.c and add the different 
> parts?

That would defeat the purpose. It would be better to implement vendor specific 
code in tcpci_rt1711h.c and call it from tcpci.c

Possible example:

struct tcpci_vendor_data {
int (*init)(...);
int (*irq_handler)(...);
...
}

static irqreturn_t tcpci_irq(...)
{
struct tcpci *tcpci = dev_id;

...
if (tcpci->vendor_data->irq_handler) {
ret = (*tcpci->vendor_data->irq_handler)(...);
...
}
...
}

tcpci_init()
{
struct tcpci_vendor_data *vendor_data = _rt1711h_data;
// eg from devicetree compatible property

...
if (vendor_data->init) {
ret = (*vendor_data->init)(...);
if (ret)
return ret;
}
...
}

>   Q3. If there are IRQs defined in vendor defined registers, will an 
> interface that is called in tcpci_irq be released for different vendors to 
> implement.
> So that they can handle their own IRQs first?

If there are vendor specific interrupts, I would assume that vendor specific 
code will have to be called. Either the generic interrupt handler can call 
vendor specific code, or the vendor specific code handles the interrupt and 
calls the generic handler. I don't know at this point which one is better.

> If the suggestion of Q2 is to copy tcpci.c to tcpci_rt1711h.c, then Q3 will 
> not be a problem.
>   Q4. According to TCPCI Specification Revision 1.0, we should set DRP = 1 
> and role to Rp/Rp or Rd/Rd and set LOOK4CONNECTION command to start toggle.
> So we modify the tcpci_start_drp_toggling in TCPCI driver as following. Here 
> we write Rd/Rd and DRP = 0 simultaneously so that Rd/Rd takes effect.
> Then we write DRP = 1 and set LOOK4CONNECTION command to start toggling.
>
SGTM.

> static int tcpci_start_drp_toggling(struct tcpc_dev *tcpc,
>  enum typec_cc_status cc)
>  {
> +int ret = 0;
>  struct tcpci *tcpci = tcpc_to_tcpci(tcpc); -unsigned int reg =
> TCPC_R

RE: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

[李書帆]

Hi Guenter,

  We try to use the TCPCI driver on RT1711H and here are some questions.

  Q1. Is current TCPCI driver written according to TypeC Port Controller 
Interface Specification Revision 1.0 & Version 1.2?
  Q2. Because 0x80~0xFF are vendor defined registers. Some of them are needed 
to be initialized in tcpci_init for RT1711H (or other chips also).
In the future TCPCI driver, will an initial interface that is called in 
tcpci_init be released for different vendors to implement.
Or, we should directly copy tcpci.c to tcpci_rt1711h.c and add the different 
parts?
  Q3. If there are IRQs defined in vendor defined registers, will an interface 
that is called in tcpci_irq be released for different vendors to implement.
So that they can handle their own IRQs first?
If the suggestion of Q2 is to copy tcpci.c to tcpci_rt1711h.c, then Q3 will not 
be a problem.
  Q4. According to TCPCI Specification Revision 1.0, we should set DRP = 1 and 
role to Rp/Rp or Rd/Rd and set LOOK4CONNECTION command to start toggle.
So we modify the tcpci_start_drp_toggling in TCPCI driver as following. Here we 
write Rd/Rd and DRP = 0 simultaneously so that Rd/Rd takes effect.
Then we write DRP = 1 and set LOOK4CONNECTION command to start toggling.

static int tcpci_start_drp_toggling(struct tcpc_dev *tcpc,
 enum typec_cc_status cc)
 {
+int ret = 0;
 struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
-unsigned int reg = TCPC_ROLE_CTRL_DRP;
+u32 reg = (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC1_SHIFT) |
+(TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC2_SHIFT);

 switch (cc) {
 default:
@@ -125,8 +672,19 @@ static int tcpci_start_drp_toggling(stru
 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;
+mdelay(1);
+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;
 }

  Q5. The tcpci_set_vbus in TCPCI driver uses command to control Sink/Source 
VBUS.
If our chip does not support power path, i.e. Sink & Source are controlled by 
other charger IC. Our chip will do nothing while setting these commands.
In the future TCPCI driver, will a framework be applied to notify these events. 
i.g. power_supply or notifier.

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

-Original Message-
From: Guenter Roeck [mailto:groe...@gmail.com] On Behalf Of Guenter Roeck
Sent: Tuesday, January 23, 2018 2:51 AM
To: shufan_lee(李書帆)
Cc: Heikki Krogerus; 'Jun Li'; ShuFanLee; cy_huang(黃啟原); 
linux-kernel@vger.kernel.org; linux-...@vger.kernel.org
Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

On Mon, Jan 22, 2018 at 02:01:13AM +, shufan_lee(李書帆) wrote:
> Dear Heikki and Guenter,
>
>   Because there are still other controls of RT1711H that are different from 
> standard TCPCI, e.x. flow of drp toggling.
>
>   Is the suggestion to customize the difference based on tcpci.c for RT1711H?
>

In general, I would say yes. However, I won't have ime to review the 
differences between tcpci and the RT1711H. On a high level, if RT1711H claims 
to suport TCPCI, it should use (or, rather, extend) the TCPCI driver.

Note that the TCPCI driver does not claim to be complete; there is a reason why 
it is still in staging. However, I would prefer if new devices claiming to 
support TCPCI would use it instead of going their own way. I don't have 
problems extending it with chip specific details if needed. Such extensions may 
be implemented in tcpci.c, or maybe better in a chip specific file.

Even if you don't use the existing driver, I don't really see why it would make 
sense to redeclare all its defines.

Either case, you might want to run checkpatch --strict on your driver. Most of 
that it reports is really unnecessary.
Also, some of the code, such as

+#ifndef BIT
+#define BIT(x) (1 << (x))
+#endif

is _really_ odd and, at least in this case, simply wrong.

Guenter

> Best Regards,
> *
> Shu-Fan Lee
> Richtek Technology Corporation
> TEL: +886-3-5526789 #2359
> FAX: +886-3-5526612
> *
>
> -Original Message-
> From: Guenter Roeck [mailto:groe...@gmail.com] On Behalf Of Guenter
> Roeck
> Sent: Saturday, January 20, 2018 12:03 AM
> To: Heikki Krogerus
> Cc: shufan_lee(李書帆); 'Jun Li'; ShuFanLee; cy_huang(黃啟原);
> linux-kernel@vger.kernel.org; linux-...@vger.kernel.org
> Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver
>
> On Fri, Jan 19, 2018 at 11:24:13AM +0200, Heikki Krogerus wrote:
> > Hi,
> >
> > On Fri, Jan 19, 2018

RE: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

[李書帆]

Hi Guenter,

  We try to use the TCPCI driver on RT1711H and here are some questions.

  Q1. Is current TCPCI driver written according to TypeC Port Controller 
Interface Specification Revision 1.0 & Version 1.2?
  Q2. Because 0x80~0xFF are vendor defined registers. Some of them are needed 
to be initialized in tcpci_init for RT1711H (or other chips also).
In the future TCPCI driver, will an initial interface that is called in 
tcpci_init be released for different vendors to implement.
Or, we should directly copy tcpci.c to tcpci_rt1711h.c and add the different 
parts?
  Q3. If there are IRQs defined in vendor defined registers, will an interface 
that is called in tcpci_irq be released for different vendors to implement.
So that they can handle their own IRQs first?
If the suggestion of Q2 is to copy tcpci.c to tcpci_rt1711h.c, then Q3 will not 
be a problem.
  Q4. According to TCPCI Specification Revision 1.0, we should set DRP = 1 and 
role to Rp/Rp or Rd/Rd and set LOOK4CONNECTION command to start toggle.
So we modify the tcpci_start_drp_toggling in TCPCI driver as following. Here we 
write Rd/Rd and DRP = 0 simultaneously so that Rd/Rd takes effect.
Then we write DRP = 1 and set LOOK4CONNECTION command to start toggling.

static int tcpci_start_drp_toggling(struct tcpc_dev *tcpc,
 enum typec_cc_status cc)
 {
+int ret = 0;
 struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
-unsigned int reg = TCPC_ROLE_CTRL_DRP;
+u32 reg = (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC1_SHIFT) |
+(TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC2_SHIFT);

 switch (cc) {
 default:
@@ -125,8 +672,19 @@ static int tcpci_start_drp_toggling(stru
 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;
+mdelay(1);
+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;
 }

  Q5. The tcpci_set_vbus in TCPCI driver uses command to control Sink/Source 
VBUS.
If our chip does not support power path, i.e. Sink & Source are controlled by 
other charger IC. Our chip will do nothing while setting these commands.
In the future TCPCI driver, will a framework be applied to notify these events. 
i.g. power_supply or notifier.

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

-Original Message-
From: Guenter Roeck [mailto:groe...@gmail.com] On Behalf Of Guenter Roeck
Sent: Tuesday, January 23, 2018 2:51 AM
To: shufan_lee(李書帆)
Cc: Heikki Krogerus; 'Jun Li'; ShuFanLee; cy_huang(黃啟原); 
linux-kernel@vger.kernel.org; linux-...@vger.kernel.org
Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

On Mon, Jan 22, 2018 at 02:01:13AM +, shufan_lee(李書帆) wrote:
> Dear Heikki and Guenter,
>
>   Because there are still other controls of RT1711H that are different from 
> standard TCPCI, e.x. flow of drp toggling.
>
>   Is the suggestion to customize the difference based on tcpci.c for RT1711H?
>

In general, I would say yes. However, I won't have ime to review the 
differences between tcpci and the RT1711H. On a high level, if RT1711H claims 
to suport TCPCI, it should use (or, rather, extend) the TCPCI driver.

Note that the TCPCI driver does not claim to be complete; there is a reason why 
it is still in staging. However, I would prefer if new devices claiming to 
support TCPCI would use it instead of going their own way. I don't have 
problems extending it with chip specific details if needed. Such extensions may 
be implemented in tcpci.c, or maybe better in a chip specific file.

Even if you don't use the existing driver, I don't really see why it would make 
sense to redeclare all its defines.

Either case, you might want to run checkpatch --strict on your driver. Most of 
that it reports is really unnecessary.
Also, some of the code, such as

+#ifndef BIT
+#define BIT(x) (1 << (x))
+#endif

is _really_ odd and, at least in this case, simply wrong.

Guenter

> Best Regards,
> *
> Shu-Fan Lee
> Richtek Technology Corporation
> TEL: +886-3-5526789 #2359
> FAX: +886-3-5526612
> *
>
> -Original Message-
> From: Guenter Roeck [mailto:groe...@gmail.com] On Behalf Of Guenter
> Roeck
> Sent: Saturday, January 20, 2018 12:03 AM
> To: Heikki Krogerus
> Cc: shufan_lee(李書帆); 'Jun Li'; ShuFanLee; cy_huang(黃啟原);
> linux-kernel@vger.kernel.org; linux-...@vger.kernel.org
> Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver
>
> On Fri, Jan 19, 2018 at 11:24:13AM +0200, Heikki Krogerus wrote:
> > Hi,
> >
> > On Fri, Jan 19, 2018

RE: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

[李書帆]

Dear Heikki and Guenter,

  Because there are still other controls of RT1711H that are different from 
standard TCPCI, e.x. flow of drp toggling.

  Is the suggestion to customize the difference based on tcpci.c for RT1711H?

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

-Original Message-
From: Guenter Roeck [mailto:groe...@gmail.com] On Behalf Of Guenter Roeck
Sent: Saturday, January 20, 2018 12:03 AM
To: Heikki Krogerus
Cc: shufan_lee(李書帆); 'Jun Li'; ShuFanLee; cy_huang(黃啟原); 
linux-kernel@vger.kernel.org; linux-...@vger.kernel.org
Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

On Fri, Jan 19, 2018 at 11:24:13AM +0200, Heikki Krogerus wrote:
> Hi,
>
> On Fri, Jan 19, 2018 at 09:01:24AM +, shufan_lee(?|) wrote:
> > Hi Heikki,
> >
> >   For example, the flow of tcpci_init is a little bit different.
> >   In tcpci_init, there are more parameters need to be set for RT1711H.
>
> Different init parameters is really not a reason for a fork of the
> driver. The configuration of the TCPC will depend on the platform and
> TCPC vendor most cases.
>
Agreed. dwc3 usb support is an excellent example on how to handle this kind of 
variation.

Guenter
* 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!

RE: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

[李書帆]

Dear Heikki and Guenter,

  Because there are still other controls of RT1711H that are different from 
standard TCPCI, e.x. flow of drp toggling.

  Is the suggestion to customize the difference based on tcpci.c for RT1711H?

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

-Original Message-
From: Guenter Roeck [mailto:groe...@gmail.com] On Behalf Of Guenter Roeck
Sent: Saturday, January 20, 2018 12:03 AM
To: Heikki Krogerus
Cc: shufan_lee(李書帆); 'Jun Li'; ShuFanLee; cy_huang(黃啟原); 
linux-kernel@vger.kernel.org; linux-...@vger.kernel.org
Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

On Fri, Jan 19, 2018 at 11:24:13AM +0200, Heikki Krogerus wrote:
> Hi,
>
> On Fri, Jan 19, 2018 at 09:01:24AM +, shufan_lee(?|) wrote:
> > Hi Heikki,
> >
> >   For example, the flow of tcpci_init is a little bit different.
> >   In tcpci_init, there are more parameters need to be set for RT1711H.
>
> Different init parameters is really not a reason for a fork of the
> driver. The configuration of the TCPC will depend on the platform and
> TCPC vendor most cases.
>
Agreed. dwc3 usb support is an excellent example on how to handle this kind of 
variation.

Guenter
* 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!

RE: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

[李書帆]

Hi Heikki,

  For example, the flow of tcpci_init is a little bit different.
  In tcpci_init, there are more parameters need to be set for RT1711H.

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

-Original Message-
From: Heikki Krogerus [mailto:heikki.kroge...@linux.intel.com]
Sent: Friday, January 19, 2018 4:22 PM
To: shufan_lee(李書帆)
Cc: 'Jun Li'; ShuFanLee; cy_huang(黃啟原); linux-kernel@vger.kernel.org; 
linux-...@vger.kernel.org; Guenter Roeck
Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

Hi Shu-Fan,

On Fri, Jan 19, 2018 at 05:48:02AM +, shufan_lee(?) wrote:
> Hi Jun,
>
>   For now, RT1711H is not fully compatible with TCPCI. So the existing tcpci.c
>   may not work for it.

The datasheet for RT1711H does talk about TCPCi and TCPM+TCPC [1].
What are the differences that justify a separate driver?

[1] http://www.richtek.com/assets/product_file/RT1711H/DS1711H-02.pdf


Br,

--
heikki
* 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!

RE: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

[李書帆]

Hi Heikki,

  For example, the flow of tcpci_init is a little bit different.
  In tcpci_init, there are more parameters need to be set for RT1711H.

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

-Original Message-
From: Heikki Krogerus [mailto:heikki.kroge...@linux.intel.com]
Sent: Friday, January 19, 2018 4:22 PM
To: shufan_lee(李書帆)
Cc: 'Jun Li'; ShuFanLee; cy_huang(黃啟原); linux-kernel@vger.kernel.org; 
linux-...@vger.kernel.org; Guenter Roeck
Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

Hi Shu-Fan,

On Fri, Jan 19, 2018 at 05:48:02AM +, shufan_lee(?) wrote:
> Hi Jun,
>
>   For now, RT1711H is not fully compatible with TCPCI. So the existing tcpci.c
>   may not work for it.

The datasheet for RT1711H does talk about TCPCi and TCPM+TCPC [1].
What are the differences that justify a separate driver?

[1] http://www.richtek.com/assets/product_file/RT1711H/DS1711H-02.pdf


Br,

--
heikki
* 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!

RE: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

[李書帆]

Hi Jun,

  For now, RT1711H is not fully compatible with TCPCI. So the existing tcpci.c 
may not work for it.

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

-Original Message-
From: Jun Li [mailto:jun...@nxp.com]
Sent: Friday, January 19, 2018 11:10 AM
To: ShuFanLee; heikki.kroge...@linux.intel.com
Cc: cy_huang(黃啟原); shufan_lee(李書帆); linux-kernel@vger.kernel.org; 
linux-...@vger.kernel.org; Guenter Roeck
Subject: RE: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

Hi
> -Original Message-
> From: linux-usb-ow...@vger.kernel.org [mailto:linux-usb-
> ow...@vger.kernel.org] On Behalf Of ShuFanLee
> Sent: Wednesday, January 10, 2018 2:59 PM
> To: heikki.kroge...@linux.intel.com
> Cc: cy_hu...@richtek.com; shufan_...@richtek.com; linux-
> ker...@vger.kernel.org; linux-...@vger.kernel.org
> Subject: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver
>
> From: ShuFanLee <shufan_...@richtek.com>
>
> Richtek RT1711H Type-C chip driver that works with Type-C Port
> Controller Manager to provide USB PD and USB Type-C functionalities.

A general question, is this Rt1711h type-c chip compatible with TCPCI 
(Universal Serial Bus Type-C Port Controller Interface Specification)?
looks like it has the same register map and has some extension, can the 
existing ./drivers/staging/typec/tcpic.c basically work for you?

+Guenter

Li Jun

>
> Signed-off-by: ShuFanLee <shufan_...@richtek.com>
> ---
>  .../devicetree/bindings/usb/richtek,rt1711h.txt|   38 +
>  arch/arm64/boot/dts/hisilicon/rt1711h.dtsi |   11 +
>  drivers/usb/typec/Kconfig  |2 +
>  drivers/usb/typec/Makefile |1 +
>  drivers/usb/typec/rt1711h/Kconfig  |7 +
>  drivers/usb/typec/rt1711h/Makefile |2 +
>  drivers/usb/typec/rt1711h/rt1711h.c| 2241 
> 
>  drivers/usb/typec/rt1711h/rt1711h.h|  300 +++
>  8 files changed, 2602 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
>  create mode 100644 arch/arm64/boot/dts/hisilicon/rt1711h.dtsi
>  create mode 100644 drivers/usb/typec/rt1711h/Kconfig  create mode
> 100644 drivers/usb/typec/rt1711h/Makefile
>  create mode 100644 drivers/usb/typec/rt1711h/rt1711h.c
>  create mode 100644 drivers/usb/typec/rt1711h/rt1711h.h
>
* 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!

RE: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

[李書帆]

Hi Jun,

  For now, RT1711H is not fully compatible with TCPCI. So the existing tcpci.c 
may not work for it.

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

-Original Message-
From: Jun Li [mailto:jun...@nxp.com]
Sent: Friday, January 19, 2018 11:10 AM
To: ShuFanLee; heikki.kroge...@linux.intel.com
Cc: cy_huang(黃啟原); shufan_lee(李書帆); linux-kernel@vger.kernel.org; 
linux-...@vger.kernel.org; Guenter Roeck
Subject: RE: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

Hi
> -Original Message-
> From: linux-usb-ow...@vger.kernel.org [mailto:linux-usb-
> ow...@vger.kernel.org] On Behalf Of ShuFanLee
> Sent: Wednesday, January 10, 2018 2:59 PM
> To: heikki.kroge...@linux.intel.com
> Cc: cy_hu...@richtek.com; shufan_...@richtek.com; linux-
> ker...@vger.kernel.org; linux-...@vger.kernel.org
> Subject: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver
>
> From: ShuFanLee 
>
> Richtek RT1711H Type-C chip driver that works with Type-C Port
> Controller Manager to provide USB PD and USB Type-C functionalities.

A general question, is this Rt1711h type-c chip compatible with TCPCI 
(Universal Serial Bus Type-C Port Controller Interface Specification)?
looks like it has the same register map and has some extension, can the 
existing ./drivers/staging/typec/tcpic.c basically work for you?

+Guenter

Li Jun

>
> Signed-off-by: ShuFanLee 
> ---
>  .../devicetree/bindings/usb/richtek,rt1711h.txt|   38 +
>  arch/arm64/boot/dts/hisilicon/rt1711h.dtsi |   11 +
>  drivers/usb/typec/Kconfig  |2 +
>  drivers/usb/typec/Makefile |1 +
>  drivers/usb/typec/rt1711h/Kconfig  |7 +
>  drivers/usb/typec/rt1711h/Makefile |2 +
>  drivers/usb/typec/rt1711h/rt1711h.c| 2241 
> 
>  drivers/usb/typec/rt1711h/rt1711h.h|  300 +++
>  8 files changed, 2602 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
>  create mode 100644 arch/arm64/boot/dts/hisilicon/rt1711h.dtsi
>  create mode 100644 drivers/usb/typec/rt1711h/Kconfig  create mode
> 100644 drivers/usb/typec/rt1711h/Makefile
>  create mode 100644 drivers/usb/typec/rt1711h/rt1711h.c
>  create mode 100644 drivers/usb/typec/rt1711h/rt1711h.h
>
* 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!

RE: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

[李書帆]

reg0x%02x=0x", desc->addr);
> +for (i = 0; i < desc->size; i++)
> +seq_printf(s, "%02x,", regval[i]);
> +seq_puts(s, "\n");
> +return 0;
> +}
> +
> +static int rt1711h_dbg_show(struct seq_file *s, void *v) {
> +int ret = 0;
> +struct rt1711h_dbg_info *info = (struct rt1711h_dbg_info *)s->private;
> +struct rt1711h_chip *chip = info->chip;
> +
> +mutex_lock(>dbgops_lock);
> +switch (info->id) {
> +case RT1711H_DBG_LOG:
> +ret = rt1711h_log_show(chip, s);
> +break;
> +case RT1711H_DBG_REGS:
> +ret = rt1711h_regs_show(chip, s);
> +break;
> +case RT1711H_DBG_REG_ADDR:
> +ret = rt1711h_reg_addr_show(chip, s);
> +break;
> +case RT1711H_DBG_DATA:
> +ret = rt1711h_data_show(chip, s);
> +break;
> +default:
> +ret = -EINVAL;
> +break;
> +}
> +
> +mutex_unlock(>dbgops_lock);
> +return ret;
> +}
> +
> +static int rt1711h_dbg_open(struct inode *inode, struct file *file) {
> +if (file->f_mode & FMODE_READ)
> +return single_open(file, rt1711h_dbg_show, inode->i_private);
> +file->private_data = inode->i_private;
> +return 0;
> +}
> +
> +static int get_parameters(char *buf, long int *param1, int
> +num_of_par) {
> +char *token;
> +int base, cnt;
> +
> +token = strsep(, " ");
> +
> +for (cnt = 0; cnt < num_of_par; cnt++) {
> +if (token != NULL) {
> +if ((token[1] == 'x') || (token[1] == 'X'))
> +base = 16;
> +else
> +base = 10;
> +
> +if (kstrtoul(token, base, [cnt]) != 0)
> +return -EINVAL;
> +
> +token = strsep(, " ");
> +} else
> +return -EINVAL;
> +}
> +return 0;
> +}

What is this function doing?  What is your debugfs files for?

There are 4 debug files.
First(log) is for logging which needs a lock for log buffer. The way to log is 
referenced from fusb302 and tcpm.
Second(regs) is used to dump all register of rt1711h.
Third(reg_addr)(data) are used to write/read a register specified in 
reg_addr.
The reason to create these debug files is to make issue support easier.


> +#ifdef CONFIG_DEBUG_FS
> +struct dentry *dbgdir;
> +struct rt1711h_dbg_info dbg_info[RT1711H_DBG_MAX];
> +struct dentry *dbg_files[RT1711H_DBG_MAX];
> +int dbg_regidx;
> +struct mutex dbgops_lock;
> +/* lock for log buffer access */
> +struct mutex logbuffer_lock;
> +int logbuffer_head;
> +int logbuffer_tail;
> +u8 *logbuffer[LOG_BUFFER_ENTRIES];
> +#endif /* CONFIG_DEBUG_FS */

That's a lot of stuff jsut for debugfs.  Why do you care about #define at all?  
The code should not.

Is the suggestion to remove #ifdef CONFIG_DEBUG_FS?

And another 2 locks?  Ick, no.

dbgops_lock is used to prevent user from accessing different debug files 
simultaneously.
Is the suggestion to use the lock of the following one?
> +/* lock for sharing chip states */
> +struct mutex lock;


> +snprintf(dirname, len + 9, "rt1711h-%s", dev_name(chip->dev));
> +if (!chip->dbgdir) {
> +chip->dbgdir = debugfs_create_dir(dirname, NULL);
> +if (!chip->dbgdir)
> +return -ENOMEM;

No need to ever check the return value of debugfs_ calls, you should not care 
and can always use the value to any future debugfs calls, if you really need it.

If it is NULL without checking and we use it in debugfs_create_file, all the 
debug files will be created in the root of the debugfs filesystem.
Is this correct?


> +for (i = 0; i < RT1711H_DBG_MAX; i++) {
> +info = >dbg_info[i];

static array of debug info?  That feels odd.

Is the suggestion to use pointer of array and dynamically allocated it?


Like here, you don't need this, and you don't need to care about the return 
value.

> +goto err;
> +}
> +}
> +
> +return 0;
> +err:
> +debugfs_remove_recursive(chip->dbgdir);
> +return ret;

Why do you care about an error here?  Your code should not do anything 
different if debugfs stuff does not work or if it does.  It's debugging only.
Ok, this will be removed.


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

-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Wednesday, January 17, 2018 9:42 PM
To: ShuFanLee
Cc: heikki.kroge...@linux.intel.com; cy_huang(黃啟原); shufan_lee(李書帆); 
linux-kernel@vger.kernel.org; linux-...@vger.kernel.org
Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

On Wed, Jan 10, 2018 at 02:59:12PM 

RE: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

[李書帆]

reg0x%02x=0x", desc->addr);
> +for (i = 0; i < desc->size; i++)
> +seq_printf(s, "%02x,", regval[i]);
> +seq_puts(s, "\n");
> +return 0;
> +}
> +
> +static int rt1711h_dbg_show(struct seq_file *s, void *v) {
> +int ret = 0;
> +struct rt1711h_dbg_info *info = (struct rt1711h_dbg_info *)s->private;
> +struct rt1711h_chip *chip = info->chip;
> +
> +mutex_lock(>dbgops_lock);
> +switch (info->id) {
> +case RT1711H_DBG_LOG:
> +ret = rt1711h_log_show(chip, s);
> +break;
> +case RT1711H_DBG_REGS:
> +ret = rt1711h_regs_show(chip, s);
> +break;
> +case RT1711H_DBG_REG_ADDR:
> +ret = rt1711h_reg_addr_show(chip, s);
> +break;
> +case RT1711H_DBG_DATA:
> +ret = rt1711h_data_show(chip, s);
> +break;
> +default:
> +ret = -EINVAL;
> +break;
> +}
> +
> +mutex_unlock(>dbgops_lock);
> +return ret;
> +}
> +
> +static int rt1711h_dbg_open(struct inode *inode, struct file *file) {
> +if (file->f_mode & FMODE_READ)
> +return single_open(file, rt1711h_dbg_show, inode->i_private);
> +file->private_data = inode->i_private;
> +return 0;
> +}
> +
> +static int get_parameters(char *buf, long int *param1, int
> +num_of_par) {
> +char *token;
> +int base, cnt;
> +
> +token = strsep(, " ");
> +
> +for (cnt = 0; cnt < num_of_par; cnt++) {
> +if (token != NULL) {
> +if ((token[1] == 'x') || (token[1] == 'X'))
> +base = 16;
> +else
> +base = 10;
> +
> +if (kstrtoul(token, base, [cnt]) != 0)
> +return -EINVAL;
> +
> +token = strsep(, " ");
> +} else
> +return -EINVAL;
> +}
> +return 0;
> +}

What is this function doing?  What is your debugfs files for?

There are 4 debug files.
First(log) is for logging which needs a lock for log buffer. The way to log is 
referenced from fusb302 and tcpm.
Second(regs) is used to dump all register of rt1711h.
Third(reg_addr)(data) are used to write/read a register specified in 
reg_addr.
The reason to create these debug files is to make issue support easier.


> +#ifdef CONFIG_DEBUG_FS
> +struct dentry *dbgdir;
> +struct rt1711h_dbg_info dbg_info[RT1711H_DBG_MAX];
> +struct dentry *dbg_files[RT1711H_DBG_MAX];
> +int dbg_regidx;
> +struct mutex dbgops_lock;
> +/* lock for log buffer access */
> +struct mutex logbuffer_lock;
> +int logbuffer_head;
> +int logbuffer_tail;
> +u8 *logbuffer[LOG_BUFFER_ENTRIES];
> +#endif /* CONFIG_DEBUG_FS */

That's a lot of stuff jsut for debugfs.  Why do you care about #define at all?  
The code should not.

Is the suggestion to remove #ifdef CONFIG_DEBUG_FS?

And another 2 locks?  Ick, no.

dbgops_lock is used to prevent user from accessing different debug files 
simultaneously.
Is the suggestion to use the lock of the following one?
> +/* lock for sharing chip states */
> +struct mutex lock;


> +snprintf(dirname, len + 9, "rt1711h-%s", dev_name(chip->dev));
> +if (!chip->dbgdir) {
> +chip->dbgdir = debugfs_create_dir(dirname, NULL);
> +if (!chip->dbgdir)
> +return -ENOMEM;

No need to ever check the return value of debugfs_ calls, you should not care 
and can always use the value to any future debugfs calls, if you really need it.

If it is NULL without checking and we use it in debugfs_create_file, all the 
debug files will be created in the root of the debugfs filesystem.
Is this correct?


> +for (i = 0; i < RT1711H_DBG_MAX; i++) {
> +info = >dbg_info[i];

static array of debug info?  That feels odd.

Is the suggestion to use pointer of array and dynamically allocated it?


Like here, you don't need this, and you don't need to care about the return 
value.

> +goto err;
> +}
> +}
> +
> +return 0;
> +err:
> +debugfs_remove_recursive(chip->dbgdir);
> +return ret;

Why do you care about an error here?  Your code should not do anything 
different if debugfs stuff does not work or if it does.  It's debugging only.
Ok, this will be removed.


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

-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Wednesday, January 17, 2018 9:42 PM
To: ShuFanLee
Cc: heikki.kroge...@linux.intel.com; cy_huang(黃啟原); shufan_lee(李書帆); 
linux-kernel@vger.kernel.org; linux-...@vger.kernel.org
Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

On Wed, Jan 10, 2018 at 02:59:12PM 

RE: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

[李書帆]

Dear Heikki,

  Sorry for bothering.

  Just want to check is there anything we need to modify?

  Thank you!

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

-Original Message-
From: ShuFanLee [mailto:leechu...@gmail.com]
Sent: Wednesday, January 10, 2018 2:59 PM
To: heikki.kroge...@linux.intel.com
Cc: cy_huang(黃啟原); shufan_lee(李書帆); linux-kernel@vger.kernel.org; 
linux-...@vger.kernel.org
Subject: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

From: ShuFanLee <shufan_...@richtek.com>

Richtek RT1711H Type-C chip driver that works with
Type-C Port Controller Manager to provide USB PD and
USB Type-C functionalities.

Signed-off-by: ShuFanLee <shufan_...@richtek.com>
---
 .../devicetree/bindings/usb/richtek,rt1711h.txt|   38 +
 arch/arm64/boot/dts/hisilicon/rt1711h.dtsi |   11 +
 drivers/usb/typec/Kconfig  |2 +
 drivers/usb/typec/Makefile |1 +
 drivers/usb/typec/rt1711h/Kconfig  |7 +
 drivers/usb/typec/rt1711h/Makefile |2 +
 drivers/usb/typec/rt1711h/rt1711h.c| 2241 
 drivers/usb/typec/rt1711h/rt1711h.h|  300 +++
 8 files changed, 2602 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
 create mode 100644 arch/arm64/boot/dts/hisilicon/rt1711h.dtsi
 create mode 100644 drivers/usb/typec/rt1711h/Kconfig
 create mode 100644 drivers/usb/typec/rt1711h/Makefile
 create mode 100644 drivers/usb/typec/rt1711h/rt1711h.c
 create mode 100644 drivers/usb/typec/rt1711h/rt1711h.h

diff --git a/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt 
b/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
new file mode 100644
index 000..c28299c
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
@@ -0,0 +1,38 @@
+Richtek RT1711H Type-C Port Controller.
+
+Required properties:
+- compatible : Must be "richtek,typec_rt1711h";
+- reg : Must be 0x4e, it's default slave address of RT1711H.
+- rt,intr_gpio : IRQ GPIO pin that's connected to RT1711H interrupt.
+
+Optional node:
+- rt,name : Name used for registering IRQ and creating kthread.
+If this property is not specified, "default" will be applied.
+- rt,def_role : Default port role (TYPEC_SINK(0) or TYPEC_SOURCE(1)).
+Set to TYPEC_NO_PREFERRED_ROLE(-1) if no default role.
+If this property is not specified, TYPEC_SINK will be applied.
+- rt,port_type : Port type (TYPEC_PORT_DFP(0), TYPEC_PORT_UFP(1),
+ or TYPEC_PORT_DRP(2)). If this property is not specified,
+ TYPEC_PORT_DRP will be applied.
+- rt,max_snk_mv : Maximum acceptable sink voltage in mV.
+  If this property is not specified, 5000mV will be applied.
+- rt,max_snk_ma : Maximum sink current in mA.
+  If this property is not specified, 3000mA will be applied.
+- rt,max_snk_mw : Maximum required sink power in mW.
+  If this property is not specified, 15000mW will be applied.
+- rt,operating_snk_mw : Required operating sink power in mW.
+If this property is not specified,
+2500mW will be applied.
+- rt,try_role_hw : True if try.{Src,Snk} is implemented in hardware.
+   If this property is not specified, False will be applied.
+
+Example:
+rt1711h@4e {
+status = "ok";
+compatible = "richtek,typec_rt1711h";
+reg = <0x4e>;
+rt,intr_gpio = < 0 0x0>;
+rt,name = "rt1711h";
+rt,port_type = <2>; /* 0: DFP, 1: UFP, 2: DRP */
+rt,def_role = <0>; /* 0: SNK, 1: SRC, -1: TYPEC_NO_PREFERRED_ROLE */
+};
diff --git a/arch/arm64/boot/dts/hisilicon/rt1711h.dtsi 
b/arch/arm64/boot/dts/hisilicon/rt1711h.dtsi
new file mode 100644
index 000..4196cc0
--- /dev/null
+++ b/arch/arm64/boot/dts/hisilicon/rt1711h.dtsi
@@ -0,0 +1,11 @@
+ {
+rt1711h@4e {
+status = "ok";
+compatible = "richtek,typec_rt1711h";
+reg = <0x4e>;
+rt,intr_gpio = < 0 0x0>;
+rt,name = "rt1711h";
+rt,port_type = <2>; /* 0: DFP, 1: UFP, 2: DRP */
+rt,def_role = <0>; /* 0: SNK, 1: SRC */
+};
+};
diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
index bcb2744..7bede0b 100644
--- a/drivers/usb/typec/Kconfig
+++ b/drivers/usb/typec/Kconfig
@@ -56,6 +56,8 @@ if TYPEC_TCPM

 source "drivers/usb/typec/fusb302/Kconfig"

+source "drivers/usb/typec/rt1711h/Kconfig"
+
 config TYPEC_WCOVE
 tristate "Intel WhiskeyCove PMIC USB Type-C PHY driver"
 depends on ACPI
diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
index bb3138a..e3aaf3c 100644
--- a/drivers/usb/typec/Makefile
+++ b/drivers/usb/typec/Makefile
@@ -2,6 +2,7 @@
 obj-$(CONFIG_TYPEC)+= typec.o
 obj-$(CONFIG_TYPEC_TCPM)+= tcpm.o
 obj-y+= fusb302/
+obj-$(CONFIG_TYPEC_RT1711H)+= rt1711h/
 obj-$(CONFIG_TYPEC_WCOVE)+= typec_wcove.o
 obj-$(CONFIG_TYPEC_UCSI)+= ucsi/

RE: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

[李書帆]

Dear Heikki,

  Sorry for bothering.

  Just want to check is there anything we need to modify?

  Thank you!

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

-Original Message-
From: ShuFanLee [mailto:leechu...@gmail.com]
Sent: Wednesday, January 10, 2018 2:59 PM
To: heikki.kroge...@linux.intel.com
Cc: cy_huang(黃啟原); shufan_lee(李書帆); linux-kernel@vger.kernel.org; 
linux-...@vger.kernel.org
Subject: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

From: ShuFanLee 

Richtek RT1711H Type-C chip driver that works with
Type-C Port Controller Manager to provide USB PD and
USB Type-C functionalities.

Signed-off-by: ShuFanLee 
---
 .../devicetree/bindings/usb/richtek,rt1711h.txt|   38 +
 arch/arm64/boot/dts/hisilicon/rt1711h.dtsi |   11 +
 drivers/usb/typec/Kconfig  |2 +
 drivers/usb/typec/Makefile |1 +
 drivers/usb/typec/rt1711h/Kconfig  |7 +
 drivers/usb/typec/rt1711h/Makefile |2 +
 drivers/usb/typec/rt1711h/rt1711h.c| 2241 
 drivers/usb/typec/rt1711h/rt1711h.h|  300 +++
 8 files changed, 2602 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
 create mode 100644 arch/arm64/boot/dts/hisilicon/rt1711h.dtsi
 create mode 100644 drivers/usb/typec/rt1711h/Kconfig
 create mode 100644 drivers/usb/typec/rt1711h/Makefile
 create mode 100644 drivers/usb/typec/rt1711h/rt1711h.c
 create mode 100644 drivers/usb/typec/rt1711h/rt1711h.h

diff --git a/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt 
b/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
new file mode 100644
index 000..c28299c
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
@@ -0,0 +1,38 @@
+Richtek RT1711H Type-C Port Controller.
+
+Required properties:
+- compatible : Must be "richtek,typec_rt1711h";
+- reg : Must be 0x4e, it's default slave address of RT1711H.
+- rt,intr_gpio : IRQ GPIO pin that's connected to RT1711H interrupt.
+
+Optional node:
+- rt,name : Name used for registering IRQ and creating kthread.
+If this property is not specified, "default" will be applied.
+- rt,def_role : Default port role (TYPEC_SINK(0) or TYPEC_SOURCE(1)).
+Set to TYPEC_NO_PREFERRED_ROLE(-1) if no default role.
+If this property is not specified, TYPEC_SINK will be applied.
+- rt,port_type : Port type (TYPEC_PORT_DFP(0), TYPEC_PORT_UFP(1),
+ or TYPEC_PORT_DRP(2)). If this property is not specified,
+ TYPEC_PORT_DRP will be applied.
+- rt,max_snk_mv : Maximum acceptable sink voltage in mV.
+  If this property is not specified, 5000mV will be applied.
+- rt,max_snk_ma : Maximum sink current in mA.
+  If this property is not specified, 3000mA will be applied.
+- rt,max_snk_mw : Maximum required sink power in mW.
+  If this property is not specified, 15000mW will be applied.
+- rt,operating_snk_mw : Required operating sink power in mW.
+If this property is not specified,
+2500mW will be applied.
+- rt,try_role_hw : True if try.{Src,Snk} is implemented in hardware.
+   If this property is not specified, False will be applied.
+
+Example:
+rt1711h@4e {
+status = "ok";
+compatible = "richtek,typec_rt1711h";
+reg = <0x4e>;
+rt,intr_gpio = < 0 0x0>;
+rt,name = "rt1711h";
+rt,port_type = <2>; /* 0: DFP, 1: UFP, 2: DRP */
+rt,def_role = <0>; /* 0: SNK, 1: SRC, -1: TYPEC_NO_PREFERRED_ROLE */
+};
diff --git a/arch/arm64/boot/dts/hisilicon/rt1711h.dtsi 
b/arch/arm64/boot/dts/hisilicon/rt1711h.dtsi
new file mode 100644
index 000..4196cc0
--- /dev/null
+++ b/arch/arm64/boot/dts/hisilicon/rt1711h.dtsi
@@ -0,0 +1,11 @@
+ {
+rt1711h@4e {
+status = "ok";
+compatible = "richtek,typec_rt1711h";
+reg = <0x4e>;
+rt,intr_gpio = < 0 0x0>;
+rt,name = "rt1711h";
+rt,port_type = <2>; /* 0: DFP, 1: UFP, 2: DRP */
+rt,def_role = <0>; /* 0: SNK, 1: SRC */
+};
+};
diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
index bcb2744..7bede0b 100644
--- a/drivers/usb/typec/Kconfig
+++ b/drivers/usb/typec/Kconfig
@@ -56,6 +56,8 @@ if TYPEC_TCPM

 source "drivers/usb/typec/fusb302/Kconfig"

+source "drivers/usb/typec/rt1711h/Kconfig"
+
 config TYPEC_WCOVE
 tristate "Intel WhiskeyCove PMIC USB Type-C PHY driver"
 depends on ACPI
diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
index bb3138a..e3aaf3c 100644
--- a/drivers/usb/typec/Makefile
+++ b/drivers/usb/typec/Makefile
@@ -2,6 +2,7 @@
 obj-$(CONFIG_TYPEC)+= typec.o
 obj-$(CONFIG_TYPEC_TCPM)+= tcpm.o
 obj-y+= fusb302/
+obj-$(CONFIG_TYPEC_RT1711H)+= rt1711h/
 obj-$(CONFIG_TYPEC_WCOVE)+= typec_wcove.o
 obj-$(CONFIG_TYPEC_UCSI)+= ucsi/
 obj-$(CONFIG_TYPEC_TPS6598X)+= tps6598x.o
diff --git a/drivers/us

RE: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

[李書帆]

Hi Randy,

  It is different. It seems that format is changed after sending.

  I'll try to fix that and send again, thank you!

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

-Original Message-
From: Randy Dunlap [mailto:rdun...@infradead.org]
Sent: Wednesday, January 10, 2018 1:26 AM
To: shufan_lee(李書帆); heikki.kroge...@linux.intel.com
Cc: linux-kernel@vger.kernel.org; cy_huang(黃啟原)
Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

On 01/09/18 00:45, shufan_lee(李書帆) wrote:
> Hi Randy,
>
>  I've checked the email and tabs are still there.
>
>  Would your mind providing an example of what you see and highlight the 
> incorrect format?
>
>  Thank you!
>
> Best Regards,

Please see your message in the email archive at:
https://marc.info/?l=linux-kernel=151546820509868=2

Is that how it was supposed to be formatted?

Thanks.

> *
> Shu-Fan Lee
> Richtek Technology Corporation
> TEL: +886-3-5526789 #2359
> FAX: +886-3-5526612
> *
>
> -Original Message-
> From: Randy Dunlap [mailto:rdun...@infradead.org]
> Sent: Tuesday, January 09, 2018 2:19 PM
> To: shufan_lee(李書帆); heikki.kroge...@linux.intel.com
> Cc: linux-kernel@vger.kernel.org; cy_huang(黃啟原)
> Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver
>
> On 01/08/18 19:13, shufan_lee(李書帆) wrote:
>> From: ShuFan Lee <shufan_...@richtek.com>
>>
>> Richtek RT1711H Type-C chip driver that works with Type-C Port
>> Controller Manager to provide USB PD and USB Type-C functionalities.
>>
>> Signed-off-by: ShuFan Lee <shufan_...@richtek.com> ---​
>>
>> Documentation/devicetree/bindings/usb/richtek,rt1711h.txt |   38
>> arch/arm64/boot/dts/hisilicon/rt1711h.dtsi|   11
>> drivers/usb/typec/Kconfig |2
>> drivers/usb/typec/Makefile|1
>> drivers/usb/typec/rt1711h/Kconfig |7
>> drivers/usb/typec/rt1711h/Makefile|2
>> drivers/usb/typec/rt1711h/rt1711h.c   | 2241 ++
>> drivers/usb/typec/rt1711h/rt1711h.h   |  300 +
>> 8 files changed, 2602 insertions(+)
>
> Hi,
> It looks like your mail client or your mail server (or something else) ate 
> all of the tabs that would make this patch look normal.
>
> When you have that fixed, I guess that you should also Cc: the linux-usb 
> mailing list with the patch.
>
> Thanks.
>
>> * 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!
>>
>
>
> --
> ~Randy
> * 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!
>


--
~Randy
* 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 b

RE: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

[李書帆]

Hi Randy,

  It is different. It seems that format is changed after sending.

  I'll try to fix that and send again, thank you!

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

-Original Message-
From: Randy Dunlap [mailto:rdun...@infradead.org]
Sent: Wednesday, January 10, 2018 1:26 AM
To: shufan_lee(李書帆); heikki.kroge...@linux.intel.com
Cc: linux-kernel@vger.kernel.org; cy_huang(黃啟原)
Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

On 01/09/18 00:45, shufan_lee(李書帆) wrote:
> Hi Randy,
>
>  I've checked the email and tabs are still there.
>
>  Would your mind providing an example of what you see and highlight the 
> incorrect format?
>
>  Thank you!
>
> Best Regards,

Please see your message in the email archive at:
https://marc.info/?l=linux-kernel=151546820509868=2

Is that how it was supposed to be formatted?

Thanks.

> *
> Shu-Fan Lee
> Richtek Technology Corporation
> TEL: +886-3-5526789 #2359
> FAX: +886-3-5526612
> *
>
> -Original Message-
> From: Randy Dunlap [mailto:rdun...@infradead.org]
> Sent: Tuesday, January 09, 2018 2:19 PM
> To: shufan_lee(李書帆); heikki.kroge...@linux.intel.com
> Cc: linux-kernel@vger.kernel.org; cy_huang(黃啟原)
> Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver
>
> On 01/08/18 19:13, shufan_lee(李書帆) wrote:
>> From: ShuFan Lee 
>>
>> Richtek RT1711H Type-C chip driver that works with Type-C Port
>> Controller Manager to provide USB PD and USB Type-C functionalities.
>>
>> Signed-off-by: ShuFan Lee  ---​
>>
>> Documentation/devicetree/bindings/usb/richtek,rt1711h.txt |   38
>> arch/arm64/boot/dts/hisilicon/rt1711h.dtsi|   11
>> drivers/usb/typec/Kconfig |2
>> drivers/usb/typec/Makefile|1
>> drivers/usb/typec/rt1711h/Kconfig |7
>> drivers/usb/typec/rt1711h/Makefile|2
>> drivers/usb/typec/rt1711h/rt1711h.c   | 2241 ++
>> drivers/usb/typec/rt1711h/rt1711h.h   |  300 +
>> 8 files changed, 2602 insertions(+)
>
> Hi,
> It looks like your mail client or your mail server (or something else) ate 
> all of the tabs that would make this patch look normal.
>
> When you have that fixed, I guess that you should also Cc: the linux-usb 
> mailing list with the patch.
>
> Thanks.
>
>> * 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!
>>
>
>
> --
> ~Randy
> * 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!
>


--
~Randy
* 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 us

RE: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

[李書帆]

Hi Randy,

 I've checked the email and tabs are still there.

 Would your mind providing an example of what you see and highlight the 
incorrect format?

 Thank you!

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

-Original Message-
From: Randy Dunlap [mailto:rdun...@infradead.org]
Sent: Tuesday, January 09, 2018 2:19 PM
To: shufan_lee(李書帆); heikki.kroge...@linux.intel.com
Cc: linux-kernel@vger.kernel.org; cy_huang(黃啟原)
Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

On 01/08/18 19:13, shufan_lee(李書帆) wrote:
> From: ShuFan Lee <shufan_...@richtek.com>
>
> Richtek RT1711H Type-C chip driver that works with Type-C Port
> Controller Manager to provide USB PD and USB Type-C functionalities.
>
> Signed-off-by: ShuFan Lee <shufan_...@richtek.com> ---​
>
> Documentation/devicetree/bindings/usb/richtek,rt1711h.txt |   38
> arch/arm64/boot/dts/hisilicon/rt1711h.dtsi|   11
> drivers/usb/typec/Kconfig |2
> drivers/usb/typec/Makefile|1
> drivers/usb/typec/rt1711h/Kconfig |7
> drivers/usb/typec/rt1711h/Makefile|2
> drivers/usb/typec/rt1711h/rt1711h.c   | 2241 ++
> drivers/usb/typec/rt1711h/rt1711h.h   |  300 +
> 8 files changed, 2602 insertions(+)

Hi,
It looks like your mail client or your mail server (or something else) ate all 
of the tabs that would make this patch look normal.

When you have that fixed, I guess that you should also Cc: the linux-usb 
mailing list with the patch.

Thanks.

> * 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!
>


--
~Randy
* 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!

RE: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

[李書帆]

Hi Randy,

 I've checked the email and tabs are still there.

 Would your mind providing an example of what you see and highlight the 
incorrect format?

 Thank you!

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

-Original Message-
From: Randy Dunlap [mailto:rdun...@infradead.org]
Sent: Tuesday, January 09, 2018 2:19 PM
To: shufan_lee(李書帆); heikki.kroge...@linux.intel.com
Cc: linux-kernel@vger.kernel.org; cy_huang(黃啟原)
Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

On 01/08/18 19:13, shufan_lee(李書帆) wrote:
> From: ShuFan Lee 
>
> Richtek RT1711H Type-C chip driver that works with Type-C Port
> Controller Manager to provide USB PD and USB Type-C functionalities.
>
> Signed-off-by: ShuFan Lee  ---​
>
> Documentation/devicetree/bindings/usb/richtek,rt1711h.txt |   38
> arch/arm64/boot/dts/hisilicon/rt1711h.dtsi|   11
> drivers/usb/typec/Kconfig |2
> drivers/usb/typec/Makefile|1
> drivers/usb/typec/rt1711h/Kconfig |7
> drivers/usb/typec/rt1711h/Makefile|2
> drivers/usb/typec/rt1711h/rt1711h.c   | 2241 ++
> drivers/usb/typec/rt1711h/rt1711h.h   |  300 +
> 8 files changed, 2602 insertions(+)

Hi,
It looks like your mail client or your mail server (or something else) ate all 
of the tabs that would make this patch look normal.

When you have that fixed, I guess that you should also Cc: the linux-usb 
mailing list with the patch.

Thanks.

> * 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!
>


--
~Randy
* 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!

[PATCH] USB TYPEC: RT1711H Type-C Chip Driver

[李書帆]

From: ShuFan Lee 

Richtek RT1711H Type-C chip driver that works with
Type-C Port Controller Manager to provide USB PD and
USB Type-C functionalities.

Signed-off-by: ShuFan Lee 
---​

Documentation/devicetree/bindings/usb/richtek,rt1711h.txt |   38
arch/arm64/boot/dts/hisilicon/rt1711h.dtsi|   11
drivers/usb/typec/Kconfig |2
drivers/usb/typec/Makefile|1
drivers/usb/typec/rt1711h/Kconfig |7
drivers/usb/typec/rt1711h/Makefile|2
drivers/usb/typec/rt1711h/rt1711h.c   | 2241 ++
drivers/usb/typec/rt1711h/rt1711h.h   |  300 +
8 files changed, 2602 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt 
b/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
new file mode 100644
index 000..c28299c
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
@@ -0,0 +1,38 @@
+Richtek RT1711H Type-C Port Controller.
+
+Required properties:
+- compatible : Must be "richtek,typec_rt1711h";
+- reg : Must be 0x4e, it's default slave address of RT1711H.
+- rt,intr_gpio : IRQ GPIO pin that's connected to RT1711H interrupt.
+
+Optional node:
+- rt,name : Name used for registering IRQ and creating kthread.
+If this property is not specified, "default" will be applied.
+- rt,def_role : Default port role (TYPEC_SINK(0) or TYPEC_SOURCE(1)).
+Set to TYPEC_NO_PREFERRED_ROLE(-1) if no default role.
+If this property is not specified, TYPEC_SINK will be applied.
+- rt,port_type : Port type (TYPEC_PORT_DFP(0), TYPEC_PORT_UFP(1),
+ or TYPEC_PORT_DRP(2)). If this property is not specified,
+ TYPEC_PORT_DRP will be applied.
+- rt,max_snk_mv : Maximum acceptable sink voltage in mV.
+  If this property is not specified, 5000mV will be applied.
+- rt,max_snk_ma : Maximum sink current in mA.
+  If this property is not specified, 3000mA will be applied.
+- rt,max_snk_mw : Maximum required sink power in mW.
+  If this property is not specified, 15000mW will be applied.
+- rt,operating_snk_mw : Required operating sink power in mW.
+If this property is not specified,
+2500mW will be applied.
+- rt,try_role_hw : True if try.{Src,Snk} is implemented in hardware.
+   If this property is not specified, False will be applied.
+
+Example:
+rt1711h@4e {
+status = "ok";
+compatible = "richtek,typec_rt1711h";
+reg = <0x4e>;
+rt,intr_gpio = < 0 0x0>;
+rt,name = "rt1711h";
+rt,port_type = <2>; /* 0: DFP, 1: UFP, 2: DRP */
+rt,def_role = <0>; /* 0: SNK, 1: SRC, -1: TYPEC_NO_PREFERRED_ROLE */
+};
diff --git a/arch/arm64/boot/dts/hisilicon/rt1711h.dtsi 
b/arch/arm64/boot/dts/hisilicon/rt1711h.dtsi
new file mode 100644
index 000..4196cc0
--- /dev/null
+++ b/arch/arm64/boot/dts/hisilicon/rt1711h.dtsi
@@ -0,0 +1,11 @@
+ {
+rt1711h@4e {
+status = "ok";
+compatible = "richtek,typec_rt1711h";
+reg = <0x4e>;
+rt,intr_gpio = < 0 0x0>;
+rt,name = "rt1711h";
+rt,port_type = <2>; /* 0: DFP, 1: UFP, 2: DRP */
+rt,def_role = <0>; /* 0: SNK, 1: SRC */
+};
+};
diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
index bcb2744..7bede0b 100644
--- a/drivers/usb/typec/Kconfig
+++ b/drivers/usb/typec/Kconfig
@@ -56,6 +56,8 @@ if TYPEC_TCPM

 source "drivers/usb/typec/fusb302/Kconfig"

+source "drivers/usb/typec/rt1711h/Kconfig"
+
 config TYPEC_WCOVE
 tristate "Intel WhiskeyCove PMIC USB Type-C PHY driver"
 depends on ACPI
diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
index bb3138a..e3aaf3c 100644
--- a/drivers/usb/typec/Makefile
+++ b/drivers/usb/typec/Makefile
@@ -2,6 +2,7 @@
 obj-$(CONFIG_TYPEC)+= typec.o
 obj-$(CONFIG_TYPEC_TCPM)+= tcpm.o
 obj-y+= fusb302/
+obj-$(CONFIG_TYPEC_RT1711H)+= rt1711h/
 obj-$(CONFIG_TYPEC_WCOVE)+= typec_wcove.o
 obj-$(CONFIG_TYPEC_UCSI)+= ucsi/
 obj-$(CONFIG_TYPEC_TPS6598X)+= tps6598x.o
diff --git a/drivers/usb/typec/rt1711h/Kconfig 
b/drivers/usb/typec/rt1711h/Kconfig
new file mode 100644
index 000..2fbfff5
--- /dev/null
+++ b/drivers/usb/typec/rt1711h/Kconfig
@@ -0,0 +1,7 @@
+config TYPEC_RT1711H
+tristate "Richtek RT1711H Type-C chip driver"
+depends on I2C && POWER_SUPPLY
+help
+  The Richtek RT1711H   Type-C chip driver that works with
+  Type-C Port Controller Manager to provide USB PD and USB
+  Type-C functionalities.
diff --git a/drivers/usb/typec/rt1711h/Makefile 
b/drivers/usb/typec/rt1711h/Makefile
new file mode 100644
index 000..5fab8ae
--- /dev/null
+++ b/drivers/usb/typec/rt1711h/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_TYPEC_RT1711H)+= rt1711h.o
diff --git a/drivers/usb/typec/rt1711h/rt1711h.c 
b/drivers/usb/typec/rt1711h/rt1711h.c
new file mode 100644
index 000..1aef3e8
--- /dev/null
+++ b/drivers/usb/typec/rt1711h/rt1711h.c
@@ -0,0 +1,2241 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2017 Richtek Technologh Corp.
+ *
+ * Richtek 

[PATCH] USB TYPEC: RT1711H Type-C Chip Driver

[李書帆]

From: ShuFan Lee 

Richtek RT1711H Type-C chip driver that works with
Type-C Port Controller Manager to provide USB PD and
USB Type-C functionalities.

Signed-off-by: ShuFan Lee 
---​

Documentation/devicetree/bindings/usb/richtek,rt1711h.txt |   38
arch/arm64/boot/dts/hisilicon/rt1711h.dtsi|   11
drivers/usb/typec/Kconfig |2
drivers/usb/typec/Makefile|1
drivers/usb/typec/rt1711h/Kconfig |7
drivers/usb/typec/rt1711h/Makefile|2
drivers/usb/typec/rt1711h/rt1711h.c   | 2241 ++
drivers/usb/typec/rt1711h/rt1711h.h   |  300 +
8 files changed, 2602 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt 
b/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
new file mode 100644
index 000..c28299c
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
@@ -0,0 +1,38 @@
+Richtek RT1711H Type-C Port Controller.
+
+Required properties:
+- compatible : Must be "richtek,typec_rt1711h";
+- reg : Must be 0x4e, it's default slave address of RT1711H.
+- rt,intr_gpio : IRQ GPIO pin that's connected to RT1711H interrupt.
+
+Optional node:
+- rt,name : Name used for registering IRQ and creating kthread.
+If this property is not specified, "default" will be applied.
+- rt,def_role : Default port role (TYPEC_SINK(0) or TYPEC_SOURCE(1)).
+Set to TYPEC_NO_PREFERRED_ROLE(-1) if no default role.
+If this property is not specified, TYPEC_SINK will be applied.
+- rt,port_type : Port type (TYPEC_PORT_DFP(0), TYPEC_PORT_UFP(1),
+ or TYPEC_PORT_DRP(2)). If this property is not specified,
+ TYPEC_PORT_DRP will be applied.
+- rt,max_snk_mv : Maximum acceptable sink voltage in mV.
+  If this property is not specified, 5000mV will be applied.
+- rt,max_snk_ma : Maximum sink current in mA.
+  If this property is not specified, 3000mA will be applied.
+- rt,max_snk_mw : Maximum required sink power in mW.
+  If this property is not specified, 15000mW will be applied.
+- rt,operating_snk_mw : Required operating sink power in mW.
+If this property is not specified,
+2500mW will be applied.
+- rt,try_role_hw : True if try.{Src,Snk} is implemented in hardware.
+   If this property is not specified, False will be applied.
+
+Example:
+rt1711h@4e {
+status = "ok";
+compatible = "richtek,typec_rt1711h";
+reg = <0x4e>;
+rt,intr_gpio = < 0 0x0>;
+rt,name = "rt1711h";
+rt,port_type = <2>; /* 0: DFP, 1: UFP, 2: DRP */
+rt,def_role = <0>; /* 0: SNK, 1: SRC, -1: TYPEC_NO_PREFERRED_ROLE */
+};
diff --git a/arch/arm64/boot/dts/hisilicon/rt1711h.dtsi 
b/arch/arm64/boot/dts/hisilicon/rt1711h.dtsi
new file mode 100644
index 000..4196cc0
--- /dev/null
+++ b/arch/arm64/boot/dts/hisilicon/rt1711h.dtsi
@@ -0,0 +1,11 @@
+ {
+rt1711h@4e {
+status = "ok";
+compatible = "richtek,typec_rt1711h";
+reg = <0x4e>;
+rt,intr_gpio = < 0 0x0>;
+rt,name = "rt1711h";
+rt,port_type = <2>; /* 0: DFP, 1: UFP, 2: DRP */
+rt,def_role = <0>; /* 0: SNK, 1: SRC */
+};
+};
diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
index bcb2744..7bede0b 100644
--- a/drivers/usb/typec/Kconfig
+++ b/drivers/usb/typec/Kconfig
@@ -56,6 +56,8 @@ if TYPEC_TCPM

 source "drivers/usb/typec/fusb302/Kconfig"

+source "drivers/usb/typec/rt1711h/Kconfig"
+
 config TYPEC_WCOVE
 tristate "Intel WhiskeyCove PMIC USB Type-C PHY driver"
 depends on ACPI
diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
index bb3138a..e3aaf3c 100644
--- a/drivers/usb/typec/Makefile
+++ b/drivers/usb/typec/Makefile
@@ -2,6 +2,7 @@
 obj-$(CONFIG_TYPEC)+= typec.o
 obj-$(CONFIG_TYPEC_TCPM)+= tcpm.o
 obj-y+= fusb302/
+obj-$(CONFIG_TYPEC_RT1711H)+= rt1711h/
 obj-$(CONFIG_TYPEC_WCOVE)+= typec_wcove.o
 obj-$(CONFIG_TYPEC_UCSI)+= ucsi/
 obj-$(CONFIG_TYPEC_TPS6598X)+= tps6598x.o
diff --git a/drivers/usb/typec/rt1711h/Kconfig 
b/drivers/usb/typec/rt1711h/Kconfig
new file mode 100644
index 000..2fbfff5
--- /dev/null
+++ b/drivers/usb/typec/rt1711h/Kconfig
@@ -0,0 +1,7 @@
+config TYPEC_RT1711H
+tristate "Richtek RT1711H Type-C chip driver"
+depends on I2C && POWER_SUPPLY
+help
+  The Richtek RT1711H   Type-C chip driver that works with
+  Type-C Port Controller Manager to provide USB PD and USB
+  Type-C functionalities.
diff --git a/drivers/usb/typec/rt1711h/Makefile 
b/drivers/usb/typec/rt1711h/Makefile
new file mode 100644
index 000..5fab8ae
--- /dev/null
+++ b/drivers/usb/typec/rt1711h/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_TYPEC_RT1711H)+= rt1711h.o
diff --git a/drivers/usb/typec/rt1711h/rt1711h.c 
b/drivers/usb/typec/rt1711h/rt1711h.c
new file mode 100644
index 000..1aef3e8
--- /dev/null
+++ b/drivers/usb/typec/rt1711h/rt1711h.c
@@ -0,0 +1,2241 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2017 Richtek Technologh Corp.
+ *
+ * Richtek RT1711H Type-C Chip Driver
+ */
+
+#include