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

[PATCH 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>
---
 drivers/usb/typec/tcpm/tcpci.c | 36 +++++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
index 0ee3e6e29bb1..698d00b7fce9 100644
--- a/drivers/usb/typec/tcpm/tcpci.c
+++ b/drivers/usb/typec/tcpm/tcpci.c
@@ -657,21 +657,30 @@ 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) {
+		dev_err(tcpci->dev, "ALERT read failed\n");
+		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 +696,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 +710,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 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>
---
 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 698d00b7fce9..c128c24f8603 100644
--- a/drivers/usb/typec/tcpm/tcpci.c
+++ b/drivers/usb/typec/tcpm/tcpci.c
@@ -863,6 +863,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 1/2] usb: typec: tcpci: add check code for tcpci/regmap_read/write()

[Greg KH]

On Mon, Aug 28, 2023 at 05:30:34PM +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.

What do you mean by "other modules" here?

> 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>
> ---
>  drivers/usb/typec/tcpm/tcpci.c | 36 +++++++++++++++++++++++++---------
>  1 file changed, 27 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> index 0ee3e6e29bb1..698d00b7fce9 100644
> --- a/drivers/usb/typec/tcpm/tcpci.c
> +++ b/drivers/usb/typec/tcpm/tcpci.c
> @@ -657,21 +657,30 @@ 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) {
> +		dev_err(tcpci->dev, "ALERT read failed\n");

You are printing something in an irq handler, are you sure you want to
do that?  What happens if this is very noisy?

> +		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 +696,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 +710,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;

For all of these, if an error happens, you are not able to unwind from
the previous actions that the irq handler took.

Are you sure this is ok?  How was this tested?

thanks,

greg k-h

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

[Xu Yang]

Hi Greg,

> On Mon, Aug 28, 2023 at 05:30:34PM +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.
> 
> What do you mean by "other modules" here?

The PTN5110 chip communicates with SoC via i2c bus. If i2c controller
behaves abnormally, regmap_read/write() will fail and return a negative
value. So the "other modules" mainly mean i2c controller.

> 
> > 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>
> > ---
> >  drivers/usb/typec/tcpm/tcpci.c | 36 +++++++++++++++++++++++++---------
> >  1 file changed, 27 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> > index 0ee3e6e29bb1..698d00b7fce9 100644
> > --- a/drivers/usb/typec/tcpm/tcpci.c
> > +++ b/drivers/usb/typec/tcpm/tcpci.c
> > @@ -657,21 +657,30 @@ 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) {
> > +             dev_err(tcpci->dev, "ALERT read failed\n");
> 
> You are printing something in an irq handler, are you sure you want to
> do that?  What happens if this is very noisy?

This dev_err() may need to be removed. A log may be helpful to locate
issue. I just notice max_tcpci_irq() is doing the same thing:

359         ret = max_tcpci_read16(chip, TCPC_ALERT, &status);
360         if (ret < 0) {
361                 dev_err(chip->dev, "ALERT read failed\n");
362                 return ret;
363         }

> 
> > +             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 +696,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 +710,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;
> 
> For all of these, if an error happens, you are not able to unwind from
> the previous actions that the irq handler took.

The previous actions are necessary since they are real states. If an
error happens, the tcpm will also take over it. Normally, if the first
place to read ALERT register succeed, the following read/write() will
also success.

> 
> Are you sure this is ok?  How was this tested?

I just tested tcpci_read16(tcpci, TCPC_ALERT, &status). I have enabled
tcpci wake interrupt. When system had waked up from tcpci interrupt,
tcpci_read16() sometimes failed to get ALERT register (i2c controller is
abnormal in this period), then I got below log:

# cat /sys/kernel/debug/usb/tcpm-2-0050/log
[   80.696202] PD TX complete, status: 1
[   80.696226] PD TX complete, status: 1
[   80.696256] PD TX complete, status: 1
[   80.696280] PD TX complete, status: 1
[   80.696310] PD TX complete, status: 1
...
[   80.699982] PD RX, header: 0x8000 [1]
[   80.699987] Unchunked extended messages unsupported
[   80.699991] PD TX, header: 0x1b0
[   80.700034] PD TX complete, status: 1
[   80.700063] PD TX complete, status: 1
[   80.700089] PD TX complete, status: 1
[   80.700112] PD TX complete, status: 1
...
[   80.705056] PD TX complete, status: 1
[   80.705075] PD TX complete, status: 1
[   80.706364] CC1: 2 -> 0, CC2: 0 -> 0 [state SRC_READY, polarity 0, disconnected]
[   80.706373] state change SRC_READY -> SNK_UNATTACHED [rev3 NONE_AMS]
[   80.708250] Start toggling
[   80.709560] VBUS off
[   80.709564] VBUS VSAFE0V
[   80.709900] VBUS off
[   80.709902] VBUS VSAFE0V

Although i2c bus was recovered at the end, tcpci had still transferred wrong state
to tcpm. After I add check code for read/write(), the behavior is normal.

Thanks,
Xu Yang

> 
> thanks,
> 
> greg k-h

[PATCH 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