[PATCH 0/4] USB-C TCPM Orientation Support

[PATCH 0/4] USB-C TCPM Orientation Support

[Marco Felsch]

Hi,

this adds the support to controll the optional connector-orientation
available on some TCPC from the TCPM.

I used an custom board with OnSemi FUSB307B TCPC which is spec [1]
compatible but albeit the spec [1] says that this pin is controlled by
the TCPC if 'TCPC_CONTROL.DebugAccessoryControl = 0' it isn't at least
for this device.

I'm unsure if the usb tcpci spec has an copy'n'paste failure since
'TCPC_CONTROL.DebugAccessoryControl' shouldn't control the state of the
'connector orientation' pin or if the OnSemi FUSB307B has an HW bug.
Because on my device the 'TCPC_CONTROL.DebugAccessoryControl' is set to
0 but the register wasn't updated automatically.

Regards,
  Marco

[1] https://www.usb.org/sites/default/files/documents/usb-port_controller_specification_rev2.0_v1.0_0.pdf

Marco Felsch (4):
  dt-bindings: usb: typec-tcpci: add tcpci compatible binding
  usb: typec: tcpci: add generic tcpci compatible
  usb: typec: tcpm: add support to set tcpc connector orientatition
  usb: typec: tcpci: add support to set connector orientation

 .../devicetree/bindings/usb/nxp,ptn5110.yaml  |  4 +-
 drivers/usb/typec/tcpm/tcpci.c                | 44 +++++++++++++++++++
 drivers/usb/typec/tcpm/tcpm.c                 |  6 +++
 include/linux/usb/tcpci.h                     |  8 ++++
 include/linux/usb/tcpm.h                      |  2 +
 5 files changed, 63 insertions(+), 1 deletion(-)

-- 
2.39.2

[PATCH 1/4] dt-bindings: usb: typec-tcpci: add tcpci compatible binding

[Marco Felsch]

This binding descripes the generic TCPCI specification [1]. So add the
generic binding support since which can be used if an different TCPC is
used compatible which is compatible to [1].

[1] https://www.usb.org/sites/default/files/documents/usb-port_controller_specification_rev2.0_v1.0_0.pdf

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 Documentation/devicetree/bindings/usb/nxp,ptn5110.yaml | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/usb/nxp,ptn5110.yaml b/Documentation/devicetree/bindings/usb/nxp,ptn5110.yaml
index eaedb4cc6b6c..7bd7bbbac9e0 100644
--- a/Documentation/devicetree/bindings/usb/nxp,ptn5110.yaml
+++ b/Documentation/devicetree/bindings/usb/nxp,ptn5110.yaml
@@ -11,7 +11,9 @@ maintainers:
 
 properties:
   compatible:
-    const: nxp,ptn5110
+    enum:
+      - nxp,ptn5110
+      - tcpci
 
   reg:
     maxItems: 1
-- 
2.39.2

[PATCH 2/4] usb: typec: tcpci: add generic tcpci compatible

[Marco Felsch]

The driver already support the tcpci binding for the i2c_device_id so
add the support for the of_device_id too.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/usb/typec/tcpm/tcpci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
index 0ee3e6e29bb1..7118551827f6 100644
--- a/drivers/usb/typec/tcpm/tcpci.c
+++ b/drivers/usb/typec/tcpm/tcpci.c
@@ -889,6 +889,7 @@ MODULE_DEVICE_TABLE(i2c, tcpci_id);
 #ifdef CONFIG_OF
 static const struct of_device_id tcpci_of_match[] = {
 	{ .compatible = "nxp,ptn5110", },
+	{ .compatible = "tcpci", },
 	{},
 };
 MODULE_DEVICE_TABLE(of, tcpci_of_match);
-- 
2.39.2

[PATCH 3/4] usb: typec: tcpm: add support to set tcpc connector orientatition

[Marco Felsch]

This adds the support to set the connector orientation value
accordingly. This is part of the optional CONFIG_STANDARD_OUTPUT
register 0x18, specified within the USB port controller spsicification
rev. 2.0 [1].

[1] https://www.usb.org/sites/default/files/documents/usb-port_controller_specification_rev2.0_v1.0_0.pdf

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/usb/typec/tcpm/tcpm.c | 6 ++++++
 include/linux/usb/tcpm.h      | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 5945e3a2b0f7..85ca26687324 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -1099,6 +1099,12 @@ static int tcpm_set_roles(struct tcpm_port *port, bool attached,
 	if (ret < 0)
 		return ret;
 
+	if (port->tcpc->set_orientation) {
+		ret = port->tcpc->set_orientation(port->tcpc, orientation);
+		if (ret < 0)
+			return ret;
+	}
+
 	port->pwr_role = role;
 	port->data_role = data;
 	typec_set_data_role(port->typec_port, data);
diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
index 65fac5e1f317..93b681ff3ef9 100644
--- a/include/linux/usb/tcpm.h
+++ b/include/linux/usb/tcpm.h
@@ -133,6 +133,8 @@ struct tcpc_dev {
 		      enum typec_cc_status *cc2);
 	int (*set_polarity)(struct tcpc_dev *dev,
 			    enum typec_cc_polarity polarity);
+	int (*set_orientation)(struct tcpc_dev *dev,
+			       enum typec_orientation orientation);
 	int (*set_vconn)(struct tcpc_dev *dev, bool on);
 	int (*set_vbus)(struct tcpc_dev *dev, bool on, bool charge);
 	int (*set_current_limit)(struct tcpc_dev *dev, u32 max_ma, u32 mv);
-- 
2.39.2

[PATCH 4/4] usb: typec: tcpci: add support to set connector orientation

[Marco Felsch]

This add the support to set the optional connector orientation bit which
is part of the optional CONFIG_STANDARD_OUTPUT register 0x18 [1]. This
allows system designers to connect the tcpc orientation pin directly to
the 2:1 ss-mux.

[1] https://www.usb.org/sites/default/files/documents/usb-port_controller_specification_rev2.0_v1.0_0.pdf

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/usb/typec/tcpm/tcpci.c | 43 ++++++++++++++++++++++++++++++++++
 include/linux/usb/tcpci.h      |  8 +++++++
 2 files changed, 51 insertions(+)

diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
index 7118551827f6..7ce9d4923bc7 100644
--- a/drivers/usb/typec/tcpm/tcpci.c
+++ b/drivers/usb/typec/tcpm/tcpci.c
@@ -67,6 +67,18 @@ static int tcpci_write16(struct tcpci *tcpci, unsigned int reg, u16 val)
 	return regmap_raw_write(tcpci->regmap, reg, &val, sizeof(u16));
 }
 
+static bool tcpci_check_std_output_cap(struct regmap *regmap, u8 mask)
+{
+	unsigned int reg;
+	int ret;
+
+	ret = regmap_read(regmap, TCPC_STD_OUTPUT_CAP, &reg);
+	if (ret < 0)
+		return ret;
+
+	return (reg & mask) == mask;
+}
+
 static int tcpci_set_cc(struct tcpc_dev *tcpc, enum typec_cc_status cc)
 {
 	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
@@ -301,6 +313,27 @@ static int tcpci_set_polarity(struct tcpc_dev *tcpc,
 			   TCPC_TCPC_CTRL_ORIENTATION : 0);
 }
 
