| Message ID | 20210312052443.3797674-2-badhri@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] usb: typec: tcpci: Add tcpc chip level callbacks | expand |
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; > >
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; > > > > >
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,
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;
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(+)