Re: [RFC PATCH 1/9] ACPI: Add support for device specific properties

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On Mon, Aug 18, 2014 at 04:13:29PM +0800, Hanjun Guo wrote:
> Hi,
> 
> Some minor comments below.
> 
> On 2014-8-17 14:04, Mika Westerberg wrote:
> > Device Tree is used in many embedded systems to describe the system
> > configuration to the OS. It supports attaching properties or name-value
> > pairs to the devices it describe. With these properties one can pass
> > additional information to the drivers that would not be available
> > otherwise.
> > 
> > ACPI is another configuration mechanism (among other things) typically
> > seen, but not limited to, x86 machines. ACPI allows passing arbitrary
> > data from methods but there has not been mechanism equivalent to Device
> > Tree until the introduction of _DSD in the recent publication of the
> > ACPI 5.1 specification.
> > 
> > In order to facilitate ACPI usage in systems where Device Tree is
> > typically used, it would be beneficial to standardize a way to retrieve
> > Device Tree style properties from ACPI devices, which is what we do in
> > this patch.
> > 
> > If a given device described in ACPI namespace wants to export properties it
> > must implement _DSD method (Device Specific Data, introduced with ACPI 5.1)
> > that returns the properties in a package of packages. For example:
> > 
> > 	Name (_DSD, Package () {
> > 		ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > 		Package () {
> > 			Package () {"name1", <VALUE1>},
> > 			Package () {"name2", <VALUE2>},
> > 			...
> > 		}
> > 	})
> > 
> > The UUID reserved for properties is daffd814-6eba-4d8c-8a91-bc9bbf4aa301
> > and is documented in the ACPI 5.1 companion document called "_DSD
> > Implementation Guide" [1], [2].
> > 
> > We add several helper functions that can be used to extract these
> > properties and convert them to different Linux data types.
> > 
> > The ultimate goal is that we only have one device property API that
> > retrieves the requested properties from Device Tree or from ACPI
> > transparent to the caller.
> > 
> > [1] http://www.uefi.org/sites/default/files/resources/_DSD-implementation-guide-toplevel.htm
> > [2] http://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf
> > 
> > Signed-off-by: Darren Hart <dvhart@xxxxxxxxxxxxxxx>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> > ---
> >  drivers/acpi/Makefile   |   1 +
> >  drivers/acpi/internal.h |   6 +
> >  drivers/acpi/property.c | 365 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  drivers/acpi/scan.c     |   2 +
> >  include/acpi/acpi_bus.h |   7 +
> >  include/linux/acpi.h    |  40 ++++++
> >  6 files changed, 421 insertions(+)
> >  create mode 100644 drivers/acpi/property.c
> > 
> > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> > index ea55e0179f81..6e8269a111db 100644
> > --- a/drivers/acpi/Makefile
> > +++ b/drivers/acpi/Makefile
> > @@ -45,6 +45,7 @@ acpi-y				+= acpi_pnp.o
> >  acpi-y				+= power.o
> >  acpi-y				+= event.o
> >  acpi-y				+= sysfs.o
> > +acpi-y				+= property.o
> >  acpi-$(CONFIG_X86)		+= acpi_cmos_rtc.o
> >  acpi-$(CONFIG_DEBUG_FS)		+= debugfs.o
> >  acpi-$(CONFIG_ACPI_NUMA)	+= numa.o
> > diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> > index 7de5b603f272..35001e2a8769 100644
> > --- a/drivers/acpi/internal.h
> > +++ b/drivers/acpi/internal.h
> > @@ -178,4 +178,10 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev);
> >  bool acpi_osi_is_win8(void);
> >  #endif
> >  
> > +/*--------------------------------------------------------------------------
> > +				Device properties
> > +  -------------------------------------------------------------------------- */
> > +void acpi_init_properties(struct acpi_device *adev);
> > +void acpi_free_properties(struct acpi_device *adev);
> > +
> >  #endif /* _ACPI_INTERNAL_H_ */
> > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > new file mode 100644
> > index 000000000000..093e834c35db
> > --- /dev/null
> > +++ b/drivers/acpi/property.c
> > @@ -0,0 +1,365 @@
> > +/*
> > + * ACPI device specific properties support.
> > + *
> > + * Copyright (C) 2014, Intel Corporation
> > + * All rights reserved.
> > + *
> > + * Authors: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> > + *          Darren Hart <dvhart@xxxxxxxxxxxxxxx>
> > + *          Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/device.h>
> > +#include <linux/export.h>
> > +
> > +#include "internal.h"
> > +
> > +/* ACPI _DSD device properties UUID: daffd814-6eba-4d8c-8a91-bc9bbf4aa301 */
> > +static const u8 prp_uuid[16] = {
> 
> s/prp_uuid/dsd_uuid ?

Well, it is actually the _DSD UUID that implements ACPI device
properties (hence the prp_) but I have no problems in changing that to
dsd_uuid instead.

> 
> > +	0x14, 0xd8, 0xff, 0xda, 0xba, 0x6e, 0x8c, 0x4d,
> > +	0x8a, 0x91, 0xbc, 0x9b, 0xbf, 0x4a, 0xa3, 0x01
> > +};
> > +
> > +static bool acpi_property_value_ok(const union acpi_object *value)
> > +{
> > +	int j;
> > +
> > +	/*
> > +	 * The value must be an integer, a string, a reference, or a package
> > +	 * whose every element must be an integer, a string, or a reference.
> > +	 */
> > +	switch (value->type) {
> > +	case ACPI_TYPE_INTEGER:
> > +	case ACPI_TYPE_STRING:
> > +	case ACPI_TYPE_LOCAL_REFERENCE:
> > +		return true;
> > +
> > +	case ACPI_TYPE_PACKAGE:
> > +		for (j = 0; j < value->package.count; j++)
> > +			switch (value->package.elements[j].type) {
> > +			case ACPI_TYPE_INTEGER:
> > +			case ACPI_TYPE_STRING:
> > +			case ACPI_TYPE_LOCAL_REFERENCE:
> > +				continue;
> > +
> > +			default:
> > +				return false;
> > +			}
> > +
> > +		return true;
> > +	}
> > +	return false;
> > +}
> > +
> > +static bool acpi_properties_format_valid(const union acpi_object *properties)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < properties->package.count; i++) {
> > +		const union acpi_object *property;
> > +
> > +		property = &properties->package.elements[i];
> > +		/*
> > +		 * Only two elements allowed, the first one must be a string and
> > +		 * the second one has to satisfy certain conditions.
> > +		 */
> > +		if (property->package.count != 2
> > +		    || property->package.elements[0].type != ACPI_TYPE_STRING
> > +		    || !acpi_property_value_ok(&property->package.elements[1]))
> > +			return false;
> > +	}
> > +	return true;
> > +}
> > +
> > +void acpi_init_properties(struct acpi_device *adev)
> > +{
> > +
> 
> I think the above line is not needed.

Right, I'll remove it.

> With that fixed,
> 
> Reviewed-by: Hanjun Guo <hanjun.guo@xxxxxxxxxx>

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux