[PATCH v1 0/3] usb: typec: Separate sysfs directory for all USB PD objects

[PATCH v1 0/3] usb: typec: Separate sysfs directory for all USB PD objects

[Heikki Krogerus]

Hi,

Ideally after this there should be no need to add any new USB Power
Delivery specific attribute files directly to the USB Type-C devices
in sysfs. They now have their own directory.

The idea of the series is that any device (so not just USB Type-C
connectors and the partners attached to them) that supports USB Power
Delivery can have this separate sub-directory "usb_power_delivery" in
sysfs, and that sub-directory will have all the USB Power Delivery
objects and all the other USB Power Delivery details.

There are already ways that allow us to read the USB Power Delivery
capabilities from potentially any USB PD capable USB device attached
to the bus - one way is defined in the USB Type-C Bridge
Specification.

Initially the Capability Messages (i.e. PDOs) are exposed.

This is an example (tree view) of the capabilities that the ports on a
normal x86 system advertise to the partner. First you have the message
directory (source_capabilities and sink_capabilities), and that will
have a sub-directory for each PDO that capability message has. The PDO
sub-directories are named by their type. The number in front of the
name is the object position of the PDO:

/sys/class/typec/port0/usb_power_delivery
|-- revision
|-- sink_capabilities/
|   |-- 1:fixed_supply/
|   |   |-- dual_role_data
|   |   |-- dual_role_power
|   |   |-- fast_role_swap_current
|   |   |-- operational_current
|   |   |-- unchunked_extended_messages_supported
|   |   |-- unconstrained_power
|   |   |-- usb_communication_capable
|   |   |-- usb_suspend_supported
|   |   `-- voltage
|   |-- 2:variable_supply/
|   |   |-- maximum_voltage
|   |   |-- minimum_voltage
|   |   `-- operational_current
|   `-- 3:battery/
|       |-- maximum_voltage
|       |-- minimum_voltage
|       `-- operational_power
`-- source_capabilities/
    `-- 1:fixed_supply/
        |-- dual_role_data
        |-- dual_role_power
        |-- maximum_current
        |-- unchunked_extended_messages_supported
        |-- unconstrained_power
        |-- usb_communication_capable
        |-- usb_suspend_supported
        `-- voltage

And these are the capabilities of my Thunderbolt3 dock:

/sys/class/typec/port0-partner/usb_power_delivery
|-- revision
|-- sink_capabilities/
|   `-- 1:fixed_supply/
|       |-- dual_role_data
|       |-- dual_role_power
|       |-- fast_role_swap_current
|       |-- operational_current
|       |-- unchunked_extended_messages_supported
|       |-- unconstrained_power
|       |-- usb_communication_capable
|       |-- usb_suspend_supported
|       `-- voltage
`-- source_capabilities/
    |-- 1:fixed_supply/
    |   |-- dual_role_data
    |   |-- dual_role_power
    |   |-- maximum_current
    |   |-- unchunked_extended_messages_supported
    |   |-- unconstrained_power
    |   |-- usb_communication_capable
    |   |-- usb_suspend_supported
    |   `-- voltage
    |-- 2:fixed_supply/
    |   |-- maximum_current
    |   `-- voltage
    |-- 3:fixed_supply/
    |   |-- maximum_current
    |   `-- voltage
    |-- 4:fixed_supply/
    |   |-- maximum_current
    |   `-- voltage
    `-- 5:fixed_supply/
        |-- maximum_current
        `-- voltage

thanks,

Heikki Krogerus (3):
  usb: typec: Separate USB Power Delivery from USB Type-C
  usb: typec: Functions for USB PD capabilities registration
  usb: typec: ucsi: Register USB Power Delivery Capabilities

 Documentation/ABI/testing/sysfs-class-typec   |  22 +
 .../testing/sysfs-devices-usb_power_delivery  | 273 ++++++++
 drivers/usb/typec/Makefile                    |   2 +-
 drivers/usb/typec/class.c                     | 328 +++++++++
 drivers/usb/typec/class.h                     |   8 +
 drivers/usb/typec/pd.c                        | 651 ++++++++++++++++++
 drivers/usb/typec/pd.h                        |  39 ++
 drivers/usb/typec/ucsi/ucsi.c                 | 128 +++-
 drivers/usb/typec/ucsi/ucsi.h                 |   8 +
 include/linux/usb/pd.h                        |  30 +
 include/linux/usb/typec.h                     |  25 +
 11 files changed, 1502 insertions(+), 12 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-devices-usb_power_delivery
 create mode 100644 drivers/usb/typec/pd.c
 create mode 100644 drivers/usb/typec/pd.h

-- 
2.34.1

[PATCH v1 1/3] usb: typec: Separate USB Power Delivery from USB Type-C

[Heikki Krogerus]

Introducing a separate sysfs directory "usb_power_delivery"
for the devices. This directory should be used to expose all
the USB Power Delivery (USB PD) objects and other details a
device supports from now on.

Initially only USB Type-C devices can expose their USB Power
Delivery capabilities, but later any device attached to the
USB bus may potentially be able to expose them too. That is
already possible with devices that support the USB Type-C
Bridge standard. The API that is introduced here that can be
used to register the USB PD support does not depend on USB
Type-C devices, but it is made part of USB Type-C Connector
Class for now.

First adding support for registration of the Power Data
Objects (PDO) that are part of the Source_Capabilities and
Sink_Capabilities Messages. The content of the Request
Message will likely follow soon.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 .../testing/sysfs-devices-usb_power_delivery  | 273 ++++++++
 drivers/usb/typec/Makefile                    |   2 +-
 drivers/usb/typec/pd.c                        | 651 ++++++++++++++++++
 drivers/usb/typec/pd.h                        |  39 ++
 include/linux/usb/pd.h                        |  30 +
 include/linux/usb/typec.h                     |  10 +
 6 files changed, 1004 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/ABI/testing/sysfs-devices-usb_power_delivery
 create mode 100644 drivers/usb/typec/pd.c
 create mode 100644 drivers/usb/typec/pd.h

diff --git a/Documentation/ABI/testing/sysfs-devices-usb_power_delivery b/Documentation/ABI/testing/sysfs-devices-usb_power_delivery
new file mode 100644
index 0000000000000..10b005e8e315a
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-devices-usb_power_delivery
@@ -0,0 +1,273 @@
+What:		/sys/devices/.../usb_power_delivery
+Date:		February 2022
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		Directory containing details about the USB Power Delivery
+		support of given device.
+
+		When representing the USB Power Delivery Messages, if the
+		message can contain multiple objects (like the capabilities
+		messages - Source_Capabilities and Sink_Capabilities), each
+		message will have its own sub-directory and that sub-directory
+		will then have a separate folder for each object.
+
+			usb_power_delivery/<message>/<object>/<attributes>
+
+		Otherwise the sub-directory will be for the object itself (for
+		example the Battery Status Message can only contain a single
+		Battery Status Data Object).
+
+			usb_power_delivery/<object>/<attributes>
+
+What:		/sys/devices/.../usb_power_delivery/revision
+Date:		February 2022
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		File showing the USB Power Delivery Specification Revision used
+		in communication.
+
+What:		/sys/devices/.../usb_power_delivery/version
+Date:		February 2022
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		This is an optional attribute file showing the version of the
+		specific revision of the USB Power Delivery Specification that
+		the device supports. In most cases the specification version is
+		not known and the file is not available.
+
+What:		/sys/devices/.../usb_power_delivery/source_capabilities<index>
+Date:		February 2022
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		Directory containing the source capabilities for a given USB
+		Power Delivery capable device. Ports on a system can have
+		multiple sets of source capabilities defined for them. The first
+		directory is always named "source_capabilities" (index 0), but
+		starting from the second capabilities set the index number is
+		added to the directory name ("source_capabilities",
+		"source-capabillities1", ...).
+
+		The source capabilities message "Source_Capabilities" contains a
+		set of Power Data Objects (PDO), each representing a type of
+		power supply the device supports. The order of the PDO objects
+		is defined in USB Power Delivery Specification. Each object in
+		this directory will have the object position number as the first
+		character followed by the object type name (":" as delimiter).
+
+			/sys/devices/.../usb_power_delivery/<capability>/<position>:<type>
+
+What:		/sys/devices/.../usb_power_delivery/sink_capabilities<index>
+Date:		February 2022
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		Directory containing the sink capabilities of a given USB Power
+		Delivery capable device. Ports on a system can have multiple
+		sets of sink capabilities defined for them. The first directory
+		is always named "sink_capabilities" (index 0), but starting from
+		the second set the index number is added to the directory name
+		("sink_capabilities", "sink_capabilities1", ...).
+
+		The sink capability message "Sink_Capabilities" contains a set
+		of Power Data Objects (PDO) just like with source capabilities,
+		but instead of describing the power capabilities, these objects
+		describe the power requirements the device has for each type of
+		object.
+
+		The order of the objects in the sink capability message is the
+		same as it's in the source capabilities message.
+
+Fixed Supply Data Objects
+
+What:		/sys/devices/.../usb_power_delivery/<capability>/<position>:fixed_supply
+Date:		February 2022
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		Directory containing the attributes (the bit fields) defined for
+		Fixed Supply PDO.
+
+		The directory "1:fixed_supply" is special. USB Power Delivery
+		Specification dictates that the first PDO (at object position
+		1), and the only mandatory PDO, is always the vSafe5V Fixed
+		Supply Object. vSafe5V Object has additional fields defined for
+		it that the other Fixed Supply Objects do not have and that are
+		related to the USB capabilities rather than power capabilities.
+
+What:		/sys/devices/.../usb_power_delivery/<capability>/1:fixed_supply/dual_role_power
+Date:		February 2022
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		This file contains boolean value that tells does the device
+		support both source and sink power roles.
+
+What:		/sys/devices/.../usb_power_delivery/<capability>/1:fixed_supply/usb_suspend_supported
+Date:		February 2022
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		This file shows the value of the USB Suspend Supported bit in
+		vSafe5V Fixed Supply Object. If the bit is set then the device
+		will follow the USB 2.0 and USB 3.2 rules for suspend and
+		resume.
+
+What:		/sys/devices/.../usb_power_delivery/<capability>/1:fixed_supply/unconstrained_power
+Date:		February 2022
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		This file shows the value of the Unconstrained Power bit in
+		vSafe5V Fixed Supply Object. The bit is set when an external
+		source of power, powerful enough to power the entire system on
+		its own, is available for the device.
+
+What:		/sys/devices/.../usb_power_delivery/<capability>/1:fixed_supply/usb_communication_capable
+Date:		February 2022
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		This file shows the value of the USB Communication Capable bit in
+		vSafe5V Fixed Supply Object.
+
+What:		/sys/devices/.../usb_power_delivery/<capability>/1:fixed_supply/dual_role_data
+Date:		February 2022
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		This file shows the value of the Dual-Role Data bit in vSafe5V
+		Fixed Supply Object. Dual role data means ability act as both
+		USB host and USB device.
+
+What:		/sys/devices/.../usb_power_delivery/<capability>/1:fixed_supply/unchunked_extended_messages_supported
+Date:		February 2022
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		This file shows the value of the Unchunked Extended Messages
+		Supported bit in vSafe5V Fixed Supply Object.
+
+What:		/sys/devices/.../usb_power_delivery/<capability>/<position>:fixed_supply/voltage
+Date:		February 2022
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		The voltage the supply supports in millivolts.
+
+What:		/sys/devices/.../usb_power_delivery/source_capabilities/<position>:fixed_supply/maximum_current
+Date:		February 2022
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		Maximum current of the fixed source supply in milliamperes.
+
+What:		/sys/devices/.../usb_power_delivery/sink_capabilities/<position>:fixed_supply/operational_current
+Date:		February 2022
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		Operational current of the sink in milliamperes.
+
+What:		/sys/devices/.../usb_power_delivery/sink_capabilities/<position>:fixed_supply/fast_role_swap_current
+Date:		February 2022
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		This file contains the value of the "Fast Role Swap USB Type-C
+		Current" field that tells the current level the sink requires
+		after a Fast Role Swap.
+		0 - Fast Swap not supported"
+		1 - Default USB Power"
+		2 - 1.5A@5V"
+		3 - 3.0A@5V"
+
+Variable Supply Data Objects
+
+What:		/sys/devices/.../usb_power_delivery/<capability>/<position>:variable_supply
+Date:		February 2022
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		Directory containing the attributes (the bit fields) defined for
+		Variable Supply PDO.
+
+What:		/sys/devices/.../usb_power_delivery/<capability>/<position>:variable_supply/maximum_voltage
+Date:		February 2022
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		Maximum Voltage in millivolts.
+
+What:		/sys/devices/.../usb_power_delivery/<capability>/<position>:variable_supply/minimum_voltage
+Date:		February 2022
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		Minimum Voltage in millivolts.
+
+What:		/sys/devices/.../usb_power_delivery/source_capabilities/<position>:variable_supply/maximum_current
+Date:		February 2022
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		The maximum current in milliamperes that the source can supply
+		at the given Voltage range.
+
+What:		/sys/devices/.../usb_power_delivery/sink_capabilities/<position>:variable_supply/operational_current
+Date:		February 2022
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		The operational current in milliamperes that the sink requires
+		at the given Voltage range.
+
+Battery Supply Data Objects
+
+What:		/sys/devices/.../usb_power_delivery/<capability>/<position>:battery
+Date:		February 2022
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		Directory containing the attributes (the bit fields) defined for
+		Battery PDO.
+
+What:		/sys/devices/.../usb_power_delivery/<capability>/<position>:battery/maximum_voltage
+Date:		February 2022
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		Maximum Voltage in millivolts.
+
+What:		/sys/devices/.../usb_power_delivery/<capability>/<position>:battery/minimum_voltage
+Date:		February 2022
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		Minimum Voltage in millivolts.
+
+What:		/sys/devices/.../usb_power_delivery/source_capabilities/<position>:battery/maximum_power
+Date:		February 2022
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		Maximum allowable Power in milliwatts.
+
+What:		/sys/devices/.../usb_power_delivery/sink_capabilities/<position>:battery/operational_power
+Date:		February 2022
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		The operational power that the sink requires at the given
+		voltage range.
+
+Standard Power Range (SPR) Programmable Power Supply Objects
+
+What:		/sys/devices/.../usb_power_delivery/<capability>/<position>:programmable_supply
+Date:		February 2022
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		Directory containing the attributes (the bit fields) defined for
+		Programmable Power Supply (PPS) Augmented PDO (APDO).
+
+What:		/sys/devices/.../usb_power_delivery/<capability>/<position>:programmable_supply/maximum_voltage
+Date:		February 2022
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		Maximum Voltage in millivolts.
+
+What:		/sys/devices/.../usb_power_delivery/<capability>/<position>:programmable_supply/minimum_voltage
+Date:		February 2022
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		Minimum Voltage in millivolts.
+
+What:		/sys/devices/.../usb_power_delivery/<capability>/<position>:programmable_supply/maximum_current
+Date:		February 2022
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		Maximum Current in milliamperes.
+
+What:		/sys/devices/.../usb_power_delivery/source_capabilities/<position>:programmable_supply/pps_power_limited
+Date:		February 2022
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		The PPS Power Limited bit indicates whether or not the source
+		supply will exceed the rated output power if requested.
diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
index 57870a2bd7873..4b71a1f5c7e87 100644
--- a/drivers/usb/typec/Makefile
+++ b/drivers/usb/typec/Makefile
@@ -1,6 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_TYPEC)		+= typec.o
-typec-y				:= class.o mux.o bus.o
+typec-y				:= class.o mux.o bus.o pd.o
 typec-$(CONFIG_ACPI)		+= port-mapper.o
 obj-$(CONFIG_TYPEC)		+= altmodes/
 obj-$(CONFIG_TYPEC_TCPM)	+= tcpm/
