lkml.org 
[lkml]   [2019]   [May]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2/3] usb: typec: tcpm: Add functions to read the VBUS voltage
Date
From
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
>>

\
 
 \ /
  Last update: 2019-05-07 14:21    [W:0.038 / U:0.064 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site