On Mon, Feb 28, 2022 at 07:06:49PM +0000, Won Chung wrote:
> When ACPI table includes _PLD field for a Type C connector, share _PLD
> values in its sysfs. _PLD stands for physical location of device.
>
> Currently without connector's location information, when there are
> multiple Type C ports, it is hard to distinguish which connector
> corresponds to which physical port at which location. For example, when
> there are two Type C connectors, it is hard to find out which connector
> corresponds to the Type C port on the left panel versus the Type C port
> on the right panel. With location information provided, we can determine
> which specific device at which location is doing what.
>
> _PLD output includes much more fields, but only generic fields are added
> and exposed to sysfs, so that non-ACPI devices can also support it in
> the future. The minimal generic fields needed for locating a port are
> the following.
> - panel
> - horizontal_position
> - vertical_position
> - dock
> - lid
>
> Signed-off-by: Won Chung <wonchung@xxxxxxxxxx>
> ---
> Documentation/ABI/testing/sysfs-class-typec | 43 +++++++++++++++++
> drivers/usb/typec/class.c | 52 +++++++++++++++++++++
> drivers/usb/typec/class.h | 3 ++
> 3 files changed, 98 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
> index 75088ecad202..2879bc6e6ad2 100644
> --- a/Documentation/ABI/testing/sysfs-class-typec
> +++ b/Documentation/ABI/testing/sysfs-class-typec
> @@ -141,6 +141,49 @@ Description:
> - "reverse": CC2 orientation
> - "unknown": Orientation cannot be determined.
>
> +What: /sys/class/typec/<port>/location/panel
> +Date: February 2022
> +Contact: Won Chung <wonchung@xxxxxxxxxx>
> +Description:
> + Describes which panel surface of the system’s housing the
> + Type C port resides on:
> + 0 - Top
> + 1 - Bottom
> + 2 - Left
> + 3 - Right
> + 4 - Front
> + 5 - Back
> + 6 - Unknown (Vertical Position and Horizontal Position will be
> + ignored)
This is text files, why not say "top", "bottom", and so on? Why use a
number that means nothing?
> +
> +What: /sys/class/typec/<port>/location/vertical_position
> +Date: February 2022
> +Contact: Won Chung <wonchung@xxxxxxxxxx>
> +Description:
> + 0 - Upper
> + 1 - Center
> + 2 - Lower
Same here.
> +
> +What: /sys/class/typec/<port>/location/horizontal_position
> +Date: Feb, 2022
> +Contact: Won Chung <wonchung@xxxxxxxxxx>
> +Description:
> + 0 - Left
> + 1 - Center
> + 2 - Right
And here.
> +
> +What: /sys/class/typec/<port>/location/dock
> +Date: Feb, 2022
Note that date ends in a few hours :(
> +Contact: Won Chung <wonchung@xxxxxxxxxx>
> +Description:
> + Set if the port resides in a docking station or a port replicator.
> +
> +What: /sys/class/typec/<port>/location/lid
> +Date: Feb, 2022
> +Contact: Won Chung <wonchung@xxxxxxxxxx>
> +Description:
> + Set if the port resides on the lid of laptop system.
"set"? What does that mean?
> +
> 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 45a6f0c807cb..43b23c221f95 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -1579,8 +1579,40 @@ static const struct attribute_group typec_group = {
> .attrs = typec_attrs,
> };
>
> +#define DEV_ATTR_LOCATION_PROP(prop) \
> + static ssize_t prop##_show(struct device *dev, struct device_attribute *attr, \
> + char *buf) \
> + { \
> + struct typec_port *port = to_typec_port(dev); \
> + if (port->pld) \
> + return sprintf(buf, "%u\n", port->pld->prop); \
> + return 0; \
> + }; \
> +static DEVICE_ATTR_RO(prop)
> +
> +DEV_ATTR_LOCATION_PROP(panel);
> +DEV_ATTR_LOCATION_PROP(vertical_position);
> +DEV_ATTR_LOCATION_PROP(horizontal_position);
> +DEV_ATTR_LOCATION_PROP(dock);
> +DEV_ATTR_LOCATION_PROP(lid);
> +
> +static struct attribute *typec_location_attrs[] = {
> + &dev_attr_panel.attr,
> + &dev_attr_vertical_position.attr,
> + &dev_attr_horizontal_position.attr,
> + &dev_attr_dock.attr,
> + &dev_attr_lid.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group typec_location_group = {
> + .name = "location",
> + .attrs = typec_location_attrs,
> +};
> +
> static const struct attribute_group *typec_groups[] = {
> &typec_group,
> + &typec_location_group,
> NULL
> };
>
> @@ -1614,6 +1646,24 @@ const struct device_type typec_port_dev_type = {
> .release = typec_release,
> };
>
> +void *get_pld(struct device *dev)
That is a horrible global function name :(
And why a void pointer? We have real types in the kernel, please use
them.
> +{
> +#ifdef CONFIG_ACPI
No #ifdefs in .c files please.
> + struct acpi_pld_info *pld;
> + acpi_status status;
> +
> + if (!has_acpi_companion(dev))
> + return NULL;
> +
> + status = acpi_get_physical_device_location(ACPI_HANDLE(dev), &pld);
> + if (ACPI_FAILURE(status))
> + return NULL;
> + return pld;
See, you return a real type, don't throw that information away. This
isn't Windows :)
thanks,
gre gk-h
