[PATCH v4 0/2] usb: typec: tcpci: Make the driver be compatible with TCPCI Spec

[PATCH v4 0/2] usb: typec: tcpci: Make the driver be compatible with TCPCI Spec

[Miao.Zhu]

The tcpci driver doesn't fully follow the TCPCI Spec [R2.0 V1.0]
even if it mentions this Spec in its comments.
The changes make the driver be compatible with the TCPCI spec and
won't impact existing HW.

Miao Zhu (2):
  usb: typec: tcpm: tcpci: Make the driver be compatible with the TCPCI
    spec [Rev 2.0 Ver 1.0, October 2017]
  dt-bindings: usb: ptn5110: add TCPC properties

 .../devicetree/bindings/usb/nxp,ptn5110.yaml       |  22 ++++
 drivers/usb/typec/tcpm/tcpci.c                     | 115 +++++++++++++++++----
 include/linux/usb/tcpci.h                          |  11 ++
 3 files changed, 128 insertions(+), 20 deletions(-)

-- 
2.9.3

base-commit: b86545e02e8c22fb89218f29d381fa8e8b91d815

[PATCH v4 1/2] usb: typec: tcpm: tcpci: Make the driver be compatible with the TCPCI spec [Rev 2.0 Ver 1.0, October 2017]

[Miao.Zhu]

The tcpci driver doesn't fully follow the TCPCI spec even if
it mentions this spec in its comments.
- Add two flags into tcpci_data:
	RX_BUF_BYTE_x_hidden
	conn_present_capable
- Following flags in tcpci_data are read from device tree in tcpci_probe.
	TX_BUF_BYTE_x_hidden
	RX_BUF_BYTE_x_hidden
	auto_discharge_disconnect
	vbus_vsafe0v
The change makes the driver be compatible with the TCPCI spec and
won't impact existing HW.

Signed-off-by: Miao Zhu <miao@synopsys.com>
---
V3 -> V4: no changes
V2 -> V3: no changes
V1 -> V2: Cleaned up typo and addressed review comments
---
 drivers/usb/typec/tcpm/tcpci.c | 115 ++++++++++++++++++++++++++++++++++-------
 include/linux/usb/tcpci.h      |  11 ++++
 2 files changed, 106 insertions(+), 20 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
index ed32583..7c831c0 100644
--- a/drivers/usb/typec/tcpm/tcpci.c
+++ b/drivers/usb/typec/tcpm/tcpci.c
@@ -453,19 +453,26 @@ static int tcpci_set_roles(struct tcpc_dev *tcpc, bool attached,
 			   enum typec_role role, enum typec_data_role data)
 {
 	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
-	unsigned int reg;
+	unsigned int reg = 0;
 	int ret;
 
-	reg = FIELD_PREP(TCPC_MSG_HDR_INFO_REV, PD_REV20);
-	if (role == TYPEC_SOURCE)
-		reg |= TCPC_MSG_HDR_INFO_PWR_ROLE;
-	if (data == TYPEC_HOST)
-		reg |= TCPC_MSG_HDR_INFO_DATA_ROLE;
+	if (attached) {
+		reg = FIELD_PREP(TCPC_MSG_HDR_INFO_REV, PD_REV20);
+		if (role == TYPEC_SOURCE)
+			reg |= TCPC_MSG_HDR_INFO_PWR_ROLE;
+		if (data == TYPEC_HOST)
+			reg |= TCPC_MSG_HDR_INFO_DATA_ROLE;
+	}
 	ret = regmap_write(tcpci->regmap, TCPC_MSG_HDR_INFO, reg);
 	if (ret < 0)
 		return ret;
 
-	return 0;
+	if (tcpci->data->conn_present_capable)
+		return regmap_update_bits(tcpci->regmap, TCPC_CONFIG_STD_OUTPUT,
+					TCPC_CONFIG_STD_OUTPUT_CON_PRES,
+					attached ? TCPC_CONFIG_STD_OUTPUT_CON_PRES : 0);
+	else
+		return 0;
 }
 
 static int tcpci_set_pd_rx(struct tcpc_dev *tcpc, bool enable)
@@ -741,33 +748,86 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
 		struct pd_message msg;
 		unsigned int cnt, payload_cnt;
 		u16 header;
