* [RFC PATCH 1/9] ACPI: Add support for device specific properties
2014-08-16 6:53 [RFC PATCH 0/9] Add ACPI _DSD and unified device properties support Mika Westerberg
@ 2014-08-16 6:53 ` Mika Westerberg
2014-08-16 6:53 ` [RFC PATCH 2/9] ACPI: Document ACPI " Mika Westerberg
` (10 subsequent siblings)
11 siblings, 0 replies; 39+ messages in thread
From: Mika Westerberg @ 2014-08-16 6:53 UTC (permalink / raw)
To: Darren Hart, Rafael J. Wysocki
Cc: Al Stone, Olof Johansson, Matthew Garrett, Matt Fleming,
David Woodhouse, H. Peter Anvin, Jacob Pan, Josh Triplett,
Aaron Lu, Max Eliaser, Robert Moore, Len Brown,
Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot, Mark Brown,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Grant Likely, Rob Herring, Mika Westerberg,
linux-acpi, devicetree, linux-kernel
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@linux.intel.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
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@linux.intel.com>
+ * Darren Hart <dvhart@linux.intel.com>
+ * Rafael J. Wysocki <rafael.j.wysocki@intel.com>
+ *
+ * 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] = {
+ 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)
+{
+
+ struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER };
+ const union acpi_object *desc;
+ acpi_status status;
+ int i;
+
+ status = acpi_evaluate_object_typed(adev->handle, "_DSD", NULL, &buf,
+ ACPI_TYPE_PACKAGE);
+ if (ACPI_FAILURE(status))
+ return;
+
+ desc = buf.pointer;
+ if (desc->package.count % 2)
+ goto fail;
+
+ /* Look for the device properties UUID. */
+ for (i = 0; i < desc->package.count; i += 2) {
+ const union acpi_object *uuid, *properties;
+
+ uuid = &desc->package.elements[i];
+ properties = &desc->package.elements[i + 1];
+
+ /*
+ * The first element must be a UUID and the second one must be
+ * a package.
+ */
+ if (uuid->type != ACPI_TYPE_BUFFER || uuid->buffer.length != 16
+ || properties->type != ACPI_TYPE_PACKAGE)
+ break;
+
+ if (memcmp(uuid->buffer.pointer, prp_uuid, sizeof(prp_uuid)))
+ continue;
+
+ /*
+ * We found the matching UUID. Now validate the format of the
+ * package immediately following it.
+ */
+ if (!acpi_properties_format_valid(properties))
+ break;
+
+ adev->data.pointer = buf.pointer;
+ adev->data.properties = properties;
+ return;
+ }
+
+ fail:
+ dev_warn(&adev->dev, "Returned _DSD data is not valid, skipping\n");
+ ACPI_FREE(buf.pointer);
+}
+
+void acpi_free_properties(struct acpi_device *adev)
+{
+ ACPI_FREE((void *)adev->data.pointer);
+ adev->data.pointer = NULL;
+ adev->data.properties = NULL;
+}
+
+/**
+ * acpi_dev_get_property - return an ACPI property with given name
+ * @adev: ACPI device to get property
+ * @name: Name of the property
+ * @type: Expected property type
+ * @obj: Location to store the property value (if not %NULL)
+ *
+ * Look up a property with @name and store a pointer to the resulting ACPI
+ * object at the location pointed to by @obj if found.
+ *
+ * Callers must not attempt to free the returned objects. These objects will be
+ * freed by the ACPI core automatically during the removal of @adev.
+ *
+ * Return: %0 if property with @name has been found (success),
+ * %-EINVAL if the arguments are invalid,
+ * %-ENODATA if the property doesn't exist,
+ * %-EPROTO if the property value type doesn't match @type.
+ */
+int acpi_dev_get_property(struct acpi_device *adev, const char *name,
+ acpi_object_type type, const union acpi_object **obj)
+{
+ const union acpi_object *properties;
+ int i;
+
+ if (!adev || !name)
+ return -EINVAL;
+
+ if (!adev->data.pointer || !adev->data.properties)
+ return -ENODATA;
+
+ properties = adev->data.properties;
+ for (i = 0; i < properties->package.count; i++) {
+ const union acpi_object *propname, *propvalue;
+ const union acpi_object *property;
+
+ property = &properties->package.elements[i];
+
+ propname = &property->package.elements[0];
+ propvalue = &property->package.elements[1];
+
+ if (!strcmp(name, propname->string.pointer)) {
+ if (type != ACPI_TYPE_ANY && propvalue->type != type)
+ return -EPROTO;
+ else if (obj)
+ *obj = propvalue;
+
+ return 0;
+ }
+ }
+ return -ENODATA;
+}
+EXPORT_SYMBOL_GPL(acpi_dev_get_property);
+
+/**
+ * acpi_dev_get_property_array - return an ACPI array property with given name
+ * @adev: ACPI device to get property
+ * @name: Name of the property
+ * @type: Expected type of array elements
+ * @obj: Location to store a pointer to the property value (if not NULL)
+ *
+ * Look up an array property with @name and store a pointer to the resulting
+ * ACPI object at the location pointed to by @obj if found.
+ *
+ * Callers must not attempt to free the returned objects. Those objects will be
+ * freed by the ACPI core automatically during the removal of @adev.
+ *
+ * Return: %0 if array property (package) with @name has been found (success),
+ * %-EINVAL if the arguments are invalid,
+ * %-ENODATA if the property doesn't exist,
+ * %-EPROTO if the property is not a package or the type of its elements
+ * doesn't match @type.
+ */
+int acpi_dev_get_property_array(struct acpi_device *adev, const char *name,
+ acpi_object_type type,
+ const union acpi_object **obj)
+{
+ const union acpi_object *prop;
+ int ret, i;
+
+ ret = acpi_dev_get_property(adev, name, ACPI_TYPE_PACKAGE, &prop);
+ if (ret)
+ return ret;
+
+ if (type != ACPI_TYPE_ANY) {
+ /* Check that all elements are of correct type. */
+ for (i = 0; i < prop->package.count; i++)
+ if (prop->package.elements[i].type != type)
+ return -EPROTO;
+ }
+ if (obj)
+ *obj = prop;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(acpi_dev_get_property_array);
+
+/**
+ * acpi_dev_get_property_reference - returns handle to the referenced object
+ * @adev: ACPI device to get property
+ * @name: Name of the property
+ * @size_prop: Name of the "size" property in referenced object
+ * @index: Index of the reference to return
+ * @args: Location to store the returned reference with optional arguments
+ *
+ * Find property with @name, verifify that it is a package containing at least
+ * one object reference and if so, store the ACPI device object pointer to the
+ * target object in @args->adev.
+ *
+ * If the reference includes arguments (@size_prop is not %NULL) follow the
+ * reference and check whether or not there is an integer property @size_prop
+ * under the target object and if so, whether or not its value matches the
+ * number of arguments that follow the reference. If there's more than one
+ * reference in the property value package, @index is used to select the one to
+ * return.
+ *
+ * Return: %0 on success, negative error code on failure.
+ */
+int acpi_dev_get_property_reference(struct acpi_device *adev, const char *name,
+ const char *size_prop, size_t index,
+ struct acpi_reference_args *args)
+{
+ const union acpi_object *element, *end;
+ const union acpi_object *obj;
+ struct acpi_device *device;
+ int ret, idx = 0;
+
+ ret = acpi_dev_get_property(adev, name, ACPI_TYPE_ANY, &obj);
+ if (ret)
+ return ret;
+
+ /*
+ * The simplest case is when the value is a single reference. Just
+ * return that reference then.
+ */
+ if (obj->type == ACPI_TYPE_LOCAL_REFERENCE) {
+ if (size_prop || index)
+ return -EINVAL;
+
+ ret = acpi_bus_get_device(obj->reference.handle, &device);
+ if (ret)
+ return ret;
+
+ args->adev = device;
+ args->nargs = 0;
+ return 0;
+ }
+
+ /*
+ * If it is not a single reference, then it is a package of
+ * references followed by number of ints as follows:
+ *
+ * Package () { REF, INT, REF, INT, INT }
+ *
+ * The index argument is then used to determine which reference
+ * the caller wants (along with the arguments).
+ */
+ if (obj->type != ACPI_TYPE_PACKAGE || index >= obj->package.count)
+ return -EPROTO;
+
+ element = obj->package.elements;
+ end = element + obj->package.count;
+
+ while (element < end) {
+ u32 nargs, i;
+
+ if (element->type != ACPI_TYPE_LOCAL_REFERENCE)
+ return -EPROTO;
+
+ ret = acpi_bus_get_device(element->reference.handle, &device);
+ if (ret)
+ return -ENODEV;
+
+ element++;
+ nargs = 0;
+
+ if (size_prop) {
+ const union acpi_object *prop;
+
+ /*
+ * Find out how many arguments the refenced object
+ * expects by reading its size_prop property.
+ */
+ ret = acpi_dev_get_property(device, size_prop,
+ ACPI_TYPE_INTEGER, &prop);
+ if (ret)
+ return ret;
+
+ nargs = prop->integer.value;
+ if (nargs > MAX_ACPI_REFERENCE_ARGS
+ || element + nargs > end)
+ return -EPROTO;
+
+ /*
+ * Skip to the start of the arguments and verify
+ * that they all are in fact integers.
+ */
+ for (i = 0; i < nargs; i++)
+ if (element[i].type != ACPI_TYPE_INTEGER)
+ return -EPROTO;
+ } else {
+ /* assume following integer elements are all args */
+ for (i = 0; element + i < end; i++) {
+ int type = element[i].type;
+
+ if (type == ACPI_TYPE_INTEGER)
+ nargs++;
+ else if (type == ACPI_TYPE_LOCAL_REFERENCE)
+ break;
+ else
+ return -EPROTO;
+ }
+ }
+
+ if (idx++ == index) {
+ args->adev = device;
+ args->nargs = nargs;
+ for (i = 0; i < nargs; i++)
+ args->args[i] = element[i].integer.value;
+
+ return 0;
+ }
+
+ element += nargs;
+ }
+
+ return -EPROTO;
+}
+EXPORT_SYMBOL_GPL(acpi_dev_get_property_reference);
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index f775fa0d850f..fc97e6123864 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -880,6 +880,7 @@ static void acpi_device_release(struct device *dev)
{
struct acpi_device *acpi_dev = to_acpi_device(dev);
+ acpi_free_properties(acpi_dev);
acpi_free_pnp_ids(&acpi_dev->pnp);
acpi_free_power_resources_lists(acpi_dev);
kfree(acpi_dev);
@@ -1876,6 +1877,7 @@ void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
acpi_set_device_status(device, sta);
acpi_device_get_busid(device);
acpi_set_pnp_ids(handle, &device->pnp, type);
+ acpi_init_properties(device);
acpi_bus_get_flags(device);
device->flags.match_driver = false;
device->flags.initialized = true;
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index b5714580801a..348bd260a6b4 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -331,6 +331,12 @@ struct acpi_device_physical_node {
bool put_online:1;
};
+/* ACPI Device Specific Data (_DSD) */
+struct acpi_device_data {
+ const union acpi_object *pointer;
+ const union acpi_object *properties;
+};
+
/* Device */
struct acpi_device {
int device_type;
@@ -347,6 +353,7 @@ struct acpi_device {
struct acpi_device_wakeup wakeup;
struct acpi_device_perf performance;
struct acpi_device_dir dir;
+ struct acpi_device_data data;
struct acpi_scan_handler *handler;
struct acpi_hotplug_context *hp;
struct acpi_driver *driver;
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 358c01b971db..9181d78eb7c4 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -649,4 +649,44 @@ do { \
#endif
#endif
+/* Device properties */
+
+#define MAX_ACPI_REFERENCE_ARGS 8
+struct acpi_reference_args {
+ struct acpi_device *adev;
+ size_t nargs;
+ u64 args[MAX_ACPI_REFERENCE_ARGS];
+};
+
+#ifdef CONFIG_ACPI
+int acpi_dev_get_property(struct acpi_device *adev, const char *name,
+ acpi_object_type type, const union acpi_object **obj);
+int acpi_dev_get_property_array(struct acpi_device *adev, const char *name,
+ acpi_object_type type,
+ const union acpi_object **obj);
+int acpi_dev_get_property_reference(struct acpi_device *adev, const char *name,
+ const char *cells_name, size_t index,
+ struct acpi_reference_args *args);
+#else
+static inline int acpi_dev_get_property(struct acpi_device *adev,
+ const char *name, acpi_object_type type,
+ const union acpi_object **obj)
+{
+ return -ENOSYS;
+}
+static inline int acpi_dev_get_property_array(struct acpi_device *adev,
+ const char *name,
+ acpi_object_type type,
+ const union acpi_object **obj)
+{
+ return -ENOSYS;
+}
+static inline int acpi_dev_get_property_reference(struct acpi_device *adev,
+ const char *name, const char *cells_name,
+ size_t index, struct acpi_reference_args *args)
+{
+ return -ENOSYS;
+}
+#endif
+
#endif /*_LINUX_ACPI_H*/
--
2.1.0.rc1
^ permalink raw reply related [flat|nested] 39+ messages in thread* [RFC PATCH 2/9] ACPI: Document ACPI device specific properties
2014-08-16 6:53 [RFC PATCH 0/9] Add ACPI _DSD and unified device properties support Mika Westerberg
2014-08-16 6:53 ` [RFC PATCH 1/9] ACPI: Add support for device specific properties Mika Westerberg
@ 2014-08-16 6:53 ` Mika Westerberg
2014-08-16 6:53 ` [RFC PATCH 3/9] Driver core: Unified device properties interface for platform firmware Mika Westerberg
` (9 subsequent siblings)
11 siblings, 0 replies; 39+ messages in thread
From: Mika Westerberg @ 2014-08-16 6:53 UTC (permalink / raw)
To: Darren Hart, Rafael J. Wysocki
Cc: Al Stone, Olof Johansson, Matthew Garrett, Matt Fleming,
David Woodhouse, H. Peter Anvin, Jacob Pan, Josh Triplett,
Aaron Lu, Max Eliaser, Robert Moore, Len Brown,
Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot, Mark Brown,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Grant Likely, Rob Herring, Mika Westerberg,
linux-acpi, devicetree, linux-kernel
This document describes the data format and interfaces of ACPI device
specific properties.
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Darren Hart <dvhart@linux.intel.com>
---
Documentation/acpi/properties.txt | 359 ++++++++++++++++++++++++++++++++++++++
1 file changed, 359 insertions(+)
create mode 100644 Documentation/acpi/properties.txt
diff --git a/Documentation/acpi/properties.txt b/Documentation/acpi/properties.txt
new file mode 100644
index 000000000000..a1e93267c1fa
--- /dev/null
+++ b/Documentation/acpi/properties.txt
@@ -0,0 +1,359 @@
+ACPI device properties
+======================
+This document describes the format and interfaces of ACPI device
+properties as specified in "Device Properties UUID For _DSD" available
+here:
+
+http://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf
+
+1. Introduction
+---------------
+In systems that use ACPI and want to take advantage of device specific
+properties, there needs to be a standard way to return and extract
+name-value pairs for a given ACPI device.
+
+An ACPI device that wants to export its properties must implement a
+static name called _DSD that takes no arguments and returns a package of
+packages:
+
+ Name (_DSD, Package () {
+ ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+ Package () {
+ Package () {"name1", <VALUE1>},
+ Package () {"name2", <VALUE2>}
+ }
+ })
+
+The UUID identifies contents of the following package. In case of ACPI
+device properties it is daffd814-6eba-4d8c-8a91-bc9bbf4aa301.
+
+In each returned package, the first item is the name and must be a string.
+The corresponding value can be a string, integer, reference, or package. If
+a package it may only contain strings, integers, and references.
+
+An example device where we might need properties is a GPIO keys device.
+In addition to the GpioIo/GpioInt resources the driver needs to know how
+to map each GpioIo resource to the corresponding Linux input event.
+
+To solve this we add the following ACPI device properties from the
+gpio-keys schema:
+
+ Device (KEYS) {
+ Name (_DSD, Package () {
+ ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+ Package () {
+ Package () {"poll-interval", 100},
+ Package () {"autorepeat", 1}
+ }
+ })
+
+ Device (BTN0) {
+ Name (_DSD, Package () {
+ ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+ Package () {
+ Package () {"linux,code", 105},
+ Package () {"linux,input-type", 1},
+ ...
+ }
+ })
+ }
+
+ ...
+ }
+
+Of course the device driver then needs to iterate over these devices but
+it can be done easily via the ->children field of the companion ACPI
+device. This will be demonstrated later on in the document.
+
+If there is an existing Device Tree binding for a device, it is expected
+that the same bindings are used with ACPI properties, so that the driver
+dealing with the device needs only minor modifications if any.
+
+2. Formal definition of properties
+----------------------------------
+The following chapters define the currently supported properties. For
+these there exists a helper function that can be used to extract the
+property value.
+
+2.1 Integer types
+-----------------
+ACPI integers are always 64-bit. However, for drivers the full range is
+typically not needed so we provide a set of functions which convert the
+64-bit integer to a smaller Linux integer type.
+
+An integer property looks like this:
+
+ Package () {"poll-interval", 100},
+ Package () {"i2c-sda-hold-time-ns", 300},
+ Package () {"clock-frequency", 400000},
+
+To read a property value, use a unified property accessor as shown
+below:
+
+ u32 val;
+ int ret;
+
+ ret = device_property_read(dev, "poll-interval", DEV_PROP_U32, &val);
+ if (ret)
+ /* Handle error */
+
+The function returns 0 if the property is copied to 'val' or negative
+errno if something went wrong (or the property does not exist).
+
+2.2 Integer arrays
+------------------
+An integer array is a package holding only integers. Arrays can be used to
+represent different things like Linux input key codes to GPIO mappings, pin
+control settings, dma request lines, etc.
+
+An integer array looks like this:
+
+ Package () {
+ "max8952,dvs-mode-microvolt",
+ Package () {
+ 1250000,
+ 1200000,
+ 1050000,
+ 950000,
+ }
+ }
+
+The above array property can be accessed like:
+
+ u32 voltages[4];
+ int ret;
+
+ ret = device_property_read_array(dev, "max8952,dvs-mode-microvolt",
+ DEV_PROP_U32, voltages,
+ ARRAY_SIZE(voltages));
+ if (ret)
+ /* Handle error */
+
+
+All functions copy the resulting values cast to a requested type to the
+caller supplied array. If you pass NULL in the value pointer ('voltages' in
+this case), the function returns number of items in the array. This can be
+useful if caller does not know size of the array beforehand.
+
+2.3 Strings
+-----------
+String properties can be used to describe many things like labels for GPIO
+buttons, compability ids, etc.
+
+A string property looks like this:
+
+ Package () {"pwm-names", "backlight"},
+ Package () {"label", "Status-LED"},
+
+You can also use device_property_read() to extract strings:
+
+ const char *val;
+ int ret;
+
+ ret = device_property_read(dev, "label", DEV_PROP_STRING, &val);
+ if (ret)
+ /* Handle error */
+
+Note that the function does not copy the returned string but instead the
+value is modified to point to the string property itself.
+
+The memory is owned by the associated ACPI device object and released
+when it is removed. The user need not free the associated memory.
+
+2.4 String arrays
+-----------------
+String arrays can be useful in describing a list of labels, names for
+DMA channels, etc.
+
+A string array property looks like this:
+
+ Package () {"dma-names", Package () {"tx", "rx", "rx-tx"}},
+ Package () {"clock-output-names", Package () {"pll", "pll-switched"}},
+
+And these can be read in similar way that the integer arrrays:
+
+ const char *dma_names[3];
+ int ret;
+
+ ret = device_property_read_array(dev, "dma-names", DEV_PROP_STRING,
+ dma_names, ARRAY_SIZE(dma_names));
+ if (ret)
+ /* Handle error */
+
+The memory management rules follow what is specified for single strings.
+Specifically the returned pointers should be treated as constant and not to
+be freed. That is done automatically when the correspondig ACPI device
+object is released.
+
+2.5 Object references
+---------------------
+An ACPI object reference is used to refer to some object in the
+namespace. For example, if a device has dependencies with some other
+object, an object reference can be used.
+
+An object reference looks like this:
+
+ Package () {"clock", \_SB.CLK0},
+
+At the time of writing this, there is no unified device_property_* accessor
+for references so one needs to use the following ACPI helper function:
+
+ int acpi_dev_get_property_reference(struct acpi_device *adev,
+ const char *name,
+ const char *size_prop, int index,
+ struct acpi_reference_args *args);
+
+The referenced ACPI device is returned in args->adev if found.
+
+In addition to simple object references it is also possible to have object
+references with arguments. These are represented in ASL as follows:
+
+ Device (\_SB.PCI0.PWM) {
+ Name (_DSD, Package () {
+ ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+ Package () {
+ Package () {"#pwm-cells", 2}
+ }
+ })
+ }
+
+ Device (\_SB.PCI0.BL) {
+ Name (_DSD, Package () {
+ ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+ Package () {
+ Package () {
+ "pwms",
+ Package () {
+ \_SB.PCI0.PWM, 0, 5000000,
+ \_SB.PCI0.PWM, 1, 4500000,
+ }
+ }
+ }
+ })
+ }
+
+In the above example, the referenced device declares a property that
+returns the number of expected arguments (here it is "#pwm-cells"). If
+no such property is given we assume that all the integers following the
+reference are arguments.
+
+In the above example PWM device expects 2 additional arguments. This
+will be validated by the ACPI property core.
+
+The additional arguments must be integers. Nothing else is supported.
+
+It is possible, as in the above example, to have multiple references
+with varying number of integer arguments. It is up to the referenced
+device to declare how many arguments it expects. The 'index' parameter
+selects which reference is returned.
+
+One can use acpi_dev_get_property_reference() as well to extract the
+information in additional parameters:
+
+ struct acpi_reference_args args;
+ struct acpi_device *adev = /* this will point to the BL device */
+ int ret;
+
+ /* extract the first reference */
+ acpi_dev_get_property_reference(adev, "pwms", "#pwm-cells", 0, &args);
+
+ BUG_ON(args.nargs != 2);
+ BUG_ON(args.args[0] != 0);
+ BUG_ON(args.args[1] != 5000000);
+
+ /* extract the second reference */
+ acpi_dev_get_property_reference(adev, "pwms", "#pwm-cells", 1, &args);
+
+ BUG_ON(args.nargs != 2);
+ BUG_ON(args.args[0] != 1);
+ BUG_ON(args.args[1] != 4500000);
+
+In addition to arguments, args.adev now points to the ACPI device that
+corresponds to \_SB.PCI0.PWM.
+
+It is intended that this function is not used directly but instead
+subsystems like pwm implement their ACPI support on top of this function
+in such way that it is hidden from the client drivers, such as via
+pwm_get().
+
+3. Device property hierarchies
+------------------------------
+Devices are organized in a tree within the Linux kernel. It follows that
+the configuration data would also be hierarchical. In order to reach
+equivalence with Device Tree, the ACPI mechanism must also provide some
+sort of tree-like representation. Fortunately, the ACPI namespace is
+already such a structure.
+
+For example, we could have the following device in ACPI namespace. The
+KEYS device is much like gpio_keys_polled.c in that it includes "pseudo"
+devices for each GPIO:
+
+ Device (KEYS) {
+ Name (_CRS, ResourceTemplate () {
+ GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly,
+ "\\_SB.PCI0.LPC", 0, ResourceConsumer,,) { 0 }
+ GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly,
+ "\\_SB.PCI0.LPC", 0, ResourceConsumer,,) { 1 }
+ ...
+ })
+
+ Name (_DSD, Package () {
+ ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+ Package () {
+ // Properties for KEYS device
+ Package () {"#gpio-cells", 1}
+ }
+ })
+
+ // "pseudo" devices declared under the parent device
+ Device (BTN0) {
+ Name (_DSD, Package () {
+ ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+ Package () {
+ Package () {"label", "minnow_btn0"}
+ Package () {"gpios", Package () { ^KEYS, 0 0 1 }}
+ }
+ })
+ }
+
+ Device (BTN1) {
+ Name (_DSD, Package () {
+ ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+ Package () {
+ Package () {"label", "minnow_btn1"}
+ Package () {"gpios", Package () { ^KEYS, 1 0 1 }}
+ }
+ })
+ }
+ }
+
+We can extract the above in gpio_keys_polled.c like:
+
+ void gpio_keys_polled_acpi_probe(struct device *dev)
+ {
+ struct acpi_device *adev = ACPI_COMPANION(dev);
+ struct acpi_device *button;
+
+ /* Properties for the KEYS device itself */
+ device_property_read(dev, ...);
+ /* or acpi_dev_get_property(adev, ...) */
+
+ /*
+ * Iterate over button devices and extract their
+ * configuration. Here we need to use ACPI specific
+ * functions instead because the pseudo devices don't have
+ * physical device attached to them.
+ */
+ list_for_each_entry(button, &adev->children, node) {
+ const union acpi_object *label;
+
+ acpi_dev_get_property(button, "label",
+ ACPI_TYPE_STRING, &label);
+ dev_info(dev, "label: %s\n", label->string.pointer);
+ }
+ }
+
+Note that you still need proper error handling which is omitted in the
+above example, and also if possible instead of calling to ACPI specific
+functions, one should add the support to the subsystem in question which
+can then provide this information to the drivers in more generic way.
--
2.1.0.rc1
^ permalink raw reply related [flat|nested] 39+ messages in thread* [RFC PATCH 3/9] Driver core: Unified device properties interface for platform firmware
2014-08-16 6:53 [RFC PATCH 0/9] Add ACPI _DSD and unified device properties support Mika Westerberg
2014-08-16 6:53 ` [RFC PATCH 1/9] ACPI: Add support for device specific properties Mika Westerberg
2014-08-16 6:53 ` [RFC PATCH 2/9] ACPI: Document ACPI " Mika Westerberg
@ 2014-08-16 6:53 ` Mika Westerberg
2014-08-16 6:53 ` [RFC PATCH 4/9] of: Add property_ops callback for devices with of_node Mika Westerberg
` (8 subsequent siblings)
11 siblings, 0 replies; 39+ messages in thread
From: Mika Westerberg @ 2014-08-16 6:53 UTC (permalink / raw)
To: Darren Hart, Rafael J. Wysocki
Cc: Al Stone, Olof Johansson, Matthew Garrett, Matt Fleming,
David Woodhouse, H. Peter Anvin, Jacob Pan, Josh Triplett,
Aaron Lu, Max Eliaser, Robert Moore, Len Brown,
Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot, Mark Brown,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Grant Likely, Rob Herring, Mika Westerberg,
linux-acpi, devicetree, linux-kernel
From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Add a uniform interface by which device drivers can request device
properties from the platform firmware by providing a property name
and the corresponding data type.
Three general helper functions, device_property_get(),
device_property_read() and device_property_read_array() are provided.
The first one allows the raw value of a given device property to be
accessed by the driver. The remaining two allow the value of a numeric
or string property and multiple numeric or string values of one array
property to be acquired, respectively.
The interface is supposed to cover both ACPI and Device Trees,
although the ACPI part is only implemented at this time.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Aaron Lu <aaron.lu@intel.com>
Reviewed-by: Darren Hart <dvhart@linux.intel.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
drivers/acpi/glue.c | 4 +-
drivers/acpi/property.c | 179 ++++++++++++++++++++++++++++++++++++++++++++++-
drivers/acpi/scan.c | 12 +++-
drivers/base/Makefile | 2 +-
drivers/base/property.c | 48 +++++++++++++
include/acpi/acpi_bus.h | 1 +
include/linux/device.h | 3 +
include/linux/property.h | 50 +++++++++++++
8 files changed, 293 insertions(+), 6 deletions(-)
create mode 100644 drivers/base/property.c
create mode 100644 include/linux/property.h
diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
index f774c65ecb8b..1df5223bb98d 100644
--- a/drivers/acpi/glue.c
+++ b/drivers/acpi/glue.c
@@ -9,7 +9,7 @@
#include <linux/export.h>
#include <linux/init.h>
#include <linux/list.h>
-#include <linux/device.h>
+#include <linux/property.h>
#include <linux/slab.h>
#include <linux/rwsem.h>
#include <linux/acpi.h>
@@ -220,6 +220,7 @@ int acpi_bind_one(struct device *dev, struct acpi_device *acpi_dev)
list_add(&physical_node->node, physnode_list);
acpi_dev->physical_node_count++;
+ dev->property_ops = &acpi_property_ops;
if (!ACPI_COMPANION(dev))
ACPI_COMPANION_SET(dev, acpi_dev);
@@ -271,6 +272,7 @@ int acpi_unbind_one(struct device *dev)
acpi_physnode_link_name(physnode_name, entry->node_id);
sysfs_remove_link(&acpi_dev->dev.kobj, physnode_name);
sysfs_remove_link(&dev->kobj, "firmware_node");
+ dev->property_ops = NULL;
ACPI_COMPANION_SET(dev, NULL);
/* Drop references taken by acpi_bind_one(). */
put_device(dev);
diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 093e834c35db..bdba5f85c919 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -13,8 +13,8 @@
* published by the Free Software Foundation.
*/
+#include <linux/property.h>
#include <linux/acpi.h>
-#include <linux/device.h>
#include <linux/export.h>
#include "internal.h"
@@ -363,3 +363,180 @@ int acpi_dev_get_property_reference(struct acpi_device *adev, const char *name,
return -EPROTO;
}
EXPORT_SYMBOL_GPL(acpi_dev_get_property_reference);
+
+static int acpi_dev_prop_get(struct device *dev, const char *propname,
+ void **valptr)
+{
+ return acpi_dev_get_property(ACPI_COMPANION(dev), propname,
+ ACPI_TYPE_ANY,
+ (const union acpi_object **)valptr);
+}
+
+static int acpi_dev_prop_read(struct device *dev, const char *propname,
+ enum dev_prop_type proptype, void *val)
+{
+ const union acpi_object *obj;
+ int ret = -EINVAL;
+
+ if (!val)
+ return -EINVAL;
+
+ if (proptype >= DEV_PROP_U8 && proptype <= DEV_PROP_U64) {
+ ret = acpi_dev_get_property(ACPI_COMPANION(dev), propname,
+ ACPI_TYPE_INTEGER, &obj);
+ if (ret)
+ return ret;
+
+ switch (proptype) {
+ case DEV_PROP_U8:
+ *(u8 *)val = obj->integer.value;
+ break;
+ case DEV_PROP_U16:
+ *(u16 *)val = obj->integer.value;
+ break;
+ case DEV_PROP_U32:
+ *(u32 *)val = obj->integer.value;
+ break;
+ default:
+ *(u64 *)val = obj->integer.value;
+ break;
+ }
+ } else if (proptype == DEV_PROP_STRING) {
+ ret = acpi_dev_get_property(ACPI_COMPANION(dev), propname,
+ ACPI_TYPE_STRING, &obj);
+ if (ret)
+ return ret;
+
+ *(char **)val = obj->string.pointer;
+ }
+ return ret;
+}
+
+static int acpi_copy_property_array_u8(const union acpi_object *items, u8 *val,
+ size_t nval)
+{
+ int i;
+
+ for (i = 0; i < nval; i++) {
+ if (items[i].type != ACPI_TYPE_INTEGER)
+ return -EPROTO;
+
+ val[i] = items[i].integer.value;
+ }
+ return 0;
+}
+
+static int acpi_copy_property_array_u16(const union acpi_object *items,
+ u16 *val, size_t nval)
+{
+ int i;
+
+ for (i = 0; i < nval; i++) {
+ if (items[i].type != ACPI_TYPE_INTEGER)
+ return -EPROTO;
+
+ val[i] = items[i].integer.value;
+ }
+ return 0;
+}
+
+static int acpi_copy_property_array_u32(const union acpi_object *items,
+ u32 *val, size_t nval)
+{
+ int i;
+
+ for (i = 0; i < nval; i++) {
+ if (items[i].type != ACPI_TYPE_INTEGER)
+ return -EPROTO;
+
+ val[i] = items[i].integer.value;
+ }
+ return 0;
+}
+
+static int acpi_copy_property_array_u64(const union acpi_object *items,
+ u64 *val, size_t nval)
+{
+ int i;
+
+ for (i = 0; i < nval; i++) {
+ if (items[i].type != ACPI_TYPE_INTEGER)
+ return -EPROTO;
+
+ val[i] = items[i].integer.value;
+ }
+ return 0;
+}
+
+static int acpi_copy_property_array_string(const union acpi_object *items,
+ char **val, size_t nval)
+{
+ int i;
+
+ for (i = 0; i < nval; i++) {
+ if (items[i].type != ACPI_TYPE_STRING)
+ return -EPROTO;
+
+ val[i] = items[i].string.pointer;
+ }
+ return 0;
+}
+
+static int acpi_dev_prop_read_array(struct device *dev, const char *propname,
+ enum dev_prop_type proptype, void *val,
+ size_t nval)
+{
+ const union acpi_object *obj;
+ const union acpi_object *items;
+ int ret;
+
+ ret = acpi_dev_get_property_array(ACPI_COMPANION(dev), propname,
+ ACPI_TYPE_ANY, &obj);
+ if (ret)
+ return ret;
+
+ if (!val)
+ return obj->package.count;
+
+ if (nval > obj->package.count)
+ nval = obj->package.count;
+
+ items = obj->package.elements;
+ switch (proptype) {
+ case DEV_PROP_U8:
+ ret = acpi_copy_property_array_u8(items, (u8 *)val, nval);
+ break;
+ case DEV_PROP_U16:
+ ret = acpi_copy_property_array_u16(items, (u16 *)val, nval);
+ break;
+ case DEV_PROP_U32:
+ ret = acpi_copy_property_array_u32(items, (u32 *)val, nval);
+ break;
+ case DEV_PROP_U64:
+ ret = acpi_copy_property_array_u64(items, (u64 *)val, nval);
+ break;
+ case DEV_PROP_STRING:
+ ret = acpi_copy_property_array_string(items, (char **)val, nval);
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+ return ret;
+}
+
+static int acpi_dev_get_child_count(struct device *dev)
+{
+ struct acpi_device *adev = ACPI_COMPANION(dev);
+
+ if (!adev)
+ return -EINVAL;
+ return adev->child_count;
+}
+
+struct dev_prop_ops acpi_property_ops = {
+ .get = acpi_dev_prop_get,
+ .read = acpi_dev_prop_read,
+ .read_array = acpi_dev_prop_read_array,
+ .child_count = acpi_dev_get_child_count,
+};
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index fc97e6123864..75b1b91e5ab0 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1035,8 +1035,10 @@ struct bus_type acpi_bus_type = {
static void acpi_device_del(struct acpi_device *device)
{
mutex_lock(&acpi_device_lock);
- if (device->parent)
+ if (device->parent) {
list_del(&device->node);
+ device->parent->child_count--;
+ }
list_del(&device->wakeup_list);
mutex_unlock(&acpi_device_lock);
@@ -1222,8 +1224,10 @@ int acpi_device_add(struct acpi_device *device,
}
dev_set_name(&device->dev, "%s:%02x", acpi_device_bus_id->bus_id, acpi_device_bus_id->instance_no);
- if (device->parent)
+ if (device->parent) {
list_add_tail(&device->node, &device->parent->children);
+ device->parent->child_count++;
+ }
if (device->wakeup.flags.valid)
list_add_tail(&device->wakeup_list, &acpi_wakeup_device_list);
@@ -1248,8 +1252,10 @@ int acpi_device_add(struct acpi_device *device,
err:
mutex_lock(&acpi_device_lock);
- if (device->parent)
+ if (device->parent) {
list_del(&device->node);
+ device->parent->child_count--;
+ }
list_del(&device->wakeup_list);
mutex_unlock(&acpi_device_lock);
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 04b314e0fa51..1acd294b6514 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -4,7 +4,7 @@ obj-y := component.o core.o bus.o dd.o syscore.o \
driver.o class.o platform.o \
cpu.o firmware.o init.o map.o devres.o \
attribute_container.o transport_class.o \
- topology.o container.o
+ topology.o container.o property.o
obj-$(CONFIG_DEVTMPFS) += devtmpfs.o
obj-$(CONFIG_DMA_CMA) += dma-contiguous.o
obj-y += power/
diff --git a/drivers/base/property.c b/drivers/base/property.c
new file mode 100644
index 000000000000..d770b6edc91a
--- /dev/null
+++ b/drivers/base/property.c
@@ -0,0 +1,48 @@
+/*
+ * property.c - Unified device property interface.
+ *
+ * Copyright (C) 2014, Intel Corporation
+ * All rights reserved.
+ *
+ * Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
+ *
+ * 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/property.h>
+#include <linux/export.h>
+
+int device_property_get(struct device *dev, const char *propname, void **valptr)
+{
+ return dev->property_ops && dev->property_ops->get ?
+ dev->property_ops->get(dev, propname, valptr) : -ENODATA;
+}
+EXPORT_SYMBOL_GPL(device_property_get);
+
+int device_property_read(struct device *dev, const char *propname,
+ enum dev_prop_type proptype, void *val)
+{
+ return dev->property_ops && dev->property_ops->read ?
+ dev->property_ops->read(dev, propname, proptype, val) :
+ -ENODATA;
+}
+EXPORT_SYMBOL_GPL(device_property_read);
+
+int device_property_read_array(struct device *dev, const char *propname,
+ enum dev_prop_type proptype, void *val,
+ size_t nval)
+{
+ return dev->property_ops && dev->property_ops->read_array ?
+ dev->property_ops->read_array(dev, propname, proptype, val, nval) :
+ -ENODATA;
+}
+EXPORT_SYMBOL_GPL(device_property_read_array);
+
+int device_property_child_count(struct device *dev)
+{
+ return dev->property_ops && dev->property_ops->child_count ?
+ dev->property_ops->child_count(dev) : -ENODATA;
+}
+EXPORT_SYMBOL_GPL(device_property_child_count);
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 348bd260a6b4..312813f60a22 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -340,6 +340,7 @@ struct acpi_device_data {
/* Device */
struct acpi_device {
int device_type;
+ int child_count;
acpi_handle handle; /* no handle for fixed hardware */
struct acpi_device *parent;
struct list_head children;
diff --git a/include/linux/device.h b/include/linux/device.h
index af424acd393d..ea4cf31f8b9e 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -38,6 +38,7 @@ struct class;
struct subsys_private;
struct bus_type;
struct device_node;
+struct dev_prop_ops;
struct iommu_ops;
struct iommu_group;
@@ -701,6 +702,7 @@ struct acpi_dev_node {
* @archdata: For arch-specific additions.
* @of_node: Associated device tree node.
* @acpi_node: Associated ACPI device node.
+ * @property_ops: Firmware interface for device properties
* @devt: For creating the sysfs "dev".
* @id: device instance
* @devres_lock: Spinlock to protect the resource of the device.
@@ -777,6 +779,7 @@ struct device {
struct device_node *of_node; /* associated device tree node */
struct acpi_dev_node acpi_node; /* associated ACPI device node */
+ struct dev_prop_ops *property_ops;
dev_t devt; /* dev_t, creates the sysfs "dev" */
u32 id; /* device instance */
diff --git a/include/linux/property.h b/include/linux/property.h
new file mode 100644
index 000000000000..52ea7fe7fe09
--- /dev/null
+++ b/include/linux/property.h
@@ -0,0 +1,50 @@
+/*
+ * property.h - Unified device property interface.
+ *
+ * Copyright (C) 2014, Intel Corporation
+ * Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
+ *
+ * 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.
+ */
+
+#ifndef _LINUX_PROPERTY_H_
+#define _LINUX_PROPERTY_H_
+
+#include <linux/device.h>
+#include <linux/acpi.h>
+#include <linux/of.h>
+
+enum dev_prop_type {
+ DEV_PROP_U8,
+ DEV_PROP_U16,
+ DEV_PROP_U32,
+ DEV_PROP_U64,
+ DEV_PROP_STRING,
+ DEV_PROP_MAX,
+};
+
+struct dev_prop_ops {
+ int (*get)(struct device *dev, const char *propname, void **valptr);
+ int (*read)(struct device *dev, const char *propname,
+ enum dev_prop_type proptype, void *val);
+ int (*read_array)(struct device *dev, const char *propname,
+ enum dev_prop_type proptype, void *val, size_t nval);
+ int (*child_count)(struct device *dev);
+};
+
+#ifdef CONFIG_ACPI
+extern struct dev_prop_ops acpi_property_ops;
+#endif
+
+int device_property_get(struct device *dev, const char *propname,
+ void **valptr);
+int device_property_read(struct device *dev, const char *propname,
+ enum dev_prop_type proptype, void *val);
+int device_property_read_array(struct device *dev, const char *propname,
+ enum dev_prop_type proptype, void *val,
+ size_t nval);
+int device_property_child_count(struct device *dev);
+
+#endif /* _LINUX_PROPERTY_H_ */
--
2.1.0.rc1
^ permalink raw reply related [flat|nested] 39+ messages in thread* [RFC PATCH 4/9] of: Add property_ops callback for devices with of_node
2014-08-16 6:53 [RFC PATCH 0/9] Add ACPI _DSD and unified device properties support Mika Westerberg
` (2 preceding siblings ...)
2014-08-16 6:53 ` [RFC PATCH 3/9] Driver core: Unified device properties interface for platform firmware Mika Westerberg
@ 2014-08-16 6:53 ` Mika Westerberg
2014-08-16 6:53 ` [RFC PATCH 5/9] mfd: Add ACPI support Mika Westerberg
` (7 subsequent siblings)
11 siblings, 0 replies; 39+ messages in thread
From: Mika Westerberg @ 2014-08-16 6:53 UTC (permalink / raw)
To: Darren Hart, Rafael J. Wysocki
Cc: Al Stone, Olof Johansson, Matthew Garrett, Matt Fleming,
David Woodhouse, H. Peter Anvin, Jacob Pan, Josh Triplett,
Aaron Lu, Max Eliaser, Robert Moore, Len Brown,
Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot, Mark Brown,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Grant Likely, Rob Herring, Mika Westerberg,
linux-acpi, devicetree, linux-kernel
From: Aaron Lu <aaron.lu@intel.com>
With the unified device properties interface in place, add device tree support.
By adding the dev_prop_ops for of_node devices, drivers can access properties
from ACPI or Device Tree in a generic way.
Signed-off-by: Aaron Lu <aaron.lu@intel.com>
Reviewed-by: Darren Hart <dvhart@linux.intel.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
drivers/of/base.c | 194 ++++++++++++++++++++++++++++++++++++++++++++++-
drivers/of/platform.c | 4 +-
include/linux/property.h | 4 +
3 files changed, 200 insertions(+), 2 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index b9864806e9b8..527004d01423 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -20,7 +20,7 @@
#include <linux/ctype.h>
#include <linux/cpu.h>
#include <linux/module.h>
-#include <linux/of.h>
+#include <linux/property.h>
#include <linux/of_graph.h>
#include <linux/spinlock.h>
#include <linux/slab.h>
@@ -1343,6 +1343,39 @@ int of_property_read_u64(const struct device_node *np, const char *propname,
EXPORT_SYMBOL_GPL(of_property_read_u64);
/**
+ * of_property_read_u64_array - Find and read an array of 64 bit integers
+ * from a property.
+ *
+ * @np: device node from which the property value is to be read.
+ * @propname: name of the property to be searched.
+ * @out_values: pointer to return value, modified only if return value is 0.
+ * @sz: number of array elements to read
+ *
+ * Search for a property in a device node and read 64-bit value(s) from
+ * it. Returns 0 on success, -EINVAL if the property does not exist,
+ * -ENODATA if property does not have a value, and -EOVERFLOW if the
+ * property data isn't large enough.
+ *
+ * The out_values is modified only if a valid u64 value can be decoded.
+ */
+int of_property_read_u64_array(const struct device_node *np,
+ const char *propname, u64 *out_values,
+ size_t sz)
+{
+ const __be32 *val = of_find_property_value_of_size(np, propname,
+ (sz * sizeof(*out_values)));
+
+ if (IS_ERR(val))
+ return PTR_ERR(val);
+
+ while (sz--) {
+ *out_values++ = of_read_number(val, 2);
+ val += 2;
+ };
+ return 0;
+}
+
+/**
* of_property_read_string - Find and read a string from a property
* @np: device node from which the property value is to be read.
* @propname: name of the property to be searched.
@@ -1490,6 +1523,47 @@ int of_property_count_strings(struct device_node *np, const char *propname)
}
EXPORT_SYMBOL_GPL(of_property_count_strings);
+/**
+ * of_property_read_string_array - Find and read an array of strings
+ * from a multiple strings property.
+ * @np: device node from which the property value is to be read.
+ * @propname: name of the property to be searched.
+ * @out_string: pointer to null terminated return string, modified only if
+ * return value is 0.
+ * @sz: number of array elements to read
+ *
+ * Search for a property in a device tree node and retrieve a list of
+ * terminated string value (pointer to data, not a copy) in that property.
+ * Returns 0 on success, -EINVAL if the property does not exist, -ENODATA if
+ * property does not have a value, and -EILSEQ if the string is not
+ * null-terminated within the length of the property data.
+ *
+ * The out_string pointer is modified only if a valid string can be decoded.
+ */
+int of_property_read_string_array(struct device_node *np, const char *propname,
+ char **output, size_t sz)
+{
+ struct property *prop = of_find_property(np, propname, NULL);
+ int i = 0;
+ size_t l = 0, total = 0;
+ char *p;
+
+ if (!prop)
+ return -EINVAL;
+ if (!prop->value)
+ return -ENODATA;
+ if (strnlen(prop->value, prop->length) >= prop->length)
+ return -EILSEQ;
+
+ p = prop->value;
+
+ for (i = 0; total < prop->length; total += l, p += l) {
+ output[i++] = p;
+ l = strlen(p) + 1;
+ }
+ return 0;
+}
+
void of_print_phandle_args(const char *msg, const struct of_phandle_args *args)
{
int i;
@@ -2376,3 +2450,121 @@ struct device_node *of_graph_get_remote_port(const struct device_node *node)
return of_get_next_parent(np);
}
EXPORT_SYMBOL(of_graph_get_remote_port);
+
+static int of_dev_prop_get(struct device *dev, const char *propname,
+ void **valptr)
+{
+ struct property *pp = of_find_property(dev->of_node, propname, NULL);
+
+ if (!pp)
+ return -ENODATA;
+
+ if (valptr)
+ *valptr = pp->value;
+ return 0;
+}
+
+static int of_dev_prop_read(struct device *dev, const char *propname,
+ enum dev_prop_type proptype, void *val)
+{
+ void *value;
+ int ret = of_dev_prop_get(dev, propname, &value);
+
+ if (ret)
+ return ret;
+
+ if (proptype >= DEV_PROP_U8 && proptype <= DEV_PROP_U64) {
+ switch (proptype) {
+ case DEV_PROP_U8:
+ *(u8 *)val = *(u8 *)value;
+ break;
+ case DEV_PROP_U16:
+ *(u16 *)val = *(u16 *)value;
+ break;
+ case DEV_PROP_U32:
+ *(u32 *)val = *(u32 *)value;
+ break;
+ default:
+ *(u64 *)val = *(u64 *)value;
+ break;
+ }
+ } else if (proptype == DEV_PROP_STRING) {
+ *(char **)val = value;
+ }
+ return ret;
+
+}
+
+static int of_dev_prop_read_array(struct device *dev, const char *propname,
+ enum dev_prop_type proptype, void *val,
+ size_t nval)
+{
+ int ret, elem_size;
+
+ if (!val) {
+ switch (proptype) {
+ case DEV_PROP_U8:
+ elem_size = sizeof(u8);
+ break;
+ case DEV_PROP_U16:
+ elem_size = sizeof(u16);
+ break;
+ case DEV_PROP_U32:
+ elem_size = sizeof(u32);
+ break;
+ case DEV_PROP_U64:
+ elem_size = sizeof(u64);
+ break;
+ case DEV_PROP_STRING:
+ return of_property_count_strings(dev->of_node,
+ propname);
+ default:
+ return -EINVAL;
+ }
+ return of_property_count_elems_of_size(dev->of_node, propname,
+ elem_size);
+ }
+
+ switch (proptype) {
+ case DEV_PROP_U8:
+ ret = of_property_read_u8_array(dev->of_node, propname,
+ (u8 *)val, nval);
+ break;
+ case DEV_PROP_U16:
+ ret = of_property_read_u16_array(dev->of_node, propname,
+ (u16 *)val, nval);
+ break;
+ case DEV_PROP_U32:
+ ret = of_property_read_u32_array(dev->of_node, propname,
+ (u32 *)val, nval);
+ break;
+ case DEV_PROP_U64:
+ ret = of_property_read_u64_array(dev->of_node, propname,
+ (u64 *)val, nval);
+ break;
+ case DEV_PROP_STRING:
+ ret = of_property_read_string_array(dev->of_node, propname,
+ (char **)val, nval);
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+ return ret;
+}
+
+static int of_dev_get_child_count(struct device *dev)
+{
+ struct device_node *node = dev->of_node;
+
+ if (!node)
+ return -EINVAL;
+ return of_get_child_count(node);
+}
+
+struct dev_prop_ops of_property_ops = {
+ .get = of_dev_prop_get,
+ .read = of_dev_prop_read,
+ .read_array = of_dev_prop_read_array,
+ .child_count = of_dev_get_child_count,
+};
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 500436f9be7f..3113d3704eff 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -14,7 +14,7 @@
#include <linux/errno.h>
#include <linux/module.h>
#include <linux/amba/bus.h>
-#include <linux/device.h>
+#include <linux/property.h>
#include <linux/dma-mapping.h>
#include <linux/slab.h>
#include <linux/of_address.h>
@@ -138,6 +138,7 @@ struct platform_device *of_device_alloc(struct device_node *np,
}
dev->dev.of_node = of_node_get(np);
+ dev->dev.property_ops = &of_property_ops;
dev->dev.parent = parent;
if (bus_id)
@@ -293,6 +294,7 @@ static struct amba_device *of_amba_device_create(struct device_node *node,
/* setup generic device info */
dev->dev.coherent_dma_mask = ~0;
dev->dev.of_node = of_node_get(node);
+ dev->dev.property_ops = &of_property_ops;
dev->dev.parent = parent;
dev->dev.platform_data = platform_data;
if (bus_id)
diff --git a/include/linux/property.h b/include/linux/property.h
index 52ea7fe7fe09..a840c1784c82 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -38,6 +38,10 @@ struct dev_prop_ops {
extern struct dev_prop_ops acpi_property_ops;
#endif
+#ifdef CONFIG_OF
+extern struct dev_prop_ops of_property_ops;
+#endif
+
int device_property_get(struct device *dev, const char *propname,
void **valptr);
int device_property_read(struct device *dev, const char *propname,
--
2.1.0.rc1
^ permalink raw reply related [flat|nested] 39+ messages in thread* [RFC PATCH 5/9] mfd: Add ACPI support
2014-08-16 6:53 [RFC PATCH 0/9] Add ACPI _DSD and unified device properties support Mika Westerberg
` (3 preceding siblings ...)
2014-08-16 6:53 ` [RFC PATCH 4/9] of: Add property_ops callback for devices with of_node Mika Westerberg
@ 2014-08-16 6:53 ` Mika Westerberg
2014-08-20 15:54 ` Lee Jones
2014-08-16 6:53 ` [RFC PATCH 6/9] gpiolib: add API to get gpio desc and flags Mika Westerberg
` (6 subsequent siblings)
11 siblings, 1 reply; 39+ messages in thread
From: Mika Westerberg @ 2014-08-16 6:53 UTC (permalink / raw)
To: Darren Hart, Rafael J. Wysocki
Cc: Al Stone, Olof Johansson, Matthew Garrett, Matt Fleming,
David Woodhouse, H. Peter Anvin, Jacob Pan, Josh Triplett,
Aaron Lu, Max Eliaser, Robert Moore, Len Brown,
Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot, Mark Brown,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Grant Likely, Rob Herring, Mika Westerberg,
linux-acpi, devicetree, linux-kernel
If an MFD device is backed by ACPI namespace, we should allow subdevice
drivers to access their corresponding ACPI companion devices through normal
means (e.g using ACPI_COMPANION()).
This patch adds such support to the MFD core. If the MFD parent device
doesn't specify any ACPI _HID/_CID for the child device, the child device
will share the parent ACPI companion device. Otherwise the child device
will be assigned with the corresponding ACPI companion, if found in the
namespace below the parent.
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Reviewed-by: Darren Hart <dvhart@linux.intel.com>
---
Documentation/acpi/enumeration.txt | 27 +++++++++++++++++++++++++
drivers/mfd/mfd-core.c | 41 ++++++++++++++++++++++++++++++++++++++
include/linux/mfd/core.h | 3 +++
3 files changed, 71 insertions(+)
diff --git a/Documentation/acpi/enumeration.txt b/Documentation/acpi/enumeration.txt
index e182be5e3c83..74e35c54febf 100644
--- a/Documentation/acpi/enumeration.txt
+++ b/Documentation/acpi/enumeration.txt
@@ -312,3 +312,30 @@ a code like this:
There are also devm_* versions of these functions which release the
descriptors once the device is released.
+
+MFD devices
+~~~~~~~~~~~
+The MFD devices create platform devices from their children. For the
+child devices there needs to be an ACPI handle that they can use to
+reference parts of the ACPI namespace that relate to them. In the Linux
+MFD subsystem we provide two ways:
+
+ o The children share the parent ACPI handle.
+ o The MFD cell can specify the ACPI id of the device.
+
+For the first case, the MFD drivers do not need to do anything. The
+resulting child platform device will have its ACPI_COMPANION() set to point
+to the parent device.
+
+If the ACPI namespace has a device that we can match using an ACPI id,
+the id should be set like:
+
+ static struct mfd_cell my_subdevice_cell = {
+ .name = "my_subdevice",
+ /* set the resources relative to the parent */
+ .acpi_pnpid = "XYZ0001",
+ };
+
+The ACPI id "XYZ0001" is then used to lookup an ACPI device directly under
+the MFD device and if found, that ACPI companion device is bound to the
+resulting child platform device.
diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index 892d343193ad..bb466b28b3b6 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -78,6 +78,45 @@ static int mfd_platform_add_cell(struct platform_device *pdev,
return 0;
}
+#if IS_ENABLED(CONFIG_ACPI)
+static void mfd_acpi_add_device(const struct mfd_cell *cell,
+ struct platform_device *pdev)
+{
+ struct acpi_device *parent_adev;
+ struct acpi_device *adev = NULL;
+
+ parent_adev = ACPI_COMPANION(pdev->dev.parent);
+ if (!parent_adev)
+ return;
+
+ /*
+ * MFD child device gets its ACPI handle either from the ACPI
+ * device directly under the parent that matches the acpi_pnpid or
+ * it will use the parent handle if is no acpi_pnpid is given.
+ */
+ if (cell->acpi_pnpid) {
+ struct acpi_device_id ids[2] = {};
+ struct acpi_device *child_adev;
+
+ strlcpy(ids[0].id, cell->acpi_pnpid, sizeof(ids[0].id));
+ list_for_each_entry(child_adev, &parent_adev->children, node)
+ if (acpi_match_device_ids(child_adev, ids)) {
+ adev = child_adev;
+ break;
+ }
+ } else {
+ adev = parent_adev;
+ }
+
+ ACPI_COMPANION_SET(&pdev->dev, adev);
+}
+#else
+static inline void mfd_acpi_add_device(const struct mfd_cell *cell,
+ struct platform_device *pdev)
+{
+}
+#endif
+
static int mfd_add_device(struct device *parent, int id,
const struct mfd_cell *cell, atomic_t *usage_count,
struct resource *mem_base,
@@ -118,6 +157,8 @@ static int mfd_add_device(struct device *parent, int id,
}
}
+ mfd_acpi_add_device(cell, pdev);
+
if (cell->pdata_size) {
ret = platform_device_add_data(pdev,
cell->platform_data, cell->pdata_size);
diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index f543de91ce19..73e1709d4c09 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -44,6 +44,9 @@ struct mfd_cell {
*/
const char *of_compatible;
+ /* Matches ACPI PNP id, either _HID or _CID */
+ const char *acpi_pnpid;
+
/*
* These resources can be specified relative to the parent device.
* For accessing hardware you should use resources from the platform dev
--
2.1.0.rc1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [RFC PATCH 5/9] mfd: Add ACPI support
2014-08-16 6:53 ` [RFC PATCH 5/9] mfd: Add ACPI support Mika Westerberg
@ 2014-08-20 15:54 ` Lee Jones
0 siblings, 0 replies; 39+ messages in thread
From: Lee Jones @ 2014-08-20 15:54 UTC (permalink / raw)
To: Mika Westerberg
Cc: Darren Hart, Rafael J. Wysocki, Al Stone, Olof Johansson,
Matthew Garrett, Matt Fleming, David Woodhouse, H. Peter Anvin,
Jacob Pan, Josh Triplett, Aaron Lu, Max Eliaser, Robert Moore,
Len Brown, Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot,
Mark Brown, Dmitry Torokhov, Bryan Wu, Richard Purdie,
Samuel Ortiz, Grant Likely
On Sat, 16 Aug 2014, Mika Westerberg wrote:
> If an MFD device is backed by ACPI namespace, we should allow subdevice
> drivers to access their corresponding ACPI companion devices through normal
> means (e.g using ACPI_COMPANION()).
>
> This patch adds such support to the MFD core. If the MFD parent device
> doesn't specify any ACPI _HID/_CID for the child device, the child device
> will share the parent ACPI companion device. Otherwise the child device
> will be assigned with the corresponding ACPI companion, if found in the
> namespace below the parent.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Reviewed-by: Darren Hart <dvhart@linux.intel.com>
> ---
> Documentation/acpi/enumeration.txt | 27 +++++++++++++++++++++++++
> drivers/mfd/mfd-core.c | 41 ++++++++++++++++++++++++++++++++++++++
> include/linux/mfd/core.h | 3 +++
> 3 files changed, 71 insertions(+)
>
> diff --git a/Documentation/acpi/enumeration.txt b/Documentation/acpi/enumeration.txt
> index e182be5e3c83..74e35c54febf 100644
> --- a/Documentation/acpi/enumeration.txt
> +++ b/Documentation/acpi/enumeration.txt
> @@ -312,3 +312,30 @@ a code like this:
>
> There are also devm_* versions of these functions which release the
> descriptors once the device is released.
> +
> +MFD devices
> +~~~~~~~~~~~
> +The MFD devices create platform devices from their children. For the
What does this mean? MFD drivers register their children _as_
platform devices.
> +child devices there needs to be an ACPI handle that they can use to
> +reference parts of the ACPI namespace that relate to them. In the Linux
> +MFD subsystem we provide two ways:
> +
> + o The children share the parent ACPI handle.
> + o The MFD cell can specify the ACPI id of the device.
> +
> +For the first case, the MFD drivers do not need to do anything. The
> +resulting child platform device will have its ACPI_COMPANION() set to point
> +to the parent device.
> +
> +If the ACPI namespace has a device that we can match using an ACPI id,
> +the id should be set like:
> +
> + static struct mfd_cell my_subdevice_cell = {
> + .name = "my_subdevice",
> + /* set the resources relative to the parent */
> + .acpi_pnpid = "XYZ0001",
> + };
> +
> +The ACPI id "XYZ0001" is then used to lookup an ACPI device directly under
> +the MFD device and if found, that ACPI companion device is bound to the
> +resulting child platform device.
> diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
> index 892d343193ad..bb466b28b3b6 100644
> --- a/drivers/mfd/mfd-core.c
> +++ b/drivers/mfd/mfd-core.c
> @@ -78,6 +78,45 @@ static int mfd_platform_add_cell(struct platform_device *pdev,
> return 0;
> }
>
> +#if IS_ENABLED(CONFIG_ACPI)
> +static void mfd_acpi_add_device(const struct mfd_cell *cell,
> + struct platform_device *pdev)
> +{
> + struct acpi_device *parent_adev;
> + struct acpi_device *adev = NULL;
> +
> + parent_adev = ACPI_COMPANION(pdev->dev.parent);
> + if (!parent_adev)
> + return;
> +
> + /*
> + * MFD child device gets its ACPI handle either from the ACPI
> + * device directly under the parent that matches the acpi_pnpid or
> + * it will use the parent handle if is no acpi_pnpid is given.
> + */
> + if (cell->acpi_pnpid) {
> + struct acpi_device_id ids[2] = {};
> + struct acpi_device *child_adev;
> +
> + strlcpy(ids[0].id, cell->acpi_pnpid, sizeof(ids[0].id));
> + list_for_each_entry(child_adev, &parent_adev->children, node)
> + if (acpi_match_device_ids(child_adev, ids)) {
> + adev = child_adev;
> + break;
> + }
> + } else {
> + adev = parent_adev;
> + }
> +
> + ACPI_COMPANION_SET(&pdev->dev, adev);
> +}
> +#else
> +static inline void mfd_acpi_add_device(const struct mfd_cell *cell,
> + struct platform_device *pdev)
> +{
> +}
> +#endif
> +
I'm not keen on polluting the MFD core driver with #differy. Can't you
think of another way to do it?
> static int mfd_add_device(struct device *parent, int id,
> const struct mfd_cell *cell, atomic_t *usage_count,
> struct resource *mem_base,
> @@ -118,6 +157,8 @@ static int mfd_add_device(struct device *parent, int id,
> }
> }
>
> + mfd_acpi_add_device(cell, pdev);
> +
> if (cell->pdata_size) {
> ret = platform_device_add_data(pdev,
> cell->platform_data, cell->pdata_size);
> diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
> index f543de91ce19..73e1709d4c09 100644
> --- a/include/linux/mfd/core.h
> +++ b/include/linux/mfd/core.h
> @@ -44,6 +44,9 @@ struct mfd_cell {
> */
> const char *of_compatible;
>
> + /* Matches ACPI PNP id, either _HID or _CID */
> + const char *acpi_pnpid;
> +
> /*
> * These resources can be specified relative to the parent device.
> * For accessing hardware you should use resources from the platform dev
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [RFC PATCH 5/9] mfd: Add ACPI support
@ 2014-08-20 15:54 ` Lee Jones
0 siblings, 0 replies; 39+ messages in thread
From: Lee Jones @ 2014-08-20 15:54 UTC (permalink / raw)
To: Mika Westerberg
Cc: Darren Hart, Rafael J. Wysocki, Al Stone, Olof Johansson,
Matthew Garrett, Matt Fleming, David Woodhouse, H. Peter Anvin,
Jacob Pan, Josh Triplett, Aaron Lu, Max Eliaser, Robert Moore,
Len Brown, Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot,
Mark Brown, Dmitry Torokhov, Bryan Wu, Richard Purdie,
Samuel Ortiz, Grant Likely, Rob Herring, linux-acpi, devicetree,
linux-kernel
On Sat, 16 Aug 2014, Mika Westerberg wrote:
> If an MFD device is backed by ACPI namespace, we should allow subdevice
> drivers to access their corresponding ACPI companion devices through normal
> means (e.g using ACPI_COMPANION()).
>
> This patch adds such support to the MFD core. If the MFD parent device
> doesn't specify any ACPI _HID/_CID for the child device, the child device
> will share the parent ACPI companion device. Otherwise the child device
> will be assigned with the corresponding ACPI companion, if found in the
> namespace below the parent.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Reviewed-by: Darren Hart <dvhart@linux.intel.com>
> ---
> Documentation/acpi/enumeration.txt | 27 +++++++++++++++++++++++++
> drivers/mfd/mfd-core.c | 41 ++++++++++++++++++++++++++++++++++++++
> include/linux/mfd/core.h | 3 +++
> 3 files changed, 71 insertions(+)
>
> diff --git a/Documentation/acpi/enumeration.txt b/Documentation/acpi/enumeration.txt
> index e182be5e3c83..74e35c54febf 100644
> --- a/Documentation/acpi/enumeration.txt
> +++ b/Documentation/acpi/enumeration.txt
> @@ -312,3 +312,30 @@ a code like this:
>
> There are also devm_* versions of these functions which release the
> descriptors once the device is released.
> +
> +MFD devices
> +~~~~~~~~~~~
> +The MFD devices create platform devices from their children. For the
What does this mean? MFD drivers register their children _as_
platform devices.
> +child devices there needs to be an ACPI handle that they can use to
> +reference parts of the ACPI namespace that relate to them. In the Linux
> +MFD subsystem we provide two ways:
> +
> + o The children share the parent ACPI handle.
> + o The MFD cell can specify the ACPI id of the device.
> +
> +For the first case, the MFD drivers do not need to do anything. The
> +resulting child platform device will have its ACPI_COMPANION() set to point
> +to the parent device.
> +
> +If the ACPI namespace has a device that we can match using an ACPI id,
> +the id should be set like:
> +
> + static struct mfd_cell my_subdevice_cell = {
> + .name = "my_subdevice",
> + /* set the resources relative to the parent */
> + .acpi_pnpid = "XYZ0001",
> + };
> +
> +The ACPI id "XYZ0001" is then used to lookup an ACPI device directly under
> +the MFD device and if found, that ACPI companion device is bound to the
> +resulting child platform device.
> diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
> index 892d343193ad..bb466b28b3b6 100644
> --- a/drivers/mfd/mfd-core.c
> +++ b/drivers/mfd/mfd-core.c
> @@ -78,6 +78,45 @@ static int mfd_platform_add_cell(struct platform_device *pdev,
> return 0;
> }
>
> +#if IS_ENABLED(CONFIG_ACPI)
> +static void mfd_acpi_add_device(const struct mfd_cell *cell,
> + struct platform_device *pdev)
> +{
> + struct acpi_device *parent_adev;
> + struct acpi_device *adev = NULL;
> +
> + parent_adev = ACPI_COMPANION(pdev->dev.parent);
> + if (!parent_adev)
> + return;
> +
> + /*
> + * MFD child device gets its ACPI handle either from the ACPI
> + * device directly under the parent that matches the acpi_pnpid or
> + * it will use the parent handle if is no acpi_pnpid is given.
> + */
> + if (cell->acpi_pnpid) {
> + struct acpi_device_id ids[2] = {};
> + struct acpi_device *child_adev;
> +
> + strlcpy(ids[0].id, cell->acpi_pnpid, sizeof(ids[0].id));
> + list_for_each_entry(child_adev, &parent_adev->children, node)
> + if (acpi_match_device_ids(child_adev, ids)) {
> + adev = child_adev;
> + break;
> + }
> + } else {
> + adev = parent_adev;
> + }
> +
> + ACPI_COMPANION_SET(&pdev->dev, adev);
> +}
> +#else
> +static inline void mfd_acpi_add_device(const struct mfd_cell *cell,
> + struct platform_device *pdev)
> +{
> +}
> +#endif
> +
I'm not keen on polluting the MFD core driver with #differy. Can't you
think of another way to do it?
> static int mfd_add_device(struct device *parent, int id,
> const struct mfd_cell *cell, atomic_t *usage_count,
> struct resource *mem_base,
> @@ -118,6 +157,8 @@ static int mfd_add_device(struct device *parent, int id,
> }
> }
>
> + mfd_acpi_add_device(cell, pdev);
> +
> if (cell->pdata_size) {
> ret = platform_device_add_data(pdev,
> cell->platform_data, cell->pdata_size);
> diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
> index f543de91ce19..73e1709d4c09 100644
> --- a/include/linux/mfd/core.h
> +++ b/include/linux/mfd/core.h
> @@ -44,6 +44,9 @@ struct mfd_cell {
> */
> const char *of_compatible;
>
> + /* Matches ACPI PNP id, either _HID or _CID */
> + const char *acpi_pnpid;
> +
> /*
> * These resources can be specified relative to the parent device.
> * For accessing hardware you should use resources from the platform dev
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [RFC PATCH 5/9] mfd: Add ACPI support
2014-08-20 15:54 ` Lee Jones
@ 2014-08-21 9:05 ` Mika Westerberg
-1 siblings, 0 replies; 39+ messages in thread
From: Mika Westerberg @ 2014-08-21 9:05 UTC (permalink / raw)
To: Lee Jones
Cc: Darren Hart, Rafael J. Wysocki, Al Stone, Olof Johansson,
Matthew Garrett, Matt Fleming, David Woodhouse, H. Peter Anvin,
Jacob Pan, Josh Triplett, Aaron Lu, Max Eliaser, Robert Moore,
Len Brown, Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot,
Mark Brown, Dmitry Torokhov, Bryan Wu, Richard Purdie,
Samuel Ortiz, Grant Likely
On Wed, Aug 20, 2014 at 04:54:59PM +0100, Lee Jones wrote:
> On Sat, 16 Aug 2014, Mika Westerberg wrote:
> > If an MFD device is backed by ACPI namespace, we should allow subdevice
> > drivers to access their corresponding ACPI companion devices through normal
> > means (e.g using ACPI_COMPANION()).
> >
> > This patch adds such support to the MFD core. If the MFD parent device
> > doesn't specify any ACPI _HID/_CID for the child device, the child device
> > will share the parent ACPI companion device. Otherwise the child device
> > will be assigned with the corresponding ACPI companion, if found in the
> > namespace below the parent.
> >
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Reviewed-by: Darren Hart <dvhart@linux.intel.com>
> > ---
> > Documentation/acpi/enumeration.txt | 27 +++++++++++++++++++++++++
> > drivers/mfd/mfd-core.c | 41 ++++++++++++++++++++++++++++++++++++++
> > include/linux/mfd/core.h | 3 +++
> > 3 files changed, 71 insertions(+)
> >
> > diff --git a/Documentation/acpi/enumeration.txt b/Documentation/acpi/enumeration.txt
> > index e182be5e3c83..74e35c54febf 100644
> > --- a/Documentation/acpi/enumeration.txt
> > +++ b/Documentation/acpi/enumeration.txt
> > @@ -312,3 +312,30 @@ a code like this:
> >
> > There are also devm_* versions of these functions which release the
> > descriptors once the device is released.
> > +
> > +MFD devices
> > +~~~~~~~~~~~
> > +The MFD devices create platform devices from their children. For the
>
> What does this mean? MFD drivers register their children _as_
> platform devices.
Right, will fix.
> > +child devices there needs to be an ACPI handle that they can use to
> > +reference parts of the ACPI namespace that relate to them. In the Linux
> > +MFD subsystem we provide two ways:
> > +
> > + o The children share the parent ACPI handle.
> > + o The MFD cell can specify the ACPI id of the device.
> > +
> > +For the first case, the MFD drivers do not need to do anything. The
> > +resulting child platform device will have its ACPI_COMPANION() set to point
> > +to the parent device.
> > +
> > +If the ACPI namespace has a device that we can match using an ACPI id,
> > +the id should be set like:
> > +
> > + static struct mfd_cell my_subdevice_cell = {
> > + .name = "my_subdevice",
> > + /* set the resources relative to the parent */
> > + .acpi_pnpid = "XYZ0001",
> > + };
> > +
> > +The ACPI id "XYZ0001" is then used to lookup an ACPI device directly under
> > +the MFD device and if found, that ACPI companion device is bound to the
> > +resulting child platform device.
> > diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
> > index 892d343193ad..bb466b28b3b6 100644
> > --- a/drivers/mfd/mfd-core.c
> > +++ b/drivers/mfd/mfd-core.c
> > @@ -78,6 +78,45 @@ static int mfd_platform_add_cell(struct platform_device *pdev,
> > return 0;
> > }
> >
> > +#if IS_ENABLED(CONFIG_ACPI)
> > +static void mfd_acpi_add_device(const struct mfd_cell *cell,
> > + struct platform_device *pdev)
> > +{
> > + struct acpi_device *parent_adev;
> > + struct acpi_device *adev = NULL;
> > +
> > + parent_adev = ACPI_COMPANION(pdev->dev.parent);
> > + if (!parent_adev)
> > + return;
> > +
> > + /*
> > + * MFD child device gets its ACPI handle either from the ACPI
> > + * device directly under the parent that matches the acpi_pnpid or
> > + * it will use the parent handle if is no acpi_pnpid is given.
> > + */
> > + if (cell->acpi_pnpid) {
> > + struct acpi_device_id ids[2] = {};
> > + struct acpi_device *child_adev;
> > +
> > + strlcpy(ids[0].id, cell->acpi_pnpid, sizeof(ids[0].id));
> > + list_for_each_entry(child_adev, &parent_adev->children, node)
> > + if (acpi_match_device_ids(child_adev, ids)) {
> > + adev = child_adev;
> > + break;
> > + }
> > + } else {
> > + adev = parent_adev;
> > + }
> > +
> > + ACPI_COMPANION_SET(&pdev->dev, adev);
> > +}
> > +#else
> > +static inline void mfd_acpi_add_device(const struct mfd_cell *cell,
> > + struct platform_device *pdev)
> > +{
> > +}
> > +#endif
> > +
>
> I'm not keen on polluting the MFD core driver with #differy. Can't you
> think of another way to do it?
It may be possible to do that since we only use few ACPI functions that
have dummy stubs already.
>
> > static int mfd_add_device(struct device *parent, int id,
> > const struct mfd_cell *cell, atomic_t *usage_count,
> > struct resource *mem_base,
> > @@ -118,6 +157,8 @@ static int mfd_add_device(struct device *parent, int id,
> > }
> > }
> >
> > + mfd_acpi_add_device(cell, pdev);
> > +
> > if (cell->pdata_size) {
> > ret = platform_device_add_data(pdev,
> > cell->platform_data, cell->pdata_size);
> > diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
> > index f543de91ce19..73e1709d4c09 100644
> > --- a/include/linux/mfd/core.h
> > +++ b/include/linux/mfd/core.h
> > @@ -44,6 +44,9 @@ struct mfd_cell {
> > */
> > const char *of_compatible;
> >
> > + /* Matches ACPI PNP id, either _HID or _CID */
> > + const char *acpi_pnpid;
> > +
> > /*
> > * These resources can be specified relative to the parent device.
> > * For accessing hardware you should use resources from the platform dev
>
> --
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog
--
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
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [RFC PATCH 5/9] mfd: Add ACPI support
@ 2014-08-21 9:05 ` Mika Westerberg
0 siblings, 0 replies; 39+ messages in thread
From: Mika Westerberg @ 2014-08-21 9:05 UTC (permalink / raw)
To: Lee Jones
Cc: Darren Hart, Rafael J. Wysocki, Al Stone, Olof Johansson,
Matthew Garrett, Matt Fleming, David Woodhouse, H. Peter Anvin,
Jacob Pan, Josh Triplett, Aaron Lu, Max Eliaser, Robert Moore,
Len Brown, Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot,
Mark Brown, Dmitry Torokhov, Bryan Wu, Richard Purdie,
Samuel Ortiz, Grant Likely, Rob Herring, linux-acpi, devicetree,
linux-kernel
On Wed, Aug 20, 2014 at 04:54:59PM +0100, Lee Jones wrote:
> On Sat, 16 Aug 2014, Mika Westerberg wrote:
> > If an MFD device is backed by ACPI namespace, we should allow subdevice
> > drivers to access their corresponding ACPI companion devices through normal
> > means (e.g using ACPI_COMPANION()).
> >
> > This patch adds such support to the MFD core. If the MFD parent device
> > doesn't specify any ACPI _HID/_CID for the child device, the child device
> > will share the parent ACPI companion device. Otherwise the child device
> > will be assigned with the corresponding ACPI companion, if found in the
> > namespace below the parent.
> >
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Reviewed-by: Darren Hart <dvhart@linux.intel.com>
> > ---
> > Documentation/acpi/enumeration.txt | 27 +++++++++++++++++++++++++
> > drivers/mfd/mfd-core.c | 41 ++++++++++++++++++++++++++++++++++++++
> > include/linux/mfd/core.h | 3 +++
> > 3 files changed, 71 insertions(+)
> >
> > diff --git a/Documentation/acpi/enumeration.txt b/Documentation/acpi/enumeration.txt
> > index e182be5e3c83..74e35c54febf 100644
> > --- a/Documentation/acpi/enumeration.txt
> > +++ b/Documentation/acpi/enumeration.txt
> > @@ -312,3 +312,30 @@ a code like this:
> >
> > There are also devm_* versions of these functions which release the
> > descriptors once the device is released.
> > +
> > +MFD devices
> > +~~~~~~~~~~~
> > +The MFD devices create platform devices from their children. For the
>
> What does this mean? MFD drivers register their children _as_
> platform devices.
Right, will fix.
> > +child devices there needs to be an ACPI handle that they can use to
> > +reference parts of the ACPI namespace that relate to them. In the Linux
> > +MFD subsystem we provide two ways:
> > +
> > + o The children share the parent ACPI handle.
> > + o The MFD cell can specify the ACPI id of the device.
> > +
> > +For the first case, the MFD drivers do not need to do anything. The
> > +resulting child platform device will have its ACPI_COMPANION() set to point
> > +to the parent device.
> > +
> > +If the ACPI namespace has a device that we can match using an ACPI id,
> > +the id should be set like:
> > +
> > + static struct mfd_cell my_subdevice_cell = {
> > + .name = "my_subdevice",
> > + /* set the resources relative to the parent */
> > + .acpi_pnpid = "XYZ0001",
> > + };
> > +
> > +The ACPI id "XYZ0001" is then used to lookup an ACPI device directly under
> > +the MFD device and if found, that ACPI companion device is bound to the
> > +resulting child platform device.
> > diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
> > index 892d343193ad..bb466b28b3b6 100644
> > --- a/drivers/mfd/mfd-core.c
> > +++ b/drivers/mfd/mfd-core.c
> > @@ -78,6 +78,45 @@ static int mfd_platform_add_cell(struct platform_device *pdev,
> > return 0;
> > }
> >
> > +#if IS_ENABLED(CONFIG_ACPI)
> > +static void mfd_acpi_add_device(const struct mfd_cell *cell,
> > + struct platform_device *pdev)
> > +{
> > + struct acpi_device *parent_adev;
> > + struct acpi_device *adev = NULL;
> > +
> > + parent_adev = ACPI_COMPANION(pdev->dev.parent);
> > + if (!parent_adev)
> > + return;
> > +
> > + /*
> > + * MFD child device gets its ACPI handle either from the ACPI
> > + * device directly under the parent that matches the acpi_pnpid or
> > + * it will use the parent handle if is no acpi_pnpid is given.
> > + */
> > + if (cell->acpi_pnpid) {
> > + struct acpi_device_id ids[2] = {};
> > + struct acpi_device *child_adev;
> > +
> > + strlcpy(ids[0].id, cell->acpi_pnpid, sizeof(ids[0].id));
> > + list_for_each_entry(child_adev, &parent_adev->children, node)
> > + if (acpi_match_device_ids(child_adev, ids)) {
> > + adev = child_adev;
> > + break;
> > + }
> > + } else {
> > + adev = parent_adev;
> > + }
> > +
> > + ACPI_COMPANION_SET(&pdev->dev, adev);
> > +}
> > +#else
> > +static inline void mfd_acpi_add_device(const struct mfd_cell *cell,
> > + struct platform_device *pdev)
> > +{
> > +}
> > +#endif
> > +
>
> I'm not keen on polluting the MFD core driver with #differy. Can't you
> think of another way to do it?
It may be possible to do that since we only use few ACPI functions that
have dummy stubs already.
>
> > static int mfd_add_device(struct device *parent, int id,
> > const struct mfd_cell *cell, atomic_t *usage_count,
> > struct resource *mem_base,
> > @@ -118,6 +157,8 @@ static int mfd_add_device(struct device *parent, int id,
> > }
> > }
> >
> > + mfd_acpi_add_device(cell, pdev);
> > +
> > if (cell->pdata_size) {
> > ret = platform_device_add_data(pdev,
> > cell->platform_data, cell->pdata_size);
> > diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
> > index f543de91ce19..73e1709d4c09 100644
> > --- a/include/linux/mfd/core.h
> > +++ b/include/linux/mfd/core.h
> > @@ -44,6 +44,9 @@ struct mfd_cell {
> > */
> > const char *of_compatible;
> >
> > + /* Matches ACPI PNP id, either _HID or _CID */
> > + const char *acpi_pnpid;
> > +
> > /*
> > * These resources can be specified relative to the parent device.
> > * For accessing hardware you should use resources from the platform dev
>
> --
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 39+ messages in thread
* [RFC PATCH 6/9] gpiolib: add API to get gpio desc and flags
2014-08-16 6:53 [RFC PATCH 0/9] Add ACPI _DSD and unified device properties support Mika Westerberg
` (4 preceding siblings ...)
2014-08-16 6:53 ` [RFC PATCH 5/9] mfd: Add ACPI support Mika Westerberg
@ 2014-08-16 6:53 ` Mika Westerberg
2014-08-18 16:24 ` Alexandre Courbot
2014-08-16 6:53 ` [RFC PATCH 7/9] gpio: sch: Consolidate core and resume banks Mika Westerberg
` (5 subsequent siblings)
11 siblings, 1 reply; 39+ messages in thread
From: Mika Westerberg @ 2014-08-16 6:53 UTC (permalink / raw)
To: Darren Hart, Rafael J. Wysocki
Cc: Al Stone, Olof Johansson, Matthew Garrett, Matt Fleming,
David Woodhouse, H. Peter Anvin, Jacob Pan, Josh Triplett,
Aaron Lu, Max Eliaser, Robert Moore, Len Brown,
Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot, Mark Brown,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Grant Likely, Rob Herring, Mika Westerberg,
linux-acpi, devicetree, linux-kernel
From: Aaron Lu <aaron.lu@intel.com>
Add a new API to get the GPIO's description pointer and its flags for
both OF based system and ACPI based system. This is useful in drivers
that do not need to care about the underlying firmware interface.
Signed-off-by: Aaron Lu <aaron.lu@intel.com>
Signed-off-by: Max Eliaser <max.eliaser@intel.com>
Signed-off-by: Darren Hart <dvhart@linux.intel.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
drivers/gpio/gpiolib-acpi.c | 81 ++++++++++++++++++++++++++++++++-----------
drivers/gpio/gpiolib.c | 18 ++++++++++
drivers/gpio/gpiolib.h | 8 +++++
include/linux/gpio/consumer.h | 11 ++++++
4 files changed, 97 insertions(+), 21 deletions(-)
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 4a987917c186..f35e88d29a47 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -293,11 +293,38 @@ static int acpi_find_gpio(struct acpi_resource *ares, void *data)
agpio->connection_type == ACPI_RESOURCE_GPIO_TYPE_INT;
lookup->info.active_low =
agpio->polarity == ACPI_ACTIVE_LOW;
+ lookup->info.pin_config = agpio->pin_config;
}
return 1;
}
+static struct gpio_desc *
+__acpi_get_gpiod_by_index(struct acpi_device *adev, int index,
+ struct acpi_gpio_info *info)
+{
+ struct acpi_gpio_lookup lookup;
+ struct list_head resource_list;
+ int ret;
+
+ memset(&lookup, 0, sizeof(lookup));
+ lookup.index = index;
+
+ INIT_LIST_HEAD(&resource_list);
+ ret = acpi_dev_get_resources(adev, &resource_list, acpi_find_gpio,
+ &lookup);
+ if (ret < 0)
+ return ERR_PTR(ret);
+
+ acpi_dev_free_resource_list(&resource_list);
+
+ if (lookup.desc && info)
+ *info = lookup.info;
+
+ return lookup.desc ? lookup.desc : ERR_PTR(-ENOENT);
+}
+
+
/**
* acpi_get_gpiod_by_index() - get a GPIO descriptor from device resources
* @dev: pointer to a device to get GPIO from
@@ -318,34 +345,46 @@ static int acpi_find_gpio(struct acpi_resource *ares, void *data)
struct gpio_desc *acpi_get_gpiod_by_index(struct device *dev, int index,
struct acpi_gpio_info *info)
{
- struct acpi_gpio_lookup lookup;
- struct list_head resource_list;
- struct acpi_device *adev;
- acpi_handle handle;
- int ret;
-
- if (!dev)
+ if (!dev || !ACPI_COMPANION(dev))
return ERR_PTR(-EINVAL);
+ return __acpi_get_gpiod_by_index(ACPI_COMPANION(dev), index, info);
+}
- handle = ACPI_HANDLE(dev);
- if (!handle || acpi_bus_get_device(handle, &adev))
- return ERR_PTR(-ENODEV);
-
- memset(&lookup, 0, sizeof(lookup));
- lookup.index = index;
+struct gpio_desc *acpi_get_gpiod_flags(struct device *dev, int idx,
+ enum gpio_lookup_flags *flags)
+{
+ struct acpi_device *adev = ACPI_COMPANION(dev);
+ struct acpi_reference_args args;
+ struct acpi_gpio_info info;
+ struct gpio_desc *desc;
+ bool active_low;
+ int ret;
- INIT_LIST_HEAD(&resource_list);
- ret = acpi_dev_get_resources(adev, &resource_list, acpi_find_gpio,
- &lookup);
- if (ret < 0)
+ ret = acpi_dev_get_property_reference(adev, "gpios", NULL, idx, &args);
+ if (ret)
return ERR_PTR(ret);
- acpi_dev_free_resource_list(&resource_list);
+ desc = __acpi_get_gpiod_by_index(args.adev, args.args[0], &info);
+ if (IS_ERR(desc))
+ return desc;
- if (lookup.desc && info)
- *info = lookup.info;
+ /*
+ * The 3rd argument optionally specifies the pin polarity. We
+ * use that if it exists, otherwise we resort to the pin config.
+ * (Note that the first element of the "gpios" package goes into
+ * arg.adev, not args.args.)
+ */
+ if (args.nargs >= 3)
+ active_low = !!args.args[2];
+ else if (info.gpioint)
+ active_low = !!info.active_low;
+ else
+ active_low = !!(info.pin_config & ACPI_PIN_CONFIG_PULLUP);
- return lookup.desc ? lookup.desc : ERR_PTR(-ENOENT);
+ if (active_low)
+ *flags |= GPIO_ACTIVE_LOW;
+
+ return desc;
}
static acpi_status
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 2ebc9071e354..e6c2413a6fbf 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2644,6 +2644,24 @@ static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
return desc;
}
+struct gpio_desc *dev_get_gpiod_flags(struct device *dev, unsigned int idx,
+ enum gpio_lookup_flags *flags)
+{
+ struct gpio_desc *desc = ERR_PTR(-ENOENT);
+
+ if (!dev || !flags)
+ return ERR_PTR(-EINVAL);
+
+ /* Using device tree? */
+ if (IS_ENABLED(CONFIG_OF) && dev->of_node)
+ desc = of_find_gpio(dev, NULL, idx, flags);
+ else if (IS_ENABLED(CONFIG_ACPI) && ACPI_COMPANION(dev))
+ desc = acpi_get_gpiod_flags(dev, idx, flags);
+
+ return desc;
+}
+EXPORT_SYMBOL(dev_get_gpiod_flags);
+
static struct gpiod_lookup_table *gpiod_find_lookup_table(struct device *dev)
{
const char *dev_id = dev ? dev_name(dev) : NULL;
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 1a4103dd38df..a759db968e51 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -25,6 +25,7 @@ enum of_gpio_flags;
struct acpi_gpio_info {
bool gpioint;
bool active_low;
+ u8 pin_config;
};
#ifdef CONFIG_ACPI
@@ -33,6 +34,8 @@ void acpi_gpiochip_remove(struct gpio_chip *chip);
struct gpio_desc *acpi_get_gpiod_by_index(struct device *dev, int index,
struct acpi_gpio_info *info);
+struct gpio_desc *acpi_get_gpiod_flags(struct device *dev, int index,
+ enum gpio_lookup_flags *flags);
#else
static inline void acpi_gpiochip_add(struct gpio_chip *chip) { }
static inline void acpi_gpiochip_remove(struct gpio_chip *chip) { }
@@ -43,6 +46,11 @@ acpi_get_gpiod_by_index(struct device *dev, int index,
{
return ERR_PTR(-ENOSYS);
}
+static struct gpio_desc *acpi_get_gpiod_flags(struct device *dev, int index,
+ enum gpio_lookup_flags *flags)
+{
+ return ERR_PTR(-ENOSYS);
+}
#endif
int gpiochip_request_own_desc(struct gpio_desc *desc, const char *label);
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index 05e53ccb708b..53f422e4f0c9 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -7,6 +7,8 @@
struct device;
+enum gpio_lookup_flags;
+
/**
* Opaque descriptor for a GPIO. These are obtained using gpiod_get() and are
* preferable to the old integer-based handles.
@@ -73,6 +75,9 @@ int gpiod_to_irq(const struct gpio_desc *desc);
struct gpio_desc *gpio_to_desc(unsigned gpio);
int desc_to_gpio(const struct gpio_desc *desc);
+struct gpio_desc *dev_get_gpiod_flags(struct device *dev, unsigned int idx,
+ enum gpio_lookup_flags *flags);
+
#else /* CONFIG_GPIOLIB */
static inline struct gpio_desc *__must_check gpiod_get(struct device *dev,
@@ -254,6 +259,12 @@ static inline int desc_to_gpio(const struct gpio_desc *desc)
return -EINVAL;
}
+static inline struct gpio_desc *dev_get_gpiod_flags(struct device *dev,
+ unsigned int idx, enum gpio_lookup_flags *flags)
+{
+ return ERR_PTR(-ENOSYS);
+}
+
#endif /* CONFIG_GPIOLIB */
--
2.1.0.rc1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [RFC PATCH 6/9] gpiolib: add API to get gpio desc and flags
2014-08-16 6:53 ` [RFC PATCH 6/9] gpiolib: add API to get gpio desc and flags Mika Westerberg
@ 2014-08-18 16:24 ` Alexandre Courbot
2014-08-19 8:56 ` Mika Westerberg
0 siblings, 1 reply; 39+ messages in thread
From: Alexandre Courbot @ 2014-08-18 16:24 UTC (permalink / raw)
To: Mika Westerberg
Cc: Darren Hart, Rafael J. Wysocki, Al Stone, Olof Johansson,
Matthew Garrett, Matt Fleming, David Woodhouse, H. Peter Anvin,
Jacob Pan, Josh Triplett, Aaron Lu, Max Eliaser, Robert Moore,
Len Brown, Greg Kroah-Hartman, Linus Walleij, Mark Brown,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Grant Likely, Rob Herring, ACPI Devel Maling List,
devicetree, Linux Kernel Mailing List
On Fri, Aug 15, 2014 at 11:53 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 2ebc9071e354..e6c2413a6fbf 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2644,6 +2644,24 @@ static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
> return desc;
> }
>
> +struct gpio_desc *dev_get_gpiod_flags(struct device *dev, unsigned int idx,
> + enum gpio_lookup_flags *flags)
> +{
> + struct gpio_desc *desc = ERR_PTR(-ENOENT);
> +
> + if (!dev || !flags)
> + return ERR_PTR(-EINVAL);
> +
> + /* Using device tree? */
> + if (IS_ENABLED(CONFIG_OF) && dev->of_node)
> + desc = of_find_gpio(dev, NULL, idx, flags);
> + else if (IS_ENABLED(CONFIG_ACPI) && ACPI_COMPANION(dev))
> + desc = acpi_get_gpiod_flags(dev, idx, flags);
> +
> + return desc;
> +}
> +EXPORT_SYMBOL(dev_get_gpiod_flags);
Putting aside the fact that this function is clearly ACPI-centric (no
con_id parameter and no handling of the platform interface), I have
two big problems with it and it ending up in the consumer interface:
1) The returned descriptor is not requested by gpiolib, which means no
check is made about whether the GPIO has already been requested by
someone else, and another driver can very well request the same GPIO
later and obtain it. Any descriptor returned by a function in
consumer.h *must* be properly requested. Furthermore the 1:1 mapping
between GPIO descriptors and GPIO numbers is not something we can take
for granted (since it will likely change soon), so this practice is
definitely to ban.
2) It exposes the GPIO flags, while they are supposed to be opaque to consumers.
These two points would somehow be acceptable if this function was
gpiolib-private, but here it is clearly not the case and this allows
pretty nasty thing to happen. Basically you are using it to take
advantage of the gpiod lookup mechanism and then quickly fall back to
the legacy integer interface. That's really not something to encourage
- these drivers should be converted to use gpiod internally (while
preserving integer-based lookup for compatiblity, if needed).
In patch 8 you say:
"this can be solved by adding a new field of type
struct gpio_desc but then there is another problem: the devm_gpiod_get
needs to operate on the button device instead of its parent device that
has the driver binded, so when the driver is unloaded, the resources for
the gpio will not get freed automatically."
I'd very much prefer that you use the non-devm variant of gpiod_get()
and free the resources manually when the driver is unloaded than this
workaround that introduces an loophole in the gpiod consumer lookup
functions.
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [RFC PATCH 6/9] gpiolib: add API to get gpio desc and flags
2014-08-18 16:24 ` Alexandre Courbot
@ 2014-08-19 8:56 ` Mika Westerberg
0 siblings, 0 replies; 39+ messages in thread
From: Mika Westerberg @ 2014-08-19 8:56 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Darren Hart, Rafael J. Wysocki, Al Stone, Olof Johansson,
Matthew Garrett, Matt Fleming, David Woodhouse, H. Peter Anvin,
Jacob Pan, Josh Triplett, Aaron Lu, Max Eliaser, Robert Moore,
Len Brown, Greg Kroah-Hartman, Linus Walleij, Mark Brown,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Grant Likely
On Mon, Aug 18, 2014 at 09:24:48AM -0700, Alexandre Courbot wrote:
> On Fri, Aug 15, 2014 at 11:53 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index 2ebc9071e354..e6c2413a6fbf 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -2644,6 +2644,24 @@ static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
> > return desc;
> > }
> >
> > +struct gpio_desc *dev_get_gpiod_flags(struct device *dev, unsigned int idx,
> > + enum gpio_lookup_flags *flags)
> > +{
> > + struct gpio_desc *desc = ERR_PTR(-ENOENT);
> > +
> > + if (!dev || !flags)
> > + return ERR_PTR(-EINVAL);
> > +
> > + /* Using device tree? */
> > + if (IS_ENABLED(CONFIG_OF) && dev->of_node)
> > + desc = of_find_gpio(dev, NULL, idx, flags);
> > + else if (IS_ENABLED(CONFIG_ACPI) && ACPI_COMPANION(dev))
> > + desc = acpi_get_gpiod_flags(dev, idx, flags);
> > +
> > + return desc;
> > +}
> > +EXPORT_SYMBOL(dev_get_gpiod_flags);
>
> Putting aside the fact that this function is clearly ACPI-centric (no
> con_id parameter and no handling of the platform interface), I have
> two big problems with it and it ending up in the consumer interface:
>
> 1) The returned descriptor is not requested by gpiolib, which means no
> check is made about whether the GPIO has already been requested by
> someone else, and another driver can very well request the same GPIO
> later and obtain it. Any descriptor returned by a function in
> consumer.h *must* be properly requested. Furthermore the 1:1 mapping
> between GPIO descriptors and GPIO numbers is not something we can take
> for granted (since it will likely change soon), so this practice is
> definitely to ban.
My bad, somehow I missed the part that it never requested the GPIO.
Thanks for pointing it out.
> 2) It exposes the GPIO flags, while they are supposed to be opaque to consumers.
And this, of course we should be using gpiod_is_active_low() and similar
functions that work with descriptors.
> These two points would somehow be acceptable if this function was
> gpiolib-private, but here it is clearly not the case and this allows
> pretty nasty thing to happen. Basically you are using it to take
> advantage of the gpiod lookup mechanism and then quickly fall back to
> the legacy integer interface. That's really not something to encourage
> - these drivers should be converted to use gpiod internally (while
> preserving integer-based lookup for compatiblity, if needed).
>
> In patch 8 you say:
>
> "this can be solved by adding a new field of type
> struct gpio_desc but then there is another problem: the devm_gpiod_get
> needs to operate on the button device instead of its parent device that
> has the driver binded, so when the driver is unloaded, the resources for
> the gpio will not get freed automatically."
>
> I'd very much prefer that you use the non-devm variant of gpiod_get()
> and free the resources manually when the driver is unloaded than this
> workaround that introduces an loophole in the gpiod consumer lookup
> functions.
I agree and we are going to rework this and the consumer patches to do
exactly what you say.
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [RFC PATCH 6/9] gpiolib: add API to get gpio desc and flags
@ 2014-08-19 8:56 ` Mika Westerberg
0 siblings, 0 replies; 39+ messages in thread
From: Mika Westerberg @ 2014-08-19 8:56 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Darren Hart, Rafael J. Wysocki, Al Stone, Olof Johansson,
Matthew Garrett, Matt Fleming, David Woodhouse, H. Peter Anvin,
Jacob Pan, Josh Triplett, Aaron Lu, Max Eliaser, Robert Moore,
Len Brown, Greg Kroah-Hartman, Linus Walleij, Mark Brown,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Grant Likely, Rob Herring, ACPI Devel Maling List,
devicetree, Linux Kernel Mailing List
On Mon, Aug 18, 2014 at 09:24:48AM -0700, Alexandre Courbot wrote:
> On Fri, Aug 15, 2014 at 11:53 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index 2ebc9071e354..e6c2413a6fbf 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -2644,6 +2644,24 @@ static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
> > return desc;
> > }
> >
> > +struct gpio_desc *dev_get_gpiod_flags(struct device *dev, unsigned int idx,
> > + enum gpio_lookup_flags *flags)
> > +{
> > + struct gpio_desc *desc = ERR_PTR(-ENOENT);
> > +
> > + if (!dev || !flags)
> > + return ERR_PTR(-EINVAL);
> > +
> > + /* Using device tree? */
> > + if (IS_ENABLED(CONFIG_OF) && dev->of_node)
> > + desc = of_find_gpio(dev, NULL, idx, flags);
> > + else if (IS_ENABLED(CONFIG_ACPI) && ACPI_COMPANION(dev))
> > + desc = acpi_get_gpiod_flags(dev, idx, flags);
> > +
> > + return desc;
> > +}
> > +EXPORT_SYMBOL(dev_get_gpiod_flags);
>
> Putting aside the fact that this function is clearly ACPI-centric (no
> con_id parameter and no handling of the platform interface), I have
> two big problems with it and it ending up in the consumer interface:
>
> 1) The returned descriptor is not requested by gpiolib, which means no
> check is made about whether the GPIO has already been requested by
> someone else, and another driver can very well request the same GPIO
> later and obtain it. Any descriptor returned by a function in
> consumer.h *must* be properly requested. Furthermore the 1:1 mapping
> between GPIO descriptors and GPIO numbers is not something we can take
> for granted (since it will likely change soon), so this practice is
> definitely to ban.
My bad, somehow I missed the part that it never requested the GPIO.
Thanks for pointing it out.
> 2) It exposes the GPIO flags, while they are supposed to be opaque to consumers.
And this, of course we should be using gpiod_is_active_low() and similar
functions that work with descriptors.
> These two points would somehow be acceptable if this function was
> gpiolib-private, but here it is clearly not the case and this allows
> pretty nasty thing to happen. Basically you are using it to take
> advantage of the gpiod lookup mechanism and then quickly fall back to
> the legacy integer interface. That's really not something to encourage
> - these drivers should be converted to use gpiod internally (while
> preserving integer-based lookup for compatiblity, if needed).
>
> In patch 8 you say:
>
> "this can be solved by adding a new field of type
> struct gpio_desc but then there is another problem: the devm_gpiod_get
> needs to operate on the button device instead of its parent device that
> has the driver binded, so when the driver is unloaded, the resources for
> the gpio will not get freed automatically."
>
> I'd very much prefer that you use the non-devm variant of gpiod_get()
> and free the resources manually when the driver is unloaded than this
> workaround that introduces an loophole in the gpiod consumer lookup
> functions.
I agree and we are going to rework this and the consumer patches to do
exactly what you say.
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [RFC PATCH 6/9] gpiolib: add API to get gpio desc and flags
2014-08-19 8:56 ` Mika Westerberg
@ 2014-08-19 9:02 ` Aaron Lu
-1 siblings, 0 replies; 39+ messages in thread
From: Aaron Lu @ 2014-08-19 9:02 UTC (permalink / raw)
To: Mika Westerberg, Alexandre Courbot
Cc: Darren Hart, Rafael J. Wysocki, Al Stone, Olof Johansson,
Matthew Garrett, Matt Fleming, David Woodhouse, H. Peter Anvin,
Jacob Pan, Josh Triplett, Max Eliaser, Robert Moore, Len Brown,
Greg Kroah-Hartman, Linus Walleij, Mark Brown, Dmitry Torokhov,
Bryan Wu, Richard Purdie, Samuel Ortiz, Lee Jones, Grant Likely,
Rob Herring, AC
On 08/19/2014 04:56 PM, Mika Westerberg wrote:
> On Mon, Aug 18, 2014 at 09:24:48AM -0700, Alexandre Courbot wrote:
>> On Fri, Aug 15, 2014 at 11:53 PM, Mika Westerberg
>> <mika.westerberg@linux.intel.com> wrote:
>>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>>> index 2ebc9071e354..e6c2413a6fbf 100644
>>> --- a/drivers/gpio/gpiolib.c
>>> +++ b/drivers/gpio/gpiolib.c
>>> @@ -2644,6 +2644,24 @@ static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
>>> return desc;
>>> }
>>>
>>> +struct gpio_desc *dev_get_gpiod_flags(struct device *dev, unsigned int idx,
>>> + enum gpio_lookup_flags *flags)
>>> +{
>>> + struct gpio_desc *desc = ERR_PTR(-ENOENT);
>>> +
>>> + if (!dev || !flags)
>>> + return ERR_PTR(-EINVAL);
>>> +
>>> + /* Using device tree? */
>>> + if (IS_ENABLED(CONFIG_OF) && dev->of_node)
>>> + desc = of_find_gpio(dev, NULL, idx, flags);
>>> + else if (IS_ENABLED(CONFIG_ACPI) && ACPI_COMPANION(dev))
>>> + desc = acpi_get_gpiod_flags(dev, idx, flags);
>>> +
>>> + return desc;
>>> +}
>>> +EXPORT_SYMBOL(dev_get_gpiod_flags);
>>
>> Putting aside the fact that this function is clearly ACPI-centric (no
>> con_id parameter and no handling of the platform interface), I have
>> two big problems with it and it ending up in the consumer interface:
>>
>> 1) The returned descriptor is not requested by gpiolib, which means no
>> check is made about whether the GPIO has already been requested by
>> someone else, and another driver can very well request the same GPIO
>> later and obtain it. Any descriptor returned by a function in
>> consumer.h *must* be properly requested. Furthermore the 1:1 mapping
>> between GPIO descriptors and GPIO numbers is not something we can take
>> for granted (since it will likely change soon), so this practice is
>> definitely to ban.
>
> My bad, somehow I missed the part that it never requested the GPIO.
> Thanks for pointing it out.
>
>> 2) It exposes the GPIO flags, while they are supposed to be opaque to consumers.
>
> And this, of course we should be using gpiod_is_active_low() and similar
> functions that work with descriptors.
>
>> These two points would somehow be acceptable if this function was
>> gpiolib-private, but here it is clearly not the case and this allows
>> pretty nasty thing to happen. Basically you are using it to take
>> advantage of the gpiod lookup mechanism and then quickly fall back to
>> the legacy integer interface. That's really not something to encourage
>> - these drivers should be converted to use gpiod internally (while
>> preserving integer-based lookup for compatiblity, if needed).
>>
>> In patch 8 you say:
>>
>> "this can be solved by adding a new field of type
>> struct gpio_desc but then there is another problem: the devm_gpiod_get
>> needs to operate on the button device instead of its parent device that
>> has the driver binded, so when the driver is unloaded, the resources for
>> the gpio will not get freed automatically."
>>
>> I'd very much prefer that you use the non-devm variant of gpiod_get()
>> and free the resources manually when the driver is unloaded than this
>> workaround that introduces an loophole in the gpiod consumer lookup
>> functions.
>
> I agree and we are going to rework this and the consumer patches to do
> exactly what you say.
I agree, and thanks for the suggestions Alexandre.
Will work on this and send an update when it's ready.
Regards,
Aaron
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [RFC PATCH 6/9] gpiolib: add API to get gpio desc and flags
@ 2014-08-19 9:02 ` Aaron Lu
0 siblings, 0 replies; 39+ messages in thread
From: Aaron Lu @ 2014-08-19 9:02 UTC (permalink / raw)
To: Mika Westerberg, Alexandre Courbot
Cc: Darren Hart, Rafael J. Wysocki, Al Stone, Olof Johansson,
Matthew Garrett, Matt Fleming, David Woodhouse, H. Peter Anvin,
Jacob Pan, Josh Triplett, Max Eliaser, Robert Moore, Len Brown,
Greg Kroah-Hartman, Linus Walleij, Mark Brown, Dmitry Torokhov,
Bryan Wu, Richard Purdie, Samuel Ortiz, Lee Jones, Grant Likely,
Rob Herring, ACPI Devel Maling List, devicetree,
Linux Kernel Mailing List
On 08/19/2014 04:56 PM, Mika Westerberg wrote:
> On Mon, Aug 18, 2014 at 09:24:48AM -0700, Alexandre Courbot wrote:
>> On Fri, Aug 15, 2014 at 11:53 PM, Mika Westerberg
>> <mika.westerberg@linux.intel.com> wrote:
>>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>>> index 2ebc9071e354..e6c2413a6fbf 100644
>>> --- a/drivers/gpio/gpiolib.c
>>> +++ b/drivers/gpio/gpiolib.c
>>> @@ -2644,6 +2644,24 @@ static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
>>> return desc;
>>> }
>>>
>>> +struct gpio_desc *dev_get_gpiod_flags(struct device *dev, unsigned int idx,
>>> + enum gpio_lookup_flags *flags)
>>> +{
>>> + struct gpio_desc *desc = ERR_PTR(-ENOENT);
>>> +
>>> + if (!dev || !flags)
>>> + return ERR_PTR(-EINVAL);
>>> +
>>> + /* Using device tree? */
>>> + if (IS_ENABLED(CONFIG_OF) && dev->of_node)
>>> + desc = of_find_gpio(dev, NULL, idx, flags);
>>> + else if (IS_ENABLED(CONFIG_ACPI) && ACPI_COMPANION(dev))
>>> + desc = acpi_get_gpiod_flags(dev, idx, flags);
>>> +
>>> + return desc;
>>> +}
>>> +EXPORT_SYMBOL(dev_get_gpiod_flags);
>>
>> Putting aside the fact that this function is clearly ACPI-centric (no
>> con_id parameter and no handling of the platform interface), I have
>> two big problems with it and it ending up in the consumer interface:
>>
>> 1) The returned descriptor is not requested by gpiolib, which means no
>> check is made about whether the GPIO has already been requested by
>> someone else, and another driver can very well request the same GPIO
>> later and obtain it. Any descriptor returned by a function in
>> consumer.h *must* be properly requested. Furthermore the 1:1 mapping
>> between GPIO descriptors and GPIO numbers is not something we can take
>> for granted (since it will likely change soon), so this practice is
>> definitely to ban.
>
> My bad, somehow I missed the part that it never requested the GPIO.
> Thanks for pointing it out.
>
>> 2) It exposes the GPIO flags, while they are supposed to be opaque to consumers.
>
> And this, of course we should be using gpiod_is_active_low() and similar
> functions that work with descriptors.
>
>> These two points would somehow be acceptable if this function was
>> gpiolib-private, but here it is clearly not the case and this allows
>> pretty nasty thing to happen. Basically you are using it to take
>> advantage of the gpiod lookup mechanism and then quickly fall back to
>> the legacy integer interface. That's really not something to encourage
>> - these drivers should be converted to use gpiod internally (while
>> preserving integer-based lookup for compatiblity, if needed).
>>
>> In patch 8 you say:
>>
>> "this can be solved by adding a new field of type
>> struct gpio_desc but then there is another problem: the devm_gpiod_get
>> needs to operate on the button device instead of its parent device that
>> has the driver binded, so when the driver is unloaded, the resources for
>> the gpio will not get freed automatically."
>>
>> I'd very much prefer that you use the non-devm variant of gpiod_get()
>> and free the resources manually when the driver is unloaded than this
>> workaround that introduces an loophole in the gpiod consumer lookup
>> functions.
>
> I agree and we are going to rework this and the consumer patches to do
> exactly what you say.
I agree, and thanks for the suggestions Alexandre.
Will work on this and send an update when it's ready.
Regards,
Aaron
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [RFC PATCH 6/9] gpiolib: add API to get gpio desc and flags
2014-08-19 8:56 ` Mika Westerberg
@ 2014-08-19 17:16 ` Alexandre Courbot
-1 siblings, 0 replies; 39+ messages in thread
From: Alexandre Courbot @ 2014-08-19 17:16 UTC (permalink / raw)
To: Mika Westerberg
Cc: Darren Hart, Rafael J. Wysocki, Al Stone, Olof Johansson,
Matthew Garrett, Matt Fleming, David Woodhouse, H. Peter Anvin,
Jacob Pan, Josh Triplett, Aaron Lu, Max Eliaser, Robert Moore,
Len Brown, Greg Kroah-Hartman, Linus Walleij, Mark Brown,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Grant Likely
On Tue, Aug 19, 2014 at 1:56 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Mon, Aug 18, 2014 at 09:24:48AM -0700, Alexandre Courbot wrote:
>> On Fri, Aug 15, 2014 at 11:53 PM, Mika Westerberg
>> <mika.westerberg@linux.intel.com> wrote:
>> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> > index 2ebc9071e354..e6c2413a6fbf 100644
>> > --- a/drivers/gpio/gpiolib.c
>> > +++ b/drivers/gpio/gpiolib.c
>> > @@ -2644,6 +2644,24 @@ static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
>> > return desc;
>> > }
>> >
>> > +struct gpio_desc *dev_get_gpiod_flags(struct device *dev, unsigned int idx,
>> > + enum gpio_lookup_flags *flags)
>> > +{
>> > + struct gpio_desc *desc = ERR_PTR(-ENOENT);
>> > +
>> > + if (!dev || !flags)
>> > + return ERR_PTR(-EINVAL);
>> > +
>> > + /* Using device tree? */
>> > + if (IS_ENABLED(CONFIG_OF) && dev->of_node)
>> > + desc = of_find_gpio(dev, NULL, idx, flags);
>> > + else if (IS_ENABLED(CONFIG_ACPI) && ACPI_COMPANION(dev))
>> > + desc = acpi_get_gpiod_flags(dev, idx, flags);
>> > +
>> > + return desc;
>> > +}
>> > +EXPORT_SYMBOL(dev_get_gpiod_flags);
>>
>> Putting aside the fact that this function is clearly ACPI-centric (no
>> con_id parameter and no handling of the platform interface), I have
>> two big problems with it and it ending up in the consumer interface:
>>
>> 1) The returned descriptor is not requested by gpiolib, which means no
>> check is made about whether the GPIO has already been requested by
>> someone else, and another driver can very well request the same GPIO
>> later and obtain it. Any descriptor returned by a function in
>> consumer.h *must* be properly requested. Furthermore the 1:1 mapping
>> between GPIO descriptors and GPIO numbers is not something we can take
>> for granted (since it will likely change soon), so this practice is
>> definitely to ban.
>
> My bad, somehow I missed the part that it never requested the GPIO.
> Thanks for pointing it out.
>
>> 2) It exposes the GPIO flags, while they are supposed to be opaque to consumers.
>
> And this, of course we should be using gpiod_is_active_low() and similar
> functions that work with descriptors.
Yes, although if you convert the driver to use descriptors you should
not even have to worry about active_low status.
For drivers that still need to handle GPIO numbers for compatibility
reasons, it might be nice if gpiolib provided a gpio_to_desc() variant
that accepts an ACTIVE_LOW flag, so you don't have to worry about the
active low status once you have converted your GPIO number to a
descriptor. Actually for these cases we may be better with a function
that does what gpio_to_desc() does, but also requests the GPIO and
allows some flags to be specified so the integer-handling part of
drivers can be completely dropped afterwards. That's another problem
though. :)
>
>> These two points would somehow be acceptable if this function was
>> gpiolib-private, but here it is clearly not the case and this allows
>> pretty nasty thing to happen. Basically you are using it to take
>> advantage of the gpiod lookup mechanism and then quickly fall back to
>> the legacy integer interface. That's really not something to encourage
>> - these drivers should be converted to use gpiod internally (while
>> preserving integer-based lookup for compatiblity, if needed).
>>
>> In patch 8 you say:
>>
>> "this can be solved by adding a new field of type
>> struct gpio_desc but then there is another problem: the devm_gpiod_get
>> needs to operate on the button device instead of its parent device that
>> has the driver binded, so when the driver is unloaded, the resources for
>> the gpio will not get freed automatically."
>>
>> I'd very much prefer that you use the non-devm variant of gpiod_get()
>> and free the resources manually when the driver is unloaded than this
>> workaround that introduces an loophole in the gpiod consumer lookup
>> functions.
>
> I agree and we are going to rework this and the consumer patches to do
> exactly what you say.
Great, thanks to you and Aaron for your understanding!
Alex.
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [RFC PATCH 6/9] gpiolib: add API to get gpio desc and flags
@ 2014-08-19 17:16 ` Alexandre Courbot
0 siblings, 0 replies; 39+ messages in thread
From: Alexandre Courbot @ 2014-08-19 17:16 UTC (permalink / raw)
To: Mika Westerberg
Cc: Darren Hart, Rafael J. Wysocki, Al Stone, Olof Johansson,
Matthew Garrett, Matt Fleming, David Woodhouse, H. Peter Anvin,
Jacob Pan, Josh Triplett, Aaron Lu, Max Eliaser, Robert Moore,
Len Brown, Greg Kroah-Hartman, Linus Walleij, Mark Brown,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Grant Likely, Rob Herring, ACPI Devel Maling List,
devicetree, Linux Kernel Mailing List
On Tue, Aug 19, 2014 at 1:56 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Mon, Aug 18, 2014 at 09:24:48AM -0700, Alexandre Courbot wrote:
>> On Fri, Aug 15, 2014 at 11:53 PM, Mika Westerberg
>> <mika.westerberg@linux.intel.com> wrote:
>> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> > index 2ebc9071e354..e6c2413a6fbf 100644
>> > --- a/drivers/gpio/gpiolib.c
>> > +++ b/drivers/gpio/gpiolib.c
>> > @@ -2644,6 +2644,24 @@ static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
>> > return desc;
>> > }
>> >
>> > +struct gpio_desc *dev_get_gpiod_flags(struct device *dev, unsigned int idx,
>> > + enum gpio_lookup_flags *flags)
>> > +{
>> > + struct gpio_desc *desc = ERR_PTR(-ENOENT);
>> > +
>> > + if (!dev || !flags)
>> > + return ERR_PTR(-EINVAL);
>> > +
>> > + /* Using device tree? */
>> > + if (IS_ENABLED(CONFIG_OF) && dev->of_node)
>> > + desc = of_find_gpio(dev, NULL, idx, flags);
>> > + else if (IS_ENABLED(CONFIG_ACPI) && ACPI_COMPANION(dev))
>> > + desc = acpi_get_gpiod_flags(dev, idx, flags);
>> > +
>> > + return desc;
>> > +}
>> > +EXPORT_SYMBOL(dev_get_gpiod_flags);
>>
>> Putting aside the fact that this function is clearly ACPI-centric (no
>> con_id parameter and no handling of the platform interface), I have
>> two big problems with it and it ending up in the consumer interface:
>>
>> 1) The returned descriptor is not requested by gpiolib, which means no
>> check is made about whether the GPIO has already been requested by
>> someone else, and another driver can very well request the same GPIO
>> later and obtain it. Any descriptor returned by a function in
>> consumer.h *must* be properly requested. Furthermore the 1:1 mapping
>> between GPIO descriptors and GPIO numbers is not something we can take
>> for granted (since it will likely change soon), so this practice is
>> definitely to ban.
>
> My bad, somehow I missed the part that it never requested the GPIO.
> Thanks for pointing it out.
>
>> 2) It exposes the GPIO flags, while they are supposed to be opaque to consumers.
>
> And this, of course we should be using gpiod_is_active_low() and similar
> functions that work with descriptors.
Yes, although if you convert the driver to use descriptors you should
not even have to worry about active_low status.
For drivers that still need to handle GPIO numbers for compatibility
reasons, it might be nice if gpiolib provided a gpio_to_desc() variant
that accepts an ACTIVE_LOW flag, so you don't have to worry about the
active low status once you have converted your GPIO number to a
descriptor. Actually for these cases we may be better with a function
that does what gpio_to_desc() does, but also requests the GPIO and
allows some flags to be specified so the integer-handling part of
drivers can be completely dropped afterwards. That's another problem
though. :)
>
>> These two points would somehow be acceptable if this function was
>> gpiolib-private, but here it is clearly not the case and this allows
>> pretty nasty thing to happen. Basically you are using it to take
>> advantage of the gpiod lookup mechanism and then quickly fall back to
>> the legacy integer interface. That's really not something to encourage
>> - these drivers should be converted to use gpiod internally (while
>> preserving integer-based lookup for compatiblity, if needed).
>>
>> In patch 8 you say:
>>
>> "this can be solved by adding a new field of type
>> struct gpio_desc but then there is another problem: the devm_gpiod_get
>> needs to operate on the button device instead of its parent device that
>> has the driver binded, so when the driver is unloaded, the resources for
>> the gpio will not get freed automatically."
>>
>> I'd very much prefer that you use the non-devm variant of gpiod_get()
>> and free the resources manually when the driver is unloaded than this
>> workaround that introduces an loophole in the gpiod consumer lookup
>> functions.
>
> I agree and we are going to rework this and the consumer patches to do
> exactly what you say.
Great, thanks to you and Aaron for your understanding!
Alex.
^ permalink raw reply [flat|nested] 39+ messages in thread
* [RFC PATCH 7/9] gpio: sch: Consolidate core and resume banks
2014-08-16 6:53 [RFC PATCH 0/9] Add ACPI _DSD and unified device properties support Mika Westerberg
` (5 preceding siblings ...)
2014-08-16 6:53 ` [RFC PATCH 6/9] gpiolib: add API to get gpio desc and flags Mika Westerberg
@ 2014-08-16 6:53 ` Mika Westerberg
2014-08-29 6:36 ` Linus Walleij
2014-08-16 6:53 ` [RFC PATCH 8/9] Input: gpio_keys_polled - Make use of device property API Mika Westerberg
` (4 subsequent siblings)
11 siblings, 1 reply; 39+ messages in thread
From: Mika Westerberg @ 2014-08-16 6:53 UTC (permalink / raw)
To: Darren Hart, Rafael J. Wysocki
Cc: Al Stone, Olof Johansson, Matthew Garrett, Matt Fleming,
David Woodhouse, H. Peter Anvin, Jacob Pan, Josh Triplett,
Aaron Lu, Max Eliaser, Robert Moore, Len Brown,
Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot, Mark Brown,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Grant Likely, Rob Herring, Mika Westerberg,
linux-acpi, devicetree, linux-kernel
This is actually a single device with two sets of identical registers,
which just happen to start from a different offset. Instead of having
separate GPIO chips created we consolidate them to be single GPIO chip.
In addition having a single GPIO chip allows us to handle ACPI GPIO
translation in the core in a more generic way, since the two GPIO chips
share the same parent ACPI device.
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Tested-by: Max Eliaser <max.eliaser@intel.com>
---
drivers/gpio/gpio-sch.c | 303 ++++++++++++++++++------------------------------
1 file changed, 112 insertions(+), 191 deletions(-)
diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
index a9b1cd16c848..99720c8bc8ed 100644
--- a/drivers/gpio/gpio-sch.c
+++ b/drivers/gpio/gpio-sch.c
@@ -29,300 +29,221 @@
#include <linux/gpio.h>
-static DEFINE_SPINLOCK(gpio_lock);
-
-#define CGEN (0x00)
-#define CGIO (0x04)
-#define CGLV (0x08)
-
-#define RGEN (0x20)
-#define RGIO (0x24)
-#define RGLV (0x28)
+#define GEN 0x00
+#define GIO 0x04
+#define GLV 0x08
+
+struct sch_gpio {
+ struct gpio_chip chip;
+ spinlock_t lock;
+ unsigned short iobase;
+ unsigned short core_base;
+ unsigned short resume_base;
+};
-static unsigned short gpio_ba;
+#define to_sch_gpio(c) container_of(c, struct sch_gpio, chip)
-static int sch_gpio_core_direction_in(struct gpio_chip *gc, unsigned gpio_num)
+static unsigned sch_gpio_offset(struct sch_gpio *sch, unsigned gpio,
+ unsigned reg)
{
- u8 curr_dirs;
- unsigned short offset, bit;
+ unsigned base = 0;
- spin_lock(&gpio_lock);
-
- offset = CGIO + gpio_num / 8;
- bit = gpio_num % 8;
-
- curr_dirs = inb(gpio_ba + offset);
-
- if (!(curr_dirs & (1 << bit)))
- outb(curr_dirs | (1 << bit), gpio_ba + offset);
+ if (gpio >= sch->resume_base) {
+ gpio -= sch->resume_base;
+ base += 0x20;
+ }
- spin_unlock(&gpio_lock);
- return 0;
+ return base + reg + gpio / 8;
}
-static int sch_gpio_core_get(struct gpio_chip *gc, unsigned gpio_num)
+static unsigned sch_gpio_bit(struct sch_gpio *sch, unsigned gpio)
{
- int res;
- unsigned short offset, bit;
-
- offset = CGLV + gpio_num / 8;
- bit = gpio_num % 8;
-
- res = !!(inb(gpio_ba + offset) & (1 << bit));
- return res;
+ if (gpio >= sch->resume_base)
+ gpio -= sch->resume_base;
+ return gpio % 8;
}
-static void sch_gpio_core_set(struct gpio_chip *gc, unsigned gpio_num, int val)
+static void sch_gpio_enable(struct sch_gpio *sch, unsigned gpio)
{
- u8 curr_vals;
unsigned short offset, bit;
+ u8 enable;
- spin_lock(&gpio_lock);
+ spin_lock(&sch->lock);
- offset = CGLV + gpio_num / 8;
- bit = gpio_num % 8;
+ offset = sch_gpio_offset(sch, gpio, GEN);
+ bit = sch_gpio_bit(sch, gpio);
- curr_vals = inb(gpio_ba + offset);
+ enable = inb(sch->iobase + offset);
+ if (!(enable & (1 << bit)))
+ outb(enable | (1 << bit), sch->iobase + offset);
- if (val)
- outb(curr_vals | (1 << bit), gpio_ba + offset);
- else
- outb((curr_vals & ~(1 << bit)), gpio_ba + offset);
- spin_unlock(&gpio_lock);
+ spin_unlock(&sch->lock);
}
-static int sch_gpio_core_direction_out(struct gpio_chip *gc,
- unsigned gpio_num, int val)
+static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned gpio_num)
{
+ struct sch_gpio *sch = to_sch_gpio(gc);
u8 curr_dirs;
unsigned short offset, bit;
- spin_lock(&gpio_lock);
-
- offset = CGIO + gpio_num / 8;
- bit = gpio_num % 8;
+ spin_lock(&sch->lock);
- curr_dirs = inb(gpio_ba + offset);
- if (curr_dirs & (1 << bit))
- outb(curr_dirs & ~(1 << bit), gpio_ba + offset);
+ offset = sch_gpio_offset(sch, gpio_num, GIO);
+ bit = sch_gpio_bit(sch, gpio_num);
- spin_unlock(&gpio_lock);
-
- /*
- * according to the datasheet, writing to the level register has no
- * effect when GPIO is programmed as input.
- * Actually the the level register is read-only when configured as input.
- * Thus presetting the output level before switching to output is _NOT_ possible.
- * Hence we set the level after configuring the GPIO as output.
- * But we cannot prevent a short low pulse if direction is set to high
- * and an external pull-up is connected.
- */
- sch_gpio_core_set(gc, gpio_num, val);
- return 0;
-}
-
-static struct gpio_chip sch_gpio_core = {
- .label = "sch_gpio_core",
- .owner = THIS_MODULE,
- .direction_input = sch_gpio_core_direction_in,
- .get = sch_gpio_core_get,
- .direction_output = sch_gpio_core_direction_out,
- .set = sch_gpio_core_set,
-};
-
-static int sch_gpio_resume_direction_in(struct gpio_chip *gc,
- unsigned gpio_num)
-{
- u8 curr_dirs;
- unsigned short offset, bit;
-
- spin_lock(&gpio_lock);
-
- offset = RGIO + gpio_num / 8;
- bit = gpio_num % 8;
-
- curr_dirs = inb(gpio_ba + offset);
+ curr_dirs = inb(sch->iobase + offset);
if (!(curr_dirs & (1 << bit)))
- outb(curr_dirs | (1 << bit), gpio_ba + offset);
+ outb(curr_dirs | (1 << bit), sch->iobase + offset);
- spin_unlock(&gpio_lock);
+ spin_unlock(&sch->lock);
return 0;
}
-static int sch_gpio_resume_get(struct gpio_chip *gc, unsigned gpio_num)
+static int sch_gpio_get(struct gpio_chip *gc, unsigned gpio_num)
{
+ struct sch_gpio *sch = to_sch_gpio(gc);
+ int res;
unsigned short offset, bit;
- offset = RGLV + gpio_num / 8;
- bit = gpio_num % 8;
+ offset = sch_gpio_offset(sch, gpio_num, GLV);
+ bit = sch_gpio_bit(sch, gpio_num);
- return !!(inb(gpio_ba + offset) & (1 << bit));
+ res = !!(inb(sch->iobase + offset) & (1 << bit));
+
+ return res;
}
-static void sch_gpio_resume_set(struct gpio_chip *gc,
- unsigned gpio_num, int val)
+static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int val)
{
+ struct sch_gpio *sch = to_sch_gpio(gc);
u8 curr_vals;
unsigned short offset, bit;
- spin_lock(&gpio_lock);
+ spin_lock(&sch->lock);
- offset = RGLV + gpio_num / 8;
- bit = gpio_num % 8;
+ offset = sch_gpio_offset(sch, gpio_num, GLV);
+ bit = sch_gpio_bit(sch, gpio_num);
- curr_vals = inb(gpio_ba + offset);
+ curr_vals = inb(sch->iobase + offset);
if (val)
- outb(curr_vals | (1 << bit), gpio_ba + offset);
+ outb(curr_vals | (1 << bit), sch->iobase + offset);
else
- outb((curr_vals & ~(1 << bit)), gpio_ba + offset);
+ outb((curr_vals & ~(1 << bit)), sch->iobase + offset);
- spin_unlock(&gpio_lock);
+ spin_unlock(&sch->lock);
}
-static int sch_gpio_resume_direction_out(struct gpio_chip *gc,
- unsigned gpio_num, int val)
+static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned gpio_num,
+ int val)
{
+ struct sch_gpio *sch = to_sch_gpio(gc);
u8 curr_dirs;
unsigned short offset, bit;
- offset = RGIO + gpio_num / 8;
- bit = gpio_num % 8;
+ spin_lock(&sch->lock);
- spin_lock(&gpio_lock);
+ offset = sch_gpio_offset(sch, gpio_num, GIO);
+ bit = sch_gpio_bit(sch, gpio_num);
- curr_dirs = inb(gpio_ba + offset);
+ curr_dirs = inb(sch->iobase + offset);
if (curr_dirs & (1 << bit))
- outb(curr_dirs & ~(1 << bit), gpio_ba + offset);
+ outb(curr_dirs & ~(1 << bit), sch->iobase + offset);
- spin_unlock(&gpio_lock);
+ spin_unlock(&sch->lock);
/*
- * according to the datasheet, writing to the level register has no
- * effect when GPIO is programmed as input.
- * Actually the the level register is read-only when configured as input.
- * Thus presetting the output level before switching to output is _NOT_ possible.
- * Hence we set the level after configuring the GPIO as output.
- * But we cannot prevent a short low pulse if direction is set to high
- * and an external pull-up is connected.
- */
- sch_gpio_resume_set(gc, gpio_num, val);
+ * according to the datasheet, writing to the level register has no
+ * effect when GPIO is programmed as input.
+ * Actually the the level register is read-only when configured as input.
+ * Thus presetting the output level before switching to output is _NOT_ possible.
+ * Hence we set the level after configuring the GPIO as output.
+ * But we cannot prevent a short low pulse if direction is set to high
+ * and an external pull-up is connected.
+ */
+ sch_gpio_set(gc, gpio_num, val);
return 0;
}
-static struct gpio_chip sch_gpio_resume = {
- .label = "sch_gpio_resume",
+static struct gpio_chip sch_gpio_chip = {
+ .label = "sch_gpio",
.owner = THIS_MODULE,
- .direction_input = sch_gpio_resume_direction_in,
- .get = sch_gpio_resume_get,
- .direction_output = sch_gpio_resume_direction_out,
- .set = sch_gpio_resume_set,
+ .direction_input = sch_gpio_direction_in,
+ .get = sch_gpio_get,
+ .direction_output = sch_gpio_direction_out,
+ .set = sch_gpio_set,
};
static int sch_gpio_probe(struct platform_device *pdev)
{
+ struct sch_gpio *sch;
struct resource *res;
- int err, id;
- id = pdev->id;
- if (!id)
- return -ENODEV;
+ sch = devm_kzalloc(&pdev->dev, sizeof(*sch), GFP_KERNEL);
+ if (!sch)
+ return -ENOMEM;
res = platform_get_resource(pdev, IORESOURCE_IO, 0);
if (!res)
return -EBUSY;
- if (!request_region(res->start, resource_size(res), pdev->name))
+ if (!devm_request_region(&pdev->dev, res->start, resource_size(res),
+ pdev->name))
return -EBUSY;
- gpio_ba = res->start;
+ spin_lock_init(&sch->lock);
+ sch->iobase = res->start;
+ sch->chip = sch_gpio_chip;
+ sch->chip.label = dev_name(&pdev->dev);
+ sch->chip.dev = &pdev->dev;
- switch (id) {
+ switch (pdev->id) {
case PCI_DEVICE_ID_INTEL_SCH_LPC:
- sch_gpio_core.base = 0;
- sch_gpio_core.ngpio = 10;
- sch_gpio_resume.base = 10;
- sch_gpio_resume.ngpio = 4;
+ sch->core_base = 0;
+ sch->resume_base = 10;
+ sch->chip.ngpio = 14;
+
/*
* GPIO[6:0] enabled by default
* GPIO7 is configured by the CMC as SLPIOVR
* Enable GPIO[9:8] core powered gpios explicitly
*/
- outb(0x3, gpio_ba + CGEN + 1);
+ sch_gpio_enable(sch, 8);
+ sch_gpio_enable(sch, 9);
/*
* SUS_GPIO[2:0] enabled by default
* Enable SUS_GPIO3 resume powered gpio explicitly
*/
- outb(0x8, gpio_ba + RGEN);
+ sch_gpio_enable(sch, 13);
break;
case PCI_DEVICE_ID_INTEL_ITC_LPC:
- sch_gpio_core.base = 0;
- sch_gpio_core.ngpio = 5;
- sch_gpio_resume.base = 5;
- sch_gpio_resume.ngpio = 9;
+ sch->core_base = 0;
+ sch->resume_base = 5;
+ sch->chip.ngpio = 14;
break;
case PCI_DEVICE_ID_INTEL_CENTERTON_ILB:
- sch_gpio_core.base = 0;
- sch_gpio_core.ngpio = 21;
- sch_gpio_resume.base = 21;
- sch_gpio_resume.ngpio = 9;
+ sch->core_base = 0;
+ sch->resume_base = 21;
+ sch->chip.ngpio = 30;
break;
default:
- err = -ENODEV;
- goto err_sch_gpio_core;
+ return -ENODEV;
}
- sch_gpio_core.dev = &pdev->dev;
- sch_gpio_resume.dev = &pdev->dev;
-
- err = gpiochip_add(&sch_gpio_core);
- if (err < 0)
- goto err_sch_gpio_core;
-
- err = gpiochip_add(&sch_gpio_resume);
- if (err < 0)
- goto err_sch_gpio_resume;
-
- return 0;
+ platform_set_drvdata(pdev, sch);
-err_sch_gpio_resume:
- if (gpiochip_remove(&sch_gpio_core))
- dev_err(&pdev->dev, "%s gpiochip_remove failed\n", __func__);
-
-err_sch_gpio_core:
- release_region(res->start, resource_size(res));
- gpio_ba = 0;
-
- return err;
+ return gpiochip_add(&sch->chip);
}
static int sch_gpio_remove(struct platform_device *pdev)
{
- struct resource *res;
- if (gpio_ba) {
- int err;
-
- err = gpiochip_remove(&sch_gpio_core);
- if (err)
- dev_err(&pdev->dev, "%s failed, %d\n",
- "gpiochip_remove()", err);
- err = gpiochip_remove(&sch_gpio_resume);
- if (err)
- dev_err(&pdev->dev, "%s failed, %d\n",
- "gpiochip_remove()", err);
-
- res = platform_get_resource(pdev, IORESOURCE_IO, 0);
-
- release_region(res->start, resource_size(res));
- gpio_ba = 0;
-
- return err;
- }
+ struct sch_gpio *sch = platform_get_drvdata(pdev);
+ gpiochip_remove(&sch->chip);
return 0;
}
--
2.1.0.rc1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [RFC PATCH 7/9] gpio: sch: Consolidate core and resume banks
2014-08-16 6:53 ` [RFC PATCH 7/9] gpio: sch: Consolidate core and resume banks Mika Westerberg
@ 2014-08-29 6:36 ` Linus Walleij
0 siblings, 0 replies; 39+ messages in thread
From: Linus Walleij @ 2014-08-29 6:36 UTC (permalink / raw)
To: Mika Westerberg
Cc: Darren Hart, Rafael J. Wysocki, Al Stone, Olof Johansson,
Matthew Garrett, Matt Fleming, David Woodhouse, H. Peter Anvin,
Jacob Pan, Josh Triplett, Aaron Lu, Max Eliaser, Robert Moore,
Len Brown, Greg Kroah-Hartman, Alexandre Courbot, Mark Brown,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Grant Likely
On Sat, Aug 16, 2014 at 8:53 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> This is actually a single device with two sets of identical registers,
> which just happen to start from a different offset. Instead of having
> separate GPIO chips created we consolidate them to be single GPIO chip.
>
> In addition having a single GPIO chip allows us to handle ACPI GPIO
> translation in the core in a more generic way, since the two GPIO chips
> share the same parent ACPI device.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Tested-by: Max Eliaser <max.eliaser@intel.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
I guess this needs to be merged with the rest of the stuff in this series
so for the GPIO sch part go ahead.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [RFC PATCH 7/9] gpio: sch: Consolidate core and resume banks
@ 2014-08-29 6:36 ` Linus Walleij
0 siblings, 0 replies; 39+ messages in thread
From: Linus Walleij @ 2014-08-29 6:36 UTC (permalink / raw)
To: Mika Westerberg
Cc: Darren Hart, Rafael J. Wysocki, Al Stone, Olof Johansson,
Matthew Garrett, Matt Fleming, David Woodhouse, H. Peter Anvin,
Jacob Pan, Josh Triplett, Aaron Lu, Max Eliaser, Robert Moore,
Len Brown, Greg Kroah-Hartman, Alexandre Courbot, Mark Brown,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Grant Likely, Rob Herring, ACPI Devel Maling List,
devicetree, linux-kernel
On Sat, Aug 16, 2014 at 8:53 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> This is actually a single device with two sets of identical registers,
> which just happen to start from a different offset. Instead of having
> separate GPIO chips created we consolidate them to be single GPIO chip.
>
> In addition having a single GPIO chip allows us to handle ACPI GPIO
> translation in the core in a more generic way, since the two GPIO chips
> share the same parent ACPI device.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Tested-by: Max Eliaser <max.eliaser@intel.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
I guess this needs to be merged with the rest of the stuff in this series
so for the GPIO sch part go ahead.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 39+ messages in thread
* [RFC PATCH 8/9] Input: gpio_keys_polled - Make use of device property API
2014-08-16 6:53 [RFC PATCH 0/9] Add ACPI _DSD and unified device properties support Mika Westerberg
` (6 preceding siblings ...)
2014-08-16 6:53 ` [RFC PATCH 7/9] gpio: sch: Consolidate core and resume banks Mika Westerberg
@ 2014-08-16 6:53 ` Mika Westerberg
2014-08-18 17:55 ` Jacob Pan
2014-08-16 6:53 ` [RFC PATCH 9/9] leds: leds-gpio: " Mika Westerberg
` (3 subsequent siblings)
11 siblings, 1 reply; 39+ messages in thread
From: Mika Westerberg @ 2014-08-16 6:53 UTC (permalink / raw)
To: Darren Hart, Rafael J. Wysocki
Cc: Al Stone, Olof Johansson, Matthew Garrett, Matt Fleming,
David Woodhouse, H. Peter Anvin, Jacob Pan, Josh Triplett,
Aaron Lu, Max Eliaser, Robert Moore, Len Brown,
Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot, Mark Brown,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Grant Likely, Rob Herring, Mika Westerberg,
linux-acpi, devicetree, linux-kernel
From: Aaron Lu <aaron.lu@intel.com>
Make use of device property API in this driver so that both OF based
system and ACPI based system can use this driver.
The driver isn't converted to descriptor based gpio API due to there are
a lot of existing users of struct gpio_keys_button that expects the gpio
integer field. Though this can be solved by adding a new field of type
struct gpio_desc but then there is another problem: the devm_gpiod_get
needs to operate on the button device instead of its parent device that
has the driver binded, so when the driver is unloaded, the resources for
the gpio will not get freed automatically. Alternatively, we can
introduce a new API, something like gpiod_find_index that does almost
the same thing as gpiod_get_index but doesn't do the gpiod_request call
and use it during finding button resources in gpio_keys_polled_get_button
function. But then this may seem too complicated and not desirable. So
in the end, a simple dev_get_gpiod_flags which is introduced in the last
patch gets used and the gpio field of the struct gpio_keys_button can be
got from the desc_to_gpio.
Signed-off-by: Aaron Lu <aaron.lu@intel.com>
Signed-off-by: Max Eliaser <max@meliaserlow.dyndns.tv>
Reviewed-by: Darren Hart <dvhart@linux.intel.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
drivers/input/keyboard/gpio_keys_polled.c | 139 ++++++++++++++++++------------
1 file changed, 84 insertions(+), 55 deletions(-)
diff --git a/drivers/input/keyboard/gpio_keys_polled.c b/drivers/input/keyboard/gpio_keys_polled.c
index 432d36395f35..49895d75aaff 100644
--- a/drivers/input/keyboard/gpio_keys_polled.c
+++ b/drivers/input/keyboard/gpio_keys_polled.c
@@ -27,6 +27,7 @@
#include <linux/of.h>
#include <linux/of_platform.h>
#include <linux/of_gpio.h>
+#include <linux/property.h>
#define DRV_NAME "gpio-keys-polled"
@@ -102,78 +103,95 @@ static void gpio_keys_polled_close(struct input_polled_dev *dev)
pdata->disable(bdev->dev);
}
-#ifdef CONFIG_OF
-static struct gpio_keys_platform_data *gpio_keys_polled_get_devtree_pdata(struct device *dev)
-{
- struct device_node *node, *pp;
+#if defined(CONFIG_OF) || defined(CONFIG_ACPI)
+
+struct button_proc_context {
struct gpio_keys_platform_data *pdata;
- struct gpio_keys_button *button;
- int error;
- int nbuttons;
int i;
+};
- node = dev->of_node;
- if (!node)
- return NULL;
+static int gpio_keys_polled_get_button(struct device *dev, void *data)
+{
+ struct button_proc_context *c = data;
+ struct gpio_keys_platform_data *pdata = c->pdata;
+ struct gpio_keys_button *button = &pdata->buttons[c->i];
+ struct gpio_desc *desc;
+ enum gpio_lookup_flags flags;
+ void *val;
+
+ if (device_property_get(dev, "gpios", NULL)) {
+ pdata->nbuttons--;
+ dev_warn(dev, "Found button without gpios\n");
+ return 0;
+ }
- nbuttons = of_get_child_count(node);
- if (nbuttons == 0)
- return NULL;
+ desc = dev_get_gpiod_flags(dev, 0, &flags);
+ if (IS_ERR(desc)) {
+ int error = PTR_ERR(desc);
- pdata = devm_kzalloc(dev, sizeof(*pdata) + nbuttons * sizeof(*button),
- GFP_KERNEL);
- if (!pdata)
- return ERR_PTR(-ENOMEM);
+ if (error != -EPROBE_DEFER)
+ dev_err(dev, "Failed to get gpio flags, error: %d\n",
+ error);
+ return error;
+ }
- pdata->buttons = (struct gpio_keys_button *)(pdata + 1);
- pdata->nbuttons = nbuttons;
+ button->gpio = desc_to_gpio(desc);
+ button->active_low = flags & GPIO_ACTIVE_LOW;
- pdata->rep = !!of_get_property(node, "autorepeat", NULL);
- of_property_read_u32(node, "poll-interval", &pdata->poll_interval);
+ if (device_property_read(dev, "linux,code", DEV_PROP_U32,
+ &button->code)) {
+ dev_err(dev, "Button without keycode: 0x%x\n",
+ button->gpio);
+ return -EINVAL;
+ }
- i = 0;
- for_each_child_of_node(node, pp) {
- int gpio;
- enum of_gpio_flags flags;
+ if (!device_property_get(dev, "label", &val))
+ button->desc = val;
- if (!of_find_property(pp, "gpios", NULL)) {
- pdata->nbuttons--;
- dev_warn(dev, "Found button without gpios\n");
- continue;
- }
+ if (device_property_read(dev, "linux,input-type", DEV_PROP_U32,
+ &button->type))
+ button->type = EV_KEY;
- gpio = of_get_gpio_flags(pp, 0, &flags);
- if (gpio < 0) {
- error = gpio;
- if (error != -EPROBE_DEFER)
- dev_err(dev,
- "Failed to get gpio flags, error: %d\n",
- error);
- return ERR_PTR(error);
- }
+ button->wakeup = !device_property_get(dev, "gpio-key,wakeup", NULL);
- button = &pdata->buttons[i++];
+ if (device_property_read(dev, "debounce-interval", DEV_PROP_U32,
+ &button->debounce_interval))
+ button->debounce_interval = 5;
- button->gpio = gpio;
- button->active_low = flags & OF_GPIO_ACTIVE_LOW;
+ c->i++;
+ return 0;
+}
- if (of_property_read_u32(pp, "linux,code", &button->code)) {
- dev_err(dev, "Button without keycode: 0x%x\n",
- button->gpio);
- return ERR_PTR(-EINVAL);
- }
+static struct gpio_keys_platform_data *
+gpio_keys_polled_get_devtree_pdata(struct device *dev)
+{
+ struct gpio_keys_platform_data *pdata;
+ int size;
+ struct button_proc_context c;
+ int error;
+ int nbuttons;
- button->desc = of_get_property(pp, "label", NULL);
+ nbuttons = device_property_child_count(dev);
+ if (nbuttons <= 0)
+ return NULL;
- if (of_property_read_u32(pp, "linux,input-type", &button->type))
- button->type = EV_KEY;
+ size = sizeof(*pdata) + nbuttons * sizeof(struct gpio_keys_button);
+ pdata = devm_kzalloc(dev, size, GFP_KERNEL);
+ if (!pdata)
+ return ERR_PTR(-ENOMEM);
- button->wakeup = !!of_get_property(pp, "gpio-key,wakeup", NULL);
+ pdata->buttons = (struct gpio_keys_button *)(pdata + 1);
+ pdata->nbuttons = nbuttons;
- if (of_property_read_u32(pp, "debounce-interval",
- &button->debounce_interval))
- button->debounce_interval = 5;
- }
+ pdata->rep = !device_property_get(dev, "autorepeat", NULL);
+ device_property_read(dev, "poll-interval", DEV_PROP_U32,
+ &pdata->poll_interval);
+
+ c.pdata = pdata;
+ c.i = 0;
+ error = device_for_each_child(dev, &c, gpio_keys_polled_get_button);
+ if (error)
+ return ERR_PTR(error);
if (pdata->nbuttons == 0)
return ERR_PTR(-EINVAL);
@@ -181,11 +199,21 @@ static struct gpio_keys_platform_data *gpio_keys_polled_get_devtree_pdata(struct
return pdata;
}
+#ifdef CONFIG_OF
static const struct of_device_id gpio_keys_polled_of_match[] = {
{ .compatible = "gpio-keys-polled", },
{ },
};
MODULE_DEVICE_TABLE(of, gpio_keys_polled_of_match);
+#endif
+
+#ifdef CONFIG_ACPI
+static struct acpi_device_id gpio_keys_polled_acpi_match[] = {
+ { "MNW0002" },
+ { },
+};
+MODULE_DEVICE_TABLE(acpi, gpio_keys_polled_acpi_match);
+#endif
#else
@@ -309,6 +337,7 @@ static struct platform_driver gpio_keys_polled_driver = {
.name = DRV_NAME,
.owner = THIS_MODULE,
.of_match_table = of_match_ptr(gpio_keys_polled_of_match),
+ .acpi_match_table = ACPI_PTR(gpio_keys_polled_acpi_match),
},
};
module_platform_driver(gpio_keys_polled_driver);
--
2.1.0.rc1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [RFC PATCH 8/9] Input: gpio_keys_polled - Make use of device property API
2014-08-16 6:53 ` [RFC PATCH 8/9] Input: gpio_keys_polled - Make use of device property API Mika Westerberg
@ 2014-08-18 17:55 ` Jacob Pan
0 siblings, 0 replies; 39+ messages in thread
From: Jacob Pan @ 2014-08-18 17:55 UTC (permalink / raw)
To: Mika Westerberg
Cc: Darren Hart, Rafael J. Wysocki, Al Stone, Olof Johansson,
Matthew Garrett, Matt Fleming, David Woodhouse, H. Peter Anvin,
Josh Triplett, Aaron Lu, Max Eliaser, Robert Moore, Len Brown,
Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot, Mark Brown,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Grant Likely
On Sat, 16 Aug 2014 09:53:58 +0300
Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
> From: Aaron Lu <aaron.lu@intel.com>
>
> Make use of device property API in this driver so that both OF based
> system and ACPI based system can use this driver.
>
Do we always assume OF and ACPI _DSD will have the same property name
strings? i.e. in this patch "gpios"
> + if (device_property_get(dev, "gpios", NULL)) {
> - if (!of_find_property(pp, "gpios", NULL)) {
Maybe i missed something, but I don't think we can make that assumption
in BIOS. If not, what is the point of having unified interface?
> The driver isn't converted to descriptor based gpio API due to there
> are a lot of existing users of struct gpio_keys_button that expects
> the gpio integer field. Though this can be solved by adding a new
> field of type struct gpio_desc but then there is another problem: the
> devm_gpiod_get needs to operate on the button device instead of its
> parent device that has the driver binded, so when the driver is
> unloaded, the resources for the gpio will not get freed
> automatically. Alternatively, we can introduce a new API, something
> like gpiod_find_index that does almost the same thing as
> gpiod_get_index but doesn't do the gpiod_request call and use it
> during finding button resources in gpio_keys_polled_get_button
> function. But then this may seem too complicated and not desirable.
> So in the end, a simple dev_get_gpiod_flags which is introduced in
> the last patch gets used and the gpio field of the struct
> gpio_keys_button can be got from the desc_to_gpio.
>
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> Signed-off-by: Max Eliaser <max@meliaserlow.dyndns.tv>
> Reviewed-by: Darren Hart <dvhart@linux.intel.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
> drivers/input/keyboard/gpio_keys_polled.c | 139
> ++++++++++++++++++------------ 1 file changed, 84 insertions(+), 55
> deletions(-)
>
> diff --git a/drivers/input/keyboard/gpio_keys_polled.c
> b/drivers/input/keyboard/gpio_keys_polled.c index
> 432d36395f35..49895d75aaff 100644 ---
> a/drivers/input/keyboard/gpio_keys_polled.c +++
> b/drivers/input/keyboard/gpio_keys_polled.c @@ -27,6 +27,7 @@
> #include <linux/of.h>
> #include <linux/of_platform.h>
> #include <linux/of_gpio.h>
> +#include <linux/property.h>
>
> #define DRV_NAME "gpio-keys-polled"
>
> @@ -102,78 +103,95 @@ static void gpio_keys_polled_close(struct
> input_polled_dev *dev) pdata->disable(bdev->dev);
> }
>
> -#ifdef CONFIG_OF
> -static struct gpio_keys_platform_data
> *gpio_keys_polled_get_devtree_pdata(struct device *dev) -{
> - struct device_node *node, *pp;
> +#if defined(CONFIG_OF) || defined(CONFIG_ACPI)
> +
> +struct button_proc_context {
> struct gpio_keys_platform_data *pdata;
> - struct gpio_keys_button *button;
> - int error;
> - int nbuttons;
> int i;
> +};
>
> - node = dev->of_node;
> - if (!node)
> - return NULL;
> +static int gpio_keys_polled_get_button(struct device *dev, void
> *data) +{
> + struct button_proc_context *c = data;
> + struct gpio_keys_platform_data *pdata = c->pdata;
> + struct gpio_keys_button *button = &pdata->buttons[c->i];
> + struct gpio_desc *desc;
> + enum gpio_lookup_flags flags;
> + void *val;
> +
> + if (device_property_get(dev, "gpios", NULL)) {
> + pdata->nbuttons--;
> + dev_warn(dev, "Found button without gpios\n");
> + return 0;
> + }
>
> - nbuttons = of_get_child_count(node);
> - if (nbuttons == 0)
> - return NULL;
> + desc = dev_get_gpiod_flags(dev, 0, &flags);
> + if (IS_ERR(desc)) {
> + int error = PTR_ERR(desc);
>
> - pdata = devm_kzalloc(dev, sizeof(*pdata) + nbuttons *
> sizeof(*button),
> - GFP_KERNEL);
> - if (!pdata)
> - return ERR_PTR(-ENOMEM);
> + if (error != -EPROBE_DEFER)
> + dev_err(dev, "Failed to get gpio flags,
> error: %d\n",
> + error);
> + return error;
> + }
>
> - pdata->buttons = (struct gpio_keys_button *)(pdata + 1);
> - pdata->nbuttons = nbuttons;
> + button->gpio = desc_to_gpio(desc);
> + button->active_low = flags & GPIO_ACTIVE_LOW;
>
> - pdata->rep = !!of_get_property(node, "autorepeat", NULL);
> - of_property_read_u32(node, "poll-interval",
> &pdata->poll_interval);
> + if (device_property_read(dev, "linux,code", DEV_PROP_U32,
> + &button->code)) {
> + dev_err(dev, "Button without keycode: 0x%x\n",
> + button->gpio);
> + return -EINVAL;
> + }
>
> - i = 0;
> - for_each_child_of_node(node, pp) {
> - int gpio;
> - enum of_gpio_flags flags;
> + if (!device_property_get(dev, "label", &val))
> + button->desc = val;
>
> - if (!of_find_property(pp, "gpios", NULL)) {
> - pdata->nbuttons--;
> - dev_warn(dev, "Found button without
> gpios\n");
> - continue;
> - }
> + if (device_property_read(dev, "linux,input-type",
> DEV_PROP_U32,
> + &button->type))
> + button->type = EV_KEY;
>
> - gpio = of_get_gpio_flags(pp, 0, &flags);
> - if (gpio < 0) {
> - error = gpio;
> - if (error != -EPROBE_DEFER)
> - dev_err(dev,
> - "Failed to get gpio flags,
> error: %d\n",
> - error);
> - return ERR_PTR(error);
> - }
> + button->wakeup = !device_property_get(dev,
> "gpio-key,wakeup", NULL);
> - button = &pdata->buttons[i++];
> + if (device_property_read(dev, "debounce-interval",
> DEV_PROP_U32,
> + &button->debounce_interval))
> + button->debounce_interval = 5;
>
> - button->gpio = gpio;
> - button->active_low = flags & OF_GPIO_ACTIVE_LOW;
> + c->i++;
> + return 0;
> +}
>
> - if (of_property_read_u32(pp, "linux,code",
> &button->code)) {
> - dev_err(dev, "Button without keycode:
> 0x%x\n",
> - button->gpio);
> - return ERR_PTR(-EINVAL);
> - }
> +static struct gpio_keys_platform_data *
> +gpio_keys_polled_get_devtree_pdata(struct device *dev)
> +{
> + struct gpio_keys_platform_data *pdata;
> + int size;
> + struct button_proc_context c;
> + int error;
> + int nbuttons;
>
> - button->desc = of_get_property(pp, "label", NULL);
> + nbuttons = device_property_child_count(dev);
> + if (nbuttons <= 0)
> + return NULL;
>
> - if (of_property_read_u32(pp, "linux,input-type",
> &button->type))
> - button->type = EV_KEY;
> + size = sizeof(*pdata) + nbuttons * sizeof(struct
> gpio_keys_button);
> + pdata = devm_kzalloc(dev, size, GFP_KERNEL);
> + if (!pdata)
> + return ERR_PTR(-ENOMEM);
>
> - button->wakeup = !!of_get_property(pp,
> "gpio-key,wakeup", NULL);
> + pdata->buttons = (struct gpio_keys_button *)(pdata + 1);
> + pdata->nbuttons = nbuttons;
>
> - if (of_property_read_u32(pp, "debounce-interval",
> - &button->debounce_interval))
> - button->debounce_interval = 5;
> - }
> + pdata->rep = !device_property_get(dev, "autorepeat", NULL);
> + device_property_read(dev, "poll-interval", DEV_PROP_U32,
> + &pdata->poll_interval);
> +
> + c.pdata = pdata;
> + c.i = 0;
> + error = device_for_each_child(dev, &c,
> gpio_keys_polled_get_button);
> + if (error)
> + return ERR_PTR(error);
>
> if (pdata->nbuttons == 0)
> return ERR_PTR(-EINVAL);
> @@ -181,11 +199,21 @@ static struct gpio_keys_platform_data
> *gpio_keys_polled_get_devtree_pdata(struct return pdata;
> }
>
> +#ifdef CONFIG_OF
> static const struct of_device_id gpio_keys_polled_of_match[] = {
> { .compatible = "gpio-keys-polled", },
> { },
> };
> MODULE_DEVICE_TABLE(of, gpio_keys_polled_of_match);
> +#endif
> +
> +#ifdef CONFIG_ACPI
> +static struct acpi_device_id gpio_keys_polled_acpi_match[] = {
> + { "MNW0002" },
> + { },
> +};
> +MODULE_DEVICE_TABLE(acpi, gpio_keys_polled_acpi_match);
> +#endif
>
> #else
>
> @@ -309,6 +337,7 @@ static struct platform_driver
> gpio_keys_polled_driver = { .name = DRV_NAME,
> .owner = THIS_MODULE,
> .of_match_table =
> of_match_ptr(gpio_keys_polled_of_match),
> + .acpi_match_table =
> ACPI_PTR(gpio_keys_polled_acpi_match), },
> };
> module_platform_driver(gpio_keys_polled_driver);
[Jacob Pan]
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [RFC PATCH 8/9] Input: gpio_keys_polled - Make use of device property API
@ 2014-08-18 17:55 ` Jacob Pan
0 siblings, 0 replies; 39+ messages in thread
From: Jacob Pan @ 2014-08-18 17:55 UTC (permalink / raw)
To: Mika Westerberg
Cc: Darren Hart, Rafael J. Wysocki, Al Stone, Olof Johansson,
Matthew Garrett, Matt Fleming, David Woodhouse, H. Peter Anvin,
Josh Triplett, Aaron Lu, Max Eliaser, Robert Moore, Len Brown,
Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot, Mark Brown,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Grant Likely, Rob Herring, linux-acpi, devicetree,
linux-kernel
On Sat, 16 Aug 2014 09:53:58 +0300
Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
> From: Aaron Lu <aaron.lu@intel.com>
>
> Make use of device property API in this driver so that both OF based
> system and ACPI based system can use this driver.
>
Do we always assume OF and ACPI _DSD will have the same property name
strings? i.e. in this patch "gpios"
> + if (device_property_get(dev, "gpios", NULL)) {
> - if (!of_find_property(pp, "gpios", NULL)) {
Maybe i missed something, but I don't think we can make that assumption
in BIOS. If not, what is the point of having unified interface?
> The driver isn't converted to descriptor based gpio API due to there
> are a lot of existing users of struct gpio_keys_button that expects
> the gpio integer field. Though this can be solved by adding a new
> field of type struct gpio_desc but then there is another problem: the
> devm_gpiod_get needs to operate on the button device instead of its
> parent device that has the driver binded, so when the driver is
> unloaded, the resources for the gpio will not get freed
> automatically. Alternatively, we can introduce a new API, something
> like gpiod_find_index that does almost the same thing as
> gpiod_get_index but doesn't do the gpiod_request call and use it
> during finding button resources in gpio_keys_polled_get_button
> function. But then this may seem too complicated and not desirable.
> So in the end, a simple dev_get_gpiod_flags which is introduced in
> the last patch gets used and the gpio field of the struct
> gpio_keys_button can be got from the desc_to_gpio.
>
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> Signed-off-by: Max Eliaser <max@meliaserlow.dyndns.tv>
> Reviewed-by: Darren Hart <dvhart@linux.intel.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
> drivers/input/keyboard/gpio_keys_polled.c | 139
> ++++++++++++++++++------------ 1 file changed, 84 insertions(+), 55
> deletions(-)
>
> diff --git a/drivers/input/keyboard/gpio_keys_polled.c
> b/drivers/input/keyboard/gpio_keys_polled.c index
> 432d36395f35..49895d75aaff 100644 ---
> a/drivers/input/keyboard/gpio_keys_polled.c +++
> b/drivers/input/keyboard/gpio_keys_polled.c @@ -27,6 +27,7 @@
> #include <linux/of.h>
> #include <linux/of_platform.h>
> #include <linux/of_gpio.h>
> +#include <linux/property.h>
>
> #define DRV_NAME "gpio-keys-polled"
>
> @@ -102,78 +103,95 @@ static void gpio_keys_polled_close(struct
> input_polled_dev *dev) pdata->disable(bdev->dev);
> }
>
> -#ifdef CONFIG_OF
> -static struct gpio_keys_platform_data
> *gpio_keys_polled_get_devtree_pdata(struct device *dev) -{
> - struct device_node *node, *pp;
> +#if defined(CONFIG_OF) || defined(CONFIG_ACPI)
> +
> +struct button_proc_context {
> struct gpio_keys_platform_data *pdata;
> - struct gpio_keys_button *button;
> - int error;
> - int nbuttons;
> int i;
> +};
>
> - node = dev->of_node;
> - if (!node)
> - return NULL;
> +static int gpio_keys_polled_get_button(struct device *dev, void
> *data) +{
> + struct button_proc_context *c = data;
> + struct gpio_keys_platform_data *pdata = c->pdata;
> + struct gpio_keys_button *button = &pdata->buttons[c->i];
> + struct gpio_desc *desc;
> + enum gpio_lookup_flags flags;
> + void *val;
> +
> + if (device_property_get(dev, "gpios", NULL)) {
> + pdata->nbuttons--;
> + dev_warn(dev, "Found button without gpios\n");
> + return 0;
> + }
>
> - nbuttons = of_get_child_count(node);
> - if (nbuttons == 0)
> - return NULL;
> + desc = dev_get_gpiod_flags(dev, 0, &flags);
> + if (IS_ERR(desc)) {
> + int error = PTR_ERR(desc);
>
> - pdata = devm_kzalloc(dev, sizeof(*pdata) + nbuttons *
> sizeof(*button),
> - GFP_KERNEL);
> - if (!pdata)
> - return ERR_PTR(-ENOMEM);
> + if (error != -EPROBE_DEFER)
> + dev_err(dev, "Failed to get gpio flags,
> error: %d\n",
> + error);
> + return error;
> + }
>
> - pdata->buttons = (struct gpio_keys_button *)(pdata + 1);
> - pdata->nbuttons = nbuttons;
> + button->gpio = desc_to_gpio(desc);
> + button->active_low = flags & GPIO_ACTIVE_LOW;
>
> - pdata->rep = !!of_get_property(node, "autorepeat", NULL);
> - of_property_read_u32(node, "poll-interval",
> &pdata->poll_interval);
> + if (device_property_read(dev, "linux,code", DEV_PROP_U32,
> + &button->code)) {
> + dev_err(dev, "Button without keycode: 0x%x\n",
> + button->gpio);
> + return -EINVAL;
> + }
>
> - i = 0;
> - for_each_child_of_node(node, pp) {
> - int gpio;
> - enum of_gpio_flags flags;
> + if (!device_property_get(dev, "label", &val))
> + button->desc = val;
>
> - if (!of_find_property(pp, "gpios", NULL)) {
> - pdata->nbuttons--;
> - dev_warn(dev, "Found button without
> gpios\n");
> - continue;
> - }
> + if (device_property_read(dev, "linux,input-type",
> DEV_PROP_U32,
> + &button->type))
> + button->type = EV_KEY;
>
> - gpio = of_get_gpio_flags(pp, 0, &flags);
> - if (gpio < 0) {
> - error = gpio;
> - if (error != -EPROBE_DEFER)
> - dev_err(dev,
> - "Failed to get gpio flags,
> error: %d\n",
> - error);
> - return ERR_PTR(error);
> - }
> + button->wakeup = !device_property_get(dev,
> "gpio-key,wakeup", NULL);
> - button = &pdata->buttons[i++];
> + if (device_property_read(dev, "debounce-interval",
> DEV_PROP_U32,
> + &button->debounce_interval))
> + button->debounce_interval = 5;
>
> - button->gpio = gpio;
> - button->active_low = flags & OF_GPIO_ACTIVE_LOW;
> + c->i++;
> + return 0;
> +}
>
> - if (of_property_read_u32(pp, "linux,code",
> &button->code)) {
> - dev_err(dev, "Button without keycode:
> 0x%x\n",
> - button->gpio);
> - return ERR_PTR(-EINVAL);
> - }
> +static struct gpio_keys_platform_data *
> +gpio_keys_polled_get_devtree_pdata(struct device *dev)
> +{
> + struct gpio_keys_platform_data *pdata;
> + int size;
> + struct button_proc_context c;
> + int error;
> + int nbuttons;
>
> - button->desc = of_get_property(pp, "label", NULL);
> + nbuttons = device_property_child_count(dev);
> + if (nbuttons <= 0)
> + return NULL;
>
> - if (of_property_read_u32(pp, "linux,input-type",
> &button->type))
> - button->type = EV_KEY;
> + size = sizeof(*pdata) + nbuttons * sizeof(struct
> gpio_keys_button);
> + pdata = devm_kzalloc(dev, size, GFP_KERNEL);
> + if (!pdata)
> + return ERR_PTR(-ENOMEM);
>
> - button->wakeup = !!of_get_property(pp,
> "gpio-key,wakeup", NULL);
> + pdata->buttons = (struct gpio_keys_button *)(pdata + 1);
> + pdata->nbuttons = nbuttons;
>
> - if (of_property_read_u32(pp, "debounce-interval",
> - &button->debounce_interval))
> - button->debounce_interval = 5;
> - }
> + pdata->rep = !device_property_get(dev, "autorepeat", NULL);
> + device_property_read(dev, "poll-interval", DEV_PROP_U32,
> + &pdata->poll_interval);
> +
> + c.pdata = pdata;
> + c.i = 0;
> + error = device_for_each_child(dev, &c,
> gpio_keys_polled_get_button);
> + if (error)
> + return ERR_PTR(error);
>
> if (pdata->nbuttons == 0)
> return ERR_PTR(-EINVAL);
> @@ -181,11 +199,21 @@ static struct gpio_keys_platform_data
> *gpio_keys_polled_get_devtree_pdata(struct return pdata;
> }
>
> +#ifdef CONFIG_OF
> static const struct of_device_id gpio_keys_polled_of_match[] = {
> { .compatible = "gpio-keys-polled", },
> { },
> };
> MODULE_DEVICE_TABLE(of, gpio_keys_polled_of_match);
> +#endif
> +
> +#ifdef CONFIG_ACPI
> +static struct acpi_device_id gpio_keys_polled_acpi_match[] = {
> + { "MNW0002" },
> + { },
> +};
> +MODULE_DEVICE_TABLE(acpi, gpio_keys_polled_acpi_match);
> +#endif
>
> #else
>
> @@ -309,6 +337,7 @@ static struct platform_driver
> gpio_keys_polled_driver = { .name = DRV_NAME,
> .owner = THIS_MODULE,
> .of_match_table =
> of_match_ptr(gpio_keys_polled_of_match),
> + .acpi_match_table =
> ACPI_PTR(gpio_keys_polled_acpi_match), },
> };
> module_platform_driver(gpio_keys_polled_driver);
[Jacob Pan]
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [RFC PATCH 8/9] Input: gpio_keys_polled - Make use of device property API
2014-08-18 17:55 ` Jacob Pan
@ 2014-08-19 9:27 ` Mika Westerberg
-1 siblings, 0 replies; 39+ messages in thread
From: Mika Westerberg @ 2014-08-19 9:27 UTC (permalink / raw)
To: Jacob Pan
Cc: Darren Hart, Rafael J. Wysocki, Al Stone, Olof Johansson,
Matthew Garrett, Matt Fleming, David Woodhouse, H. Peter Anvin,
Josh Triplett, Aaron Lu, Max Eliaser, Robert Moore, Len Brown,
Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot, Mark Brown,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Grant Likely
On Mon, Aug 18, 2014 at 10:55:12AM -0700, Jacob Pan wrote:
> On Sat, 16 Aug 2014 09:53:58 +0300
> Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
>
> > From: Aaron Lu <aaron.lu@intel.com>
> >
> > Make use of device property API in this driver so that both OF based
> > system and ACPI based system can use this driver.
> >
> Do we always assume OF and ACPI _DSD will have the same property name
> strings? i.e. in this patch "gpios"
> > + if (device_property_get(dev, "gpios", NULL)) {
> > - if (!of_find_property(pp, "gpios", NULL)) {
>
> Maybe i missed something, but I don't think we can make that assumption
> in BIOS. If not, what is the point of having unified interface?
We recommend that when it makes sense, the property names in _DSD follow
the corresponding DT names.
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [RFC PATCH 8/9] Input: gpio_keys_polled - Make use of device property API
@ 2014-08-19 9:27 ` Mika Westerberg
0 siblings, 0 replies; 39+ messages in thread
From: Mika Westerberg @ 2014-08-19 9:27 UTC (permalink / raw)
To: Jacob Pan
Cc: Darren Hart, Rafael J. Wysocki, Al Stone, Olof Johansson,
Matthew Garrett, Matt Fleming, David Woodhouse, H. Peter Anvin,
Josh Triplett, Aaron Lu, Max Eliaser, Robert Moore, Len Brown,
Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot, Mark Brown,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Grant Likely, Rob Herring, linux-acpi, devicetree,
linux-kernel
On Mon, Aug 18, 2014 at 10:55:12AM -0700, Jacob Pan wrote:
> On Sat, 16 Aug 2014 09:53:58 +0300
> Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
>
> > From: Aaron Lu <aaron.lu@intel.com>
> >
> > Make use of device property API in this driver so that both OF based
> > system and ACPI based system can use this driver.
> >
> Do we always assume OF and ACPI _DSD will have the same property name
> strings? i.e. in this patch "gpios"
> > + if (device_property_get(dev, "gpios", NULL)) {
> > - if (!of_find_property(pp, "gpios", NULL)) {
>
> Maybe i missed something, but I don't think we can make that assumption
> in BIOS. If not, what is the point of having unified interface?
We recommend that when it makes sense, the property names in _DSD follow
the corresponding DT names.
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [RFC PATCH 8/9] Input: gpio_keys_polled - Make use of device property API
2014-08-19 9:27 ` Mika Westerberg
@ 2014-08-19 15:21 ` Darren Hart
-1 siblings, 0 replies; 39+ messages in thread
From: Darren Hart @ 2014-08-19 15:21 UTC (permalink / raw)
To: Mika Westerberg, Jacob Pan
Cc: Rafael J. Wysocki, Al Stone, Olof Johansson, Matthew Garrett,
Matt Fleming, David Woodhouse, H. Peter Anvin, Josh Triplett,
Aaron Lu, Max Eliaser, Robert Moore, Len Brown,
Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot, Mark Brown,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Grant Likely, Rob Herring, linux-acpi
On 8/19/14, 4:27, "Mika Westerberg" <mika.westerberg@linux.intel.com>
wrote:
>On Mon, Aug 18, 2014 at 10:55:12AM -0700, Jacob Pan wrote:
>> On Sat, 16 Aug 2014 09:53:58 +0300
>> Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
>>
>> > From: Aaron Lu <aaron.lu@intel.com>
>> >
>> > Make use of device property API in this driver so that both OF based
>> > system and ACPI based system can use this driver.
>> >
>> Do we always assume OF and ACPI _DSD will have the same property name
>> strings? i.e. in this patch "gpios"
>> > + if (device_property_get(dev, "gpios", NULL)) {
>> > - if (!of_find_property(pp, "gpios", NULL)) {
>>
>> Maybe i missed something, but I don't think we can make that assumption
>> in BIOS. If not, what is the point of having unified interface?
>
>We recommend that when it makes sense, the property names in _DSD follow
>the corresponding DT names.
>
This is especially try for platform drivers such as this.
We will be creating subsystem types, "gpios", and defining them in the
_DSD Implementors Guide (per our discussion this morning at Kernel Summit).
--
Darren Hart Open Source Technology Center
darren.hart@intel.com Intel Corporation
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [RFC PATCH 8/9] Input: gpio_keys_polled - Make use of device property API
@ 2014-08-19 15:21 ` Darren Hart
0 siblings, 0 replies; 39+ messages in thread
From: Darren Hart @ 2014-08-19 15:21 UTC (permalink / raw)
To: Mika Westerberg, Jacob Pan
Cc: Rafael J. Wysocki, Al Stone, Olof Johansson, Matthew Garrett,
Matt Fleming, David Woodhouse, H. Peter Anvin, Josh Triplett,
Aaron Lu, Max Eliaser, Robert Moore, Len Brown,
Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot, Mark Brown,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Grant Likely, Rob Herring, linux-acpi, devicetree,
linux-kernel
On 8/19/14, 4:27, "Mika Westerberg" <mika.westerberg@linux.intel.com>
wrote:
>On Mon, Aug 18, 2014 at 10:55:12AM -0700, Jacob Pan wrote:
>> On Sat, 16 Aug 2014 09:53:58 +0300
>> Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
>>
>> > From: Aaron Lu <aaron.lu@intel.com>
>> >
>> > Make use of device property API in this driver so that both OF based
>> > system and ACPI based system can use this driver.
>> >
>> Do we always assume OF and ACPI _DSD will have the same property name
>> strings? i.e. in this patch "gpios"
>> > + if (device_property_get(dev, "gpios", NULL)) {
>> > - if (!of_find_property(pp, "gpios", NULL)) {
>>
>> Maybe i missed something, but I don't think we can make that assumption
>> in BIOS. If not, what is the point of having unified interface?
>
>We recommend that when it makes sense, the property names in _DSD follow
>the corresponding DT names.
>
This is especially try for platform drivers such as this.
We will be creating subsystem types, "gpios", and defining them in the
_DSD Implementors Guide (per our discussion this morning at Kernel Summit).
--
Darren Hart Open Source Technology Center
darren.hart@intel.com Intel Corporation
^ permalink raw reply [flat|nested] 39+ messages in thread
* [RFC PATCH 9/9] leds: leds-gpio: Make use of device property API
2014-08-16 6:53 [RFC PATCH 0/9] Add ACPI _DSD and unified device properties support Mika Westerberg
` (7 preceding siblings ...)
2014-08-16 6:53 ` [RFC PATCH 8/9] Input: gpio_keys_polled - Make use of device property API Mika Westerberg
@ 2014-08-16 6:53 ` Mika Westerberg
2014-08-16 16:06 ` Darren Hart
` (2 subsequent siblings)
11 siblings, 0 replies; 39+ messages in thread
From: Mika Westerberg @ 2014-08-16 6:53 UTC (permalink / raw)
To: Darren Hart, Rafael J. Wysocki
Cc: Al Stone, Olof Johansson, Matthew Garrett, Matt Fleming,
David Woodhouse, H. Peter Anvin, Jacob Pan, Josh Triplett,
Aaron Lu, Max Eliaser, Robert Moore, Len Brown,
Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot, Mark Brown,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Grant Likely, Rob Herring, Mika Westerberg,
linux-acpi, devicetree, linux-kernel
From: Max Eliaser <max.eliaser@intel.com>
Make use of device property API in this driver so that both OF and ACPI
based system can use the same driver.
Signed-off-by: Max Eliaser <max@meliaserlow.dyndns.tv>
Signed-off-by: Aaron Lu <aaron.lu@intel.com>
Reviewed-by: Darren Hart <dvhart@linux.intel.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
drivers/leds/leds-gpio.c | 130 +++++++++++++++++++++++++++++------------------
1 file changed, 80 insertions(+), 50 deletions(-)
diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index 57ff20fecf57..d7334b8a21ae 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -21,6 +21,7 @@
#include <linux/workqueue.h>
#include <linux/module.h>
#include <linux/err.h>
+#include <linux/property.h>
struct gpio_led_data {
struct led_classdev cdev;
@@ -161,75 +162,102 @@ static inline int sizeof_gpio_leds_priv(int num_leds)
(sizeof(struct gpio_led_data) * num_leds);
}
-/* Code to create from OpenFirmware platform devices */
-#ifdef CONFIG_OF_GPIO
-static struct gpio_leds_priv *gpio_leds_create_of(struct platform_device *pdev)
+#if defined(CONFIG_OF) || defined(CONFIG_ACPI)
+
+static int gpio_leds_get_led(struct device *dev, void *data)
+{
+ struct gpio_led led = {};
+ struct gpio_leds_priv *priv = data;
+ const char *state = NULL;
+ struct gpio_desc *desc;
+ enum gpio_lookup_flags flags;
+ void *val;
+
+ if (device_property_get(dev, "gpios", NULL)) {
+ dev_warn(dev, "Found LED without gpios\n");
+ return 0;
+ }
+
+ desc = dev_get_gpiod_flags(dev, 0, &flags);
+ if (IS_ERR(desc)) {
+ int error = PTR_ERR(desc);
+
+ if (error != -EPROBE_DEFER)
+ dev_err(dev, "Failed to get gpio flags for LED, error: %d\n",
+ error);
+ return error;
+ }
+
+ led.gpio = desc_to_gpio(desc);
+ led.active_low = flags & GPIO_ACTIVE_LOW;
+
+ if (!device_property_read(dev, "label", DEV_PROP_STRING, &val))
+ led.name = val;
+
+ if (!device_property_read(dev, "linux,default-trigger",
+ DEV_PROP_STRING, &val))
+ led.default_trigger = val;
+
+ if (!device_property_read(dev, "linux,default-state", DEV_PROP_STRING,
+ &val))
+ state = val;
+ if (state) {
+ if (!strcmp(state, "keep"))
+ led.default_state = LEDS_GPIO_DEFSTATE_KEEP;
+ else if (!strcmp(state, "on"))
+ led.default_state = LEDS_GPIO_DEFSTATE_ON;
+ else
+ led.default_state = LEDS_GPIO_DEFSTATE_OFF;
+ }
+
+ if (!device_property_get(dev, "linux,retain-state-suspended", NULL))
+ led.retain_state_suspended = 1;
+
+ return create_gpio_led(&led, &priv->leds[priv->num_leds++], dev, NULL);
+}
+
+static struct gpio_leds_priv *gpio_leds_create(struct device *dev)
{
- struct device_node *np = pdev->dev.of_node, *child;
struct gpio_leds_priv *priv;
- int count, ret;
+ int count, error;
/* count LEDs in this device, so we know how much to allocate */
- count = of_get_available_child_count(np);
- if (!count)
+ count = device_property_child_count(dev);
+ if (count <= 0)
return ERR_PTR(-ENODEV);
- for_each_available_child_of_node(np, child)
- if (of_get_gpio(child, 0) == -EPROBE_DEFER)
- return ERR_PTR(-EPROBE_DEFER);
-
- priv = devm_kzalloc(&pdev->dev, sizeof_gpio_leds_priv(count),
+ priv = devm_kzalloc(dev, sizeof_gpio_leds_priv(count),
GFP_KERNEL);
if (!priv)
return ERR_PTR(-ENOMEM);
- for_each_available_child_of_node(np, child) {
- struct gpio_led led = {};
- enum of_gpio_flags flags;
- const char *state;
-
- led.gpio = of_get_gpio_flags(child, 0, &flags);
- led.active_low = flags & OF_GPIO_ACTIVE_LOW;
- led.name = of_get_property(child, "label", NULL) ? : child->name;
- led.default_trigger =
- of_get_property(child, "linux,default-trigger", NULL);
- state = of_get_property(child, "default-state", NULL);
- if (state) {
- if (!strcmp(state, "keep"))
- led.default_state = LEDS_GPIO_DEFSTATE_KEEP;
- else if (!strcmp(state, "on"))
- led.default_state = LEDS_GPIO_DEFSTATE_ON;
- else
- led.default_state = LEDS_GPIO_DEFSTATE_OFF;
- }
-
- if (of_get_property(child, "retain-state-suspended", NULL))
- led.retain_state_suspended = 1;
-
- ret = create_gpio_led(&led, &priv->leds[priv->num_leds++],
- &pdev->dev, NULL);
- if (ret < 0) {
- of_node_put(child);
- goto err;
- }
- }
-
- return priv;
+ error = device_for_each_child(dev, priv, gpio_leds_get_led);
+ if (!error)
+ return priv;
-err:
for (count = priv->num_leds - 2; count >= 0; count--)
delete_gpio_led(&priv->leds[count]);
return ERR_PTR(-ENODEV);
}
+#ifdef CONFIG_OF
static const struct of_device_id of_gpio_leds_match[] = {
{ .compatible = "gpio-leds", },
{},
};
-
MODULE_DEVICE_TABLE(of, of_gpio_leds_match);
-#else /* CONFIG_OF_GPIO */
-static struct gpio_leds_priv *gpio_leds_create_of(struct platform_device *pdev)
+#endif
+
+#ifdef CONFIG_ACPI
+static struct acpi_device_id acpi_gpio_leds_match[] = {
+ { "MNW0001" },
+ { },
+};
+MODULE_DEVICE_TABLE(acpi, acpi_gpio_leds_match);
+#endif
+
+#else /* CONFIG_OF || CONFIG_ACPI */
+static struct gpio_leds_priv *gpio_leds_create(struct device *dev)
{
return ERR_PTR(-ENODEV);
}
@@ -238,7 +266,8 @@ static struct gpio_leds_priv *gpio_leds_create_of(struct platform_device *pdev)
static int gpio_led_probe(struct platform_device *pdev)
{
- struct gpio_led_platform_data *pdata = dev_get_platdata(&pdev->dev);
+ struct device *dev = &pdev->dev;
+ const struct gpio_led_platform_data *pdata = dev_get_platdata(dev);
struct gpio_leds_priv *priv;
int i, ret = 0;
@@ -263,7 +292,7 @@ static int gpio_led_probe(struct platform_device *pdev)
}
}
} else {
- priv = gpio_leds_create_of(pdev);
+ priv = gpio_leds_create(dev);
if (IS_ERR(priv))
return PTR_ERR(priv);
}
@@ -291,6 +320,7 @@ static struct platform_driver gpio_led_driver = {
.name = "leds-gpio",
.owner = THIS_MODULE,
.of_match_table = of_match_ptr(of_gpio_leds_match),
+ .acpi_match_table = ACPI_PTR(acpi_gpio_leds_match),
},
};
--
2.1.0.rc1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [RFC PATCH 0/9] Add ACPI _DSD and unified device properties support
2014-08-16 6:53 [RFC PATCH 0/9] Add ACPI _DSD and unified device properties support Mika Westerberg
@ 2014-08-16 16:06 ` Darren Hart
2014-08-16 6:53 ` [RFC PATCH 2/9] ACPI: Document ACPI " Mika Westerberg
` (10 subsequent siblings)
11 siblings, 0 replies; 39+ messages in thread
From: Darren Hart @ 2014-08-16 16:06 UTC (permalink / raw)
To: Mika Westerberg, Rafael J. Wysocki
Cc: Al Stone, Olof Johansson, Matthew Garrett, Matt Fleming,
David Woodhouse, H. Peter Anvin, Jacob Pan, Josh Triplett,
Aaron Lu, Max Eliaser, Robert Moore, Len Brown,
Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot, Mark Brown,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Grant Likely, Rob Herring, linux-acpi
On 8/15/14, 23:53, "Mika Westerberg" <mika.westerberg@linux.intel.com>
wrote:
>The recent publication of the ACPI 5.1 specification [1] adds a reserved
>name
>for Device Specific Data (_DSD, Section 6.2.5). This mechanism allows for
>passing arbitrary hardware description data to the OS. The exact format
>of the
>_DSD data is specific to the UUID paired with it [2].
>
>An ACPI Device Properties UUID has been defined [3] to provide a format
>compatible with existing device tree schemas. The purpose for this was to
>allow
>for the reuse of the existing schemas and encourage the development of
>firmware
>agnostic device drivers.
>
>This series accomplishes the following (as well as some other
>dependencies):
>
> * Add _DSD support to the ACPI core
> This simply reads the UUID and the accompanying Package
>
> * Add ACPI Device Properties _DSD format support
> This understands the hierarchical key:value pair structure
> defined by the Device Properties UUID
>
> * Add a unified device properties API with ACPI and OF backends
> This provides for the firmware agnostic device properties
> Interface to be used by drivers
>
> * Provides 2 example drivers that were previously Device Tree aware that
> can now be used with either Device Tree or ACPI Device Properties. The
> both drivers use an arbitrary _HID.
>
>This has been tested on Minnowboard with relevant parts of the modified
>DSDT at the end of this email.
This eliminates the need for the board files that were the subject of my
"How not to write x86 platform drivers" talk at ELC-E last year. With
These ACPI core changes and the small changes to the two example drivers,
the Minnowboard can now use the GPIO buttons and LEDs through these
drivers by adding the ASL fragment below to the DSDT.
--
Darren
>
>------ DSDT For Minnowboard ------
>
> Scope (\_SB.PCI0.LPC)
> {
> Device (LEDS)
> {
> Name (_HID, "MNW0001")
>
> Name (_CRS, ResourceTemplate ()
> {
> GpioIo (Exclusive, PullDown, 0, 0,
>IoRestrictionOutputOnly,
> "\\_SB.PCI0.LPC", 0, ResourceConsumer) {10}
> GpioIo (Exclusive, PullDown, 0, 0,
>IoRestrictionOutputOnly,
> "\\_SB.PCI0.LPC", 0, ResourceConsumer) {11}
> })
>
> Device (LEDH)
> {
> Name (_HID, "PRP0000")
> Name (_DSD, Package ()
> {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package ()
> {
> Package () {"label", "Heartbeat"},
> Package () {"gpios", Package () {^^LEDS, 0, 0,
>0}},
> Package () {"linux,default-trigger", "heartbeat"},
> Package () {"linux,default-state", "off"},
> Package () {"linux,retain-state-suspended", 1},
> }
> })
> }
>
> Device (LEDM)
> {
> Name (_HID, "PRP0000")
> Name (_DSD, Package ()
> {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package ()
> {
> Package () {"label", "MMC0 Activity"},
> Package () {"gpios", Package () {^^LEDS, 1, 0,
>0}},
> Package () {"linux,default-trigger", "mmc0"},
> Package () {"linux,default-state", "on"},
> Package () {"linux,retain-state-suspended", 1},
> }
> })
> }
> }
>
> Device (BTNS)
> {
> Name (_HID, "MNW0002")
>
> Name (_CRS, ResourceTemplate ()
> {
> GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly,
> "\\_SB.PCI0.LPC", 0, ResourceConsumer) {0}
> GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly,
> "\\_SB.PCI0.LPC", 0, ResourceConsumer) {1}
> GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly,
> "\\_SB.PCI0.LPC", 0, ResourceConsumer) {2}
> GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly,
> "\\_SB.PCI0.LPC", 0, ResourceConsumer) {3}
> })
>
> Name (_DSD, Package ()
> {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package ()
> {
> Package () {"poll-interval", 100},
> Package () {"autorepeat", 1}
> }
> })
>
> Device (BTN0)
> {
> Name (_HID, "PRP0000")
> Name (_DSD, Package ()
> {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package ()
> {
> Package () {"linux,code", 105},
> Package () {"linux,input-type", 1},
> Package () {"gpios", Package () {^^BTNS, 0, 0,
>1}},
> }
> })
> }
>
> Device (BTN1)
> {
> Name (_HID, "PRP0000")
> Name (_DSD, Package ()
> {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package ()
> {
> Package () {"linux,code", 108},
> Package () {"linux,input-type", 1},
> Package () {"gpios", Package (4) {^^BTNS, 1, 0,
>1}},
> }
> })
> }
>
> Device (BTN2)
> {
> Name (_HID, "PRP0000")
> Name (_DSD, Package ()
> {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package ()
> {
> Package () {"linux,code", 103},
> Package () {"linux,input-type", 1},
> Package () {"gpios", Package () {^^BTNS, 2, 0,
>1}},
> }
> })
> }
>
> Device (BTN3)
> {
> Name (_HID, "PRP0000")
> Name (_DSD, Package ()
> {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package ()
> {
> Package () {"linux,code", 106},
> Package () {"linux,input-type", 1},
> Package () {"gpios", Package (4) {^^BTNS, 3, 0,
>1}},
> }
> })
> }
>
> }
>
> }
>
>--
>2.1.0.rc1
>
>
--
Darren Hart Open Source Technology Center
darren.hart@intel.com Intel Corporation
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [RFC PATCH 0/9] Add ACPI _DSD and unified device properties support
@ 2014-08-16 16:06 ` Darren Hart
0 siblings, 0 replies; 39+ messages in thread
From: Darren Hart @ 2014-08-16 16:06 UTC (permalink / raw)
To: Mika Westerberg, Rafael J. Wysocki
Cc: Al Stone, Olof Johansson, Matthew Garrett, Matt Fleming,
David Woodhouse, H. Peter Anvin, Jacob Pan, Josh Triplett,
Aaron Lu, Max Eliaser, Robert Moore, Len Brown,
Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot, Mark Brown,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Grant Likely, Rob Herring, linux-acpi, devicetree,
linux-kernel
On 8/15/14, 23:53, "Mika Westerberg" <mika.westerberg@linux.intel.com>
wrote:
>The recent publication of the ACPI 5.1 specification [1] adds a reserved
>name
>for Device Specific Data (_DSD, Section 6.2.5). This mechanism allows for
>passing arbitrary hardware description data to the OS. The exact format
>of the
>_DSD data is specific to the UUID paired with it [2].
>
>An ACPI Device Properties UUID has been defined [3] to provide a format
>compatible with existing device tree schemas. The purpose for this was to
>allow
>for the reuse of the existing schemas and encourage the development of
>firmware
>agnostic device drivers.
>
>This series accomplishes the following (as well as some other
>dependencies):
>
> * Add _DSD support to the ACPI core
> This simply reads the UUID and the accompanying Package
>
> * Add ACPI Device Properties _DSD format support
> This understands the hierarchical key:value pair structure
> defined by the Device Properties UUID
>
> * Add a unified device properties API with ACPI and OF backends
> This provides for the firmware agnostic device properties
> Interface to be used by drivers
>
> * Provides 2 example drivers that were previously Device Tree aware that
> can now be used with either Device Tree or ACPI Device Properties. The
> both drivers use an arbitrary _HID.
>
>This has been tested on Minnowboard with relevant parts of the modified
>DSDT at the end of this email.
This eliminates the need for the board files that were the subject of my
"How not to write x86 platform drivers" talk at ELC-E last year. With
These ACPI core changes and the small changes to the two example drivers,
the Minnowboard can now use the GPIO buttons and LEDs through these
drivers by adding the ASL fragment below to the DSDT.
--
Darren
>
>------ DSDT For Minnowboard ------
>
> Scope (\_SB.PCI0.LPC)
> {
> Device (LEDS)
> {
> Name (_HID, "MNW0001")
>
> Name (_CRS, ResourceTemplate ()
> {
> GpioIo (Exclusive, PullDown, 0, 0,
>IoRestrictionOutputOnly,
> "\\_SB.PCI0.LPC", 0, ResourceConsumer) {10}
> GpioIo (Exclusive, PullDown, 0, 0,
>IoRestrictionOutputOnly,
> "\\_SB.PCI0.LPC", 0, ResourceConsumer) {11}
> })
>
> Device (LEDH)
> {
> Name (_HID, "PRP0000")
> Name (_DSD, Package ()
> {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package ()
> {
> Package () {"label", "Heartbeat"},
> Package () {"gpios", Package () {^^LEDS, 0, 0,
>0}},
> Package () {"linux,default-trigger", "heartbeat"},
> Package () {"linux,default-state", "off"},
> Package () {"linux,retain-state-suspended", 1},
> }
> })
> }
>
> Device (LEDM)
> {
> Name (_HID, "PRP0000")
> Name (_DSD, Package ()
> {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package ()
> {
> Package () {"label", "MMC0 Activity"},
> Package () {"gpios", Package () {^^LEDS, 1, 0,
>0}},
> Package () {"linux,default-trigger", "mmc0"},
> Package () {"linux,default-state", "on"},
> Package () {"linux,retain-state-suspended", 1},
> }
> })
> }
> }
>
> Device (BTNS)
> {
> Name (_HID, "MNW0002")
>
> Name (_CRS, ResourceTemplate ()
> {
> GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly,
> "\\_SB.PCI0.LPC", 0, ResourceConsumer) {0}
> GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly,
> "\\_SB.PCI0.LPC", 0, ResourceConsumer) {1}
> GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly,
> "\\_SB.PCI0.LPC", 0, ResourceConsumer) {2}
> GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly,
> "\\_SB.PCI0.LPC", 0, ResourceConsumer) {3}
> })
>
> Name (_DSD, Package ()
> {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package ()
> {
> Package () {"poll-interval", 100},
> Package () {"autorepeat", 1}
> }
> })
>
> Device (BTN0)
> {
> Name (_HID, "PRP0000")
> Name (_DSD, Package ()
> {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package ()
> {
> Package () {"linux,code", 105},
> Package () {"linux,input-type", 1},
> Package () {"gpios", Package () {^^BTNS, 0, 0,
>1}},
> }
> })
> }
>
> Device (BTN1)
> {
> Name (_HID, "PRP0000")
> Name (_DSD, Package ()
> {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package ()
> {
> Package () {"linux,code", 108},
> Package () {"linux,input-type", 1},
> Package () {"gpios", Package (4) {^^BTNS, 1, 0,
>1}},
> }
> })
> }
>
> Device (BTN2)
> {
> Name (_HID, "PRP0000")
> Name (_DSD, Package ()
> {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package ()
> {
> Package () {"linux,code", 103},
> Package () {"linux,input-type", 1},
> Package () {"gpios", Package () {^^BTNS, 2, 0,
>1}},
> }
> })
> }
>
> Device (BTN3)
> {
> Name (_HID, "PRP0000")
> Name (_DSD, Package ()
> {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package ()
> {
> Package () {"linux,code", 106},
> Package () {"linux,input-type", 1},
> Package () {"gpios", Package (4) {^^BTNS, 3, 0,
>1}},
> }
> })
> }
>
> }
>
> }
>
>--
>2.1.0.rc1
>
>
--
Darren Hart Open Source Technology Center
darren.hart@intel.com Intel Corporation
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [RFC PATCH 0/9] Add ACPI _DSD and unified device properties support
2014-08-16 16:06 ` Darren Hart
@ 2014-08-17 14:11 ` Dmitry Torokhov
-1 siblings, 0 replies; 39+ messages in thread
From: Dmitry Torokhov @ 2014-08-17 14:11 UTC (permalink / raw)
To: Darren Hart
Cc: Mika Westerberg, Rafael J. Wysocki, Al Stone, Olof Johansson,
Matthew Garrett, Matt Fleming, David Woodhouse, H. Peter Anvin,
Jacob Pan, Josh Triplett, Aaron Lu, Max Eliaser, Robert Moore,
Len Brown, Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot,
Mark Brown, Bryan Wu, Richard Purdie, Samuel Ortiz, Lee Jones,
Grant Likely
On Sat, Aug 16, 2014 at 09:06:05AM -0700, Darren Hart wrote:
> On 8/15/14, 23:53, "Mika Westerberg" <mika.westerberg@linux.intel.com>
> wrote:
>
> >The recent publication of the ACPI 5.1 specification [1] adds a reserved
> >name
> >for Device Specific Data (_DSD, Section 6.2.5). This mechanism allows for
> >passing arbitrary hardware description data to the OS. The exact format
> >of the
> >_DSD data is specific to the UUID paired with it [2].
> >
> >An ACPI Device Properties UUID has been defined [3] to provide a format
> >compatible with existing device tree schemas. The purpose for this was to
> >allow
> >for the reuse of the existing schemas and encourage the development of
> >firmware
> >agnostic device drivers.
> >
> >This series accomplishes the following (as well as some other
> >dependencies):
> >
> > * Add _DSD support to the ACPI core
> > This simply reads the UUID and the accompanying Package
> >
> > * Add ACPI Device Properties _DSD format support
> > This understands the hierarchical key:value pair structure
> > defined by the Device Properties UUID
> >
> > * Add a unified device properties API with ACPI and OF backends
> > This provides for the firmware agnostic device properties
> > Interface to be used by drivers
> >
> > * Provides 2 example drivers that were previously Device Tree aware that
> > can now be used with either Device Tree or ACPI Device Properties. The
> > both drivers use an arbitrary _HID.
> >
> >This has been tested on Minnowboard with relevant parts of the modified
> >DSDT at the end of this email.
>
>
> This eliminates the need for the board files that were the subject of my
> "How not to write x86 platform drivers" talk at ELC-E last year. With
> These ACPI core changes and the small changes to the two example drivers,
> the Minnowboard can now use the GPIO buttons and LEDs through these
> drivers by adding the ASL fragment below to the DSDT.
>From the drivers perspective I am less than impressed with the need to
reshuffle all the drivers to support ACPI with the new API. I thought the plan
was to try and keep OF API and try to translate as much as possible to it?
The same goes for bringing arbitrary HIDs into the drivers. Can we have HID->OF
naming hidden in ACPI (define a new property like "dt-name", "of_compat" or
whatever) and have ACPI core map one to another.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [RFC PATCH 0/9] Add ACPI _DSD and unified device properties support
@ 2014-08-17 14:11 ` Dmitry Torokhov
0 siblings, 0 replies; 39+ messages in thread
From: Dmitry Torokhov @ 2014-08-17 14:11 UTC (permalink / raw)
To: Darren Hart
Cc: Mika Westerberg, Rafael J. Wysocki, Al Stone, Olof Johansson,
Matthew Garrett, Matt Fleming, David Woodhouse, H. Peter Anvin,
Jacob Pan, Josh Triplett, Aaron Lu, Max Eliaser, Robert Moore,
Len Brown, Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot,
Mark Brown, Bryan Wu, Richard Purdie, Samuel Ortiz, Lee Jones,
Grant Likely, Rob Herring, linux-acpi, devicetree, linux-kernel
On Sat, Aug 16, 2014 at 09:06:05AM -0700, Darren Hart wrote:
> On 8/15/14, 23:53, "Mika Westerberg" <mika.westerberg@linux.intel.com>
> wrote:
>
> >The recent publication of the ACPI 5.1 specification [1] adds a reserved
> >name
> >for Device Specific Data (_DSD, Section 6.2.5). This mechanism allows for
> >passing arbitrary hardware description data to the OS. The exact format
> >of the
> >_DSD data is specific to the UUID paired with it [2].
> >
> >An ACPI Device Properties UUID has been defined [3] to provide a format
> >compatible with existing device tree schemas. The purpose for this was to
> >allow
> >for the reuse of the existing schemas and encourage the development of
> >firmware
> >agnostic device drivers.
> >
> >This series accomplishes the following (as well as some other
> >dependencies):
> >
> > * Add _DSD support to the ACPI core
> > This simply reads the UUID and the accompanying Package
> >
> > * Add ACPI Device Properties _DSD format support
> > This understands the hierarchical key:value pair structure
> > defined by the Device Properties UUID
> >
> > * Add a unified device properties API with ACPI and OF backends
> > This provides for the firmware agnostic device properties
> > Interface to be used by drivers
> >
> > * Provides 2 example drivers that were previously Device Tree aware that
> > can now be used with either Device Tree or ACPI Device Properties. The
> > both drivers use an arbitrary _HID.
> >
> >This has been tested on Minnowboard with relevant parts of the modified
> >DSDT at the end of this email.
>
>
> This eliminates the need for the board files that were the subject of my
> "How not to write x86 platform drivers" talk at ELC-E last year. With
> These ACPI core changes and the small changes to the two example drivers,
> the Minnowboard can now use the GPIO buttons and LEDs through these
> drivers by adding the ASL fragment below to the DSDT.
>From the drivers perspective I am less than impressed with the need to
reshuffle all the drivers to support ACPI with the new API. I thought the plan
was to try and keep OF API and try to translate as much as possible to it?
The same goes for bringing arbitrary HIDs into the drivers. Can we have HID->OF
naming hidden in ACPI (define a new property like "dt-name", "of_compat" or
whatever) and have ACPI core map one to another.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [RFC PATCH 0/9] Add ACPI _DSD and unified device properties support
2014-08-17 14:11 ` Dmitry Torokhov
(?)
@ 2014-08-17 16:52 ` Darren Hart
-1 siblings, 0 replies; 39+ messages in thread
From: Darren Hart @ 2014-08-17 16:52 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Mika Westerberg, Rafael J. Wysocki, Al Stone, Olof Johansson,
Matthew Garrett, Matt Fleming, David Woodhouse, H. Peter Anvin,
Jacob Pan, Josh Triplett, Aaron Lu, Max Eliaser, Robert Moore,
Len Brown, Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot,
Mark Brown, Bryan Wu, Richard Purdie, Samuel Ortiz, Lee Jones,
Grant Likely, Rob Herring, linux-acpi, devicetree, linux-kernel
On 8/17/14, 7:11, "Dmitry Torokhov" <dmitry.torokhov@gmail.com> wrote:
>On Sat, Aug 16, 2014 at 09:06:05AM -0700, Darren Hart wrote:
>> On 8/15/14, 23:53, "Mika Westerberg" <mika.westerberg@linux.intel.com>
>> wrote:
>>
>> >The recent publication of the ACPI 5.1 specification [1] adds a
>>reserved
>> >name
>> >for Device Specific Data (_DSD, Section 6.2.5). This mechanism allows
>>for
>> >passing arbitrary hardware description data to the OS. The exact format
>> >of the
>> >_DSD data is specific to the UUID paired with it [2].
>> >
>> >An ACPI Device Properties UUID has been defined [3] to provide a format
>> >compatible with existing device tree schemas. The purpose for this was
>>to
>> >allow
>> >for the reuse of the existing schemas and encourage the development of
>> >firmware
>> >agnostic device drivers.
>> >
>> >This series accomplishes the following (as well as some other
>> >dependencies):
>> >
>> > * Add _DSD support to the ACPI core
>> > This simply reads the UUID and the accompanying Package
>> >
>> > * Add ACPI Device Properties _DSD format support
>> > This understands the hierarchical key:value pair structure
>> > defined by the Device Properties UUID
>> >
>> > * Add a unified device properties API with ACPI and OF backends
>> > This provides for the firmware agnostic device properties
>> > Interface to be used by drivers
>> >
>> > * Provides 2 example drivers that were previously Device Tree aware
>>that
>> > can now be used with either Device Tree or ACPI Device Properties.
>>The
>> > both drivers use an arbitrary _HID.
>> >
>> >This has been tested on Minnowboard with relevant parts of the modified
>> >DSDT at the end of this email.
>>
>>
>> This eliminates the need for the board files that were the subject of my
>> "How not to write x86 platform drivers" talk at ELC-E last year. With
>> These ACPI core changes and the small changes to the two example
>>drivers,
>> the Minnowboard can now use the GPIO buttons and LEDs through these
>> drivers by adding the ASL fragment below to the DSDT.
>
>>From the drivers perspective I am less than impressed with the need to
>reshuffle all the drivers to support ACPI with the new API. I thought the
>plan
>was to try and keep OF API and try to translate as much as possible to it?
Hi Dmitry,
We discussed this point at length. Ultimately we felt that any solution
would require changes to what the drivers use today. The OF interface uses
OF-domain-specific structures that would have to be wrapped and translated
for ACPI. Providing a firmware-agnostic device properties interface allows
for drivers to be unaware of the underlying firmware implementation, and
for each firmware backend to be a clean implementation with structures and
logic that is appropriate for each.
I do acknowledge the challenge associated with updating the 200+ OF-aware
drivers in the kernel today. The conversion itself is fairly straight
forward, but testing each will require a significant coordination effort.
>
>The same goes for bringing arbitrary HIDs into the drivers. Can we have
>HID->OF
>naming hidden in ACPI (define a new property like "dt-name", "of_compat"
>or
>whatever) and have ACPI core map one to another.
Another topic of some debate. We have discussed the following:
OF-compatible schemas implemented in ACPI should use the Device Properties
HID, PRP0000, and contain a "compatible" property listing the driver. This
removes the need to allocate new HIDs and reduces the changes to the
current OF drivers (although only minimally so). However, this would
require changes to the driver core to match not only on ACPI HID, but also
include the _DSD "compatible" property - something some developers have
objected to.
I believe the PRP0000 method makes a lot of sense for platform devices, as
opposed to "real" hardware, for things like gpio-keys or leds-gpio where
the driver is mostly glue logic as opposed to providing the hardware
communication itself.
Thanks,
--
Darren Hart Open Source Technology Center
darren.hart@intel.com Intel Corporation
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [RFC PATCH 0/9] Add ACPI _DSD and unified device properties support
2014-08-16 6:53 [RFC PATCH 0/9] Add ACPI _DSD and unified device properties support Mika Westerberg
@ 2014-08-16 18:48 ` Josh Triplett
2014-08-16 6:53 ` [RFC PATCH 2/9] ACPI: Document ACPI " Mika Westerberg
` (10 subsequent siblings)
11 siblings, 0 replies; 39+ messages in thread
From: Josh Triplett @ 2014-08-16 18:48 UTC (permalink / raw)
To: Mika Westerberg
Cc: Darren Hart, Rafael J. Wysocki, Al Stone, Olof Johansson,
Matthew Garrett, Matt Fleming, David Woodhouse, H. Peter Anvin,
Jacob Pan, Aaron Lu, Max Eliaser, Robert Moore, Len Brown,
Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot, Mark Brown,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Grant Likely
On Sat, Aug 16, 2014 at 09:53:50AM +0300, Mika Westerberg wrote:
> The recent publication of the ACPI 5.1 specification [1] adds a reserved name
> for Device Specific Data (_DSD, Section 6.2.5). This mechanism allows for
> passing arbitrary hardware description data to the OS. The exact format of the
> _DSD data is specific to the UUID paired with it [2].
>
> An ACPI Device Properties UUID has been defined [3] to provide a format
> compatible with existing device tree schemas. The purpose for this was to allow
> for the reuse of the existing schemas and encourage the development of firmware
> agnostic device drivers.
>
> This series accomplishes the following (as well as some other dependencies):
>
> * Add _DSD support to the ACPI core
> This simply reads the UUID and the accompanying Package
>
> * Add ACPI Device Properties _DSD format support
> This understands the hierarchical key:value pair structure
> defined by the Device Properties UUID
>
> * Add a unified device properties API with ACPI and OF backends
> This provides for the firmware agnostic device properties
> Interface to be used by drivers
>
> * Provides 2 example drivers that were previously Device Tree aware that
> can now be used with either Device Tree or ACPI Device Properties. The
> both drivers use an arbitrary _HID.
>
> This has been tested on Minnowboard with relevant parts of the modified
> DSDT at the end of this email.
>
> This series does not provide for a means to append to a system DSDT. That
> will ultimately be required to make the most effective use of the _DSD
> mechanism. Work is underway on that as a separate effort.
>
> [1] http://www.uefi.org/sites/default/files/resources/ACPI_5_1release.pdf
> [2] http://www.uefi.org/sites/default/files/resources/_DSD-implementation-guide-toplevel.htm
> [3] http://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf
>
> Aaron Lu (3):
> of: Add property_ops callback for devices with of_node
> gpiolib: add API to get gpio desc and flags
> Input: gpio_keys_polled - Make use of device property API
>
> Max Eliaser (1):
> leds: leds-gpio: Make use of device property API
>
> Mika Westerberg (4):
> ACPI: Add support for device specific properties
> ACPI: Document ACPI device specific properties
> mfd: Add ACPI support
> gpio: sch: Consolidate core and resume banks
>
> Rafael J. Wysocki (1):
> Driver core: Unified device properties interface for platform firmware
One issue I noticed with the series: the new read functions, when given
an integer type, don't appear to do any range-checking to make sure the
returned value falls within the range of that type. They should do so,
and return -EOVERFLOW if so. And the documentation of the new functions
should note that explicitly as an error case.
With that fixed:
Reviewed-by: Josh Triplett <josh@joshtriplett.org>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [RFC PATCH 0/9] Add ACPI _DSD and unified device properties support
@ 2014-08-16 18:48 ` Josh Triplett
0 siblings, 0 replies; 39+ messages in thread
From: Josh Triplett @ 2014-08-16 18:48 UTC (permalink / raw)
To: Mika Westerberg
Cc: Darren Hart, Rafael J. Wysocki, Al Stone, Olof Johansson,
Matthew Garrett, Matt Fleming, David Woodhouse, H. Peter Anvin,
Jacob Pan, Aaron Lu, Max Eliaser, Robert Moore, Len Brown,
Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot, Mark Brown,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Grant Likely, Rob Herring, linux-acpi, devicetree,
linux-kernel
On Sat, Aug 16, 2014 at 09:53:50AM +0300, Mika Westerberg wrote:
> The recent publication of the ACPI 5.1 specification [1] adds a reserved name
> for Device Specific Data (_DSD, Section 6.2.5). This mechanism allows for
> passing arbitrary hardware description data to the OS. The exact format of the
> _DSD data is specific to the UUID paired with it [2].
>
> An ACPI Device Properties UUID has been defined [3] to provide a format
> compatible with existing device tree schemas. The purpose for this was to allow
> for the reuse of the existing schemas and encourage the development of firmware
> agnostic device drivers.
>
> This series accomplishes the following (as well as some other dependencies):
>
> * Add _DSD support to the ACPI core
> This simply reads the UUID and the accompanying Package
>
> * Add ACPI Device Properties _DSD format support
> This understands the hierarchical key:value pair structure
> defined by the Device Properties UUID
>
> * Add a unified device properties API with ACPI and OF backends
> This provides for the firmware agnostic device properties
> Interface to be used by drivers
>
> * Provides 2 example drivers that were previously Device Tree aware that
> can now be used with either Device Tree or ACPI Device Properties. The
> both drivers use an arbitrary _HID.
>
> This has been tested on Minnowboard with relevant parts of the modified
> DSDT at the end of this email.
>
> This series does not provide for a means to append to a system DSDT. That
> will ultimately be required to make the most effective use of the _DSD
> mechanism. Work is underway on that as a separate effort.
>
> [1] http://www.uefi.org/sites/default/files/resources/ACPI_5_1release.pdf
> [2] http://www.uefi.org/sites/default/files/resources/_DSD-implementation-guide-toplevel.htm
> [3] http://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf
>
> Aaron Lu (3):
> of: Add property_ops callback for devices with of_node
> gpiolib: add API to get gpio desc and flags
> Input: gpio_keys_polled - Make use of device property API
>
> Max Eliaser (1):
> leds: leds-gpio: Make use of device property API
>
> Mika Westerberg (4):
> ACPI: Add support for device specific properties
> ACPI: Document ACPI device specific properties
> mfd: Add ACPI support
> gpio: sch: Consolidate core and resume banks
>
> Rafael J. Wysocki (1):
> Driver core: Unified device properties interface for platform firmware
One issue I noticed with the series: the new read functions, when given
an integer type, don't appear to do any range-checking to make sure the
returned value falls within the range of that type. They should do so,
and return -EOVERFLOW if so. And the documentation of the new functions
should note that explicitly as an error case.
With that fixed:
Reviewed-by: Josh Triplett <josh@joshtriplett.org>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [RFC PATCH 0/9] Add ACPI _DSD and unified device properties support
2014-08-16 6:53 [RFC PATCH 0/9] Add ACPI _DSD and unified device properties support Mika Westerberg
@ 2014-08-17 6:55 ` Mika Westerberg
2014-08-16 6:53 ` [RFC PATCH 2/9] ACPI: Document ACPI " Mika Westerberg
` (10 subsequent siblings)
11 siblings, 0 replies; 39+ messages in thread
From: Mika Westerberg @ 2014-08-17 6:55 UTC (permalink / raw)
To: Darren Hart, Rafael J. Wysocki
Cc: Al Stone, Olof Johansson, Matthew Garrett, Matt Fleming,
David Woodhouse, H. Peter Anvin, Jacob Pan, Josh Triplett,
Aaron Lu, Max Eliaser, Robert Moore, Len Brown,
Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot, Mark Brown,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Grant Likely, Rob Herring, linux-acpi
On Sat, Aug 16, 2014 at 09:53:50AM +0300, Mika Westerberg wrote:
> The recent publication of the ACPI 5.1 specification [1] adds a reserved name
> for Device Specific Data (_DSD, Section 6.2.5). This mechanism allows for
> passing arbitrary hardware description data to the OS. The exact format of the
> _DSD data is specific to the UUID paired with it [2].
>
> An ACPI Device Properties UUID has been defined [3] to provide a format
> compatible with existing device tree schemas. The purpose for this was to allow
> for the reuse of the existing schemas and encourage the development of firmware
> agnostic device drivers.
>
> This series accomplishes the following (as well as some other dependencies):
>
> * Add _DSD support to the ACPI core
> This simply reads the UUID and the accompanying Package
>
> * Add ACPI Device Properties _DSD format support
> This understands the hierarchical key:value pair structure
> defined by the Device Properties UUID
>
> * Add a unified device properties API with ACPI and OF backends
> This provides for the firmware agnostic device properties
> Interface to be used by drivers
>
> * Provides 2 example drivers that were previously Device Tree aware that
> can now be used with either Device Tree or ACPI Device Properties. The
> both drivers use an arbitrary _HID.
>
> This has been tested on Minnowboard with relevant parts of the modified
> DSDT at the end of this email.
>
> This series does not provide for a means to append to a system DSDT. That
> will ultimately be required to make the most effective use of the _DSD
> mechanism. Work is underway on that as a separate effort.
>
> [1] http://www.uefi.org/sites/default/files/resources/ACPI_5_1release.pdf
> [2] http://www.uefi.org/sites/default/files/resources/_DSD-implementation-guide-toplevel.htm
> [3] http://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf
For some reason this series never reached the mailing lists intented, so
I re-sent the series with shorter CC list to LKML, linux-acpi and
devicetree lists.
LKML thread is here:
https://lkml.org/lkml/2014/8/17/10
Sorry for the inconvenience.
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [RFC PATCH 0/9] Add ACPI _DSD and unified device properties support
@ 2014-08-17 6:55 ` Mika Westerberg
0 siblings, 0 replies; 39+ messages in thread
From: Mika Westerberg @ 2014-08-17 6:55 UTC (permalink / raw)
To: Darren Hart, Rafael J. Wysocki
Cc: Al Stone, Olof Johansson, Matthew Garrett, Matt Fleming,
David Woodhouse, H. Peter Anvin, Jacob Pan, Josh Triplett,
Aaron Lu, Max Eliaser, Robert Moore, Len Brown,
Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot, Mark Brown,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Grant Likely, Rob Herring, linux-acpi, devicetree,
linux-kernel
On Sat, Aug 16, 2014 at 09:53:50AM +0300, Mika Westerberg wrote:
> The recent publication of the ACPI 5.1 specification [1] adds a reserved name
> for Device Specific Data (_DSD, Section 6.2.5). This mechanism allows for
> passing arbitrary hardware description data to the OS. The exact format of the
> _DSD data is specific to the UUID paired with it [2].
>
> An ACPI Device Properties UUID has been defined [3] to provide a format
> compatible with existing device tree schemas. The purpose for this was to allow
> for the reuse of the existing schemas and encourage the development of firmware
> agnostic device drivers.
>
> This series accomplishes the following (as well as some other dependencies):
>
> * Add _DSD support to the ACPI core
> This simply reads the UUID and the accompanying Package
>
> * Add ACPI Device Properties _DSD format support
> This understands the hierarchical key:value pair structure
> defined by the Device Properties UUID
>
> * Add a unified device properties API with ACPI and OF backends
> This provides for the firmware agnostic device properties
> Interface to be used by drivers
>
> * Provides 2 example drivers that were previously Device Tree aware that
> can now be used with either Device Tree or ACPI Device Properties. The
> both drivers use an arbitrary _HID.
>
> This has been tested on Minnowboard with relevant parts of the modified
> DSDT at the end of this email.
>
> This series does not provide for a means to append to a system DSDT. That
> will ultimately be required to make the most effective use of the _DSD
> mechanism. Work is underway on that as a separate effort.
>
> [1] http://www.uefi.org/sites/default/files/resources/ACPI_5_1release.pdf
> [2] http://www.uefi.org/sites/default/files/resources/_DSD-implementation-guide-toplevel.htm
> [3] http://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf
For some reason this series never reached the mailing lists intented, so
I re-sent the series with shorter CC list to LKML, linux-acpi and
devicetree lists.
LKML thread is here:
https://lkml.org/lkml/2014/8/17/10
Sorry for the inconvenience.
^ permalink raw reply [flat|nested] 39+ messages in thread