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