Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1371730 > unrolled thread

[RFC PATCH v2 0/3] Add ACPI support for pinctrl configuration

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH v2 0/3] Add ACPI support for pinctrl configuration Irina Tirdea <irina.tirdea@intel.com> - 2016-04-05 17:40 +0200
    [RFC PATCH v2 3/3] pinctrl: Parse GpioInt/GpioIo resources Irina Tirdea <irina.tirdea@intel.com> - 2016-04-05 17:40 +0200
    Re: [RFC PATCH v2 0/3] Add ACPI support for pinctrl configuration Mika Westerberg <mika.westerberg@linux.intel.com> - 2016-04-07 15:00 +0200
      Re: [RFC PATCH v2 0/3] Add ACPI support for pinctrl configuration Linus Walleij <linus.walleij@linaro.org> - 2016-04-07 19:50 +0200
    Re: [RFC PATCH v2 0/3] Add ACPI support for pinctrl configuration Linus Walleij <linus.walleij@linaro.org> - 2016-04-14 14:00 +0200

#1371730 — [RFC PATCH v2 0/3] Add ACPI support for pinctrl configuration

This is a proposal for adding ACPI support for pin controller
configuration.

It has been developed to enable the MinnowBoard and IoT community
by providing an easy way to specify pin multiplexing and
pin configuration.

This proposal is based on using _DSD properties to specify device
states and configuration nodes and it follows closely the device
tree model. Device states are defined using the Device Properties
format and the configuration nodes are defined using the
Hierarchical Properties Extension format. The generic properties
for the configuration nodes are the same as the ones for device
tree, while pincontroller drivers can also define custom ones.

Changes from v1:
 - address code review comments regarding coding style, documentation
and fixes
 - rewrote patch 3 ("pinctrl: Parse GpioInt/GpioIo resources") to
avoid using triple pointers
 - define pinconf_generic_dt_node_to_map since it is used by pinctrl
sirf driver
 - add dependency on PINCONF_GENERIC to the entire ACPI parsing code
 - dropped first patch from the series since it got merged
("pinctrl: Rename pinctrl_utils_dt_free_map to pinctrl_utils_free_map")

Irina Tirdea (3):
  pinctrl: pinconf-generic: Add ACPI support
  pinctrl: Add ACPI support
  pinctrl: Parse GpioInt/GpioIo resources

 Documentation/acpi/pinctrl-properties.txt | 292 +++++++++++++++++
 drivers/acpi/property.c                   |   8 +-
 drivers/base/property.c                   |  36 ++-
 drivers/pinctrl/Makefile                  |   1 +
 drivers/pinctrl/acpi.c                    | 519 ++++++++++++++++++++++++++++++
 drivers/pinctrl/acpi.h                    |  32 ++
 drivers/pinctrl/core.c                    |  26 ++
 drivers/pinctrl/core.h                    |   2 +
 drivers/pinctrl/pinconf-generic.c         | 121 ++++---
 include/linux/acpi.h                      |   4 +-
 include/linux/pinctrl/pinconf-generic.h   |  35 +-
 include/linux/property.h                  |   9 +
 12 files changed, 1010 insertions(+), 75 deletions(-)
 create mode 100644 Documentation/acpi/pinctrl-properties.txt
 create mode 100644 drivers/pinctrl/acpi.c
 create mode 100644 drivers/pinctrl/acpi.h

-- 
1.9.1

[toc] | [next] | [standalone]

#1371732 — [RFC PATCH v2 3/3] pinctrl: Parse GpioInt/GpioIo resources

Parse GpioInt/GpioIo ACPI resources and use the pin configuration
information to generate pin controller maps. These maps are associated
with the "default" state, only if this state is defined in the _DSD
of the acpi device.

Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
---
 Documentation/acpi/pinctrl-properties.txt |   8 ++
 drivers/pinctrl/acpi.c                    | 184 ++++++++++++++++++++++++++++++
 2 files changed, 192 insertions(+)

