[PATCH v1 1/3] usb: typec: tcpm: Add Callback to Usb Communication capable partner

[PATCH v1 1/3] usb: typec: tcpm: Add Callback to Usb Communication capable partner

[Badhri Jagan Sridharan]

The USB Communications Capable bit indicates if port
partner is capable of communication over the USB data lines
(e.g. D+/- or SS Tx/Rx). Notify the status of the bit to low
level drivers to perform chip specific operation.
For instance, low level driver enables USB switches on D+/D-
lines to set up data path when the bit is set.

Refactored from patch initially authored by
Kyle Tso <kyletso@google.com>

Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
---
 drivers/usb/typec/tcpm/tcpm.c | 16 ++++++++++++++++
 include/linux/usb/tcpm.h      |  5 +++++
 2 files changed, 21 insertions(+)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 0afd8ef692e8..96190b4d46a7 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -298,6 +298,7 @@ struct tcpm_port {
 	struct usb_pd_identity partner_ident;
 	struct typec_partner_desc partner_desc;
 	struct typec_partner *partner;
+	bool partner_usb_comm_capable;
 
 	enum typec_cc_status cc_req;
 
@@ -3429,6 +3430,16 @@ static void tcpm_unregister_altmodes(struct tcpm_port *port)
 	memset(modep, 0, sizeof(*modep));
 }
 
