2022-01-31 11:31:04

by Won Chung

[permalink] [raw]
Subject: [PATCH v3] ACPI: device_sysfs: Add sysfs support for _PLD

When ACPI table includes _PLD fields for a device, create a new file
(pld) in sysfs to share _PLD fields.

Signed-off-by: Won Chung <[email protected]>
---
Documentation/ABI/testing/sysfs-bus-acpi | 53 ++++++++++++++++++++++++
drivers/acpi/device_sysfs.c | 42 +++++++++++++++++++
2 files changed, 95 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-acpi b/Documentation/ABI/testing/sysfs-bus-acpi
index 58abacf59b2a..3a9c6a4f4603 100644
--- a/Documentation/ABI/testing/sysfs-bus-acpi
+++ b/Documentation/ABI/testing/sysfs-bus-acpi
@@ -96,3 +96,56 @@ Description:
hardware, if the _HRV control method is present. It is mostly
useful for non-PCI devices because lspci can list the hardware
version for PCI devices.
+
+What: /sys/bus/acpi/devices/.../pld
+Date: Jan, 2022
+Contact: Won Chung <[email protected]>
+Description:
+ This attribute contains the output of the device object's
+ _PLD control method, if present. This information provides
+ details on physical location of a port.
+
+ Description on each _PLD field from ACPI specification:
+
+ =============== ============================================
+ GROUP_TOKEN Unique numerical value identifying a group.
+ GROUP_POSITION Identifies this device connection point’s
+ position in the group.
+ USER_VISIBLE Set if the device connection point can be
+ seen by the user without disassembly.
+ DOCK Set if the device connection point resides
+ in a docking station or port replicator.
+ BAY Set if describing a device in a bay or if
+ device connection point is a bay.
+ LID Set if this device connection point resides
+ on the lid of laptop system.
+ PANEL Describes which panel surface of the system’s
+ housing the device connection point resides on:
+ 0 - Top
+ 1 - Bottom
+ 2 - Left
+ 3 - Right
+ 4 - Front
+ 5 - Back
+ 6 - Unknown (Vertical Position and Horizontal
+ Position will be ignored)
+ HORIZONTAL_ 0 - Left
+ POSITION 1 - Center
+ 2 - Right
+ VERTICAL_ 0 - Upper
+ POSITION 1 - Center
+ 2 - Lower
+ SHAPE Describes the shape of the device connection
+ point.
+ 0 - Round
+ 1 - Oval
+ 2 - Square
+ 3 - Vertical Rectangle
+ 4 - Horizontal Rectangle
+ 5 - Vertical Trapezoid
+ 6 - Horizontal Trapezoid
+ 7 - Unknown - Shape rendered as a Rectangle
+ with dotted lines
+ 8 - Chamfered
+ 15:9 - Reserved
+ =============== ===============================================
diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
index d5d6403ba07b..8d4df5fb1c45 100644
--- a/drivers/acpi/device_sysfs.c
+++ b/drivers/acpi/device_sysfs.c
@@ -509,6 +509,40 @@ static ssize_t status_show(struct device *dev, struct device_attribute *attr,
}
static DEVICE_ATTR_RO(status);

