[PATCH RFC 0/3] pinctrl: add ACPI support to pin controller

[PATCH RFC 0/3] pinctrl: add ACPI support to pin controller

[Niyas Sait]

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

The patch supports following resources introduced in ACPI from v6.2

- PinFunction
- PinConfig
- PinGroupFunction
- PinGroupConfig
- PinGroup

The patch has been tested on NXP I.MX8MP Plus platform with ACPI.

Niyas Sait (3):
  pinctrl: add support for acpi PinGroup resource
  pinconf-generic: add pull up and pull down config with resistance
  pinctrl: add support for acpi pin function and config resources

 drivers/pinctrl/Makefile                |   1 +
 drivers/pinctrl/core.c                  |  19 +-
 drivers/pinctrl/core.h                  |   3 +
 drivers/pinctrl/pinctrl-acpi.c          | 450 ++++++++++++++++++++++++
 drivers/pinctrl/pinctrl-acpi.h          |  50 +++
 include/linux/pinctrl/pinconf-generic.h |   6 +
 include/linux/pinctrl/pinctrl.h         |  15 +
 7 files changed, 540 insertions(+), 4 deletions(-)
 create mode 100644 drivers/pinctrl/pinctrl-acpi.c
 create mode 100644 drivers/pinctrl/pinctrl-acpi.h

-- 
2.25.1

[PATCH RFC 1/3] pinctrl: add support for acpi PinGroup resource

[Niyas Sait]

pinctrl-acpi parses and decode PinGroup resources for
the device and generate list of group descriptor.
Descriptors can be used by the pin controller to identify
the groups and pins provided in the group.

Signed-off-by: Niyas Sait <niyas.sait@linaro.org>
---
 drivers/pinctrl/Makefile       |  1 +
 drivers/pinctrl/pinctrl-acpi.c | 59 ++++++++++++++++++++++++++++++++++
 drivers/pinctrl/pinctrl-acpi.h | 22 +++++++++++++
 3 files changed, 82 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-acpi.c
 create mode 100644 drivers/pinctrl/pinctrl-acpi.h

diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index e76f5cdc64b0..0b0ec4080942 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_PINMUX)		+= pinmux.o
 obj-$(CONFIG_PINCONF)		+= pinconf.o
 obj-$(CONFIG_GENERIC_PINCONF)	+= pinconf-generic.o
 obj-$(CONFIG_OF)		+= devicetree.o
+obj-$(CONFIG_ACPI)		+= pinctrl-acpi.o
 
 obj-$(CONFIG_PINCTRL_AMD)	+= pinctrl-amd.o
 obj-$(CONFIG_PINCTRL_APPLE_GPIO) += pinctrl-apple-gpio.o
diff --git a/drivers/pinctrl/pinctrl-acpi.c b/drivers/pinctrl/pinctrl-acpi.c
new file mode 100644
index 000000000000..75e59fe22387
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-acpi.c
@@ -0,0 +1,59 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2022 Linaro Ltd.
+ */
+#include <linux/acpi.h>
+#include <linux/errno.h>
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/driver.h>
+#include <linux/gpio/machine.h>
+#include <linux/list.h>
+
+#include "pinctrl-acpi.h"
+
+static int pinctrl_acpi_populate_group_desc(struct acpi_resource *ares, void *data)
+{
+	struct acpi_resource_pin_group *ares_pin_group;
+	struct pinctrl_acpi_group_desc *desc;
+	struct list_head *group_desc_list = data;
+
+	if (ares->type != ACPI_RESOURCE_TYPE_PIN_GROUP)
+		return 1;
+
+	ares_pin_group = &ares->data.pin_group;
+
+	desc = kzalloc(sizeof(struct pinctrl_acpi_group_desc), GFP_KERNEL);
+	if (!desc)
+		return -ENOMEM;
+
+	desc->name = kstrdup_const(ares_pin_group->resource_label.string_ptr, GFP_KERNEL);
+	desc->pins = ares_pin_group->pin_table;
+	desc->num_pins = ares_pin_group->pin_table_length;
+	desc->vendor_data = ares_pin_group->vendor_data;
+	desc->vendor_length = ares_pin_group->vendor_length;
+
+	INIT_LIST_HEAD(&desc->list);
+	list_add(&desc->list, group_desc_list);
+
+	return 1;
+}
+
+/* Get list of acpi pin groups definitions for the controller */
+int pinctrl_acpi_get_pin_groups(struct acpi_device *adev, struct list_head *group_desc_list)
+{
+	struct list_head res_list;
+	int ret;
+
+	INIT_LIST_HEAD(&res_list);
+	INIT_LIST_HEAD(group_desc_list);
+
+	ret = acpi_dev_get_resources(adev, &res_list,
+								 pinctrl_acpi_populate_group_desc,
+								 group_desc_list);
+	if (ret < 0)
+		return ret;
+
+	acpi_dev_free_resource_list(&res_list);
+
+	return 0;
+}
diff --git a/drivers/pinctrl/pinctrl-acpi.h b/drivers/pinctrl/pinctrl-acpi.h
new file mode 100644
index 000000000000..1a0c751a7594
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-acpi.h
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2022 Linaro Ltd.
+ */
+
+struct pinctrl_acpi_group_desc {
+	const char *name;
+	short unsigned int *pins;
+	unsigned num_pins;
+	void *vendor_data;
+	unsigned vendor_length;
+	struct list_head list;
+};
+
+#ifdef CONFIG_ACPI
+int pinctrl_acpi_get_pin_groups(struct acpi_device *adev, struct list_head *group_desc_list);
+#else
+int pinctrl_acpi_get_pin_groups(struct acpi_device *adev, struct list_head *group_desc_list)
+{
+	return -ENODEV;
+}
+#endif
-- 
2.25.1

[PATCH RFC 2/3] pinconf-generic: add pull up and pull down config with resistance

[Niyas Sait]

pin configuration types have been extended to include a pull up
and pull down config with resistance in ohms.

Signed-off-by: Niyas Sait <niyas.sait@linaro.org>
---
 include/linux/pinctrl/pinconf-generic.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
index 2422211d6a5a..57e4b364e877 100644
--- a/include/linux/pinctrl/pinconf-generic.h
+++ b/include/linux/pinctrl/pinconf-generic.h
@@ -36,6 +36,8 @@ struct pinctrl_map;
  * @PIN_CONFIG_BIAS_PULL_DOWN: the pin will be pulled down (usually with high
  *	impedance to GROUND). If the argument is != 0 pull-down is enabled,
  *	if it is 0, pull-down is total, i.e. the pin is connected to GROUND.
+ * @PIN_CONFIG_BIAS_PULL_DOWN_OHMS: the pin will be pulled down with value
+ *  passed as argument. The argument is in ohms.
  * @PIN_CONFIG_BIAS_PULL_PIN_DEFAULT: the pin will be pulled up or down based
  *	on embedded knowledge of the controller hardware, like current mux
  *	function. The pull direction and possibly strength too will normally
@@ -47,6 +49,8 @@ struct pinctrl_map;
  * @PIN_CONFIG_BIAS_PULL_UP: the pin will be pulled up (usually with high
  *	impedance to VDD). If the argument is != 0 pull-up is enabled,
  *	if it is 0, pull-up is total, i.e. the pin is connected to VDD.
+  * @PIN_CONFIG_BIAS_PULL_UP_OHMS: the pin will be pulled up with value
+  * passed as argument. The argument is in ohms.
  * @PIN_CONFIG_DRIVE_OPEN_DRAIN: the pin will be driven with open drain (open
  *	collector) which means it is usually wired with other output ports
  *	which are then pulled up with an external resistor. Setting this
