> -----Original Message----- > From: Mika Westerberg [mailto:mika.westerberg@xxxxxxxxxxxxxxx] > Sent: 04 April, 2016 16:37 > To: Tirdea, Irina > Cc: Rafael J. Wysocki; Len Brown; Linus Walleij; linux-gpio@xxxxxxxxxxxxxxx; linux-acpi@xxxxxxxxxxxxxxx; Rob Herring; Heikki Krogerus; > Andy Shevchenko; Purdila, Octavian; Ciocan, Cristina; devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx > Subject: Re: [RFC PATCH 3/4] pinctrl: Add ACPI support > > On Thu, Mar 31, 2016 at 02:44:44PM +0300, Irina Tirdea wrote: > > Add ACPI support for pin controller properties. These are > > based on ACPI _DSD properties and follow the device tree > > model based on states and node configurations. The states > > are defined as _DSD properties and configuration nodes > > are defined using the _DSD Hierarchical Properties Extension. > > > > A configuration node supports the generic device tree properties. > > > > The implementation is based on device tree code from devicetree.c. > > > > Signed-off-by: Irina Tirdea <irina.tirdea@xxxxxxxxx> > > Looks good to me. Few minor comments below, though. Thanks for the review, Mika! > > > --- > > Documentation/acpi/pinctrl-properties.txt | 274 +++++++++++++++++++++++++ > > drivers/pinctrl/Makefile | 1 + > > drivers/pinctrl/acpi.c | 322 ++++++++++++++++++++++++++++++ > > drivers/pinctrl/acpi.h | 32 +++ > > drivers/pinctrl/core.c | 26 +++ > > drivers/pinctrl/core.h | 2 + > > 6 files changed, 657 insertions(+) > > create mode 100644 Documentation/acpi/pinctrl-properties.txt > > create mode 100644 drivers/pinctrl/acpi.c > > create mode 100644 drivers/pinctrl/acpi.h > > > > diff --git a/Documentation/acpi/pinctrl-properties.txt b/Documentation/acpi/pinctrl-properties.txt > > new file mode 100644 > > index 0000000..e93aaaf > > --- /dev/null > > +++ b/Documentation/acpi/pinctrl-properties.txt > > @@ -0,0 +1,274 @@ > > += _DSD Device Properties related to pin controllers = > > + > > +== Introduction == > > + > > +This document is an extension of the pin control subsystem in Linux [1] > > +and provides a way to describe pin controller properties in ACPI. It is > > +based on the Device Specific Data (_DSD) configuration object [2] that > > +was introduced in ACPI 5.1. > > + > > +Pin controllers are hardware modules that control pins by allowing pin > > +multiplexing and configuration. Pin multiplexing allows using the same > > +physical pins for multiple functions; for example, one pin or group of pins > > +may be used for the I2C bus, SPI bus or as general-purpose GPIO pin. Pin > > +configuration allows setting various properties such as pull-up/down, > > +tri-state, drive-strength, etc. > > + > > +Hardware modules whose signals are affected by pin configuration are > > +designated client devices. For a client device to operate correctly, > > +certain pin controllers must set up certain specific pin configurations. > > +Some client devices need a single static pin configuration, e.g. set up > > +during initialization. Others need to reconfigure pins at run-time, > > +for example to tri-state pins when the device is inactive. Hence, each > > +client device can define a set of named states. Each named state is > > +mapped to a pin controller configuration that describes the pin multiplexing > > +or configuration for that state. > > + > > +In ACPI, each pin controller and each client device is represented as an > > +ACPI device, just like any other hardware module. The pin controller > > +properties are defined using _DSD properties [2] under these devices. > > +The named states are defined using Device Properties UUID [3] under the > > +ACPI client device. The configuration nodes are defined using Hierarchical > > +Properties Extension UUID [4] and are split between the ACPI client device > > +and the pin controller device. The configuration nodes contain properties > > +that describe pin multiplexing or configuration that very similar to the > > +ones used for device tree [5]. > > + > > +== Example == > > + > > +For example, let's consider an accelerometer connected to the I2C bus on > > +a platform with a Baytrail pin controller. The accelerometer uses 2 GPIO > > +pins for I2C (SDA, SCL) and one GPIO pin for interrupt. > > + > > +The name for the pins, groups and functions used are the ones defined in the > > +pin controller driver, in the same way as it is done for device tree [5]. > > + > > +For the I2C pins, the pin controller driver defines one group called > > +"i2c5_grp" that can be multiplexed with functions "i2c" or "gpio". > > +In our case, we need to select function "i2c" for group "i2c5_grp" in > > +the ACPI description. > > + > > +For the GPIO pin, the pin controller driver defines the name "GPIO_S5[00]" > > BTW, those pin names were changed for Baytrail pinctrl driver so you > should probably do that here also. > Right, will update it according to the current naming. > > +for the pin with index 0 that we use. We need to configure this pin to > > +pull-down with pull strength of 10000 Ohms. We might also want to disable > > +the bias for the GPIO interrupt pin when entering sleep. > > + > > +Here is an ASL example for this device: > > + > > + // Pin controller device > > + Scope (_SB.GPO0) > > + { > > + Name (MUX0, Package() > > + { > > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > > + Package() > > + { > > + Package (2) {"function", "i2c"}, > > + Package (2) {"groups", Package () {"i2c5_grp"}}, > > + } > > + }) > > + > > + Name (CFG0, Package() > > + { > > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > > + Package() > > + { > > + Package (2) {"pins", Package () {"GPIO_S5[00]"}}, > > + Package (2) {"bias-pull-down", 10000}, > > + } > > + }) > > + > > + Name (CFG1, Package() > > + { > > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > > + Package() > > + { > > + Package (2) {"pins", Package () {"GPIO_S5[00]"}}, > > + Package (2) {"bias-disable", 0}, > > + } > > + }) > > + } > > + > > + <snip> > > + Scope (GPO0) > > + { > > + Name (DPN0, Package() > > Maybe we should use node names that relate to the pinctrl subsystem and > not the ones used in the hierarchical properties extension example? I mean > real examples. > OK, I will use MUX0 and CFG0 instead. > > + { > > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > > + Package() {...} > > + }) > > + Name (DPN1, Package() > > + { > > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > > + Package() {...} > > + }) > > + } > > + > > +Each DPN data subnode is a regular _DSD node that uses Device Properties > > +UUID [3]. There are 2 types of subnodes, depending on the properties it > > +contains: pin multiplexing nodes and pin configuration nodes. > > + > > +==== Pin multiplexing nodes ==== > > + > > +The pin multiplexing nodes must contain a property named "function" and > > +define a mux function to be applied to a list of pin groups. The properties > > +supported by this node are the same as for device tree [5]. The name for the > > +pins, groups and functions used are the ones defined in the pin controller > > +driver, in the same way as it is done for device tree [5]. The format for > > +this data subnode is: > > + > > + Name (DPN0, Package() > > Same here. > > > + { > > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > > + Package() > > + { > > + Package (2) {"function", functioname}, > > + Package (2) {"groups", Package () {groupname1, groupname2, ...}}, > > + } > > + }) > > + > > + functioname - the pinmux function to select. > > + groups - the list of groups to select with this function > > + > > +==== Pin configuration nodes ==== > > + > > +The pin configuration nodes do not contain a property named "function". > > +They must contain a property named "group" or "pins". They will also > > +contain one or more configuration properties like bias-pull-up, > > +drive-open-drain, etc. The properties supported by this node are the > > +same as for device tree. Standard pinctrl properties are defined in the > > +device tree documentation [5] and in <include/linux/pinctrl/pinconf-generic.h>. > > +Pinctrl drivers may also define their own custom properties. The name for the > > +pins/groups used are the ones defined in the pin controller driver, in the > > +same way as it is done for device tree [5]. The format for the data subnode is: > > + > > + Name (DPN0, Package() > > And here. > > > + { > > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > > + Package() > > + { > > + Package (2) {"pins", Package () {pin1, pin2, ...}}, > > + Package (2) {configname1, configval1}, > > These should be enclosed in quotes, like "configname1" and so on. > OK. Should all string properties be enclosed in quotes or only the property names (e.g. should I also change values like "configval1", "pin1", "statename1", etc.)? > > + Package (2) {configname2, configval2}, > > + } > > + }) > > + > > + pins - list of pins that properties in the node apply to > > + configname - name of the pin configuration property > > + configval - value of the pin configuration property > > + > > +== References == > > + > > +[1] Documentation/pinctrl.txt > > +[2] http://www.uefi.org/sites/default/files/resources/_DSD-implementation-guide-toplevel-1_1.htm > > +[3] http://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf > > +[4] http://www.uefi.org/sites/default/files/resources/_DSD-hierarchical-data-extension-UUID-v1.pdf > > +[5] Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt > > diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile > > index e4bc115..12d3af6 100644 > > --- a/drivers/pinctrl/Makefile > > +++ b/drivers/pinctrl/Makefile > > @@ -6,6 +6,7 @@ obj-y += core.o pinctrl-utils.o > > obj-$(CONFIG_PINMUX) += pinmux.o > > obj-$(CONFIG_PINCONF) += pinconf.o > > obj-$(CONFIG_OF) += devicetree.o > > +obj-$(CONFIG_ACPI) += acpi.o > > obj-$(CONFIG_GENERIC_PINCONF) += pinconf-generic.o > > obj-$(CONFIG_PINCTRL_ADI2) += pinctrl-adi2.o > > obj-$(CONFIG_PINCTRL_AS3722) += pinctrl-as3722.o > > diff --git a/drivers/pinctrl/acpi.c b/drivers/pinctrl/acpi.c > > new file mode 100644 > > index 0000000..bed1d88 > > --- /dev/null > > +++ b/drivers/pinctrl/acpi.c > > @@ -0,0 +1,322 @@ > > +/* > > + * ACPI integration for the pin control subsystem > > + * > > + * Copyright (c) 2016, Intel Corporation. > > + * > > + * Derived from: > > + * devicetree.c - Copyright (C) 2012 NVIDIA CORPORATION > > + * > > + * This program is free software; you can redistribute it and/or modify it > > + * under the terms and conditions of the GNU General Public License, > > + * version 2, as published by the Free Software Foundation. > > + * > > + * This program is distributed in the hope it will be useful, but WITHOUT > > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > > + * more details. > > + */ > > + > > +#include <linux/acpi.h> > > +#include <linux/device.h> > > +#include <linux/pinctrl/pinctrl.h> > > +#include <linux/pinctrl/pinconf-generic.h> > > + > > +#include "acpi.h" > > +#include "core.h" > > +#include "pinconf.h" > > +#include "pinctrl-utils.h" > > + > > +/** > > + * struct pinctrl_acpi_map - mapping table chunk parsed from ACPI > > + * @node: list node for struct pinctrl's @fw_maps field > > + * @pctldev: the pin controller that allocated this struct, and will free it > > + * @maps: the mapping table entries > > + */ > > +struct pinctrl_acpi_map { > > + struct list_head node; > > + struct pinctrl_dev *pctldev; > > + struct pinctrl_map *map; > > + unsigned num_maps; > > +}; > > + > > +static void acpi_maps_list_dh(acpi_handle handle, void *data) > > +{ > > + /* The address of this function is used as a key. */ > > +} > > + > > +static struct list_head *acpi_get_maps(struct device *dev) > > +{ > > + acpi_handle handle = ACPI_HANDLE(dev); > > + struct list_head *maps; > > + acpi_status status; > > + > > + status = acpi_get_data(handle, acpi_maps_list_dh, (void **)&maps); > > + if (ACPI_FAILURE(status)) > > + return NULL; > > + > > + return maps; > > +} > > + > > +static void acpi_free_maps(struct device *dev, struct list_head *maps) > > +{ > > + acpi_handle handle = ACPI_HANDLE(dev); > > + > > + acpi_detach_data(handle, acpi_maps_list_dh); > > + kfree(maps); > > +} > > + > > +static int acpi_init_maps(struct device *dev) > > +{ > > + acpi_handle handle = ACPI_HANDLE(dev); > > + struct list_head *maps; > > + acpi_status status; > > + > > + maps = kzalloc(sizeof(*maps), GFP_KERNEL); > > + if (!maps) > > + return -ENOMEM; > > + > > + INIT_LIST_HEAD(maps); > > + > > + status = acpi_attach_data(handle, acpi_maps_list_dh, maps); > > + if (ACPI_FAILURE(status)) > > This leaks maps. Oops. I'll fix this. > > > + return -EINVAL; > > + > > + return 0; > > +} -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
