[PATCH v2 1/2] usb: typec: tcpci: Add tcpc chip level callbacks

[PATCH v2 1/2] usb: typec: tcpci: Add tcpc chip level callbacks

[Badhri Jagan Sridharan]

This change adds chip callbacks for the following operations:
1. Notifying port role
2. Notifying orientation

Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
---
Changes since V1:
- Dropped changes related to get_/set_current_limit and pd_capable
  callback. Will send them in as separate patches.
---
 drivers/usb/typec/tcpm/tcpci.c | 12 +++++++++++-
 drivers/usb/typec/tcpm/tcpci.h | 10 ++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
index 027afd7dfdce..c37bbb2e9961 100644
--- a/drivers/usb/typec/tcpm/tcpci.c
+++ b/drivers/usb/typec/tcpm/tcpci.c
@@ -265,9 +265,16 @@ static int tcpci_set_polarity(struct tcpc_dev *tcpc,
 	if (ret < 0)
 		return ret;
 
-	return regmap_write(tcpci->regmap, TCPC_TCPC_CTRL,
+	ret = regmap_write(tcpci->regmap, TCPC_TCPC_CTRL,
 			   (polarity == TYPEC_POLARITY_CC2) ?
 			   TCPC_TCPC_CTRL_ORIENTATION : 0);
+	if (ret < 0)
+		return ret;
+
+	if (tcpci->data->set_cc_polarity)
+		tcpci->data->set_cc_polarity(tcpci, tcpci->data, polarity);
+
+	return ret;
 }
 
 static void tcpci_set_partner_usb_comm_capable(struct tcpc_dev *tcpc, bool capable)
@@ -395,6 +402,9 @@ static int tcpci_set_roles(struct tcpc_dev *tcpc, bool attached,
 	if (ret < 0)
 		return ret;
 
+	if (tcpci->data->set_roles)
+		tcpci->data->set_roles(tcpci, tcpci->data, attached, role, data);
+
 	return 0;
 }
 
diff --git a/drivers/usb/typec/tcpm/tcpci.h b/drivers/usb/typec/tcpm/tcpci.h
index 57b6e24e0a0c..75a2c9b575b5 100644
--- a/drivers/usb/typec/tcpm/tcpci.h
+++ b/drivers/usb/typec/tcpm/tcpci.h
@@ -165,6 +165,12 @@ struct tcpci;
  *		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.
+ * @set_roles:
+ *		Optional; This callback notifies the TCPC driver of the resolved
+ *		port role and attached status.
+ * @set_cc_polarity:
+ *		Optional; This callback notifies the TCPC driver of the resolved
+ *		polarity.
  */
 struct tcpci_data {
 	struct regmap *regmap;
@@ -181,6 +187,10 @@ struct tcpci_data {
 	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);
+	void (*set_roles)(struct tcpci *tcpci, struct tcpci_data *data, bool attached,
+			  enum typec_role role, enum typec_data_role data_role);
+	void (*set_cc_polarity)(struct tcpci *tcpci, struct tcpci_data *data,
+				enum typec_cc_polarity polarity);
 };
 
 struct tcpci *tcpci_register_port(struct device *dev, struct tcpci_data *data);
-- 
2.31.0.rc2.261.g7f71774620-goog

[PATCH v2 2/2] usb: typec: tcpci_maxim: configure charging & data paths

[Badhri Jagan Sridharan]

The change exposes the data_role and the orientation as a extcon
interface for configuring the USB data controller.

Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
---
Changes since V1:
- Dropped changes related to get_/set_current_limit and pd_capable
  callback. Will send them in as separate patches.
---
 drivers/usb/typec/tcpm/tcpci_maxim.c | 56 ++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/drivers/usb/typec/tcpm/tcpci_maxim.c b/drivers/usb/typec/tcpm/tcpci_maxim.c
index 041a1c393594..1210445713ee 100644
--- a/drivers/usb/typec/tcpm/tcpci_maxim.c
+++ b/drivers/usb/typec/tcpm/tcpci_maxim.c
@@ -7,6 +7,8 @@
 
 #include <linux/interrupt.h>
 #include <linux/i2c.h>
+#include <linux/extcon.h>
+#include <linux/extcon-provider.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/regmap.h>
@@ -46,6 +48,8 @@ struct max_tcpci_chip {
 	struct device *dev;
 	struct i2c_client *client;
 	struct tcpm_port *port;
+	bool attached;
+	struct extcon_dev *extcon;
 };
 
 static const struct regmap_range max_tcpci_tcpci_range[] = {
@@ -439,6 +443,39 @@ static int tcpci_init(struct tcpci *tcpci, struct tcpci_data *data)
 	return -1;
 }
 
+static void max_tcpci_set_roles(struct tcpci *tcpci, struct tcpci_data *data, bool attached,
+				enum typec_role role, enum typec_data_role data_role)
+{
+	struct max_tcpci_chip *chip = tdata_to_max_tcpci(data);
+
+	chip->attached = attached;
+
+	if (!attached) {
+		extcon_set_state_sync(chip->extcon, EXTCON_USB_HOST, 0);
+		extcon_set_state_sync(chip->extcon, EXTCON_USB, 0);
+		return;
+	}
+
+	extcon_set_state_sync(chip->extcon, data_role == TYPEC_HOST ? EXTCON_USB_HOST : EXTCON_USB,
+			      1);
+}
+
+static void max_tcpci_set_cc_polarity(struct tcpci *tcpci, struct tcpci_data *data,
+				      enum typec_cc_polarity polarity)
+{
+	struct max_tcpci_chip *chip = tdata_to_max_tcpci(data);
+
+	extcon_set_property(chip->extcon, EXTCON_USB, EXTCON_PROP_USB_TYPEC_POLARITY,
+			    (union extcon_property_value)(int)polarity);
+	extcon_set_property(chip->extcon, EXTCON_USB_HOST, EXTCON_PROP_USB_TYPEC_POLARITY,
+			    (union extcon_property_value)(int)polarity);
+}
+
+static const unsigned int usbpd_extcon[] = {
+	EXTCON_USB,
+	EXTCON_USB_HOST,
+};
+
 static int max_tcpci_probe(struct i2c_client *client, const struct i2c_device_id *i2c_id)
 {
 	int ret;
@@ -472,6 +509,8 @@ static int max_tcpci_probe(struct i2c_client *client, const struct i2c_device_id
 	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;
+	chip->data.set_roles = max_tcpci_set_roles;
+	chip->data.set_cc_polarity = max_tcpci_set_cc_polarity;
 
 	max_tcpci_init_regs(chip);
 	chip->tcpci = tcpci_register_port(chip->dev, &chip->data);
@@ -484,6 +523,23 @@ static int max_tcpci_probe(struct i2c_client *client, const struct i2c_device_id
 	if (ret < 0)
 		goto unreg_port;
 
+	chip->extcon = devm_extcon_dev_allocate(&client->dev, usbpd_extcon);
+	if (IS_ERR(chip->extcon)) {
+		dev_err(&client->dev, "Error allocating extcon: %ld\n", PTR_ERR(chip->extcon));
+		ret = PTR_ERR(chip->extcon);
+		goto unreg_port;
+	}
+
+	ret = devm_extcon_dev_register(&client->dev, chip->extcon);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to register extcon device");
+		goto unreg_port;
+	}
+
+	extcon_set_property_capability(chip->extcon, EXTCON_USB, EXTCON_PROP_USB_TYPEC_POLARITY);
+	extcon_set_property_capability(chip->extcon, EXTCON_USB_HOST,
+				       EXTCON_PROP_USB_TYPEC_POLARITY);
+
 	device_init_wakeup(chip->dev, true);
 	return 0;
 
-- 
2.31.0.rc2.261.g7f71774620-goog

Re: [PATCH v2 2/2] usb: typec: tcpci_maxim: configure charging & data paths

[Guenter Roeck]

On 3/11/21 9:24 PM, Badhri Jagan Sridharan wrote:
> The change exposes the data_role and the orientation as a extcon
> interface for configuring the USB data controller.
> 
> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> ---
> Changes since V1:
> - Dropped changes related to get_/set_current_limit and pd_capable
>   callback. Will send them in as separate patches.
> ---
>  drivers/usb/typec/tcpm/tcpci_maxim.c | 56 ++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpci_maxim.c b/drivers/usb/typec/tcpm/tcpci_maxim.c
> index 041a1c393594..1210445713ee 100644
> --- a/drivers/usb/typec/tcpm/tcpci_maxim.c
> +++ b/drivers/usb/typec/tcpm/tcpci_maxim.c
> @@ -7,6 +7,8 @@
>  
>  #include <linux/interrupt.h>
>  #include <linux/i2c.h>
> +#include <linux/extcon.h>
> +#include <linux/extcon-provider.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/regmap.h>
> @@ -46,6 +48,8 @@ struct max_tcpci_chip {
>  	struct device *dev;
>  	struct i2c_client *client;
>  	struct tcpm_port *port;
> +	bool attached;
> +	struct extcon_dev *extcon;
>  };
>  
>  static const struct regmap_range max_tcpci_tcpci_range[] = {
> @@ -439,6 +443,39 @@ static int tcpci_init(struct tcpci *tcpci, struct tcpci_data *data)
>  	return -1;
>  }
>  
> +static void max_tcpci_set_roles(struct tcpci *tcpci, struct tcpci_data *data, bool attached,
> +				enum typec_role role, enum typec_data_role data_role)
> +{
> +	struct max_tcpci_chip *chip = tdata_to_max_tcpci(data);
> +
> +	chip->attached = attached;
> +
> +	if (!attached) {
> +		extcon_set_state_sync(chip->extcon, EXTCON_USB_HOST, 0);
> +		extcon_set_state_sync(chip->extcon, EXTCON_USB, 0);
> +		return;
> +	}
> +
> +	extcon_set_state_sync(chip->extcon, data_role == TYPEC_HOST ? EXTCON_USB_HOST : EXTCON_USB,
> +			      1);
> +}
> +
> +static void max_tcpci_set_cc_polarity(struct tcpci *tcpci, struct tcpci_data *data,
> +				      enum typec_cc_polarity polarity)
> +{
> +	struct max_tcpci_chip *chip = tdata_to_max_tcpci(data);
> +
> +	extcon_set_property(chip->extcon, EXTCON_USB, EXTCON_PROP_USB_TYPEC_POLARITY,
> +			    (union extcon_property_value)(int)polarity);
> +	extcon_set_property(chip->extcon, EXTCON_USB_HOST, EXTCON_PROP_USB_TYPEC_POLARITY,
> +			    (union extcon_property_value)(int)polarity);
> +}
> +
> +static const unsigned int usbpd_extcon[] = {
> +	EXTCON_USB,
> +	EXTCON_USB_HOST,
> +};
> +
>  static int max_tcpci_probe(struct i2c_client *client, const struct i2c_device_id *i2c_id)
>  {
>  	int ret;
> @@ -472,6 +509,8 @@ static int max_tcpci_probe(struct i2c_client *client, const struct i2c_device_id
>  	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;
> +	chip->data.set_roles = max_tcpci_set_roles;
> +	chip->data.set_cc_polarity = max_tcpci_set_cc_polarity;
>  
>  	max_tcpci_init_regs(chip);
>  	chip->tcpci = tcpci_register_port(chip->dev, &chip->data);
> @@ -484,6 +523,23 @@ static int max_tcpci_probe(struct i2c_client *client, const struct i2c_device_id
>  	if (ret < 0)
>  		goto unreg_port;
>  
> +	chip->extcon = devm_extcon_dev_allocate(&client->dev, usbpd_extcon);
> +	if (IS_ERR(chip->extcon)) {
> +		dev_err(&client->dev, "Error allocating extcon: %ld\n", PTR_ERR(chip->extcon));
> +		ret = PTR_ERR(chip->extcon);
> +		goto unreg_port;
> +	}
> +
> +	ret = devm_extcon_dev_register(&client->dev, chip->extcon);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to register extcon device");
> +		goto unreg_port;
> +	}

Effectively this mandates extcon support to be able to use this driver/chip.
Does that make sense ? If this is indeed mandatory, how did it work so far ?

Also, what makes this code chip specific ?

Thanks,
Guenter

> +
> +	extcon_set_property_capability(chip->extcon, EXTCON_USB, EXTCON_PROP_USB_TYPEC_POLARITY);
> +	extcon_set_property_capability(chip->extcon, EXTCON_USB_HOST,
> +				       EXTCON_PROP_USB_TYPEC_POLARITY);
> +
>  	device_init_wakeup(chip->dev, true);
>  	return 0;
>  
> 

Re: [PATCH v2 2/2] usb: typec: tcpci_maxim: configure charging & data paths

[Badhri Jagan Sridharan]

On Thu, Mar 11, 2021 at 10:39 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 3/11/21 9:24 PM, Badhri Jagan Sridharan wrote:
> > The change exposes the data_role and the orientation as a extcon
> > interface for configuring the USB data controller.
> >
> > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> > ---
> > Changes since V1:
> > - Dropped changes related to get_/set_current_limit and pd_capable
> >   callback. Will send them in as separate patches.
> > ---
> >  drivers/usb/typec/tcpm/tcpci_maxim.c | 56 ++++++++++++++++++++++++++++
> >  1 file changed, 56 insertions(+)
> >
> > diff --git a/drivers/usb/typec/tcpm/tcpci_maxim.c b/drivers/usb/typec/tcpm/tcpci_maxim.c
> > index 041a1c393594..1210445713ee 100644
> > --- a/drivers/usb/typec/tcpm/tcpci_maxim.c
> > +++ b/drivers/usb/typec/tcpm/tcpci_maxim.c
> > @@ -7,6 +7,8 @@
> >
> >  #include <linux/interrupt.h>
> >  #include <linux/i2c.h>
> > +#include <linux/extcon.h>
> > +#include <linux/extcon-provider.h>
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> >  #include <linux/regmap.h>
> > @@ -46,6 +48,8 @@ struct max_tcpci_chip {
> >       struct device *dev;
> >       struct i2c_client *client;
> >       struct tcpm_port *port;
> > +     bool attached;
> > +     struct extcon_dev *extcon;
> >  };
> >
> >  static const struct regmap_range max_tcpci_tcpci_range[] = {
> > @@ -439,6 +443,39 @@ static int tcpci_init(struct tcpci *tcpci, struct tcpci_data *data)
> >       return -1;
> >  }
> >
> > +static void max_tcpci_set_roles(struct tcpci *tcpci, struct tcpci_data *data, bool attached,
> > +                             enum typec_role role, enum typec_data_role data_role)
> > +{
> > +     struct max_tcpci_chip *chip = tdata_to_max_tcpci(data);
> > +
> > +     chip->attached = attached;
> > +
> > +     if (!attached) {
> > +             extcon_set_state_sync(chip->extcon, EXTCON_USB_HOST, 0);
> > +             extcon_set_state_sync(chip->extcon, EXTCON_USB, 0);
> > +             return;
> > +     }
> > +
> > +     extcon_set_state_sync(chip->extcon, data_role == TYPEC_HOST ? EXTCON_USB_HOST : EXTCON_USB,
> > +                           1);
> > +}
> > +
> > +static void max_tcpci_set_cc_polarity(struct tcpci *tcpci, struct tcpci_data *data,
> > +                                   enum typec_cc_polarity polarity)
> > +{
> > +     struct max_tcpci_chip *chip = tdata_to_max_tcpci(data);
> > +
> > +     extcon_set_property(chip->extcon, EXTCON_USB, EXTCON_PROP_USB_TYPEC_POLARITY,
> > +                         (union extcon_property_value)(int)polarity);
> > +     extcon_set_property(chip->extcon, EXTCON_USB_HOST, EXTCON_PROP_USB_TYPEC_POLARITY,
> > +                         (union extcon_property_value)(int)polarity);
> > +}
> > +
> > +static const unsigned int usbpd_extcon[] = {
> > +     EXTCON_USB,
> > +     EXTCON_USB_HOST,
> > +};
> > +
> >  static int max_tcpci_probe(struct i2c_client *client, const struct i2c_device_id *i2c_id)
> >  {
> >       int ret;
> > @@ -472,6 +509,8 @@ static int max_tcpci_probe(struct i2c_client *client, const struct i2c_device_id
> >       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;
> > +     chip->data.set_roles = max_tcpci_set_roles;
> > +     chip->data.set_cc_polarity = max_tcpci_set_cc_polarity;
> >
> >       max_tcpci_init_regs(chip);
> >       chip->tcpci = tcpci_register_port(chip->dev, &chip->data);
> > @@ -484,6 +523,23 @@ static int max_tcpci_probe(struct i2c_client *client, const struct i2c_device_id
> >       if (ret < 0)
> >               goto unreg_port;
> >
> > +     chip->extcon = devm_extcon_dev_allocate(&client->dev, usbpd_extcon);
> > +     if (IS_ERR(chip->extcon)) {
> > +             dev_err(&client->dev, "Error allocating extcon: %ld\n", PTR_ERR(chip->extcon));
> > +             ret = PTR_ERR(chip->extcon);
> > +             goto unreg_port;
> > +     }
> > +
> > +     ret = devm_extcon_dev_register(&client->dev, chip->extcon);
> > +     if (ret < 0) {
> > +             dev_err(&client->dev, "failed to register extcon device");
> > +             goto unreg_port;
> > +     }
>
> Effectively this mandates extcon support to be able to use this driver/chip.
> Does that make sense ? If this is indeed mandatory, how did it work so far ?
Hi Guenter,

We had this in our downstream branch but didnt get a chance to send it
to linux upstream.
I should wrap it in "if(IS_ENABLED(CONFIG_EXTCON))", the tcpc can work
without the
extcon.

>
> Also, what makes this code chip specific ?

Extcon here as is not chip code specific, but, the driver which
subscribes to the extcon interface is chip specific.
I hope it's ok to still send this.

Thanks,
Badhri


>
> Thanks,
> Guenter
>
> > +
> > +     extcon_set_property_capability(chip->extcon, EXTCON_USB, EXTCON_PROP_USB_TYPEC_POLARITY);
> > +     extcon_set_property_capability(chip->extcon, EXTCON_USB_HOST,
> > +                                    EXTCON_PROP_USB_TYPEC_POLARITY);
> > +
> >       device_init_wakeup(chip->dev, true);
> >       return 0;
> >
> >
>

Re: [PATCH v2 2/2] usb: typec: tcpci_maxim: configure charging & data paths

[Heikki Krogerus]

Thu, Mar 11, 2021 at 09:24:43PM -0800, Badhri Jagan Sridharan kirjoitti:
> The change exposes the data_role and the orientation as a extcon
> interface for configuring the USB data controller.
> 
> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> ---
> Changes since V1:
> - Dropped changes related to get_/set_current_limit and pd_capable
>   callback. Will send them in as separate patches.
> ---
>  drivers/usb/typec/tcpm/tcpci_maxim.c | 56 ++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpci_maxim.c b/drivers/usb/typec/tcpm/tcpci_maxim.c
> index 041a1c393594..1210445713ee 100644
> --- a/drivers/usb/typec/tcpm/tcpci_maxim.c
> +++ b/drivers/usb/typec/tcpm/tcpci_maxim.c
> @@ -7,6 +7,8 @@
>  
>  #include <linux/interrupt.h>
>  #include <linux/i2c.h>
> +#include <linux/extcon.h>
> +#include <linux/extcon-provider.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/regmap.h>
> @@ -46,6 +48,8 @@ struct max_tcpci_chip {
>  	struct device *dev;
>  	struct i2c_client *client;
>  	struct tcpm_port *port;
> +	bool attached;
> +	struct extcon_dev *extcon;
>  };
>  
>  static const struct regmap_range max_tcpci_tcpci_range[] = {
> @@ -439,6 +443,39 @@ static int tcpci_init(struct tcpci *tcpci, struct tcpci_data *data)
>  	return -1;
>  }
>  
> +static void max_tcpci_set_roles(struct tcpci *tcpci, struct tcpci_data *data, bool attached,
> +				enum typec_role role, enum typec_data_role data_role)
> +{
> +	struct max_tcpci_chip *chip = tdata_to_max_tcpci(data);
> +
> +	chip->attached = attached;
> +
> +	if (!attached) {
> +		extcon_set_state_sync(chip->extcon, EXTCON_USB_HOST, 0);
> +		extcon_set_state_sync(chip->extcon, EXTCON_USB, 0);
> +		return;
> +	}
> +
> +	extcon_set_state_sync(chip->extcon, data_role == TYPEC_HOST ? EXTCON_USB_HOST : EXTCON_USB,
> +			      1);
> +}
> +
> +static void max_tcpci_set_cc_polarity(struct tcpci *tcpci, struct tcpci_data *data,
> +				      enum typec_cc_polarity polarity)
> +{
> +	struct max_tcpci_chip *chip = tdata_to_max_tcpci(data);
> +
> +	extcon_set_property(chip->extcon, EXTCON_USB, EXTCON_PROP_USB_TYPEC_POLARITY,
> +			    (union extcon_property_value)(int)polarity);
> +	extcon_set_property(chip->extcon, EXTCON_USB_HOST, EXTCON_PROP_USB_TYPEC_POLARITY,
> +			    (union extcon_property_value)(int)polarity);
> +}
> +
> +static const unsigned int usbpd_extcon[] = {
> +	EXTCON_USB,
> +	EXTCON_USB_HOST,
> +};
> +
>  static int max_tcpci_probe(struct i2c_client *client, const struct i2c_device_id *i2c_id)
>  {
>  	int ret;
> @@ -472,6 +509,8 @@ static int max_tcpci_probe(struct i2c_client *client, const struct i2c_device_id
>  	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;
> +	chip->data.set_roles = max_tcpci_set_roles;
> +	chip->data.set_cc_polarity = max_tcpci_set_cc_polarity;
>  
>  	max_tcpci_init_regs(chip);
>  	chip->tcpci = tcpci_register_port(chip->dev, &chip->data);
> @@ -484,6 +523,23 @@ static int max_tcpci_probe(struct i2c_client *client, const struct i2c_device_id
>  	if (ret < 0)
>  		goto unreg_port;
>  
> +	chip->extcon = devm_extcon_dev_allocate(&client->dev, usbpd_extcon);
> +	if (IS_ERR(chip->extcon)) {
> +		dev_err(&client->dev, "Error allocating extcon: %ld\n", PTR_ERR(chip->extcon));
> +		ret = PTR_ERR(chip->extcon);
> +		goto unreg_port;
> +	}
> +
> +	ret = devm_extcon_dev_register(&client->dev, chip->extcon);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to register extcon device");
> +		goto unreg_port;
> +	}

Why do you need this? We have the dedicated USB role class because
extcon could not handle every type of system. Things are simple enough
when you have a single dual-role capable USB controller, but when you
start having more bits and pieces like muxes in between, the
consumer/supplier extcon roles get twisted.

So in case you did not know this, our goal was originally to use
extcon for handling the data role (and orientation too), but some of
drivers were refused by the extcon maintainers because of the above
reason.

Most USB controller drivers for dual-role capable USB controllers
already register a role switch, and tcpm.c always requests a handle to
one that it uses to inform the current data role, so this part should
not require any new code.


> +	extcon_set_property_capability(chip->extcon, EXTCON_USB, EXTCON_PROP_USB_TYPEC_POLARITY);
> +	extcon_set_property_capability(chip->extcon, EXTCON_USB_HOST,
> +				       EXTCON_PROP_USB_TYPEC_POLARITY);
> +
>  	device_init_wakeup(chip->dev, true);
>  	return 0;
>  
> -- 
> 2.31.0.rc2.261.g7f71774620-goog

thanks,

-- 
heikki

Re: [PATCH v2 1/2] usb: typec: tcpci: Add tcpc chip level callbacks

[Greg Kroah-Hartman]

On Thu, Mar 11, 2021 at 09:24:42PM -0800, Badhri Jagan Sridharan wrote:
> This change adds chip callbacks for the following operations:
> 1. Notifying port role
> 2. Notifying orientation

This should be 2 different patches, one per callback, right?

And where is the code using these callbacks?  We can't add any hooks
without in-tree users, as you know.

thanks,

greg k-h

Re: [PATCH v2 1/2] usb: typec: tcpci: Add tcpc chip level callbacks

[Greg Kroah-Hartman]

On Fri, Mar 12, 2021 at 07:33:45AM +0100, Greg Kroah-Hartman wrote:
> On Thu, Mar 11, 2021 at 09:24:42PM -0800, Badhri Jagan Sridharan wrote:
> > This change adds chip callbacks for the following operations:
> > 1. Notifying port role
> > 2. Notifying orientation
> 
> This should be 2 different patches, one per callback, right?
> 
> And where is the code using these callbacks?  We can't add any hooks
> without in-tree users, as you know.

Ah, your second patch added that, sorry, missed it.

This should be a 4 patch series, remember, only do one thing per patch.

thanks,

greg k-h

Re: [PATCH v2 1/2] usb: typec: tcpci: Add tcpc chip level callbacks

[Badhri Jagan Sridharan]

On Thu, Mar 11, 2021 at 10:34 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Fri, Mar 12, 2021 at 07:33:45AM +0100, Greg Kroah-Hartman wrote:
> > On Thu, Mar 11, 2021 at 09:24:42PM -0800, Badhri Jagan Sridharan wrote:
> > > This change adds chip callbacks for the following operations:
> > > 1. Notifying port role
> > > 2. Notifying orientation
> >
> > This should be 2 different patches, one per callback, right?
> >
> > And where is the code using these callbacks?  We can't add any hooks
> > without in-tree users, as you know.
>
> Ah, your second patch added that, sorry, missed it.
>
> This should be a 4 patch series, remember, only do one thing per patch.

Sure. Will do in the next version.

Thanks,
Badhri

>
> thanks,
>
> greg k-h