+static ssize_t pld_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct acpi_device *acpi_dev = to_acpi_device(dev);
+ acpi_status status;
+ struct acpi_pld_info *pld;
+
+ status = acpi_get_physical_device_location(acpi_dev->handle, &pld);
+ if (ACPI_FAILURE(status))
+ return -ENODEV;
+
+ return sprintf(buf, "GROUP_TOKEN=%u\n"
+ "GROUP_POSITION=%u\n"
+ "USER_VISIBLE=%u\n"
+ "DOCK=%u\n"
+ "BAY=%u\n"
+ "LID=%u\n"
+ "PANEL=%u\n"
+ "HORIZONTAL_POSITION=%u\n"
+ "VERTICAL_POSITION=%u\n"
+ "SHAPE=%u\n",
+ pld->group_token,
+ pld->group_position,
+ pld->user_visible,
+ pld->dock,
+ pld->bay,
+ pld->lid,
+ pld->panel,
+ pld->horizontal_position,
+ pld->vertical_position,
+ pld->shape);
+}
+static DEVICE_ATTR_RO(pld);
+
/**
* acpi_device_setup_files - Create sysfs attributes of an ACPI device.
* @dev: ACPI device object.
@@ -595,6 +629,12 @@ int acpi_device_setup_files(struct acpi_device *dev)
&dev_attr_real_power_state);
}

+ if (acpi_has_method(dev->handle, "_PLD")) {
+ result = device_create_file(&dev->dev, &dev_attr_pld);
+ if (result)
+ goto end;
+ }
+
acpi_expose_nondev_subnodes(&dev->dev.kobj, &dev->data);

end:
@@ -645,4 +685,6 @@ void acpi_device_remove_files(struct acpi_device *dev)
device_remove_file(&dev->dev, &dev_attr_status);
if (dev->handle)
device_remove_file(&dev->dev, &dev_attr_path);
+ if (acpi_has_method(dev->handle, "_PLD"))
+ device_remove_file(&dev->dev, &dev_attr_pld);
}
--
2.35.0.rc2.247.g8bbb082509-goog


2022-02-01 15:37:16

by Heikki Krogerus

[permalink] [raw]
Subject: Re: [PATCH v3] ACPI: device_sysfs: Add sysfs support for _PLD

On Fri, Jan 28, 2022 at 06:08:32PM +0000, Won Chung wrote:
> When ACPI table includes _PLD fields for a device, create a new file
> (pld) in sysfs to share _PLD fields.
>
> Signed-off-by: Won Chung <[email protected]>
> ---
> Documentation/ABI/testing/sysfs-bus-acpi | 53 ++++++++++++++++++++++++
> drivers/acpi/device_sysfs.c | 42 +++++++++++++++++++
> 2 files changed, 95 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-acpi b/Documentation/ABI/testing/sysfs-bus-acpi
> index 58abacf59b2a..3a9c6a4f4603 100644
> --- a/Documentation/ABI/testing/sysfs-bus-acpi
> +++ b/Documentation/ABI/testing/sysfs-bus-acpi
> @@ -96,3 +96,56 @@ Description:
> hardware, if the _HRV control method is present. It is mostly
> useful for non-PCI devices because lspci can list the hardware
> version for PCI devices.
> +
> +What: /sys/bus/acpi/devices/.../pld
> +Date: Jan, 2022
> +Contact: Won Chung <[email protected]>
> +Description:
> + This attribute contains the output of the device object's
> + _PLD control method, if present. This information provides
> + details on physical location of a port.
> +
> + Description on each _PLD field from ACPI specification:
> +
> + =============== ============================================
> + GROUP_TOKEN Unique numerical value identifying a group.
> + GROUP_POSITION Identifies this device connection point’s
> + position in the group.
> + USER_VISIBLE Set if the device connection point can be
> + seen by the user without disassembly.
> + DOCK Set if the device connection point resides
> + in a docking station or port replicator.
> + BAY Set if describing a device in a bay or if
> + device connection point is a bay.
> + LID Set if this device connection point resides
> + on the lid of laptop system.
> + PANEL Describes which panel surface of the system’s
> + housing the device connection point resides on:
> + 0 - Top
> + 1 - Bottom
> + 2 - Left
> + 3 - Right
> + 4 - Front
> + 5 - Back
> + 6 - Unknown (Vertical Position and Horizontal
> + Position will be ignored)
> + HORIZONTAL_ 0 - Left
> + POSITION 1 - Center
> + 2 - Right
> + VERTICAL_ 0 - Upper
> + POSITION 1 - Center
> + 2 - Lower
> + SHAPE Describes the shape of the device connection
> + point.
> + 0 - Round
> + 1 - Oval
> + 2 - Square
> + 3 - Vertical Rectangle
> + 4 - Horizontal Rectangle
> + 5 - Vertical Trapezoid
> + 6 - Horizontal Trapezoid
> + 7 - Unknown - Shape rendered as a Rectangle
> + with dotted lines
> + 8 - Chamfered
> + 15:9 - Reserved
> + =============== ===============================================
> diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
> index d5d6403ba07b..8d4df5fb1c45 100644
> --- a/drivers/acpi/device_sysfs.c
> +++ b/drivers/acpi/device_sysfs.c
> @@ -509,6 +509,40 @@ static ssize_t status_show(struct device *dev, struct device_attribute *attr,
> }
> static DEVICE_ATTR_RO(status);
>
> +static ssize_t pld_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct acpi_device *acpi_dev = to_acpi_device(dev);
> + acpi_status status;
> + struct acpi_pld_info *pld;
> +
> + status = acpi_get_physical_device_location(acpi_dev->handle, &pld);
> + if (ACPI_FAILURE(status))
> + return -ENODEV;
> +
> + return sprintf(buf, "GROUP_TOKEN=%u\n"
> + "GROUP_POSITION=%u\n"
> + "USER_VISIBLE=%u\n"
> + "DOCK=%u\n"
> + "BAY=%u\n"
> + "LID=%u\n"
> + "PANEL=%u\n"
> + "HORIZONTAL_POSITION=%u\n"
> + "VERTICAL_POSITION=%u\n"
> + "SHAPE=%u\n",
> + pld->group_token,
> + pld->group_position,
> + pld->user_visible,
> + pld->dock,
> + pld->bay,
> + pld->lid,
> + pld->panel,
> + pld->horizontal_position,
> + pld->vertical_position,
> + pld->shape);
> +}
> +static DEVICE_ATTR_RO(pld);

Why not have a pld group (directory) and a separate attribute file for
each field?


thanks,

--
heikki

2022-02-02 02:40:26

by Won Chung

[permalink] [raw]
Subject: Re: [PATCH v3] ACPI: device_sysfs: Add sysfs support for _PLD

Hi Heikki,

Thank you for the review. It seems to be the convention to have a
separate attribute file for each field, as you pointed out. I have
made the change and sent v4.

Thanks,
Won


On Sun, Jan 30, 2022 at 11:33 PM Heikki Krogerus
<[email protected]> wrote:
>
> On Fri, Jan 28, 2022 at 06:08:32PM +0000, Won Chung wrote:
> > When ACPI table includes _PLD fields for a device, create a new file
> > (pld) in sysfs to share _PLD fields.
> >
> > Signed-off-by: Won Chung <[email protected]>
> > ---
> > Documentation/ABI/testing/sysfs-bus-acpi | 53 ++++++++++++++++++++++++
> > drivers/acpi/device_sysfs.c | 42 +++++++++++++++++++
> > 2 files changed, 95 insertions(+)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-acpi b/Documentation/ABI/testing/sysfs-bus-acpi
> > index 58abacf59b2a..3a9c6a4f4603 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-acpi
> > +++ b/Documentation/ABI/testing/sysfs-bus-acpi
> > @@ -96,3 +96,56 @@ Description:
> > hardware, if the _HRV control method is present. It is mostly
> > useful for non-PCI devices because lspci can list the hardware
> > version for PCI devices.
> > +
> > +What: /sys/bus/acpi/devices/.../pld
> > +Date: Jan, 2022
> > +Contact: Won Chung <[email protected]>
> > +Description:
> > + This attribute contains the output of the device object's
> > + _PLD control method, if present. This information provides
> > + details on physical location of a port.
> > +
> > + Description on each _PLD field from ACPI specification:
> > +
> > + =============== ============================================
> > + GROUP_TOKEN Unique numerical value identifying a group.
> > + GROUP_POSITION Identifies this device connection point’s
> > + position in the group.
> > + USER_VISIBLE Set if the device connection point can be
> > + seen by the user without disassembly.
> > + DOCK Set if the device connection point resides
> > + in a docking station or port replicator.
> > + BAY Set if describing a device in a bay or if
> > + device connection point is a bay.
> > + LID Set if this device connection point resides
> > + on the lid of laptop system.
> > + PANEL Describes which panel surface of the system’s
> > + housing the device connection point resides on:
> > + 0 - Top
> > + 1 - Bottom
> > + 2 - Left
> > + 3 - Right
> > + 4 - Front
> > + 5 - Back
> > + 6 - Unknown (Vertical Position and Horizontal
> > + Position will be ignored)
> > + HORIZONTAL_ 0 - Left
> > + POSITION 1 - Center
> > + 2 - Right
> > + VERTICAL_ 0 - Upper
> > + POSITION 1 - Center
> > + 2 - Lower
> > + SHAPE Describes the shape of the device connection
> > + point.
> > + 0 - Round
> > + 1 - Oval
> > + 2 - Square
> > + 3 - Vertical Rectangle
> > + 4 - Horizontal Rectangle
> > + 5 - Vertical Trapezoid
> > + 6 - Horizontal Trapezoid
> > + 7 - Unknown - Shape rendered as a Rectangle
> > + with dotted lines
> > + 8 - Chamfered
> > + 15:9 - Reserved
> > + =============== ===============================================
> > diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
> > index d5d6403ba07b..8d4df5fb1c45 100644
> > --- a/drivers/acpi/device_sysfs.c
> > +++ b/drivers/acpi/device_sysfs.c
> > @@ -509,6 +509,40 @@ static ssize_t status_show(struct device *dev, struct device_attribute *attr,
> > }
> > static DEVICE_ATTR_RO(status);
> >
> > +static ssize_t pld_show(struct device *dev, struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct acpi_device *acpi_dev = to_acpi_device(dev);
> > + acpi_status status;
> > + struct acpi_pld_info *pld;
> > +
> > + status = acpi_get_physical_device_location(acpi_dev->handle, &pld);
> > + if (ACPI_FAILURE(status))
> > + return -ENODEV;
> > +
> > + return sprintf(buf, "GROUP_TOKEN=%u\n"
> > + "GROUP_POSITION=%u\n"
> > + "USER_VISIBLE=%u\n"
> > + "DOCK=%u\n"
> > + "BAY=%u\n"
> > + "LID=%u\n"
> > + "PANEL=%u\n"
> > + "HORIZONTAL_POSITION=%u\n"
> > + "VERTICAL_POSITION=%u\n"
> > + "SHAPE=%u\n",
> > + pld->group_token,
> > + pld->group_position,
> > + pld->user_visible,
> > + pld->dock,
> > + pld->bay,
> > + pld->lid,
> > + pld->panel,
> > + pld->horizontal_position,
> > + pld->vertical_position,
> > + pld->shape);
> > +}
> > +static DEVICE_ATTR_RO(pld);
>
> Why not have a pld group (directory) and a separate attribute file for
> each field?
>
>
> thanks,
>
> --
> heikki

2022-02-02 02:40:53

by Won Chung

[permalink] [raw]
Subject: Re: [PATCH v3] ACPI: device_sysfs: Add sysfs support for _PLD

On Sun, Jan 30, 2022 at 11:33 PM Heikki Krogerus
<[email protected]> wrote:
>
> On Fri, Jan 28, 2022 at 06:08:32PM +0000, Won Chung wrote:
> > When ACPI table includes _PLD fields for a device, create a new file
> > (pld) in sysfs to share _PLD fields.
> >
> > Signed-off-by: Won Chung <[email protected]>
> > ---
> > Documentation/ABI/testing/sysfs-bus-acpi | 53 ++++++++++++++++++++++++
> > drivers/acpi/device_sysfs.c | 42 +++++++++++++++++++
> > 2 files changed, 95 insertions(+)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-acpi b/Documentation/ABI/testing/sysfs-bus-acpi
> > index 58abacf59b2a..3a9c6a4f4603 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-acpi
> > +++ b/Documentation/ABI/testing/sysfs-bus-acpi
> > @@ -96,3 +96,56 @@ Description:
> > hardware, if the _HRV control method is present. It is mostly
> > useful for non-PCI devices because lspci can list the hardware
> > version for PCI devices.
> > +
> > +What: /sys/bus/acpi/devices/.../pld
> > +Date: Jan, 2022
> > +Contact: Won Chung <[email protected]>
> > +Description:
> > + This attribute contains the output of the device object's
> > + _PLD control method, if present. This information provides
> > + details on physical location of a port.
> > +
> > + Description on each _PLD field from ACPI specification:
> > +
> > + =============== ============================================
> > + GROUP_TOKEN Unique numerical value identifying a group.
> > + GROUP_POSITION Identifies this device connection point’s
> > + position in the group.
> > + USER_VISIBLE Set if the device connection point can be
> > + seen by the user without disassembly.
> > + DOCK Set if the device connection point resides
> > + in a docking station or port replicator.
> > + BAY Set if describing a device in a bay or if
> > + device connection point is a bay.
> > + LID Set if this device connection point resides
> > + on the lid of laptop system.
> > + PANEL Describes which panel surface of the system’s
> > + housing the device connection point resides on:
> > + 0 - Top
> > + 1 - Bottom
> > + 2 - Left
> > + 3 - Right
> > + 4 - Front
> > + 5 - Back
> > + 6 - Unknown (Vertical Position and Horizontal
> > + Position will be ignored)
> > + HORIZONTAL_ 0 - Left
> > + POSITION 1 - Center
> > + 2 - Right
> > + VERTICAL_ 0 - Upper
> > + POSITION 1 - Center
> > + 2 - Lower
> > + SHAPE Describes the shape of the device connection
> > + point.
> > + 0 - Round
> > + 1 - Oval
> > + 2 - Square
> > + 3 - Vertical Rectangle
> > + 4 - Horizontal Rectangle
> > + 5 - Vertical Trapezoid
> > + 6 - Horizontal Trapezoid
> > + 7 - Unknown - Shape rendered as a Rectangle
> > + with dotted lines
> > + 8 - Chamfered
> > + 15:9 - Reserved
> > + =============== ===============================================
> > diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
> > index d5d6403ba07b..8d4df5fb1c45 100644
> > --- a/drivers/acpi/device_sysfs.c
> > +++ b/drivers/acpi/device_sysfs.c
> > @@ -509,6 +509,40 @@ static ssize_t status_show(struct device *dev, struct device_attribute *attr,
> > }
> > static DEVICE_ATTR_RO(status);
> >
> > +static ssize_t pld_show(struct device *dev, struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct acpi_device *acpi_dev = to_acpi_device(dev);
> > + acpi_status status;
> > + struct acpi_pld_info *pld;
> > +
> > + status = acpi_get_physical_device_location(acpi_dev->handle, &pld);
> > + if (ACPI_FAILURE(status))
> > + return -ENODEV;
> > +
> > + return sprintf(buf, "GROUP_TOKEN=%u\n"
> > + "GROUP_POSITION=%u\n"
> > + "USER_VISIBLE=%u\n"
> > + "DOCK=%u\n"
> > + "BAY=%u\n"
> > + "LID=%u\n"
> > + "PANEL=%u\n"
> > + "HORIZONTAL_POSITION=%u\n"
> > + "VERTICAL_POSITION=%u\n"
> > + "SHAPE=%u\n",
> > + pld->group_token,
> > + pld->group_position,
> > + pld->user_visible,
> > + pld->dock,
> > + pld->bay,
> > + pld->lid,
> > + pld->panel,
> > + pld->horizontal_position,
> > + pld->vertical_position,
> > + pld->shape);
> > +}
> > +static DEVICE_ATTR_RO(pld);
>
> Why not have a pld group (directory) and a separate attribute file for
> each field?
>
>
> thanks,
>
> --
> heikki

Hi Heikki,

Thank you for the review. It seems to be the convention to have a
separate attribute file for each field, as you pointed out. I have
made the change and sent v4.

Thanks,
Won