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

[Guenter Roeck]

On Tue, Jan 30, 2018 at 01:21:01PM +, shufan_lee(李書帆) wrote:
> 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.
> 

I would not say expect, but it is one possibility. Sure,
it requires rt1711h_init and rt1711h_irq_handler to be public,
and a bit of ifdefery, but it is simpler than option #2.

Another option would be to instantiate tcpci from vendor drivers.
In this case, there would be an exported registration function which
would be called from tcpci_rt1711h.c:rt1711h_init(), similar to
tcpm_register_port(). In that case, tcpci_rt1711h.c would have its
own init function and compatible property.

To do that, you would effectively split tcpci_probe() into two functions,
tcpci_probe() and tcpci_register_port(), and call tcpci_register_port()
from the probe function.

int tcpci_register_port(struct i2c_client *client,
const struct tcpci_vendor_data *data)
{
/* pretty much verything currently done in the probe function */
}
EXPORT_SYMBOL(tcpci_register_port);

static int tcpci_probe(struct i2c_client *client,
   const struct i2c_device_id *i2c_id)
{
return tcpci_register_port(client, NULL);
}

Maybe you can experiment with this and see if it makes sense.
If not, you can still fall back to option #1.

Thanks,
Guenter
--
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

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-ker...@vger.kernel.org; linux-usb@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

[Guenter Roeck]

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_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);

That is bad; you don't want to hold up teh system for that much.
Try to use usleep_range().

> +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.
> 
I would think so.

Note that the driver is, at this point, fair game to change to
make it work with your chip. The only condition is that a standard
chip should still work, ie you should not make any changes which
would cause the driver to _only_ work with your chip. Everything else
is fair game.

Thanks,
Guenter
--
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

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-ker...@vger.kernel.org; linux-usb@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-ker...@vger.kernel.org; linux-usb@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

[Guenter Roeck]

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-ker...@vger.kernel.org; linux-usb@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!
--
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

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-ker...@vger.kernel.org; linux-usb@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

[Guenter Roeck]

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

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

[Heikki Krogerus]

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.


Thanks,

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

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-ker...@vger.kernel.org; 
linux-usb@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

[Heikki Krogerus]

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

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

['Greg KH']

On Thu, Jan 18, 2018 at 01:13:15PM +, shufan_lee(李書帆) wrote:
> > +
> > +#include "rt1711h.h"
> 
> Why a .h file for a single .c file?
> 
> Is the suggestion to move all content in rt1711h.h into rt1711h.c?

If it can be, sure, you only need a .h file for things that are shared
among other .c files.

> > +/* I2C */
> > +atomic_t i2c_busy;
> > +atomic_t pm_suspend;
> 
> Why are these atomic?  You know that doesn't mean they do not need locking, 
> right?
> 
> For my understanding, a single operation on atomic_t doesn't need lock, like 
> a single atomic_set.
> But two consecutive operations doesn't guarantee anything. Like atomic_set 
> followed by an atomic_read.
> This part is referenced from fusb302 used to make sure I2C is idle before 
> system suspends.
> It only needs to guarantee a single read/write on these variable is atomic 
> operation, so atomic is used.

It's atomic for read/write, yes, but that does not mean it can not be
instantly changed after the value is read, right?  So you might need to
look and ensure you are not doing something wrong that can race.  A
single lock should be simpler than this type of thing, and will be
correct.

> > +#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?

Yes.  Or just move it all to another structure that you can dynamically
add to this one if needed.

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

Sure, why not?

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

If it returns NULL then any future calls to debugfs will also not be
working, so all will be fine.  So there is no need to check this.

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

If that makes more sense, it's up to you.  Just a suggestion.

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

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-ker...@vger.kernel.org; 
linux-usb@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-usb@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

[Jun Li]

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

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-ker...@vger.kernel.org; linux-usb@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

[Greg KH]

On Wed, Jan 10, 2018 at 02:59:12PM +0800, ShuFanLee wrote:
> 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 

Minor review of your main structure and your debugfs code and other
stuff, all of which need work:

> ---
>  .../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 */
> +};

dts stuff needs to always be in a separate file so the DT maintainers
can review/ack it.  Split this patch up into smaller pieces please.


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

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

[Greg KH]