diff --git a/Documentation/acpi/pinctrl-properties.txt b/Documentation/acpi/pinctrl-properties.txt
index 9cdf6fa..3f949d4 100644
--- a/Documentation/acpi/pinctrl-properties.txt
+++ b/Documentation/acpi/pinctrl-properties.txt
@@ -275,6 +275,14 @@ Pinctrl "sleep" state provides power management capabilities to the device that
 conflict with ACPI power management methods. Special care must be taken when using
 the "sleep" state not to create conflicts with the existing ACPI configuration.
 
+== GpioInt()/GpioIo() _CRS resources ==
+
+If the device has any GpioInt/GpioIo _CRS ACPI resources, they are parsed so
+that the pin configuration information is used (e.g. PullUp/PullDown/PullNone).
+The pin configuration from GpioInt/GpioIo will be associated with the "default"
+state, only if such a state is defined in the _DSD of the device. If no
+"default" state is defined, the GPIO configuration from _CRS will be ignored.
+
 == References ==
 
 [1] Documentation/pinctrl.txt
diff --git a/drivers/pinctrl/acpi.c b/drivers/pinctrl/acpi.c
index 0ddacaf..3b8f824 100644
--- a/drivers/pinctrl/acpi.c
+++ b/drivers/pinctrl/acpi.c
@@ -42,6 +42,17 @@ struct pinctrl_acpi_map {
 	unsigned num_maps;
 };
 
