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

[shufan_lee(ææå)]

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 *)&tcpci_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:groeck7@xxxxxxxxx] 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@xxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx
Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

On Mon, Jan 29, 2018 at 07:19:06AM +0000, 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 = &tcpci_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
************* 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!