RE: [PATCH] staging: typec: handle vendor defined part and modify drp toggling flow

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

> -----Original Message-----
> From: linux-usb-owner@xxxxxxxxxxxxxxx
> [mailto:linux-usb-owner@xxxxxxxxxxxxxxx] On Behalf Of ShuFanLee
> Sent: 2018年2月21日 23:02
> To: heikki.krogerus@xxxxxxxxxxxxxxx; linux@xxxxxxxxxxxx; greg@xxxxxxxxx
> Cc: shufan_lee@xxxxxxxxxxx; cy_huang@xxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx
> Subject: [PATCH] staging: typec: handle vendor defined part and modify drp
> toggling flow
> 
> From: ShuFanLee <shufan_lee@xxxxxxxxxxx>
> 
> Handle vendor defined behavior in tcpci_init, tcpci_set_vconn and export
> tcpci_irq.
> More operations can be extended in tcpci_data if needed.
> According to TCPCI specification, 4.4.5.2 ROLE_CONTROL, TCPC shall not
> start DRP toggling until subsequently the TCPM writes to the COMMAND
> register to start DRP toggling.

My understanding of above statement is TCPM(this Linux driver) can enable
DRP and CC1/CC2 in one shot, but TCPC(your typec chip internal firmware)
needs wait until TCPM writes to COMMAND register to start drp toggling.

> DRP toggling flow is chagned as following:
>   - Write DRP = 0 & Rd/Rd

Why fixed to be Rd/Rd? in this case, it means the starting value:

Tcpci 4.4.5.2:
"When initiating autonomous DRP toggling, the TCPM shall write B6 (DRP) =1b
and the starting value of Rp/Rd to B3..0 (CC1/CC2) to indicate DRP autonomous
toggling mode to the TCPC."

>   - Write DRP = 1

What's your problem if combine above 2 in one shot?

>   - Set LOOK4CONNECTION command
> 
> Signed-off-by: ShuFanLee <shufan_lee@xxxxxxxxxxx>
> ---
>  drivers/staging/typec/tcpci.c | 128
> +++++++++++++++++++++++++++++++++---------
>  drivers/staging/typec/tcpci.h |  13 +++++
>  2 files changed, 115 insertions(+), 26 deletions(-)
> 
>  patch changelogs between v1 & v2
>  - Remove unnecessary i2c_client in the structure of tcpci
>  - Rename structure of tcpci_vendor_data to tcpci_data
>  - Not exporting tcpci read/write wrappers but register i2c regmap in glue
> driver
>  - Add set_vconn ops in tcpci_data
>    (It is necessary for RT1711H to enable/disable idle mode before
> disabling/enabling vconn)
>  - Export tcpci_irq so that vendor can call it in their own IRQ handler
> 
>  patch changelogs between v2 & v3
>  - Change the return type of tcpci_irq from int to irqreturn_t
> 
> diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
> index 9bd4412..4959c69 100644
> --- a/drivers/staging/typec/tcpci.c
> +++ b/drivers/staging/typec/tcpci.c
> @@ -21,7 +21,6 @@
> 
>  struct tcpci {
>  	struct device *dev;
> -	struct i2c_client *client;
> 
>  	struct tcpm_port *port;
> 
> @@ -30,6 +29,12 @@ struct tcpci {
>  	bool controls_vbus;
> 
>  	struct tcpc_dev tcpc;
> +	struct tcpci_data *data;
> +};
> +
> +struct tcpci_chip {
> +	struct tcpci *tcpci;
> +	struct tcpci_data data;
>  };
> 
>  static inline struct tcpci *tcpc_to_tcpci(struct tcpc_dev *tcpc) @@ -37,8
> +42,7 @@ static inline struct tcpci *tcpc_to_tcpci(struct tcpc_dev *tcpc)
>  	return container_of(tcpc, struct tcpci, tcpc);  }
> 
> -static int tcpci_read16(struct tcpci *tcpci, unsigned int reg,
> -			u16 *val)
> +static int tcpci_read16(struct tcpci *tcpci, unsigned int reg, u16
> +*val)
>  {
>  	return regmap_raw_read(tcpci->regmap, reg, val, sizeof(u16));  } @@
> -98,8 +102,10 @@ static int tcpci_set_cc(struct tcpc_dev *tcpc, enum
> typec_cc_status cc)  static int tcpci_start_drp_toggling(struct tcpc_dev
> *tcpc,
>  				    enum typec_cc_status cc)
>  {
> +	int ret;
>  	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> -	unsigned int reg = TCPC_ROLE_CTRL_DRP;
> +	unsigned int reg = (TCPC_ROLE_CTRL_CC_RD <<
> TCPC_ROLE_CTRL_CC1_SHIFT) |
> +			   (TCPC_ROLE_CTRL_CC_RD <<
> TCPC_ROLE_CTRL_CC2_SHIFT);
> 
>  	switch (cc) {
>  	default:
> @@ -117,7 +123,19 @@ static int tcpci_start_drp_toggling(struct tcpc_dev
> *tcpc,
>  		break;
>  	}
> 
> -	return regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
> +	ret = regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
> +	if (ret < 0)
> +		return ret;
> +	usleep_range(500, 1000);

Why need a wait here? It seems you actually don't want to do autonomously toggling
from the very beginning, instead, you begin with a directly control on CC, keep it(Rd)
for some time, then switch to use autonomously toggling, why you need such kind of
sequence? I think it's special and not following tcpci spec.
 
> +	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;
>  }
> 
>  static enum typec_cc_status tcpci_to_typec_cc(unsigned int cc, bool sink)
> @@ -178,6 +196,16 @@ static int tcpci_set_vconn(struct tcpc_dev *tcpc,
> bool enable)
>  	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
>  	int ret;
> 
> +	/* Handle vendor set vconn */
> +	if (tcpci->data) {
> +		if (tcpci->data->set_vconn) {
> +			ret = tcpci->data->set_vconn(tcpci, tcpci->data,
> +						     enable);
> +			if (ret < 0)
> +				return ret;
> +		}
> +	}
> +
>  	ret = regmap_write(tcpci->regmap, TCPC_POWER_CTRL,
>  			   enable ? TCPC_POWER_CTRL_VCONN_ENABLE : 0);
>  	if (ret < 0)
> @@ -323,6 +351,15 @@ static int tcpci_init(struct tcpc_dev *tcpc)
>  	if (time_after(jiffies, timeout))
>  		return -ETIMEDOUT;
> 
> +	/* Handle vendor init */
> +	if (tcpci->data) {
> +		if (tcpci->data->init) {
> +			ret = tcpci->data->init(tcpci, tcpci->data);
> +			if (ret < 0)
> +				return ret;
> +		}
> +	}
> +
>  	/* Clear all events */
>  	ret = tcpci_write16(tcpci, TCPC_ALERT, 0xffff);
>  	if (ret < 0)
> @@ -344,9 +381,15 @@ static int tcpci_init(struct tcpc_dev *tcpc)
>  	return tcpci_write16(tcpci, TCPC_ALERT_MASK, reg);  }
> 
> -static irqreturn_t tcpci_irq(int irq, void *dev_id)
> +static irqreturn_t _tcpci_irq(int irq, void *dev_id)
>  {
>  	struct tcpci *tcpci = dev_id;
> +
> +	return tcpci_irq(tcpci);
> +}
> +
> +irqreturn_t tcpci_irq(struct tcpci *tcpci) {
>  	u16 status;
> 
>  	tcpci_read16(tcpci, TCPC_ALERT, &status); @@ -412,6 +455,7 @@
> static irqreturn_t tcpci_irq(int irq, void *dev_id)
> 
>  	return IRQ_HANDLED;
>  }
> +EXPORT_SYMBOL_GPL(tcpci_irq);

