Re: [PATCH v2 2/2] usb: typec: tcpci_maxim: configure charging & data paths
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
> ---
> 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
> #include
> +#include
> +#include
> #include
> #include
> #include
> @@ -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, >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(>dev, usbpd_extcon);
> + if (IS_ERR(chip->extcon)) {
> + dev_err(>dev, "Error allocating extcon: %ld\n",
> PTR_ERR(chip->extcon));
> + ret = PTR_ERR(chip->extcon);
> + goto unreg_port;
> + }
> +
> + ret = devm_extcon_dev_register(>dev, chip->extcon);
> + if (ret < 0) {
> + dev_err(>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 2/2] usb: typec: tcpci_maxim: configure charging & data paths
On Thu, Mar 11, 2021 at 10:39 PM Guenter Roeck 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
> > ---
> > 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
> > #include
> > +#include
> > +#include
> > #include
> > #include
> > #include
> > @@ -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, >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(>dev, usbpd_extcon);
> > + if (IS_ERR(chip->extcon)) {
> > + dev_err(>dev, "Error allocating extcon: %ld\n",
> > PTR_ERR(chip->extcon));
> > + ret = PTR_ERR(chip->extcon);
> > + goto unreg_port;
> > + }
> > +
> > + ret = devm_extcon_dev_register(>dev, chip->extcon);
> > + if (ret < 0) {
> > + dev_err(>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);
> >
Re: [PATCH v2 2/2] usb: typec: tcpci_maxim: configure charging & data paths
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
> ---
> 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
> #include
> +#include
> +#include
> #include
> #include
> #include
> @@ -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, >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(>dev, usbpd_extcon);
> + if (IS_ERR(chip->extcon)) {
> + dev_err(>dev, "Error allocating extcon: %ld\n",
> PTR_ERR(chip->extcon));
> + ret = PTR_ERR(chip->extcon);
> + goto unreg_port;
> + }
> +
> + ret = devm_extcon_dev_register(>dev, chip->extcon);
> + if (ret < 0) {
> + dev_err(>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;
>
>
[PATCH v2 2/2] usb: typec: tcpci_maxim: configure charging & data paths
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
---
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
#include
+#include
+#include
#include
#include
#include
@@ -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, >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(>dev, usbpd_extcon);
+ if (IS_ERR(chip->extcon)) {
+ dev_err(>dev, "Error allocating extcon: %ld\n",
PTR_ERR(chip->extcon));
+ ret = PTR_ERR(chip->extcon);
+ goto unreg_port;
+ }
+
+ ret = devm_extcon_dev_register(>dev, chip->extcon);
+ if (ret < 0) {
+ dev_err(>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
