Re: [PATCH 2/3] usb: typec: tcpm: Add functions to read the VBUS voltage

[Guenter Roeck]

On Mon, May 06, 2019 at 08:08:29AM -0600, Angus Ainslie (Purism) wrote:
> Put some diagnostics in the tcpm log when there's an over
> or under voltage situation.
> 
> Signed-off-by: Angus Ainslie (Purism) <angus@xxxxxxxx>

Subject is missing 'tcpci'.

> ---
>  drivers/usb/typec/tcpm/tcpci.c | 44 ++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> index c1f7073a56de..c6e0e48b9a2a 100644
> --- a/drivers/usb/typec/tcpm/tcpci.c
> +++ b/drivers/usb/typec/tcpm/tcpci.c
> @@ -261,6 +261,39 @@ static int tcpci_set_pd_rx(struct tcpc_dev *tcpc, bool enable)
>  	return 0;
>  }
>  
> +static int tcpci_get_vbus_voltage(struct tcpc_dev *tcpc)
> +{
> +	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> +	u16 vbus_reg;
> +	unsigned int vbus_voltage;
> +	int ret, scale;
> +
> +	ret = tcpci_read16(tcpci, TCPC_VBUS_VOLTAGE, &vbus_reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	vbus_voltage = vbus_reg & 0x3f;
> +	switch ((ret >> 10) & 3) {

Did you test this code ?

> +	case 0:
> +		scale = 1;
> +		break;
> +	case 1:
> +		scale = 2;
> +		break;
> +	case 2:
> +		scale = 4;
> +		break;
> +	case 3:
> +		tcpm_log(tcpci->port, "invalid VBUS scale");
> +		return -1;

Any special reason for not using standard error codes ?
The code above does, meaning this is a hardcodesd -EPERM, which doesn't
really make any sense.

> +	}
> +
> +	if (scale != 1)
> +		vbus_voltage *= scale;

I don't immediately see why this is better than, say,

	scale = (vbus_reg >> 10) & 3;
	if (scale == 3)
		return -Esomething;	// -EPROTO, maybe
	return vbus_voltage << scale;

> +
> +	return vbus_voltage;
> +}
> +
>  static int tcpci_get_vbus(struct tcpc_dev *tcpc)
>  {
>  	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> @@ -463,6 +496,17 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
>  	else if (status & TCPC_ALERT_TX_FAILED)
>  		tcpm_pd_transmit_complete(tcpci->port, TCPC_TX_FAILED);
>  
> +	if (status & (TCPC_ALERT_V_ALARM_LO | TCPC_ALERT_V_ALARM_HI)) {
> +		int ret;
> +
> +		ret = tcpci_get_vbus_voltage(&tcpci->tcpc);
> +
Unnecessary empty line.

> +		if (IS_ERR(ret))
> +			tcpm_log(tcpci->port, "Can't read VBUS voltage");

VBUS_VOLTAGE is an optional register. This is not an error. Besides, the
message doesn't match the event and is useless.

> +		else
> +			tcpm_log(tcpci->port, "Invalid VBUS voltage %d", ret);

Displaying a raw number without context is not very useful.
'ret' is the voltage in multiples of 25mV. Besides, the error is that a low
or high voltage was detected. That doesn't mean the voltage is still invalid.
The error message should reflect that situation. Something like

		"VBUS {low, high} detected, VBUS=x.yy V"

would be much more useful (with VBUS=x.yy being optional).

Also, please no tcpm log. The tcpci driver needs to implement
its own logging if that is desired.

> +	}
> +
>  	return IRQ_HANDLED;
>  }
>  EXPORT_SYMBOL_GPL(tcpci_irq);
> -- 
> 2.17.1
>