diff --git a/drivers/usb/typec/pd.c b/drivers/usb/typec/pd.c
new file mode 100644
index 0000000000000..b2f0d677a6bce
--- /dev/null
+++ b/drivers/usb/typec/pd.c
@@ -0,0 +1,651 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * USB Power Delivery sysfs entries
+ *
+ * Copyright (C) 2022, Intel Corporation
+ * Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
+ */
+
+#include <linux/device.h>
+#include <linux/slab.h>
+#include <linux/usb/pd.h>
+
+#include "pd.h"
+
+#define to_pdo(o) container_of(o, struct pdo, kobj)
+
+struct pdo {
+	struct kobject kobj;
+	int object_position;
+	u32 pdo;
+	struct list_head node;
+};
+
+static void pdo_release(struct kobject *kobj)
+{
+	kfree(to_pdo(kobj));
+}
+
+/* -------------------------------------------------------------------------- */
+/* Fixed Supply */
+
+static ssize_t
+dual_role_power_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "%u\n", !!(to_pdo(kobj)->pdo & PDO_FIXED_DUAL_ROLE));
+}
+
+static ssize_t
+usb_suspend_supported_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "%u\n", !!(to_pdo(kobj)->pdo & PDO_FIXED_SUSPEND));
+}
+
+static ssize_t
+unconstrained_power_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "%u\n", !!(to_pdo(kobj)->pdo & PDO_FIXED_EXTPOWER));
+}
+
+static ssize_t
+usb_communication_capable_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "%u\n", !!(to_pdo(kobj)->pdo & PDO_FIXED_USB_COMM));
+}
+
+static ssize_t
+dual_role_data_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "%u\n", !!(to_pdo(kobj)->pdo & PDO_FIXED_DATA_SWAP));
+}
+
+static ssize_t
+unchunked_extended_messages_supported_show(struct kobject *kobj,
+					   struct kobj_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "%u\n", !!(to_pdo(kobj)->pdo & PDO_FIXED_UNCHUNK_EXT));
+}
+
+/*
+ * REVISIT: Peak Current requires access also to the RDO.
+static ssize_t
+peak_current_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+	...
+}
+*/
+
+static ssize_t
+fast_role_swap_current_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "%u\n", to_pdo(kobj)->pdo >> PDO_FIXED_FRS_CURR_SHIFT) & 3;
+}
+
+static ssize_t
+voltage_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "%umV\n", pdo_fixed_voltage(to_pdo(kobj)->pdo));
+}
+
+/* Shared with Variable supplies, both source and sink */
+static ssize_t current_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "%umA\n", pdo_max_current(to_pdo(kobj)->pdo));
+}
+
+/* These additional details are only available with vSafe5V supplies */
+static struct kobj_attribute dual_role_power_attr = __ATTR_RO(dual_role_power);
+static struct kobj_attribute usb_suspend_supported_attr = __ATTR_RO(usb_suspend_supported);
+static struct kobj_attribute unconstrained_power_attr = __ATTR_RO(unconstrained_power);
+static struct kobj_attribute usb_communication_capable_attr = __ATTR_RO(usb_communication_capable);
+static struct kobj_attribute dual_role_data_attr = __ATTR_RO(dual_role_data);
+static struct kobj_attribute
+unchunked_extended_messages_supported_attr = __ATTR_RO(unchunked_extended_messages_supported);
+
+/* Visible on all Fixed type source supplies */
+/*static struct kobj_attribute peak_current_attr = __ATTR_RO(peak_current);*/
+/* Visible on Fixed type sink supplies */
+static struct kobj_attribute fast_role_swap_current_attr = __ATTR_RO(fast_role_swap_current);
+
+/* Shared with Variable type supplies */
+static struct kobj_attribute maximum_current_attr = {
+	.attr = {
+		.name = "maximum_current",
+		.mode = 0444,
+	},
+	.show = current_show,
+};
+
+static struct kobj_attribute operational_current_attr = {
+	.attr = {
+		.name = "operational_current",
+		.mode = 0444,
+	},
+	.show = current_show,
+};
+
+/* Visible on all Fixed type supplies */
+static struct kobj_attribute voltage_attr = __ATTR_RO(voltage);
+
+static struct attribute *source_fixed_supply_attrs[] = {
+	&dual_role_power_attr.attr,
+	&usb_suspend_supported_attr.attr,
+	&unconstrained_power_attr.attr,
+	&usb_communication_capable_attr.attr,
+	&dual_role_data_attr.attr,
+	&unchunked_extended_messages_supported_attr.attr,
+	/*&peak_current_attr.attr,*/
+	&voltage_attr.attr,
+	&maximum_current_attr.attr,
+	NULL
+};
+
+static umode_t fixed_attr_is_visible(struct kobject *kobj, struct attribute *attr, int n)
+{
+	if (to_pdo(kobj)->object_position &&
+	    /*attr != &peak_current_attr.attr &&*/
+	    attr != &voltage_attr.attr &&
+	    attr != &maximum_current_attr.attr &&
+	    attr != &operational_current_attr.attr)
+		return 0;
+
+	return attr->mode;
+}
+
+static const struct attribute_group source_fixed_supply_group = {
+	.is_visible = fixed_attr_is_visible,
+	.attrs = source_fixed_supply_attrs,
+};
+__ATTRIBUTE_GROUPS(source_fixed_supply);
+
+static struct kobj_type source_fixed_supply_type = {
+	.release = pdo_release,
+	.sysfs_ops = &kobj_sysfs_ops,
+	.default_groups = source_fixed_supply_groups,
+};
+
+static struct attribute *sink_fixed_supply_attrs[] = {
+	&dual_role_power_attr.attr,
+	&usb_suspend_supported_attr.attr,
+	&unconstrained_power_attr.attr,
+	&usb_communication_capable_attr.attr,
+	&dual_role_data_attr.attr,
+	&unchunked_extended_messages_supported_attr.attr,
+	&fast_role_swap_current_attr.attr,
+	&voltage_attr.attr,
+	&operational_current_attr.attr,
+	NULL
+};
+
+static const struct attribute_group sink_fixed_supply_group = {
+	.is_visible = fixed_attr_is_visible,
+	.attrs = sink_fixed_supply_attrs,
+};
+__ATTRIBUTE_GROUPS(sink_fixed_supply);
+
+static struct kobj_type sink_fixed_supply_type = {
+	.release = pdo_release,
+	.sysfs_ops = &kobj_sysfs_ops,
+	.default_groups = sink_fixed_supply_groups,
+};
+
+/* -------------------------------------------------------------------------- */
+/* Variable Supply */
+
+static ssize_t
+maximum_voltage_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "%umV\n", pdo_max_voltage(to_pdo(kobj)->pdo));
+}
+
+static ssize_t
+minimum_voltage_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "%umV\n", pdo_min_voltage(to_pdo(kobj)->pdo));
+}
+
+/* Shared with Battery */
+static struct kobj_attribute maximum_voltage_attr = __ATTR_RO(maximum_voltage);
+static struct kobj_attribute minimum_voltage_attr = __ATTR_RO(minimum_voltage);
+
+static struct attribute *source_variable_supply_attrs[] = {
+	&maximum_voltage_attr.attr,
+	&minimum_voltage_attr.attr,
+	&maximum_current_attr.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(source_variable_supply);
+
+static struct kobj_type source_variable_supply_type = {
+	.release = pdo_release,
+	.sysfs_ops = &kobj_sysfs_ops,
+	.default_groups = source_variable_supply_groups,
+};
+
+static struct attribute *sink_variable_supply_attrs[] = {
+	&maximum_voltage_attr.attr,
+	&minimum_voltage_attr.attr,
+	&operational_current_attr.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(sink_variable_supply);
+
+static struct kobj_type sink_variable_supply_type = {
+	.release = pdo_release,
+	.sysfs_ops = &kobj_sysfs_ops,
+	.default_groups = sink_variable_supply_groups,
+};
+
+/* -------------------------------------------------------------------------- */
+/* Battery */
+
+static ssize_t
+maximum_power_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "%umW\n", pdo_max_power(to_pdo(kobj)->pdo));
+}
+
+static ssize_t
+operational_power_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "%umW\n", pdo_max_power(to_pdo(kobj)->pdo));
+}
+
+static struct kobj_attribute maximum_power_attr = __ATTR_RO(maximum_power);
+static struct kobj_attribute operational_power_attr = __ATTR_RO(operational_power);
+
+static struct attribute *source_battery_attrs[] = {
+	&maximum_voltage_attr.attr,
+	&minimum_voltage_attr.attr,
+	&maximum_power_attr.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(source_battery);
+
+static struct kobj_type source_battery_type = {
+	.release = pdo_release,
+	.sysfs_ops = &kobj_sysfs_ops,
+	.default_groups = source_battery_groups,
+};
+
+static struct attribute *sink_battery_attrs[] = {
+	&maximum_voltage_attr.attr,
+	&minimum_voltage_attr.attr,
+	&operational_power_attr.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(sink_battery);
+
+static struct kobj_type sink_battery_type = {
+	.release = pdo_release,
+	.sysfs_ops = &kobj_sysfs_ops,
+	.default_groups = sink_battery_groups,
+};
+
+/* -------------------------------------------------------------------------- */
+/* Standard Power Range (SPR) Programmable Power Supply (PPS) */
+
+static ssize_t
+pps_power_limited_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "%u\n", !!(to_pdo(kobj)->pdo & BIT(27)));
+}
+
+static ssize_t
+pps_max_voltage_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "%umV\n", pdo_pps_apdo_max_voltage(to_pdo(kobj)->pdo));
+}
+
+static ssize_t
+pps_min_voltage_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "%umV\n", pdo_pps_apdo_min_voltage(to_pdo(kobj)->pdo));
+}
+
+static ssize_t
+pps_max_current_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "%umA\n", pdo_pps_apdo_max_current(to_pdo(kobj)->pdo));
+}
+
+static struct kobj_attribute pps_power_limited_attr = __ATTR_RO(pps_power_limited);
+
+static struct kobj_attribute pps_max_voltage_attr = {
+	.attr = {
+		.name = "maximum_voltage",
+		.mode = 0444,
+	},
+	.show = pps_max_voltage_show,
+};
+
+static struct kobj_attribute pps_min_voltage_attr = {
+	.attr = {
+		.name = "minimum_voltage",
+		.mode = 0444,
+	},
+	.show = pps_min_voltage_show,
+};
+
+static struct kobj_attribute pps_max_current_attr = {
+	.attr = {
+		.name = "maximum_current",
+		.mode = 0444,
+	},
+	.show = pps_max_current_show,
+};
+
+static struct attribute *source_pps_attrs[] = {
+	&pps_power_limited_attr.attr,
+	&pps_max_voltage_attr.attr,
+	&pps_min_voltage_attr.attr,
+	&pps_max_current_attr.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(source_pps);
+
+static struct kobj_type source_pps_type = {
+	.release = pdo_release,
+	.sysfs_ops = &kobj_sysfs_ops,
+	.default_groups = source_pps_groups,
+};
+
+static struct attribute *sink_pps_attrs[] = {
+	&pps_max_voltage_attr.attr,
+	&pps_min_voltage_attr.attr,
+	&pps_max_current_attr.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(sink_pps);
+
+static struct kobj_type sink_pps_type = {
+	.release = pdo_release,
+	.sysfs_ops = &kobj_sysfs_ops,
+	.default_groups = sink_pps_groups,
+};
+
+/* -------------------------------------------------------------------------- */
+
+static const char * const supply_name[] = {
+	[PDO_TYPE_FIXED] = "fixed_supply",
+	[PDO_TYPE_BATT]  = "battery",
+	[PDO_TYPE_VAR]	 = "variable_supply",
+};
+
+static const char * const apdo_supply_name[] = {
+	[APDO_TYPE_PPS]  = "programmable_supply",
+};
+
+static struct kobj_type *source_type[] = {
+	[PDO_TYPE_FIXED] = &source_fixed_supply_type,
+	[PDO_TYPE_BATT]  = &source_battery_type,
+	[PDO_TYPE_VAR]   = &source_variable_supply_type,
+};
+
+static struct kobj_type *source_apdo_type[] = {
+	[APDO_TYPE_PPS]  = &source_pps_type,
+};
+
+static struct kobj_type *sink_type[] = {
+	[PDO_TYPE_FIXED] = &sink_fixed_supply_type,
+	[PDO_TYPE_BATT]  = &sink_battery_type,
+	[PDO_TYPE_VAR]   = &sink_variable_supply_type,
+};
+
+static struct kobj_type *sink_apdo_type[] = {
+	[APDO_TYPE_PPS]  = &sink_pps_type,
+};
+
+/* REVISIT: Export when EPR_*_Capabilities need to be supported. */
+static int add_pdo(struct pd_capabilities *cap, u32 pdo, int position)
+{
+	struct kobj_type *type;
+	const char *name;
+	struct pdo *p;
+	int ret;
+
+	p = kzalloc(sizeof(*p), GFP_KERNEL);
+	if (!p)
+		return -ENOMEM;
+
+	p->pdo = pdo;
+	p->object_position = position;
+
+	if (pdo_type(pdo) == PDO_TYPE_APDO) {
+		/* FIXME: Only PPS supported for now! Skipping others. */
+		if (pdo_apdo_type(pdo) > APDO_TYPE_PPS) {
+			dev_warn(kobj_to_dev(cap->kobj.parent->parent),
+				 "%s: Unknown APDO type. PDO 0x%08x\n",
+				 kobject_name(&cap->kobj), pdo);
+			kfree(p);
+			return 0;
+		}
+
+		if (is_source(cap->role))
+			type = source_apdo_type[pdo_apdo_type(pdo)];
+		else
+			type = sink_apdo_type[pdo_apdo_type(pdo)];
+
+		name = apdo_supply_name[pdo_apdo_type(pdo)];
+	} else {
+		if (is_source(cap->role))
+			type = source_type[pdo_type(pdo)];
+		else
+			type = sink_type[pdo_type(pdo)];
+
+		name = supply_name[pdo_type(pdo)];
+	}
+
+	ret = kobject_init_and_add(&p->kobj, type, &cap->kobj, "%u:%s", position + 1, name);
+	if (ret) {
+		kobject_put(&p->kobj);
+		return ret;
+	}
+
+	list_add_tail(&p->node, &cap->pdos);
+
+	return 0;
+}
+
+/* -------------------------------------------------------------------------- */
+
+static const char * const cap_name[] = {
+	[TYPEC_SINK]    = "sink_capabilities",
+	[TYPEC_SOURCE]  = "source_capabilities",
+};
+
+static void pd_capabilities_release(struct kobject *kobj)
+{
+	struct pd_capabilities *cap = to_pd_capabilities(kobj);
+
+	if (is_source(cap->role))
+		ida_simple_remove(&cap->pd->source_cap_ids, cap->id);
+	else
+		ida_simple_remove(&cap->pd->sink_cap_ids, cap->id);
+
+	kfree(cap);
+}
+
+static struct kobj_type pd_capabilities_type = {
+	.release = pd_capabilities_release,
+	.sysfs_ops = &kobj_sysfs_ops,
+};
+
+/**
+ * pd_register_capabilities - Register a set of capabilities
+ * @pd: The USB PD instance that the capabilities belong to
+ * @desc: Description of the capablities message
+ *
+ * This function registers a Capability Message described in @desc. The
+ * capabilities will have their own sub-directory under @pd in sysfs. @pd can
+ * have multiple sets of capabilities defined for it.
+ *
+ * The function returns pointer to struct pd_capabilities, or ERR_PRT(errno).
+ */
+struct pd_capabilities *pd_register_capabilities(struct pd *pd, struct pd_caps_desc *desc)
+{
+	struct pd_capabilities *cap;
+	struct pdo *pdo, *tmp;
+	int ret;
+	int i;
+
+	cap = kzalloc(sizeof(*cap), GFP_KERNEL);
+	if (!cap)
+		return ERR_PTR(-ENOMEM);
+
+	ret = ida_simple_get(is_source(desc->role) ? &pd->source_cap_ids :
+			     &pd->sink_cap_ids, 0, 0, GFP_KERNEL);
+	if (ret < 0) {
+		kfree(cap);
+		return ERR_PTR(ret);
+	}
+
+	cap->id = ret;
+	cap->pd = pd;
+	cap->role = desc->role;
+	INIT_LIST_HEAD(&cap->pdos);
+
+	if (cap->id)
+		ret = kobject_init_and_add(&cap->kobj, &pd_capabilities_type, &pd->kobj,
+					   "%s%u", cap_name[cap->role], cap->id);
+	else
+		ret = kobject_init_and_add(&cap->kobj, &pd_capabilities_type, &pd->kobj,
+					   "%s", cap_name[cap->role]);
+	if (ret)
+		goto err_remove_capability;
+
+	for (i = 0; i < PDO_MAX_OBJECTS && desc->pdo[i]; i++) {
+		ret = add_pdo(cap, desc->pdo[i], i);
+		if (ret)
+			goto err_remove_pdos;
+	}
+
+	if (is_source(cap->role))
+		list_add_tail(&cap->node, &pd->source_capabilities);
+	else
+		list_add_tail(&cap->node, &pd->sink_capabilities);
+
+	return cap;
+
+err_remove_pdos:
+	list_for_each_entry_safe(pdo, tmp, &cap->pdos, node) {
+		list_del(&pdo->node);
+		kobject_put(&pdo->kobj);
+	}
+
+err_remove_capability:
+	kobject_put(&cap->kobj);
+
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(pd_register_capabilities);
+
+/**
+ * pd_unregister_capabilities - Unregister a set of capabilities
+ * @cap: The capabilities
+ */
+void pd_unregister_capabilities(struct pd_capabilities *cap)
+{
+	struct pdo *pdo, *tmp;
+
+	if (!cap)
+		return;
+
+	list_for_each_entry_safe(pdo, tmp, &cap->pdos, node) {
+		list_del(&pdo->node);
+		kobject_put(&pdo->kobj);
+	}
+
+	list_del(&cap->node);
+	kobject_put(&cap->kobj);
+}
+EXPORT_SYMBOL_GPL(pd_unregister_capabilities);
+
+/* -------------------------------------------------------------------------- */
+
+static ssize_t revision_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+	struct pd *pd = to_pd(kobj);
+
+	return sysfs_emit(buf, "%u.%u\n", (pd->revision >> 8) & 0xff, (pd->revision >> 4) & 0xf);
+}
+
+static ssize_t version_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+	struct pd *pd = to_pd(kobj);
+
+	return sysfs_emit(buf, "%u.%u\n", (pd->version >> 8) & 0xff, (pd->version >> 4) & 0xf);
+}
+
+static struct kobj_attribute revision_attr = __ATTR_RO(revision);
+static struct kobj_attribute version_attr = __ATTR_RO(version);
+
+static struct attribute *pd_attrs[] = {
+	&revision_attr.attr,
+	&version_attr.attr,
+	NULL
+};
+
+static umode_t pd_attr_is_visible(struct kobject *kobj, struct attribute *attr, int n)
+{
+	struct pd *pd = to_pd(kobj);
+
+	if (attr == &version_attr.attr && !pd->version)
+		return 0;
+
+	return attr->mode;
+}
+
+static const struct attribute_group pd_group = {
+	.is_visible = pd_attr_is_visible,
+	.attrs = pd_attrs,
+};
+__ATTRIBUTE_GROUPS(pd);
+
+static void pd_release(struct kobject *kobj)
+{
+	struct pd *pd = to_pd(kobj);
+
+	ida_destroy(&pd->source_cap_ids);
+	ida_destroy(&pd->sink_cap_ids);
+	put_device(pd->dev);
+	kfree(pd);
+}
+
+static struct kobj_type pd_type = {
+	.release = pd_release,
+	.sysfs_ops = &kobj_sysfs_ops,
+	.default_groups = pd_groups,
+};
+
+struct pd *pd_register(struct device *dev, struct pd_desc *desc)
+{
+	struct pd *pd;
+	int ret;
+
+	pd = kzalloc(sizeof(*pd), GFP_KERNEL);
+	if (!pd)
+		return ERR_PTR(-ENOMEM);
+
+	ida_init(&pd->sink_cap_ids);
+	ida_init(&pd->source_cap_ids);
+	INIT_LIST_HEAD(&pd->sink_capabilities);
+	INIT_LIST_HEAD(&pd->source_capabilities);
+
+	pd->dev = get_device(dev);
+	pd->revision = desc->revision;
+	pd->version = desc->version;
+
+	ret = kobject_init_and_add(&pd->kobj, &pd_type, &dev->kobj, "usb_power_delivery");
+	if (ret) {
+		kobject_put(&pd->kobj);
+		return ERR_PTR(ret);
+	}
+
+	return pd;
+}
+
+void pd_unregister(struct pd *pd)
+{
+	kobject_put(&pd->kobj);
+}
diff --git a/drivers/usb/typec/pd.h b/drivers/usb/typec/pd.h
new file mode 100644
index 0000000000000..a24416e05a3c2
--- /dev/null
+++ b/drivers/usb/typec/pd.h
@@ -0,0 +1,39 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __USB_POWER_DELIVERY__
+#define __USB_POWER_DELIVERY__
+
+#include <linux/kobject.h>
+
+struct pd_capabilities {
+	struct kobject kobj;
+	unsigned int id;
+	struct pd *pd;
+	enum typec_role role;
+	struct list_head pdos;
+	struct list_head node;
+};
+
+struct pd {
+	struct kobject		kobj;
+	struct device		*dev;
+
+	u16			revision; /* 0300H = "3.0" */
+	u16			version;
+
+	struct ida		source_cap_ids;
+	struct ida		sink_cap_ids;
+	struct list_head	source_capabilities;
+	struct list_head	sink_capabilities;
+};
+
+#define to_pd_capabilities(o) container_of(o, struct pd_capabilities, kobj)
+#define to_pd(o) container_of(o, struct pd, kobj)
+
+struct pd *pd_register(struct device *dev, struct pd_desc *desc);
+void pd_unregister(struct pd *pd);
+
+int pd_init(void);
+void pd_exit(void);
+
+#endif /* __USB_POWER_DELIVERY__ */
diff --git a/include/linux/usb/pd.h b/include/linux/usb/pd.h
index 96b7ff66f074b..9cf78fa32623d 100644
--- a/include/linux/usb/pd.h
+++ b/include/linux/usb/pd.h
@@ -495,4 +495,34 @@ static inline unsigned int rdo_max_power(u32 rdo)
 
 #define PD_P_SNK_STDBY_MW	2500	/* 2500 mW */
 
+#if IS_ENABLED(CONFIG_TYPEC)
+
+/**
+ * pd_desc - USB Power Delivery Descriptor
+ * @revision: USB Power Delivery Specification Revision
+ * @version: USB Power Delivery Specicication Version - optional
+ */
+struct pd_desc {
+	u16 revision;
+	u16 version;
+};
+
+/**
+ * pd_caps_desc - Description of USB Power Delivery Capabilities Message
+ * @pdo: The Power Data Objects in the Capability Message
+ * @role: Power role of the capabilities
+ */
+struct pd_caps_desc {
+	u32 pdo[PDO_MAX_OBJECTS];
+	enum typec_role role;
+};
+
+struct pd_capabilities *pd_register_capabilities(struct pd *pd, struct pd_caps_desc *desc);
+void pd_unregister_capabilities(struct pd_capabilities *cap);
+
+struct pd *pd_register(struct device *dev, struct pd_desc *desc);
+void pd_unregister(struct pd *pd);
+
+#endif /* CONFIG_TYPEC */
+
 #endif /* __LINUX_USB_PD_H */
diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
index 7ba45a97eeae3..a7525b9931b69 100644
--- a/include/linux/usb/typec.h
+++ b/include/linux/usb/typec.h
@@ -52,6 +52,16 @@ enum typec_role {
 	TYPEC_SOURCE,
 };
 
+static inline int is_sink(enum typec_role role)
+{
+	return role == TYPEC_SINK;
+}
+
+static inline int is_source(enum typec_role role)
+{
+	return role == TYPEC_SOURCE;
+}
+
 enum typec_pwr_opmode {
 	TYPEC_PWR_MODE_USB,
 	TYPEC_PWR_MODE_1_5A,
-- 
2.34.1

Re: [PATCH v1 1/3] usb: typec: Separate USB Power Delivery from USB Type-C

[Greg Kroah-Hartman]

On Thu, Feb 03, 2022 at 05:46:55PM +0300, Heikki Krogerus wrote:
> +/* These additional details are only available with vSafe5V supplies */
> +static struct kobj_attribute dual_role_power_attr = __ATTR_RO(dual_role_power);
> +static struct kobj_attribute usb_suspend_supported_attr = __ATTR_RO(usb_suspend_supported);
> +static struct kobj_attribute unconstrained_power_attr = __ATTR_RO(unconstrained_power);
> +static struct kobj_attribute usb_communication_capable_attr = __ATTR_RO(usb_communication_capable);
> +static struct kobj_attribute dual_role_data_attr = __ATTR_RO(dual_role_data);
> +static struct kobj_attribute
> +unchunked_extended_messages_supported_attr = __ATTR_RO(unchunked_extended_messages_supported);

Note, no 'struct device' should ever have a "raw" kobject hanging off of
it.  If so, something went wrong.

If you do this, userspace will never be notified of the attributes and
any userspace representation of the tree will be messed up.

Please, use an attribute directory with a name, or if you really need to
go another level deep, use a real 'struct device'.  As-is here, I can't
take it.

thanks,

greg k-h

Re: [PATCH v1 1/3] usb: typec: Separate USB Power Delivery from USB Type-C

[Heikki Krogerus]

Hi Greg,

On Thu, Feb 03, 2022 at 03:55:19PM +0100, Greg Kroah-Hartman wrote:
> On Thu, Feb 03, 2022 at 05:46:55PM +0300, Heikki Krogerus wrote:
> > +/* These additional details are only available with vSafe5V supplies */
> > +static struct kobj_attribute dual_role_power_attr = __ATTR_RO(dual_role_power);
> > +static struct kobj_attribute usb_suspend_supported_attr = __ATTR_RO(usb_suspend_supported);
> > +static struct kobj_attribute unconstrained_power_attr = __ATTR_RO(unconstrained_power);
> > +static struct kobj_attribute usb_communication_capable_attr = __ATTR_RO(usb_communication_capable);
> > +static struct kobj_attribute dual_role_data_attr = __ATTR_RO(dual_role_data);
> > +static struct kobj_attribute
> > +unchunked_extended_messages_supported_attr = __ATTR_RO(unchunked_extended_messages_supported);
> 
> Note, no 'struct device' should ever have a "raw" kobject hanging off of
> it.  If so, something went wrong.
> 
> If you do this, userspace will never be notified of the attributes and
> any userspace representation of the tree will be messed up.
> 
> Please, use an attribute directory with a name, or if you really need to
> go another level deep, use a real 'struct device'.  As-is here, I can't
> take it.

OK, got it. I don't think we can avoid the deeper levels, not without
making this really cryptic, and not really usable in all cases. These
objects are trying to represent (parts) of the protocol - the
messages, the objects in those messages, and later the responses to
those messages.

But I'm also trying to avoid having to claim that these objects are
"devices", because honestly, claiming that the packages used in
communication are devices is confusing, and just wrong. If we take
that road, then we really should redefine what struct device is
supposed to represent, and rename it also.

So would it be OK that, instead of registering these objects as
devices, we just introduce a kset where we can group them
(/sys/kernel/usb_power_delivery)?

thanks,

-- 
heikki

Re: [PATCH v1 1/3] usb: typec: Separate USB Power Delivery from USB Type-C

[Greg Kroah-Hartman]

On Fri, Feb 04, 2022 at 12:04:41PM +0200, Heikki Krogerus wrote:
> Hi Greg,
> 
> On Thu, Feb 03, 2022 at 03:55:19PM +0100, Greg Kroah-Hartman wrote:
> > On Thu, Feb 03, 2022 at 05:46:55PM +0300, Heikki Krogerus wrote:
> > > +/* These additional details are only available with vSafe5V supplies */
> > > +static struct kobj_attribute dual_role_power_attr = __ATTR_RO(dual_role_power);
> > > +static struct kobj_attribute usb_suspend_supported_attr = __ATTR_RO(usb_suspend_supported);
> > > +static struct kobj_attribute unconstrained_power_attr = __ATTR_RO(unconstrained_power);
> > > +static struct kobj_attribute usb_communication_capable_attr = __ATTR_RO(usb_communication_capable);
> > > +static struct kobj_attribute dual_role_data_attr = __ATTR_RO(dual_role_data);
> > > +static struct kobj_attribute
> > > +unchunked_extended_messages_supported_attr = __ATTR_RO(unchunked_extended_messages_supported);
> > 
> > Note, no 'struct device' should ever have a "raw" kobject hanging off of
> > it.  If so, something went wrong.
> > 
> > If you do this, userspace will never be notified of the attributes and
> > any userspace representation of the tree will be messed up.
> > 
> > Please, use an attribute directory with a name, or if you really need to
> > go another level deep, use a real 'struct device'.  As-is here, I can't
> > take it.
> 
> OK, got it. I don't think we can avoid the deeper levels, not without
> making this really cryptic, and not really usable in all cases. These
> objects are trying to represent (parts) of the protocol - the
> messages, the objects in those messages, and later the responses to
> those messages.
> 
> But I'm also trying to avoid having to claim that these objects are
> "devices", because honestly, claiming that the packages used in
> communication are devices is confusing, and just wrong. If we take
> that road, then we really should redefine what struct device is
> supposed to represent, and rename it also.

Fair enough, this isn't really a device, it's an "attribute" of your
device you are wanting to show.  It's just that you are really "deep".

You asked for:

/sys/class/typec/port0/usb_power_delivery
|-- revision
|-- sink_capabilities/
|   |-- 1:fixed_supply/
|   |   |-- dual_role_data
|   |   |-- dual_role_power
|   |   |-- fast_role_swap_current
|   |   |-- operational_current
|   |   |-- unchunked_extended_messages_supported
|   |   |-- unconstrained_power
|   |   |-- usb_communication_capable
|   |   |-- usb_suspend_supported
|   |   `-- voltage
|   |-- 2:variable_supply/
|   |   |-- maximum_voltage
|   |   |-- minimum_voltage
|   |   `-- operational_current
|   `-- 3:battery/
|       |-- maximum_voltage
|       |-- minimum_voltage
|       `-- operational_power
`-- source_capabilities/
    `-- 1:fixed_supply/
        |-- dual_role_data
        |-- dual_role_power
        |-- maximum_current
        |-- unchunked_extended_messages_supported
        |-- unconstrained_power
        |-- usb_communication_capable
        |-- usb_suspend_supported
        `-- voltage


To start with, your "attribute" is really "usb_power_delivery" here, so
you can just use an attribute group name to get the "revision" file.

But then the later ones could be flat in that directory as well, using a
':' to split as you did already, and the above could turn into:

/sys/class/typec/port0/usb_power_delivery
|-- revision
|-- sink_capabilites:1:fixed_supply:dual_role_data
|-- sink_capabilites:1:fixed_supply:dual_role_power
|-- sink_capabilites:1:fixed_supply:fase_role_swap_current
....
|-- sink_capabilites:2:variable_supply:maximum_voltage
|-- sink_capabilites:2:variable_supply:minimum_voltage
...
|-- source_capabilities:1:fixed_supply:dual_role_data
|-- source_capabilities:1:fixed_supply:dual_role_power
|-- source_capabilities:1:fixed_supply:maximum_current
...

But ick, that's also a mess as you are now forced to parse filenames in
userspace in a different way than "normal".

Is there anything special about the number here?  It's the "position"
which will be unique.  So make that position a device, as that's kind of
what it is (like usb endpoints are devices)

Then you could make a bus for the positions and all would be good, and
you could turn this into:


/sys/class/typec/port0/usb_power_delivery
|-- revision
|-- sink_capabilities:1/
|   `-- fixed_supply/
|       |-- dual_role_data
|       |-- dual_role_power
|       |-- fast_role_swap_current
|       |-- operational_current
|       |-- unchunked_extended_messages_supported
|       |-- unconstrained_power
|       |-- usb_communication_capable
|       |-- usb_suspend_supported
|       `-- voltage
|-- sink_capabilities:2/
|   `-- variable_supply/
|       |-- maximum_voltage
|       |-- minimum_voltage
|       `-- operational_current
|-- sink_capabilities:3/
|   `-- battery/
|       |-- maximum_voltage
|       |-- minimum_voltage
|       `-- operational_power
`-- source_capabilities:1/
    `-- fixed_supply/
        |-- dual_role_data
        |-- dual_role_power
        |-- maximum_current
        |-- unchunked_extended_messages_supported
        |-- unconstrained_power
        |-- usb_communication_capable
        |-- usb_suspend_supported
        `-- voltage

Would that work?

> So would it be OK that, instead of registering these objects as
> devices, we just introduce a kset where we can group them
> (/sys/kernel/usb_power_delivery)?

You want to show this as attched to a specific port somehow, so that
location is not going to work.

thanks,

greg k-h

Re: [PATCH v1 1/3] usb: typec: Separate USB Power Delivery from USB Type-C

[Heikki Krogerus]

On Fri, Feb 04, 2022 at 02:59:26PM +0100, Greg Kroah-Hartman wrote:
> On Fri, Feb 04, 2022 at 12:04:41PM +0200, Heikki Krogerus wrote:
> > Hi Greg,
> > 
> > On Thu, Feb 03, 2022 at 03:55:19PM +0100, Greg Kroah-Hartman wrote:
> > > On Thu, Feb 03, 2022 at 05:46:55PM +0300, Heikki Krogerus wrote:
> > > > +/* These additional details are only available with vSafe5V supplies */
> > > > +static struct kobj_attribute dual_role_power_attr = __ATTR_RO(dual_role_power);
> > > > +static struct kobj_attribute usb_suspend_supported_attr = __ATTR_RO(usb_suspend_supported);
> > > > +static struct kobj_attribute unconstrained_power_attr = __ATTR_RO(unconstrained_power);
> > > > +static struct kobj_attribute usb_communication_capable_attr = __ATTR_RO(usb_communication_capable);
> > > > +static struct kobj_attribute dual_role_data_attr = __ATTR_RO(dual_role_data);
> > > > +static struct kobj_attribute
> > > > +unchunked_extended_messages_supported_attr = __ATTR_RO(unchunked_extended_messages_supported);
> > > 
> > > Note, no 'struct device' should ever have a "raw" kobject hanging off of
> > > it.  If so, something went wrong.
> > > 
> > > If you do this, userspace will never be notified of the attributes and
> > > any userspace representation of the tree will be messed up.
> > > 
> > > Please, use an attribute directory with a name, or if you really need to
> > > go another level deep, use a real 'struct device'.  As-is here, I can't
> > > take it.
> > 
> > OK, got it. I don't think we can avoid the deeper levels, not without
> > making this really cryptic, and not really usable in all cases. These
> > objects are trying to represent (parts) of the protocol - the
> > messages, the objects in those messages, and later the responses to
> > those messages.
> > 
> > But I'm also trying to avoid having to claim that these objects are
> > "devices", because honestly, claiming that the packages used in
> > communication are devices is confusing, and just wrong. If we take
> > that road, then we really should redefine what struct device is
> > supposed to represent, and rename it also.
> 
> Fair enough, this isn't really a device, it's an "attribute" of your
> device you are wanting to show.  It's just that you are really "deep".
> 
> You asked for:
> 
> /sys/class/typec/port0/usb_power_delivery
> |-- revision
> |-- sink_capabilities/
> |   |-- 1:fixed_supply/
> |   |   |-- dual_role_data
> |   |   |-- dual_role_power
> |   |   |-- fast_role_swap_current
> |   |   |-- operational_current
> |   |   |-- unchunked_extended_messages_supported
> |   |   |-- unconstrained_power
> |   |   |-- usb_communication_capable
> |   |   |-- usb_suspend_supported
> |   |   `-- voltage
> |   |-- 2:variable_supply/
> |   |   |-- maximum_voltage
> |   |   |-- minimum_voltage
> |   |   `-- operational_current
> |   `-- 3:battery/
> |       |-- maximum_voltage
> |       |-- minimum_voltage
> |       `-- operational_power
> `-- source_capabilities/
>     `-- 1:fixed_supply/
>         |-- dual_role_data
>         |-- dual_role_power
>         |-- maximum_current
>         |-- unchunked_extended_messages_supported
>         |-- unconstrained_power
>         |-- usb_communication_capable
>         |-- usb_suspend_supported
>         `-- voltage
> 
> 
> To start with, your "attribute" is really "usb_power_delivery" here, so
> you can just use an attribute group name to get the "revision" file.
> 
> But then the later ones could be flat in that directory as well, using a
> ':' to split as you did already, and the above could turn into:
> 
> /sys/class/typec/port0/usb_power_delivery
> |-- revision
> |-- sink_capabilites:1:fixed_supply:dual_role_data
> |-- sink_capabilites:1:fixed_supply:dual_role_power
> |-- sink_capabilites:1:fixed_supply:fase_role_swap_current
> ....
> |-- sink_capabilites:2:variable_supply:maximum_voltage
> |-- sink_capabilites:2:variable_supply:minimum_voltage
> ...
> |-- source_capabilities:1:fixed_supply:dual_role_data
> |-- source_capabilities:1:fixed_supply:dual_role_power
> |-- source_capabilities:1:fixed_supply:maximum_current
> ...
> 
> But ick, that's also a mess as you are now forced to parse filenames in
> userspace in a different way than "normal".
> 
> Is there anything special about the number here?  It's the "position"
> which will be unique.  So make that position a device, as that's kind of
> what it is (like usb endpoints are devices)
> 
> Then you could make a bus for the positions and all would be good, and
> you could turn this into:
> 
> 
> /sys/class/typec/port0/usb_power_delivery
> |-- revision
> |-- sink_capabilities:1/
> |   `-- fixed_supply/
> |       |-- dual_role_data
> |       |-- dual_role_power
> |       |-- fast_role_swap_current
> |       |-- operational_current
> |       |-- unchunked_extended_messages_supported
> |       |-- unconstrained_power
> |       |-- usb_communication_capable
> |       |-- usb_suspend_supported
> |       `-- voltage
> |-- sink_capabilities:2/
> |   `-- variable_supply/
> |       |-- maximum_voltage
> |       |-- minimum_voltage
> |       `-- operational_current
> |-- sink_capabilities:3/
> |   `-- battery/
> |       |-- maximum_voltage
> |       |-- minimum_voltage
> |       `-- operational_power
> `-- source_capabilities:1/
>     `-- fixed_supply/
>         |-- dual_role_data
>         |-- dual_role_power
>         |-- maximum_current
>         |-- unchunked_extended_messages_supported
>         |-- unconstrained_power
>         |-- usb_communication_capable
>         |-- usb_suspend_supported
>         `-- voltage
> 
> Would that work?

Unfortunately the object position is only defined for these
capability messages, not for the other messages. It's not going to
work :-(

> > So would it be OK that, instead of registering these objects as
> > devices, we just introduce a kset where we can group them
> > (/sys/kernel/usb_power_delivery)?
> 
> You want to show this as attched to a specific port somehow, so that
> location is not going to work.

But the idea with that kset would be that you have a separate
directory for each port there for this stuff:

        /sys/kernel/usb_power_delivery/port0
        |-- revision
        ...

And those directories we could then link to the actual device:

        /sys/class/typec/port0/usb_power_delivery -> ../../../kernel/usb_power_delivery/port0

thanks,

-- 
heikki

[PATCH v1 2/3] usb: typec: Functions for USB PD capabilities registration

[Heikki Krogerus]

This adds functions that can be used to register and assign
USB Power Delivery Capabilities Messages so they are exposed
to the user via sysfs. By assigning the capabilities the
driver will create a symlink pointing to those capabilities
named "source_capabilities" and (or) "sink_capabilities"
depending on the role of the message (Source_Capablities or
Sink_Capabilities Message).

Ports can support multiple sets of capabilities but only one
that is advertised to the partner can be active at any given
time. Separate sysfs attribute files are added that can be
used to select the currently active capabilities message.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 Documentation/ABI/testing/sysfs-class-typec |  22 ++
 drivers/usb/typec/class.c                   | 328 ++++++++++++++++++++
 drivers/usb/typec/class.h                   |   8 +
 include/linux/usb/typec.h                   |  15 +
 4 files changed, 373 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
index 75088ecad2029..d60720befeeb3 100644
--- a/Documentation/ABI/testing/sysfs-class-typec
+++ b/Documentation/ABI/testing/sysfs-class-typec
@@ -141,6 +141,28 @@ Description:
 		- "reverse": CC2 orientation
 		- "unknown": Orientation cannot be determined.
 
+What:		/sys/class/typec/<port>/select_sink_capabilities
+Date:		February 2022
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		This file lists the available USB Power Delivery
+		Sink_Capabilities Messages, the same that can be read from the
+		"usb_power_delivery" sub-directory under the port device's sysfs
+		directory. The active message, the one that is advertised to the
+		partner, is in brackets. The active message can be changed by
+		writing to this file.
+
+What:		/sys/class/typec/<port>/select_source_capabilities
+Date:		February 2022
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		This file lists the available USB Power Delivery
+		Source_Capabilities Messages, the same that can be read from the
+		"usb_power_delivery" sub-directory under the port device's sysfs
+		directory. The active message, the one that is advertised to the
+		partner, is in brackets. The active message can be changed by
+		writing to this file.
+
 USB Type-C partner devices (eg. /sys/class/typec/port0-partner/)
 
 What:		/sys/class/typec/<port>-partner/accessory_mode
diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 45a6f0c807cb5..eef0b59209331 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -15,6 +15,7 @@
 
 #include "bus.h"
 #include "class.h"
+#include "pd.h"
 
 static DEFINE_IDA(typec_index_ida);
 
@@ -26,6 +27,11 @@ struct class typec_class = {
 /* ------------------------------------------------------------------------- */
 /* Common attributes */
 
+static const char * const cap_name[] = {
+	[TYPEC_SINK]		= "sink_capabilities",
+	[TYPEC_SOURCE]		= "source_capabilities",
+};
+
 static const char * const typec_accessory_modes[] = {
 	[TYPEC_ACCESSORY_NONE]		= "none",
 	[TYPEC_ACCESSORY_AUDIO]		= "analog_audio",
@@ -589,6 +595,100 @@ EXPORT_SYMBOL_GPL(typec_unregister_altmode);
 /* ------------------------------------------------------------------------- */
 /* Type-C Partners */
 
+/**
+ * typec_partner_set_pd_capabilities - Create link to the USB PD capabilities
+ * @partner: USB Type-C partner
+ * @cap: USB Power Delivery capabilities
+ *
+ * The link will be named "source_capabilities" or "sink_capabilities" depending
+ * on the role of @cap.
+ */
+int typec_partner_set_pd_capabilities(struct typec_partner *partner, struct pd_capabilities *cap)
+{
+	int ret;
+
+	if (!cap)
+		return -EINVAL;
+
+	if ((is_source(cap->role) && partner->source_caps) || partner->sink_caps)
+		return -EEXIST;
+
+	ret = sysfs_create_link(&partner->dev.kobj, &cap->kobj, cap_name[cap->role]);
+	if (ret)
+		return ret;
+
+	if (is_source(cap->role))
+		partner->source_caps = cap;
+	else
+		partner->sink_caps = cap;
+
+	kobject_get(&cap->kobj);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(typec_partner_set_pd_capabilities);
+
+/**
+ * typec_partner_unset_pd_capabilities - Remove link to the USB PD capabilities
+ * @port: USB Type-C partner
+ * @cap: USB Power Delivery capabilities
+ */
+void typec_partner_unset_pd_capabilities(struct typec_partner *partner, enum typec_role role)
+{
+	struct pd_capabilities *cap;
+
+	if (is_source(role)) {
+		cap = partner->source_caps;
+		partner->source_caps = NULL;
+	} else {
+		cap = partner->sink_caps;
+		partner->sink_caps = NULL;
+	}
+
+	if (!cap)
+		return;
+
+	sysfs_remove_link(&partner->dev.kobj, cap_name[role]);
+	kobject_put(&cap->kobj);
+}
+EXPORT_SYMBOL_GPL(typec_partner_unset_pd_capabilities);
+
+/**
+ * typec_partner_register_pd - Register USB Power Delivery support
+ * @partner: The partner device that support USB Power Delivery
+ * @desc: Description of the USB Power Delivery support
+ *
+ * The function returns pointer to struct pd, or errno on failure.
+ */
+struct pd *typec_partner_register_pd(struct typec_partner *partner, struct pd_desc *desc)
+{
+	struct pd *pd;
+
+	if (partner->pd)
+		return ERR_PTR(-EEXIST);
+
+	pd = pd_register(&partner->dev, desc);
+	if (!IS_ERR(pd))
+		partner->pd = pd;
+
+	return pd;
+}
+EXPORT_SYMBOL_GPL(typec_partner_register_pd);
+
+/**
+ * typec_partner_unregister_pd - Unregister USB Power Delivery sysfs directory
+ * @partner: The partner device
+ */
+void typec_partner_unregister_pd(struct typec_partner *partner)
+{
+	if (!partner->pd)
+		return;
+
+	pd_unregister(partner->pd);
+	partner->pd = NULL;
+}
+EXPORT_SYMBOL_GPL(typec_partner_unregister_pd);
+
 static ssize_t accessory_mode_show(struct device *dev,
 				   struct device_attribute *attr,
 				   char *buf)
@@ -1170,6 +1270,234 @@ EXPORT_SYMBOL_GPL(typec_unregister_cable);
 /* ------------------------------------------------------------------------- */
 /* USB Type-C ports */
 
+/**
+ * typec_port_set_pd_capabilities - Create link to the USB PD capabilities
+ * @port: USB Type-C port
+ * @cap: Active USB Power Delivery capabilities
+ *
+ * Creates a link to the currently active capabilities @cap for @port, i.e.
+ * capabilities that are advertised to the partner. Both roles have their own
+ * active capabilities, so the link will be "source_capabilities" or
+ * "sink_capabilities" depending on the role of @cap.
+ */
+int typec_port_set_pd_capabilities(struct typec_port *port, struct pd_capabilities *cap)
+{
+	int ret;
+
+	if (!cap)
+		return -EINVAL;
+
+	if (is_source(cap->role) && port->source_caps)
+		kobject_put(&port->source_caps->kobj);
+	else if (port->sink_caps)
+		kobject_put(&port->sink_caps->kobj);
+
+	sysfs_remove_link(&port->dev.kobj, cap_name[cap->role]);
+
+	ret = sysfs_create_link(&port->dev.kobj, &cap->kobj, cap_name[cap->role]);
+	if (ret)
+		return ret;
+
+	if (is_source(cap->role))
+		port->source_caps = cap;
+	else
+		port->sink_caps = cap;
+
+	kobject_get(&cap->kobj);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(typec_port_set_pd_capabilities);
+
+/**
+ * typec_port_unset_pd_capabilities - Remove link to the USB PD capabilities
+ * @port: USB Type-C port
+ * @cap: Active USB Power Delivery capabilities
+ */
+int typec_port_unset_pd_capabilities(struct typec_port *port, struct pd_capabilities *cap)
+{
+	if (is_source(cap->role)) {
+		if (port->source_caps != cap)
+			return -EINVAL;
+		port->source_caps = NULL;
+	} else {
+		if (port->sink_caps != cap)
+			return -EINVAL;
+		port->sink_caps = NULL;
+	}
+
+	sysfs_remove_link(&port->dev.kobj, cap_name[cap->role]);
+	kobject_put(&cap->kobj);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(typec_port_unset_pd_capabilities);
+
+static ssize_t select_source_capabilities_store(struct device *dev,
+						struct device_attribute *attr,
+						const char *buf, size_t size)
+{
+	struct typec_port *port = to_typec_port(dev);
+	struct pd_capabilities *cap;
+	int ret;
+	int i;
+
+	if (!port->ops || !port->ops->capabilities_set)
+		return -EOPNOTSUPP;
+
+	if (sscanf(buf, "source_capabilities%d", &i) != 1)
+		return -EINVAL;
+
+	list_for_each_entry(cap, &port->pd->source_capabilities, node) {
+		if (cap->id == i) {
+			ret = port->ops->capabilities_set(port, cap);
+			if (ret)
+				return ret;
+			return size;
+		}
+	}
+
+	return -EINVAL;
+}
+
+static ssize_t select_source_capabilities_show(struct device *dev,
+					       struct device_attribute *attr, char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+	const char *name = cap_name[TYPEC_SOURCE];
+	struct pd_capabilities *cap;
+	ssize_t ret = 0;
+
+	list_for_each_entry(cap, &port->pd->source_capabilities, node) {
+		if (cap == port->source_caps)
+			ret += sysfs_emit(buf + ret, "[%s%d] ", name, cap->id);
+		else
+			ret += sysfs_emit(buf + ret, "%s%d ", name, cap->id);
+	}
+
+	buf[ret - 1] = '\n';
+
+	return ret;
+}
+static DEVICE_ATTR_RW(select_source_capabilities);
+
+static ssize_t select_sink_capabilities_store(struct device *dev,
+					      struct device_attribute *attr,
+					      const char *buf, size_t size)
+{
+	struct typec_port *port = to_typec_port(dev);
+	struct pd_capabilities *cap;
+	int ret;
+	int i;
+
+	if (!port->ops || !port->ops->capabilities_set)
+		return -EOPNOTSUPP;
+
+	if (sscanf(buf, "sink_capabilities%d", &i) != 1)
+		return -EINVAL;
+
+	list_for_each_entry(cap, &port->pd->sink_capabilities, node) {
+		if (cap->id == i) {
+			ret = port->ops->capabilities_set(port, cap);
+			if (ret)
+				return ret;
+			return size;
+		}
+	}
+
+	return -EINVAL;
+}
+
+static ssize_t select_sink_capabilities_show(struct device *dev,
+					     struct device_attribute *attr, char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+	const char *name = cap_name[TYPEC_SINK];
+	struct pd_capabilities *cap;
+	ssize_t ret = 0;
+
+	list_for_each_entry(cap, &port->pd->sink_capabilities, node) {
+		if (cap == port->sink_caps)
+			ret += sysfs_emit(buf + ret, "[%s%d] ", name, cap->id);
+		else
+			ret += sysfs_emit(buf + ret, "%s%d ", name, cap->id);
+	}
+
+	buf[ret - 1] = '\n';
+
+	return ret;
+}
+static DEVICE_ATTR_RW(select_sink_capabilities);
+
+static struct attribute *port_attrs[] = {
+	&dev_attr_select_source_capabilities.attr,
+	&dev_attr_select_sink_capabilities.attr,
+	NULL
+};
+
+static umode_t port_attr_is_visible(struct kobject *kobj, struct attribute *attr, int n)
+{
+	struct typec_port *port = to_typec_port(kobj_to_dev(kobj));
+
+	if (!port->ops || !port->ops->capabilities_set)
+		return 0;
+
+	return attr->mode;
+}
+
+static const struct attribute_group port_group = {
+	.is_visible = port_attr_is_visible,
+	.attrs = port_attrs,
+};
+
+/**
+ * typec_port_register_pd - Register USB Power Delivery support
+ * @port: The port that support USB Power Delivery
+ * @desc: Description of the USB Power Delivery support
+ *
+ * This directory will house all the USB Power Delivery objects of @port that
+ * the operating system can access. The function returns pointer to struct
+ * pd, or errno on failure.
+ */
+struct pd *typec_port_register_pd(struct typec_port *port, struct pd_desc *desc)
+{
+	struct pd *pd;
+	int ret;
+
+	if (port->pd)
+		return ERR_PTR(-EEXIST);
+
+	pd = pd_register(&port->dev, desc);
+	if (IS_ERR(pd))
+		return pd;
+
+	port->pd = pd;
+
+	ret = sysfs_create_group(&port->dev.kobj, &port_group);
+	if (ret) {
+		pd_unregister(pd);
+		return ERR_PTR(ret);
+	}
+
+	return pd;
+}
+EXPORT_SYMBOL_GPL(typec_port_register_pd);
+
+/**
+ * typec_port_unregister_pd - Unregister USB Power Delivery sysfs directory
+ * @port: The port device
+ */
+void typec_port_unregister_pd(struct typec_port *port)
+{
+	if (!port->pd)
+		return;
+
+	sysfs_remove_group(&port->dev.kobj, &port_group);
+	pd_unregister(port->pd);
+	port->pd = NULL;
+}
+EXPORT_SYMBOL_GPL(typec_port_unregister_pd);
+
 static const char * const typec_orientations[] = {
 	[TYPEC_ORIENTATION_NONE]	= "unknown",
 	[TYPEC_ORIENTATION_NORMAL]	= "normal",
diff --git a/drivers/usb/typec/class.h b/drivers/usb/typec/class.h
index 0f1bd6d19d67e..db1b6bbd4c779 100644
--- a/drivers/usb/typec/class.h
+++ b/drivers/usb/typec/class.h
@@ -33,6 +33,10 @@ struct typec_partner {
 	int				num_altmodes;
 	u16				pd_revision; /* 0300H = "3.0" */
 	enum usb_pd_svdm_ver		svdm_version;
+
+	struct pd			*pd;
+	struct pd_capabilities		*source_caps;
+	struct pd_capabilities		*sink_caps;
 };
 
 struct typec_port {
@@ -40,6 +44,10 @@ struct typec_port {
 	struct device			dev;
 	struct ida			mode_ids;
 
+	struct pd			*pd;
+	struct pd_capabilities		*source_caps;
+	struct pd_capabilities		*sink_caps;
+
 	int				prefer_role;
 	enum typec_data_role		data_role;
 	enum typec_role			pwr_role;
diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
index a7525b9931b69..a315802b50039 100644
--- a/include/linux/usb/typec.h
+++ b/include/linux/usb/typec.h
@@ -22,6 +22,10 @@ struct typec_altmode_ops;
 struct fwnode_handle;
 struct device;
 
+struct pd_capabilities;
+struct pd_desc;
+struct pd;
+
 enum typec_port_type {
 	TYPEC_PORT_SRC,
 	TYPEC_PORT_SNK,
@@ -231,6 +235,7 @@ struct typec_operations {
 	int (*vconn_set)(struct typec_port *port, enum typec_role role);
 	int (*port_type_set)(struct typec_port *port,
 			     enum typec_port_type type);
+	int (*capabilities_set)(struct typec_port *port, struct pd_capabilities *cap);
 };
 
 enum usb_pd_svdm_ver {
@@ -315,4 +320,14 @@ void typec_partner_set_svdm_version(struct typec_partner *partner,
 				    enum usb_pd_svdm_ver svdm_version);
 int typec_get_negotiated_svdm_version(struct typec_port *port);
 
+struct pd *typec_partner_register_pd(struct typec_partner *partner, struct pd_desc *desc);
+void typec_partner_unregister_pd(struct typec_partner *partner);
+int typec_partner_set_pd_capabilities(struct typec_partner *partner, struct pd_capabilities *cap);
+void typec_partner_unset_pd_capabilities(struct typec_partner *partner, enum typec_role role);
+
+struct pd *typec_port_register_pd(struct typec_port *port, struct pd_desc *desc);
+void typec_port_unregister_pd(struct typec_port *port);
+int typec_port_set_pd_capabilities(struct typec_port *port, struct pd_capabilities *cap);
+int typec_port_unset_pd_capabilities(struct typec_port *port, struct pd_capabilities *cap);
+
 #endif /* __LINUX_USB_TYPEC_H */
-- 
2.34.1

[PATCH v1 3/3] usb: typec: ucsi: Register USB Power Delivery Capabilities

[Heikki Krogerus]

UCSI allows the USB PD capabilities to be read with the
GET_PDO command. This will register those capabilities, and
that way make them visible to the user space.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/ucsi/ucsi.c | 128 +++++++++++++++++++++++++++++++---
 drivers/usb/typec/ucsi/ucsi.h |   8 +++
 2 files changed, 125 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index f0c2fa19f3e0f..5149001093c7f 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -568,8 +568,8 @@ static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8 recipient)
 	}
 }
 
-static int ucsi_get_pdos(struct ucsi_connector *con, int is_partner,
-			 u32 *pdos, int offset, int num_pdos)
+static int ucsi_read_pdos(struct ucsi_connector *con, enum typec_role role, int is_partner,
+			  u32 *pdos, int offset, int num_pdos)
 {
 	struct ucsi *ucsi = con->ucsi;
 	u64 command;
@@ -579,7 +579,7 @@ static int ucsi_get_pdos(struct ucsi_connector *con, int is_partner,
 	command |= UCSI_GET_PDOS_PARTNER_PDO(is_partner);
 	command |= UCSI_GET_PDOS_PDO_OFFSET(offset);
 	command |= UCSI_GET_PDOS_NUM_PDOS(num_pdos - 1);
-	command |= UCSI_GET_PDOS_SRC_PDOS;
+	command |= is_source(role) ? UCSI_GET_PDOS_SRC_PDOS : 0;
 	ret = ucsi_send_command(ucsi, command, pdos + offset,
 				num_pdos * sizeof(u32));
 	if (ret < 0 && ret != -ETIMEDOUT)
@@ -590,26 +590,39 @@ static int ucsi_get_pdos(struct ucsi_connector *con, int is_partner,
 	return ret;
 }
 
-static int ucsi_get_src_pdos(struct ucsi_connector *con)
+static int ucsi_get_pdos(struct ucsi_connector *con, enum typec_role role,
+			 int is_partner, u32 *pdos)
 {
+	u8 num_pdos;
 	int ret;
 
 	/* UCSI max payload means only getting at most 4 PDOs at a time */
-	ret = ucsi_get_pdos(con, 1, con->src_pdos, 0, UCSI_MAX_PDOS);
+	ret = ucsi_read_pdos(con, role, is_partner, pdos, 0, UCSI_MAX_PDOS);
 	if (ret < 0)
 		return ret;
 
-	con->num_pdos = ret / sizeof(u32); /* number of bytes to 32-bit PDOs */
-	if (con->num_pdos < UCSI_MAX_PDOS)
-		return 0;
+	num_pdos = ret / sizeof(u32); /* number of bytes to 32-bit PDOs */
+	if (num_pdos < UCSI_MAX_PDOS)
+		return num_pdos;
 
 	/* get the remaining PDOs, if any */
-	ret = ucsi_get_pdos(con, 1, con->src_pdos, UCSI_MAX_PDOS,
-			    PDO_MAX_OBJECTS - UCSI_MAX_PDOS);
+	ret = ucsi_read_pdos(con, role, is_partner, pdos, UCSI_MAX_PDOS,
+			     PDO_MAX_OBJECTS - UCSI_MAX_PDOS);
+	if (ret < 0)
+		return ret;
+
+	return ret / sizeof(u32) + num_pdos;
+}
+
+static int ucsi_get_src_pdos(struct ucsi_connector *con)
+{
+	int ret;
+
+	ret = ucsi_get_pdos(con, TYPEC_SOURCE, 1, con->src_pdos);
 	if (ret < 0)
 		return ret;
 
-	con->num_pdos += ret / sizeof(u32);
+	con->num_pdos += ret;
 
 	ucsi_port_psy_changed(con);
 
@@ -638,6 +651,60 @@ static int ucsi_check_altmodes(struct ucsi_connector *con)
 	return ret;
 }
 
+static int ucsi_register_partner_pdos(struct ucsi_connector *con)
+{
+	struct pd_desc desc = { con->ucsi->cap.pd_version };
+	struct pd_capabilities *cap;
+	struct pd_caps_desc caps;
+	int ret;
+
+	con->partner_pd = typec_partner_register_pd(con->partner, &desc);
+	if (IS_ERR(con->partner_pd))
+		return PTR_ERR(con->partner_pd);
+
+	ret = ucsi_get_pdos(con, TYPEC_SOURCE, 1, caps.pdo);
+	if (ret < 0)
+		return ret;
+
+	if (ret < PDO_MAX_OBJECTS)
+		caps.pdo[ret] = 0;
+	caps.role = TYPEC_SOURCE;
+
+	cap = pd_register_capabilities(con->partner_pd, &caps);
+	if (IS_ERR(cap))
+		return PTR_ERR(cap);
+
+	ret = typec_partner_set_pd_capabilities(con->partner, cap);
+	if (ret) {
+		pd_unregister_capabilities(cap);
+		return ret;
+	}
+
+	con->partner_source_caps = cap;
+
+	ret = ucsi_get_pdos(con, TYPEC_SINK, 1, caps.pdo);
+	if (ret <= 0)
+		return ret;
+
+	if (ret < PDO_MAX_OBJECTS)
+		caps.pdo[ret] = 0;
+	caps.role = TYPEC_SINK;
+
+	cap = pd_register_capabilities(con->partner_pd, &caps);
+	if (IS_ERR(cap))
+		return PTR_ERR(cap);
+
+	ret = typec_partner_set_pd_capabilities(con->partner, cap);
+	if (ret) {
+		pd_unregister_capabilities(cap);
+		return ret;
+	}
+
+	con->partner_sink_caps = cap;
+
+	return 0;
+}
+
 static void ucsi_pwr_opmode_change(struct ucsi_connector *con)
 {
 	switch (UCSI_CONSTAT_PWR_OPMODE(con->status.flags)) {
@@ -646,6 +713,7 @@ static void ucsi_pwr_opmode_change(struct ucsi_connector *con)
 		typec_set_pwr_opmode(con->port, TYPEC_PWR_MODE_PD);
 		ucsi_partner_task(con, ucsi_get_src_pdos, 30, 0);
 		ucsi_partner_task(con, ucsi_check_altmodes, 30, 0);
+		ucsi_partner_task(con, ucsi_register_partner_pdos, 1, HZ);
 		break;
 	case UCSI_CONSTAT_PWR_OPMODE_TYPEC1_5:
 		con->rdo = 0;
@@ -704,6 +772,17 @@ static void ucsi_unregister_partner(struct ucsi_connector *con)
 	if (!con->partner)
 		return;
 
+	typec_partner_unset_pd_capabilities(con->partner, TYPEC_SINK);
+	pd_unregister_capabilities(con->partner_sink_caps);
+	con->partner_sink_caps = NULL;
+
+	typec_partner_unset_pd_capabilities(con->partner, TYPEC_SOURCE);
+	pd_unregister_capabilities(con->partner_source_caps);
+	con->partner_source_caps = NULL;
+
+	typec_partner_unregister_pd(con->partner);
+	con->partner_pd = NULL;
+
 	ucsi_unregister_altmodes(con, UCSI_RECIPIENT_SOP);
 	typec_unregister_partner(con->partner);
 	con->partner = NULL;
@@ -1037,6 +1116,8 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
 	u64 command;
 	char *name;
 	int ret;
+	struct pd_desc desc = { ucsi->cap.pd_version };
+	struct pd_caps_desc caps;
 
 	name = kasprintf(GFP_KERNEL, "%s-con%d", dev_name(ucsi->dev), con->num);
 	if (!name)
@@ -1103,6 +1184,24 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
 		goto out;
 	}
 
+	con->pd = typec_port_register_pd(con->port, &desc);
+
+	ret = ucsi_get_pdos(con, TYPEC_SOURCE, 0, caps.pdo);
+	if (ret > 0) {
+		caps.pdo[ret] = 0;
+		caps.role = TYPEC_SOURCE;
+		con->source_caps = pd_register_capabilities(con->pd, &caps);
+		typec_port_set_pd_capabilities(con->port, con->source_caps);
+	}
+
+	ret = ucsi_get_pdos(con, TYPEC_SINK, 0, caps.pdo);
+	if (ret > 0) {
+		caps.pdo[ret] = 0;
+		caps.role = TYPEC_SINK;
+		con->sink_caps = pd_register_capabilities(con->pd, &caps);
+		typec_port_set_pd_capabilities(con->port, con->sink_caps);
+	}
+
 	/* Alternate modes */
 	ret = ucsi_register_altmodes(con, UCSI_RECIPIENT_CON);
 	if (ret) {
@@ -1169,6 +1268,7 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
 	    UCSI_CONSTAT_PWR_OPMODE_PD) {
 		ucsi_get_src_pdos(con);
 		ucsi_check_altmodes(con);
+		ucsi_register_partner_pdos(con);
 	}
 
 	trace_ucsi_register_port(con->num, &con->status);
@@ -1379,6 +1479,12 @@ void ucsi_unregister(struct ucsi *ucsi)
 		ucsi_unregister_port_psy(&ucsi->connector[i]);
 		if (ucsi->connector[i].wq)
 			destroy_workqueue(ucsi->connector[i].wq);
+		typec_port_unset_pd_capabilities(ucsi->connector[i].port,
+						 ucsi->connector[i].source_caps);
+		pd_unregister_capabilities(ucsi->connector[i].source_caps);
+		typec_port_unset_pd_capabilities(ucsi->connector[i].port,
+						 ucsi->connector[i].sink_caps);
+		pd_unregister_capabilities(ucsi->connector[i].sink_caps);
 		typec_unregister_port(ucsi->connector[i].port);
 	}
 
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index 280f1e1bda2c9..aa23df98b7730 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -335,6 +335,14 @@ struct ucsi_connector {
 	int num_pdos;
 
 	struct usb_role_switch *usb_role_sw;
+
+	struct pd *pd;
+	struct pd_capabilities *source_caps;
+	struct pd_capabilities *sink_caps;
+
+	struct pd *partner_pd;
+	struct pd_capabilities *partner_source_caps;
+	struct pd_capabilities *partner_sink_caps;
 };
 
 int ucsi_send_command(struct ucsi *ucsi, u64 command,
-- 
2.34.1

Re: [PATCH v1 3/3] usb: typec: ucsi: Register USB Power Delivery Capabilities

[Jack Pham]

Hi Heikki,

On Thu, Feb 03, 2022 at 05:46:57PM +0300, Heikki Krogerus wrote:
> UCSI allows the USB PD capabilities to be read with the
> GET_PDO command. This will register those capabilities, and
> that way make them visible to the user space.
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>  drivers/usb/typec/ucsi/ucsi.c | 128 +++++++++++++++++++++++++++++++---
>  drivers/usb/typec/ucsi/ucsi.h |   8 +++
>  2 files changed, 125 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index f0c2fa19f3e0f..5149001093c7f 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -568,8 +568,8 @@ static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8 recipient)
>  	}
>  }
>  
> -static int ucsi_get_pdos(struct ucsi_connector *con, int is_partner,
> -			 u32 *pdos, int offset, int num_pdos)
> +static int ucsi_read_pdos(struct ucsi_connector *con, enum typec_role role, int is_partner,
> +			  u32 *pdos, int offset, int num_pdos)
>  {
>  	struct ucsi *ucsi = con->ucsi;
>  	u64 command;
> @@ -579,7 +579,7 @@ static int ucsi_get_pdos(struct ucsi_connector *con, int is_partner,
>  	command |= UCSI_GET_PDOS_PARTNER_PDO(is_partner);
>  	command |= UCSI_GET_PDOS_PDO_OFFSET(offset);
>  	command |= UCSI_GET_PDOS_NUM_PDOS(num_pdos - 1);
> -	command |= UCSI_GET_PDOS_SRC_PDOS;
> +	command |= is_source(role) ? UCSI_GET_PDOS_SRC_PDOS : 0;
>  	ret = ucsi_send_command(ucsi, command, pdos + offset,
>  				num_pdos * sizeof(u32));
>  	if (ret < 0 && ret != -ETIMEDOUT)
> @@ -590,26 +590,39 @@ static int ucsi_get_pdos(struct ucsi_connector *con, int is_partner,
>  	return ret;
>  }
>  
> -static int ucsi_get_src_pdos(struct ucsi_connector *con)
> +static int ucsi_get_pdos(struct ucsi_connector *con, enum typec_role role,
> +			 int is_partner, u32 *pdos)
>  {
> +	u8 num_pdos;
>  	int ret;
>  
>  	/* UCSI max payload means only getting at most 4 PDOs at a time */
> -	ret = ucsi_get_pdos(con, 1, con->src_pdos, 0, UCSI_MAX_PDOS);
> +	ret = ucsi_read_pdos(con, role, is_partner, pdos, 0, UCSI_MAX_PDOS);
>  	if (ret < 0)
>  		return ret;
>  
> -	con->num_pdos = ret / sizeof(u32); /* number of bytes to 32-bit PDOs */
> -	if (con->num_pdos < UCSI_MAX_PDOS)
> -		return 0;
> +	num_pdos = ret / sizeof(u32); /* number of bytes to 32-bit PDOs */
> +	if (num_pdos < UCSI_MAX_PDOS)
> +		return num_pdos;
>  
>  	/* get the remaining PDOs, if any */
> -	ret = ucsi_get_pdos(con, 1, con->src_pdos, UCSI_MAX_PDOS,
> -			    PDO_MAX_OBJECTS - UCSI_MAX_PDOS);
> +	ret = ucsi_read_pdos(con, role, is_partner, pdos, UCSI_MAX_PDOS,
> +			     PDO_MAX_OBJECTS - UCSI_MAX_PDOS);
> +	if (ret < 0)
> +		return ret;
> +
> +	return ret / sizeof(u32) + num_pdos;
> +}
> +
> +static int ucsi_get_src_pdos(struct ucsi_connector *con)
> +{
> +	int ret;
> +
> +	ret = ucsi_get_pdos(con, TYPEC_SOURCE, 1, con->src_pdos);

This issues the GET_PDOS command to PPM to get the source PDOs of the
partner...

>  	if (ret < 0)
>  		return ret;
>  
> -	con->num_pdos += ret / sizeof(u32);
> +	con->num_pdos += ret;
>  
>  	ucsi_port_psy_changed(con);
>  
> @@ -638,6 +651,60 @@ static int ucsi_check_altmodes(struct ucsi_connector *con)
>  	return ret;
>  }
>  
> +static int ucsi_register_partner_pdos(struct ucsi_connector *con)
> +{
> +	struct pd_desc desc = { con->ucsi->cap.pd_version };
> +	struct pd_capabilities *cap;
> +	struct pd_caps_desc caps;
> +	int ret;
> +
> +	con->partner_pd = typec_partner_register_pd(con->partner, &desc);
> +	if (IS_ERR(con->partner_pd))
> +		return PTR_ERR(con->partner_pd);
> +
> +	ret = ucsi_get_pdos(con, TYPEC_SOURCE, 1, caps.pdo);

... and also here.

> +	if (ret < 0)
> +		return ret;
> +
> +	if (ret < PDO_MAX_OBJECTS)
> +		caps.pdo[ret] = 0;
> +	caps.role = TYPEC_SOURCE;
> +
> +	cap = pd_register_capabilities(con->partner_pd, &caps);
> +	if (IS_ERR(cap))
> +		return PTR_ERR(cap);
> +
> +	ret = typec_partner_set_pd_capabilities(con->partner, cap);
> +	if (ret) {
> +		pd_unregister_capabilities(cap);
> +		return ret;
> +	}
> +
> +	con->partner_source_caps = cap;
> +
> +	ret = ucsi_get_pdos(con, TYPEC_SINK, 1, caps.pdo);
> +	if (ret <= 0)
> +		return ret;
> +
> +	if (ret < PDO_MAX_OBJECTS)
> +		caps.pdo[ret] = 0;
> +	caps.role = TYPEC_SINK;
> +
> +	cap = pd_register_capabilities(con->partner_pd, &caps);
> +	if (IS_ERR(cap))
> +		return PTR_ERR(cap);
> +
> +	ret = typec_partner_set_pd_capabilities(con->partner, cap);
> +	if (ret) {
> +		pd_unregister_capabilities(cap);
> +		return ret;
> +	}
> +
> +	con->partner_sink_caps = cap;
> +
> +	return 0;
> +}
> +
>  static void ucsi_pwr_opmode_change(struct ucsi_connector *con)
>  {
>  	switch (UCSI_CONSTAT_PWR_OPMODE(con->status.flags)) {
> @@ -646,6 +713,7 @@ static void ucsi_pwr_opmode_change(struct ucsi_connector *con)
>  		typec_set_pwr_opmode(con->port, TYPEC_PWR_MODE_PD);
>  		ucsi_partner_task(con, ucsi_get_src_pdos, 30, 0);
>  		ucsi_partner_task(con, ucsi_check_altmodes, 30, 0);
> +		ucsi_partner_task(con, ucsi_register_partner_pdos, 1, HZ);

And, both ucsi_get_src_pdos() and ucsi_register_partner_pdos() are
scheduled to run here...

>  		break;
>  	case UCSI_CONSTAT_PWR_OPMODE_TYPEC1_5:
>  		con->rdo = 0;
> @@ -704,6 +772,17 @@ static void ucsi_unregister_partner(struct ucsi_connector *con)
>  	if (!con->partner)
>  		return;
>  
> +	typec_partner_unset_pd_capabilities(con->partner, TYPEC_SINK);
> +	pd_unregister_capabilities(con->partner_sink_caps);
> +	con->partner_sink_caps = NULL;
> +
> +	typec_partner_unset_pd_capabilities(con->partner, TYPEC_SOURCE);
> +	pd_unregister_capabilities(con->partner_source_caps);
> +	con->partner_source_caps = NULL;
> +
> +	typec_partner_unregister_pd(con->partner);
> +	con->partner_pd = NULL;
> +
>  	ucsi_unregister_altmodes(con, UCSI_RECIPIENT_SOP);
>  	typec_unregister_partner(con->partner);
>  	con->partner = NULL;
> @@ -1037,6 +1116,8 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
>  	u64 command;
>  	char *name;
>  	int ret;
> +	struct pd_desc desc = { ucsi->cap.pd_version };
> +	struct pd_caps_desc caps;
>  
>  	name = kasprintf(GFP_KERNEL, "%s-con%d", dev_name(ucsi->dev), con->num);
>  	if (!name)
> @@ -1103,6 +1184,24 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
>  		goto out;
>  	}
>  
> +	con->pd = typec_port_register_pd(con->port, &desc);
> +
> +	ret = ucsi_get_pdos(con, TYPEC_SOURCE, 0, caps.pdo);
> +	if (ret > 0) {
> +		caps.pdo[ret] = 0;
> +		caps.role = TYPEC_SOURCE;
> +		con->source_caps = pd_register_capabilities(con->pd, &caps);
> +		typec_port_set_pd_capabilities(con->port, con->source_caps);
> +	}
> +
> +	ret = ucsi_get_pdos(con, TYPEC_SINK, 0, caps.pdo);
> +	if (ret > 0) {
> +		caps.pdo[ret] = 0;
> +		caps.role = TYPEC_SINK;
> +		con->sink_caps = pd_register_capabilities(con->pd, &caps);
> +		typec_port_set_pd_capabilities(con->port, con->sink_caps);
> +	}
> +
>  	/* Alternate modes */
>  	ret = ucsi_register_altmodes(con, UCSI_RECIPIENT_CON);
>  	if (ret) {
> @@ -1169,6 +1268,7 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
>  	    UCSI_CONSTAT_PWR_OPMODE_PD) {
>  		ucsi_get_src_pdos(con);
>  		ucsi_check_altmodes(con);
> +		ucsi_register_partner_pdos(con);

... as well as here.

So wouldn't this result in the PPM issuing the same PD Get_Source_Cap
message twice to the port partner (in either case of initial port
registration or op mode change)?  Could we just consolidate them to just
issue GET_PDOS only once and take care of populating the partner's
Source Caps for both the pd_capabilties as well as power_supply purposes
from a single helper?

Another aside, thinking back to a previous patch [1] I had proposed a
few months ago, another question I had is whether it is proper to even
issue a Get_Source_Cap message to sink-only devices, as we did encounter
certain DisplayPort adapters that don't like when that happens.
Wondering if it could be possible to limit calling the GET_PDOS command
unless we know the partner is capable of operating in that particular
power role.  e.g. don't call get_src_pdos() if partner is sink-only.
Or is this the kind of thing that the PPM is supposed to be able to
figure out and allow OPM to naively issue the command regardless, and
just get an error/empty return?

[1] https://lore.kernel.org/all/20211027064842.6901-1-quic_jackp@quicinc.com/

Thanks,
Jack

Re: [PATCH v1 3/3] usb: typec: ucsi: Register USB Power Delivery Capabilities

[Heikki Krogerus]

Hi,

On Wed, Feb 09, 2022 at 11:56:11PM -0800, Jack Pham wrote:
> Hi Heikki,
> 
> On Thu, Feb 03, 2022 at 05:46:57PM +0300, Heikki Krogerus wrote:
> > UCSI allows the USB PD capabilities to be read with the
> > GET_PDO command. This will register those capabilities, and
> > that way make them visible to the user space.
> > 
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > ---
> >  drivers/usb/typec/ucsi/ucsi.c | 128 +++++++++++++++++++++++++++++++---
> >  drivers/usb/typec/ucsi/ucsi.h |   8 +++
> >  2 files changed, 125 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> > index f0c2fa19f3e0f..5149001093c7f 100644
> > --- a/drivers/usb/typec/ucsi/ucsi.c
> > +++ b/drivers/usb/typec/ucsi/ucsi.c
> > @@ -568,8 +568,8 @@ static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8 recipient)
> >  	}
> >  }
> >  
> > -static int ucsi_get_pdos(struct ucsi_connector *con, int is_partner,
> > -			 u32 *pdos, int offset, int num_pdos)
> > +static int ucsi_read_pdos(struct ucsi_connector *con, enum typec_role role, int is_partner,
> > +			  u32 *pdos, int offset, int num_pdos)
> >  {
> >  	struct ucsi *ucsi = con->ucsi;
> >  	u64 command;
> > @@ -579,7 +579,7 @@ static int ucsi_get_pdos(struct ucsi_connector *con, int is_partner,
> >  	command |= UCSI_GET_PDOS_PARTNER_PDO(is_partner);
> >  	command |= UCSI_GET_PDOS_PDO_OFFSET(offset);
> >  	command |= UCSI_GET_PDOS_NUM_PDOS(num_pdos - 1);
> > -	command |= UCSI_GET_PDOS_SRC_PDOS;
> > +	command |= is_source(role) ? UCSI_GET_PDOS_SRC_PDOS : 0;
> >  	ret = ucsi_send_command(ucsi, command, pdos + offset,
> >  				num_pdos * sizeof(u32));
> >  	if (ret < 0 && ret != -ETIMEDOUT)
> > @@ -590,26 +590,39 @@ static int ucsi_get_pdos(struct ucsi_connector *con, int is_partner,
> >  	return ret;
> >  }
> >  
> > -static int ucsi_get_src_pdos(struct ucsi_connector *con)
> > +static int ucsi_get_pdos(struct ucsi_connector *con, enum typec_role role,
> > +			 int is_partner, u32 *pdos)
> >  {
> > +	u8 num_pdos;
> >  	int ret;
> >  
> >  	/* UCSI max payload means only getting at most 4 PDOs at a time */
> > -	ret = ucsi_get_pdos(con, 1, con->src_pdos, 0, UCSI_MAX_PDOS);
> > +	ret = ucsi_read_pdos(con, role, is_partner, pdos, 0, UCSI_MAX_PDOS);
> >  	if (ret < 0)
> >  		return ret;
> >  
> > -	con->num_pdos = ret / sizeof(u32); /* number of bytes to 32-bit PDOs */
> > -	if (con->num_pdos < UCSI_MAX_PDOS)
> > -		return 0;
> > +	num_pdos = ret / sizeof(u32); /* number of bytes to 32-bit PDOs */
> > +	if (num_pdos < UCSI_MAX_PDOS)
> > +		return num_pdos;
> >  
> >  	/* get the remaining PDOs, if any */
> > -	ret = ucsi_get_pdos(con, 1, con->src_pdos, UCSI_MAX_PDOS,
> > -			    PDO_MAX_OBJECTS - UCSI_MAX_PDOS);
> > +	ret = ucsi_read_pdos(con, role, is_partner, pdos, UCSI_MAX_PDOS,
> > +			     PDO_MAX_OBJECTS - UCSI_MAX_PDOS);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return ret / sizeof(u32) + num_pdos;
> > +}
> > +
> > +static int ucsi_get_src_pdos(struct ucsi_connector *con)
> > +{
> > +	int ret;
> > +
> > +	ret = ucsi_get_pdos(con, TYPEC_SOURCE, 1, con->src_pdos);
> 
> This issues the GET_PDOS command to PPM to get the source PDOs of the
> partner...
> 
> >  	if (ret < 0)
> >  		return ret;
> >  
> > -	con->num_pdos += ret / sizeof(u32);
> > +	con->num_pdos += ret;
> >  
> >  	ucsi_port_psy_changed(con);
> >  
> > @@ -638,6 +651,60 @@ static int ucsi_check_altmodes(struct ucsi_connector *con)
> >  	return ret;
> >  }
> >  
> > +static int ucsi_register_partner_pdos(struct ucsi_connector *con)
> > +{
> > +	struct pd_desc desc = { con->ucsi->cap.pd_version };
> > +	struct pd_capabilities *cap;
> > +	struct pd_caps_desc caps;
> > +	int ret;
> > +
> > +	con->partner_pd = typec_partner_register_pd(con->partner, &desc);
> > +	if (IS_ERR(con->partner_pd))
> > +		return PTR_ERR(con->partner_pd);
> > +
> > +	ret = ucsi_get_pdos(con, TYPEC_SOURCE, 1, caps.pdo);
> 
> ... and also here.
> 
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (ret < PDO_MAX_OBJECTS)
> > +		caps.pdo[ret] = 0;
> > +	caps.role = TYPEC_SOURCE;
> > +
> > +	cap = pd_register_capabilities(con->partner_pd, &caps);
> > +	if (IS_ERR(cap))
> > +		return PTR_ERR(cap);
> > +
> > +	ret = typec_partner_set_pd_capabilities(con->partner, cap);
> > +	if (ret) {
> > +		pd_unregister_capabilities(cap);
> > +		return ret;
> > +	}
> > +
> > +	con->partner_source_caps = cap;
> > +
> > +	ret = ucsi_get_pdos(con, TYPEC_SINK, 1, caps.pdo);
> > +	if (ret <= 0)
> > +		return ret;
> > +
> > +	if (ret < PDO_MAX_OBJECTS)
> > +		caps.pdo[ret] = 0;
> > +	caps.role = TYPEC_SINK;
> > +
> > +	cap = pd_register_capabilities(con->partner_pd, &caps);
> > +	if (IS_ERR(cap))
> > +		return PTR_ERR(cap);
> > +
> > +	ret = typec_partner_set_pd_capabilities(con->partner, cap);
> > +	if (ret) {
> > +		pd_unregister_capabilities(cap);
> > +		return ret;
> > +	}
> > +
> > +	con->partner_sink_caps = cap;
> > +
> > +	return 0;
> > +}
> > +
> >  static void ucsi_pwr_opmode_change(struct ucsi_connector *con)
> >  {
> >  	switch (UCSI_CONSTAT_PWR_OPMODE(con->status.flags)) {
> > @@ -646,6 +713,7 @@ static void ucsi_pwr_opmode_change(struct ucsi_connector *con)
> >  		typec_set_pwr_opmode(con->port, TYPEC_PWR_MODE_PD);
> >  		ucsi_partner_task(con, ucsi_get_src_pdos, 30, 0);
> >  		ucsi_partner_task(con, ucsi_check_altmodes, 30, 0);
> > +		ucsi_partner_task(con, ucsi_register_partner_pdos, 1, HZ);
> 
> And, both ucsi_get_src_pdos() and ucsi_register_partner_pdos() are
> scheduled to run here...
> 
> >  		break;
> >  	case UCSI_CONSTAT_PWR_OPMODE_TYPEC1_5:
> >  		con->rdo = 0;
> > @@ -704,6 +772,17 @@ static void ucsi_unregister_partner(struct ucsi_connector *con)
> >  	if (!con->partner)
> >  		return;
> >  
> > +	typec_partner_unset_pd_capabilities(con->partner, TYPEC_SINK);
> > +	pd_unregister_capabilities(con->partner_sink_caps);
> > +	con->partner_sink_caps = NULL;
> > +
> > +	typec_partner_unset_pd_capabilities(con->partner, TYPEC_SOURCE);
> > +	pd_unregister_capabilities(con->partner_source_caps);
> > +	con->partner_source_caps = NULL;
> > +
> > +	typec_partner_unregister_pd(con->partner);
> > +	con->partner_pd = NULL;
> > +
> >  	ucsi_unregister_altmodes(con, UCSI_RECIPIENT_SOP);
> >  	typec_unregister_partner(con->partner);
> >  	con->partner = NULL;
> > @@ -1037,6 +1116,8 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
> >  	u64 command;
> >  	char *name;
> >  	int ret;
> > +	struct pd_desc desc = { ucsi->cap.pd_version };
> > +	struct pd_caps_desc caps;
> >  
> >  	name = kasprintf(GFP_KERNEL, "%s-con%d", dev_name(ucsi->dev), con->num);
> >  	if (!name)
> > @@ -1103,6 +1184,24 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
> >  		goto out;
> >  	}
> >  
> > +	con->pd = typec_port_register_pd(con->port, &desc);
> > +
> > +	ret = ucsi_get_pdos(con, TYPEC_SOURCE, 0, caps.pdo);
> > +	if (ret > 0) {
> > +		caps.pdo[ret] = 0;
> > +		caps.role = TYPEC_SOURCE;
> > +		con->source_caps = pd_register_capabilities(con->pd, &caps);
> > +		typec_port_set_pd_capabilities(con->port, con->source_caps);
> > +	}
> > +
> > +	ret = ucsi_get_pdos(con, TYPEC_SINK, 0, caps.pdo);
> > +	if (ret > 0) {
> > +		caps.pdo[ret] = 0;
> > +		caps.role = TYPEC_SINK;
> > +		con->sink_caps = pd_register_capabilities(con->pd, &caps);
> > +		typec_port_set_pd_capabilities(con->port, con->sink_caps);
> > +	}
> > +
> >  	/* Alternate modes */
> >  	ret = ucsi_register_altmodes(con, UCSI_RECIPIENT_CON);
> >  	if (ret) {
> > @@ -1169,6 +1268,7 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
> >  	    UCSI_CONSTAT_PWR_OPMODE_PD) {
> >  		ucsi_get_src_pdos(con);
> >  		ucsi_check_altmodes(con);
> > +		ucsi_register_partner_pdos(con);
> 
> ... as well as here.
> 
> So wouldn't this result in the PPM issuing the same PD Get_Source_Cap
> message twice to the port partner (in either case of initial port
> registration or op mode change)?  Could we just consolidate them to just
> issue GET_PDOS only once and take care of populating the partner's
> Source Caps for both the pd_capabilties as well as power_supply purposes
> from a single helper?

Yes. I tried create as little impact on existing code with this as
possible, so I just duplicated that part, but I probable should not
have done that.

> Another aside, thinking back to a previous patch [1] I had proposed a
> few months ago, another question I had is whether it is proper to even
> issue a Get_Source_Cap message to sink-only devices, as we did encounter
> certain DisplayPort adapters that don't like when that happens.
> Wondering if it could be possible to limit calling the GET_PDOS command
> unless we know the partner is capable of operating in that particular
> power role.  e.g. don't call get_src_pdos() if partner is sink-only.
> Or is this the kind of thing that the PPM is supposed to be able to
> figure out and allow OPM to naively issue the command regardless, and
> just get an error/empty return?

Well, in many cases we simply can not rely on the PPM, but I think we
should be able rely on it with the role.

What you are proposing probable would make sense. Do you want to work
on those improvements? I don't have to modify this driver now with
this series. I also can first register the sysfs attributes from
tcpm.c.

thanks,

-- 
heikki

Re: [PATCH v1 3/3] usb: typec: ucsi: Register USB Power Delivery Capabilities

[Jack Pham]

On Tue, Feb 15, 2022 at 02:33:38PM +0200, Heikki Krogerus wrote:
> Hi,
> 
> On Wed, Feb 09, 2022 at 11:56:11PM -0800, Jack Pham wrote:
> > Hi Heikki,
> > 
> > On Thu, Feb 03, 2022 at 05:46:57PM +0300, Heikki Krogerus wrote:
> > > UCSI allows the USB PD capabilities to be read with the
> > > GET_PDO command. This will register those capabilities, and
> > > that way make them visible to the user space.
> > > 
> > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > ---
> > >  drivers/usb/typec/ucsi/ucsi.c | 128 +++++++++++++++++++++++++++++++---
> > >  drivers/usb/typec/ucsi/ucsi.h |   8 +++
> > >  2 files changed, 125 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> > > index f0c2fa19f3e0f..5149001093c7f 100644
> > > --- a/drivers/usb/typec/ucsi/ucsi.c
> > > +++ b/drivers/usb/typec/ucsi/ucsi.c
> > > @@ -568,8 +568,8 @@ static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8 recipient)
> > >  	}
> > >  }
> > >  
> > > -static int ucsi_get_pdos(struct ucsi_connector *con, int is_partner,
> > > -			 u32 *pdos, int offset, int num_pdos)
> > > +static int ucsi_read_pdos(struct ucsi_connector *con, enum typec_role role, int is_partner,
> > > +			  u32 *pdos, int offset, int num_pdos)
> > >  {
> > >  	struct ucsi *ucsi = con->ucsi;
> > >  	u64 command;
> > > @@ -579,7 +579,7 @@ static int ucsi_get_pdos(struct ucsi_connector *con, int is_partner,
> > >  	command |= UCSI_GET_PDOS_PARTNER_PDO(is_partner);
> > >  	command |= UCSI_GET_PDOS_PDO_OFFSET(offset);
> > >  	command |= UCSI_GET_PDOS_NUM_PDOS(num_pdos - 1);
> > > -	command |= UCSI_GET_PDOS_SRC_PDOS;
> > > +	command |= is_source(role) ? UCSI_GET_PDOS_SRC_PDOS : 0;
> > >  	ret = ucsi_send_command(ucsi, command, pdos + offset,
> > >  				num_pdos * sizeof(u32));
> > >  	if (ret < 0 && ret != -ETIMEDOUT)
> > > @@ -590,26 +590,39 @@ static int ucsi_get_pdos(struct ucsi_connector *con, int is_partner,
> > >  	return ret;
> > >  }
> > >  
> > > -static int ucsi_get_src_pdos(struct ucsi_connector *con)
> > > +static int ucsi_get_pdos(struct ucsi_connector *con, enum typec_role role,
> > > +			 int is_partner, u32 *pdos)
> > >  {
> > > +	u8 num_pdos;
> > >  	int ret;
> > >  
> > >  	/* UCSI max payload means only getting at most 4 PDOs at a time */
> > > -	ret = ucsi_get_pdos(con, 1, con->src_pdos, 0, UCSI_MAX_PDOS);
> > > +	ret = ucsi_read_pdos(con, role, is_partner, pdos, 0, UCSI_MAX_PDOS);
> > >  	if (ret < 0)
> > >  		return ret;
> > >  
> > > -	con->num_pdos = ret / sizeof(u32); /* number of bytes to 32-bit PDOs */
> > > -	if (con->num_pdos < UCSI_MAX_PDOS)
> > > -		return 0;
> > > +	num_pdos = ret / sizeof(u32); /* number of bytes to 32-bit PDOs */
> > > +	if (num_pdos < UCSI_MAX_PDOS)
> > > +		return num_pdos;
> > >  
> > >  	/* get the remaining PDOs, if any */
> > > -	ret = ucsi_get_pdos(con, 1, con->src_pdos, UCSI_MAX_PDOS,
> > > -			    PDO_MAX_OBJECTS - UCSI_MAX_PDOS);
> > > +	ret = ucsi_read_pdos(con, role, is_partner, pdos, UCSI_MAX_PDOS,
> > > +			     PDO_MAX_OBJECTS - UCSI_MAX_PDOS);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	return ret / sizeof(u32) + num_pdos;
> > > +}
> > > +
> > > +static int ucsi_get_src_pdos(struct ucsi_connector *con)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = ucsi_get_pdos(con, TYPEC_SOURCE, 1, con->src_pdos);
> > 
> > This issues the GET_PDOS command to PPM to get the source PDOs of the
> > partner...
> > 
> > >  	if (ret < 0)
> > >  		return ret;
> > >  
> > > -	con->num_pdos += ret / sizeof(u32);
> > > +	con->num_pdos += ret;
> > >  
> > >  	ucsi_port_psy_changed(con);
> > >  
> > > @@ -638,6 +651,60 @@ static int ucsi_check_altmodes(struct ucsi_connector *con)
> > >  	return ret;
> > >  }
> > >  
> > > +static int ucsi_register_partner_pdos(struct ucsi_connector *con)
> > > +{
> > > +	struct pd_desc desc = { con->ucsi->cap.pd_version };
> > > +	struct pd_capabilities *cap;
> > > +	struct pd_caps_desc caps;
> > > +	int ret;
> > > +
> > > +	con->partner_pd = typec_partner_register_pd(con->partner, &desc);
> > > +	if (IS_ERR(con->partner_pd))
> > > +		return PTR_ERR(con->partner_pd);
> > > +
> > > +	ret = ucsi_get_pdos(con, TYPEC_SOURCE, 1, caps.pdo);
> > 
> > ... and also here.
> > 
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	if (ret < PDO_MAX_OBJECTS)
> > > +		caps.pdo[ret] = 0;
> > > +	caps.role = TYPEC_SOURCE;
> > > +
> > > +	cap = pd_register_capabilities(con->partner_pd, &caps);
> > > +	if (IS_ERR(cap))
> > > +		return PTR_ERR(cap);
> > > +
> > > +	ret = typec_partner_set_pd_capabilities(con->partner, cap);
> > > +	if (ret) {
> > > +		pd_unregister_capabilities(cap);
> > > +		return ret;
> > > +	}
> > > +
> > > +	con->partner_source_caps = cap;
> > > +
> > > +	ret = ucsi_get_pdos(con, TYPEC_SINK, 1, caps.pdo);
> > > +	if (ret <= 0)
> > > +		return ret;
> > > +
> > > +	if (ret < PDO_MAX_OBJECTS)
> > > +		caps.pdo[ret] = 0;
> > > +	caps.role = TYPEC_SINK;
> > > +
> > > +	cap = pd_register_capabilities(con->partner_pd, &caps);
> > > +	if (IS_ERR(cap))
> > > +		return PTR_ERR(cap);
> > > +
> > > +	ret = typec_partner_set_pd_capabilities(con->partner, cap);
> > > +	if (ret) {
> > > +		pd_unregister_capabilities(cap);
> > > +		return ret;
> > > +	}
> > > +
> > > +	con->partner_sink_caps = cap;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static void ucsi_pwr_opmode_change(struct ucsi_connector *con)
> > >  {
> > >  	switch (UCSI_CONSTAT_PWR_OPMODE(con->status.flags)) {
> > > @@ -646,6 +713,7 @@ static void ucsi_pwr_opmode_change(struct ucsi_connector *con)
> > >  		typec_set_pwr_opmode(con->port, TYPEC_PWR_MODE_PD);
> > >  		ucsi_partner_task(con, ucsi_get_src_pdos, 30, 0);
> > >  		ucsi_partner_task(con, ucsi_check_altmodes, 30, 0);
> > > +		ucsi_partner_task(con, ucsi_register_partner_pdos, 1, HZ);
> > 
> > And, both ucsi_get_src_pdos() and ucsi_register_partner_pdos() are
> > scheduled to run here...
> > 
> > >  		break;
> > >  	case UCSI_CONSTAT_PWR_OPMODE_TYPEC1_5:
> > >  		con->rdo = 0;
> > > @@ -704,6 +772,17 @@ static void ucsi_unregister_partner(struct ucsi_connector *con)
> > >  	if (!con->partner)
> > >  		return;
> > >  
> > > +	typec_partner_unset_pd_capabilities(con->partner, TYPEC_SINK);
> > > +	pd_unregister_capabilities(con->partner_sink_caps);
> > > +	con->partner_sink_caps = NULL;
> > > +
> > > +	typec_partner_unset_pd_capabilities(con->partner, TYPEC_SOURCE);
> > > +	pd_unregister_capabilities(con->partner_source_caps);
> > > +	con->partner_source_caps = NULL;
> > > +
> > > +	typec_partner_unregister_pd(con->partner);
> > > +	con->partner_pd = NULL;
> > > +
> > >  	ucsi_unregister_altmodes(con, UCSI_RECIPIENT_SOP);
> > >  	typec_unregister_partner(con->partner);
> > >  	con->partner = NULL;
> > > @@ -1037,6 +1116,8 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
> > >  	u64 command;
> > >  	char *name;
> > >  	int ret;
> > > +	struct pd_desc desc = { ucsi->cap.pd_version };
> > > +	struct pd_caps_desc caps;
> > >  
> > >  	name = kasprintf(GFP_KERNEL, "%s-con%d", dev_name(ucsi->dev), con->num);
> > >  	if (!name)
> > > @@ -1103,6 +1184,24 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
> > >  		goto out;
> > >  	}
> > >  
> > > +	con->pd = typec_port_register_pd(con->port, &desc);
> > > +
> > > +	ret = ucsi_get_pdos(con, TYPEC_SOURCE, 0, caps.pdo);
> > > +	if (ret > 0) {
> > > +		caps.pdo[ret] = 0;
> > > +		caps.role = TYPEC_SOURCE;
> > > +		con->source_caps = pd_register_capabilities(con->pd, &caps);
> > > +		typec_port_set_pd_capabilities(con->port, con->source_caps);
> > > +	}
> > > +
> > > +	ret = ucsi_get_pdos(con, TYPEC_SINK, 0, caps.pdo);
> > > +	if (ret > 0) {
> > > +		caps.pdo[ret] = 0;
> > > +		caps.role = TYPEC_SINK;
> > > +		con->sink_caps = pd_register_capabilities(con->pd, &caps);
> > > +		typec_port_set_pd_capabilities(con->port, con->sink_caps);
> > > +	}
> > > +
> > >  	/* Alternate modes */
> > >  	ret = ucsi_register_altmodes(con, UCSI_RECIPIENT_CON);
> > >  	if (ret) {
> > > @@ -1169,6 +1268,7 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
> > >  	    UCSI_CONSTAT_PWR_OPMODE_PD) {
> > >  		ucsi_get_src_pdos(con);
> > >  		ucsi_check_altmodes(con);
> > > +		ucsi_register_partner_pdos(con);
> > 
> > ... as well as here.
> > 
> > So wouldn't this result in the PPM issuing the same PD Get_Source_Cap
> > message twice to the port partner (in either case of initial port
> > registration or op mode change)?  Could we just consolidate them to just
> > issue GET_PDOS only once and take care of populating the partner's
> > Source Caps for both the pd_capabilties as well as power_supply purposes
> > from a single helper?
> 
> Yes. I tried create as little impact on existing code with this as
> possible, so I just duplicated that part, but I probable should not
> have done that.
> 
> > Another aside, thinking back to a previous patch [1] I had proposed a
> > few months ago, another question I had is whether it is proper to even
> > issue a Get_Source_Cap message to sink-only devices, as we did encounter
> > certain DisplayPort adapters that don't like when that happens.
> > Wondering if it could be possible to limit calling the GET_PDOS command
> > unless we know the partner is capable of operating in that particular
> > power role.  e.g. don't call get_src_pdos() if partner is sink-only.
> > Or is this the kind of thing that the PPM is supposed to be able to
> > figure out and allow OPM to naively issue the command regardless, and
> > just get an error/empty return?
> 
> Well, in many cases we simply can not rely on the PPM, but I think we
> should be able rely on it with the role.
> 
> What you are proposing probable would make sense. Do you want to work
> on those improvements? I don't have to modify this driver now with
> this series. I also can first register the sysfs attributes from
> tcpm.c.

Sure Heikki.  I'll revise the earlier patch I had sent previously and
update it to determine whether the partner is source-capable before
before allowing ucsi_get_src_pdos() to be called.

Thanks,
Jack