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

[Guenter Roeck]

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@xxxxxxxxx] 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@xxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx
> 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!