+		unsigned int frame_type;
+		enum tcpm_transmit_type rx_type;
 
 		regmap_read(tcpci->regmap, TCPC_RX_BYTE_CNT, &cnt);
 		/*
 		 * '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
 		 * defined in table 4-36 as one greater than the number of
-		 * bytes received. And that number includes the header. So:
+		 * bytes received. And that number includes the header.
+		 * In Section 4.4.14 of the TCPCI spec [Rev 2.0 Ver 1.0 October, 2017],
+		 * the RECEIVE_BUFFER comprises of three sets of registers:
+		 * READABLE_BYTE_COUNT, RX_BUF_FRAME_TYPE and RX_BUF_BYTE_x.
+		 * These registers can only be accessed by reading at a common
+		 * register address 0x30h.
 		 */
-		if (cnt > 3)
-			payload_cnt = cnt - (1 + sizeof(msg.header));
-		else
-			payload_cnt = 0;
+		if (tcpci->data->RX_BUF_BYTE_x_hidden) {
+			u8 buf[TCPC_RECEIVE_BUFFER_MAX_LEN] = {0,};
+			u8 pos = 0;
+
+			/* Read the count and frame type in RECEIVE_BUFFER */
+			regmap_raw_read(tcpci->regmap, TCPC_RX_BYTE_CNT, buf, 2);
+			/* READABLE_BYTE_COUNT */
+			cnt = buf[0];
+			/* RX_BUF_FRAME_TYPE */
+			frame_type = buf[1];
+
+			/* Read the content of the USB PD message in RECEIVE_BUFFER */
+			regmap_raw_read(tcpci->regmap, TCPC_RX_BYTE_CNT, buf, cnt + 1);
+
+			pos += 2;
+			memcpy(&msg.header, &buf[pos], sizeof(msg.header));
+
+			if (cnt > 3) {
+				pos += sizeof(msg.header);
+				payload_cnt = cnt - (1 + sizeof(msg.header));
+				if (WARN_ON(payload_cnt > sizeof(msg.payload)))
+					payload_cnt = sizeof(msg.payload);
+				memcpy(&msg.payload, &buf[pos], payload_cnt);
+			}
+		} else {
+			regmap_read(tcpci->regmap, TCPC_RX_BYTE_CNT, &cnt);
+			/*
+			 * '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
+			 * defined in table 4-36 as one greater than the number of
+			 * bytes received. And that number includes the header. So:
+			 */
+			if (cnt > 3)
+				payload_cnt = cnt - (1 + sizeof(msg.header));
+			else
+				payload_cnt = 0;
 
-		tcpci_read16(tcpci, TCPC_RX_HDR, &header);
-		msg.header = cpu_to_le16(header);
+			regmap_read(tcpci->regmap, TCPC_RX_BUF_FRAME_TYPE, &frame_type);
 
-		if (WARN_ON(payload_cnt > sizeof(msg.payload)))
-			payload_cnt = sizeof(msg.payload);
+			tcpci_read16(tcpci, TCPC_RX_HDR, &header);
+			msg.header = cpu_to_le16(header);
 
-		if (payload_cnt > 0)
-			regmap_raw_read(tcpci->regmap, TCPC_RX_DATA,
-					&msg.payload, payload_cnt);
+			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,
+							&msg.payload, payload_cnt);
+		}
 
 		/* Read complete, clear RX status alert bit */
 		tcpci_write16(tcpci, TCPC_ALERT, TCPC_ALERT_RX_STATUS);
 
