[PATCH v2 1/2] usb: typec: tcpci: add check code for tcpci/regmap_read/write()

[PATCH v2 1/2] usb: typec: tcpci: add check code for tcpci/regmap_read/write()

[Xu Yang]

The return value from tcpci/regmap_read/write() must be checked to get
rid of the bad influence of other modules. This will add check code for
all of the rest read/write() callbacks and will show error when failed
to get ALERT register.

Signed-off-by: Xu Yang <xu.yang_2@nxp.com>

---
Changes in v2:
 - remove printing code
---
 drivers/usb/typec/tcpm/tcpci.c | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
index 0ee3e6e29bb1..8ccc2d1a8ffc 100644
--- a/drivers/usb/typec/tcpm/tcpci.c
+++ b/drivers/usb/typec/tcpm/tcpci.c
@@ -657,21 +657,28 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
 	int ret;
 	unsigned int raw;
 
-	tcpci_read16(tcpci, TCPC_ALERT, &status);
+	ret = tcpci_read16(tcpci, TCPC_ALERT, &status);
+	if (ret < 0)
+		return ret;
 
 	/*
 	 * Clear alert status for everything except RX_STATUS, which shouldn't
 	 * be cleared until we have successfully retrieved message.
 	 */
-	if (status & ~TCPC_ALERT_RX_STATUS)
-		tcpci_write16(tcpci, TCPC_ALERT,
+	if (status & ~TCPC_ALERT_RX_STATUS) {
+		ret = tcpci_write16(tcpci, TCPC_ALERT,
 			      status & ~TCPC_ALERT_RX_STATUS);
+		if (ret < 0)
+			return ret;
+	}
 
 	if (status & TCPC_ALERT_CC_STATUS)
 		tcpm_cc_change(tcpci->port);
 
 	if (status & TCPC_ALERT_POWER_STATUS) {
-		regmap_read(tcpci->regmap, TCPC_POWER_STATUS_MASK, &raw);
+		ret = regmap_read(tcpci->regmap, TCPC_POWER_STATUS_MASK, &raw);
+		if (ret < 0)
+			return ret;
 		/*
 		 * If power status mask has been reset, then the TCPC
 		 * has reset.
@@ -687,7 +694,9 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
 		unsigned int cnt, payload_cnt;
 		u16 header;
 
-		regmap_read(tcpci->regmap, TCPC_RX_BYTE_CNT, &cnt);
+		ret = regmap_read(tcpci->regmap, TCPC_RX_BYTE_CNT, &cnt);
+		if (ret < 0)
+			return ret;
 		/*
 		 * 'cnt' corresponds to READABLE_BYTE_COUNT in section 4.4.14
 		 * of the TCPCI spec [Rev 2.0 Ver 1.0 October 2017] and is
@@ -699,18 +708,25 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
 		else
 			payload_cnt = 0;
 
-		tcpci_read16(tcpci, TCPC_RX_HDR, &header);
+		ret = tcpci_read16(tcpci, TCPC_RX_HDR, &header);
+		if (ret < 0)
+			return ret;
 		msg.header = cpu_to_le16(header);
 
 		if (WARN_ON(payload_cnt > sizeof(msg.payload)))
 			payload_cnt = sizeof(msg.payload);
 
-		if (payload_cnt > 0)
-			regmap_raw_read(tcpci->regmap, TCPC_RX_DATA,
+		if (payload_cnt > 0) {
+			ret = regmap_raw_read(tcpci->regmap, TCPC_RX_DATA,
 					&msg.payload, payload_cnt);
+			if (ret < 0)
+				return ret;
+		}
 
 		/* Read complete, clear RX status alert bit */
-		tcpci_write16(tcpci, TCPC_ALERT, TCPC_ALERT_RX_STATUS);
+		ret = tcpci_write16(tcpci, TCPC_ALERT, TCPC_ALERT_RX_STATUS);
+		if (ret < 0)
+			return ret;
 
 		tcpm_pd_receive(tcpci->port, &msg);
 	}
-- 
2.34.1

[PATCH v2 2/2] usb: typec: tcpci: enable vSafe0v Detection and Auto Discharge Disconnect for PTN5110

[Xu Yang]

PTN5110 itself supports vSafe0V detection and auto discharge disconnect
capabilities. This will enable these feature.

Signed-off-by: Xu Yang <xu.yang_2@nxp.com>

---
Changes in v2:
 - no changes
---
 drivers/usb/typec/tcpm/tcpci.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
index 8ccc2d1a8ffc..141d313b9a55 100644
--- a/drivers/usb/typec/tcpm/tcpci.c
+++ b/drivers/usb/typec/tcpm/tcpci.c
@@ -861,6 +861,9 @@ static int tcpci_probe(struct i2c_client *client)
 
 	i2c_set_clientdata(client, chip);
 
+	chip->data.vbus_vsafe0v = 1;
+	chip->data.auto_discharge_disconnect = 1;
+
 	/* Disable chip interrupts before requesting irq */
 	err = regmap_raw_write(chip->data.regmap, TCPC_ALERT_MASK, &val,
 			       sizeof(u16));
-- 
2.34.1

Re: [PATCH v2 1/2] usb: typec: tcpci: add check code for tcpci/regmap_read/write()

[Heikki Krogerus]

On Thu, Sep 14, 2023 at 08:11:57PM +0800, Xu Yang wrote:
> The return value from tcpci/regmap_read/write() must be checked to get
> rid of the bad influence of other modules. This will add check code for
> all of the rest read/write() callbacks and will show error when failed
> to get ALERT register.
> 
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> 
> ---
> Changes in v2:
>  - remove printing code
> ---
>  drivers/usb/typec/tcpm/tcpci.c | 34 +++++++++++++++++++++++++---------
>  1 file changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> index 0ee3e6e29bb1..8ccc2d1a8ffc 100644
> --- a/drivers/usb/typec/tcpm/tcpci.c
> +++ b/drivers/usb/typec/tcpm/tcpci.c
> @@ -657,21 +657,28 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
>  	int ret;
>  	unsigned int raw;
>  
> -	tcpci_read16(tcpci, TCPC_ALERT, &status);
> +	ret = tcpci_read16(tcpci, TCPC_ALERT, &status);
> +	if (ret < 0)
> +		return ret;
>  
>  	/*
>  	 * Clear alert status for everything except RX_STATUS, which shouldn't
>  	 * be cleared until we have successfully retrieved message.
>  	 */
> -	if (status & ~TCPC_ALERT_RX_STATUS)
> -		tcpci_write16(tcpci, TCPC_ALERT,
> +	if (status & ~TCPC_ALERT_RX_STATUS) {
> +		ret = tcpci_write16(tcpci, TCPC_ALERT,
>  			      status & ~TCPC_ALERT_RX_STATUS);
> +		if (ret < 0)
> +			return ret;
> +	}
>  
>  	if (status & TCPC_ALERT_CC_STATUS)
>  		tcpm_cc_change(tcpci->port);
>  
>  	if (status & TCPC_ALERT_POWER_STATUS) {
> -		regmap_read(tcpci->regmap, TCPC_POWER_STATUS_MASK, &raw);
> +		ret = regmap_read(tcpci->regmap, TCPC_POWER_STATUS_MASK, &raw);
> +		if (ret < 0)
> +			return ret;
>  		/*
>  		 * If power status mask has been reset, then the TCPC
>  		 * has reset.
> @@ -687,7 +694,9 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
>  		unsigned int cnt, payload_cnt;
>  		u16 header;
>  
> -		regmap_read(tcpci->regmap, TCPC_RX_BYTE_CNT, &cnt);
> +		ret = regmap_read(tcpci->regmap, TCPC_RX_BYTE_CNT, &cnt);
> +		if (ret < 0)
> +			return ret;

I think you still need to clear TCPC_ALERT_RX_STATUS in this case.
Guenter, can you check this?

>  		/*
>  		 * 'cnt' corresponds to READABLE_BYTE_COUNT in section 4.4.14
>  		 * of the TCPCI spec [Rev 2.0 Ver 1.0 October 2017] and is
> @@ -699,18 +708,25 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
>  		else
>  			payload_cnt = 0;
>  
> -		tcpci_read16(tcpci, TCPC_RX_HDR, &header);
> +		ret = tcpci_read16(tcpci, TCPC_RX_HDR, &header);
> +		if (ret < 0)
> +			return ret;
>  		msg.header = cpu_to_le16(header);
>  
>  		if (WARN_ON(payload_cnt > sizeof(msg.payload)))
>  			payload_cnt = sizeof(msg.payload);
>  
> -		if (payload_cnt > 0)
> -			regmap_raw_read(tcpci->regmap, TCPC_RX_DATA,
> +		if (payload_cnt > 0) {
> +			ret = regmap_raw_read(tcpci->regmap, TCPC_RX_DATA,
>  					&msg.payload, payload_cnt);
> +			if (ret < 0)
> +				return ret;
> +		}
>  
>  		/* Read complete, clear RX status alert bit */
> -		tcpci_write16(tcpci, TCPC_ALERT, TCPC_ALERT_RX_STATUS);
> +		ret = tcpci_write16(tcpci, TCPC_ALERT, TCPC_ALERT_RX_STATUS);
> +		if (ret < 0)
> +			return ret;
>  
>  		tcpm_pd_receive(tcpci->port, &msg);
>  	}
> -- 
> 2.34.1

-- 
heikki

Re: [PATCH v2 1/2] usb: typec: tcpci: add check code for tcpci/regmap_read/write()

[Guenter Roeck]

On 9/18/23 03:26, Heikki Krogerus wrote:
> On Thu, Sep 14, 2023 at 08:11:57PM +0800, Xu Yang wrote:
>> The return value from tcpci/regmap_read/write() must be checked to get
>> rid of the bad influence of other modules. This will add check code for
>> all of the rest read/write() callbacks and will show error when failed
>> to get ALERT register.
>>
>> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
>>
>> ---
>> Changes in v2:
>>   - remove printing code
>> ---
>>   drivers/usb/typec/tcpm/tcpci.c | 34 +++++++++++++++++++++++++---------
>>   1 file changed, 25 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
>> index 0ee3e6e29bb1..8ccc2d1a8ffc 100644
>> --- a/drivers/usb/typec/tcpm/tcpci.c
>> +++ b/drivers/usb/typec/tcpm/tcpci.c
>> @@ -657,21 +657,28 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
>>   	int ret;
>>   	unsigned int raw;
>>   
>> -	tcpci_read16(tcpci, TCPC_ALERT, &status);
>> +	ret = tcpci_read16(tcpci, TCPC_ALERT, &status);
>> +	if (ret < 0)
>> +		return ret;
>>   
>>   	/*
>>   	 * Clear alert status for everything except RX_STATUS, which shouldn't
>>   	 * be cleared until we have successfully retrieved message.
>>   	 */
>> -	if (status & ~TCPC_ALERT_RX_STATUS)
>> -		tcpci_write16(tcpci, TCPC_ALERT,
>> +	if (status & ~TCPC_ALERT_RX_STATUS) {
>> +		ret = tcpci_write16(tcpci, TCPC_ALERT,
>>   			      status & ~TCPC_ALERT_RX_STATUS);
>> +		if (ret < 0)
>> +			return ret;
>> +	}
>>   
>>   	if (status & TCPC_ALERT_CC_STATUS)
>>   		tcpm_cc_change(tcpci->port);
>>   
>>   	if (status & TCPC_ALERT_POWER_STATUS) {
>> -		regmap_read(tcpci->regmap, TCPC_POWER_STATUS_MASK, &raw);
>> +		ret = regmap_read(tcpci->regmap, TCPC_POWER_STATUS_MASK, &raw);
>> +		if (ret < 0)
>> +			return ret;
>>   		/*
>>   		 * If power status mask has been reset, then the TCPC
>>   		 * has reset.
>> @@ -687,7 +694,9 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
>>   		unsigned int cnt, payload_cnt;
>>   		u16 header;
>>   
>> -		regmap_read(tcpci->regmap, TCPC_RX_BYTE_CNT, &cnt);
>> +		ret = regmap_read(tcpci->regmap, TCPC_RX_BYTE_CNT, &cnt);
>> +		if (ret < 0)
>> +			return ret;
> 
> I think you still need to clear TCPC_ALERT_RX_STATUS in this case.
> Guenter, can you check this?
> 

If reading from or writing to the status register failed, we are pretty
much messed up anyway, so I don't think it really matters.

I think the more severe problem is that this is an interrupt handler,
which either handles the interrupt or it doesn't. It does not have the
option of returning an error (negative error code).

The submitter will have to decide what to do in the error case: Was
the interrupt handled or not ? I have no real answer to that question; all
answers seem wrong to me. I would tend to returning that the interrupt
was not handled. Most likely that would cause the handler to be called
again because the interrupt condition is not cleared. If this happens
repeatedly, the interrupt might end up being disabled, which would probably
be the best possible outcome.

Guenter

RE: [EXT] Re: [PATCH v2 1/2] usb: typec: tcpci: add check code for tcpci/regmap_read/write()

[Xu Yang]

Hi Heikki & Guenter,

> On 9/18/23 03:26, Heikki Krogerus wrote:
> > On Thu, Sep 14, 2023 at 08:11:57PM +0800, Xu Yang wrote:
> >> The return value from tcpci/regmap_read/write() must be checked to get
> >> rid of the bad influence of other modules. This will add check code for
> >> all of the rest read/write() callbacks and will show error when failed
> >> to get ALERT register.
> >>
> >> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> >>
> >> ---
> >> Changes in v2:
> >>   - remove printing code
> >> ---
> >>   drivers/usb/typec/tcpm/tcpci.c | 34 +++++++++++++++++++++++++---------
> >>   1 file changed, 25 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> >> index 0ee3e6e29bb1..8ccc2d1a8ffc 100644
> >> --- a/drivers/usb/typec/tcpm/tcpci.c
> >> +++ b/drivers/usb/typec/tcpm/tcpci.c
> >> @@ -657,21 +657,28 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
> >>      int ret;
> >>      unsigned int raw;
> >>
> >> -    tcpci_read16(tcpci, TCPC_ALERT, &status);
> >> +    ret = tcpci_read16(tcpci, TCPC_ALERT, &status);
> >> +    if (ret < 0)
> >> +            return ret;
> >>
> >>      /*
> >>       * Clear alert status for everything except RX_STATUS, which shouldn't
> >>       * be cleared until we have successfully retrieved message.
> >>       */
> >> -    if (status & ~TCPC_ALERT_RX_STATUS)
> >> -            tcpci_write16(tcpci, TCPC_ALERT,
> >> +    if (status & ~TCPC_ALERT_RX_STATUS) {
> >> +            ret = tcpci_write16(tcpci, TCPC_ALERT,
> >>                            status & ~TCPC_ALERT_RX_STATUS);
> >> +            if (ret < 0)
> >> +                    return ret;
> >> +    }
> >>
> >>      if (status & TCPC_ALERT_CC_STATUS)
> >>              tcpm_cc_change(tcpci->port);
> >>
> >>      if (status & TCPC_ALERT_POWER_STATUS) {
> >> -            regmap_read(tcpci->regmap, TCPC_POWER_STATUS_MASK, &raw);
> >> +            ret = regmap_read(tcpci->regmap, TCPC_POWER_STATUS_MASK, &raw);
> >> +            if (ret < 0)
> >> +                    return ret;
> >>              /*
> >>               * If power status mask has been reset, then the TCPC
> >>               * has reset.
> >> @@ -687,7 +694,9 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
> >>              unsigned int cnt, payload_cnt;
> >>              u16 header;
> >>
> >> -            regmap_read(tcpci->regmap, TCPC_RX_BYTE_CNT, &cnt);
> >> +            ret = regmap_read(tcpci->regmap, TCPC_RX_BYTE_CNT, &cnt);
> >> +            if (ret < 0)
> >> +                    return ret;
> >
> > I think you still need to clear TCPC_ALERT_RX_STATUS in this case.
> > Guenter, can you check this?
> >
> 
> If reading from or writing to the status register failed, we are pretty
> much messed up anyway, so I don't think it really matters.
> 
> I think the more severe problem is that this is an interrupt handler,
> which either handles the interrupt or it doesn't. It does not have the

Yes, I agree.

> option of returning an error (negative error code).

Normally speaking, it means the device has successfully handled the
interrupt event if an interrupt handler return IRQ_HANDLED, and the
interrupt event doesn't belong to this device or not be handled if the
interrupt handler return IRQ_NONE. However, the irq core will regard
an negative error code as IRQ_NONE as far as I know. And when IRQ_NONE
or a an negative error code is returned, irq core will judge if the irq
is bad or if there is need to disable this irq. Therefore, the irq handler
returns an negative error code should be ok if it's threaded.
I just do the same thing as max_tcpci_irq() in tcpci_maxim_core.c.

> 
> The submitter will have to decide what to do in the error case: Was
> the interrupt handled or not ? I have no real answer to that question; all
> answers seem wrong to me. I would tend to returning that the interrupt
> was not handled. Most likely that would cause the handler to be called
> again because the interrupt condition is not cleared. If this happens
> repeatedly, the interrupt might end up being disabled, which would probably
> be the best possible outcome.

When error occurs, I just know the tcpci irq handler should not continue
to handle events since read/write() is not successful. Otherwise, we will
see unexpected behaviors. If the error doesn't disappear in time, then irq
core will disable the irq as usual. If the hw or i2c adapter driver has
been recovered from a short fault state, then the tcpci driver will work as
usual too.

Thanks,
Xu Yang

> 
> Guenter

Re: [EXT] Re: [PATCH v2 1/2] usb: typec: tcpci: add check code for tcpci/regmap_read/write()

[Guenter Roeck]

On 9/21/23 23:47, Xu Yang wrote:
> Hi Heikki & Guenter,
> 
>> On 9/18/23 03:26, Heikki Krogerus wrote:
>>> On Thu, Sep 14, 2023 at 08:11:57PM +0800, Xu Yang wrote:
>>>> The return value from tcpci/regmap_read/write() must be checked to get
>>>> rid of the bad influence of other modules. This will add check code for
>>>> all of the rest read/write() callbacks and will show error when failed
>>>> to get ALERT register.
>>>>
>>>> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
>>>>
>>>> ---
>>>> Changes in v2:
>>>>    - remove printing code
>>>> ---
>>>>    drivers/usb/typec/tcpm/tcpci.c | 34 +++++++++++++++++++++++++---------
>>>>    1 file changed, 25 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
>>>> index 0ee3e6e29bb1..8ccc2d1a8ffc 100644
>>>> --- a/drivers/usb/typec/tcpm/tcpci.c
>>>> +++ b/drivers/usb/typec/tcpm/tcpci.c
>>>> @@ -657,21 +657,28 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
>>>>       int ret;
>>>>       unsigned int raw;
>>>>
>>>> -    tcpci_read16(tcpci, TCPC_ALERT, &status);
>>>> +    ret = tcpci_read16(tcpci, TCPC_ALERT, &status);
>>>> +    if (ret < 0)
>>>> +            return ret;
>>>>
>>>>       /*
>>>>        * Clear alert status for everything except RX_STATUS, which shouldn't
>>>>        * be cleared until we have successfully retrieved message.
>>>>        */
>>>> -    if (status & ~TCPC_ALERT_RX_STATUS)
>>>> -            tcpci_write16(tcpci, TCPC_ALERT,
>>>> +    if (status & ~TCPC_ALERT_RX_STATUS) {
>>>> +            ret = tcpci_write16(tcpci, TCPC_ALERT,
>>>>                             status & ~TCPC_ALERT_RX_STATUS);
>>>> +            if (ret < 0)
>>>> +                    return ret;
>>>> +    }
>>>>
>>>>       if (status & TCPC_ALERT_CC_STATUS)
>>>>               tcpm_cc_change(tcpci->port);
>>>>
>>>>       if (status & TCPC_ALERT_POWER_STATUS) {
>>>> -            regmap_read(tcpci->regmap, TCPC_POWER_STATUS_MASK, &raw);
>>>> +            ret = regmap_read(tcpci->regmap, TCPC_POWER_STATUS_MASK, &raw);
>>>> +            if (ret < 0)
>>>> +                    return ret;
>>>>               /*
>>>>                * If power status mask has been reset, then the TCPC
>>>>                * has reset.
>>>> @@ -687,7 +694,9 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
>>>>               unsigned int cnt, payload_cnt;
>>>>               u16 header;
>>>>
>>>> -            regmap_read(tcpci->regmap, TCPC_RX_BYTE_CNT, &cnt);
>>>> +            ret = regmap_read(tcpci->regmap, TCPC_RX_BYTE_CNT, &cnt);
>>>> +            if (ret < 0)
>>>> +                    return ret;
>>>
>>> I think you still need to clear TCPC_ALERT_RX_STATUS in this case.
>>> Guenter, can you check this?
>>>
>>
>> If reading from or writing to the status register failed, we are pretty
>> much messed up anyway, so I don't think it really matters.
>>
>> I think the more severe problem is that this is an interrupt handler,
>> which either handles the interrupt or it doesn't. It does not have the
> 
> Yes, I agree.
> 
>> option of returning an error (negative error code).
> 
> Normally speaking, it means the device has successfully handled the
> interrupt event if an interrupt handler return IRQ_HANDLED, and the
> interrupt event doesn't belong to this device or not be handled if the
> interrupt handler return IRQ_NONE. However, the irq core will regard
> an negative error code as IRQ_NONE as far as I know. And when IRQ_NONE
> or a an negative error code is returned, irq core will judge if the irq
> is bad or if there is need to disable this irq. Therefore, the irq handler
> returns an negative error code should be ok if it's threaded.
> I just do the same thing as max_tcpci_irq() in tcpci_maxim_core.c.
> 

Unless that behavior is documented, I'd rather not depend on it.
Regarding "because this other driver does it" ... telling police that you only
drove faster than the speed limit because everyone does it isn't usually a well
received argument.

Guenter