Messages in this thread | | | Subject | Re: [PATCH 2/3] usb: typec: tcpm: Add functions to read the VBUS voltage | Date | Tue, 07 May 2019 06:20:32 -0600 | From | 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@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 >>
| |