Re: [PATCH 02/14 v2] usb: typec: tcpci: Add support when hidden tx registers are inaccessible

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

Looks like you forgot v1 somewhere :-)

On Mon, Aug 24, 2020 at 09:21:58PM -0700, Badhri Jagan Sridharan wrote:
> TCPCI spec forbids direct access of TX_BUF_BYTE_x register.
> The existing version of tcpci driver assumes that those registers
> are directly addressible. Add support for tcpci chips which do
> not support direct access to TX_BUF_BYTE_x registers. TX_BUF_BYTE_x
> can only be accessed by I2C_WRITE_BYTE_COUNT.
> 
> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> ---
>  drivers/usb/typec/tcpm/tcpci.c | 49 +++++++++++++++++++++++++---------
>  drivers/usb/typec/tcpm/tcpci.h |  8 ++++++
>  2 files changed, 44 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> index f57d91fd0e09..90d348caa6a8 100644
> --- a/drivers/usb/typec/tcpm/tcpci.c
> +++ b/drivers/usb/typec/tcpm/tcpci.c
> @@ -320,8 +320,7 @@ static int tcpci_set_vbus(struct tcpc_dev *tcpc, bool source, bool sink)
>  	return 0;
>  }
>  
> -static int tcpci_pd_transmit(struct tcpc_dev *tcpc,
> -			     enum tcpm_transmit_type type,
> +static int tcpci_pd_transmit(struct tcpc_dev *tcpc, enum tcpm_transmit_type type,
>  			     const struct pd_message *msg)

That does not look like a relevant change.

>  {
>  	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> @@ -330,23 +329,47 @@ static int tcpci_pd_transmit(struct tcpc_dev *tcpc,
>  	int ret;
>  
>  	cnt = msg ? pd_header_cnt(header) * 4 : 0;
> -	ret = regmap_write(tcpci->regmap, TCPC_TX_BYTE_CNT, cnt + 2);
> -	if (ret < 0)
> -		return ret;
> +	/**
> +	 * TCPCI spec forbids direct access of TCPC_TX_DATA.
> +	 * But, since some of the chipsets offer this capability,
> +	 * it's fair to support both.
> +	 */
> +	if (!tcpci->data->TX_BUF_BYTE_x_hidden) {

Couldn't check if the flag is set first?

        if (tcpci->data->TX_BUF_BYTE_x_hidden) {
                ...

> +		ret = regmap_write(tcpci->regmap, TCPC_TX_BYTE_CNT, cnt + 2);
> +		if (ret < 0)
> +			return ret;
>  
> -	ret = tcpci_write16(tcpci, TCPC_TX_HDR, header);
> -	if (ret < 0)
> -		return ret;
> +		ret = tcpci_write16(tcpci, TCPC_TX_HDR, header);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (cnt > 0) {
> +			ret = regmap_raw_write(tcpci->regmap, TCPC_TX_DATA, &msg->payload, cnt);
> +			if (ret < 0)
> +				return ret;
> +		}
> +	} else {
> +		u8 buf[TCPC_TRANSMIT_BUFFER_MAX_LEN] = {0,};
> +		u8 pos = 0;
> +
> +		/* Payload + header + TCPC_TX_BYTE_CNT */
> +		buf[pos++] = cnt + 2;
> +
> +		if (msg)
> +			memcpy(&buf[pos], &msg->header, sizeof(msg->header));
> +
> +		pos += sizeof(header);
> +
> +		if (cnt > 0)
> +			memcpy(&buf[pos], msg->payload, cnt);
>  
> -	if (cnt > 0) {
> -		ret = regmap_raw_write(tcpci->regmap, TCPC_TX_DATA,
> -				       &msg->payload, cnt);
> +		pos += cnt;
> +		ret = regmap_raw_write(tcpci->regmap, TCPC_TX_BYTE_CNT, buf, pos);
>  		if (ret < 0)
>  			return ret;
>  	}
>  
> -	reg = (PD_RETRY_COUNT << TCPC_TRANSMIT_RETRY_SHIFT) |
> -		(type << TCPC_TRANSMIT_TYPE_SHIFT);
> +	reg = (PD_RETRY_COUNT << TCPC_TRANSMIT_RETRY_SHIFT) | (type << TCPC_TRANSMIT_TYPE_SHIFT);
>  	ret = regmap_write(tcpci->regmap, TCPC_TRANSMIT, reg);
>  	if (ret < 0)
>  		return ret;
> diff --git a/drivers/usb/typec/tcpm/tcpci.h b/drivers/usb/typec/tcpm/tcpci.h
> index fd26ca35814c..cf9d8b63adcb 100644
> --- a/drivers/usb/typec/tcpm/tcpci.h
> +++ b/drivers/usb/typec/tcpm/tcpci.h
> @@ -128,9 +128,17 @@
>  #define TCPC_VBUS_VOLTAGE_ALARM_HI_CFG		0x76
>  #define TCPC_VBUS_VOLTAGE_ALARM_LO_CFG		0x78
>  
> +/* I2C_WRITE_BYTE_COUNT + 1 when TX_BUF_BYTE_x is only accessible I2C_WRITE_BYTE_COUNT */
> +#define TCPC_TRANSMIT_BUFFER_MAX_LEN		31
> +
> +/*
> + * @TX_BUF_BYTE_x_hidden
> + *		optional; Set when TX_BUF_BYTE_x can only be accessed through I2C_WRITE_BYTE_COUNT.
> + */
>  struct tcpci;
>  struct tcpci_data {
>  	struct regmap *regmap;
> +	unsigned char TX_BUF_BYTE_x_hidden:1;
>  	int (*init)(struct tcpci *tcpci, struct tcpci_data *data);
>  	int (*set_vconn)(struct tcpci *tcpci, struct tcpci_data *data,
>  			 bool enable);
> -- 
> 2.28.0.297.g1956fa8f8d-goog

thanks,

-- 
heikki