-		tcpm_pd_receive(tcpci->port, &msg, TCPC_TX_SOP);
+		switch (frame_type) {
+		case TCPC_RX_BUF_FRAME_TYPE_SOP1:
+			rx_type = TCPC_TX_SOP_PRIME;
+			break;
+		case TCPC_RX_BUF_FRAME_TYPE_SOP:
+			rx_type = TCPC_TX_SOP;
+			break;
+		default:
+			rx_type = TCPC_TX_SOP;
+			break;
+		}
+		tcpm_pd_receive(tcpci->port, &msg, rx_type);
 	}
 
 	if (tcpci->data->vbus_vsafe0v && (status & TCPC_ALERT_EXTENDED_STATUS)) {
@@ -916,6 +976,21 @@ static int tcpci_probe(struct i2c_client *client)
 	if (err < 0)
 		return err;
 
+	chip->data.TX_BUF_BYTE_x_hidden =
+		device_property_read_bool(&client->dev, "TX_BUF_BYTE_x_hidden");
+	chip->data.RX_BUF_BYTE_x_hidden =
+		device_property_read_bool(&client->dev, "RX_BUF_BYTE_x_hidden");
+	chip->data.auto_discharge_disconnect =
+		device_property_read_bool(&client->dev, "auto_discharge_disconnect");
+	chip->data.vbus_vsafe0v = device_property_read_bool(&client->dev, "vbus_vsafe0v");
+
+	err = tcpci_check_std_output_cap(chip->data.regmap,
+					 TCPC_STD_OUTPUT_CAP_CONN_PRESENT);
+	if (err < 0)
+		return err;
+
+	chip->data.conn_present_capable = err;
+
 	err = tcpci_check_std_output_cap(chip->data.regmap,
 					 TCPC_STD_OUTPUT_CAP_ORIENTATION);
 	if (err < 0)
diff --git a/include/linux/usb/tcpci.h b/include/linux/usb/tcpci.h
index f7f5cfb..b649803 100644
--- a/include/linux/usb/tcpci.h
+++ b/include/linux/usb/tcpci.h
@@ -50,6 +50,7 @@
 #define TCPC_CONFIG_STD_OUTPUT_ORIENTATION_MASK		BIT(0)
 #define TCPC_CONFIG_STD_OUTPUT_ORIENTATION_NORMAL	0
 #define TCPC_CONFIG_STD_OUTPUT_ORIENTATION_FLIPPED	1
+#define TCPC_CONFIG_STD_OUTPUT_CON_PRES		BIT(1)
 
 #define TCPC_TCPC_CTRL			0x19
 #define TCPC_TCPC_CTRL_ORIENTATION	BIT(0)
@@ -126,6 +127,7 @@
 #define TCPC_STD_INPUT_CAP		0x28
 #define TCPC_STD_OUTPUT_CAP		0x29
 #define TCPC_STD_OUTPUT_CAP_ORIENTATION	BIT(0)
+#define TCPC_STD_OUTPUT_CAP_CONN_PRESENT	BIT(1)
 
 #define TCPC_MSG_HDR_INFO		0x2e
 #define TCPC_MSG_HDR_INFO_DATA_ROLE	BIT(3)
@@ -167,6 +169,7 @@
 
 /* I2C_WRITE_BYTE_COUNT + 1 when TX_BUF_BYTE_x is only accessible I2C_WRITE_BYTE_COUNT */
 #define TCPC_TRANSMIT_BUFFER_MAX_LEN		31
+#define TCPC_RECEIVE_BUFFER_MAX_LEN		32
 
 #define tcpc_presenting_rd(reg, cc) \
 	(!(TCPC_ROLE_CTRL_DRP & (reg)) && \
@@ -177,6 +180,9 @@ struct tcpci;
 /*
  * @TX_BUF_BYTE_x_hidden:
  *		optional; Set when TX_BUF_BYTE_x can only be accessed through I2C_WRITE_BYTE_COUNT.
+ * @RX_BUF_BYTE_x_hidden:
+ *		Optional; Set when READABLE_BYTE_COUNT, RX_BUF_FRAME_TYPE and RX_BUF_BYTE_x
+ *		can only be accessed through READABLE_BYTE_COUNT.
  * @frs_sourcing_vbus:
  *		Optional; Callback to perform chip specific operations when FRS
  *		is sourcing vbus.
@@ -204,6 +210,9 @@ struct tcpci;
  *		swap following Discover Identity on SOP' occurs.
  *		Return true when the TCPM is allowed to request a Vconn swap
  *		after Discovery Identity on SOP.
+ * @conn_present_capable:
+ *		Optional; Enable setting the connection present
+ *		CONFIG_STANDARD_OUTPUT (0x18) bit1.
  * @set_orientation:
  *		Optional; Enable setting the connector orientation
  *		CONFIG_STANDARD_OUTPUT (0x18) bit0.
@@ -211,9 +220,11 @@ struct tcpci;
 struct tcpci_data {
 	struct regmap *regmap;
 	unsigned char TX_BUF_BYTE_x_hidden:1;
+	unsigned char RX_BUF_BYTE_x_hidden:1;
 	unsigned char auto_discharge_disconnect:1;
 	unsigned char vbus_vsafe0v:1;
 	unsigned char cable_comm_capable:1;
+	unsigned char conn_present_capable:1;
 	unsigned char set_orientation:1;
 
 	int (*init)(struct tcpci *tcpci, struct tcpci_data *data);
-- 
2.9.3

[PATCH v4 2/2] dt-bindings: usb: ptn5110: add TCPC properties

[Miao.Zhu]

The TCPCI driver has flags to configure its protperties but
no way to enable these flags yet. Add these flags into DT
so that the driver can be compatible with TCPCI  Spec R2 V1.0.

Signed-off-by: Miao.Zhu <miao@synopsys.com>
---
V3 -> V4: refine description to follow DTS coding style
V2 -> V3: add description and type for new properties
V1 -> V2: new patch
---
 .../devicetree/bindings/usb/nxp,ptn5110.yaml       | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/nxp,ptn5110.yaml b/Documentation/devicetree/bindings/usb/nxp,ptn5110.yaml
index 65a8632..f5059dc7 100644
--- a/Documentation/devicetree/bindings/usb/nxp,ptn5110.yaml
+++ b/Documentation/devicetree/bindings/usb/nxp,ptn5110.yaml
@@ -21,6 +21,28 @@ properties:
   interrupts:
     maxItems: 1
 
+  TX_BUF_BYTE_x_hidden:
+    description:
+      True when TX_BUF_BYTE_x can only be accessed through
+      I2C_WRITE_BYTE_COUNT.
+    type: boolean
+
+  RX_BUF_BYTE_x_hidden:
+    description:
+      True when RX_BUF_BYTE_x can only be accessed through
+      READABLE_BYTE_COUNT.
+    type: boolean
+
+  auto_discharge_disconnect:
+    description:
+      True when TCPC can autonomously discharge vbus on disconnect.
+    type: boolean
+
+  vbus_vsafe0v:
+    description:
+      True when TCPC can detect whether vbus is at VSAFE0V.
+    type: boolean
+
   connector:
     type: object
     $ref: /schemas/connector/usb-connector.yaml#
-- 
2.9.3

Re: [PATCH v4 1/2] usb: typec: tcpm: tcpci: Make the driver be compatible with the TCPCI spec [Rev 2.0 Ver 1.0, October 2017]

[Dan Carpenter]

The subject is too long.  You've sent v2, v3, and v4 today.  Please, wait
for a day between resends.

On Wed, Dec 18, 2024 at 09:59:32AM +0100, Miao.Zhu wrote:
>  static int tcpci_set_pd_rx(struct tcpc_dev *tcpc, bool enable)
> @@ -741,33 +748,86 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
>  		struct pd_message msg;
>  		unsigned int cnt, payload_cnt;
>  		u16 header;
> +		unsigned int frame_type;
> +		enum tcpm_transmit_type rx_type;
>  
>  		regmap_read(tcpci->regmap, TCPC_RX_BYTE_CNT, &cnt);
>  		/*
>  		 * '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
>  		 * defined in table 4-36 as one greater than the number of
> -		 * bytes received. And that number includes the header. So:
> +		 * bytes received. And that number includes the header.
> +		 * In Section 4.4.14 of the TCPCI spec [Rev 2.0 Ver 1.0 October, 2017],
> +		 * the RECEIVE_BUFFER comprises of three sets of registers:
> +		 * READABLE_BYTE_COUNT, RX_BUF_FRAME_TYPE and RX_BUF_BYTE_x.
> +		 * These registers can only be accessed by reading at a common
> +		 * register address 0x30h.
>  		 */
> -		if (cnt > 3)
> -			payload_cnt = cnt - (1 + sizeof(msg.header));
> -		else
> -			payload_cnt = 0;
> +		if (tcpci->data->RX_BUF_BYTE_x_hidden) {
> +			u8 buf[TCPC_RECEIVE_BUFFER_MAX_LEN] = {0,};
> +			u8 pos = 0;
> +
> +			/* Read the count and frame type in RECEIVE_BUFFER */
> +			regmap_raw_read(tcpci->regmap, TCPC_RX_BYTE_CNT, buf, 2);
> +			/* READABLE_BYTE_COUNT */
> +			cnt = buf[0];
> +			/* RX_BUF_FRAME_TYPE */
> +			frame_type = buf[1];
> +
> +			/* Read the content of the USB PD message in RECEIVE_BUFFER */
> +			regmap_raw_read(tcpci->regmap, TCPC_RX_BYTE_CNT, buf, cnt + 1);
                                                                         ^^^
buffer overflow?

> +
> +			pos += 2;
> +			memcpy(&msg.header, &buf[pos], sizeof(msg.header));
> +
> +			if (cnt > 3) {
> +				pos += sizeof(msg.header);
> +				payload_cnt = cnt - (1 + sizeof(msg.header));
> +				if (WARN_ON(payload_cnt > sizeof(msg.payload)))
> +					payload_cnt = sizeof(msg.payload);
> +				memcpy(&msg.payload, &buf[pos], payload_cnt);

There is existing code later which does bounds checking on payload_cnt,
but it's too late.  We would have already overflowed buf[] and
msg.payload here.

> +			}
> +		} else {
> +			regmap_read(tcpci->regmap, TCPC_RX_BYTE_CNT, &cnt);
> +			/*
> +			 * '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
> +			 * defined in table 4-36 as one greater than the number of
> +			 * bytes received. And that number includes the header. So:
> +			 */
> +			if (cnt > 3)
> +				payload_cnt = cnt - (1 + sizeof(msg.header));
> +			else
> +				payload_cnt = 0;
>  
> -		tcpci_read16(tcpci, TCPC_RX_HDR, &header);
> -		msg.header = cpu_to_le16(header);
> +			regmap_read(tcpci->regmap, TCPC_RX_BUF_FRAME_TYPE, &frame_type);
>  
> -		if (WARN_ON(payload_cnt > sizeof(msg.payload)))
> -			payload_cnt = sizeof(msg.payload);

regards,
dan carpenter

Re: [PATCH v4 1/2] usb: typec: tcpm: tcpci: Make the driver be compatible with the TCPCI spec [Rev 2.0 Ver 1.0, October 2017]

[Dan Carpenter]

On Wed, Dec 18, 2024 at 12:33:42PM +0300, Dan Carpenter wrote:
> The subject is too long.  You've sent v2, v3, and v4 today.  Please, wait
> for a day between resends.
> 
> On Wed, Dec 18, 2024 at 09:59:32AM +0100, Miao.Zhu wrote:
> >  static int tcpci_set_pd_rx(struct tcpc_dev *tcpc, bool enable)
> > @@ -741,33 +748,86 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
> >  		struct pd_message msg;
> >  		unsigned int cnt, payload_cnt;
> >  		u16 header;
> > +		unsigned int frame_type;
> > +		enum tcpm_transmit_type rx_type;
> >  
> >  		regmap_read(tcpci->regmap, TCPC_RX_BYTE_CNT, &cnt);
> >  		/*
> >  		 * '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
> >  		 * defined in table 4-36 as one greater than the number of
> > -		 * bytes received. And that number includes the header. So:
> > +		 * bytes received. And that number includes the header.
> > +		 * In Section 4.4.14 of the TCPCI spec [Rev 2.0 Ver 1.0 October, 2017],
> > +		 * the RECEIVE_BUFFER comprises of three sets of registers:
> > +		 * READABLE_BYTE_COUNT, RX_BUF_FRAME_TYPE and RX_BUF_BYTE_x.
> > +		 * These registers can only be accessed by reading at a common
> > +		 * register address 0x30h.
> >  		 */
> > -		if (cnt > 3)
> > -			payload_cnt = cnt - (1 + sizeof(msg.header));
> > -		else
> > -			payload_cnt = 0;
> > +		if (tcpci->data->RX_BUF_BYTE_x_hidden) {
> > +			u8 buf[TCPC_RECEIVE_BUFFER_MAX_LEN] = {0,};
> > +			u8 pos = 0;
> > +
> > +			/* Read the count and frame type in RECEIVE_BUFFER */
> > +			regmap_raw_read(tcpci->regmap, TCPC_RX_BYTE_CNT, buf, 2);
> > +			/* READABLE_BYTE_COUNT */
> > +			cnt = buf[0];
> > +			/* RX_BUF_FRAME_TYPE */
> > +			frame_type = buf[1];
> > +
> > +			/* Read the content of the USB PD message in RECEIVE_BUFFER */
> > +			regmap_raw_read(tcpci->regmap, TCPC_RX_BYTE_CNT, buf, cnt + 1);
>                                                                          ^^^
> buffer overflow?
> 
> > +
> > +			pos += 2;
> > +			memcpy(&msg.header, &buf[pos], sizeof(msg.header));
> > +
> > +			if (cnt > 3) {
> > +				pos += sizeof(msg.header);
> > +				payload_cnt = cnt - (1 + sizeof(msg.header));
> > +				if (WARN_ON(payload_cnt > sizeof(msg.payload)))
> > +					payload_cnt = sizeof(msg.payload);
> > +				memcpy(&msg.payload, &buf[pos], payload_cnt);
> 
> There is existing code later which does bounds checking on payload_cnt,
> but it's too late.  We would have already overflowed buf[] and
> msg.payload here.
> 

This line is obviously fine.  It's only buf[] earlier from regmap_raw_read()
I'm worried about.

regards,
dan carpenter

Re: [PATCH v4 2/2] dt-bindings: usb: ptn5110: add TCPC properties

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2016 bytes --]

On Wed, Dec 18, 2024 at 09:59:33AM +0100, Miao.Zhu wrote:
> The TCPCI driver has flags to configure its protperties but
> no way to enable these flags yet. Add these flags into DT
> so that the driver can be compatible with TCPCI  Spec R2 V1.0.
> 
> Signed-off-by: Miao.Zhu <miao@synopsys.com>
> ---
> V3 -> V4: refine description to follow DTS coding style

Meanwhile missing the rest of the dt coding style?

> V2 -> V3: add description and type for new properties
> V1 -> V2: new patch
> ---
>  .../devicetree/bindings/usb/nxp,ptn5110.yaml       | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/nxp,ptn5110.yaml b/Documentation/devicetree/bindings/usb/nxp,ptn5110.yaml
> index 65a8632..f5059dc7 100644
> --- a/Documentation/devicetree/bindings/usb/nxp,ptn5110.yaml
> +++ b/Documentation/devicetree/bindings/usb/nxp,ptn5110.yaml
> @@ -21,6 +21,28 @@ properties:
>    interrupts:
>      maxItems: 1
>  
> +  TX_BUF_BYTE_x_hidden:

Is this x a wildcard?
No underscores in property names please, use -s.
Why is this capitalised?
Missing an nxp vendor prefix.
All of these properties should be type flag, not type boolean.

> +    description:
> +      True when TX_BUF_BYTE_x can only be accessed through
> +      I2C_WRITE_BYTE_COUNT.
> +    type: boolean
> +
> +  RX_BUF_BYTE_x_hidden:
> +    description:
> +      True when RX_BUF_BYTE_x can only be accessed through
> +      READABLE_BYTE_COUNT.

Is the I2C_ intentionally left out of the define here?


Cheers,
Conor.

> +    type: boolean
> +
> +  auto_discharge_disconnect:
> +    description:
> +      True when TCPC can autonomously discharge vbus on disconnect.
> +    type: boolean
> +
> +  vbus_vsafe0v:
> +    description:
> +      True when TCPC can detect whether vbus is at VSAFE0V.
> +    type: boolean
> +
>    connector:
>      type: object
>      $ref: /schemas/connector/usb-connector.yaml#
> -- 
> 2.9.3
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 2/2] dt-bindings: usb: ptn5110: add TCPC properties

[Krzysztof Kozlowski]

On 18/12/2024 09:59, Miao.Zhu wrote:
> The TCPCI driver has flags to configure its protperties but
> no way to enable these flags yet. Add these flags into DT
> so that the driver can be compatible with TCPCI  Spec R2 V1.0.
> 
> Signed-off-by: Miao.Zhu <miao@synopsys.com>
> ---
> V3 -> V4: refine description to follow DTS coding style

Nothing improved.

> V2 -> V3: add description and type for new properties
> V1 -> V2: new patch
> ---
>  .../devicetree/bindings/usb/nxp,ptn5110.yaml       | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/nxp,ptn5110.yaml b/Documentation/devicetree/bindings/usb/nxp,ptn5110.yaml
> index 65a8632..f5059dc7 100644
> --- a/Documentation/devicetree/bindings/usb/nxp,ptn5110.yaml
> +++ b/Documentation/devicetree/bindings/usb/nxp,ptn5110.yaml
> @@ -21,6 +21,28 @@ properties:
>    interrupts:
>      maxItems: 1
>  
> +  TX_BUF_BYTE_x_hidden:

You did not implement my comments.

<form letter>
This is a friendly reminder during the review process.

It seems my or other reviewer's previous comments were not fully
addressed. Maybe the feedback got lost between the quotes, maybe you
just forgot to apply it. Please go back to the previous discussion and
either implement all requested changes or keep discussing them.

Thank you.
</form letter>

Best regards,
Krzysztof