[PATCH v4 1/2] USB: Use ACPI device information

[PATCH v4 1/2] USB: Use ACPI device information

[Samuel Ortiz]

From: Faouaz TENOUTIT <faouaz.tenoutit@intel.com>

Use ACPI device pld information instead of calling
acpi_get_physical_device_location.

Signed-off-by: Faouaz TENOUTIT <faouaz.tenoutit@intel.com>
Acked-by: Samuel Ortiz <samuel.ortiz@intel.com>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/acpi/scan.c         | 13 +++++++++++++
 drivers/usb/core/usb-acpi.c | 14 +++++---------
 include/acpi/acpi_bus.h     |  1 +
 3 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 0a817ad..2ca42d5 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -767,6 +767,13 @@ static int acpi_device_setup_files(struct acpi_device *dev)
 						    &dev_attr_real_power_state);
 	}
 
+	/*
+	 * If device has _PLD, initialize the 'pld' struct
+	 */
+	if (acpi_has_method(dev->handle, "_PLD"))
+		acpi_get_physical_device_location(dev->handle,
+						  &dev->pld);
+
 end:
 	return result;
 }
@@ -806,6 +813,12 @@ static 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 device has _PLD, free 'pld' struct
+	 */
+	if (dev->pld)
+		ACPI_FREE(dev->pld);
 }
 /* --------------------------------------------------------------------------
 			ACPI Bus operations
diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
index 2776cfe..940b3e5 100644
--- a/drivers/usb/core/usb-acpi.c
+++ b/drivers/usb/core/usb-acpi.c
@@ -157,9 +157,6 @@ static struct acpi_device *usb_acpi_find_companion(struct device *dev)
 	} else if (is_usb_port(dev)) {
 		struct usb_port *port_dev = to_usb_port(dev);
 		int port1 = port_dev->portnum;
-		struct acpi_pld_info *pld;
-		acpi_handle *handle;
-		acpi_status status;
 
 		/* Get the struct usb_device point of port's hub */
 		udev = to_usb_device(dev->parent->parent);
@@ -190,15 +187,14 @@ static struct acpi_device *usb_acpi_find_companion(struct device *dev)
 			if (!adev)
 				return NULL;
 		}
-		handle = adev->handle;
-		status = acpi_get_physical_device_location(handle, &pld);
-		if (ACPI_FAILURE(status) || !pld)
+		if (!adev->pld)
 			return adev;
 
 		port_dev->location = USB_ACPI_LOCATION_VALID
-			| pld->group_token << 8 | pld->group_position;
-		port_dev->connect_type = usb_acpi_get_connect_type(handle, pld);
-		ACPI_FREE(pld);
+			| adev->pld->group_token << 8
+			| adev->pld->group_position;
+		port_dev->connect_type = usb_acpi_get_connect_type(adev->handle,
+								   adev->pld);
 
 		return adev;
 	}
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index bcfd808..e439cfa 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -357,6 +357,7 @@ struct acpi_device {
 	struct acpi_scan_handler *handler;
 	struct acpi_hotplug_context *hp;
 	struct acpi_driver *driver;
+	struct acpi_pld_info *pld;
 	void *driver_data;
 	struct device dev;
 	unsigned int physical_node_count;
-- 
2.0.0

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

[Samuel Ortiz]

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);
+			if (result) {
+				ACPI_FREE(dev->pld);
+				dev->pld = NULL;
+			}
+		}
+	}
 
 end:
 	return result;
@@ -815,10 +854,12 @@ static void acpi_device_remove_files(struct acpi_device *dev)
 		device_remove_file(&dev->dev, &dev_attr_path);
 
 	/*
-	 * If device has _PLD, free 'pld' struct
+	 * If device has _PLD, remove 'pld' directory
 	 */