+struct acpi_gpio_lookup {
+	unsigned int index;
+	bool found;
+	unsigned int n;
+	struct pinctrl *p;
+	char *statename;
+};
+
+/* For now we only handle acpi pin config values */
+#define ACPI_MAX_CFGS 1
+
 static void acpi_maps_list_dh(acpi_handle handle, void *data)
 {
 	/* The address of this function is used as a key. */
@@ -152,6 +163,166 @@ static int acpi_remember_or_free_map(struct pinctrl *p, const char *statename,
 	return pinctrl_register_map(map, num_maps, false);
 }
 
+static int acpi_parse_gpio_config(const struct acpi_resource_gpio *agpio,
+				  unsigned long **configs,
+				  unsigned int *nconfigs)
+{
+	enum pin_config_param param;
+	int ret;
+
+	/* Parse configs from GpioInt/GpioIo ACPI resource */
+	*nconfigs = 0;
+	*configs = kcalloc(ACPI_MAX_CFGS, sizeof(*configs), GFP_KERNEL);
+	if (!*configs)
+		return -ENOMEM;
+
+	/* For now, only parse pin_config */
+	switch (agpio->pin_config) {
+	case ACPI_PIN_CONFIG_DEFAULT:
+		param = PIN_CONFIG_BIAS_PULL_PIN_DEFAULT;
+		break;
+	case ACPI_PIN_CONFIG_PULLUP:
+		param = PIN_CONFIG_BIAS_PULL_UP;
+		break;
+	case ACPI_PIN_CONFIG_PULLDOWN:
+		param = PIN_CONFIG_BIAS_PULL_DOWN;
+		break;
+	case ACPI_PIN_CONFIG_NOPULL:
+		param = PIN_CONFIG_BIAS_DISABLE;
+		break;
+	default:
+		ret = -EINVAL;
+		goto exit_free;
+	}
+	*configs[*nconfigs] = pinconf_to_config_packed(param,
+				      param == PIN_CONFIG_BIAS_DISABLE ? 0 : 1);
+	(*nconfigs)++;
+
+	return 0;
+
+exit_free:
+	kfree(*configs);
+	return ret;
+}
+
+static int acpi_gpio_to_map(struct acpi_resource *ares, void *data)
+{
+	unsigned num_maps = 0, reserved_maps = 0;
+	struct acpi_gpio_lookup *lookup = data;
+	const struct acpi_resource_gpio *agpio;
+	acpi_handle pctrl_handle = NULL;
+	struct pinctrl_map *map = NULL;
+	struct pinctrl_dev *pctldev;
+	unsigned int nconfigs, i;
+	unsigned long *configs;
+	acpi_status status;
+	const char *pin;
+	int ret;
+
+	if (ares->type != ACPI_RESOURCE_TYPE_GPIO)
+		return 1;
+	if (lookup->n++ != lookup->index || lookup->found)
+		return 1;
+
+	agpio = &ares->data.gpio;
+
+	/* Get configs from ACPI GPIO resource */
+	ret = acpi_parse_gpio_config(agpio, &configs, &nconfigs);
+	if (ret)
+		return ret;
+
+	/* Get pinctrl reference from GPIO resource */
+	status = acpi_get_handle(NULL, agpio->resource_source.string_ptr,
+				 &pctrl_handle);
+	if (ACPI_FAILURE(status) || !pctrl_handle) {
+		ret = -EINVAL;
+		goto exit_free_configs;
+	}
+
+	/* Find the pin controller */
+	pctldev = get_pinctrl_dev_from_acpi(pctrl_handle);
+	if (!pctldev) {
+		ret = -EINVAL;
+		goto exit_free_configs;
+	}
+
+	/* Allocate space for maps and pinctrl_dev references */
+	ret = pinctrl_utils_reserve_map(pctldev, &map, &reserved_maps,
+					&num_maps, agpio->pin_table_length);
+	if (ret < 0)
+		goto exit_free_configs;
+
+	/* For each GPIO pin */
+	for (i = 0; i < agpio->pin_table_length; i++) {
+		pin = pin_get_name(pctldev, agpio->pin_table[i]);
+		if (!pin) {
+			ret = -EINVAL;
+			goto exit_free_map;
+		}
+		ret = pinctrl_utils_add_map_configs(pctldev, &map,
+						    &reserved_maps,
+						    &num_maps, pin,
+						    configs, nconfigs,
+						    PIN_MAP_TYPE_CONFIGS_PIN);
+		if (ret < 0)
+			goto exit_free_map;
+	}
+
+	ret = acpi_remember_or_free_map(lookup->p, lookup->statename, pctldev,
+					map, num_maps);
+	if (ret < 0)
+		goto exit_free_maps;
+
+	lookup->found = true;
+	kfree(configs);
+	return 1;
+
+exit_free_maps:
+	pinctrl_acpi_free_maps(lookup->p);
+exit_free_map:
+	pinctrl_utils_free_map(NULL, map, num_maps);
+exit_free_configs:
+	kfree(configs);
+	return ret;
+}
+
+static int acpi_parse_gpio_resources(struct pinctrl *p, char *statename)
+{
+	struct acpi_gpio_lookup lookup;
+	struct list_head res_list;
+	struct acpi_device *adev;
+	unsigned int index;
+	int ret;
+
+	adev = ACPI_COMPANION(p->dev);
+
+	memset(&lookup, 0, sizeof(lookup));
+	lookup.p = p;
+	lookup.statename = statename;
+
+	/* Parse all GpioInt/GpioIo resources in _CRS and extract pin conf */
+	for (index = 0; ; index++) {
+		lookup.index = index;
+		lookup.n = 0;
+		lookup.found = false;
+
+		INIT_LIST_HEAD(&res_list);
+		ret = acpi_dev_get_resources(adev, &res_list, acpi_gpio_to_map,
+					     &lookup);
+		if (ret < 0)
+			goto exit_free_maps;
+		acpi_dev_free_resource_list(&res_list);
+		if (!lookup.found)
+			break;
+	}
+
+	return 0;
+
+exit_free_maps:
+	pinctrl_acpi_free_maps(p);
+	return ret;
+}
+
 static int acpi_remember_dummy_state(struct pinctrl *p, const char *statename)
 {
 	struct pinctrl_map *map;
@@ -285,6 +456,19 @@ int pinctrl_acpi_to_map(struct pinctrl *p)
 		}
 		statename = statenames[state].string.pointer;
 
+		/*
+		 * Parse any GpioInt/GpioIo resources and
+		 * associate them with the 'default' state.
+		 */
+		if (!strcmp(statename, PINCTRL_STATE_DEFAULT)) {
+			ret = acpi_parse_gpio_resources(p, statename);
+			if (ret) {
+				dev_err(p->dev,
+					"Could not parse GPIO resources\n");
+				goto err_free_maps;
+			}
+		}
+
 		/* Retrieve the pinctrl-* property */
 		propname = kasprintf(GFP_KERNEL, "pinctrl-%d", state);
 		ret = acpi_dev_get_property(adev, propname, ACPI_TYPE_PACKAGE,
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1373371

On Tue, Apr 05, 2016 at 06:33:23PM +0300, Irina Tirdea wrote:
> This is a proposal for adding ACPI support for pin controller
> configuration.
> 
> It has been developed to enable the MinnowBoard and IoT community
> by providing an easy way to specify pin multiplexing and
> pin configuration.
> 
> This proposal is based on using _DSD properties to specify device
> states and configuration nodes and it follows closely the device
> tree model. Device states are defined using the Device Properties
> format and the configuration nodes are defined using the
> Hierarchical Properties Extension format. The generic properties
> for the configuration nodes are the same as the ones for device
> tree, while pincontroller drivers can also define custom ones.
> 
> Changes from v1:
>  - address code review comments regarding coding style, documentation
> and fixes
>  - rewrote patch 3 ("pinctrl: Parse GpioInt/GpioIo resources") to
> avoid using triple pointers
>  - define pinconf_generic_dt_node_to_map since it is used by pinctrl
> sirf driver
>  - add dependency on PINCONF_GENERIC to the entire ACPI parsing code
>  - dropped first patch from the series since it got merged
> ("pinctrl: Rename pinctrl_utils_dt_free_map to pinctrl_utils_free_map")
> 
> Irina Tirdea (3):
>   pinctrl: pinconf-generic: Add ACPI support
>   pinctrl: Add ACPI support
>   pinctrl: Parse GpioInt/GpioIo resources

The series looks good to me now,

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Of course this all depends on the decision whether this is the preferred
way of controlling and muxing pins in ACPI.

[toc] | [prev] | [next] | [standalone]

#1373602

On Thu, Apr 7, 2016 at 2:56 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Tue, Apr 05, 2016 at 06:33:23PM +0300, Irina Tirdea wrote:

>> Changes from v1:
>>  - address code review comments regarding coding style, documentation
>> and fixes
>>  - rewrote patch 3 ("pinctrl: Parse GpioInt/GpioIo resources") to
>> avoid using triple pointers
>>  - define pinconf_generic_dt_node_to_map since it is used by pinctrl
>> sirf driver
>>  - add dependency on PINCONF_GENERIC to the entire ACPI parsing code
>>  - dropped first patch from the series since it got merged
>> ("pinctrl: Rename pinctrl_utils_dt_free_map to pinctrl_utils_free_map")
>>
>> Irina Tirdea (3):
>>   pinctrl: pinconf-generic: Add ACPI support
>>   pinctrl: Add ACPI support
>>   pinctrl: Parse GpioInt/GpioIo resources
>
> The series looks good to me now,
>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>
> Of course this all depends on the decision whether this is the preferred
> way of controlling and muxing pins in ACPI.

I think the discussion is very interesting and intense right now so
let us see. We need to form some rough consensus before we
know how to proceed.

Yours,
Linus Walleij

[toc] | [prev] | [next] | [standalone]

#1378725

On Tue, Apr 5, 2016 at 5:33 PM, Irina Tirdea <irina.tirdea@intel.com> wrote:

> This is a proposal for adding ACPI support for pin controller
> configuration.

The patch set is tagged RFC and I see that comments you
got a lot of, so I'm looking forward to see what is happening.

I have identified the following stakeholders:

- Irina, Octavian, Mika: want to use ACPI for pin control,
  their draft idea is expressed in this RFC

- Mark R, Mark B, Graeme + more: want to avoid clashes
  with existing or upcoming ACPI standardization attempts

- Rafael as ACPI maintainer for Linux, he seems to be
  in the same diplomatic middle-ground as I am

Clearly this is too high level of conflicting interests for me
to proceed with this proposal as it stands today.

If however we see products deployed using this method as
put forward in the proposal, there is not much to do: we will
just have to support it. I am just hoping that this is not happening
as we speak.

I am somewhat waiting for the ACPI crowd to bring a
counter-proposal to the table. Or some statement of when
they would be able to present one: I certainly understand that
standardization takes time. DT and ACPI both suffer from the
fact that they sometimes need big upfront design.

Yours,
Linus Walleij

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web