| Message ID | 20190506140830.25376-3-angus@akkea.ca (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: typec: tcpm: Add some FAULT_STATUS processing | expand |
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 ? > + 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 >
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 >>
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) { + 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; + } + + if (scale != 1) + 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); + + if (IS_ERR(ret)) + tcpm_log(tcpci->port, "Can't read VBUS voltage"); + else + tcpm_log(tcpci->port, "Invalid VBUS voltage %d", ret); + } + return IRQ_HANDLED; } EXPORT_SYMBOL_GPL(tcpci_irq);
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> --- drivers/usb/typec/tcpm/tcpci.c | 44 ++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)