Re: [PATCH v4] ACPI: device_sysfs: Add sysfs support for _PLD
From: Heikki Krogerus
Date: Wed Feb 02 2022 - 03:19:22 EST
Hi Won,
On Tue, Feb 01, 2022 at 01:55:18AM +0000, Won Chung wrote:
> When ACPI table includes _PLD fields for a device, create a new
> directory (pld) in sysfs to share _PLD fields.
I think you need to explain what needs this information in user space.
> Signed-off-by: Won Chung <wonchung@xxxxxxxxxx>
> ---
> Documentation/ABI/testing/sysfs-bus-acpi | 107 +++++++++++++++++++++++
> drivers/acpi/device_sysfs.c | 55 ++++++++++++
> include/acpi/acpi_bus.h | 1 +
> 3 files changed, 163 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-acpi b/Documentation/ABI/testing/sysfs-bus-acpi
> index 58abacf59b2a..b8b71c8f3cfd 100644
> --- a/Documentation/ABI/testing/sysfs-bus-acpi
> +++ b/Documentation/ABI/testing/sysfs-bus-acpi
> @@ -96,3 +96,110 @@ 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: Feb, 2022
> +Contact: Won Chung <wonchung@xxxxxxxxxx>
> +Description:
> + This directory contains the output of the device object's _PLD
> + control method, if present. This information provides details
> + on physical location of a device.
> +
> +What: /sys/bus/acpi/devices/.../pld/revision
> +Date: Feb, 2022
> +Contact: Won Chung <wonchung@xxxxxxxxxx>
> +Description:
> + The current revision is 0x2.
> +
> +What: /sys/bus/acpi/devices/.../pld/group_token
> +Date: Feb, 2022
> +Contact: Won Chung <wonchung@xxxxxxxxxx>
> +Description:
> + Unique numerical value identifying a group.
> +
> +What: /sys/bus/acpi/devices/.../pld/group_position
> +Date: Feb, 2022
> +Contact: Won Chung <wonchung@xxxxxxxxxx>
> +Description:
> + Identifies this device connection point’s position in the group.
> +
> +What: /sys/bus/acpi/devices/.../pld/user_visible
> +Date: Feb, 2022
> +Contact: Won Chung <wonchung@xxxxxxxxxx>
> +Description:
> + Set if the device connection point can be seen by the user
> + without disassembly.
> +
> +What: /sys/bus/acpi/devices/.../pld/dock
> +Date: Feb, 2022
> +Contact: Won Chung <wonchung@xxxxxxxxxx>
> +Description:
> + Set if the device connection point resides in a docking station
> + or port replicator.
> +
> +What: /sys/bus/acpi/devices/.../pld/bay
> +Date: Feb, 2022
> +Contact: Won Chung <wonchung@xxxxxxxxxx>
> +Description:
> + Set if describing a device in a bay or if device connection
> + point is a bay.
> +
> +What: /sys/bus/acpi/devices/.../pld/lid
> +Date: Feb, 2022
> +Contact: Won Chung <wonchung@xxxxxxxxxx>
> +Description:
> + Set if this device connection point resides on the lid of
> + laptop system.
> +
> +What: /sys/bus/acpi/devices/.../pld/panel
> +Date: Feb, 2022
> +Contact: Won Chung <wonchung@xxxxxxxxxx>
> +Description:
> + 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)
> +
> +What: /sys/bus/acpi/devices/.../pld/vertical_position
> +Date: Feb, 2022
> +Contact: Won Chung <wonchung@xxxxxxxxxx>
> +Description:
> + 0 - Upper
> + 1 - Center
> + 2 - Lower
> +
> +What: /sys/bus/acpi/devices/.../pld/horizontal_position
> +Date: Feb, 2022
> +Contact: Won Chung <wonchung@xxxxxxxxxx>
> +Description:
> + ACPI specification does not define horizontal position field.
> + Can be used as either
> + 0 - Left
> + 1 - Center
> + 2 - Right
> + or
> + 0 - Leftmost
> + and higher numbers going toward the right.
> +
> +What: /sys/bus/acpi/devices/.../pld/shape
> +Date: Feb, 2022
> +Contact: Won Chung <wonchung@xxxxxxxxxx>
> +Description:
> + 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..610be93635a0 100644
> --- a/drivers/acpi/device_sysfs.c
> +++ b/drivers/acpi/device_sysfs.c
> @@ -509,6 +509,49 @@ static ssize_t status_show(struct device *dev, struct device_attribute *attr,
> }
> static DEVICE_ATTR_RO(status);
>
> +#define DEV_ATTR_PLD_PROP(prop) \
> + static ssize_t prop##_show(struct device *dev, struct device_attribute *attr, \
> + char *buf) \
> +{ \
> + struct acpi_device *acpi_dev = to_acpi_device(dev); \
> + if (acpi_dev->pld == NULL) \
> + return -EIO; \
> + return sprintf(buf, "%u\n", acpi_dev->pld->prop); \
> +}; \
Ah, you are storing the _PLD below. Before there were concerns about
the memory that the cached _PLD information would consume. Another way
of doing this would be to just always evaluate the _PLD here.
Rafael needs to comment on this. My personal opinion is that let's
just store the thing.
> +static DEVICE_ATTR_RO(prop)
> +
> +DEV_ATTR_PLD_PROP(revision);
> +DEV_ATTR_PLD_PROP(group_token);
> +DEV_ATTR_PLD_PROP(group_position);
> +DEV_ATTR_PLD_PROP(user_visible);
> +DEV_ATTR_PLD_PROP(dock);
> +DEV_ATTR_PLD_PROP(bay);
> +DEV_ATTR_PLD_PROP(lid);
> +DEV_ATTR_PLD_PROP(panel);
> +DEV_ATTR_PLD_PROP(vertical_position);
> +DEV_ATTR_PLD_PROP(horizontal_position);
> +DEV_ATTR_PLD_PROP(shape);
> +
> +static struct attribute *dev_attr_pld[] = {
> + &dev_attr_revision.attr,
> + &dev_attr_group_token.attr,
> + &dev_attr_group_position.attr,
> + &dev_attr_user_visible.attr,
> + &dev_attr_dock.attr,
> + &dev_attr_bay.attr,
> + &dev_attr_lid.attr,
> + &dev_attr_panel.attr,
> + &dev_attr_vertical_position.attr,
> + &dev_attr_horizontal_position.attr,
> + &dev_attr_shape.attr,
> + NULL,
> +};
> +
> +static struct attribute_group dev_attr_pld_group = {
> + .name = "pld",
> + .attrs = dev_attr_pld,
> +};
> +
> /**
> * acpi_device_setup_files - Create sysfs attributes of an ACPI device.
> * @dev: ACPI device object.
> @@ -595,6 +638,16 @@ int acpi_device_setup_files(struct acpi_device *dev)
> &dev_attr_real_power_state);
> }
>
> + if (acpi_has_method(dev->handle, "_PLD")) {
> + status = acpi_get_physical_device_location(dev->handle,
> + &dev->pld);
> + if (ACPI_FAILURE(status))
> + goto end;
> + result = device_add_group(&dev->dev, &dev_attr_pld_group);
> + if (result)
> + goto end;
> + }
You probable want to store the pld in acpi_store_pld_crc(). Perhaps
also rename that function to just acpi_store_pld() at the same time.
Here you just want to create the sysfs group.
> acpi_expose_nondev_subnodes(&dev->dev.kobj, &dev->data);
>
> end:
> @@ -645,4 +698,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_group(&dev->dev, &dev_attr_pld_group);
> }
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index ca88c4706f2b..929e726a666b 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -381,6 +381,7 @@ struct acpi_device {
> struct acpi_hotplug_context *hp;
> struct acpi_driver *driver;
> const struct acpi_gpio_mapping *driver_gpios;
> + struct acpi_pld_info *pld;
> void *driver_data;
> struct device dev;
> unsigned int physical_node_count;
> --
thanks,
--
heikki