+static int tcpci_set_orientation(struct tcpc_dev *tcpc,
+				 enum typec_orientation orientation)
+{
+	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
+	unsigned int reg;
+
+	switch (orientation) {
+	case TYPEC_ORIENTATION_NONE:
+		/* We can't put a single output into high impedance */
+	case TYPEC_ORIENTATION_NORMAL:
+		reg = TCPC_CONFIG_STD_OUTPUT_ORIENTATION_NORMAL;
+		break;
+	case TYPEC_ORIENTATION_REVERSE:
+		reg = TCPC_CONFIG_STD_OUTPUT_ORIENTATION_FLIPPED;
+		break;
+	}
+
+	return regmap_update_bits(tcpci->regmap, TCPC_CONFIG_STD_OUTPUT,
+				  TCPC_CONFIG_STD_OUTPUT_ORIENTATION_MASK, reg);
+}
+
 static void tcpci_set_partner_usb_comm_capable(struct tcpc_dev *tcpc, bool capable)
 {
 	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
@@ -808,6 +841,9 @@ struct tcpci *tcpci_register_port(struct device *dev, struct tcpci_data *data)
 	if (tcpci->data->vbus_vsafe0v)
 		tcpci->tcpc.is_vbus_vsafe0v = tcpci_is_vbus_vsafe0v;
 
+	if (tcpci->data->set_orientation)
+		tcpci->tcpc.set_orientation = tcpci_set_orientation;
+
 	err = tcpci_parse_config(tcpci);
 	if (err < 0)
 		return ERR_PTR(err);
@@ -851,6 +887,13 @@ static int tcpci_probe(struct i2c_client *client)
 	if (err < 0)
 		return err;
 
+	err = tcpci_check_std_output_cap(chip->data.regmap,
+					 TCPC_STD_OUTPUT_CAP_ORIENTATION);
+	if (err < 0)
+		return err;
+
+	chip->data.set_orientation = err;
+
 	chip->tcpci = tcpci_register_port(&client->dev, &chip->data);
 	if (IS_ERR(chip->tcpci))
 		return PTR_ERR(chip->tcpci);
diff --git a/include/linux/usb/tcpci.h b/include/linux/usb/tcpci.h
index 467e8045e9f8..f2bfb4250366 100644
--- a/include/linux/usb/tcpci.h
+++ b/include/linux/usb/tcpci.h
@@ -47,6 +47,9 @@
 #define TCPC_SINK_FAST_ROLE_SWAP	BIT(0)
 
 #define TCPC_CONFIG_STD_OUTPUT		0x18
+#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_TCPC_CTRL			0x19
 #define TCPC_TCPC_CTRL_ORIENTATION	BIT(0)
@@ -127,6 +130,7 @@
 #define TCPC_DEV_CAP_2			0x26
 #define TCPC_STD_INPUT_CAP		0x28
 #define TCPC_STD_OUTPUT_CAP		0x29
+#define TCPC_STD_OUTPUT_CAP_ORIENTATION	BIT(0)
 
 #define TCPC_MSG_HDR_INFO		0x2e
 #define TCPC_MSG_HDR_INFO_DATA_ROLE	BIT(3)
@@ -198,12 +202,16 @@ struct tcpci;
  *		Chip level drivers are expected to check for contaminant and call
  *		tcpm_clean_port when the port is clean to put the port back into
  *		toggling state.
+ * @set_orientation:
+ *		Optional; Enable setting the connector orientation
+ *		CONFIG_STANDARD_OUTPUT (0x18) bit0.
  */
 struct tcpci_data {
 	struct regmap *regmap;
 	unsigned char TX_BUF_BYTE_x_hidden:1;
 	unsigned char auto_discharge_disconnect:1;
 	unsigned char vbus_vsafe0v:1;
+	unsigned char set_orientation:1;
 
 	int (*init)(struct tcpci *tcpci, struct tcpci_data *data);
 	int (*set_vconn)(struct tcpci *tcpci, struct tcpci_data *data,
-- 
2.39.2

Re: [PATCH 4/4] usb: typec: tcpci: add support to set connector orientation

[Dr. David Alan Gilbert]

* Marco Felsch (m.felsch@pengutronix.de) wrote:
> This add the support to set the optional connector orientation bit which
> is part of the optional CONFIG_STANDARD_OUTPUT register 0x18 [1]. This
> allows system designers to connect the tcpc orientation pin directly to
> the 2:1 ss-mux.
> 
> [1] https://www.usb.org/sites/default/files/documents/usb-port_controller_specification_rev2.0_v1.0_0.pdf
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/usb/typec/tcpm/tcpci.c | 43 ++++++++++++++++++++++++++++++++++
>  include/linux/usb/tcpci.h      |  8 +++++++
>  2 files changed, 51 insertions(+)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> index 7118551827f6..7ce9d4923bc7 100644
> --- a/drivers/usb/typec/tcpm/tcpci.c
> +++ b/drivers/usb/typec/tcpm/tcpci.c
> @@ -67,6 +67,18 @@ static int tcpci_write16(struct tcpci *tcpci, unsigned int reg, u16 val)
>  	return regmap_raw_write(tcpci->regmap, reg, &val, sizeof(u16));
>  }
>  
> +static bool tcpci_check_std_output_cap(struct regmap *regmap, u8 mask)
> +{
> +	unsigned int reg;
> +	int ret;
> +
> +	ret = regmap_read(regmap, TCPC_STD_OUTPUT_CAP, &reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	return (reg & mask) == mask;
> +}
> +
>  static int tcpci_set_cc(struct tcpc_dev *tcpc, enum typec_cc_status cc)
>  {
>  	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> @@ -301,6 +313,27 @@ static int tcpci_set_polarity(struct tcpc_dev *tcpc,
>  			   TCPC_TCPC_CTRL_ORIENTATION : 0);
>  }
>  
> +static int tcpci_set_orientation(struct tcpc_dev *tcpc,
> +				 enum typec_orientation orientation)
> +{
> +	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> +	unsigned int reg;
> +
> +	switch (orientation) {
> +	case TYPEC_ORIENTATION_NONE:
> +		/* We can't put a single output into high impedance */

Is that intended to be a fallthrough? If so I guess it needs
marking as such with a
                fallthrough;

Dave

> +	case TYPEC_ORIENTATION_NORMAL:
> +		reg = TCPC_CONFIG_STD_OUTPUT_ORIENTATION_NORMAL;
> +		break;
> +	case TYPEC_ORIENTATION_REVERSE:
> +		reg = TCPC_CONFIG_STD_OUTPUT_ORIENTATION_FLIPPED;
> +		break;
> +	}
> +
> +	return regmap_update_bits(tcpci->regmap, TCPC_CONFIG_STD_OUTPUT,
> +				  TCPC_CONFIG_STD_OUTPUT_ORIENTATION_MASK, reg);
> +}
> +
>  static void tcpci_set_partner_usb_comm_capable(struct tcpc_dev *tcpc, bool capable)
>  {
>  	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> @@ -808,6 +841,9 @@ struct tcpci *tcpci_register_port(struct device *dev, struct tcpci_data *data)
>  	if (tcpci->data->vbus_vsafe0v)
>  		tcpci->tcpc.is_vbus_vsafe0v = tcpci_is_vbus_vsafe0v;
>  
> +	if (tcpci->data->set_orientation)
> +		tcpci->tcpc.set_orientation = tcpci_set_orientation;
> +
>  	err = tcpci_parse_config(tcpci);
>  	if (err < 0)
>  		return ERR_PTR(err);
> @@ -851,6 +887,13 @@ static int tcpci_probe(struct i2c_client *client)
>  	if (err < 0)
>  		return err;
>  
> +	err = tcpci_check_std_output_cap(chip->data.regmap,
> +					 TCPC_STD_OUTPUT_CAP_ORIENTATION);
> +	if (err < 0)
> +		return err;
> +
> +	chip->data.set_orientation = err;
> +
>  	chip->tcpci = tcpci_register_port(&client->dev, &chip->data);
>  	if (IS_ERR(chip->tcpci))
>  		return PTR_ERR(chip->tcpci);
> diff --git a/include/linux/usb/tcpci.h b/include/linux/usb/tcpci.h
> index 467e8045e9f8..f2bfb4250366 100644
> --- a/include/linux/usb/tcpci.h
> +++ b/include/linux/usb/tcpci.h
> @@ -47,6 +47,9 @@
>  #define TCPC_SINK_FAST_ROLE_SWAP	BIT(0)
>  
>  #define TCPC_CONFIG_STD_OUTPUT		0x18
> +#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_TCPC_CTRL			0x19
>  #define TCPC_TCPC_CTRL_ORIENTATION	BIT(0)
> @@ -127,6 +130,7 @@
>  #define TCPC_DEV_CAP_2			0x26
>  #define TCPC_STD_INPUT_CAP		0x28
>  #define TCPC_STD_OUTPUT_CAP		0x29
> +#define TCPC_STD_OUTPUT_CAP_ORIENTATION	BIT(0)
>  
>  #define TCPC_MSG_HDR_INFO		0x2e
>  #define TCPC_MSG_HDR_INFO_DATA_ROLE	BIT(3)
> @@ -198,12 +202,16 @@ struct tcpci;
>   *		Chip level drivers are expected to check for contaminant and call
>   *		tcpm_clean_port when the port is clean to put the port back into
>   *		toggling state.
> + * @set_orientation:
> + *		Optional; Enable setting the connector orientation
> + *		CONFIG_STANDARD_OUTPUT (0x18) bit0.
>   */
>  struct tcpci_data {
>  	struct regmap *regmap;
>  	unsigned char TX_BUF_BYTE_x_hidden:1;
>  	unsigned char auto_discharge_disconnect:1;
>  	unsigned char vbus_vsafe0v:1;
> +	unsigned char set_orientation:1;
>  
>  	int (*init)(struct tcpci *tcpci, struct tcpci_data *data);
>  	int (*set_vconn)(struct tcpci *tcpci, struct tcpci_data *data,
> -- 
> 2.39.2
> 
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

Re: [PATCH 4/4] usb: typec: tcpci: add support to set connector orientation

[Marco Felsch]

Hi David,

On 24-02-05, Dr. David Alan Gilbert wrote:
> * Marco Felsch (m.felsch@pengutronix.de) wrote:
> > This add the support to set the optional connector orientation bit which
> > is part of the optional CONFIG_STANDARD_OUTPUT register 0x18 [1]. This
> > allows system designers to connect the tcpc orientation pin directly to
> > the 2:1 ss-mux.
> > 
> > [1] https://www.usb.org/sites/default/files/documents/usb-port_controller_specification_rev2.0_v1.0_0.pdf
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> >  drivers/usb/typec/tcpm/tcpci.c | 43 ++++++++++++++++++++++++++++++++++
> >  include/linux/usb/tcpci.h      |  8 +++++++
> >  2 files changed, 51 insertions(+)
> > 
> > diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> > index 7118551827f6..7ce9d4923bc7 100644
> > --- a/drivers/usb/typec/tcpm/tcpci.c
> > +++ b/drivers/usb/typec/tcpm/tcpci.c
> > @@ -67,6 +67,18 @@ static int tcpci_write16(struct tcpci *tcpci, unsigned int reg, u16 val)
> >  	return regmap_raw_write(tcpci->regmap, reg, &val, sizeof(u16));
> >  }
> >  
> > +static bool tcpci_check_std_output_cap(struct regmap *regmap, u8 mask)
> > +{
> > +	unsigned int reg;
> > +	int ret;
> > +
> > +	ret = regmap_read(regmap, TCPC_STD_OUTPUT_CAP, &reg);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return (reg & mask) == mask;
> > +}
> > +
> >  static int tcpci_set_cc(struct tcpc_dev *tcpc, enum typec_cc_status cc)
> >  {
> >  	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> > @@ -301,6 +313,27 @@ static int tcpci_set_polarity(struct tcpc_dev *tcpc,
> >  			   TCPC_TCPC_CTRL_ORIENTATION : 0);
> >  }
> >  
> > +static int tcpci_set_orientation(struct tcpc_dev *tcpc,
> > +				 enum typec_orientation orientation)
> > +{
> > +	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> > +	unsigned int reg;
> > +
> > +	switch (orientation) {
> > +	case TYPEC_ORIENTATION_NONE:
> > +		/* We can't put a single output into high impedance */
> 
> Is that intended to be a fallthrough? If so I guess it needs
> marking as such with a
>                 fallthrough;

You need to add it if there is code in between. Since there is no code,
just this comment, it shouldn't be necessary.

Regards,
  Marco

> 
> Dave
> 
> > +	case TYPEC_ORIENTATION_NORMAL:
> > +		reg = TCPC_CONFIG_STD_OUTPUT_ORIENTATION_NORMAL;
> > +		break;
> > +	case TYPEC_ORIENTATION_REVERSE:
> > +		reg = TCPC_CONFIG_STD_OUTPUT_ORIENTATION_FLIPPED;
> > +		break;
> > +	}
> > +
> > +	return regmap_update_bits(tcpci->regmap, TCPC_CONFIG_STD_OUTPUT,
> > +				  TCPC_CONFIG_STD_OUTPUT_ORIENTATION_MASK, reg);
> > +}
> > +
> >  static void tcpci_set_partner_usb_comm_capable(struct tcpc_dev *tcpc, bool capable)
> >  {
> >  	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> > @@ -808,6 +841,9 @@ struct tcpci *tcpci_register_port(struct device *dev, struct tcpci_data *data)
> >  	if (tcpci->data->vbus_vsafe0v)
> >  		tcpci->tcpc.is_vbus_vsafe0v = tcpci_is_vbus_vsafe0v;
> >  
> > +	if (tcpci->data->set_orientation)
> > +		tcpci->tcpc.set_orientation = tcpci_set_orientation;
> > +
> >  	err = tcpci_parse_config(tcpci);
> >  	if (err < 0)
> >  		return ERR_PTR(err);
> > @@ -851,6 +887,13 @@ static int tcpci_probe(struct i2c_client *client)
> >  	if (err < 0)
> >  		return err;
> >  
> > +	err = tcpci_check_std_output_cap(chip->data.regmap,
> > +					 TCPC_STD_OUTPUT_CAP_ORIENTATION);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	chip->data.set_orientation = err;
> > +
> >  	chip->tcpci = tcpci_register_port(&client->dev, &chip->data);
> >  	if (IS_ERR(chip->tcpci))
> >  		return PTR_ERR(chip->tcpci);
> > diff --git a/include/linux/usb/tcpci.h b/include/linux/usb/tcpci.h
> > index 467e8045e9f8..f2bfb4250366 100644
> > --- a/include/linux/usb/tcpci.h
> > +++ b/include/linux/usb/tcpci.h
> > @@ -47,6 +47,9 @@
> >  #define TCPC_SINK_FAST_ROLE_SWAP	BIT(0)
> >  
> >  #define TCPC_CONFIG_STD_OUTPUT		0x18
> > +#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_TCPC_CTRL			0x19
> >  #define TCPC_TCPC_CTRL_ORIENTATION	BIT(0)
> > @@ -127,6 +130,7 @@
> >  #define TCPC_DEV_CAP_2			0x26
> >  #define TCPC_STD_INPUT_CAP		0x28
> >  #define TCPC_STD_OUTPUT_CAP		0x29
> > +#define TCPC_STD_OUTPUT_CAP_ORIENTATION	BIT(0)
> >  
> >  #define TCPC_MSG_HDR_INFO		0x2e
> >  #define TCPC_MSG_HDR_INFO_DATA_ROLE	BIT(3)
> > @@ -198,12 +202,16 @@ struct tcpci;
> >   *		Chip level drivers are expected to check for contaminant and call
> >   *		tcpm_clean_port when the port is clean to put the port back into
> >   *		toggling state.
> > + * @set_orientation:
> > + *		Optional; Enable setting the connector orientation
> > + *		CONFIG_STANDARD_OUTPUT (0x18) bit0.
> >   */
> >  struct tcpci_data {
> >  	struct regmap *regmap;
> >  	unsigned char TX_BUF_BYTE_x_hidden:1;
> >  	unsigned char auto_discharge_disconnect:1;
> >  	unsigned char vbus_vsafe0v:1;
> > +	unsigned char set_orientation:1;
> >  
> >  	int (*init)(struct tcpci *tcpci, struct tcpci_data *data);
> >  	int (*set_vconn)(struct tcpci *tcpci, struct tcpci_data *data,
> > -- 
> > 2.39.2
> > 
> > 
> -- 
>  -----Open up your eyes, open up your mind, open up your code -------   
> / Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
> \        dave @ treblig.org |                               | In Hex /
>  \ _________________________|_____ http://www.treblig.org   |_______/
> 

Re: [PATCH 4/4] usb: typec: tcpci: add support to set connector orientation

[Guenter Roeck]

On 2/5/24 08:54, Marco Felsch wrote:
> Hi David,
> 
> On 24-02-05, Dr. David Alan Gilbert wrote:
>> * Marco Felsch (m.felsch@pengutronix.de) wrote:
>>> This add the support to set the optional connector orientation bit which
>>> is part of the optional CONFIG_STANDARD_OUTPUT register 0x18 [1]. This
>>> allows system designers to connect the tcpc orientation pin directly to
>>> the 2:1 ss-mux.
>>>
>>> [1] https://www.usb.org/sites/default/files/documents/usb-port_controller_specification_rev2.0_v1.0_0.pdf
>>>
>>> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
>>> ---
>>>   drivers/usb/typec/tcpm/tcpci.c | 43 ++++++++++++++++++++++++++++++++++
>>>   include/linux/usb/tcpci.h      |  8 +++++++
>>>   2 files changed, 51 insertions(+)
>>>
>>> diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
>>> index 7118551827f6..7ce9d4923bc7 100644
>>> --- a/drivers/usb/typec/tcpm/tcpci.c
>>> +++ b/drivers/usb/typec/tcpm/tcpci.c
>>> @@ -67,6 +67,18 @@ static int tcpci_write16(struct tcpci *tcpci, unsigned int reg, u16 val)
>>>   	return regmap_raw_write(tcpci->regmap, reg, &val, sizeof(u16));
>>>   }
>>>   
>>> +static bool tcpci_check_std_output_cap(struct regmap *regmap, u8 mask)
>>> +{
>>> +	unsigned int reg;
>>> +	int ret;
>>> +
>>> +	ret = regmap_read(regmap, TCPC_STD_OUTPUT_CAP, &reg);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>> +	return (reg & mask) == mask;
>>> +}
>>> +
>>>   static int tcpci_set_cc(struct tcpc_dev *tcpc, enum typec_cc_status cc)
>>>   {
>>>   	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
>>> @@ -301,6 +313,27 @@ static int tcpci_set_polarity(struct tcpc_dev *tcpc,
>>>   			   TCPC_TCPC_CTRL_ORIENTATION : 0);
>>>   }
>>>   
>>> +static int tcpci_set_orientation(struct tcpc_dev *tcpc,
>>> +				 enum typec_orientation orientation)
>>> +{
>>> +	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
>>> +	unsigned int reg;
>>> +
>>> +	switch (orientation) {
>>> +	case TYPEC_ORIENTATION_NONE:
>>> +		/* We can't put a single output into high impedance */
>>
>> Is that intended to be a fallthrough? If so I guess it needs
>> marking as such with a
>>                  fallthrough;
> 
> You need to add it if there is code in between. Since there is no code,
> just this comment, it shouldn't be necessary.
> 

Still, I think it would be desirable here to clarify that this
is not a lost return but intentionally sets
TCPC_CONFIG_STD_OUTPUT_ORIENTATION_NORMAL.

Guenter

> Regards,
>    Marco
> 
>>
>> Dave
>>
>>> +	case TYPEC_ORIENTATION_NORMAL:
>>> +		reg = TCPC_CONFIG_STD_OUTPUT_ORIENTATION_NORMAL;
>>> +		break;
>>> +	case TYPEC_ORIENTATION_REVERSE:
>>> +		reg = TCPC_CONFIG_STD_OUTPUT_ORIENTATION_FLIPPED;
>>> +		break;
>>> +	}
>>> +
>>> +	return regmap_update_bits(tcpci->regmap, TCPC_CONFIG_STD_OUTPUT,
>>> +				  TCPC_CONFIG_STD_OUTPUT_ORIENTATION_MASK, reg);
>>> +}
>>> +
>>>   static void tcpci_set_partner_usb_comm_capable(struct tcpc_dev *tcpc, bool capable)
>>>   {
>>>   	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
>>> @@ -808,6 +841,9 @@ struct tcpci *tcpci_register_port(struct device *dev, struct tcpci_data *data)
>>>   	if (tcpci->data->vbus_vsafe0v)
>>>   		tcpci->tcpc.is_vbus_vsafe0v = tcpci_is_vbus_vsafe0v;
>>>   
>>> +	if (tcpci->data->set_orientation)
>>> +		tcpci->tcpc.set_orientation = tcpci_set_orientation;
>>> +
>>>   	err = tcpci_parse_config(tcpci);
>>>   	if (err < 0)
>>>   		return ERR_PTR(err);
>>> @@ -851,6 +887,13 @@ static int tcpci_probe(struct i2c_client *client)
>>>   	if (err < 0)
>>>   		return err;
>>>   
>>> +	err = tcpci_check_std_output_cap(chip->data.regmap,
>>> +					 TCPC_STD_OUTPUT_CAP_ORIENTATION);
>>> +	if (err < 0)
>>> +		return err;
>>> +
>>> +	chip->data.set_orientation = err;
>>> +
>>>   	chip->tcpci = tcpci_register_port(&client->dev, &chip->data);
>>>   	if (IS_ERR(chip->tcpci))
>>>   		return PTR_ERR(chip->tcpci);
>>> diff --git a/include/linux/usb/tcpci.h b/include/linux/usb/tcpci.h
>>> index 467e8045e9f8..f2bfb4250366 100644
>>> --- a/include/linux/usb/tcpci.h
>>> +++ b/include/linux/usb/tcpci.h
>>> @@ -47,6 +47,9 @@
>>>   #define TCPC_SINK_FAST_ROLE_SWAP	BIT(0)
>>>   
>>>   #define TCPC_CONFIG_STD_OUTPUT		0x18
>>> +#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_TCPC_CTRL			0x19
>>>   #define TCPC_TCPC_CTRL_ORIENTATION	BIT(0)
>>> @@ -127,6 +130,7 @@
>>>   #define TCPC_DEV_CAP_2			0x26
>>>   #define TCPC_STD_INPUT_CAP		0x28
>>>   #define TCPC_STD_OUTPUT_CAP		0x29
>>> +#define TCPC_STD_OUTPUT_CAP_ORIENTATION	BIT(0)
>>>   
>>>   #define TCPC_MSG_HDR_INFO		0x2e
>>>   #define TCPC_MSG_HDR_INFO_DATA_ROLE	BIT(3)
>>> @@ -198,12 +202,16 @@ struct tcpci;
>>>    *		Chip level drivers are expected to check for contaminant and call
>>>    *		tcpm_clean_port when the port is clean to put the port back into
>>>    *		toggling state.
>>> + * @set_orientation:
>>> + *		Optional; Enable setting the connector orientation
>>> + *		CONFIG_STANDARD_OUTPUT (0x18) bit0.
>>>    */
>>>   struct tcpci_data {
>>>   	struct regmap *regmap;
>>>   	unsigned char TX_BUF_BYTE_x_hidden:1;
>>>   	unsigned char auto_discharge_disconnect:1;
>>>   	unsigned char vbus_vsafe0v:1;
>>> +	unsigned char set_orientation:1;
>>>   
>>>   	int (*init)(struct tcpci *tcpci, struct tcpci_data *data);
>>>   	int (*set_vconn)(struct tcpci *tcpci, struct tcpci_data *data,
>>> -- 
>>> 2.39.2
>>>
>>>
>> -- 
>>   -----Open up your eyes, open up your mind, open up your code -------
>> / Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \
>> \        dave @ treblig.org |                               | In Hex /
>>   \ _________________________|_____ http://www.treblig.org   |_______/
>>

Re: [PATCH 4/4] usb: typec: tcpci: add support to set connector orientation

[Marco Felsch]

On 24-02-05, Guenter Roeck wrote:
> On 2/5/24 08:54, Marco Felsch wrote:
> > Hi David,
> > 
> > On 24-02-05, Dr. David Alan Gilbert wrote:
> > > * Marco Felsch (m.felsch@pengutronix.de) wrote:
> > > > This add the support to set the optional connector orientation bit which
> > > > is part of the optional CONFIG_STANDARD_OUTPUT register 0x18 [1]. This
> > > > allows system designers to connect the tcpc orientation pin directly to
> > > > the 2:1 ss-mux.
> > > > 
> > > > [1] https://www.usb.org/sites/default/files/documents/usb-port_controller_specification_rev2.0_v1.0_0.pdf
> > > > 
> > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > > ---
> > > >   drivers/usb/typec/tcpm/tcpci.c | 43 ++++++++++++++++++++++++++++++++++
> > > >   include/linux/usb/tcpci.h      |  8 +++++++
> > > >   2 files changed, 51 insertions(+)
> > > > 
> > > > diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> > > > index 7118551827f6..7ce9d4923bc7 100644
> > > > --- a/drivers/usb/typec/tcpm/tcpci.c
> > > > +++ b/drivers/usb/typec/tcpm/tcpci.c
> > > > @@ -67,6 +67,18 @@ static int tcpci_write16(struct tcpci *tcpci, unsigned int reg, u16 val)
> > > >   	return regmap_raw_write(tcpci->regmap, reg, &val, sizeof(u16));
> > > >   }
> > > > +static bool tcpci_check_std_output_cap(struct regmap *regmap, u8 mask)
> > > > +{
> > > > +	unsigned int reg;
> > > > +	int ret;
> > > > +
> > > > +	ret = regmap_read(regmap, TCPC_STD_OUTPUT_CAP, &reg);
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	return (reg & mask) == mask;
> > > > +}
> > > > +
> > > >   static int tcpci_set_cc(struct tcpc_dev *tcpc, enum typec_cc_status cc)
> > > >   {
> > > >   	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> > > > @@ -301,6 +313,27 @@ static int tcpci_set_polarity(struct tcpc_dev *tcpc,
> > > >   			   TCPC_TCPC_CTRL_ORIENTATION : 0);
> > > >   }
> > > > +static int tcpci_set_orientation(struct tcpc_dev *tcpc,
> > > > +				 enum typec_orientation orientation)
> > > > +{
> > > > +	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> > > > +	unsigned int reg;
> > > > +
> > > > +	switch (orientation) {
> > > > +	case TYPEC_ORIENTATION_NONE:
> > > > +		/* We can't put a single output into high impedance */
> > > 
> > > Is that intended to be a fallthrough? If so I guess it needs
> > > marking as such with a
> > >                  fallthrough;
> > 
> > You need to add it if there is code in between. Since there is no code,
> > just this comment, it shouldn't be necessary.
> > 
> 
> Still, I think it would be desirable here to clarify that this
> is not a lost return but intentionally sets
> TCPC_CONFIG_STD_OUTPUT_ORIENTATION_NORMAL.

Sure, I will adapt it once the complete patchset is reviewed.

Regards,
  Marco

> Guenter
> 
> > Regards,
> >    Marco
> > 
> > > 
> > > Dave
> > > 
> > > > +	case TYPEC_ORIENTATION_NORMAL:
> > > > +		reg = TCPC_CONFIG_STD_OUTPUT_ORIENTATION_NORMAL;
> > > > +		break;
> > > > +	case TYPEC_ORIENTATION_REVERSE:
> > > > +		reg = TCPC_CONFIG_STD_OUTPUT_ORIENTATION_FLIPPED;
> > > > +		break;
> > > > +	}
> > > > +
> > > > +	return regmap_update_bits(tcpci->regmap, TCPC_CONFIG_STD_OUTPUT,
> > > > +				  TCPC_CONFIG_STD_OUTPUT_ORIENTATION_MASK, reg);
> > > > +}
> > > > +
> > > >   static void tcpci_set_partner_usb_comm_capable(struct tcpc_dev *tcpc, bool capable)
> > > >   {
> > > >   	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> > > > @@ -808,6 +841,9 @@ struct tcpci *tcpci_register_port(struct device *dev, struct tcpci_data *data)
> > > >   	if (tcpci->data->vbus_vsafe0v)
> > > >   		tcpci->tcpc.is_vbus_vsafe0v = tcpci_is_vbus_vsafe0v;
> > > > +	if (tcpci->data->set_orientation)
> > > > +		tcpci->tcpc.set_orientation = tcpci_set_orientation;
> > > > +
> > > >   	err = tcpci_parse_config(tcpci);
> > > >   	if (err < 0)
> > > >   		return ERR_PTR(err);
> > > > @@ -851,6 +887,13 @@ static int tcpci_probe(struct i2c_client *client)
> > > >   	if (err < 0)
> > > >   		return err;
> > > > +	err = tcpci_check_std_output_cap(chip->data.regmap,
> > > > +					 TCPC_STD_OUTPUT_CAP_ORIENTATION);
> > > > +	if (err < 0)
> > > > +		return err;
> > > > +
> > > > +	chip->data.set_orientation = err;
> > > > +
> > > >   	chip->tcpci = tcpci_register_port(&client->dev, &chip->data);
> > > >   	if (IS_ERR(chip->tcpci))
> > > >   		return PTR_ERR(chip->tcpci);
> > > > diff --git a/include/linux/usb/tcpci.h b/include/linux/usb/tcpci.h
> > > > index 467e8045e9f8..f2bfb4250366 100644
> > > > --- a/include/linux/usb/tcpci.h
> > > > +++ b/include/linux/usb/tcpci.h
> > > > @@ -47,6 +47,9 @@
> > > >   #define TCPC_SINK_FAST_ROLE_SWAP	BIT(0)
> > > >   #define TCPC_CONFIG_STD_OUTPUT		0x18
> > > > +#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_TCPC_CTRL			0x19
> > > >   #define TCPC_TCPC_CTRL_ORIENTATION	BIT(0)
> > > > @@ -127,6 +130,7 @@
> > > >   #define TCPC_DEV_CAP_2			0x26
> > > >   #define TCPC_STD_INPUT_CAP		0x28
> > > >   #define TCPC_STD_OUTPUT_CAP		0x29
> > > > +#define TCPC_STD_OUTPUT_CAP_ORIENTATION	BIT(0)
> > > >   #define TCPC_MSG_HDR_INFO		0x2e
> > > >   #define TCPC_MSG_HDR_INFO_DATA_ROLE	BIT(3)
> > > > @@ -198,12 +202,16 @@ struct tcpci;
> > > >    *		Chip level drivers are expected to check for contaminant and call
> > > >    *		tcpm_clean_port when the port is clean to put the port back into
> > > >    *		toggling state.
> > > > + * @set_orientation:
> > > > + *		Optional; Enable setting the connector orientation
> > > > + *		CONFIG_STANDARD_OUTPUT (0x18) bit0.
> > > >    */
> > > >   struct tcpci_data {
> > > >   	struct regmap *regmap;
> > > >   	unsigned char TX_BUF_BYTE_x_hidden:1;
> > > >   	unsigned char auto_discharge_disconnect:1;
> > > >   	unsigned char vbus_vsafe0v:1;
> > > > +	unsigned char set_orientation:1;
> > > >   	int (*init)(struct tcpci *tcpci, struct tcpci_data *data);
> > > >   	int (*set_vconn)(struct tcpci *tcpci, struct tcpci_data *data,
> > > > -- 
> > > > 2.39.2
> > > > 
> > > > 
> > > -- 
> > >   -----Open up your eyes, open up your mind, open up your code -------
> > > / Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \
> > > \        dave @ treblig.org |                               | In Hex /
> > >   \ _________________________|_____ http://www.treblig.org   |_______/
> > > 
> 
> 

Re: [PATCH 1/4] dt-bindings: usb: typec-tcpci: add tcpci compatible binding

[Krzysztof Kozlowski]

On 05/02/2024 17:43, Marco Felsch wrote:
> This binding descripes the generic TCPCI specification [1]. So add the

Typo: describes.

No, this binding describes PTN5110, not generic TCPCI. This is not
accurate commit description.

> generic binding support since which can be used if an different TCPC is
> used compatible which is compatible to [1].

Sorry, cannot parse it. Please run it through native speaker, Google
grammar check, ChatGPT or some other way.

> 
> [1] https://www.usb.org/sites/default/files/documents/usb-port_controller_specification_rev2.0_v1.0_0.pdf
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  Documentation/devicetree/bindings/usb/nxp,ptn5110.yaml | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/usb/nxp,ptn5110.yaml b/Documentation/devicetree/bindings/usb/nxp,ptn5110.yaml
> index eaedb4cc6b6c..7bd7bbbac9e0 100644
> --- a/Documentation/devicetree/bindings/usb/nxp,ptn5110.yaml
> +++ b/Documentation/devicetree/bindings/usb/nxp,ptn5110.yaml
> @@ -11,7 +11,9 @@ maintainers:
>  
>  properties:
>    compatible:
> -    const: nxp,ptn5110
> +    enum:
> +      - nxp,ptn5110
> +      - tcpci

I don't think this is correct. First, this is binding for NXP chip, so
why generic implementation should be here? I would expect it in its own
dedicated binding.

Second, we rarely want generic compatibles. Care to share more details?
Are all details expected to follow spec, without need of quirks?

Best regards,
Krzysztof

Re: [PATCH 1/4] dt-bindings: usb: typec-tcpci: add tcpci compatible binding

[Marco Felsch]

On 24-02-06, Krzysztof Kozlowski wrote:
> On 05/02/2024 17:43, Marco Felsch wrote:
> > This binding descripes the generic TCPCI specification [1]. So add the
> 
> Typo: describes.

Argh.

> No, this binding describes PTN5110, not generic TCPCI. This is not
> accurate commit description.

This binding is currently missued if another TCPCI conform chip is used
which requires no special handling. I could have dropped this commit
since the 'tcpci' is already present at i2c-device-id level.

> 
> > generic binding support since which can be used if an different TCPC is
> > used compatible which is compatible to [1].
> 
> Sorry, cannot parse it. Please run it through native speaker, Google
> grammar check, ChatGPT or some other way.

Argh.. you're right, sorry. I will rephrase it.

> > [1] https://www.usb.org/sites/default/files/documents/usb-port_controller_specification_rev2.0_v1.0_0.pdf
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> >  Documentation/devicetree/bindings/usb/nxp,ptn5110.yaml | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/nxp,ptn5110.yaml b/Documentation/devicetree/bindings/usb/nxp,ptn5110.yaml
> > index eaedb4cc6b6c..7bd7bbbac9e0 100644
> > --- a/Documentation/devicetree/bindings/usb/nxp,ptn5110.yaml
> > +++ b/Documentation/devicetree/bindings/usb/nxp,ptn5110.yaml
> > @@ -11,7 +11,9 @@ maintainers:
> >  
> >  properties:
> >    compatible:
> > -    const: nxp,ptn5110
> > +    enum:
> > +      - nxp,ptn5110
> > +      - tcpci
> 
> I don't think this is correct. First, this is binding for NXP chip, so
> why generic implementation should be here? I would expect it in its own
> dedicated binding.

The nxp,ptn5110 device was the first driver which implements an TCPCI
conform driver. The driver already support the tcpci binding for i2c-id
devices as I mentioned above. IMHO this whole binding (file) should be
converted and the nxp,ptn5110 compatible should be marked as deprecated.

> Second, we rarely want generic compatibles. Care to share more details?

As said above this particular NXP chip is an TCPCI conform chip. There
is nothing special about it. There are other vendors like OnSemi (in my
case) which implement also an TCPCI conform chip. The (Linux) driver
already binds to the generic tcpci compatible if the i2c-core falls back
to the i2c-device id. It's even more confusing that the i2c-id supports
only the generic binding the of-compatible support only the specifc one.

> Are all details expected to follow spec, without need of quirks?

Please see above, I hope this helps.

Regards,
  Marco

> 
> Best regards,
> Krzysztof
> 
> 

Re: [PATCH 1/4] dt-bindings: usb: typec-tcpci: add tcpci compatible binding

[Marco Felsch]

On 24-02-06, Marco Felsch wrote:
> On 24-02-06, Krzysztof Kozlowski wrote:
> > On 05/02/2024 17:43, Marco Felsch wrote:
> > > This binding descripes the generic TCPCI specification [1]. So add the
> > 
> > Typo: describes.
> 
> Argh.
> 
> > No, this binding describes PTN5110, not generic TCPCI. This is not
> > accurate commit description.
> 
> This binding is currently missued if another TCPCI conform chip is used

/missused/misused/

> which requires no special handling. I could have dropped this commit
> since the 'tcpci' is already present at i2c-device-id level.
> 
> > 
> > > generic binding support since which can be used if an different TCPC is
> > > used compatible which is compatible to [1].
> > 
> > Sorry, cannot parse it. Please run it through native speaker, Google
> > grammar check, ChatGPT or some other way.
> 
> Argh.. you're right, sorry. I will rephrase it.
> 
> > > [1] https://www.usb.org/sites/default/files/documents/usb-port_controller_specification_rev2.0_v1.0_0.pdf
> > > 
> > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > ---
> > >  Documentation/devicetree/bindings/usb/nxp,ptn5110.yaml | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/usb/nxp,ptn5110.yaml b/Documentation/devicetree/bindings/usb/nxp,ptn5110.yaml
> > > index eaedb4cc6b6c..7bd7bbbac9e0 100644
> > > --- a/Documentation/devicetree/bindings/usb/nxp,ptn5110.yaml
> > > +++ b/Documentation/devicetree/bindings/usb/nxp,ptn5110.yaml
> > > @@ -11,7 +11,9 @@ maintainers:
> > >  
> > >  properties:
> > >    compatible:
> > > -    const: nxp,ptn5110
> > > +    enum:
> > > +      - nxp,ptn5110
> > > +      - tcpci
> > 
> > I don't think this is correct. First, this is binding for NXP chip, so
> > why generic implementation should be here? I would expect it in its own
> > dedicated binding.
> 
> The nxp,ptn5110 device was the first driver which implements an TCPCI
> conform driver. The driver already support the tcpci binding for i2c-id
> devices as I mentioned above. IMHO this whole binding (file) should be
> converted and the nxp,ptn5110 compatible should be marked as deprecated.
> 
> > Second, we rarely want generic compatibles. Care to share more details?
> 
> As said above this particular NXP chip is an TCPCI conform chip. There
> is nothing special about it. There are other vendors like OnSemi (in my
> case) which implement also an TCPCI conform chip. The (Linux) driver
> already binds to the generic tcpci compatible if the i2c-core falls back
> to the i2c-device id. It's even more confusing that the i2c-id supports
> only the generic binding the of-compatible support only the specifc one.
> 
> > Are all details expected to follow spec, without need of quirks?
> 
> Please see above, I hope this helps.
> 
> Regards,
>   Marco
> 
> > 
> > Best regards,
> > Krzysztof
> > 
> > 
> 
> 

Re: [PATCH 1/4] dt-bindings: usb: typec-tcpci: add tcpci compatible binding

[Krzysztof Kozlowski]

On 06/02/2024 15:52, Marco Felsch wrote:
> On 24-02-06, Krzysztof Kozlowski wrote:
>> On 05/02/2024 17:43, Marco Felsch wrote:
>>> This binding descripes the generic TCPCI specification [1]. So add the
>>
>> Typo: describes.
> 
> Argh.
> 
>> No, this binding describes PTN5110, not generic TCPCI. This is not
>> accurate commit description.
> 
> This binding is currently missued if another TCPCI conform chip is used

Why would people misuse binding instead of doing things properly? :)

> which requires no special handling. I could have dropped this commit
> since the 'tcpci' is already present at i2c-device-id level.


> 
>>
>>> generic binding support since which can be used if an different TCPC is
>>> used compatible which is compatible to [1].
>>
>> Sorry, cannot parse it. Please run it through native speaker, Google
>> grammar check, ChatGPT or some other way.
> 
> Argh.. you're right, sorry. I will rephrase it.
> 
>>> [1] https://www.usb.org/sites/default/files/documents/usb-port_controller_specification_rev2.0_v1.0_0.pdf
>>>
>>> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
>>> ---
>>>  Documentation/devicetree/bindings/usb/nxp,ptn5110.yaml | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/nxp,ptn5110.yaml b/Documentation/devicetree/bindings/usb/nxp,ptn5110.yaml
>>> index eaedb4cc6b6c..7bd7bbbac9e0 100644
>>> --- a/Documentation/devicetree/bindings/usb/nxp,ptn5110.yaml
>>> +++ b/Documentation/devicetree/bindings/usb/nxp,ptn5110.yaml
>>> @@ -11,7 +11,9 @@ maintainers:
>>>  
>>>  properties:
>>>    compatible:
>>> -    const: nxp,ptn5110
>>> +    enum:
>>> +      - nxp,ptn5110
>>> +      - tcpci
>>
>> I don't think this is correct. First, this is binding for NXP chip, so
>> why generic implementation should be here? I would expect it in its own
>> dedicated binding.
> 
> The nxp,ptn5110 device was the first driver which implements an TCPCI
> conform driver. The driver already support the tcpci binding for i2c-id
> devices as I mentioned above. IMHO this whole binding (file) should be
> converted and the nxp,ptn5110 compatible should be marked as deprecated.

You speak about driver, but I was speaking about binding.
> 
>> Second, we rarely want generic compatibles. Care to share more details?
> 
> As said above this particular NXP chip is an TCPCI conform chip. There
> is nothing special about it. There are other vendors like OnSemi (in my
> case) which implement also an TCPCI conform chip. The (Linux) driver
> already binds to the generic tcpci compatible if the i2c-core falls back
> to the i2c-device id. It's even more confusing that the i2c-id supports
> only the generic binding the of-compatible support only the specifc one.

I don't know much about TCPCI, so maybe questions are obvious: you are
claiming that there will be no differentiating hardware element, like
reset-gpios or power supply for none of TCPCI-conforming chips? All of
them will never need any different hardware configuration?

Is this what you claim?

Just to remind: there was such claim for USB and PCI till we figured out
it was simply wrong and we are living now with on-board hubs and PCI
power-sequencing stuff.

> 
>> Are all details expected to follow spec, without need of quirks?
> 
> Please see above, I hope this helps.

Sorry, doesn't. You still speak about driver and how it can bind to
something. I did not ask about this at all.

To be clear:
WE ARE NOT TALKING ABOUT LINUX DRIVER.

We talk about hardware and how it is represented in Devicetree,
including its supplies, pins, GPIOs and any ideas hardware engineers
like to bring.

Best regards,
Krzysztof

Re: [PATCH 1/4] dt-bindings: usb: typec-tcpci: add tcpci compatible binding

[Marco Felsch]

On 24-02-06, Krzysztof Kozlowski wrote:
> On 06/02/2024 15:52, Marco Felsch wrote:
> > On 24-02-06, Krzysztof Kozlowski wrote:
> >> On 05/02/2024 17:43, Marco Felsch wrote:
> >>> This binding descripes the generic TCPCI specification [1]. So add the
> >>
> >> Typo: describes.
> > 
> > Argh.
> > 
> >> No, this binding describes PTN5110, not generic TCPCI. This is not
> >> accurate commit description.
> > 
> > This binding is currently missued if another TCPCI conform chip is used
> 
> Why would people misuse binding instead of doing things properly? :)

You know people... ;)

...

> >>>  properties:
> >>>    compatible:
> >>> -    const: nxp,ptn5110
> >>> +    enum:
> >>> +      - nxp,ptn5110
> >>> +      - tcpci
> >>
> >> I don't think this is correct. First, this is binding for NXP chip, so
> >> why generic implementation should be here? I would expect it in its own
> >> dedicated binding.
> > 
> > The nxp,ptn5110 device was the first driver which implements an TCPCI
> > conform driver. The driver already support the tcpci binding for i2c-id
> > devices as I mentioned above. IMHO this whole binding (file) should be
> > converted and the nxp,ptn5110 compatible should be marked as deprecated.
> 
> You speak about driver, but I was speaking about binding.

I know and I was afraid of mention the driver within this conversation
since this is all about bindings and devices :)

Nevertheless this particular NXP device does support the generic "tcpci"
compatible already. The support is pulled indirectly via the
i2c_device_id.name which is in the end used for of/acpi/legacy devices.

> >> Second, we rarely want generic compatibles. Care to share more details?
> > 
> > As said above this particular NXP chip is an TCPCI conform chip. There
> > is nothing special about it. There are other vendors like OnSemi (in my
> > case) which implement also an TCPCI conform chip. The (Linux) driver
> > already binds to the generic tcpci compatible if the i2c-core falls back
> > to the i2c-device id. It's even more confusing that the i2c-id supports
> > only the generic binding the of-compatible support only the specifc one.
> 
> I don't know much about TCPCI, so maybe questions are obvious: you are
> claiming that there will be no differentiating hardware element, like
> reset-gpios or power supply for none of TCPCI-conforming chips? All of
> them will never need any different hardware configuration?

Of course TCPCI doesn't mention reset gpios or power supplies but if you
use this argumentation the already supported NXP device shouldn't be
available too since the binding is missing the VDD supply ;) Since we
never break compatibility, the vdd-supply have to be optional and the
same can be done for reset-gpios.

> Is this what you claim?

Please see above.

> Just to remind: there was such claim for USB and PCI till we figured out
> it was simply wrong and we are living now with on-board hubs and PCI
> power-sequencing stuff.

Don't get me wrong, I get your point. In the end I don't care and can
copy'n'paste the whole file and change the compatible to the OnSemi
device or I can add the dedicated OnSemi compatible to this file. But I
don't wanted to add an 2nd specific compatible while the device already
supports the generic one but via i2c_device_id.name. Therefore I aligned
the i2c_device_id with the of_device_id.

> >> Are all details expected to follow spec, without need of quirks?
> > 
> > Please see above, I hope this helps.
> 
> Sorry, doesn't. You still speak about driver and how it can bind to
> something. I did not ask about this at all.
> 
> To be clear:
> WE ARE NOT TALKING ABOUT LINUX DRIVER.

I KNOW

> We talk about hardware and how it is represented in Devicetree,
> including its supplies, pins, GPIOs and any ideas hardware engineers
> like to bring.

I Know that too.

Regards,
  Marco

Re: [PATCH 1/4] dt-bindings: usb: typec-tcpci: add tcpci compatible binding

[Krzysztof Kozlowski]

On 07/02/2024 10:05, Marco Felsch wrote:
> On 24-02-06, Krzysztof Kozlowski wrote:
>> On 06/02/2024 15:52, Marco Felsch wrote:
>>> On 24-02-06, Krzysztof Kozlowski wrote:
>>>> On 05/02/2024 17:43, Marco Felsch wrote:
>>>>> This binding descripes the generic TCPCI specification [1]. So add the
>>>>
>>>> Typo: describes.
>>>
>>> Argh.
>>>
>>>> No, this binding describes PTN5110, not generic TCPCI. This is not
>>>> accurate commit description.
>>>
>>> This binding is currently missued if another TCPCI conform chip is used
>>
>> Why would people misuse binding instead of doing things properly? :)
> 
> You know people... ;)
> 
> ...
> 
>>>>>  properties:
>>>>>    compatible:
>>>>> -    const: nxp,ptn5110
>>>>> +    enum:
>>>>> +      - nxp,ptn5110
>>>>> +      - tcpci
>>>>
>>>> I don't think this is correct. First, this is binding for NXP chip, so
>>>> why generic implementation should be here? I would expect it in its own
>>>> dedicated binding.
>>>
>>> The nxp,ptn5110 device was the first driver which implements an TCPCI
>>> conform driver. The driver already support the tcpci binding for i2c-id
>>> devices as I mentioned above. IMHO this whole binding (file) should be
>>> converted and the nxp,ptn5110 compatible should be marked as deprecated.
>>
>> You speak about driver, but I was speaking about binding.
> 
> I know and I was afraid of mention the driver within this conversation
> since this is all about bindings and devices :)
> 
> Nevertheless this particular NXP device does support the generic "tcpci"
> compatible already. The support is pulled indirectly via the
> i2c_device_id.name which is in the end used for of/acpi/legacy devices.
> 
>>>> Second, we rarely want generic compatibles. Care to share more details?
>>>
>>> As said above this particular NXP chip is an TCPCI conform chip. There
>>> is nothing special about it. There are other vendors like OnSemi (in my
>>> case) which implement also an TCPCI conform chip. The (Linux) driver
>>> already binds to the generic tcpci compatible if the i2c-core falls back
>>> to the i2c-device id. It's even more confusing that the i2c-id supports
>>> only the generic binding the of-compatible support only the specifc one.
>>
>> I don't know much about TCPCI, so maybe questions are obvious: you are
>> claiming that there will be no differentiating hardware element, like
>> reset-gpios or power supply for none of TCPCI-conforming chips? All of
>> them will never need any different hardware configuration?
> 
> Of course TCPCI doesn't mention reset gpios or power supplies but if you
> use this argumentation the already supported NXP device shouldn't be
> available too since the binding is missing the VDD supply ;) Since we

The existing binding is incomplete and maybe, as you suggested, misused,
but this is not a reason to make it worse.

> never break compatibility, the vdd-supply have to be optional and the
> same can be done for reset-gpios.

So the answer to my questions is: They will not be 100% identical and
they will need customization?

> 
>> Is this what you claim?
> 
> Please see above.
> 
>> Just to remind: there was such claim for USB and PCI till we figured out
>> it was simply wrong and we are living now with on-board hubs and PCI
>> power-sequencing stuff.
> 
> Don't get me wrong, I get your point. In the end I don't care and can
> copy'n'paste the whole file and change the compatible to the OnSemi
> device or I can add the dedicated OnSemi compatible to this file. But I
> don't wanted to add an 2nd specific compatible while the device already
> supports the generic one but via i2c_device_id.name. Therefore I aligned
> the i2c_device_id with the of_device_id.

You can add generic compatible used as fallback. That's pretty common
practice.

> 
>>>> Are all details expected to follow spec, without need of quirks?
>>>
>>> Please see above, I hope this helps.
>>
>> Sorry, doesn't. You still speak about driver and how it can bind to
>> something. I did not ask about this at all.
>>
>> To be clear:
>> WE ARE NOT TALKING ABOUT LINUX DRIVER.
> 
> I KNOW
> 
>> We talk about hardware and how it is represented in Devicetree,
>> including its supplies, pins, GPIOs and any ideas hardware engineers
>> like to bring.

Then terms "driver" and "binding" (or matching) do not fit here as
arguments whether specific compatible should be there or not. There is
guideline for that: writing bindings, which exactly, 100% covers this
thing here.

Best regards,
Krzysztof

Re: [PATCH 1/4] dt-bindings: usb: typec-tcpci: add tcpci compatible binding

[Marco Felsch]

On 24-02-07, Krzysztof Kozlowski wrote:
> On 07/02/2024 10:05, Marco Felsch wrote:
> > On 24-02-06, Krzysztof Kozlowski wrote:
> >> On 06/02/2024 15:52, Marco Felsch wrote:
> >>> On 24-02-06, Krzysztof Kozlowski wrote:
> >>>> On 05/02/2024 17:43, Marco Felsch wrote:
> >>>>> This binding descripes the generic TCPCI specification [1]. So add the

...

> > Don't get me wrong, I get your point. In the end I don't care and can
> > copy'n'paste the whole file and change the compatible to the OnSemi
> > device or I can add the dedicated OnSemi compatible to this file. But I
> > don't wanted to add an 2nd specific compatible while the device already
> > supports the generic one but via i2c_device_id.name. Therefore I aligned
> > the i2c_device_id with the of_device_id.
> 
> You can add generic compatible used as fallback. That's pretty common
> practice.

Okay. To bring this discussion to an end, I will add the generic
compatible as fallback :)

Thanks,
  Marco

> 
> > 
> >>>> Are all details expected to follow spec, without need of quirks?
> >>>
> >>> Please see above, I hope this helps.
> >>
> >> Sorry, doesn't. You still speak about driver and how it can bind to
> >> something. I did not ask about this at all.
> >>
> >> To be clear:
> >> WE ARE NOT TALKING ABOUT LINUX DRIVER.
> > 
> > I KNOW
> > 
> >> We talk about hardware and how it is represented in Devicetree,
> >> including its supplies, pins, GPIOs and any ideas hardware engineers
> >> like to bring.
> 
> Then terms "driver" and "binding" (or matching) do not fit here as
> arguments whether specific compatible should be there or not. There is
> guideline for that: writing bindings, which exactly, 100% covers this
> thing here.
> 
> Best regards,
> Krzysztof
> 
>