[PATCH v2 0/4] usb: typec: ucsi: Expand power supply support

[PATCH v2 0/4] usb: typec: ucsi: Expand power supply support

[Jameson Thies]

Hi Heikki,

This series makes the following updates to the UCSI power supply
driver.

1. Adds support for the power supply status property.
2. Updates the driver to distinguish between PD and PD DRP power supply
types.
3. Adds the charge control limit max property which can be used to
request a PR swap from sysfs.
4. Fixes a simple SET_PRD typo in the ucsi.h header.

I've checked that the series builds on top of the usb-next branch and
manually tested functionality on top of a 6.10-rc5 ChromeOS kernel. Let
me know if you have any questions.

Thanks,
Jameson

Changes in V2
- Uses DRP bit in source PDOs for setting USB DRP power supply type.
- Adds SET_SINK_PATH call when handling an update to
charge_control_limit_max.

Jameson Thies (4):
  usb: typec: ucsi: Add status to UCSI power supply driver
  usb: typec: ucsi: Add USB PD DRP to USB type
  usb: typec: ucsi: Set sink path based on UCSI charge control
  usb: typec: ucsi: Fix SET_PDR typo in UCSI header file

 drivers/usb/typec/ucsi/psy.c  | 81 ++++++++++++++++++++++++++++++++++-
 drivers/usb/typec/ucsi/ucsi.c | 15 +++++++
 drivers/usb/typec/ucsi/ucsi.h |  7 ++-
 3 files changed, 101 insertions(+), 2 deletions(-)


base-commit: 933069701c1b507825b514317d4edd5d3fd9d417
-- 
2.45.2.1089.g2a221341d9-goog

[PATCH v2 1/4] usb: typec: ucsi: Add status to UCSI power supply driver

[Jameson Thies]

Add status to UCSI power supply driver properties based on the port's
connection and power direction states.

Signed-off-by: Jameson Thies <jthies@google.com>
---
Changes in V2:
- None.

 drivers/usb/typec/ucsi/psy.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/usb/typec/ucsi/psy.c b/drivers/usb/typec/ucsi/psy.c