I don't catch the point of how you handle private events by above change,
maybe post your RT1711H part as a user in one series can make it clear,
I assume this can be done in existing tcpci_irq like above vender specific
handling as well:
      tcpci->data->priv_events(tcpci, tcpci->data);

Li Jun
> 
>  static const struct regmap_config tcpci_regmap_config = {
>  	.reg_bits = 8,
> @@ -435,22 +479,18 @@ static int tcpci_parse_config(struct tcpci *tcpci)
>  	return 0;
>  }
> 
> -static int tcpci_probe(struct i2c_client *client,
> -		       const struct i2c_device_id *i2c_id)
> +struct tcpci *tcpci_register_port(struct device *dev, struct tcpci_data
> +*data)
>  {
>  	struct tcpci *tcpci;
>  	int err;
> 
> -	tcpci = devm_kzalloc(&client->dev, sizeof(*tcpci), GFP_KERNEL);
> +	tcpci = devm_kzalloc(dev, sizeof(*tcpci), GFP_KERNEL);
>  	if (!tcpci)
> -		return -ENOMEM;
> +		return ERR_PTR(-ENOMEM);
> 
> -	tcpci->client = client;
> -	tcpci->dev = &client->dev;
> -	i2c_set_clientdata(client, tcpci);
> -	tcpci->regmap = devm_regmap_init_i2c(client, &tcpci_regmap_config);
> -	if (IS_ERR(tcpci->regmap))
> -		return PTR_ERR(tcpci->regmap);
> +	tcpci->dev = dev;
> +	tcpci->data = data;
> +	tcpci->regmap = data->regmap;
> 
>  	tcpci->tcpc.init = tcpci_init;
>  	tcpci->tcpc.get_vbus = tcpci_get_vbus; @@ -467,27 +507,63 @@ static
> int tcpci_probe(struct i2c_client *client,
> 
>  	err = tcpci_parse_config(tcpci);
>  	if (err < 0)
> -		return err;
> +		return ERR_PTR(err);
> +
> +	tcpci->port = tcpm_register_port(tcpci->dev, &tcpci->tcpc);
> +	if (PTR_ERR_OR_ZERO(tcpci->port))
> +		return ERR_CAST(tcpci->port);
> 
> -	/* Disable chip interrupts */
> -	tcpci_write16(tcpci, TCPC_ALERT_MASK, 0);
> +	return tcpci;
> +}
> +EXPORT_SYMBOL_GPL(tcpci_register_port);
> +
> +void tcpci_unregister_port(struct tcpci *tcpci) {
> +	tcpm_unregister_port(tcpci->port);
> +}
> +EXPORT_SYMBOL_GPL(tcpci_unregister_port);
> 
> -	err = devm_request_threaded_irq(tcpci->dev, client->irq, NULL,
> -					tcpci_irq,
> +static int tcpci_probe(struct i2c_client *client,
> +		       const struct i2c_device_id *i2c_id) {
> +	struct tcpci_chip *chip;
> +	int err;
> +	u16 val = 0;
> +
> +	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
> +	if (!chip)
> +		return -ENOMEM;
> +
> +	chip->data.regmap = devm_regmap_init_i2c(client,
> &tcpci_regmap_config);
> +	if (IS_ERR(chip->data.regmap))
> +		return PTR_ERR(chip->data.regmap);
> +
> +	/* Disable chip interrupts before requesting irq */
> +	err = regmap_raw_write(chip->data.regmap, TCPC_ALERT_MASK, &val,
> +			       sizeof(u16));
> +	if (err < 0)
> +		return err;
> +
> +	err = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> +					_tcpci_irq,
>  					IRQF_ONESHOT | IRQF_TRIGGER_LOW,
> -					dev_name(tcpci->dev), tcpci);
> +					dev_name(&client->dev), chip);
>  	if (err < 0)
>  		return err;
> 
> -	tcpci->port = tcpm_register_port(tcpci->dev, &tcpci->tcpc);
> -	return PTR_ERR_OR_ZERO(tcpci->port);
> +	chip->tcpci = tcpci_register_port(&client->dev, &chip->data);
> +	if (PTR_ERR_OR_ZERO(chip->tcpci))
> +		return PTR_ERR(chip->tcpci);
> +
> +	i2c_set_clientdata(client, chip);
> +	return 0;
>  }
> 
>  static int tcpci_remove(struct i2c_client *client)  {
> -	struct tcpci *tcpci = i2c_get_clientdata(client);
> +	struct tcpci_chip *chip = i2c_get_clientdata(client);
> 
> -	tcpm_unregister_port(tcpci->port);
> +	tcpci_unregister_port(chip->tcpci);
> 
>  	return 0;
>  }
> diff --git a/drivers/staging/typec/tcpci.h b/drivers/staging/typec/tcpci.h
> index fdfb06c..40025b2 100644
> --- a/drivers/staging/typec/tcpci.h
> +++ b/drivers/staging/typec/tcpci.h
> @@ -59,6 +59,7 @@
>  #define TCPC_POWER_CTRL_VCONN_ENABLE	BIT(0)
> 
>  #define TCPC_CC_STATUS			0x1d
> +#define TCPC_CC_STATUS_DRPRST		BIT(5)
>  #define TCPC_CC_STATUS_TERM		BIT(4)
>  #define TCPC_CC_STATUS_CC2_SHIFT	2
>  #define TCPC_CC_STATUS_CC2_MASK		0x3
> @@ -121,4 +122,16 @@
>  #define TCPC_VBUS_VOLTAGE_ALARM_HI_CFG		0x76
>  #define TCPC_VBUS_VOLTAGE_ALARM_LO_CFG		0x78
> 
> +struct tcpci;
> +struct tcpci_data {
> +	struct regmap *regmap;
> +	int (*init)(struct tcpci *tcpci, struct tcpci_data *data);
> +	int (*set_vconn)(struct tcpci *tcpci, struct tcpci_data *data,
> +			 bool enable);
> +};
> +
> +struct tcpci *tcpci_register_port(struct device *dev, struct tcpci_data
> +*data); void tcpci_unregister_port(struct tcpci *tcpci); irqreturn_t
> +tcpci_irq(struct tcpci *tcpci);
> +
>  #endif /* __LINUX_USB_TCPCI_H */
> --
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in the
> body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvger
> .kernel.org%2Fmajordomo-info.html&data=02%7C01%7Cjun.li%40nxp.com
> %7C44d11af27e0244fa4d1108d5793c3dc4%7C686ea1d3bc2b4c6fa92cd99c
> 5c301635%7C0%7C1%7C636548222008905206&sdata=zn2ougKRvs%2BbmH
> Qg2c8Rjj7vNe0ZiiVKdar7GdPa%2B3o%3D&reserved=0
?韬{.n?????%??檩??w?{.n???{炳???骅w*jg????????G??⒏⒎?:+v????????????"??????



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux