Message-ID: <b7e6482007584692972ffbdfd4238e6c@ex1.rt.l>
Date:   Mon, 29 Jan 2018 07:19:06 +0000
From:   shufan_lee(李書帆) <shufan_lee@...htek.com>
To:     'Guenter Roeck' <linux@...ck-us.net>
CC:     Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        'Jun Li' <jun.li@....com>, ShuFanLee <leechu729@...il.com>,
        cy_huang(黃啟原) <cy_huang@...htek.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>
Subject: 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:groeck7@...il.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@...r.kernel.org; linux-usb@...r.kernel.org
Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

On Mon, Jan 22, 2018 at 02:01:13AM +0000, 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:groeck7@...il.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@...r.kernel.org; linux-usb@...r.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 +0000, 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!
************* 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!

Powered by blists - more mailing lists