index e623d80e177c..d0b52cee41d2 100644
--- a/drivers/usb/typec/ucsi/psy.c
+++ b/drivers/usb/typec/ucsi/psy.c
@@ -29,6 +29,7 @@ static enum power_supply_property ucsi_psy_props[] = {
 	POWER_SUPPLY_PROP_CURRENT_MAX,
 	POWER_SUPPLY_PROP_CURRENT_NOW,
 	POWER_SUPPLY_PROP_SCOPE,
+	POWER_SUPPLY_PROP_STATUS,
 };
 
 static int ucsi_psy_get_scope(struct ucsi_connector *con,
@@ -51,6 +52,20 @@ static int ucsi_psy_get_scope(struct ucsi_connector *con,
 	return 0;
 }
 
+static int ucsi_psy_get_status(struct ucsi_connector *con,
+			       union power_supply_propval *val)
+{
+	val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
+	if (con->status.flags & UCSI_CONSTAT_CONNECTED) {
+		if ((con->status.flags & UCSI_CONSTAT_PWR_DIR) == TYPEC_SINK)
+			val->intval = POWER_SUPPLY_STATUS_CHARGING;
+		else
+			val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
+	}
+
+	return 0;
+}
+
 static int ucsi_psy_get_online(struct ucsi_connector *con,
 			       union power_supply_propval *val)
 {
@@ -249,6 +264,8 @@ static int ucsi_psy_get_prop(struct power_supply *psy,
 		return ucsi_psy_get_current_now(con, val);
 	case POWER_SUPPLY_PROP_SCOPE:
 		return ucsi_psy_get_scope(con, val);
+	case POWER_SUPPLY_PROP_STATUS:
+		return ucsi_psy_get_status(con, val);
 	default:
 		return -EINVAL;
 	}
-- 
2.45.2.1089.g2a221341d9-goog

Re: [PATCH v2 1/4] usb: typec: ucsi: Add status to UCSI power supply driver

[Dmitry Baryshkov]

On Wed, Jul 24, 2024 at 08:11:13PM GMT, Jameson Thies wrote:
> Add status to UCSI power supply driver properties based on the port's
> connection and power direction states.
> 
> Signed-off-by: Jameson Thies <jthies@google.com>

Please CC Power Supply maintainers for this patchset (added to cc).

At least per the Documentation/ABI/testing/sysfs-class-power, the status
property applies to batteries, while UCSI psy device is a charger. This
is logical, as there might be multiple reasons why the battery is not
being charging even when the supply is online.

> ---
> Changes in V2:
> - None.
> 
>  drivers/usb/typec/ucsi/psy.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/usb/typec/ucsi/psy.c b/drivers/usb/typec/ucsi/psy.c
> index e623d80e177c..d0b52cee41d2 100644
> --- a/drivers/usb/typec/ucsi/psy.c
> +++ b/drivers/usb/typec/ucsi/psy.c
> @@ -29,6 +29,7 @@ static enum power_supply_property ucsi_psy_props[] = {
>  	POWER_SUPPLY_PROP_CURRENT_MAX,
>  	POWER_SUPPLY_PROP_CURRENT_NOW,
>  	POWER_SUPPLY_PROP_SCOPE,
> +	POWER_SUPPLY_PROP_STATUS,
>  };
>  
>  static int ucsi_psy_get_scope(struct ucsi_connector *con,
> @@ -51,6 +52,20 @@ static int ucsi_psy_get_scope(struct ucsi_connector *con,
>  	return 0;
>  }
>  
> +static int ucsi_psy_get_status(struct ucsi_connector *con,
> +			       union power_supply_propval *val)
> +{
> +	val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
> +	if (con->status.flags & UCSI_CONSTAT_CONNECTED) {
> +		if ((con->status.flags & UCSI_CONSTAT_PWR_DIR) == TYPEC_SINK)
> +			val->intval = POWER_SUPPLY_STATUS_CHARGING;
> +		else
> +			val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
> +	}
> +
> +	return 0;
> +}
> +
>  static int ucsi_psy_get_online(struct ucsi_connector *con,
>  			       union power_supply_propval *val)
>  {
> @@ -249,6 +264,8 @@ static int ucsi_psy_get_prop(struct power_supply *psy,
>  		return ucsi_psy_get_current_now(con, val);
>  	case POWER_SUPPLY_PROP_SCOPE:
>  		return ucsi_psy_get_scope(con, val);
> +	case POWER_SUPPLY_PROP_STATUS:
> +		return ucsi_psy_get_status(con, val);
>  	default:
>  		return -EINVAL;
>  	}
> -- 
> 2.45.2.1089.g2a221341d9-goog
> 

-- 
With best wishes
Dmitry

Re: [PATCH v2 1/4] usb: typec: ucsi: Add status to UCSI power supply driver

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 3609 bytes --]

Hi,

On Thu, Jul 25, 2024 at 06:31:00AM GMT, Dmitry Baryshkov wrote:
> On Wed, Jul 24, 2024 at 08:11:13PM GMT, Jameson Thies wrote:
> > Add status to UCSI power supply driver properties based on the port's
> > connection and power direction states.
> > 
> > Signed-off-by: Jameson Thies <jthies@google.com>
> 
> Please CC Power Supply maintainers for this patchset (added to cc).

Thanks. I guess I should add something like this to MAINTAINERS
considering the power-supply bits are in its own file for UCSI:

UCSI POWER-SUPPLY API
R:	Sebastian Reichel <sre@kernel.org>
L:	linux-pm@vger.kernel.org
F:	drivers/usb/typec/ucsi/psy.c

> At least per the Documentation/ABI/testing/sysfs-class-power, the status
> property applies to batteries, while UCSI psy device is a charger. This
> is logical, as there might be multiple reasons why the battery is not
> being charging even when the supply is online.

Correct. These status bits are not meant for chargers. E.g.
POWER_SUPPLY_STATUS_NOT_CHARGING has a very specific meaning, that a
battery is neither charged nor discharged. Since the device is
running that can only happen when a charger is connected, but not
charging (or the device has two batteries).

For AC the power-supply API has been designed only taking the SINK
role into account. At some point USB was added and some time later
people added reverse mode to their USB chargers for OTG mode. So
far this has been handled by registering a regulator and using that
to switch the mode. This made sense for OTG, but USB-C PD makes
things even more complex.

I am open to ideas how to properly handle the source role in the
power-supply API, but let's not overload the status property for
it. Please make sure to CC me on any follow-up series.

-- Sebastian

> 
> > ---
> > Changes in V2:
> > - None.
> > 
> >  drivers/usb/typec/ucsi/psy.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/drivers/usb/typec/ucsi/psy.c b/drivers/usb/typec/ucsi/psy.c
> > index e623d80e177c..d0b52cee41d2 100644
> > --- a/drivers/usb/typec/ucsi/psy.c
> > +++ b/drivers/usb/typec/ucsi/psy.c
> > @@ -29,6 +29,7 @@ static enum power_supply_property ucsi_psy_props[] = {
> >  	POWER_SUPPLY_PROP_CURRENT_MAX,
> >  	POWER_SUPPLY_PROP_CURRENT_NOW,
> >  	POWER_SUPPLY_PROP_SCOPE,
> > +	POWER_SUPPLY_PROP_STATUS,
> >  };
> >  
> >  static int ucsi_psy_get_scope(struct ucsi_connector *con,
> > @@ -51,6 +52,20 @@ static int ucsi_psy_get_scope(struct ucsi_connector *con,
> >  	return 0;
> >  }
> >  
> > +static int ucsi_psy_get_status(struct ucsi_connector *con,
> > +			       union power_supply_propval *val)
> > +{
> > +	val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
> > +	if (con->status.flags & UCSI_CONSTAT_CONNECTED) {
> > +		if ((con->status.flags & UCSI_CONSTAT_PWR_DIR) == TYPEC_SINK)
> > +			val->intval = POWER_SUPPLY_STATUS_CHARGING;
> > +		else
> > +			val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int ucsi_psy_get_online(struct ucsi_connector *con,
> >  			       union power_supply_propval *val)
> >  {
> > @@ -249,6 +264,8 @@ static int ucsi_psy_get_prop(struct power_supply *psy,
> >  		return ucsi_psy_get_current_now(con, val);
> >  	case POWER_SUPPLY_PROP_SCOPE:
> >  		return ucsi_psy_get_scope(con, val);
> > +	case POWER_SUPPLY_PROP_STATUS:
> > +		return ucsi_psy_get_status(con, val);
> >  	default:
> >  		return -EINVAL;
> >  	}
> > -- 
> > 2.45.2.1089.g2a221341d9-goog
> > 
> 
> -- 
> With best wishes
> Dmitry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/4] usb: typec: ucsi: Add status to UCSI power supply driver

[Dmitry Baryshkov]

On Fri, Jul 26, 2024 at 11:30:37PM GMT, Sebastian Reichel wrote:
> Hi,
> 
> On Thu, Jul 25, 2024 at 06:31:00AM GMT, Dmitry Baryshkov wrote:
> > On Wed, Jul 24, 2024 at 08:11:13PM GMT, Jameson Thies wrote:
> > > Add status to UCSI power supply driver properties based on the port's
> > > connection and power direction states.
> > > 
> > > Signed-off-by: Jameson Thies <jthies@google.com>
> > 
> > Please CC Power Supply maintainers for this patchset (added to cc).
> 
> Thanks. I guess I should add something like this to MAINTAINERS
> considering the power-supply bits are in its own file for UCSI:
> 
> UCSI POWER-SUPPLY API
> R:	Sebastian Reichel <sre@kernel.org>
> L:	linux-pm@vger.kernel.org
> F:	drivers/usb/typec/ucsi/psy.c

Or maybe extract a separate USB PD PSY driver, which can get used by
other Type-C port managers (I didn't evalue if it makes sense, i.e. if
the TCPM drivers can provide data, I just assume they can).

-- 
With best wishes
Dmitry

[PATCH v2 2/4] usb: typec: ucsi: Add USB PD DRP to USB type

[Jameson Thies]

Add POWER_SUPPLY_USB_TYPE_PD_DRP as a USB type in the UCSI power supply
driver. The DRP type is set when the partner sets the DRP bit in one of
the source PDOs.

Signed-off-by: Jameson Thies <jthies@google.com>
---
Changes in V2:
- Using DRP bit of source PDOs to determine USB type. Updated commit
message.

 drivers/usb/typec/ucsi/psy.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/typec/ucsi/psy.c b/drivers/usb/typec/ucsi/psy.c
index d0b52cee41d2..d708f9eb1654 100644
--- a/drivers/usb/typec/ucsi/psy.c
+++ b/drivers/usb/typec/ucsi/psy.c
@@ -204,8 +204,17 @@ static int ucsi_psy_get_usb_type(struct ucsi_connector *con,
 
 	val->intval = POWER_SUPPLY_USB_TYPE_C;
 	if (flags & UCSI_CONSTAT_CONNECTED &&
-	    UCSI_CONSTAT_PWR_OPMODE(flags) == UCSI_CONSTAT_PWR_OPMODE_PD)
+	    UCSI_CONSTAT_PWR_OPMODE(flags) == UCSI_CONSTAT_PWR_OPMODE_PD) {
+		for (int i = 0; i < con->num_pdos; i++) {
+			if (pdo_type(con->src_pdos[i]) == PDO_TYPE_FIXED &&
+			    con->src_pdos[i] & PDO_FIXED_DUAL_ROLE) {
+				val->intval = POWER_SUPPLY_USB_TYPE_PD_DRP;
+				return 0;
+			}
+		}
+
 		val->intval = POWER_SUPPLY_USB_TYPE_PD;
+	}
 
 	return 0;
 }
@@ -275,6 +284,7 @@ static enum power_supply_usb_type ucsi_psy_usb_types[] = {
 	POWER_SUPPLY_USB_TYPE_C,
 	POWER_SUPPLY_USB_TYPE_PD,
 	POWER_SUPPLY_USB_TYPE_PD_PPS,
+	POWER_SUPPLY_USB_TYPE_PD_DRP,
 };
 
 int ucsi_register_port_psy(struct ucsi_connector *con)
-- 
2.45.2.1089.g2a221341d9-goog

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

[Jameson Thies]

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(+)

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);
-- 
2.45.2.1089.g2a221341d9-goog

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

[Dmitry Baryshkov]

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
> 

-- 
With best wishes
Dmitry

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

[Jameson Thies]

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

[PATCH v2 4/4] usb: typec: ucsi: Fix SET_PDR typo in UCSI header file

[Jameson Thies]

Fix SET_PDR typo in UCSI header file.

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
Signed-off-by: Jameson Thies <jthies@google.com>
---
Changes in V2:
- None.

 drivers/usb/typec/ucsi/ucsi.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index 6a958eac5703..a0e91335da80 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -153,7 +153,7 @@ void ucsi_connector_change(struct ucsi *ucsi, u8 num);
 #define UCSI_SET_UOR_ROLE(_r_)		(((_r_) == TYPEC_HOST ? 1 : 2) << 23)
 #define UCSI_SET_UOR_ACCEPT_ROLE_SWAPS		BIT(25)
 
-/* SET_PDF command bits */
+/* SET_PDR command bits */
 #define UCSI_SET_PDR_ROLE(_r_)		(((_r_) == TYPEC_SOURCE ? 1 : 2) << 23)
 #define UCSI_SET_PDR_ACCEPT_ROLE_SWAPS		BIT(25)
 
-- 
2.45.2.1089.g2a221341d9-goog

Re: [PATCH v2 4/4] usb: typec: ucsi: Fix SET_PDR typo in UCSI header file

[Dmitry Baryshkov]

On Wed, Jul 24, 2024 at 08:11:16PM GMT, Jameson Thies wrote:
> Fix SET_PDR typo in UCSI header file.
> 
> Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
> Signed-off-by: Jameson Thies <jthies@google.com>
> ---
> Changes in V2:
> - None.

Fixes: 470ce43a1a81 ("usb: typec: ucsi: Remove struct ucsi_control")

Note, fixes patches should come before the rest of the changes.

> 
>  drivers/usb/typec/ucsi/ucsi.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
> index 6a958eac5703..a0e91335da80 100644
> --- a/drivers/usb/typec/ucsi/ucsi.h
> +++ b/drivers/usb/typec/ucsi/ucsi.h
> @@ -153,7 +153,7 @@ void ucsi_connector_change(struct ucsi *ucsi, u8 num);
>  #define UCSI_SET_UOR_ROLE(_r_)		(((_r_) == TYPEC_HOST ? 1 : 2) << 23)
>  #define UCSI_SET_UOR_ACCEPT_ROLE_SWAPS		BIT(25)
>  
> -/* SET_PDF command bits */
> +/* SET_PDR command bits */
>  #define UCSI_SET_PDR_ROLE(_r_)		(((_r_) == TYPEC_SOURCE ? 1 : 2) << 23)
>  #define UCSI_SET_PDR_ACCEPT_ROLE_SWAPS		BIT(25)
>  
> -- 
> 2.45.2.1089.g2a221341d9-goog
> 

-- 
With best wishes
Dmitry

Re: [PATCH v2 4/4] usb: typec: ucsi: Fix SET_PDR typo in UCSI header file

[Jameson Thies]

Hi Dmitry,
thanks for the heads up. I'll move this to the front of the series in v3.

Thanks,
Jameson