On Wed, Jan 10, 2018 at 02:59:12PM +0800, ShuFanLee wrote:
> +static inline int rt1711h_reg_write_word(struct rt1711h_chip *chip, uint8_t 
> reg,
> + uint16_t data)
> +{
> + data = cpu_to_le16(data);
> + return rt1711h_reg_block_write(chip, reg, 2, (uint8_t *));
> +}

Did you run sparse on this code?  What are you doing casting the types
all over the place for data?  That does not seem correct at all.

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

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

[Greg KH]

On Wed, Jan 10, 2018 at 02:59:12PM +0800, ShuFanLee wrote:
> +static inline void rt1711h_poll_ctrl(struct rt1711h_chip *chip)
> +{
> + cancel_delayed_work_sync(>poll_work);
> +
> + if (atomic_read(>poll_count) == 0) {
> + atomic_inc(>poll_count);
> + cpu_idle_poll_ctrl(true);
> + }
> +
> + schedule_delayed_work(>poll_work, msecs_to_jiffies(40));
> +}

This is very odd, and not good.  What are you trying to do here?  And
why are you thinking that poll_count should be an atomic variable?

This feels really strange, and not something you should be doing in an
irq handler, right?

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

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

[Greg KH]

On Wed, Jan 17, 2018 at 02:00:28PM +0200, Heikki Krogerus wrote:
> Hi Greg,
> 
> On Wed, Jan 17, 2018 at 12:14:02PM +0100, Greg KH wrote:
> > On Wed, Jan 17, 2018 at 01:08:58PM +0200, Heikki Krogerus wrote:
> > > Hi,
> > > 
> > > On Wed, Jan 17, 2018 at 09:30:45AM +, shufan_lee(?) wrote:
> > > > Dear Heikki,
> > > > 
> > > >   Sorry for bothering.
> > > > 
> > > >   Just want to check is there anything we need to modify?
> > > 
> > > I'll check the patch this week, but please note that we are -rc8, so
> > > nothing is going to happen before -rc1 is out.
> > 
> > If you ack it today, I could queue it up for -rc1 as it is a stand-alone
> > driver...
> 
> The driver does not compile as module:
> 
> ERROR: "cpu_idle_poll_ctrl" [drivers/usb/typec/rt1711h/rt1711h.ko] undefined!

Well of course it has to pass review :)

Why wouuld a driver be calling that function?  That's not right at all,
ick...

Shufan, what are you doing there?

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

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

[Heikki Krogerus]

Hi Greg,

On Wed, Jan 17, 2018 at 12:14:02PM +0100, Greg KH wrote:
> On Wed, Jan 17, 2018 at 01:08:58PM +0200, Heikki Krogerus wrote:
> > Hi,
> > 
> > On Wed, Jan 17, 2018 at 09:30:45AM +, shufan_lee(?) wrote:
> > > Dear Heikki,
> > > 
> > >   Sorry for bothering.
> > > 
> > >   Just want to check is there anything we need to modify?
> > 
> > I'll check the patch this week, but please note that we are -rc8, so
> > nothing is going to happen before -rc1 is out.
> 
> If you ack it today, I could queue it up for -rc1 as it is a stand-alone
> driver...

The driver does not compile as module:

ERROR: "cpu_idle_poll_ctrl" [drivers/usb/typec/rt1711h/rt1711h.ko] undefined!


Br,

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

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

[Greg KH]

On Wed, Jan 17, 2018 at 01:08:58PM +0200, Heikki Krogerus wrote:
> Hi,
> 
> On Wed, Jan 17, 2018 at 09:30:45AM +, shufan_lee(?) wrote:
> > Dear Heikki,
> > 
> >   Sorry for bothering.
> > 
> >   Just want to check is there anything we need to modify?
> 
> I'll check the patch this week, but please note that we are -rc8, so
> nothing is going to happen before -rc1 is out.

If you ack it today, I could queue it up for -rc1 as it is a stand-alone
driver...

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

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

[Heikki Krogerus]

Hi,

On Wed, Jan 17, 2018 at 09:30:45AM +, shufan_lee(?) wrote:
> Dear Heikki,
> 
>   Sorry for bothering.
> 
>   Just want to check is there anything we need to modify?

I'll check the patch this week, but please note that we are -rc8, so
nothing is going to happen before -rc1 is out.


Br,

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

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-ker...@vger.kernel.org; 
linux-usb@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/usb/typec/rt1711h/Kconfig 
b/drivers/usb/typec/rt1711h/Kconfig
new file mode 100644
index 000..2fbfff5
---