@@ -116,8 +120,10 @@ enum pin_config_param {
 	PIN_CONFIG_BIAS_DISABLE,
 	PIN_CONFIG_BIAS_HIGH_IMPEDANCE,
 	PIN_CONFIG_BIAS_PULL_DOWN,
+	PIN_CONFIG_BIAS_PULL_DOWN_OHMS,
 	PIN_CONFIG_BIAS_PULL_PIN_DEFAULT,
 	PIN_CONFIG_BIAS_PULL_UP,
+	PIN_CONFIG_BIAS_PULL_UP_OHMS,
 	PIN_CONFIG_DRIVE_OPEN_DRAIN,
 	PIN_CONFIG_DRIVE_OPEN_SOURCE,
 	PIN_CONFIG_DRIVE_PUSH_PULL,
-- 
2.25.1

[PATCH RFC 3/3] pinctrl: add support for acpi pin function and config resources

[Niyas Sait]

Add support for following acpi pin resources

- PinFunction
- PinConfig
- PinGroupFunction
- PinGroupConfig

Pinctrl-acpi parses the acpi table and generates list of pin
descriptors that can be used by pin controller to set and config pin.

Descriptors are grouped by pin number or group name and contains list
of functions or configs to apply.

Pin config types from acpi are converted to generic pin config types
and passed through the descriptor.

Signed-off-by: Niyas Sait <niyas.sait@linaro.org>
---
 drivers/pinctrl/core.c          |  19 +-
 drivers/pinctrl/core.h          |   3 +
 drivers/pinctrl/pinctrl-acpi.c  | 391 ++++++++++++++++++++++++++++++++
 drivers/pinctrl/pinctrl-acpi.h  |  28 +++
 include/linux/pinctrl/pinctrl.h |  15 ++
 5 files changed, 452 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index ffe39336fcac..03770ac66d48 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -25,6 +25,7 @@
 #include <linux/pinctrl/consumer.h>
 #include <linux/pinctrl/pinctrl.h>
 #include <linux/pinctrl/machine.h>
+#include <linux/acpi.h>
 
 #ifdef CONFIG_GPIOLIB
 #include "../gpio/gpiolib.h"
@@ -35,7 +36,7 @@
 #include "devicetree.h"
 #include "pinmux.h"
 #include "pinconf.h"
-
+#include "pinctrl-acpi.h"
 
 static bool pinctrl_dummy_state;
 
@@ -1042,9 +1043,15 @@ static struct pinctrl *create_pinctrl(struct device *dev,
 		return ERR_PTR(-ENOMEM);
 	p->dev = dev;
 	INIT_LIST_HEAD(&p->states);
-	INIT_LIST_HEAD(&p->dt_maps);
 
-	ret = pinctrl_dt_to_map(p, pctldev);
+	if (!ACPI_COMPANION(dev)) {
+		INIT_LIST_HEAD(&p->dt_maps);
+		ret = pinctrl_dt_to_map(p, pctldev);
+	} else {
+		INIT_LIST_HEAD(&p->acpi_maps);
+		ret = pinctrl_acpi_to_map(p);
+	}
+
 	if (ret < 0) {
 		kfree(p);
 		return ERR_PTR(ret);
@@ -1168,7 +1175,11 @@ static void pinctrl_free(struct pinctrl *p, bool inlist)
 		kfree(state);
 	}
 
-	pinctrl_dt_free_maps(p);
+	if (!ACPI_COMPANION(p->dev)) {
+		pinctrl_dt_free_maps(p);
+	} else {
+		pinctrl_acpi_free_maps(p);
+	}
 
 	if (inlist)
 		list_del(&p->node);
diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
index 840103c40c14..28f2f9d518d4 100644
--- a/drivers/pinctrl/core.h
+++ b/drivers/pinctrl/core.h
@@ -72,6 +72,8 @@ struct pinctrl_dev {
  * @state: the current state
  * @dt_maps: the mapping table chunks dynamically parsed from device tree for
  *	this device, if any
+ * @acpi_maps: the mapping table chunks dynamically parsed from acpi for this
+ *  device, if any
  * @users: reference count
  */
 struct pinctrl {
@@ -80,6 +82,7 @@ struct pinctrl {
 	struct list_head states;
 	struct pinctrl_state *state;
 	struct list_head dt_maps;
+	struct list_head acpi_maps;
 	struct kref users;
 };
 
diff --git a/drivers/pinctrl/pinctrl-acpi.c b/drivers/pinctrl/pinctrl-acpi.c
index 75e59fe22387..9777577aefd6 100644
--- a/drivers/pinctrl/pinctrl-acpi.c
+++ b/drivers/pinctrl/pinctrl-acpi.c
@@ -10,6 +10,397 @@
 #include <linux/list.h>
 
 #include "pinctrl-acpi.h"
+#include "core.h"
+
+/**
+ * struct pinctrl_acpi_map - mapping table chunk parsed from device tree
+ * @node: list node for struct pinctrl's @acpi_maps field
+ * @pctldev: the pin controller that allocated this struct, and will free it
+ * @map: the mapping table entries
+ * @num_maps: number of mapping table entries
+ */
+struct pinctrl_acpi_map {
+	struct list_head node;
+	struct pinctrl_dev *pctldev;
+	struct pinctrl_map *map;
+	unsigned int num_maps;
+};
+
+static void acpi_free_map(struct pinctrl_dev *pctldev,
+			 struct pinctrl_map *map, unsigned int num_maps)
+{
+	int i;
+
+	for (i = 0; i < num_maps; ++i) {
+		kfree_const(map[i].dev_name);
+		map[i].dev_name = NULL;
+	}
+
+	if (pctldev) {
+		const struct pinctrl_ops *ops = pctldev->desc->pctlops;
+
+		if (ops->acpi_free_map)
+			ops->acpi_free_map(pctldev, map, num_maps);
+
+	} else {
+		/* There is no pctldev for PIN_MAP_TYPE_DUMMY_STATE */
+		kfree(map);
+	}
+}
+
+void pinctrl_acpi_free_maps(struct pinctrl *p)
+{
+	struct pinctrl_acpi_map *acpi_map, *n1;
+
+	list_for_each_entry_safe(acpi_map, n1, &p->acpi_maps, node) {
+		pinctrl_unregister_mappings(acpi_map->map);
+		list_del(&acpi_map->node);
+		acpi_free_map(acpi_map->pctldev, acpi_map->map,
+				acpi_map->num_maps);
+		kfree(acpi_map);
+	}
+}
+
+static int acpi_remember_or_free_map(struct pinctrl *p, const char *statename,
+				   struct pinctrl_dev *pctldev,
+				   struct pinctrl_map *map, unsigned num_maps)
+{
+	int i;
+	struct pinctrl_acpi_map *acpi_map;
+
+	/* Initialize common mapping table entry fields */
+	for (i = 0; i < num_maps; i++) {
+		const char *devname;
+
+		devname = kstrdup_const(dev_name(p->dev), GFP_KERNEL);
+		if (!devname)
+			goto err_free_map;
+
+		map[i].dev_name = devname;
+		map[i].name = statename;
+		if (pctldev)
+			map[i].ctrl_dev_name = dev_name(pctldev->dev);
+	}
+
+	/* Remember the converted mapping table entries */
+	acpi_map = kzalloc(sizeof(*acpi_map), GFP_KERNEL);
+	if (!acpi_map)
+		goto err_free_map;
+
+	acpi_map->pctldev = pctldev;
+	acpi_map->map = map;
+	acpi_map->num_maps = num_maps;
+	list_add_tail(&acpi_map->node, &p->acpi_maps);
+
+	return pinctrl_register_mappings(map, num_maps);
+
+err_free_map:
+	acpi_free_map(pctldev, map, num_maps);
+	return -ENOMEM;
+}
+
+/* Convert raw acpi device references to device name */
+static const char *acpi_node_to_device_name(char *acpi_node)
+{
+	acpi_status status;
+	acpi_handle handle;
+	struct acpi_device *controller_device;
+
+	status = acpi_get_handle(NULL, acpi_node, &handle);
+
+	if (ACPI_FAILURE(status))
+		return NULL;
+
+	controller_device = acpi_bus_get_acpi_device(handle);
+
+	if (!controller_device)
+		return NULL;
+
+	return acpi_dev_name(controller_device);
+}
+
+/* Map acpi pin configuration types to pinctrl general configuration type */
+static unsigned map_acpi_conf_to_general_conf(unsigned param, unsigned value)
+{
+	switch (param) {
+	case ACPI_PIN_CONFIG_DEFAULT:
+		return pinconf_to_config_packed(ACPI_PIN_CONFIG_DEFAULT, 0);
+	case ACPI_PIN_CONFIG_BIAS_PULL_UP:
+		return pinconf_to_config_packed(PIN_CONFIG_BIAS_PULL_UP_OHMS, value);
+	case ACPI_PIN_CONFIG_BIAS_PULL_DOWN:
+		return pinconf_to_config_packed(PIN_CONFIG_BIAS_PULL_DOWN_OHMS, value);
+	case ACPI_PIN_CONFIG_BIAS_DEFAULT:
+		return pinconf_to_config_packed(PIN_CONFIG_BIAS_PULL_PIN_DEFAULT, 0);
+	case ACPI_PIN_CONFIG_BIAS_DISABLE:
+		return pinconf_to_config_packed(PIN_CONFIG_BIAS_DISABLE, 0);
+	case ACPI_PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
+		return pinconf_to_config_packed(PIN_CONFIG_BIAS_HIGH_IMPEDANCE, 0);
+	case ACPI_PIN_CONFIG_BIAS_BUS_HOLD:
+		return pinconf_to_config_packed(PIN_CONFIG_BIAS_BUS_HOLD, 0);
+	case ACPI_PIN_CONFIG_DRIVE_OPEN_DRAIN:
+		return pinconf_to_config_packed(PIN_CONFIG_DRIVE_OPEN_DRAIN, 0);
+	case ACPI_PIN_CONFIG_DRIVE_OPEN_SOURCE:
+		return pinconf_to_config_packed(PIN_CONFIG_DRIVE_OPEN_SOURCE, 0);
+	case ACPI_PIN_CONFIG_DRIVE_PUSH_PULL:
+		return pinconf_to_config_packed(PIN_CONFIG_DRIVE_PUSH_PULL, 0);
+	case ACPI_PIN_CONFIG_DRIVE_STRENGTH:
+		return pinconf_to_config_packed(PIN_CONFIG_DRIVE_STRENGTH, value);
+	case ACPI_PIN_CONFIG_SLEW_RATE:
+		return pinconf_to_config_packed(PIN_CONFIG_SLEW_RATE, value);
+	case ACPI_PIN_CONFIG_INPUT_DEBOUNCE:
+		return pinconf_to_config_packed(PIN_CONFIG_INPUT_DEBOUNCE, value);
+	case ACPI_PIN_CONFIG_INPUT_SCHMITT_TRIGGER:
+		return pinconf_to_config_packed(PIN_CONFIG_INPUT_SCHMITT_ENABLE, value);
+	default:
+		pr_warn("PINCTRL: ACPI pin configuration type (%d) not handled\n", param);
+		return pinconf_to_config_packed(ACPI_PIN_CONFIG_DEFAULT, 0);
+	}
+}
+
+struct pinctrl_acpi_controller_map {
+	char *pinctrl_dev;
+	struct list_head list;
+	struct list_head pin_group_maps;
+};
+
+/* Add pin/group function and configuration descriptor to internal map */
+static int add_pin_group_node(struct list_head *acpi_map_head,
+							char *pinctrl_dev,
+							char *group,
+							unsigned pin,
+							bool is_config,
+							unsigned config_func,
+							void *vendor_data)
+{
+	struct pinctrl_acpi_controller_map *acpi_controller_map = NULL;
+	struct pinctrl_acpi_controller_map *acpi_controller_map_iter = NULL;
+	struct pinctrl_acpi_pin_group_map *pin_group_map = NULL;
+	struct pinctrl_acpi_pin_group_map *pin_group_map_iter = NULL;
+	struct pinctrl_acpi_pin_group_info *info = NULL;
+	bool group_pin_match;
+
+	/* Find the pin controller specific list to use to add the descriptor */
+	list_for_each_entry(acpi_controller_map_iter, acpi_map_head, list) {
+		if (!strcmp(acpi_controller_map_iter->pinctrl_dev, pinctrl_dev)) {
+			acpi_controller_map = acpi_controller_map_iter;
+			break;
+		}
+	}
+
+	/* If this is the first entry for the pin controller, allocate an entry */
+	if (!acpi_controller_map) {
+		acpi_controller_map = kzalloc(sizeof(struct pinctrl_acpi_controller_map), GFP_KERNEL);
+
+		if (!acpi_controller_map)
+			return -ENOMEM;
+
+		acpi_controller_map->pinctrl_dev = pinctrl_dev;
+		INIT_LIST_HEAD(&acpi_controller_map->list);
+		INIT_LIST_HEAD(&acpi_controller_map->pin_group_maps);
+		list_add(&acpi_controller_map->list, acpi_map_head);
+	}
+
+	/* Find the group/pin specific node from the descriptor list */
+	list_for_each_entry(pin_group_map_iter, &acpi_controller_map->pin_group_maps, list) {
+		if (group)
+			group_pin_match = !strcmp(pin_group_map_iter->group, group);
+		else
+			group_pin_match = (pin == pin_group_map_iter->pin);
+		if (pin_group_map_iter->is_config == is_config && group_pin_match) {
+			pin_group_map = pin_group_map_iter;
+			break;
+		}
+	}
+
+	if (!pin_group_map) {
+		pin_group_map = kzalloc(sizeof(struct pinctrl_acpi_pin_group_map), GFP_KERNEL);
+
+		if (!pin_group_map)
+			return -ENOMEM;
+
+		pin_group_map->group = group;
+		pin_group_map->pin = pin;
+		pin_group_map->is_config = is_config;
+		INIT_LIST_HEAD(&pin_group_map->list);
+		INIT_LIST_HEAD(&pin_group_map->info);
+		list_add(&pin_group_map->list, &acpi_controller_map->pin_group_maps);
+	}
+
+	/* Allocate descriptor and add the pin configuration/function info */
+	info = kzalloc(sizeof(struct pinctrl_acpi_pin_group_info), GFP_KERNEL);
+
+	if (!info)
+		return -ENOMEM;
+
+	info->config_func = config_func;
+	info->vendor_data = vendor_data;
+	INIT_LIST_HEAD(&info->list);
+	list_add(&info->list, &pin_group_map->info);
+
+	return 0;
+}
+
+static int pinctrl_acpi_populate_pin_group_map(struct acpi_resource *ares, void *data)
+{
+	struct acpi_resource_pin_function *ares_pin_function;
+	struct acpi_resource_pin_config *ares_pin_config;
+	struct acpi_resource_pin_group_function *ares_pin_group_function;
+	struct acpi_resource_pin_group_config *ares_pin_group_config;
+	struct list_head *acpi_map_head = data;
+	int i;
+	int ret;
+	unsigned int config;
+	char *pinctrl_dev;
+	char *group;
+	unsigned int pin;
+	void *vendor_data;
+	unsigned int func;
+
+	switch (ares->type) {
+	case ACPI_RESOURCE_TYPE_PIN_FUNCTION:
+		ares_pin_function = &ares->data.pin_function;
+		vendor_data = ares_pin_function->vendor_data;
+		pinctrl_dev = ares_pin_function->resource_source.string_ptr;
+		group = NULL;
+		func = ares_pin_function->function_number;
+		config = map_acpi_conf_to_general_conf(ares_pin_function->pin_config, 0);
+
+		for (i = 0; i < ares_pin_function->pin_table_length; i++) {
+
+			ret = add_pin_group_node(acpi_map_head, pinctrl_dev, group,
+					ares_pin_function->pin_table[i], false, func, vendor_data);
+
+			if (ret < 0)
+				return ret;
+
+			ret = add_pin_group_node(acpi_map_head, pinctrl_dev, group,
+					ares_pin_function->pin_table[i], true, config, vendor_data);
+
+			if (ret < 0)
+				return ret;
+		}
+		break;
+	case ACPI_RESOURCE_TYPE_PIN_CONFIG:
+		ares_pin_config = &ares->data.pin_config;
+		pinctrl_dev = ares_pin_config->resource_source.string_ptr;
+		group = NULL;
+		func = 0;
+
+		config = map_acpi_conf_to_general_conf(
+			ares_pin_config->pin_config_type,
+			ares_pin_config->pin_config_value);
+
+		vendor_data = ares_pin_config->vendor_data;
+
+		for (i = 0; i < ares_pin_function->pin_table_length; i++) {
+			pin = ares_pin_config->pin_table[i];
+
+			ret = add_pin_group_node(acpi_map_head, pinctrl_dev,
+						group, pin, true, config, vendor_data);
+			if (ret < 0)
+				return ret;
+		}
+		break;
+	case ACPI_RESOURCE_TYPE_PIN_GROUP_FUNCTION:
+		ares_pin_group_function = &ares->data.pin_group_function;
+		vendor_data = ares_pin_group_function->vendor_data;
+		pinctrl_dev = ares_pin_group_function->resource_source.string_ptr;
+		group = ares_pin_group_function->resource_source_label.string_ptr;
+		pin = 0;
+		func = ares_pin_group_function->function_number;
+		ret = add_pin_group_node(acpi_map_head, pinctrl_dev,
+					group, pin, false, func, vendor_data);
+		if (ret < 0)
+			return ret;
+
+		break;
+	case ACPI_RESOURCE_TYPE_PIN_GROUP_CONFIG:
+		ares_pin_group_config = &ares->data.pin_group_config;
+		vendor_data = ares_pin_group_config->vendor_data;
+		pinctrl_dev = ares_pin_group_config->resource_source.string_ptr;
+		group = ares_pin_group_config->resource_source_label.string_ptr;
+		pin = 0;
+
+		config = map_acpi_conf_to_general_conf(
+					ares_pin_group_config->pin_config_type,
+					ares_pin_group_config->pin_config_value);
+
+		ret = add_pin_group_node(acpi_map_head, pinctrl_dev, group,
+					pin, true, config, vendor_data);
+		if (ret < 0)
+			return ret;
+
+		break;
+	}
+	return 1;
+}
+
+static int pinctrl_acpi_get_pin_group_map(struct acpi_device *adev, struct list_head *pin_group_root)
+{
+	struct list_head res_list;
+	int ret;
+
+	INIT_LIST_HEAD(&res_list);
+
+	ret = acpi_dev_get_resources(adev, &res_list,
+								 pinctrl_acpi_populate_pin_group_map,
+								 pin_group_root);
+
+	acpi_dev_free_resource_list(&res_list);
+
+	return ret;
+}
+
+/* Decode and register acpi pinctrl related properties to pinctrl system */
+int pinctrl_acpi_to_map(struct pinctrl *p)
+{
+	int num_maps;
+	int ret;
+	struct acpi_device *adev;
+	struct list_head pin_group_list;
+	struct pinctrl_map *new_map;
+	struct pinctrl_dev *pctldev;
+	const struct pinctrl_ops *ops;
+	struct pinctrl_acpi_controller_map *controller_map;
+
+	adev = ACPI_COMPANION(p->dev);
+	if (!adev)
+		return -ENODEV;
+
+	/* list to hold the pin/group descriptors generated, grouped by pin controller, pin/group name*/
+	INIT_LIST_HEAD(&pin_group_list);
+
+	ret = pinctrl_acpi_get_pin_group_map(adev, &pin_group_list);
+	if (ret < 0)
+		return ret;
+
+	/* Iterate over descriptor for each pin controller and invoke the driver function */
+	list_for_each_entry(controller_map, &pin_group_list, list) {
+		const char *pinctrl_dev_name = acpi_node_to_device_name(controller_map->pinctrl_dev);
+
+		pctldev = get_pinctrl_dev_from_devname(pinctrl_dev_name);
+		ops = pctldev->desc->pctlops;
+		if (!ops->acpi_node_to_map) {
+			dev_err(p->dev, "pctldev %s doesn't support ACPI\n",
+				dev_name(pctldev->dev));
+			return -ENODEV;
+		}
+		ret = ops->acpi_node_to_map(pctldev, &controller_map->pin_group_maps, &new_map, &num_maps);
+		if (ret < 0) {
+			return ret;
+		} else if (num_maps == 0) {
+			dev_info(p->dev, "there is not valid maps for pin controller %s\n", pinctrl_dev_name);
+			return 0;
+		}
+
+		ret = acpi_remember_or_free_map(p, "default", pctldev, new_map, num_maps);
+		if (ret < 0) {
+			dev_info(p->dev, "Failed to register maps\n");
+			return ret;
+		}
+	}
+	return 0;
+}
 
 static int pinctrl_acpi_populate_group_desc(struct acpi_resource *ares, void *data)
 {
diff --git a/drivers/pinctrl/pinctrl-acpi.h b/drivers/pinctrl/pinctrl-acpi.h
index 1a0c751a7594..4ed45b22257c 100644
--- a/drivers/pinctrl/pinctrl-acpi.h
+++ b/drivers/pinctrl/pinctrl-acpi.h
@@ -12,11 +12,39 @@ struct pinctrl_acpi_group_desc {
 	struct list_head list;
 };
 
+struct pinctrl_acpi_pin_group_map {
+	const char *group;
+	unsigned int pin;
+	bool is_config;
+    struct list_head info;
+	struct list_head list;
+};
+
+struct pinctrl_acpi_pin_group_info {
+	unsigned config_func;
+	void *vendor_data;
+    struct list_head list;
+};
+
 #ifdef CONFIG_ACPI
 int pinctrl_acpi_get_pin_groups(struct acpi_device *adev, struct list_head *group_desc_list);
+
+int pinctrl_acpi_to_map(struct pinctrl *p);
+
+void pinctrl_acpi_free_maps(struct pinctrl *p);
 #else
 int pinctrl_acpi_get_pin_groups(struct acpi_device *adev, struct list_head *group_desc_list)
 {
 	return -ENODEV;
 }
+
+int pinctrl_acpi_to_map(struct pinctrl *p)
+{
+	return -ENODEV;
+}
+
+void pinctrl_acpi_free_maps(struct pinctrl *p)
+{
+
+}
 #endif
diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
index 70b45d28e7a9..99a087888c0d 100644
--- a/include/linux/pinctrl/pinctrl.h
+++ b/include/linux/pinctrl/pinctrl.h
@@ -84,6 +84,15 @@ struct pinctrl_gpio_range {
  *	allocated members of the mapping table entries themselves. This
  *	function is optional, and may be omitted for pinctrl drivers that do
  *	not support device tree.
+ * @acpi_node_to_map: process acpi pin related properties, and create
+ *	mapping table entries for it. These are returned through the @map and
+ *	@num_maps output parameters. This function is optional, and may be
+ *	omitted for pinctrl drivers that do not support acpi.
+ * @acpi_free_map: free mapping table entries created via @dt_node_to_map. The
+ *	top-level @map pointer must be freed, along with any dynamically
+ *	allocated members of the mapping table entries themselves. This
+ *	function is optional, and may be omitted for pinctrl drivers that do
+ *	not support acpi.
  */
 struct pinctrl_ops {
 	int (*get_groups_count) (struct pinctrl_dev *pctldev);
@@ -100,6 +109,12 @@ struct pinctrl_ops {
 			       struct pinctrl_map **map, unsigned *num_maps);
 	void (*dt_free_map) (struct pinctrl_dev *pctldev,
 			     struct pinctrl_map *map, unsigned num_maps);
+	int (*acpi_node_to_map) (struct pinctrl_dev *pctldev,
+			       struct list_head *info_list,
+			       struct pinctrl_map **map, unsigned *num_maps);
+	void (*acpi_free_map) (struct pinctrl_dev *pctldev,
+				   struct pinctrl_map *map, unsigned num_maps);
+
 };
 
 /**
-- 
2.25.1

Re: [PATCH RFC 0/3] pinctrl: add ACPI support to pin controller

[Andy Shevchenko]

On Thu, Nov 10, 2022 at 07:12:55PM +0000, Niyas Sait wrote:
> This is a proposal for adding ACPI support to pin controller.
> 
> The patch supports following resources introduced in ACPI from v6.2
> 
> - PinFunction
> - PinConfig
> - PinGroupFunction
> - PinGroupConfig
> - PinGroup
> 
> The patch has been tested on NXP I.MX8MP Plus platform with ACPI.

Thank you very much for this work!

I will take time to review it, presumably next week.

I'm not sure it will go to the v6/2-rc1 due to tough time range
(we are almost at -rc5), I hope you are not in such hurry.

> Niyas Sait (3):
>   pinctrl: add support for acpi PinGroup resource
>   pinconf-generic: add pull up and pull down config with resistance
>   pinctrl: add support for acpi pin function and config resources
> 
>  drivers/pinctrl/Makefile                |   1 +
>  drivers/pinctrl/core.c                  |  19 +-
>  drivers/pinctrl/core.h                  |   3 +
>  drivers/pinctrl/pinctrl-acpi.c          | 450 ++++++++++++++++++++++++
>  drivers/pinctrl/pinctrl-acpi.h          |  50 +++
>  include/linux/pinctrl/pinconf-generic.h |   6 +
>  include/linux/pinctrl/pinctrl.h         |  15 +
>  7 files changed, 540 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/pinctrl/pinctrl-acpi.c
>  create mode 100644 drivers/pinctrl/pinctrl-acpi.h
> 
> -- 
> 2.25.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFC 2/3] pinconf-generic: add pull up and pull down config with resistance

[Andy Shevchenko]

On Thu, Nov 10, 2022 at 07:12:57PM +0000, Niyas Sait wrote:
> pin configuration types have been extended to include a pull up
> and pull down config with resistance in ohms.

Why do we need this?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFC 0/3] pinctrl: add ACPI support to pin controller

[Andy Shevchenko]

On Thu, Nov 10, 2022 at 10:14:45PM +0200, Andy Shevchenko wrote:
> On Thu, Nov 10, 2022 at 07:12:55PM +0000, Niyas Sait wrote:
> > This is a proposal for adding ACPI support to pin controller.
> > 
> > The patch supports following resources introduced in ACPI from v6.2
> > 
> > - PinFunction
> > - PinConfig
> > - PinGroupFunction
> > - PinGroupConfig
> > - PinGroup
> > 
> > The patch has been tested on NXP I.MX8MP Plus platform with ACPI.
> 
> Thank you very much for this work!
> 
> I will take time to review it, presumably next week.
> 
> I'm not sure it will go to the v6/2-rc1 due to tough time range
> (we are almost at -rc5), I hope you are not in such hurry.

Meanwhile, can we see the DSDT excerpt(s) that shows how you use these
resources there?

Also note, we need some clarification to be done in the ACPI specification
regarding the numbering schema, used in those resources. TL;DR: for Gpio*()
the number space is GPIO, for Pin*() is pin control (real pin numbering,
since not all of them can be GPIOs and GPIO can have its own additional
layer of numbering mapping).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFC 0/3] pinctrl: add ACPI support to pin controller

[Niyas Sait]

> Meanwhile, can we see the DSDT excerpt(s) that shows how you use these
> resources there?
I've used the following prototype for I.MX8MP

https://gitlab.com/nsait-linaro/acpi-patches/-/blob/master/0001-add-acpi-pinctrl-support-for-i2c-controllers.patch

> Also note, we need some clarification to be done in the ACPI specification
> regarding the numbering schema, used in those resources. TL;DR: for Gpio*()
> the number space is GPIO, for Pin*() is pin control (real pin numbering,
> since not all of them can be GPIOs and GPIO can have its own additional
> layer of numbering mapping).

Yes that would be helpful!

I've done some prototyping and have used the _DSD to pass the GPIO to 
Pin mapping to the GPIO controller. See following patches

https://gitlab.com/nsait-linaro/acpi-patches/-/blob/master/0001-add-GPIO-controllers-to-ACPI-table.patch#L89

https://gitlab.com/nsait-linaro/acpi-patches/-/blob/master/0001-add-acpi-gpio-controller-support-to-IMX-platform.patch

Re: [PATCH RFC 0/3] pinctrl: add ACPI support to pin controller

[Niyas Sait]

> I'm not sure it will go to the v6/2-rc1 due to tough time range
> (we are almost at -rc5), I hope you are not in such hurry.

Thanks. That's fine.

On 10/11/2022 20:14, Andy Shevchenko wrote:
> On Thu, Nov 10, 2022 at 07:12:55PM +0000, Niyas Sait wrote:
>> This is a proposal for adding ACPI support to pin controller.
>>
>> The patch supports following resources introduced in ACPI from v6.2
>>
>> - PinFunction
>> - PinConfig
>> - PinGroupFunction
>> - PinGroupConfig
>> - PinGroup
>>
>> The patch has been tested on NXP I.MX8MP Plus platform with ACPI.
> 
> Thank you very much for this work!
> 
> I will take time to review it, presumably next week.
> 
> I'm not sure it will go to the v6/2-rc1 due to tough time range
> (we are almost at -rc5), I hope you are not in such hurry.
> 
>> Niyas Sait (3):
>>    pinctrl: add support for acpi PinGroup resource
>>    pinconf-generic: add pull up and pull down config with resistance
>>    pinctrl: add support for acpi pin function and config resources
>>
>>   drivers/pinctrl/Makefile                |   1 +
>>   drivers/pinctrl/core.c                  |  19 +-
>>   drivers/pinctrl/core.h                  |   3 +
>>   drivers/pinctrl/pinctrl-acpi.c          | 450 ++++++++++++++++++++++++
>>   drivers/pinctrl/pinctrl-acpi.h          |  50 +++
>>   include/linux/pinctrl/pinconf-generic.h |   6 +
>>   include/linux/pinctrl/pinctrl.h         |  15 +
>>   7 files changed, 540 insertions(+), 4 deletions(-)
>>   create mode 100644 drivers/pinctrl/pinctrl-acpi.c
>>   create mode 100644 drivers/pinctrl/pinctrl-acpi.h
>>
>> -- 
>> 2.25.1
>>
> 

Re: [PATCH RFC 2/3] pinconf-generic: add pull up and pull down config with resistance

[Niyas Sait]

ACPI Pull up and Pull down config also passes resistance values and 
looking at the existing PIN_CONFIG_BIAS_PULL_DOWN and 
PIN_CONFIG_BIAS_PULL_UP types I wasn't sure if it was designed to pass 
the extra resistance values.


On 10/11/2022 20:17, Andy Shevchenko wrote:
> On Thu, Nov 10, 2022 at 07:12:57PM +0000, Niyas Sait wrote:
>> pin configuration types have been extended to include a pull up
>> and pull down config with resistance in ohms.
> Why do we need this?

Re: [PATCH RFC 2/3] pinconf-generic: add pull up and pull down config with resistance

[Linus Walleij]

Hi Niyas,

thanks for your patch!

On Thu, Nov 10, 2022 at 8:13 PM Niyas Sait <niyas.sait@linaro.org> wrote:

> pin configuration types have been extended to include a pull up
> and pull down config with resistance in ohms.
>
> Signed-off-by: Niyas Sait <niyas.sait@linaro.org>
(...)
>   * @PIN_CONFIG_BIAS_PULL_DOWN: the pin will be pulled down (usually with high
>   *     impedance to GROUND). If the argument is != 0 pull-down is enabled,
>   *     if it is 0, pull-down is total, i.e. the pin is connected to GROUND.
> + * @PIN_CONFIG_BIAS_PULL_DOWN_OHMS: the pin will be pulled down with value
> + *  passed as argument. The argument is in ohms.

Hmmmmm hmmmm.

When I designed this, I definitely intended for the argument to
PIN_CONFIG_BIAS_PULL_DOWN to be used for this, so I would just:

- Use PIN_CONFIG_BIAS_PULL_DOWN
- Augment the comment to say:
  "If the argument is != 0 pull-down is enabled, the value is interpreted by
  the driver and can be custom or an SI unit such as ohms".

Yours,
Linus Walleij

Re: [PATCH RFC 2/3] pinconf-generic: add pull up and pull down config with resistance

[Niyas Sait]

That's good to know. I wasn't sure and didn't want to break any existing 
interface. I will drop the new types and add the comment to clarify the use.

On 11/11/2022 09:17, Linus Walleij wrote:
> When I designed this, I definitely intended for the argument to
> PIN_CONFIG_BIAS_PULL_DOWN to be used for this, so I would just:
> 
> - Use PIN_CONFIG_BIAS_PULL_DOWN
> - Augment the comment to say:
>    "If the argument is != 0 pull-down is enabled, the value is interpreted by
>    the driver and can be custom or an SI unit such as ohms".

Re: [PATCH RFC 1/3] pinctrl: add support for acpi PinGroup resource

[Mika Westerberg]

On Thu, Nov 10, 2022 at 07:12:56PM +0000, Niyas Sait wrote:
> pinctrl-acpi parses and decode PinGroup resources for
> the device and generate list of group descriptor.
> Descriptors can be used by the pin controller to identify
> the groups and pins provided in the group.
> 
> Signed-off-by: Niyas Sait <niyas.sait@linaro.org>
> ---
>  drivers/pinctrl/Makefile       |  1 +
>  drivers/pinctrl/pinctrl-acpi.c | 59 ++++++++++++++++++++++++++++++++++
>  drivers/pinctrl/pinctrl-acpi.h | 22 +++++++++++++
>  3 files changed, 82 insertions(+)
>  create mode 100644 drivers/pinctrl/pinctrl-acpi.c
>  create mode 100644 drivers/pinctrl/pinctrl-acpi.h
> 
> diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
> index e76f5cdc64b0..0b0ec4080942 100644
> --- a/drivers/pinctrl/Makefile
> +++ b/drivers/pinctrl/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_PINMUX)		+= pinmux.o
>  obj-$(CONFIG_PINCONF)		+= pinconf.o
>  obj-$(CONFIG_GENERIC_PINCONF)	+= pinconf-generic.o
>  obj-$(CONFIG_OF)		+= devicetree.o
> +obj-$(CONFIG_ACPI)		+= pinctrl-acpi.o
>  
>  obj-$(CONFIG_PINCTRL_AMD)	+= pinctrl-amd.o
>  obj-$(CONFIG_PINCTRL_APPLE_GPIO) += pinctrl-apple-gpio.o
> diff --git a/drivers/pinctrl/pinctrl-acpi.c b/drivers/pinctrl/pinctrl-acpi.c
> new file mode 100644
> index 000000000000..75e59fe22387
> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-acpi.c
> @@ -0,0 +1,59 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2022 Linaro Ltd.
> + */
> +#include <linux/acpi.h>
> +#include <linux/errno.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/gpio/machine.h>
> +#include <linux/list.h>
> +
> +#include "pinctrl-acpi.h"
> +
> +static int pinctrl_acpi_populate_group_desc(struct acpi_resource *ares, void *data)
> +{
> +	struct acpi_resource_pin_group *ares_pin_group;
> +	struct pinctrl_acpi_group_desc *desc;
> +	struct list_head *group_desc_list = data;
> +
> +	if (ares->type != ACPI_RESOURCE_TYPE_PIN_GROUP)
> +		return 1;
> +
> +	ares_pin_group = &ares->data.pin_group;
> +
> +	desc = kzalloc(sizeof(struct pinctrl_acpi_group_desc), GFP_KERNEL);
> +	if (!desc)
> +		return -ENOMEM;
> +
> +	desc->name = kstrdup_const(ares_pin_group->resource_label.string_ptr, GFP_KERNEL);
> +	desc->pins = ares_pin_group->pin_table;
> +	desc->num_pins = ares_pin_group->pin_table_length;
> +	desc->vendor_data = ares_pin_group->vendor_data;
> +	desc->vendor_length = ares_pin_group->vendor_length;
> +
> +	INIT_LIST_HEAD(&desc->list);
> +	list_add(&desc->list, group_desc_list);
> +
> +	return 1;
> +}
> +
> +/* Get list of acpi pin groups definitions for the controller */

Use proper kernel-doc here.

Also who is responsible of releasing the thing and how it is done?

> +int pinctrl_acpi_get_pin_groups(struct acpi_device *adev, struct list_head *group_desc_list)
> +{
> +	struct list_head res_list;
> +	int ret;
> +
> +	INIT_LIST_HEAD(&res_list);
> +	INIT_LIST_HEAD(group_desc_list);
> +
> +	ret = acpi_dev_get_resources(adev, &res_list,
> +								 pinctrl_acpi_populate_group_desc,
> +								 group_desc_list);

The formatting is wrong here.

> +	if (ret < 0)
> +		return ret;
> +
> +	acpi_dev_free_resource_list(&res_list);
> +

Drop the empty line.

> +	return 0;
> +}
> diff --git a/drivers/pinctrl/pinctrl-acpi.h b/drivers/pinctrl/pinctrl-acpi.h
> new file mode 100644
> index 000000000000..1a0c751a7594
> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-acpi.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2022 Linaro Ltd.
> + */
> +

kernel-doc here too.

> +struct pinctrl_acpi_group_desc {
> +	const char *name;
> +	short unsigned int *pins;
> +	unsigned num_pins;
> +	void *vendor_data;
> +	unsigned vendor_length;
> +	struct list_head list;
> +};
> +
> +#ifdef CONFIG_ACPI
> +int pinctrl_acpi_get_pin_groups(struct acpi_device *adev, struct list_head *group_desc_list);
> +#else
> +int pinctrl_acpi_get_pin_groups(struct acpi_device *adev, struct list_head *group_desc_list)

This needs to be static inline.

> +{
> +	return -ENODEV;

-ENXIO?

> +}
> +#endif
> -- 
> 2.25.1

Re: [PATCH RFC 3/3] pinctrl: add support for acpi pin function and config resources

[Mika Westerberg]

On Thu, Nov 10, 2022 at 07:12:58PM +0000, Niyas Sait wrote:
> Add support for following acpi pin resources
> 
> - PinFunction
> - PinConfig
> - PinGroupFunction
> - PinGroupConfig
> 
> Pinctrl-acpi parses the acpi table and generates list of pin

ACPI

> descriptors that can be used by pin controller to set and config pin.
> 
> Descriptors are grouped by pin number or group name and contains list
> of functions or configs to apply.
> 
> Pin config types from acpi are converted to generic pin config types

ACPI (ditto everywhere).

> and passed through the descriptor.
> 
> Signed-off-by: Niyas Sait <niyas.sait@linaro.org>
> ---
>  drivers/pinctrl/core.c          |  19 +-
>  drivers/pinctrl/core.h          |   3 +
>  drivers/pinctrl/pinctrl-acpi.c  | 391 ++++++++++++++++++++++++++++++++
>  drivers/pinctrl/pinctrl-acpi.h  |  28 +++
>  include/linux/pinctrl/pinctrl.h |  15 ++
>  5 files changed, 452 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> index ffe39336fcac..03770ac66d48 100644
> --- a/drivers/pinctrl/core.c
> +++ b/drivers/pinctrl/core.c
> @@ -25,6 +25,7 @@
>  #include <linux/pinctrl/consumer.h>
>  #include <linux/pinctrl/pinctrl.h>
>  #include <linux/pinctrl/machine.h>
> +#include <linux/acpi.h>
>  
>  #ifdef CONFIG_GPIOLIB
>  #include "../gpio/gpiolib.h"
> @@ -35,7 +36,7 @@
>  #include "devicetree.h"
>  #include "pinmux.h"
>  #include "pinconf.h"
> -
> +#include "pinctrl-acpi.h"
>  
>  static bool pinctrl_dummy_state;
>  
> @@ -1042,9 +1043,15 @@ static struct pinctrl *create_pinctrl(struct device *dev,
>  		return ERR_PTR(-ENOMEM);
>  	p->dev = dev;
>  	INIT_LIST_HEAD(&p->states);
> -	INIT_LIST_HEAD(&p->dt_maps);
>  
> -	ret = pinctrl_dt_to_map(p, pctldev);
> +	if (!ACPI_COMPANION(dev)) {
> +		INIT_LIST_HEAD(&p->dt_maps);
> +		ret = pinctrl_dt_to_map(p, pctldev);
> +	} else {
> +		INIT_LIST_HEAD(&p->acpi_maps);
> +		ret = pinctrl_acpi_to_map(p);
> +	}
> +
>  	if (ret < 0) {
>  		kfree(p);
>  		return ERR_PTR(ret);
> @@ -1168,7 +1175,11 @@ static void pinctrl_free(struct pinctrl *p, bool inlist)
>  		kfree(state);
>  	}
>  
> -	pinctrl_dt_free_maps(p);
> +	if (!ACPI_COMPANION(p->dev)) {
> +		pinctrl_dt_free_maps(p);
> +	} else {
> +		pinctrl_acpi_free_maps(p);
> +	}

You don't need the {}

>  
>  	if (inlist)
>  		list_del(&p->node);
> diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
> index 840103c40c14..28f2f9d518d4 100644
> --- a/drivers/pinctrl/core.h
> +++ b/drivers/pinctrl/core.h
> @@ -72,6 +72,8 @@ struct pinctrl_dev {
>   * @state: the current state
>   * @dt_maps: the mapping table chunks dynamically parsed from device tree for
>   *	this device, if any
> + * @acpi_maps: the mapping table chunks dynamically parsed from acpi for this
> + *  device, if any
>   * @users: reference count
>   */
>  struct pinctrl {
> @@ -80,6 +82,7 @@ struct pinctrl {
>  	struct list_head states;
>  	struct pinctrl_state *state;
>  	struct list_head dt_maps;
> +	struct list_head acpi_maps;
>  	struct kref users;
>  };
>  
> diff --git a/drivers/pinctrl/pinctrl-acpi.c b/drivers/pinctrl/pinctrl-acpi.c
> index 75e59fe22387..9777577aefd6 100644
> --- a/drivers/pinctrl/pinctrl-acpi.c
> +++ b/drivers/pinctrl/pinctrl-acpi.c
> @@ -10,6 +10,397 @@
>  #include <linux/list.h>
>  
>  #include "pinctrl-acpi.h"
> +#include "core.h"
> +
> +/**
> + * struct pinctrl_acpi_map - mapping table chunk parsed from device tree

Parsed from ACPI namespace?

> + * @node: list node for struct pinctrl's @acpi_maps field
> + * @pctldev: the pin controller that allocated this struct, and will free it
> + * @map: the mapping table entries
> + * @num_maps: number of mapping table entries
> + */
> +struct pinctrl_acpi_map {
> +	struct list_head node;
> +	struct pinctrl_dev *pctldev;
> +	struct pinctrl_map *map;
> +	unsigned int num_maps;

size_t?

> +};
> +
> +static void acpi_free_map(struct pinctrl_dev *pctldev,
> +			 struct pinctrl_map *map, unsigned int num_maps)
> +{
> +	int i;
> +
> +	for (i = 0; i < num_maps; ++i) {
> +		kfree_const(map[i].dev_name);
> +		map[i].dev_name = NULL;
> +	}
> +
> +	if (pctldev) {
> +		const struct pinctrl_ops *ops = pctldev->desc->pctlops;
> +
> +		if (ops->acpi_free_map)
> +			ops->acpi_free_map(pctldev, map, num_maps);
> +
> +	} else {
> +		/* There is no pctldev for PIN_MAP_TYPE_DUMMY_STATE */
> +		kfree(map);
> +	}
> +}
> +

kernel-doc?

> +void pinctrl_acpi_free_maps(struct pinctrl *p)
> +{
> +	struct pinctrl_acpi_map *acpi_map, *n1;

Why not 'tmp' instead of 'n1'?

> +
> +	list_for_each_entry_safe(acpi_map, n1, &p->acpi_maps, node) {
> +		pinctrl_unregister_mappings(acpi_map->map);
> +		list_del(&acpi_map->node);
> +		acpi_free_map(acpi_map->pctldev, acpi_map->map,
> +				acpi_map->num_maps);
> +		kfree(acpi_map);
> +	}
> +}
> +
> +static int acpi_remember_or_free_map(struct pinctrl *p, const char *statename,
> +				   struct pinctrl_dev *pctldev,
> +				   struct pinctrl_map *map, unsigned num_maps)
> +{
> +	int i;
> +	struct pinctrl_acpi_map *acpi_map;
> +
> +	/* Initialize common mapping table entry fields */
> +	for (i = 0; i < num_maps; i++) {
> +		const char *devname;
> +
> +		devname = kstrdup_const(dev_name(p->dev), GFP_KERNEL);
> +		if (!devname)
> +			goto err_free_map;
> +
> +		map[i].dev_name = devname;
> +		map[i].name = statename;
> +		if (pctldev)
> +			map[i].ctrl_dev_name = dev_name(pctldev->dev);
> +	}
> +
> +	/* Remember the converted mapping table entries */
> +	acpi_map = kzalloc(sizeof(*acpi_map), GFP_KERNEL);
> +	if (!acpi_map)
> +		goto err_free_map;
> +
> +	acpi_map->pctldev = pctldev;
> +	acpi_map->map = map;
> +	acpi_map->num_maps = num_maps;
> +	list_add_tail(&acpi_map->node, &p->acpi_maps);
> +
> +	return pinctrl_register_mappings(map, num_maps);
> +
> +err_free_map:
> +	acpi_free_map(pctldev, map, num_maps);
> +	return -ENOMEM;
> +}
> +
> +/* Convert raw acpi device references to device name */

raw ACPI device or raw acpi_device.

> +static const char *acpi_node_to_device_name(char *acpi_node)

Can this be const?

> +{
> +	acpi_status status;
> +	acpi_handle handle;
> +	struct acpi_device *controller_device;
> +
> +	status = acpi_get_handle(NULL, acpi_node, &handle);
> +

Drop the empty line.

> +	if (ACPI_FAILURE(status))
> +		return NULL;
> +
> +	controller_device = acpi_bus_get_acpi_device(handle);

Ditto.

> +
> +	if (!controller_device)
> +		return NULL;
> +
> +	return acpi_dev_name(controller_device);
> +}
> +
> +/* Map acpi pin configuration types to pinctrl general configuration type */
> +static unsigned map_acpi_conf_to_general_conf(unsigned param, unsigned value)
> +{
> +	switch (param) {
> +	case ACPI_PIN_CONFIG_DEFAULT:
> +		return pinconf_to_config_packed(ACPI_PIN_CONFIG_DEFAULT, 0);
> +	case ACPI_PIN_CONFIG_BIAS_PULL_UP:
> +		return pinconf_to_config_packed(PIN_CONFIG_BIAS_PULL_UP_OHMS, value);
> +	case ACPI_PIN_CONFIG_BIAS_PULL_DOWN:
> +		return pinconf_to_config_packed(PIN_CONFIG_BIAS_PULL_DOWN_OHMS, value);
> +	case ACPI_PIN_CONFIG_BIAS_DEFAULT:
> +		return pinconf_to_config_packed(PIN_CONFIG_BIAS_PULL_PIN_DEFAULT, 0);
> +	case ACPI_PIN_CONFIG_BIAS_DISABLE:
> +		return pinconf_to_config_packed(PIN_CONFIG_BIAS_DISABLE, 0);
> +	case ACPI_PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
> +		return pinconf_to_config_packed(PIN_CONFIG_BIAS_HIGH_IMPEDANCE, 0);
> +	case ACPI_PIN_CONFIG_BIAS_BUS_HOLD:
> +		return pinconf_to_config_packed(PIN_CONFIG_BIAS_BUS_HOLD, 0);
> +	case ACPI_PIN_CONFIG_DRIVE_OPEN_DRAIN:
> +		return pinconf_to_config_packed(PIN_CONFIG_DRIVE_OPEN_DRAIN, 0);
> +	case ACPI_PIN_CONFIG_DRIVE_OPEN_SOURCE:
> +		return pinconf_to_config_packed(PIN_CONFIG_DRIVE_OPEN_SOURCE, 0);
> +	case ACPI_PIN_CONFIG_DRIVE_PUSH_PULL:
> +		return pinconf_to_config_packed(PIN_CONFIG_DRIVE_PUSH_PULL, 0);
> +	case ACPI_PIN_CONFIG_DRIVE_STRENGTH:
> +		return pinconf_to_config_packed(PIN_CONFIG_DRIVE_STRENGTH, value);
> +	case ACPI_PIN_CONFIG_SLEW_RATE:
> +		return pinconf_to_config_packed(PIN_CONFIG_SLEW_RATE, value);
> +	case ACPI_PIN_CONFIG_INPUT_DEBOUNCE:
> +		return pinconf_to_config_packed(PIN_CONFIG_INPUT_DEBOUNCE, value);
> +	case ACPI_PIN_CONFIG_INPUT_SCHMITT_TRIGGER:
> +		return pinconf_to_config_packed(PIN_CONFIG_INPUT_SCHMITT_ENABLE, value);
> +	default:
> +		pr_warn("PINCTRL: ACPI pin configuration type (%d) not handled\n", param);

Don't you have any dev * pointer that you can use here instead of
pr_warn()?

> +		return pinconf_to_config_packed(ACPI_PIN_CONFIG_DEFAULT, 0);
> +	}
> +}
> +
> +struct pinctrl_acpi_controller_map {
> +	char *pinctrl_dev;

const char *?

> +	struct list_head list;
> +	struct list_head pin_group_maps;
> +};
> +
> +/* Add pin/group function and configuration descriptor to internal map */
> +static int add_pin_group_node(struct list_head *acpi_map_head,
> +							char *pinctrl_dev,
> +							char *group,
> +							unsigned pin,
> +							bool is_config,
> +							unsigned config_func,
> +							void *vendor_data)
> +{
> +	struct pinctrl_acpi_controller_map *acpi_controller_map = NULL;
> +	struct pinctrl_acpi_controller_map *acpi_controller_map_iter = NULL;
> +	struct pinctrl_acpi_pin_group_map *pin_group_map = NULL;
> +	struct pinctrl_acpi_pin_group_map *pin_group_map_iter = NULL;
> +	struct pinctrl_acpi_pin_group_info *info = NULL;


Do you need to initialize them all?

> +	bool group_pin_match;
> +
> +	/* Find the pin controller specific list to use to add the descriptor */
> +	list_for_each_entry(acpi_controller_map_iter, acpi_map_head, list) {
> +		if (!strcmp(acpi_controller_map_iter->pinctrl_dev, pinctrl_dev)) {
> +			acpi_controller_map = acpi_controller_map_iter;
> +			break;
> +		}
> +	}
> +
> +	/* If this is the first entry for the pin controller, allocate an entry */
> +	if (!acpi_controller_map) {
> +		acpi_controller_map = kzalloc(sizeof(struct pinctrl_acpi_controller_map), GFP_KERNEL);
> +

Drop the empty line.

> +		if (!acpi_controller_map)
> +			return -ENOMEM;
> +
> +		acpi_controller_map->pinctrl_dev = pinctrl_dev;
> +		INIT_LIST_HEAD(&acpi_controller_map->list);
> +		INIT_LIST_HEAD(&acpi_controller_map->pin_group_maps);
> +		list_add(&acpi_controller_map->list, acpi_map_head);
> +	}
> +
> +	/* Find the group/pin specific node from the descriptor list */
> +	list_for_each_entry(pin_group_map_iter, &acpi_controller_map->pin_group_maps, list) {
> +		if (group)
> +			group_pin_match = !strcmp(pin_group_map_iter->group, group);
> +		else
> +			group_pin_match = (pin == pin_group_map_iter->pin);

Empty line

> +		if (pin_group_map_iter->is_config == is_config && group_pin_match) {
> +			pin_group_map = pin_group_map_iter;
> +			break;
> +		}
> +	}
> +
> +	if (!pin_group_map) {
> +		pin_group_map = kzalloc(sizeof(struct pinctrl_acpi_pin_group_map), GFP_KERNEL);
> +

Drop the empty line ;-)

> +		if (!pin_group_map)
> +			return -ENOMEM;
> +
> +		pin_group_map->group = group;
> +		pin_group_map->pin = pin;
> +		pin_group_map->is_config = is_config;
> +		INIT_LIST_HEAD(&pin_group_map->list);
> +		INIT_LIST_HEAD(&pin_group_map->info);
> +		list_add(&pin_group_map->list, &acpi_controller_map->pin_group_maps);
> +	}
> +
> +	/* Allocate descriptor and add the pin configuration/function info */
> +	info = kzalloc(sizeof(struct pinctrl_acpi_pin_group_info), GFP_KERNEL);

Drop the empty line.

> +
> +	if (!info)
> +		return -ENOMEM;
> +
> +	info->config_func = config_func;
> +	info->vendor_data = vendor_data;
> +	INIT_LIST_HEAD(&info->list);
> +	list_add(&info->list, &pin_group_map->info);
> +
> +	return 0;
> +}
> +
> +static int pinctrl_acpi_populate_pin_group_map(struct acpi_resource *ares, void *data)
> +{
> +	struct acpi_resource_pin_function *ares_pin_function;
> +	struct acpi_resource_pin_config *ares_pin_config;
> +	struct acpi_resource_pin_group_function *ares_pin_group_function;
> +	struct acpi_resource_pin_group_config *ares_pin_group_config;
> +	struct list_head *acpi_map_head = data;
> +	int i;
> +	int ret;
> +	unsigned int config;
> +	char *pinctrl_dev;
> +	char *group;
> +	unsigned int pin;
> +	void *vendor_data;
> +	unsigned int func;

That's huge amount of variables. I wonder if this can be reduced with
helper functions etc?

> +
> +	switch (ares->type) {
> +	case ACPI_RESOURCE_TYPE_PIN_FUNCTION:
> +		ares_pin_function = &ares->data.pin_function;
> +		vendor_data = ares_pin_function->vendor_data;
> +		pinctrl_dev = ares_pin_function->resource_source.string_ptr;
> +		group = NULL;
> +		func = ares_pin_function->function_number;
> +		config = map_acpi_conf_to_general_conf(ares_pin_function->pin_config, 0);
> +
> +		for (i = 0; i < ares_pin_function->pin_table_length; i++) {
> +

Drop the empty line

> +			ret = add_pin_group_node(acpi_map_head, pinctrl_dev, group,
> +					ares_pin_function->pin_table[i], false, func, vendor_data);
> +
> +			if (ret < 0)
> +				return ret;
> +
> +			ret = add_pin_group_node(acpi_map_head, pinctrl_dev, group,
> +					ares_pin_function->pin_table[i], true, config, vendor_data);
> +
> +			if (ret < 0)
> +				return ret;
> +		}
> +		break;

Add empty line

(Ditto everywhere below)

> +	case ACPI_RESOURCE_TYPE_PIN_CONFIG:
> +		ares_pin_config = &ares->data.pin_config;
> +		pinctrl_dev = ares_pin_config->resource_source.string_ptr;
> +		group = NULL;
> +		func = 0;
> +
> +		config = map_acpi_conf_to_general_conf(
> +			ares_pin_config->pin_config_type,
> +			ares_pin_config->pin_config_value);
> +
> +		vendor_data = ares_pin_config->vendor_data;
> +
> +		for (i = 0; i < ares_pin_function->pin_table_length; i++) {
> +			pin = ares_pin_config->pin_table[i];
> +
> +			ret = add_pin_group_node(acpi_map_head, pinctrl_dev,
> +						group, pin, true, config, vendor_data);
> +			if (ret < 0)
> +				return ret;
> +		}
> +		break;
> +	case ACPI_RESOURCE_TYPE_PIN_GROUP_FUNCTION:
> +		ares_pin_group_function = &ares->data.pin_group_function;
> +		vendor_data = ares_pin_group_function->vendor_data;
> +		pinctrl_dev = ares_pin_group_function->resource_source.string_ptr;
> +		group = ares_pin_group_function->resource_source_label.string_ptr;
> +		pin = 0;
> +		func = ares_pin_group_function->function_number;
> +		ret = add_pin_group_node(acpi_map_head, pinctrl_dev,
> +					group, pin, false, func, vendor_data);
> +		if (ret < 0)
> +			return ret;
> +
> +		break;
> +	case ACPI_RESOURCE_TYPE_PIN_GROUP_CONFIG:
> +		ares_pin_group_config = &ares->data.pin_group_config;
> +		vendor_data = ares_pin_group_config->vendor_data;
> +		pinctrl_dev = ares_pin_group_config->resource_source.string_ptr;
> +		group = ares_pin_group_config->resource_source_label.string_ptr;
> +		pin = 0;
> +
> +		config = map_acpi_conf_to_general_conf(
> +					ares_pin_group_config->pin_config_type,
> +					ares_pin_group_config->pin_config_value);
> +
> +		ret = add_pin_group_node(acpi_map_head, pinctrl_dev, group,
> +					pin, true, config, vendor_data);
> +		if (ret < 0)
> +			return ret;
> +
> +		break;
> +	}
> +	return 1;
> +}
> +
> +static int pinctrl_acpi_get_pin_group_map(struct acpi_device *adev, struct list_head *pin_group_root)
> +{
> +	struct list_head res_list;
> +	int ret;
> +
> +	INIT_LIST_HEAD(&res_list);
> +
> +	ret = acpi_dev_get_resources(adev, &res_list,
> +								 pinctrl_acpi_populate_pin_group_map,
> +								 pin_group_root);
> +

Weird formatting.

> +	acpi_dev_free_resource_list(&res_list);
> +
> +	return ret;
> +}
> +
> +/* Decode and register acpi pinctrl related properties to pinctrl system */

Kernel-doc

> +int pinctrl_acpi_to_map(struct pinctrl *p)
> +{
> +	int num_maps;
> +	int ret;
> +	struct acpi_device *adev;
> +	struct list_head pin_group_list;
> +	struct pinctrl_map *new_map;
> +	struct pinctrl_dev *pctldev;
> +	const struct pinctrl_ops *ops;
> +	struct pinctrl_acpi_controller_map *controller_map;
> +
> +	adev = ACPI_COMPANION(p->dev);
> +	if (!adev)
> +		return -ENODEV;
> +
> +	/* list to hold the pin/group descriptors generated, grouped by pin controller, pin/group name*/
> +	INIT_LIST_HEAD(&pin_group_list);
> +
> +	ret = pinctrl_acpi_get_pin_group_map(adev, &pin_group_list);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Iterate over descriptor for each pin controller and invoke the driver function */
> +	list_for_each_entry(controller_map, &pin_group_list, list) {
> +		const char *pinctrl_dev_name = acpi_node_to_device_name(controller_map->pinctrl_dev);
> +
> +		pctldev = get_pinctrl_dev_from_devname(pinctrl_dev_name);
> +		ops = pctldev->desc->pctlops;
> +		if (!ops->acpi_node_to_map) {
> +			dev_err(p->dev, "pctldev %s doesn't support ACPI\n",
> +				dev_name(pctldev->dev));
> +			return -ENODEV;
> +		}
> +		ret = ops->acpi_node_to_map(pctldev, &controller_map->pin_group_maps, &new_map, &num_maps);
> +		if (ret < 0) {
> +			return ret;
> +		} else if (num_maps == 0) {
> +			dev_info(p->dev, "there is not valid maps for pin controller %s\n", pinctrl_dev_name);
> +			return 0;
> +		}
> +
> +		ret = acpi_remember_or_free_map(p, "default", pctldev, new_map, num_maps);
> +		if (ret < 0) {
> +			dev_info(p->dev, "Failed to register maps\n");
> +			return ret;
> +		}
> +	}
> +	return 0;
> +}
>  
>  static int pinctrl_acpi_populate_group_desc(struct acpi_resource *ares, void *data)
>  {
> diff --git a/drivers/pinctrl/pinctrl-acpi.h b/drivers/pinctrl/pinctrl-acpi.h
> index 1a0c751a7594..4ed45b22257c 100644
> --- a/drivers/pinctrl/pinctrl-acpi.h
> +++ b/drivers/pinctrl/pinctrl-acpi.h
> @@ -12,11 +12,39 @@ struct pinctrl_acpi_group_desc {
>  	struct list_head list;
>  };


kernel-doc

>  
> +struct pinctrl_acpi_pin_group_map {
> +	const char *group;
> +	unsigned int pin;
> +	bool is_config;
> +    struct list_head info;

indent

> +	struct list_head list;
> +};
> +

kernel-doc

> +struct pinctrl_acpi_pin_group_info {
> +	unsigned config_func;
> +	void *vendor_data;

indent

> +    struct list_head list;
> +};
> +
>  #ifdef CONFIG_ACPI
>  int pinctrl_acpi_get_pin_groups(struct acpi_device *adev, struct list_head *group_desc_list);
> +
> +int pinctrl_acpi_to_map(struct pinctrl *p);
> +
> +void pinctrl_acpi_free_maps(struct pinctrl *p);
>  #else
>  int pinctrl_acpi_get_pin_groups(struct acpi_device *adev, struct list_head *group_desc_list)
>  {
>  	return -ENODEV;
>  }
> +
> +int pinctrl_acpi_to_map(struct pinctrl *p)
> +{
> +	return -ENODEV;
> +}
> +
> +void pinctrl_acpi_free_maps(struct pinctrl *p)
> +{
> +
> +}

static inline for these. Try also to compile with CONFIG_ACPI=n to see
what warnings you get.

>  #endif
> diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
> index 70b45d28e7a9..99a087888c0d 100644
> --- a/include/linux/pinctrl/pinctrl.h
> +++ b/include/linux/pinctrl/pinctrl.h
> @@ -84,6 +84,15 @@ struct pinctrl_gpio_range {
>   *	allocated members of the mapping table entries themselves. This
>   *	function is optional, and may be omitted for pinctrl drivers that do
>   *	not support device tree.
> + * @acpi_node_to_map: process acpi pin related properties, and create

ACPI

> + *	mapping table entries for it. These are returned through the @map and
> + *	@num_maps output parameters. This function is optional, and may be
> + *	omitted for pinctrl drivers that do not support acpi.

ACPI

> + * @acpi_free_map: free mapping table entries created via @dt_node_to_map. The

dt_node_to_map? Isn't that acpi_node_to_map?

> + *	top-level @map pointer must be freed, along with any dynamically
> + *	allocated members of the mapping table entries themselves. This
> + *	function is optional, and may be omitted for pinctrl drivers that do
> + *	not support acpi.

ACPI

>   */
>  struct pinctrl_ops {
>  	int (*get_groups_count) (struct pinctrl_dev *pctldev);
> @@ -100,6 +109,12 @@ struct pinctrl_ops {
>  			       struct pinctrl_map **map, unsigned *num_maps);
>  	void (*dt_free_map) (struct pinctrl_dev *pctldev,
>  			     struct pinctrl_map *map, unsigned num_maps);
> +	int (*acpi_node_to_map) (struct pinctrl_dev *pctldev,
> +			       struct list_head *info_list,
> +			       struct pinctrl_map **map, unsigned *num_maps);
> +	void (*acpi_free_map) (struct pinctrl_dev *pctldev,
> +				   struct pinctrl_map *map, unsigned num_maps);
> +
>  };
>  
>  /**
> -- 
> 2.25.1

Re: [PATCH RFC 1/3] pinctrl: add support for acpi PinGroup resource

[Niyas Sait]

Thanks, Mika for the feedback.

I have addressed most of your comments on V2 of the patch [1]


 >> +/* Get list of acpi pin groups definitions for the controller */
 >
 > Use proper kernel-doc here.
 >
 > Also who is responsible of releasing the thing and how it is done?
 >

I added a new interface in V2 to free the descriptors.
Pinctrl can free the descriptors after processing.

[1] 
https://lore.kernel.org/linux-gpio/20221115175415.650690-1-niyas.sait@linaro.org/T/#t

--
Niyas

On 11/11/2022 12:12, Mika Westerberg wrote:
> On Thu, Nov 10, 2022 at 07:12:56PM +0000, Niyas Sait wrote:
>> pinctrl-acpi parses and decode PinGroup resources for
>> the device and generate list of group descriptor.
>> Descriptors can be used by the pin controller to identify
>> the groups and pins provided in the group.
>>
>> Signed-off-by: Niyas Sait <niyas.sait@linaro.org>
>> ---
>>   drivers/pinctrl/Makefile       |  1 +
>>   drivers/pinctrl/pinctrl-acpi.c | 59 ++++++++++++++++++++++++++++++++++
>>   drivers/pinctrl/pinctrl-acpi.h | 22 +++++++++++++
>>   3 files changed, 82 insertions(+)
>>   create mode 100644 drivers/pinctrl/pinctrl-acpi.c
>>   create mode 100644 drivers/pinctrl/pinctrl-acpi.h
>>
>> diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
>> index e76f5cdc64b0..0b0ec4080942 100644
>> --- a/drivers/pinctrl/Makefile
>> +++ b/drivers/pinctrl/Makefile
>> @@ -8,6 +8,7 @@ obj-$(CONFIG_PINMUX)		+= pinmux.o
>>   obj-$(CONFIG_PINCONF)		+= pinconf.o
>>   obj-$(CONFIG_GENERIC_PINCONF)	+= pinconf-generic.o
>>   obj-$(CONFIG_OF)		+= devicetree.o
>> +obj-$(CONFIG_ACPI)		+= pinctrl-acpi.o
>>   
>>   obj-$(CONFIG_PINCTRL_AMD)	+= pinctrl-amd.o
>>   obj-$(CONFIG_PINCTRL_APPLE_GPIO) += pinctrl-apple-gpio.o
>> diff --git a/drivers/pinctrl/pinctrl-acpi.c b/drivers/pinctrl/pinctrl-acpi.c
>> new file mode 100644
>> index 000000000000..75e59fe22387
>> --- /dev/null
>> +++ b/drivers/pinctrl/pinctrl-acpi.c
>> @@ -0,0 +1,59 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright (C) 2022 Linaro Ltd.
>> + */
>> +#include <linux/acpi.h>
>> +#include <linux/errno.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/gpio/driver.h>
>> +#include <linux/gpio/machine.h>
>> +#include <linux/list.h>
>> +
>> +#include "pinctrl-acpi.h"
>> +
>> +static int pinctrl_acpi_populate_group_desc(struct acpi_resource *ares, void *data)
>> +{
>> +	struct acpi_resource_pin_group *ares_pin_group;
>> +	struct pinctrl_acpi_group_desc *desc;
>> +	struct list_head *group_desc_list = data;
>> +
>> +	if (ares->type != ACPI_RESOURCE_TYPE_PIN_GROUP)
>> +		return 1;
>> +
>> +	ares_pin_group = &ares->data.pin_group;
>> +
>> +	desc = kzalloc(sizeof(struct pinctrl_acpi_group_desc), GFP_KERNEL);
>> +	if (!desc)
>> +		return -ENOMEM;
>> +
>> +	desc->name = kstrdup_const(ares_pin_group->resource_label.string_ptr, GFP_KERNEL);
>> +	desc->pins = ares_pin_group->pin_table;
>> +	desc->num_pins = ares_pin_group->pin_table_length;
>> +	desc->vendor_data = ares_pin_group->vendor_data;
>> +	desc->vendor_length = ares_pin_group->vendor_length;
>> +
>> +	INIT_LIST_HEAD(&desc->list);
>> +	list_add(&desc->list, group_desc_list);
>> +
>> +	return 1;
>> +}
>> +
>> +/* Get list of acpi pin groups definitions for the controller */
> 
> Use proper kernel-doc here.
> 
> Also who is responsible of releasing the thing and how it is done?
> 
>> +int pinctrl_acpi_get_pin_groups(struct acpi_device *adev, struct list_head *group_desc_list)
>> +{
>> +	struct list_head res_list;
>> +	int ret;
>> +
>> +	INIT_LIST_HEAD(&res_list);
>> +	INIT_LIST_HEAD(group_desc_list);
>> +
>> +	ret = acpi_dev_get_resources(adev, &res_list,
>> +								 pinctrl_acpi_populate_group_desc,
>> +								 group_desc_list);
> 
> The formatting is wrong here.
> 
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	acpi_dev_free_resource_list(&res_list);
>> +
> 
> Drop the empty line.
> 
>> +	return 0;
>> +}
>> diff --git a/drivers/pinctrl/pinctrl-acpi.h b/drivers/pinctrl/pinctrl-acpi.h
>> new file mode 100644
>> index 000000000000..1a0c751a7594
>> --- /dev/null
>> +++ b/drivers/pinctrl/pinctrl-acpi.h
>> @@ -0,0 +1,22 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright (C) 2022 Linaro Ltd.
>> + */
>> +
> 
> kernel-doc here too.
> 
>> +struct pinctrl_acpi_group_desc {
>> +	const char *name;
>> +	short unsigned int *pins;
>> +	unsigned num_pins;
>> +	void *vendor_data;
>> +	unsigned vendor_length;
>> +	struct list_head list;
>> +};
>> +
>> +#ifdef CONFIG_ACPI
>> +int pinctrl_acpi_get_pin_groups(struct acpi_device *adev, struct list_head *group_desc_list);
>> +#else
>> +int pinctrl_acpi_get_pin_groups(struct acpi_device *adev, struct list_head *group_desc_list)
> 
> This needs to be static inline.
> 
>> +{
>> +	return -ENODEV;
> 
> -ENXIO?
> 
>> +}
>> +#endif
>> -- 
>> 2.25.1

Re: [PATCH RFC 3/3] pinctrl: add support for acpi pin function and config resources

[Niyas Sait]

 >> +static const char *acpi_node_to_device_name(char *acpi_node)
 >
 > Can this be const?
 >
 >> +{
 >> +	acpi_status status;
 >> +	acpi_handle handle;
 >> +	struct acpi_device *controller_device;
 >> +
 >> +	status = acpi_get_handle(NULL, acpi_node, &handle);
 >> +
 >

acpi_get_handle doesn't expect const char* and generates a warning 
otherwise.


On 11/11/2022 12:24, Mika Westerberg wrote:
> On Thu, Nov 10, 2022 at 07:12:58PM +0000, Niyas Sait wrote:
>> Add support for following acpi pin resources
>>
>> - PinFunction
>> - PinConfig
>> - PinGroupFunction
>> - PinGroupConfig
>>
>> Pinctrl-acpi parses the acpi table and generates list of pin
> 
> ACPI
> 
>> descriptors that can be used by pin controller to set and config pin.
>>
>> Descriptors are grouped by pin number or group name and contains list
>> of functions or configs to apply.
>>
>> Pin config types from acpi are converted to generic pin config types
> 
> ACPI (ditto everywhere).
> 
>> and passed through the descriptor.
>>
>> Signed-off-by: Niyas Sait <niyas.sait@linaro.org>
>> ---
>>   drivers/pinctrl/core.c          |  19 +-
>>   drivers/pinctrl/core.h          |   3 +
>>   drivers/pinctrl/pinctrl-acpi.c  | 391 ++++++++++++++++++++++++++++++++
>>   drivers/pinctrl/pinctrl-acpi.h  |  28 +++
>>   include/linux/pinctrl/pinctrl.h |  15 ++
>>   5 files changed, 452 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
>> index ffe39336fcac..03770ac66d48 100644
>> --- a/drivers/pinctrl/core.c
>> +++ b/drivers/pinctrl/core.c
>> @@ -25,6 +25,7 @@
>>   #include <linux/pinctrl/consumer.h>
>>   #include <linux/pinctrl/pinctrl.h>
>>   #include <linux/pinctrl/machine.h>
>> +#include <linux/acpi.h>
>>   
>>   #ifdef CONFIG_GPIOLIB
>>   #include "../gpio/gpiolib.h"
>> @@ -35,7 +36,7 @@
>>   #include "devicetree.h"
>>   #include "pinmux.h"
>>   #include "pinconf.h"
>> -
>> +#include "pinctrl-acpi.h"
>>   
>>   static bool pinctrl_dummy_state;
>>   
>> @@ -1042,9 +1043,15 @@ static struct pinctrl *create_pinctrl(struct device *dev,
>>   		return ERR_PTR(-ENOMEM);
>>   	p->dev = dev;
>>   	INIT_LIST_HEAD(&p->states);
>> -	INIT_LIST_HEAD(&p->dt_maps);
>>   
>> -	ret = pinctrl_dt_to_map(p, pctldev);
>> +	if (!ACPI_COMPANION(dev)) {
>> +		INIT_LIST_HEAD(&p->dt_maps);
>> +		ret = pinctrl_dt_to_map(p, pctldev);
>> +	} else {
>> +		INIT_LIST_HEAD(&p->acpi_maps);
>> +		ret = pinctrl_acpi_to_map(p);
>> +	}
>> +
>>   	if (ret < 0) {
>>   		kfree(p);
>>   		return ERR_PTR(ret);
>> @@ -1168,7 +1175,11 @@ static void pinctrl_free(struct pinctrl *p, bool inlist)
>>   		kfree(state);
>>   	}
>>   
>> -	pinctrl_dt_free_maps(p);
>> +	if (!ACPI_COMPANION(p->dev)) {
>> +		pinctrl_dt_free_maps(p);
>> +	} else {
>> +		pinctrl_acpi_free_maps(p);
>> +	}
> 
> You don't need the {}
> 
>>   
>>   	if (inlist)
>>   		list_del(&p->node);
>> diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
>> index 840103c40c14..28f2f9d518d4 100644
>> --- a/drivers/pinctrl/core.h
>> +++ b/drivers/pinctrl/core.h
>> @@ -72,6 +72,8 @@ struct pinctrl_dev {
>>    * @state: the current state
>>    * @dt_maps: the mapping table chunks dynamically parsed from device tree for
>>    *	this device, if any
>> + * @acpi_maps: the mapping table chunks dynamically parsed from acpi for this
>> + *  device, if any
>>    * @users: reference count
>>    */
>>   struct pinctrl {
>> @@ -80,6 +82,7 @@ struct pinctrl {
>>   	struct list_head states;
>>   	struct pinctrl_state *state;
>>   	struct list_head dt_maps;
>> +	struct list_head acpi_maps;
>>   	struct kref users;
>>   };
>>   
>> diff --git a/drivers/pinctrl/pinctrl-acpi.c b/drivers/pinctrl/pinctrl-acpi.c
>> index 75e59fe22387..9777577aefd6 100644
>> --- a/drivers/pinctrl/pinctrl-acpi.c
>> +++ b/drivers/pinctrl/pinctrl-acpi.c
>> @@ -10,6 +10,397 @@
>>   #include <linux/list.h>
>>   
>>   #include "pinctrl-acpi.h"
>> +#include "core.h"
>> +
>> +/**
>> + * struct pinctrl_acpi_map - mapping table chunk parsed from device tree
> 
> Parsed from ACPI namespace?
> 
>> + * @node: list node for struct pinctrl's @acpi_maps field
>> + * @pctldev: the pin controller that allocated this struct, and will free it
>> + * @map: the mapping table entries
>> + * @num_maps: number of mapping table entries
>> + */
>> +struct pinctrl_acpi_map {
>> +	struct list_head node;
>> +	struct pinctrl_dev *pctldev;
>> +	struct pinctrl_map *map;
>> +	unsigned int num_maps;
> 
> size_t?
> 
>> +};
>> +
>> +static void acpi_free_map(struct pinctrl_dev *pctldev,
>> +			 struct pinctrl_map *map, unsigned int num_maps)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < num_maps; ++i) {
>> +		kfree_const(map[i].dev_name);
>> +		map[i].dev_name = NULL;
>> +	}
>> +
>> +	if (pctldev) {
>> +		const struct pinctrl_ops *ops = pctldev->desc->pctlops;
>> +
>> +		if (ops->acpi_free_map)
>> +			ops->acpi_free_map(pctldev, map, num_maps);
>> +
>> +	} else {
>> +		/* There is no pctldev for PIN_MAP_TYPE_DUMMY_STATE */
>> +		kfree(map);
>> +	}
>> +}
>> +
> 
> kernel-doc?
> 
>> +void pinctrl_acpi_free_maps(struct pinctrl *p)
>> +{
>> +	struct pinctrl_acpi_map *acpi_map, *n1;
> 
> Why not 'tmp' instead of 'n1'?
> 
>> +
>> +	list_for_each_entry_safe(acpi_map, n1, &p->acpi_maps, node) {
>> +		pinctrl_unregister_mappings(acpi_map->map);
>> +		list_del(&acpi_map->node);
>> +		acpi_free_map(acpi_map->pctldev, acpi_map->map,
>> +				acpi_map->num_maps);
>> +		kfree(acpi_map);
>> +	}
>> +}
>> +
>> +static int acpi_remember_or_free_map(struct pinctrl *p, const char *statename,
>> +				   struct pinctrl_dev *pctldev,
>> +				   struct pinctrl_map *map, unsigned num_maps)
>> +{
>> +	int i;
>> +	struct pinctrl_acpi_map *acpi_map;
>> +
>> +	/* Initialize common mapping table entry fields */
>> +	for (i = 0; i < num_maps; i++) {
>> +		const char *devname;
>> +
>> +		devname = kstrdup_const(dev_name(p->dev), GFP_KERNEL);
>> +		if (!devname)
>> +			goto err_free_map;
>> +
>> +		map[i].dev_name = devname;
>> +		map[i].name = statename;
>> +		if (pctldev)
>> +			map[i].ctrl_dev_name = dev_name(pctldev->dev);
>> +	}
>> +
>> +	/* Remember the converted mapping table entries */
>> +	acpi_map = kzalloc(sizeof(*acpi_map), GFP_KERNEL);
>> +	if (!acpi_map)
>> +		goto err_free_map;
>> +
>> +	acpi_map->pctldev = pctldev;
>> +	acpi_map->map = map;
>> +	acpi_map->num_maps = num_maps;
>> +	list_add_tail(&acpi_map->node, &p->acpi_maps);
>> +
>> +	return pinctrl_register_mappings(map, num_maps);
>> +
>> +err_free_map:
>> +	acpi_free_map(pctldev, map, num_maps);
>> +	return -ENOMEM;
>> +}
>> +
>> +/* Convert raw acpi device references to device name */
> 
> raw ACPI device or raw acpi_device.
> 
>> +static const char *acpi_node_to_device_name(char *acpi_node)
> 
> Can this be const?
> 
>> +{
>> +	acpi_status status;
>> +	acpi_handle handle;
>> +	struct acpi_device *controller_device;
>> +
>> +	status = acpi_get_handle(NULL, acpi_node, &handle);
>> +
> 
> Drop the empty line.
> 
>> +	if (ACPI_FAILURE(status))
>> +		return NULL;
>> +
>> +	controller_device = acpi_bus_get_acpi_device(handle);
> 
> Ditto.
> 
>> +
>> +	if (!controller_device)
>> +		return NULL;
>> +
>> +	return acpi_dev_name(controller_device);
>> +}
>> +
>> +/* Map acpi pin configuration types to pinctrl general configuration type */
>> +static unsigned map_acpi_conf_to_general_conf(unsigned param, unsigned value)
>> +{
>> +	switch (param) {
>> +	case ACPI_PIN_CONFIG_DEFAULT:
>> +		return pinconf_to_config_packed(ACPI_PIN_CONFIG_DEFAULT, 0);
>> +	case ACPI_PIN_CONFIG_BIAS_PULL_UP:
>> +		return pinconf_to_config_packed(PIN_CONFIG_BIAS_PULL_UP_OHMS, value);
>> +	case ACPI_PIN_CONFIG_BIAS_PULL_DOWN:
>> +		return pinconf_to_config_packed(PIN_CONFIG_BIAS_PULL_DOWN_OHMS, value);
>> +	case ACPI_PIN_CONFIG_BIAS_DEFAULT:
>> +		return pinconf_to_config_packed(PIN_CONFIG_BIAS_PULL_PIN_DEFAULT, 0);
>> +	case ACPI_PIN_CONFIG_BIAS_DISABLE:
>> +		return pinconf_to_config_packed(PIN_CONFIG_BIAS_DISABLE, 0);
>> +	case ACPI_PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
>> +		return pinconf_to_config_packed(PIN_CONFIG_BIAS_HIGH_IMPEDANCE, 0);
>> +	case ACPI_PIN_CONFIG_BIAS_BUS_HOLD:
>> +		return pinconf_to_config_packed(PIN_CONFIG_BIAS_BUS_HOLD, 0);
>> +	case ACPI_PIN_CONFIG_DRIVE_OPEN_DRAIN:
>> +		return pinconf_to_config_packed(PIN_CONFIG_DRIVE_OPEN_DRAIN, 0);
>> +	case ACPI_PIN_CONFIG_DRIVE_OPEN_SOURCE:
>> +		return pinconf_to_config_packed(PIN_CONFIG_DRIVE_OPEN_SOURCE, 0);
>> +	case ACPI_PIN_CONFIG_DRIVE_PUSH_PULL:
>> +		return pinconf_to_config_packed(PIN_CONFIG_DRIVE_PUSH_PULL, 0);
>> +	case ACPI_PIN_CONFIG_DRIVE_STRENGTH:
>> +		return pinconf_to_config_packed(PIN_CONFIG_DRIVE_STRENGTH, value);
>> +	case ACPI_PIN_CONFIG_SLEW_RATE:
>> +		return pinconf_to_config_packed(PIN_CONFIG_SLEW_RATE, value);
>> +	case ACPI_PIN_CONFIG_INPUT_DEBOUNCE:
>> +		return pinconf_to_config_packed(PIN_CONFIG_INPUT_DEBOUNCE, value);
>> +	case ACPI_PIN_CONFIG_INPUT_SCHMITT_TRIGGER:
>> +		return pinconf_to_config_packed(PIN_CONFIG_INPUT_SCHMITT_ENABLE, value);
>> +	default:
>> +		pr_warn("PINCTRL: ACPI pin configuration type (%d) not handled\n", param);
> 
> Don't you have any dev * pointer that you can use here instead of
> pr_warn()?
> 
>> +		return pinconf_to_config_packed(ACPI_PIN_CONFIG_DEFAULT, 0);
>> +	}
>> +}
>> +
>> +struct pinctrl_acpi_controller_map {
>> +	char *pinctrl_dev;
> 
> const char *?
> 
>> +	struct list_head list;
>> +	struct list_head pin_group_maps;
>> +};
>> +
>> +/* Add pin/group function and configuration descriptor to internal map */
>> +static int add_pin_group_node(struct list_head *acpi_map_head,
>> +							char *pinctrl_dev,
>> +							char *group,
>> +							unsigned pin,
>> +							bool is_config,
>> +							unsigned config_func,
>> +							void *vendor_data)
>> +{
>> +	struct pinctrl_acpi_controller_map *acpi_controller_map = NULL;
>> +	struct pinctrl_acpi_controller_map *acpi_controller_map_iter = NULL;
>> +	struct pinctrl_acpi_pin_group_map *pin_group_map = NULL;
>> +	struct pinctrl_acpi_pin_group_map *pin_group_map_iter = NULL;
>> +	struct pinctrl_acpi_pin_group_info *info = NULL;
> 
> 
> Do you need to initialize them all?
> 
>> +	bool group_pin_match;
>> +
>> +	/* Find the pin controller specific list to use to add the descriptor */
>> +	list_for_each_entry(acpi_controller_map_iter, acpi_map_head, list) {
>> +		if (!strcmp(acpi_controller_map_iter->pinctrl_dev, pinctrl_dev)) {
>> +			acpi_controller_map = acpi_controller_map_iter;
>> +			break;
>> +		}
>> +	}
>> +
>> +	/* If this is the first entry for the pin controller, allocate an entry */
>> +	if (!acpi_controller_map) {
>> +		acpi_controller_map = kzalloc(sizeof(struct pinctrl_acpi_controller_map), GFP_KERNEL);
>> +
> 
> Drop the empty line.
> 
>> +		if (!acpi_controller_map)
>> +			return -ENOMEM;
>> +
>> +		acpi_controller_map->pinctrl_dev = pinctrl_dev;
>> +		INIT_LIST_HEAD(&acpi_controller_map->list);
>> +		INIT_LIST_HEAD(&acpi_controller_map->pin_group_maps);
>> +		list_add(&acpi_controller_map->list, acpi_map_head);
>> +	}
>> +
>> +	/* Find the group/pin specific node from the descriptor list */
>> +	list_for_each_entry(pin_group_map_iter, &acpi_controller_map->pin_group_maps, list) {
>> +		if (group)
>> +			group_pin_match = !strcmp(pin_group_map_iter->group, group);
>> +		else
>> +			group_pin_match = (pin == pin_group_map_iter->pin);
> 
> Empty line
> 
>> +		if (pin_group_map_iter->is_config == is_config && group_pin_match) {
>> +			pin_group_map = pin_group_map_iter;
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (!pin_group_map) {
>> +		pin_group_map = kzalloc(sizeof(struct pinctrl_acpi_pin_group_map), GFP_KERNEL);
>> +
> 
> Drop the empty line ;-)
> 
>> +		if (!pin_group_map)
>> +			return -ENOMEM;
>> +
>> +		pin_group_map->group = group;
>> +		pin_group_map->pin = pin;
>> +		pin_group_map->is_config = is_config;
>> +		INIT_LIST_HEAD(&pin_group_map->list);
>> +		INIT_LIST_HEAD(&pin_group_map->info);
>> +		list_add(&pin_group_map->list, &acpi_controller_map->pin_group_maps);
>> +	}
>> +
>> +	/* Allocate descriptor and add the pin configuration/function info */
>> +	info = kzalloc(sizeof(struct pinctrl_acpi_pin_group_info), GFP_KERNEL);
> 
> Drop the empty line.
> 
>> +
>> +	if (!info)
>> +		return -ENOMEM;
>> +
>> +	info->config_func = config_func;
>> +	info->vendor_data = vendor_data;
>> +	INIT_LIST_HEAD(&info->list);
>> +	list_add(&info->list, &pin_group_map->info);
>> +
>> +	return 0;
>> +}
>> +
>> +static int pinctrl_acpi_populate_pin_group_map(struct acpi_resource *ares, void *data)
>> +{
>> +	struct acpi_resource_pin_function *ares_pin_function;
>> +	struct acpi_resource_pin_config *ares_pin_config;
>> +	struct acpi_resource_pin_group_function *ares_pin_group_function;
>> +	struct acpi_resource_pin_group_config *ares_pin_group_config;
>> +	struct list_head *acpi_map_head = data;
>> +	int i;
>> +	int ret;
>> +	unsigned int config;
>> +	char *pinctrl_dev;
>> +	char *group;
>> +	unsigned int pin;
>> +	void *vendor_data;
>> +	unsigned int func;
> 
> That's huge amount of variables. I wonder if this can be reduced with
> helper functions etc?
> 
>> +
>> +	switch (ares->type) {
>> +	case ACPI_RESOURCE_TYPE_PIN_FUNCTION:
>> +		ares_pin_function = &ares->data.pin_function;
>> +		vendor_data = ares_pin_function->vendor_data;
>> +		pinctrl_dev = ares_pin_function->resource_source.string_ptr;
>> +		group = NULL;
>> +		func = ares_pin_function->function_number;
>> +		config = map_acpi_conf_to_general_conf(ares_pin_function->pin_config, 0);
>> +
>> +		for (i = 0; i < ares_pin_function->pin_table_length; i++) {
>> +
> 
> Drop the empty line
> 
>> +			ret = add_pin_group_node(acpi_map_head, pinctrl_dev, group,
>> +					ares_pin_function->pin_table[i], false, func, vendor_data);
>> +
>> +			if (ret < 0)
>> +				return ret;
>> +
>> +			ret = add_pin_group_node(acpi_map_head, pinctrl_dev, group,
>> +					ares_pin_function->pin_table[i], true, config, vendor_data);
>> +
>> +			if (ret < 0)
>> +				return ret;
>> +		}
>> +		break;
> 
> Add empty line
> 
> (Ditto everywhere below)
> 
>> +	case ACPI_RESOURCE_TYPE_PIN_CONFIG:
>> +		ares_pin_config = &ares->data.pin_config;
>> +		pinctrl_dev = ares_pin_config->resource_source.string_ptr;
>> +		group = NULL;
>> +		func = 0;
>> +
>> +		config = map_acpi_conf_to_general_conf(
>> +			ares_pin_config->pin_config_type,
>> +			ares_pin_config->pin_config_value);
>> +
>> +		vendor_data = ares_pin_config->vendor_data;
>> +
>> +		for (i = 0; i < ares_pin_function->pin_table_length; i++) {
>> +			pin = ares_pin_config->pin_table[i];
>> +
>> +			ret = add_pin_group_node(acpi_map_head, pinctrl_dev,
>> +						group, pin, true, config, vendor_data);
>> +			if (ret < 0)
>> +				return ret;
>> +		}
>> +		break;
>> +	case ACPI_RESOURCE_TYPE_PIN_GROUP_FUNCTION:
>> +		ares_pin_group_function = &ares->data.pin_group_function;
>> +		vendor_data = ares_pin_group_function->vendor_data;
>> +		pinctrl_dev = ares_pin_group_function->resource_source.string_ptr;
>> +		group = ares_pin_group_function->resource_source_label.string_ptr;
>> +		pin = 0;
>> +		func = ares_pin_group_function->function_number;
>> +		ret = add_pin_group_node(acpi_map_head, pinctrl_dev,
>> +					group, pin, false, func, vendor_data);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		break;
>> +	case ACPI_RESOURCE_TYPE_PIN_GROUP_CONFIG:
>> +		ares_pin_group_config = &ares->data.pin_group_config;
>> +		vendor_data = ares_pin_group_config->vendor_data;
>> +		pinctrl_dev = ares_pin_group_config->resource_source.string_ptr;
>> +		group = ares_pin_group_config->resource_source_label.string_ptr;
>> +		pin = 0;
>> +
>> +		config = map_acpi_conf_to_general_conf(
>> +					ares_pin_group_config->pin_config_type,
>> +					ares_pin_group_config->pin_config_value);
>> +
>> +		ret = add_pin_group_node(acpi_map_head, pinctrl_dev, group,
>> +					pin, true, config, vendor_data);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		break;
>> +	}
>> +	return 1;
>> +}
>> +
>> +static int pinctrl_acpi_get_pin_group_map(struct acpi_device *adev, struct list_head *pin_group_root)
>> +{
>> +	struct list_head res_list;
>> +	int ret;
>> +
>> +	INIT_LIST_HEAD(&res_list);
>> +
>> +	ret = acpi_dev_get_resources(adev, &res_list,
>> +								 pinctrl_acpi_populate_pin_group_map,
>> +								 pin_group_root);
>> +
> 
> Weird formatting.
> 
>> +	acpi_dev_free_resource_list(&res_list);
>> +
>> +	return ret;
>> +}
>> +
>> +/* Decode and register acpi pinctrl related properties to pinctrl system */
> 
> Kernel-doc
> 
>> +int pinctrl_acpi_to_map(struct pinctrl *p)
>> +{
>> +	int num_maps;
>> +	int ret;
>> +	struct acpi_device *adev;
>> +	struct list_head pin_group_list;
>> +	struct pinctrl_map *new_map;
>> +	struct pinctrl_dev *pctldev;
>> +	const struct pinctrl_ops *ops;
>> +	struct pinctrl_acpi_controller_map *controller_map;
>> +
>> +	adev = ACPI_COMPANION(p->dev);
>> +	if (!adev)
>> +		return -ENODEV;
>> +
>> +	/* list to hold the pin/group descriptors generated, grouped by pin controller, pin/group name*/
>> +	INIT_LIST_HEAD(&pin_group_list);
>> +
>> +	ret = pinctrl_acpi_get_pin_group_map(adev, &pin_group_list);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* Iterate over descriptor for each pin controller and invoke the driver function */
>> +	list_for_each_entry(controller_map, &pin_group_list, list) {
>> +		const char *pinctrl_dev_name = acpi_node_to_device_name(controller_map->pinctrl_dev);
>> +
>> +		pctldev = get_pinctrl_dev_from_devname(pinctrl_dev_name);
>> +		ops = pctldev->desc->pctlops;
>> +		if (!ops->acpi_node_to_map) {
>> +			dev_err(p->dev, "pctldev %s doesn't support ACPI\n",
>> +				dev_name(pctldev->dev));
>> +			return -ENODEV;
>> +		}
>> +		ret = ops->acpi_node_to_map(pctldev, &controller_map->pin_group_maps, &new_map, &num_maps);
>> +		if (ret < 0) {
>> +			return ret;
>> +		} else if (num_maps == 0) {
>> +			dev_info(p->dev, "there is not valid maps for pin controller %s\n", pinctrl_dev_name);
>> +			return 0;
>> +		}
>> +
>> +		ret = acpi_remember_or_free_map(p, "default", pctldev, new_map, num_maps);
>> +		if (ret < 0) {
>> +			dev_info(p->dev, "Failed to register maps\n");
>> +			return ret;
>> +		}
>> +	}
>> +	return 0;
>> +}
>>   
>>   static int pinctrl_acpi_populate_group_desc(struct acpi_resource *ares, void *data)
>>   {
>> diff --git a/drivers/pinctrl/pinctrl-acpi.h b/drivers/pinctrl/pinctrl-acpi.h
>> index 1a0c751a7594..4ed45b22257c 100644
>> --- a/drivers/pinctrl/pinctrl-acpi.h
>> +++ b/drivers/pinctrl/pinctrl-acpi.h
>> @@ -12,11 +12,39 @@ struct pinctrl_acpi_group_desc {
>>   	struct list_head list;
>>   };
> 
> 
> kernel-doc
> 
>>   
>> +struct pinctrl_acpi_pin_group_map {
>> +	const char *group;
>> +	unsigned int pin;
>> +	bool is_config;
>> +    struct list_head info;
> 
> indent
> 
>> +	struct list_head list;
>> +};
>> +
> 
> kernel-doc
> 
>> +struct pinctrl_acpi_pin_group_info {
>> +	unsigned config_func;
>> +	void *vendor_data;
> 
> indent
> 
>> +    struct list_head list;
>> +};
>> +
>>   #ifdef CONFIG_ACPI
>>   int pinctrl_acpi_get_pin_groups(struct acpi_device *adev, struct list_head *group_desc_list);
>> +
>> +int pinctrl_acpi_to_map(struct pinctrl *p);
>> +
>> +void pinctrl_acpi_free_maps(struct pinctrl *p);
>>   #else
>>   int pinctrl_acpi_get_pin_groups(struct acpi_device *adev, struct list_head *group_desc_list)
>>   {
>>   	return -ENODEV;
>>   }
>> +
>> +int pinctrl_acpi_to_map(struct pinctrl *p)
>> +{
>> +	return -ENODEV;
>> +}
>> +
>> +void pinctrl_acpi_free_maps(struct pinctrl *p)
>> +{
>> +
>> +}
> 
> static inline for these. Try also to compile with CONFIG_ACPI=n to see
> what warnings you get.
> 
>>   #endif
>> diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
>> index 70b45d28e7a9..99a087888c0d 100644
>> --- a/include/linux/pinctrl/pinctrl.h
>> +++ b/include/linux/pinctrl/pinctrl.h
>> @@ -84,6 +84,15 @@ struct pinctrl_gpio_range {
>>    *	allocated members of the mapping table entries themselves. This
>>    *	function is optional, and may be omitted for pinctrl drivers that do
>>    *	not support device tree.
>> + * @acpi_node_to_map: process acpi pin related properties, and create
> 
> ACPI
> 
>> + *	mapping table entries for it. These are returned through the @map and
>> + *	@num_maps output parameters. This function is optional, and may be
>> + *	omitted for pinctrl drivers that do not support acpi.
> 
> ACPI
> 
>> + * @acpi_free_map: free mapping table entries created via @dt_node_to_map. The
> 
> dt_node_to_map? Isn't that acpi_node_to_map?
> 
>> + *	top-level @map pointer must be freed, along with any dynamically
>> + *	allocated members of the mapping table entries themselves. This
>> + *	function is optional, and may be omitted for pinctrl drivers that do
>> + *	not support acpi.
> 
> ACPI
> 
>>    */
>>   struct pinctrl_ops {
>>   	int (*get_groups_count) (struct pinctrl_dev *pctldev);
>> @@ -100,6 +109,12 @@ struct pinctrl_ops {
>>   			       struct pinctrl_map **map, unsigned *num_maps);
>>   	void (*dt_free_map) (struct pinctrl_dev *pctldev,
>>   			     struct pinctrl_map *map, unsigned num_maps);
>> +	int (*acpi_node_to_map) (struct pinctrl_dev *pctldev,
>> +			       struct list_head *info_list,
>> +			       struct pinctrl_map **map, unsigned *num_maps);
>> +	void (*acpi_free_map) (struct pinctrl_dev *pctldev,
>> +				   struct pinctrl_map *map, unsigned num_maps);
>> +
>>   };
>>   
>>   /**
>> -- 
>> 2.25.1

Thanks again for the detailed review.

I've addressed all of the comments in V2 - 
https://lore.kernel.org/linux-gpio/Y24+8mtqqqJWwNSR@black.fi.intel.com/T/#u