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 <[email protected]>
---
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 <[email protected]>
+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)
+
+What: /sys/class/typec/<port>/location/vertical_position
+Date: February 2022
+Contact: Won Chung <[email protected]>
+Description:
+ 0 - Upper
+ 1 - Center
+ 2 - Lower
+
+What: /sys/class/typec/<port>/location/horizontal_position
+Date: Feb, 2022
+Contact: Won Chung <[email protected]>
+Description:
+ 0 - Left
+ 1 - Center
+ 2 - Right
+
+What: /sys/class/typec/<port>/location/dock
+Date: Feb, 2022
+Contact: Won Chung <[email protected]>
+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 <[email protected]>
+Description:
+ Set if the port resides on the lid of laptop system.
+
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)
+{
+#ifdef CONFIG_ACPI
+ 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;
+#else
+ return NULL;
+#endif
+}
+
/* --------------------------------------- */
/* Driver callbacks to report role updates */
@@ -2073,6 +2123,8 @@ struct typec_port *typec_register_port(struct device *parent,
return ERR_PTR(ret);
}
+ port->pld = get_pld(&port->dev);
+
ret = device_add(&port->dev);
if (ret) {
dev_err(parent, "failed to register port (%d)\n", ret);
diff --git a/drivers/usb/typec/class.h b/drivers/usb/typec/class.h
index 0f1bd6d19d67..1b52633400c8 100644
--- a/drivers/usb/typec/class.h
+++ b/drivers/usb/typec/class.h
@@ -3,6 +3,7 @@
#ifndef __USB_TYPEC_CLASS__
#define __USB_TYPEC_CLASS__
+#include <linux/acpi.h>
#include <linux/device.h>
#include <linux/usb/typec.h>
@@ -54,6 +55,8 @@ struct typec_port {
const struct typec_capability *cap;
const struct typec_operations *ops;
+
+ struct acpi_pld_info *pld;
};
#define to_typec_port(_dev_) container_of(_dev_, struct typec_port, dev)
--
2.35.1.574.g5d30c73bfb-goog
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 <[email protected]>
> ---
> 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 <[email protected]>
> +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 <[email protected]>
> +Description:
> + 0 - Upper
> + 1 - Center
> + 2 - Lower
Same here.
> +
> +What: /sys/class/typec/<port>/location/horizontal_position
> +Date: Feb, 2022
> +Contact: Won Chung <[email protected]>
> +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 <[email protected]>
> +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 <[email protected]>
> +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
Hi Won,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on usb/usb-testing]
[also build test WARNING on chrome-platform/for-next linus/master v5.17-rc6 next-20220228]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Won-Chung/usb-typec-Add-sysfs-support-for-Type-C-connector-s-physical-location/20220301-030738
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: arm-randconfig-c002-20220228 (https://download.01.org/0day-ci/archive/20220301/[email protected]/config)
compiler: arm-linux-gnueabi-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/93d7a0fa1ed009ae6cc98fe5039cec8c9c77609f
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Won-Chung/usb-typec-Add-sysfs-support-for-Type-C-connector-s-physical-location/20220301-030738
git checkout 93d7a0fa1ed009ae6cc98fe5039cec8c9c77609f
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arm SHELL=/bin/bash drivers/usb/typec/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> drivers/usb/typec/class.c:1649:7: warning: no previous prototype for 'get_pld' [-Wmissing-prototypes]
1649 | void *get_pld(struct device *dev)
| ^~~~~~~
vim +/get_pld +1649 drivers/usb/typec/class.c
1648
> 1649 void *get_pld(struct device *dev)
1650 {
1651 #ifdef CONFIG_ACPI
1652 struct acpi_pld_info *pld;
1653 acpi_status status;
1654
1655 if (!has_acpi_companion(dev))
1656 return NULL;
1657
1658 status = acpi_get_physical_device_location(ACPI_HANDLE(dev), &pld);
1659 if (ACPI_FAILURE(status))
1660 return NULL;
1661 return pld;
1662 #else
1663 return NULL;
1664 #endif
1665 }
1666
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi Won,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on usb/usb-testing]
[also build test WARNING on chrome-platform/for-next linus/master v5.17-rc6 next-20220228]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Won-Chung/usb-typec-Add-sysfs-support-for-Type-C-connector-s-physical-location/20220301-030738
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: hexagon-randconfig-r041-20220227 (https://download.01.org/0day-ci/archive/20220301/[email protected]/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project d271fc04d5b97b12e6b797c6067d3c96a8d7470e)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/93d7a0fa1ed009ae6cc98fe5039cec8c9c77609f
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Won-Chung/usb-typec-Add-sysfs-support-for-Type-C-connector-s-physical-location/20220301-030738
git checkout 93d7a0fa1ed009ae6cc98fe5039cec8c9c77609f
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/usb/typec/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> drivers/usb/typec/class.c:1649:7: warning: no previous prototype for function 'get_pld' [-Wmissing-prototypes]
void *get_pld(struct device *dev)
^
drivers/usb/typec/class.c:1649:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
void *get_pld(struct device *dev)
^
static
1 warning generated.
vim +/get_pld +1649 drivers/usb/typec/class.c
1648
> 1649 void *get_pld(struct device *dev)
1650 {
1651 #ifdef CONFIG_ACPI
1652 struct acpi_pld_info *pld;
1653 acpi_status status;
1654
1655 if (!has_acpi_companion(dev))
1656 return NULL;
1657
1658 status = acpi_get_physical_device_location(ACPI_HANDLE(dev), &pld);
1659 if (ACPI_FAILURE(status))
1660 return NULL;
1661 return pld;
1662 #else
1663 return NULL;
1664 #endif
1665 }
1666
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]