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
