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

[Guenter Roeck <linux@roeck-us.net>]

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