+static void tcpm_set_partner_usb_comm_capable(struct tcpm_port *port, bool capable)
+{
+	tcpm_log(port, "Setting usb_comm capable %s", capable ? "true" : "false");
+
+	if (port->tcpc->set_partner_usb_comm_capable)
+		port->tcpc->set_partner_usb_comm_capable(port->tcpc, capable);
+
+	port->partner_usb_comm_capable = capable;
+}
+
 static void tcpm_reset_port(struct tcpm_port *port)
 {
 	int ret;
@@ -3445,6 +3456,7 @@ static void tcpm_reset_port(struct tcpm_port *port)
 	port->attached = false;
 	port->pd_capable = false;
 	port->pps_data.supported = false;
+	tcpm_set_partner_usb_comm_capable(port, false);
 
 	/*
 	 * First Rx ID should be 0; set this to a sentinel of -1 so that
@@ -3785,6 +3797,8 @@ static void run_state_machine(struct tcpm_port *port)
 			}
 		} else {
 			tcpm_pd_send_control(port, PD_CTRL_ACCEPT);
+			port->partner_usb_comm_capable = port->sink_request & RDO_USB_COMM;
+			tcpm_set_partner_usb_comm_capable(port, port->partner_usb_comm_capable);
 			tcpm_set_state(port, SRC_TRANSITION_SUPPLY,
 				       PD_T_SRC_TRANSITION);
 		}
@@ -4004,6 +4018,8 @@ static void run_state_machine(struct tcpm_port *port)
 		break;
 	case SNK_NEGOTIATE_CAPABILITIES:
 		port->pd_capable = true;
+		port->partner_usb_comm_capable = port->sink_request & RDO_USB_COMM;
+		tcpm_set_partner_usb_comm_capable(port, port->partner_usb_comm_capable);
 		port->hard_reset_count = 0;
 		ret = tcpm_pd_send_request(port);
 		if (ret < 0) {
diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
index 3af99f85e8b9..42fcfbe10590 100644
--- a/include/linux/usb/tcpm.h
+++ b/include/linux/usb/tcpm.h
@@ -108,6 +108,10 @@ enum tcpm_transmit_type {
  *		is supported by TCPC, set this callback for TCPM to query
  *		whether vbus is at VSAFE0V when needed.
  *		Returns true when vbus is at VSAFE0V, false otherwise.
+ * @set_partner_usb_comm_capable:
+ *              Optional; The USB Communications Capable bit indicates if port
+ *              partner is capable of communication over the USB data lines
+ *              (e.g. D+/- or SS Tx/Rx). Called to notify the status of the bit.
  */
 struct tcpc_dev {
 	struct fwnode_handle *fwnode;
@@ -139,6 +143,7 @@ struct tcpc_dev {
 	int (*set_auto_vbus_discharge_threshold)(struct tcpc_dev *dev, enum typec_pwr_opmode mode,
 						 bool pps_active, u32 requested_vbus_voltage);
 	bool (*is_vbus_vsafe0v)(struct tcpc_dev *dev);
+	void (*set_partner_usb_comm_capable)(struct tcpc_dev *dev, bool enable);
 };
 
 struct tcpm_port;
-- 
2.30.0.365.g02bc693789-goog

[PATCH v1 2/3] usb: typec: tcpci: Add Callback to Usb Communication capable partner

[Badhri Jagan Sridharan]

The USB Communications Capable bit indicates if port
partner is capable of communication over the USB data lines
(e.g. D+/- or SS Tx/Rx). TCPM passes this information for chip specific
operations.

Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
---
 drivers/usb/typec/tcpm/tcpci.c | 9 +++++++++
 drivers/usb/typec/tcpm/tcpci.h | 6 ++++++
 2 files changed, 15 insertions(+)

diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
index f676abab044b..a27deb0b5f03 100644
--- a/drivers/usb/typec/tcpm/tcpci.c
+++ b/drivers/usb/typec/tcpm/tcpci.c
@@ -255,6 +255,14 @@ static int tcpci_set_polarity(struct tcpc_dev *tcpc,
 			   TCPC_TCPC_CTRL_ORIENTATION : 0);
 }
 
+static void tcpci_set_partner_usb_comm_capable(struct tcpc_dev *tcpc, bool capable)
+{
+	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
+
+	if (tcpci->data->set_partner_usb_comm_capable)
+		tcpci->data->set_partner_usb_comm_capable(tcpci, tcpci->data, capable);
+}
+
 static int tcpci_set_vconn(struct tcpc_dev *tcpc, bool enable)
 {
 	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
@@ -720,6 +728,7 @@ struct tcpci *tcpci_register_port(struct device *dev, struct tcpci_data *data)
 	tcpci->tcpc.set_bist_data = tcpci_set_bist_data;
 	tcpci->tcpc.enable_frs = tcpci_enable_frs;
 	tcpci->tcpc.frs_sourcing_vbus = tcpci_frs_sourcing_vbus;
+	tcpci->tcpc.set_partner_usb_comm_capable = tcpci_set_partner_usb_comm_capable;
 
 	if (tcpci->data->auto_discharge_disconnect) {
 		tcpci->tcpc.enable_auto_vbus_discharge = tcpci_enable_auto_vbus_discharge;
diff --git a/drivers/usb/typec/tcpm/tcpci.h b/drivers/usb/typec/tcpm/tcpci.h
index c3c7d07d9b4e..57b6e24e0a0c 100644
--- a/drivers/usb/typec/tcpm/tcpci.h
+++ b/drivers/usb/typec/tcpm/tcpci.h
@@ -161,6 +161,10 @@ struct tcpci;
  *		Optional; Enables TCPC to autonously discharge vbus on disconnect.
  * @vbus_vsafe0v:
  *		optional; Set when TCPC can detect whether vbus is at VSAFE0V.
+ * @set_partner_usb_comm_capable:
+ *		Optional; The USB Communications Capable bit indicates if port
+ *		partner is capable of communication over the USB data lines
+ *		(e.g. D+/- or SS Tx/Rx). Called to notify the status of the bit.
  */
 struct tcpci_data {
 	struct regmap *regmap;
@@ -175,6 +179,8 @@ struct tcpci_data {
 				  enum typec_cc_status cc);
 	int (*set_vbus)(struct tcpci *tcpci, struct tcpci_data *data, bool source, bool sink);
 	void (*frs_sourcing_vbus)(struct tcpci *tcpci, struct tcpci_data *data);
+	void (*set_partner_usb_comm_capable)(struct tcpci *tcpci, struct tcpci_data *data,
+					     bool capable);
 };
 
 struct tcpci *tcpci_register_port(struct device *dev, struct tcpci_data *data);
-- 
2.30.0.365.g02bc693789-goog

[PATCH v1 3/3] usb: typec: tcpci_maxim: Enable data path when partner is USB Comm capable

[Badhri Jagan Sridharan]

Configure USB switches when partner is USB Communication capable.
The is enabled USB data communication over D+/D- pins.

Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
---
 drivers/usb/typec/tcpm/tcpci_maxim.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/usb/typec/tcpm/tcpci_maxim.c b/drivers/usb/typec/tcpm/tcpci_maxim.c
index f1674a611033..041a1c393594 100644
--- a/drivers/usb/typec/tcpm/tcpci_maxim.c
+++ b/drivers/usb/typec/tcpm/tcpci_maxim.c
@@ -19,6 +19,9 @@
 #define PD_ACTIVITY_TIMEOUT_MS				10000
 
 #define TCPC_VENDOR_ALERT				0x80
+#define TCPC_VENDOR_USBSW_CTRL				0x93
+#define TCPC_VENDOR_USBSW_CTRL_ENABLE_USB_DATA		0x9
+#define TCPC_VENDOR_USBSW_CTRL_DISABLE_USB_DATA		0
 
 #define TCPC_RECEIVE_BUFFER_COUNT_OFFSET		0
 #define TCPC_RECEIVE_BUFFER_FRAME_TYPE_OFFSET		1
@@ -274,6 +277,21 @@ static void process_tx(struct max_tcpci_chip *chip, u16 status)
 		max_tcpci_init_regs(chip);
 }
 
+/* Enable USB switches when partner is USB communications capable */
+static void max_tcpci_set_partner_usb_comm_capable(struct tcpci *tcpci, struct tcpci_data *data,
+						   bool capable)
+{
+	struct max_tcpci_chip *chip = tdata_to_max_tcpci(data);
+	int ret;
+
+	ret = max_tcpci_write8(chip, TCPC_VENDOR_USBSW_CTRL, capable ?
+			       TCPC_VENDOR_USBSW_CTRL_ENABLE_USB_DATA :
+			       TCPC_VENDOR_USBSW_CTRL_DISABLE_USB_DATA);
+
+	if (ret < 0)
+		dev_err(chip->dev, "Failed to enable USB switches");
+}
+
 static irqreturn_t _max_tcpci_irq(struct max_tcpci_chip *chip, u16 status)
 {
 	u16 mask;
@@ -453,6 +471,7 @@ static int max_tcpci_probe(struct i2c_client *client, const struct i2c_device_id
 	chip->data.frs_sourcing_vbus = max_tcpci_frs_sourcing_vbus;
 	chip->data.auto_discharge_disconnect = true;
 	chip->data.vbus_vsafe0v = true;
+	chip->data.set_partner_usb_comm_capable = max_tcpci_set_partner_usb_comm_capable;
 
 	max_tcpci_init_regs(chip);
 	chip->tcpci = tcpci_register_port(chip->dev, &chip->data);
-- 
2.30.0.365.g02bc693789-goog

Re: [PATCH v1 1/3] usb: typec: tcpm: Add Callback to Usb Communication capable partner

[Guenter Roeck]

On 2/1/21 1:53 AM, Badhri Jagan Sridharan wrote:
> The USB Communications Capable bit indicates if port
> partner is capable of communication over the USB data lines
> (e.g. D+/- or SS Tx/Rx). Notify the status of the bit to low
> level drivers to perform chip specific operation.
> For instance, low level driver enables USB switches on D+/D-
> lines to set up data path when the bit is set.
> 
> Refactored from patch initially authored by
> Kyle Tso <kyletso@google.com>
> 
> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 16 ++++++++++++++++
>  include/linux/usb/tcpm.h      |  5 +++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 0afd8ef692e8..96190b4d46a7 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -298,6 +298,7 @@ struct tcpm_port {
>  	struct usb_pd_identity partner_ident;
>  	struct typec_partner_desc partner_desc;
>  	struct typec_partner *partner;
> +	bool partner_usb_comm_capable;
>  
>  	enum typec_cc_status cc_req;
>  
> @@ -3429,6 +3430,16 @@ static void tcpm_unregister_altmodes(struct tcpm_port *port)
>  	memset(modep, 0, sizeof(*modep));
>  }
>  
> +static void tcpm_set_partner_usb_comm_capable(struct tcpm_port *port, bool capable)
> +{
> +	tcpm_log(port, "Setting usb_comm capable %s", capable ? "true" : "false");
> +
> +	if (port->tcpc->set_partner_usb_comm_capable)
> +		port->tcpc->set_partner_usb_comm_capable(port->tcpc, capable);
> +
> +	port->partner_usb_comm_capable = capable;
> +}
> +
>  static void tcpm_reset_port(struct tcpm_port *port)
>  {
>  	int ret;
> @@ -3445,6 +3456,7 @@ static void tcpm_reset_port(struct tcpm_port *port)
>  	port->attached = false;
>  	port->pd_capable = false;
>  	port->pps_data.supported = false;
> +	tcpm_set_partner_usb_comm_capable(port, false);
>  
>  	/*
>  	 * First Rx ID should be 0; set this to a sentinel of -1 so that
> @@ -3785,6 +3797,8 @@ static void run_state_machine(struct tcpm_port *port)
>  			}
>  		} else {
>  			tcpm_pd_send_control(port, PD_CTRL_ACCEPT);
> +			port->partner_usb_comm_capable = port->sink_request & RDO_USB_COMM;

That seems to be redundant since tcpm_set_partner_usb_comm_capable() is setting it again.

> +			tcpm_set_partner_usb_comm_capable(port, port->partner_usb_comm_capable);
>  			tcpm_set_state(port, SRC_TRANSITION_SUPPLY,
>  				       PD_T_SRC_TRANSITION);
>  		}
> @@ -4004,6 +4018,8 @@ static void run_state_machine(struct tcpm_port *port)
>  		break;
>  	case SNK_NEGOTIATE_CAPABILITIES:
>  		port->pd_capable = true;
> +		port->partner_usb_comm_capable = port->sink_request & RDO_USB_COMM;

Same here. But then I don't really see where this variable is actually used.
Am I missing something ?

Thanks,
Guenter

> +		tcpm_set_partner_usb_comm_capable(port, port->partner_usb_comm_capable);
>  		port->hard_reset_count = 0;
>  		ret = tcpm_pd_send_request(port);
>  		if (ret < 0) {
> diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
> index 3af99f85e8b9..42fcfbe10590 100644
> --- a/include/linux/usb/tcpm.h
> +++ b/include/linux/usb/tcpm.h
> @@ -108,6 +108,10 @@ enum tcpm_transmit_type {
>   *		is supported by TCPC, set this callback for TCPM to query
>   *		whether vbus is at VSAFE0V when needed.
>   *		Returns true when vbus is at VSAFE0V, false otherwise.
> + * @set_partner_usb_comm_capable:
> + *              Optional; The USB Communications Capable bit indicates if port
> + *              partner is capable of communication over the USB data lines
> + *              (e.g. D+/- or SS Tx/Rx). Called to notify the status of the bit.
>   */
>  struct tcpc_dev {
>  	struct fwnode_handle *fwnode;
> @@ -139,6 +143,7 @@ struct tcpc_dev {
>  	int (*set_auto_vbus_discharge_threshold)(struct tcpc_dev *dev, enum typec_pwr_opmode mode,
>  						 bool pps_active, u32 requested_vbus_voltage);
>  	bool (*is_vbus_vsafe0v)(struct tcpc_dev *dev);
> +	void (*set_partner_usb_comm_capable)(struct tcpc_dev *dev, bool enable);
>  };
>  
>  struct tcpm_port;
> 

Re: [PATCH v1 1/3] usb: typec: tcpm: Add Callback to Usb Communication capable partner

[Heikki Krogerus]

On Mon, Feb 01, 2021 at 01:53:07AM -0800, Badhri Jagan Sridharan wrote:
> The USB Communications Capable bit indicates if port
> partner is capable of communication over the USB data lines
> (e.g. D+/- or SS Tx/Rx). Notify the status of the bit to low
> level drivers to perform chip specific operation.
> For instance, low level driver enables USB switches on D+/D-
> lines to set up data path when the bit is set.
> 
> Refactored from patch initially authored by
> Kyle Tso <kyletso@google.com>
> 
> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 16 ++++++++++++++++
>  include/linux/usb/tcpm.h      |  5 +++++
>  2 files changed, 21 insertions(+)

...

> diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
> index 3af99f85e8b9..42fcfbe10590 100644
> --- a/include/linux/usb/tcpm.h
> +++ b/include/linux/usb/tcpm.h
> @@ -108,6 +108,10 @@ enum tcpm_transmit_type {
>   *		is supported by TCPC, set this callback for TCPM to query
>   *		whether vbus is at VSAFE0V when needed.
>   *		Returns true when vbus is at VSAFE0V, false otherwise.
> + * @set_partner_usb_comm_capable:
> + *              Optional; The USB Communications Capable bit indicates if port
> + *              partner is capable of communication over the USB data lines
> + *              (e.g. D+/- or SS Tx/Rx). Called to notify the status of the bit.
>   */
>  struct tcpc_dev {
>  	struct fwnode_handle *fwnode;
> @@ -139,6 +143,7 @@ struct tcpc_dev {
>  	int (*set_auto_vbus_discharge_threshold)(struct tcpc_dev *dev, enum typec_pwr_opmode mode,
>  						 bool pps_active, u32 requested_vbus_voltage);
>  	bool (*is_vbus_vsafe0v)(struct tcpc_dev *dev);
> +	void (*set_partner_usb_comm_capable)(struct tcpc_dev *dev, bool enable);
>  };
>  
>  struct tcpm_port;

There start to be a lot of callback there, separate for each function.
And I guess flags too... Would it be possible to have a single
notification callback instead, that would take the type of the
notification as a parameter (we could have an enum for those), and
then the specific object(s) for each type as another paramter (RDO I
guess in this case)?

It would then be up to the TCPC driver to extract the detail it needs
from that object. That would somehow feel more cleaner to me, but what
do you guys think?

thanks,

-- 
heikki

Re: [PATCH v1 1/3] usb: typec: tcpm: Add Callback to Usb Communication capable partner

[Greg Kroah-Hartman]

On Mon, Feb 01, 2021 at 05:12:53PM +0200, Heikki Krogerus wrote:
> On Mon, Feb 01, 2021 at 01:53:07AM -0800, Badhri Jagan Sridharan wrote:
> > The USB Communications Capable bit indicates if port
> > partner is capable of communication over the USB data lines
> > (e.g. D+/- or SS Tx/Rx). Notify the status of the bit to low
> > level drivers to perform chip specific operation.
> > For instance, low level driver enables USB switches on D+/D-
> > lines to set up data path when the bit is set.
> > 
> > Refactored from patch initially authored by
> > Kyle Tso <kyletso@google.com>
> > 
> > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> > ---
> >  drivers/usb/typec/tcpm/tcpm.c | 16 ++++++++++++++++
> >  include/linux/usb/tcpm.h      |  5 +++++
> >  2 files changed, 21 insertions(+)
> 
> ...
> 
> > diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
> > index 3af99f85e8b9..42fcfbe10590 100644
> > --- a/include/linux/usb/tcpm.h
> > +++ b/include/linux/usb/tcpm.h
> > @@ -108,6 +108,10 @@ enum tcpm_transmit_type {
> >   *		is supported by TCPC, set this callback for TCPM to query
> >   *		whether vbus is at VSAFE0V when needed.
> >   *		Returns true when vbus is at VSAFE0V, false otherwise.
> > + * @set_partner_usb_comm_capable:
> > + *              Optional; The USB Communications Capable bit indicates if port
> > + *              partner is capable of communication over the USB data lines
> > + *              (e.g. D+/- or SS Tx/Rx). Called to notify the status of the bit.
> >   */
> >  struct tcpc_dev {
> >  	struct fwnode_handle *fwnode;
> > @@ -139,6 +143,7 @@ struct tcpc_dev {
> >  	int (*set_auto_vbus_discharge_threshold)(struct tcpc_dev *dev, enum typec_pwr_opmode mode,
> >  						 bool pps_active, u32 requested_vbus_voltage);
> >  	bool (*is_vbus_vsafe0v)(struct tcpc_dev *dev);
> > +	void (*set_partner_usb_comm_capable)(struct tcpc_dev *dev, bool enable);
> >  };
> >  
> >  struct tcpm_port;
> 
> There start to be a lot of callback there, separate for each function.
> And I guess flags too... Would it be possible to have a single
> notification callback instead, that would take the type of the
> notification as a parameter (we could have an enum for those), and
> then the specific object(s) for each type as another paramter (RDO I
> guess in this case)?
> 
> It would then be up to the TCPC driver to extract the detail it needs
> from that object. That would somehow feel more cleaner to me, but what
> do you guys think?

It's pretty much the same thing, a "mux" function vs. individual
function calls.  Personally, individual callbacks are much more
explicit, and I think make it easier to determine what is really going
on in each driver.

But it all does the same thing, if there's going to be loads of
callbacks needed, then a single one makes it easier to maintain over
time.

So it's up to the maintainer what they want to see :)

thanks,

greg k-h

Re: [PATCH v1 1/3] usb: typec: tcpm: Add Callback to Usb Communication capable partner

[Heikki Krogerus]

On Mon, Feb 01, 2021 at 04:19:38PM +0100, Greg Kroah-Hartman wrote:
> On Mon, Feb 01, 2021 at 05:12:53PM +0200, Heikki Krogerus wrote:
> > On Mon, Feb 01, 2021 at 01:53:07AM -0800, Badhri Jagan Sridharan wrote:
> > > The USB Communications Capable bit indicates if port
> > > partner is capable of communication over the USB data lines
> > > (e.g. D+/- or SS Tx/Rx). Notify the status of the bit to low
> > > level drivers to perform chip specific operation.
> > > For instance, low level driver enables USB switches on D+/D-
> > > lines to set up data path when the bit is set.
> > > 
> > > Refactored from patch initially authored by
> > > Kyle Tso <kyletso@google.com>
> > > 
> > > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> > > ---
> > >  drivers/usb/typec/tcpm/tcpm.c | 16 ++++++++++++++++
> > >  include/linux/usb/tcpm.h      |  5 +++++
> > >  2 files changed, 21 insertions(+)
> > 
> > ...
> > 
> > > diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
> > > index 3af99f85e8b9..42fcfbe10590 100644
> > > --- a/include/linux/usb/tcpm.h
> > > +++ b/include/linux/usb/tcpm.h
> > > @@ -108,6 +108,10 @@ enum tcpm_transmit_type {
> > >   *		is supported by TCPC, set this callback for TCPM to query
> > >   *		whether vbus is at VSAFE0V when needed.
> > >   *		Returns true when vbus is at VSAFE0V, false otherwise.
> > > + * @set_partner_usb_comm_capable:
> > > + *              Optional; The USB Communications Capable bit indicates if port
> > > + *              partner is capable of communication over the USB data lines
> > > + *              (e.g. D+/- or SS Tx/Rx). Called to notify the status of the bit.
> > >   */
> > >  struct tcpc_dev {
> > >  	struct fwnode_handle *fwnode;
> > > @@ -139,6 +143,7 @@ struct tcpc_dev {
> > >  	int (*set_auto_vbus_discharge_threshold)(struct tcpc_dev *dev, enum typec_pwr_opmode mode,
> > >  						 bool pps_active, u32 requested_vbus_voltage);
> > >  	bool (*is_vbus_vsafe0v)(struct tcpc_dev *dev);
> > > +	void (*set_partner_usb_comm_capable)(struct tcpc_dev *dev, bool enable);
> > >  };
> > >  
> > >  struct tcpm_port;
> > 
> > There start to be a lot of callback there, separate for each function.
> > And I guess flags too... Would it be possible to have a single
> > notification callback instead, that would take the type of the
> > notification as a parameter (we could have an enum for those), and
> > then the specific object(s) for each type as another paramter (RDO I
> > guess in this case)?
> > 
> > It would then be up to the TCPC driver to extract the detail it needs
> > from that object. That would somehow feel more cleaner to me, but what
> > do you guys think?
> 
> It's pretty much the same thing, a "mux" function vs. individual
> function calls.  Personally, individual callbacks are much more
> explicit, and I think make it easier to determine what is really going
> on in each driver.
> 
> But it all does the same thing, if there's going to be loads of
> callbacks needed, then a single one makes it easier to maintain over
> time.
> 
> So it's up to the maintainer what they want to see :)

I understand your point, and I guess a "generic" notification callback
for all that would not be a good idea. However, right now it looks
like we are picking individual bits from various PD objects with those
callbacks, and that does not feel ideal to me either. After all, each of
those bits has its own flag now, even though the details is just
extracted from some PD object that we should also have access to.

I think there are ways we can improve this for example by attempting
to create the notifications per transaction instead of for each
individual result of those transactions. That way we would not need to
store the flags at least because we could deliver the entire object
that was the result of the specific transaction.

So basically, I fear that dealing with these individual bits will in
many case only serve individual device drivers, and in the worst case
start making the tcpm.c a bit more difficult to manage if we start to
have more and more of these bit callbacks.

But on the other hand, I guess we are nowhere near that point, so
let's forget about this for now.


thanks,

-- 
heikki

Re: [PATCH v1 1/3] usb: typec: tcpm: Add Callback to Usb Communication capable partner

[Greg Kroah-Hartman]

On Mon, Feb 01, 2021 at 06:09:25PM +0200, Heikki Krogerus wrote:
> On Mon, Feb 01, 2021 at 04:19:38PM +0100, Greg Kroah-Hartman wrote:
> > On Mon, Feb 01, 2021 at 05:12:53PM +0200, Heikki Krogerus wrote:
> > > On Mon, Feb 01, 2021 at 01:53:07AM -0800, Badhri Jagan Sridharan wrote:
> > > > The USB Communications Capable bit indicates if port
> > > > partner is capable of communication over the USB data lines
> > > > (e.g. D+/- or SS Tx/Rx). Notify the status of the bit to low
> > > > level drivers to perform chip specific operation.
> > > > For instance, low level driver enables USB switches on D+/D-
> > > > lines to set up data path when the bit is set.
> > > > 
> > > > Refactored from patch initially authored by
> > > > Kyle Tso <kyletso@google.com>
> > > > 
> > > > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> > > > ---
> > > >  drivers/usb/typec/tcpm/tcpm.c | 16 ++++++++++++++++
> > > >  include/linux/usb/tcpm.h      |  5 +++++
> > > >  2 files changed, 21 insertions(+)
> > > 
> > > ...
> > > 
> > > > diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
> > > > index 3af99f85e8b9..42fcfbe10590 100644
> > > > --- a/include/linux/usb/tcpm.h
> > > > +++ b/include/linux/usb/tcpm.h
> > > > @@ -108,6 +108,10 @@ enum tcpm_transmit_type {
> > > >   *		is supported by TCPC, set this callback for TCPM to query
> > > >   *		whether vbus is at VSAFE0V when needed.
> > > >   *		Returns true when vbus is at VSAFE0V, false otherwise.
> > > > + * @set_partner_usb_comm_capable:
> > > > + *              Optional; The USB Communications Capable bit indicates if port
> > > > + *              partner is capable of communication over the USB data lines
> > > > + *              (e.g. D+/- or SS Tx/Rx). Called to notify the status of the bit.
> > > >   */
> > > >  struct tcpc_dev {
> > > >  	struct fwnode_handle *fwnode;
> > > > @@ -139,6 +143,7 @@ struct tcpc_dev {
> > > >  	int (*set_auto_vbus_discharge_threshold)(struct tcpc_dev *dev, enum typec_pwr_opmode mode,
> > > >  						 bool pps_active, u32 requested_vbus_voltage);
> > > >  	bool (*is_vbus_vsafe0v)(struct tcpc_dev *dev);
> > > > +	void (*set_partner_usb_comm_capable)(struct tcpc_dev *dev, bool enable);
> > > >  };
> > > >  
> > > >  struct tcpm_port;
> > > 
> > > There start to be a lot of callback there, separate for each function.
> > > And I guess flags too... Would it be possible to have a single
> > > notification callback instead, that would take the type of the
> > > notification as a parameter (we could have an enum for those), and
> > > then the specific object(s) for each type as another paramter (RDO I
> > > guess in this case)?
> > > 
> > > It would then be up to the TCPC driver to extract the detail it needs
> > > from that object. That would somehow feel more cleaner to me, but what
> > > do you guys think?
> > 
> > It's pretty much the same thing, a "mux" function vs. individual
> > function calls.  Personally, individual callbacks are much more
> > explicit, and I think make it easier to determine what is really going
> > on in each driver.
> > 
> > But it all does the same thing, if there's going to be loads of
> > callbacks needed, then a single one makes it easier to maintain over
> > time.
> > 
> > So it's up to the maintainer what they want to see :)
> 
> I understand your point, and I guess a "generic" notification callback
> for all that would not be a good idea. However, right now it looks
> like we are picking individual bits from various PD objects with those
> callbacks, and that does not feel ideal to me either. After all, each of
> those bits has its own flag now, even though the details is just
> extracted from some PD object that we should also have access to.
> 
> I think there are ways we can improve this for example by attempting
> to create the notifications per transaction instead of for each
> individual result of those transactions. That way we would not need to
> store the flags at least because we could deliver the entire object
> that was the result of the specific transaction.
> 
> So basically, I fear that dealing with these individual bits will in
> many case only serve individual device drivers, and in the worst case
> start making the tcpm.c a bit more difficult to manage if we start to
> have more and more of these bit callbacks.
> 
> But on the other hand, I guess we are nowhere near that point, so
> let's forget about this for now.

If it gets unwieldy, we can always change it in the future, there's no
reason these types of in-kernel apis can not be modified and cleaned up
over time.

thanks,

greg k-h

Re: [PATCH v1 1/3] usb: typec: tcpm: Add Callback to Usb Communication capable partner

[Guenter Roeck]

On 2/1/21 8:38 AM, Greg Kroah-Hartman wrote:
> On Mon, Feb 01, 2021 at 06:09:25PM +0200, Heikki Krogerus wrote:
>> On Mon, Feb 01, 2021 at 04:19:38PM +0100, Greg Kroah-Hartman wrote:
>>> On Mon, Feb 01, 2021 at 05:12:53PM +0200, Heikki Krogerus wrote:
>>>> On Mon, Feb 01, 2021 at 01:53:07AM -0800, Badhri Jagan Sridharan wrote:
>>>>> The USB Communications Capable bit indicates if port
>>>>> partner is capable of communication over the USB data lines
>>>>> (e.g. D+/- or SS Tx/Rx). Notify the status of the bit to low
>>>>> level drivers to perform chip specific operation.
>>>>> For instance, low level driver enables USB switches on D+/D-
>>>>> lines to set up data path when the bit is set.
>>>>>
>>>>> Refactored from patch initially authored by
>>>>> Kyle Tso <kyletso@google.com>
>>>>>
>>>>> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
>>>>> ---
>>>>>  drivers/usb/typec/tcpm/tcpm.c | 16 ++++++++++++++++
>>>>>  include/linux/usb/tcpm.h      |  5 +++++
>>>>>  2 files changed, 21 insertions(+)
>>>>
>>>> ...
>>>>
>>>>> diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
>>>>> index 3af99f85e8b9..42fcfbe10590 100644
>>>>> --- a/include/linux/usb/tcpm.h
>>>>> +++ b/include/linux/usb/tcpm.h
>>>>> @@ -108,6 +108,10 @@ enum tcpm_transmit_type {
>>>>>   *		is supported by TCPC, set this callback for TCPM to query
>>>>>   *		whether vbus is at VSAFE0V when needed.
>>>>>   *		Returns true when vbus is at VSAFE0V, false otherwise.
>>>>> + * @set_partner_usb_comm_capable:
>>>>> + *              Optional; The USB Communications Capable bit indicates if port
>>>>> + *              partner is capable of communication over the USB data lines
>>>>> + *              (e.g. D+/- or SS Tx/Rx). Called to notify the status of the bit.
>>>>>   */
>>>>>  struct tcpc_dev {
>>>>>  	struct fwnode_handle *fwnode;
>>>>> @@ -139,6 +143,7 @@ struct tcpc_dev {
>>>>>  	int (*set_auto_vbus_discharge_threshold)(struct tcpc_dev *dev, enum typec_pwr_opmode mode,
>>>>>  						 bool pps_active, u32 requested_vbus_voltage);
>>>>>  	bool (*is_vbus_vsafe0v)(struct tcpc_dev *dev);
>>>>> +	void (*set_partner_usb_comm_capable)(struct tcpc_dev *dev, bool enable);
>>>>>  };
>>>>>  
>>>>>  struct tcpm_port;
>>>>
>>>> There start to be a lot of callback there, separate for each function.
>>>> And I guess flags too... Would it be possible to have a single
>>>> notification callback instead, that would take the type of the
>>>> notification as a parameter (we could have an enum for those), and
>>>> then the specific object(s) for each type as another paramter (RDO I
>>>> guess in this case)?
>>>>
>>>> It would then be up to the TCPC driver to extract the detail it needs
>>>> from that object. That would somehow feel more cleaner to me, but what
>>>> do you guys think?
>>>
>>> It's pretty much the same thing, a "mux" function vs. individual
>>> function calls.  Personally, individual callbacks are much more
>>> explicit, and I think make it easier to determine what is really going
>>> on in each driver.
>>>
>>> But it all does the same thing, if there's going to be loads of
>>> callbacks needed, then a single one makes it easier to maintain over
>>> time.
>>>
>>> So it's up to the maintainer what they want to see :)
>>
>> I understand your point, and I guess a "generic" notification callback
>> for all that would not be a good idea. However, right now it looks
>> like we are picking individual bits from various PD objects with those
>> callbacks, and that does not feel ideal to me either. After all, each of
>> those bits has its own flag now, even though the details is just
>> extracted from some PD object that we should also have access to.
>>
>> I think there are ways we can improve this for example by attempting
>> to create the notifications per transaction instead of for each
>> individual result of those transactions. That way we would not need to
>> store the flags at least because we could deliver the entire object
>> that was the result of the specific transaction.
>>
>> So basically, I fear that dealing with these individual bits will in
>> many case only serve individual device drivers, and in the worst case
>> start making the tcpm.c a bit more difficult to manage if we start to
>> have more and more of these bit callbacks.
>>
>> But on the other hand, I guess we are nowhere near that point, so
>> let's forget about this for now.
> 
> If it gets unwieldy, we can always change it in the future, there's no
> reason these types of in-kernel apis can not be modified and cleaned up
> over time.
> 
Agreed.

Thanks,
Guenter

Re: [PATCH v1 1/3] usb: typec: tcpm: Add Callback to Usb Communication capable partner

[Badhri Jagan Sridharan]

On Mon, Feb 1, 2021 at 6:59 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 2/1/21 1:53 AM, Badhri Jagan Sridharan wrote:
> > The USB Communications Capable bit indicates if port
> > partner is capable of communication over the USB data lines
> > (e.g. D+/- or SS Tx/Rx). Notify the status of the bit to low
> > level drivers to perform chip specific operation.
> > For instance, low level driver enables USB switches on D+/D-
> > lines to set up data path when the bit is set.
> >
> > Refactored from patch initially authored by
> > Kyle Tso <kyletso@google.com>
> >
> > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> > ---
> >  drivers/usb/typec/tcpm/tcpm.c | 16 ++++++++++++++++
> >  include/linux/usb/tcpm.h      |  5 +++++
> >  2 files changed, 21 insertions(+)
> >
> > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> > index 0afd8ef692e8..96190b4d46a7 100644
> > --- a/drivers/usb/typec/tcpm/tcpm.c
> > +++ b/drivers/usb/typec/tcpm/tcpm.c
> > @@ -298,6 +298,7 @@ struct tcpm_port {
> >       struct usb_pd_identity partner_ident;
> >       struct typec_partner_desc partner_desc;
> >       struct typec_partner *partner;
> > +     bool partner_usb_comm_capable;
> >
> >       enum typec_cc_status cc_req;
> >
> > @@ -3429,6 +3430,16 @@ static void tcpm_unregister_altmodes(struct tcpm_port *port)
> >       memset(modep, 0, sizeof(*modep));
> >  }
> >
> > +static void tcpm_set_partner_usb_comm_capable(struct tcpm_port *port, bool capable)
> > +{
> > +     tcpm_log(port, "Setting usb_comm capable %s", capable ? "true" : "false");
> > +
> > +     if (port->tcpc->set_partner_usb_comm_capable)
> > +             port->tcpc->set_partner_usb_comm_capable(port->tcpc, capable);
> > +
> > +     port->partner_usb_comm_capable = capable;
> > +}
> > +
> >  static void tcpm_reset_port(struct tcpm_port *port)
> >  {
> >       int ret;
> > @@ -3445,6 +3456,7 @@ static void tcpm_reset_port(struct tcpm_port *port)
> >       port->attached = false;
> >       port->pd_capable = false;
> >       port->pps_data.supported = false;
> > +     tcpm_set_partner_usb_comm_capable(port, false);
> >
> >       /*
> >        * First Rx ID should be 0; set this to a sentinel of -1 so that
> > @@ -3785,6 +3797,8 @@ static void run_state_machine(struct tcpm_port *port)
> >                       }
> >               } else {
> >                       tcpm_pd_send_control(port, PD_CTRL_ACCEPT);
> > +                     port->partner_usb_comm_capable = port->sink_request & RDO_USB_COMM;
>
> That seems to be redundant since tcpm_set_partner_usb_comm_capable() is setting it again.

You are correct. This is redundant. Removing in V2 version.

>
> > +                     tcpm_set_partner_usb_comm_capable(port, port->partner_usb_comm_capable);
> >                       tcpm_set_state(port, SRC_TRANSITION_SUPPLY,
> >                                      PD_T_SRC_TRANSITION);
> >               }
> > @@ -4004,6 +4018,8 @@ static void run_state_machine(struct tcpm_port *port)
> >               break;
> >       case SNK_NEGOTIATE_CAPABILITIES:
> >               port->pd_capable = true;
> > +             port->partner_usb_comm_capable = port->sink_request & RDO_USB_COMM;
>
> Same here. But then I don't really see where this variable is actually used.
> Am I missing something ?

Not used anywhere else. Removing this in V2.
Also fixing the check for this case. It should have been
port->source_caps[0] & PDO_FIXED_USB_COMM

Thanks for the reviews,
Badhri

>
> Thanks,
> Guenter
>
> > +             tcpm_set_partner_usb_comm_capable(port, port->partner_usb_comm_capable);
> >               port->hard_reset_count = 0;
> >               ret = tcpm_pd_send_request(port);
> >               if (ret < 0) {
> > diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
> > index 3af99f85e8b9..42fcfbe10590 100644
> > --- a/include/linux/usb/tcpm.h
> > +++ b/include/linux/usb/tcpm.h
> > @@ -108,6 +108,10 @@ enum tcpm_transmit_type {
> >   *           is supported by TCPC, set this callback for TCPM to query
> >   *           whether vbus is at VSAFE0V when needed.
> >   *           Returns true when vbus is at VSAFE0V, false otherwise.
> > + * @set_partner_usb_comm_capable:
> > + *              Optional; The USB Communications Capable bit indicates if port
> > + *              partner is capable of communication over the USB data lines
> > + *              (e.g. D+/- or SS Tx/Rx). Called to notify the status of the bit.
> >   */
> >  struct tcpc_dev {
> >       struct fwnode_handle *fwnode;
> > @@ -139,6 +143,7 @@ struct tcpc_dev {
> >       int (*set_auto_vbus_discharge_threshold)(struct tcpc_dev *dev, enum typec_pwr_opmode mode,
> >                                                bool pps_active, u32 requested_vbus_voltage);
> >       bool (*is_vbus_vsafe0v)(struct tcpc_dev *dev);
> > +     void (*set_partner_usb_comm_capable)(struct tcpc_dev *dev, bool enable);
> >  };
> >
> >  struct tcpm_port;
> >
>