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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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