| Message ID | 20240724201116.2094059-4-jthies@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | usb: typec: ucsi: Expand power supply support | expand |
On Wed, Jul 24, 2024 at 08:11:15PM GMT, Jameson Thies wrote: > Add POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX as a property to the UCSI > power supply driver. When set to a positive value, enable the > connector's sink path. When set to a negative value, disable the > connector's sink path. [Also added power supply maintainers to cc] This really looks like a hack. As a user I'd expect that if I set the charging limit, it really gets applied. In other words, I'd expect to be able to limit 60W PSY to provide just 15W by limiting the current. This is not the case with this patch. Please provide rational for this change, i.e. why using the existing typec property isn't enough for your use case. > > Signed-off-by: Jameson Thies <jthies@google.com> > --- > Changes in V2: > - Added SET_SINK_PATH call when handling charge_control_limit_max > update. Updated commit message. > > drivers/usb/typec/ucsi/psy.c | 52 +++++++++++++++++++++++++++++++++++ > drivers/usb/typec/ucsi/ucsi.c | 15 ++++++++++ > drivers/usb/typec/ucsi/ucsi.h | 5 ++++ > 3 files changed, 72 insertions(+) > > diff --git a/drivers/usb/typec/ucsi/psy.c b/drivers/usb/typec/ucsi/psy.c > index d708f9eb1654..28265feb9d13 100644 > --- a/drivers/usb/typec/ucsi/psy.c > +++ b/drivers/usb/typec/ucsi/psy.c > @@ -30,6 +30,7 @@ static enum power_supply_property ucsi_psy_props[] = { > POWER_SUPPLY_PROP_CURRENT_NOW, > POWER_SUPPLY_PROP_SCOPE, > POWER_SUPPLY_PROP_STATUS, > + POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX, > }; > > static int ucsi_psy_get_scope(struct ucsi_connector *con, > @@ -275,11 +276,60 @@ static int ucsi_psy_get_prop(struct power_supply *psy, > return ucsi_psy_get_scope(con, val); > case POWER_SUPPLY_PROP_STATUS: > return ucsi_psy_get_status(con, val); > + case POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX: > + val->intval = 0; > + return 0; > + default: > + return -EINVAL; > + } > +} > + > +static int ucsi_psy_set_charge_control_limit_max(struct ucsi_connector *con, > + const union power_supply_propval *val) > +{ > + /* > + * Writing a negative value to the charge control limit max implies the > + * port should not accept charge. Disable the sink path for a negative > + * charge control limit, and enable the sink path for a positive charge > + * control limit. If the requested charge port is a source, update the > + * power role. > + */ > + int ret; > + bool sink_path = false; > + > + if (val->intval >= 0) { > + sink_path = true; > + if (!con->typec_cap.ops || !con->typec_cap.ops->pr_set) > + return -EINVAL; > + > + ret = con->typec_cap.ops->pr_set(con->port, TYPEC_SINK); Should there be a call to pr_set(TYPEC_SOURCE) if the value is negative? > + if (ret < 0) > + return ret; > + } > + > + return ucsi_set_sink_path(con, sink_path); You are calling SET_SINK_PATH uncoditionally, while this command was not defined for UCSI 1.x. Also it is an error to call it when device is in power source mode or if there is no partner connected. Last, but not least, the property value survives between PSY reconnects (which is expected), however after reconnecting the partner device, the value won't be reprogrammed by the UCSI driver, resulting in the inconsistency between the sysfs and actual system state. > +} > + > +static int ucsi_psy_set_prop(struct power_supply *psy, > + enum power_supply_property psp, > + const union power_supply_propval *val) > +{ > + struct ucsi_connector *con = power_supply_get_drvdata(psy); > + > + switch (psp) { > + case POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX: > + return ucsi_psy_set_charge_control_limit_max(con, val); > default: > return -EINVAL; > } > } > > +static int ucsi_psy_prop_is_writeable(struct power_supply *psy, > + enum power_supply_property psp) > +{ > + return psp == POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX; > +} > + > static enum power_supply_usb_type ucsi_psy_usb_types[] = { > POWER_SUPPLY_USB_TYPE_C, > POWER_SUPPLY_USB_TYPE_PD, > @@ -308,6 +358,8 @@ int ucsi_register_port_psy(struct ucsi_connector *con) > con->psy_desc.properties = ucsi_psy_props; > con->psy_desc.num_properties = ARRAY_SIZE(ucsi_psy_props); > con->psy_desc.get_property = ucsi_psy_get_prop; > + con->psy_desc.set_property = ucsi_psy_set_prop; > + con->psy_desc.property_is_writeable = ucsi_psy_prop_is_writeable; > > con->psy = power_supply_register(dev, &con->psy_desc, &psy_cfg); > > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c > index dcd3765cc1f5..02663fdebdd9 100644 > --- a/drivers/usb/typec/ucsi/ucsi.c > +++ b/drivers/usb/typec/ucsi/ucsi.c > @@ -1545,6 +1545,21 @@ static const struct typec_operations ucsi_ops = { > .pr_set = ucsi_pr_swap > }; > > +int ucsi_set_sink_path(struct ucsi_connector *con, bool sink_path) > +{ > + struct ucsi *ucsi = con->ucsi; > + u64 command; > + int ret; > + > + command = UCSI_SET_SINK_PATH | UCSI_CONNECTOR_NUMBER(con->num); > + command |= UCSI_SET_SINK_PATH_SINK_PATH(sink_path); > + ret = ucsi_send_command(ucsi, command, NULL, 0); > + if (ret < 0) > + dev_err(con->ucsi->dev, "SET_SINK_PATH failed (%d)\n", ret); > + > + return ret; > +} > + > /* Caller must call fwnode_handle_put() after use */ > static struct fwnode_handle *ucsi_find_fwnode(struct ucsi_connector *con) > { > diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h > index 57129f3c0814..6a958eac5703 100644 > --- a/drivers/usb/typec/ucsi/ucsi.h > +++ b/drivers/usb/typec/ucsi/ucsi.h > @@ -114,6 +114,7 @@ void ucsi_connector_change(struct ucsi *ucsi, u8 num); > #define UCSI_GET_CONNECTOR_STATUS 0x12 > #define UCSI_GET_ERROR_STATUS 0x13 > #define UCSI_GET_PD_MESSAGE 0x15 > +#define UCSI_SET_SINK_PATH 0x1c > > #define UCSI_CONNECTOR_NUMBER(_num_) ((u64)(_num_) << 16) > #define UCSI_COMMAND(_cmd_) ((_cmd_) & 0xff) > @@ -187,6 +188,9 @@ void ucsi_connector_change(struct ucsi *ucsi, u8 num); > #define UCSI_GET_PD_MESSAGE_TYPE_IDENTITY 4 > #define UCSI_GET_PD_MESSAGE_TYPE_REVISION 5 > > +/* SET_SINK_PATH command bits */ > +#define UCSI_SET_SINK_PATH_SINK_PATH(_r_) (((_r_) ? 1 : 0) << 23) > + > /* -------------------------------------------------------------------------- */ > > /* Error information returned by PPM in response to GET_ERROR_STATUS command. */ > @@ -492,6 +496,7 @@ int ucsi_send_command(struct ucsi *ucsi, u64 command, > > void ucsi_altmode_update_active(struct ucsi_connector *con); > int ucsi_resume(struct ucsi *ucsi); > +int ucsi_set_sink_path(struct ucsi_connector *con, bool sink_path); > > void ucsi_notify_common(struct ucsi *ucsi, u32 cci); > int ucsi_sync_control_common(struct ucsi *ucsi, u64 command); > -- > 2.45.2.1089.g2a221341d9-goog >
Hi Dmitry, thank you for your feedback on this patch. The intention here is to allow power source selection through the UCSI driver. The existing typec operation wouldn't work here because setting the power roles alone won't set the charge source. That's also why there is no pr_set(TYPEC_SOURCE) call for a negative value. It should disable charging from that port, but it doesn't need to change the power role. But I take your point that writing positive/negative values to charge_control_limit_max is not an intuitive way to enable this functionality. Thanks for catching this issues with UCSI version and inconsistencies between sysfs and system state. I need to revisit the design here. I'll remove this patch from the V3 series and take another look at how we could implement power source selection. Thanks, Jameson
diff --git a/drivers/usb/typec/ucsi/psy.c b/drivers/usb/typec/ucsi/psy.c index d708f9eb1654..28265feb9d13 100644 --- a/drivers/usb/typec/ucsi/psy.c +++ b/drivers/usb/typec/ucsi/psy.c @@ -30,6 +30,7 @@ static enum power_supply_property ucsi_psy_props[] = { POWER_SUPPLY_PROP_CURRENT_NOW, POWER_SUPPLY_PROP_SCOPE, POWER_SUPPLY_PROP_STATUS, + POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX, }; static int ucsi_psy_get_scope(struct ucsi_connector *con, @@ -275,11 +276,60 @@ static int ucsi_psy_get_prop(struct power_supply *psy, return ucsi_psy_get_scope(con, val); case POWER_SUPPLY_PROP_STATUS: return ucsi_psy_get_status(con, val); + case POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX: + val->intval = 0; + return 0; + default: + return -EINVAL; + } +} + +static int ucsi_psy_set_charge_control_limit_max(struct ucsi_connector *con, + const union power_supply_propval *val) +{ + /* + * Writing a negative value to the charge control limit max implies the + * port should not accept charge. Disable the sink path for a negative + * charge control limit, and enable the sink path for a positive charge + * control limit. If the requested charge port is a source, update the + * power role. + */ + int ret; + bool sink_path = false; + + if (val->intval >= 0) { + sink_path = true; + if (!con->typec_cap.ops || !con->typec_cap.ops->pr_set) + return -EINVAL; + + ret = con->typec_cap.ops->pr_set(con->port, TYPEC_SINK); + if (ret < 0) + return ret; + } + + return ucsi_set_sink_path(con, sink_path); +} + +static int ucsi_psy_set_prop(struct power_supply *psy, + enum power_supply_property psp, + const union power_supply_propval *val) +{ + struct ucsi_connector *con = power_supply_get_drvdata(psy); + + switch (psp) { + case POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX: + return ucsi_psy_set_charge_control_limit_max(con, val); default: return -EINVAL; } } +static int ucsi_psy_prop_is_writeable(struct power_supply *psy, + enum power_supply_property psp) +{ + return psp == POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX; +} + static enum power_supply_usb_type ucsi_psy_usb_types[] = { POWER_SUPPLY_USB_TYPE_C, POWER_SUPPLY_USB_TYPE_PD, @@ -308,6 +358,8 @@ int ucsi_register_port_psy(struct ucsi_connector *con) con->psy_desc.properties = ucsi_psy_props; con->psy_desc.num_properties = ARRAY_SIZE(ucsi_psy_props); con->psy_desc.get_property = ucsi_psy_get_prop; + con->psy_desc.set_property = ucsi_psy_set_prop; + con->psy_desc.property_is_writeable = ucsi_psy_prop_is_writeable; con->psy = power_supply_register(dev, &con->psy_desc, &psy_cfg); diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c index dcd3765cc1f5..02663fdebdd9 100644 --- a/drivers/usb/typec/ucsi/ucsi.c +++ b/drivers/usb/typec/ucsi/ucsi.c @@ -1545,6 +1545,21 @@ static const struct typec_operations ucsi_ops = { .pr_set = ucsi_pr_swap }; +int ucsi_set_sink_path(struct ucsi_connector *con, bool sink_path) +{ + struct ucsi *ucsi = con->ucsi; + u64 command; + int ret; + + command = UCSI_SET_SINK_PATH | UCSI_CONNECTOR_NUMBER(con->num); + command |= UCSI_SET_SINK_PATH_SINK_PATH(sink_path); + ret = ucsi_send_command(ucsi, command, NULL, 0); + if (ret < 0) + dev_err(con->ucsi->dev, "SET_SINK_PATH failed (%d)\n", ret); + + return ret; +} + /* Caller must call fwnode_handle_put() after use */ static struct fwnode_handle *ucsi_find_fwnode(struct ucsi_connector *con) { diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h index 57129f3c0814..6a958eac5703 100644 --- a/drivers/usb/typec/ucsi/ucsi.h +++ b/drivers/usb/typec/ucsi/ucsi.h @@ -114,6 +114,7 @@ void ucsi_connector_change(struct ucsi *ucsi, u8 num); #define UCSI_GET_CONNECTOR_STATUS 0x12 #define UCSI_GET_ERROR_STATUS 0x13 #define UCSI_GET_PD_MESSAGE 0x15 +#define UCSI_SET_SINK_PATH 0x1c #define UCSI_CONNECTOR_NUMBER(_num_) ((u64)(_num_) << 16) #define UCSI_COMMAND(_cmd_) ((_cmd_) & 0xff) @@ -187,6 +188,9 @@ void ucsi_connector_change(struct ucsi *ucsi, u8 num); #define UCSI_GET_PD_MESSAGE_TYPE_IDENTITY 4 #define UCSI_GET_PD_MESSAGE_TYPE_REVISION 5 +/* SET_SINK_PATH command bits */ +#define UCSI_SET_SINK_PATH_SINK_PATH(_r_) (((_r_) ? 1 : 0) << 23) + /* -------------------------------------------------------------------------- */ /* Error information returned by PPM in response to GET_ERROR_STATUS command. */ @@ -492,6 +496,7 @@ int ucsi_send_command(struct ucsi *ucsi, u64 command, void ucsi_altmode_update_active(struct ucsi_connector *con); int ucsi_resume(struct ucsi *ucsi); +int ucsi_set_sink_path(struct ucsi_connector *con, bool sink_path); void ucsi_notify_common(struct ucsi *ucsi, u32 cci); int ucsi_sync_control_common(struct ucsi *ucsi, u64 command);
Add POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX as a property to the UCSI power supply driver. When set to a positive value, enable the connector's sink path. When set to a negative value, disable the connector's sink path. Signed-off-by: Jameson Thies <jthies@google.com> --- Changes in V2: - Added SET_SINK_PATH call when handling charge_control_limit_max update. Updated commit message. drivers/usb/typec/ucsi/psy.c | 52 +++++++++++++++++++++++++++++++++++ drivers/usb/typec/ucsi/ucsi.c | 15 ++++++++++ drivers/usb/typec/ucsi/ucsi.h | 5 ++++ 3 files changed, 72 insertions(+)