diff mbox series

[v2,3/4] usb: typec: ucsi: Set sink path based on UCSI charge control

Message ID 20240724201116.2094059-4-jthies@google.com (mailing list archive)
State New
Headers show
Series usb: typec: ucsi: Expand power supply support | expand

Commit Message

Jameson Thies July 24, 2024, 8:11 p.m. UTC
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(+)

Comments

Dmitry Baryshkov July 25, 2024, 4:07 a.m. UTC | #1
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
>
Jameson Thies July 25, 2024, 7:11 p.m. UTC | #2
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 mbox series

Patch

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);