Re: [PATCH v4 2/2] ACPI: Export PLD (Physical Location of Device)

Greg Kroah-Hartman
2014-08-13T22:06:00Z

On Wed, Aug 13, 2014 at 07:15:32PM +0200, Samuel Ortiz wrote:
> From: Faouaz TENOUTIT <faouaz.tenoutit@intel.com>
>
> This patch exports a sysfs directory with ACPI _PLD information:
>
> $ ls -l /sys/bus/acpi/devices/ACPI-DEV/pld
> -r--r--r-- root root 4096 2014-05-30 08:39 panel
> -r--r--r-- root root 4096 2014-05-30 08:39 revision
> -r--r--r-- root root 4096 2014-05-30 08:39 rotation
> -r--r--r-- root root 4096 2014-05-30 08:39 shape
>
> This information can be used by user applications to:
> - Determine which specific connector or device input mechanism may be used
> for a given task.
> - Describes which panel surface of the system's housing the device
> connection point resides on (Front, Back, ...)
>
> More information about these _PLD fields can be found here:
> Documentation/ABI/testing/sysfs-bus-acpi
>
> Signed-off-by: Faouaz TENOUTIT <faouaz.tenoutit@intel.com>
> Acked-by: Samuel Ortiz <samuel.ortiz@intel.com>
> Acked-by: Andrew J Ross <andrew.j.ross@intel.com>
> ---
> Documentation/ABI/testing/sysfs-bus-acpi | 60 ++++++++++++++++++++++++++++++++
> drivers/acpi/scan.c | 53 ++++++++++++++++++++++++----
> 2 files changed, 107 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-acpi b/Documentation/ABI/testing/sysfs-bus-acpi
> index 7fa9cbc..be58159 100644
> --- a/Documentation/ABI/testing/sysfs-bus-acpi
> +++ b/Documentation/ABI/testing/sysfs-bus-acpi
> @@ -56,3 +56,63 @@ Description:
> Writing 1 to this attribute will trigger hot removal of
> this device object. This file exists for every device
> object that has _EJ0 method.
> +
> +What: /sys/bus/acpi/devices/.../pld
> +Date: August 2014
> +Contact: Faouaz Tenoutit <faouaz.tenoutit@intel.com>
> +Description:
> + This optional directory provides description of the physical
> + location, orientation and position of external devices.
> + It is for example used to describe how camera sensors are
> + physically mounted and allows user space software to rotate
> + images accordingly.
> +
> +What: /sys/bus/acpi/devices/.../pld/revision
> +Date: August 2014
> +Contact: Faouaz Tenoutit <faouaz.tenoutit@intel.com>
> +Description:
> + The current Revision is 0x2
> +
> +What: /sys/bus/acpi/devices/.../pld/panel
> +Date: August 2014
> +Contact: Faouaz Tenoutit <faouaz.tenoutit@intel.com>
> +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
> +
> +What: /sys/bus/acpi/devices/.../pld/shape
> +Date: August 2014
> +Contact: Faouaz Tenoutit <faouaz.tenoutit@intel.com>
> +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
> +
> +What: /sys/bus/acpi/devices/.../pld/rotation
> +Date: August 2014
> +Contact: Faouaz Tenoutit <faouaz.tenoutit@intel.com>
> +Description:
> + Rotates the Shape clockwise in 45 degree steps around its
> + origin where:
> + 0 0°
> + 1 45°
> + 2 90°
> + 3 135°
> + 4 180°
> + 5 225°
> + 6 270°
> + 7 315°
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 2ca42d5..19889a3 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -686,6 +686,36 @@ static ssize_t status_show(struct device *dev, struct device_attribute *attr,
> }
> static DEVICE_ATTR_RO(status);
>
> +#define ACPI_SYSFS_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); \
> + return sprintf(buf, "%d\n", acpi_dev->pld->prop); \
> +}; \
> +static DEVICE_ATTR_RO(prop)
> +
> +/*
> + * sysfs PLD parameters
> + */
> +ACPI_SYSFS_PLD_PROP(revision);
> +ACPI_SYSFS_PLD_PROP(panel);
> +ACPI_SYSFS_PLD_PROP(shape);
> +ACPI_SYSFS_PLD_PROP(rotation);
> +
> +static struct attribute *acpi_pld_attrs[] = {
> + &dev_attr_revision.attr,
> + &dev_attr_panel.attr,
> + &dev_attr_shape.attr,
> + &dev_attr_rotation.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group acpi_pld_attr_group = {
> + .name = "pld",
> + .attrs = acpi_pld_attrs,
> +};
> +
> static int acpi_device_setup_files(struct acpi_device *dev)
> {
> struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
> @@ -768,11 +798,20 @@ static int acpi_device_setup_files(struct acpi_device *dev)
> }
>
> /*
> - * If device has _PLD, initialize the 'pld' struct
> + * If device has _PLD, 'pld' directory is created
> */
> - if (acpi_has_method(dev->handle, "_PLD"))
> - acpi_get_physical_device_location(dev->handle,
> - &dev->pld);
> + if (acpi_has_method(dev->handle, "_PLD")) {
> + status = acpi_get_physical_device_location(dev->handle,
> + &dev->pld);
> + if (ACPI_SUCCESS(status)) {
> + result = sysfs_create_group(&dev->dev.kobj,
> + &acpi_pld_attr_group);

It's a device, why not call device_create_groups()?

> + if (result) {
> + ACPI_FREE(dev->pld);
> + dev->pld = NULL;
> + }
> + }
> + }

Is this being called after or before the device in question is
registered with the driver core?

Hint, I think I know the answer, but I want you to learn why this code
is wrong and will not work properly...

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html