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

[Angus Ainslie <angus@akkea.ca>]

On 2019-05-06 10:20, Guenter Roeck wrote:
> 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@akkea.ca>
> 
> 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 ?
> 

It turned out this wasn't how the device was failing so the code path 
never got executed. I'll figure out how to get it to run before v2.

>> +	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.
> 

Ok I'll find a better return value.

>> +	}
>> +
>> +	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;
> 

That looks more concise than what I can up with.

>> +
>> +	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.
> 

Ok I'll clear up the logging.

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