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

[Angus Ainslie]

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@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 ?

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