-	if (dev->pld)
+	if (dev->pld) {
+		sysfs_remove_group(&dev->dev.kobj, &acpi_pld_attr_group);
 		ACPI_FREE(dev->pld);
+	}
 }
 /* --------------------------------------------------------------------------
 			ACPI Bus operations
-- 
2.0.0

--
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

Re: [PATCH v4 1/2] USB: Use ACPI device information

[Greg Kroah-Hartman]

On Wed, Aug 13, 2014 at 07:15:31PM +0200, Samuel Ortiz wrote:
> From: Faouaz TENOUTIT <faouaz.tenoutit@intel.com>
> 
> Use ACPI device pld information instead of calling
> acpi_get_physical_device_location.

Why?  What does this do "better" than the existing code?


thanks,

gre gk-h

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

[Greg Kroah-Hartman]

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

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

[Rafael J. Wysocki]

On Thursday, August 14, 2014 06:05:24 AM Greg Kroah-Hartman wrote:
> 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...

It will work.  There are other groups created in the same function and ACPI has
a deferred KOBJ_ADD notification.

Rafael

--
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

Re: [PATCH v4 1/2] USB: Use ACPI device information

[Rafael J. Wysocki]

On Thursday, August 14, 2014 06:03:09 AM Greg Kroah-Hartman wrote:
> On Wed, Aug 13, 2014 at 07:15:31PM +0200, Samuel Ortiz wrote:
> > From: Faouaz TENOUTIT <faouaz.tenoutit@intel.com>
> > 
> > Use ACPI device pld information instead of calling
> > acpi_get_physical_device_location.
> 
> Why?  What does this do "better" than the existing code?

Well, you've acked this patch before. :-)

It just doesn't execute AML from within the USB core.  I thought that would
be a sufficient reason, wouldn't it?

Rafael

Re: [PATCH v4 1/2] USB: Use ACPI device information

[Greg Kroah-Hartman]

On Thu, Aug 14, 2014 at 02:17:59AM +0200, Rafael J. Wysocki wrote:
> On Thursday, August 14, 2014 06:03:09 AM Greg Kroah-Hartman wrote:
> > On Wed, Aug 13, 2014 at 07:15:31PM +0200, Samuel Ortiz wrote:
> > > From: Faouaz TENOUTIT <faouaz.tenoutit@intel.com>
> > > 
> > > Use ACPI device pld information instead of calling
> > > acpi_get_physical_device_location.
> > 
> > Why?  What does this do "better" than the existing code?
> 
> Well, you've acked this patch before. :-)

I can't remember what I ate for Lunch yesterday, let alone previous
patches I've acked :)

> It just doesn't execute AML from within the USB core.  I thought that would
> be a sufficient reason, wouldn't it?

Yes, that's fine, but a little more description of "why" would have been
helpful here...

thanks,

greg k-h

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

[Greg Kroah-Hartman]

On Thu, Aug 14, 2014 at 02:15:22AM +0200, Rafael J. Wysocki wrote:
> On Thursday, August 14, 2014 06:05:24 AM Greg Kroah-Hartman wrote:
> > 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...
> 
> It will work.  There are other groups created in the same function and ACPI has
> a deferred KOBJ_ADD notification.

Ah, good, nevermind then.

Well, I'm still curious about the sysfs_create_group() call, please try
to use device_* functions when dealing with a struct device, if at all
possible.

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

Re: [PATCH v4 1/2] USB: Use ACPI device information

[Rafael J. Wysocki]

On Thursday, August 14, 2014 08:45:32 AM Greg Kroah-Hartman wrote:
> On Thu, Aug 14, 2014 at 02:17:59AM +0200, Rafael J. Wysocki wrote:
> > On Thursday, August 14, 2014 06:03:09 AM Greg Kroah-Hartman wrote:
> > > On Wed, Aug 13, 2014 at 07:15:31PM +0200, Samuel Ortiz wrote:
> > > > From: Faouaz TENOUTIT <faouaz.tenoutit@intel.com>
> > > > 
> > > > Use ACPI device pld information instead of calling
> > > > acpi_get_physical_device_location.
> > > 
> > > Why?  What does this do "better" than the existing code?
> > 
> > Well, you've acked this patch before. :-)
> 
> I can't remember what I ate for Lunch yesterday, let alone previous
> patches I've acked :)
> 
> > It just doesn't execute AML from within the USB core.  I thought that would
> > be a sufficient reason, wouldn't it?
> 
> Yes, that's fine, but a little more description of "why" would have been
> helpful here...

Since we are going to export _PLD output via sysfs ([PATCH 2/2]) and it's
guaranteed to return the same data every time it is evaluated, it simply
makes more sense to evaluate it once and cache the data for when it's
needed.  This particular one makes USB use the cached data.

Rafael

Re: [PATCH v4 1/2] USB: Use ACPI device information

[Rafael J. Wysocki]

On Wednesday, August 13, 2014 07:15:31 PM Samuel Ortiz wrote:
> From: Faouaz TENOUTIT <faouaz.tenoutit@intel.com>
> 
> Use ACPI device pld information instead of calling
> acpi_get_physical_device_location.
> 
> Signed-off-by: Faouaz TENOUTIT <faouaz.tenoutit@intel.com>
> Acked-by: Samuel Ortiz <samuel.ortiz@intel.com>
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

There is a problem with this approach I've been overlooking, sorry about that.

Namely, the information returned by _PLD is generally not guaranteed to be
the same every time that object is evaluated, so either we shouldn't cache it,
or we should change the cached data in response to specific device notifications
as described in the _PLD section of the spec.

> ---
>  drivers/acpi/scan.c         | 13 +++++++++++++
>  drivers/usb/core/usb-acpi.c | 14 +++++---------
>  include/acpi/acpi_bus.h     |  1 +
>  3 files changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 0a817ad..2ca42d5 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -767,6 +767,13 @@ static int acpi_device_setup_files(struct acpi_device *dev)
>  						    &dev_attr_real_power_state);
>  	}
>  
> +	/*
> +	 * If device has _PLD, initialize the 'pld' struct
> +	 */
> +	if (acpi_has_method(dev->handle, "_PLD"))
> +		acpi_get_physical_device_location(dev->handle,
> +						  &dev->pld);
> +
>  end:
>  	return result;
>  }
> @@ -806,6 +813,12 @@ static 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 device has _PLD, free 'pld' struct
> +	 */
> +	if (dev->pld)
> +		ACPI_FREE(dev->pld);
>  }
>  /* --------------------------------------------------------------------------
>  			ACPI Bus operations
> diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
> index 2776cfe..940b3e5 100644
> --- a/drivers/usb/core/usb-acpi.c
> +++ b/drivers/usb/core/usb-acpi.c
> @@ -157,9 +157,6 @@ static struct acpi_device *usb_acpi_find_companion(struct device *dev)
>  	} else if (is_usb_port(dev)) {
>  		struct usb_port *port_dev = to_usb_port(dev);
>  		int port1 = port_dev->portnum;
> -		struct acpi_pld_info *pld;
> -		acpi_handle *handle;
> -		acpi_status status;
>  
>  		/* Get the struct usb_device point of port's hub */
>  		udev = to_usb_device(dev->parent->parent);
> @@ -190,15 +187,14 @@ static struct acpi_device *usb_acpi_find_companion(struct device *dev)
>  			if (!adev)
>  				return NULL;
>  		}
> -		handle = adev->handle;
> -		status = acpi_get_physical_device_location(handle, &pld);
> -		if (ACPI_FAILURE(status) || !pld)
> +		if (!adev->pld)
>  			return adev;
>  
>  		port_dev->location = USB_ACPI_LOCATION_VALID
> -			| pld->group_token << 8 | pld->group_position;
> -		port_dev->connect_type = usb_acpi_get_connect_type(handle, pld);
> -		ACPI_FREE(pld);
> +			| adev->pld->group_token << 8
> +			| adev->pld->group_position;
> +		port_dev->connect_type = usb_acpi_get_connect_type(adev->handle,
> +								   adev->pld);
>  
>  		return adev;
>  	}
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index bcfd808..e439cfa 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -357,6 +357,7 @@ struct acpi_device {
>  	struct acpi_scan_handler *handler;
>  	struct acpi_hotplug_context *hp;
>  	struct acpi_driver *driver;
> +	struct acpi_pld_info *pld;
>  	void *driver_data;
>  	struct device dev;
>  	unsigned int physical_node_count;
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.