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

[PATCH RFC v2 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.

--
V2: 1, Fix styling issues pointed out by Mika Westerberg
    2, Added a new interface to free pin group descriptor
    3, Added vendor length to the descriptors
    4, Reworked map_config_acpi_to_general to pass an error value
    5, Few refactoring to keep functions shorter
    6, Dropped new generic pinconf types added in v1

Niyas Sait (3):
  pinctrl: add support for ACPI PinGroup resource
  pinconf-generic: clarify pull up and pull down config values
  pinctrl: add support for ACPI pin function and config resources

 drivers/pinctrl/Makefile                |   1 +
 drivers/pinctrl/core.c                  |  18 +-
 drivers/pinctrl/core.h                  |   3 +
 drivers/pinctrl/pinctrl-acpi.c          | 542 ++++++++++++++++++++++++
 drivers/pinctrl/pinctrl-acpi.h          |  78 ++++
 include/linux/pinctrl/pinconf-generic.h |   6 +-
 include/linux/pinctrl/pinctrl.h         |  15 +
 7 files changed, 657 insertions(+), 6 deletions(-)
 create mode 100644 drivers/pinctrl/pinctrl-acpi.c
 create mode 100644 drivers/pinctrl/pinctrl-acpi.h

-- 
2.25.1

[PATCH RFC v2 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 | 99 ++++++++++++++++++++++++++++++++++
 drivers/pinctrl/pinctrl-acpi.h | 34 ++++++++++++
 3 files changed, 134 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 89bfa01b5231..b5423465131f 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..cd0d4b2d8868
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-acpi.c
@@ -0,0 +1,99 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ACPI helpers for PinCtrl API
+ *
+ * Copyright (C) 2022 Linaro Ltd.
+ * Author: Niyas Sait <niyas.sait@linaro.org>
+ */
+#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;
+	int i;
+
+	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->num_pins = ares_pin_group->pin_table_length;
+	desc->pins = kmalloc_array(desc->num_pins, sizeof(*desc->pins), GFP_KERNEL);
+	if (!desc->pins)
+		return -ENOMEM;
+
+	for (i = 0; i < desc->num_pins; i++)
+		desc->pins[i] = ares_pin_group->pin_table[i];
+
+	desc->vendor_length = ares_pin_group->vendor_length;
+	desc->vendor_data = kmalloc_array(desc->vendor_length,
+				sizeof(*desc->vendor_data),
+				GFP_KERNEL);
+	if (!desc->vendor_data)
+		return -ENOMEM;
+
+	for (i = 0; i < desc->vendor_length; i++)
+		desc->vendor_data[i] = ares_pin_group->vendor_data[i];
+
+	INIT_LIST_HEAD(&desc->list);
+	list_add(&desc->list, group_desc_list);
+
+	return 1;
+}
+
+/**
+ * pinctrl_acpi_get_pin_groups() - Get ACPI PinGroup Descriptors for the device
+ * @adev: ACPI device node for retrieving PinGroup descriptors
+ * @group_desc_list: list head to add PinGroup descriptors
+ *
+ * This will parse ACPI PinGroup resources for the given ACPI device
+ * and will add descriptors to the provided @group_desc_list list
+ */
+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;
+}
+
+/**
+ * pinctrl_acpi_free_group_desc() - free allocated group descriptor
+ * @group_desc_list: list head for group descriptor to free
+ *
+ * Call this function to free the allocated group descriptors
+ */
+void pinctrl_acpi_free_group_desc(struct list_head *group_desc_list)
+{
+	struct pinctrl_acpi_group_desc *grp, *tmp;
+
+	list_for_each_entry_safe(grp, tmp, group_desc_list, list) {
+		list_del(&grp->list);
+		kfree_const(grp->name);
+		kfree(grp->pins);
+		kfree(grp->vendor_data);
+		kfree(grp);
+	}
+}
diff --git a/drivers/pinctrl/pinctrl-acpi.h b/drivers/pinctrl/pinctrl-acpi.h
new file mode 100644
index 000000000000..e3a6b61bea90
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-acpi.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * ACPI helpers for PinCtrl API
+ *
+ * Copyright (C) 2022 Linaro Ltd.
+ */
+
+/**
+ * struct pinctrl_acpi_group_desc - Descriptor to hold PinGroup resource from ACPI
+ * @name: name of the pin group
+ * @pins: array of pins that belong to the group
+ * @num_pins: number of pins in the group
+ * @vendor_data: vendor data from parsed ACPI resources
+ * @vendor_length: length of vendor data
+ * @list: list head for the descriptor
+ */
+struct pinctrl_acpi_group_desc {
+	const char *name;
+	u16 *pins;
+	unsigned int num_pins;
+	u8 *vendor_data;
+	unsigned int 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
+static inline int pinctrl_acpi_get_pin_groups(struct acpi_device *adev,
+			struct list_head *group_desc_list)
+{
+	return -ENXIO;
+}
+#endif
-- 
2.25.1

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

[Mika Westerberg]

On Tue, Nov 15, 2022 at 05:54:13PM +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 | 99 ++++++++++++++++++++++++++++++++++
>  drivers/pinctrl/pinctrl-acpi.h | 34 ++++++++++++
>  3 files changed, 134 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 89bfa01b5231..b5423465131f 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..cd0d4b2d8868
> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-acpi.c
> @@ -0,0 +1,99 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ACPI helpers for PinCtrl API
> + *
> + * Copyright (C) 2022 Linaro Ltd.
> + * Author: Niyas Sait <niyas.sait@linaro.org>
> + */
> +#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;
> +	int i;
> +
> +	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->num_pins = ares_pin_group->pin_table_length;
> +	desc->pins = kmalloc_array(desc->num_pins, sizeof(*desc->pins), GFP_KERNEL);
> +	if (!desc->pins)
> +		return -ENOMEM;

Here you leak desc.

> +
> +	for (i = 0; i < desc->num_pins; i++)
> +		desc->pins[i] = ares_pin_group->pin_table[i];
> +
> +	desc->vendor_length = ares_pin_group->vendor_length;
> +	desc->vendor_data = kmalloc_array(desc->vendor_length,
> +				sizeof(*desc->vendor_data),
> +				GFP_KERNEL);
> +	if (!desc->vendor_data)
> +		return -ENOMEM;

And this one leaks also ->pins.

> +
> +	for (i = 0; i < desc->vendor_length; i++)
> +		desc->vendor_data[i] = ares_pin_group->vendor_data[i];
> +
> +	INIT_LIST_HEAD(&desc->list);
> +	list_add(&desc->list, group_desc_list);
> +
> +	return 1;
> +}
> +
> +/**
> + * pinctrl_acpi_get_pin_groups() - Get ACPI PinGroup Descriptors for the device
> + * @adev: ACPI device node for retrieving PinGroup descriptors
> + * @group_desc_list: list head to add PinGroup descriptors
> + *
> + * This will parse ACPI PinGroup resources for the given ACPI device
> + * and will add descriptors to the provided @group_desc_list list

I would add here what happens to group_desc_list if the function returns
non-zero.

Also perhaps the API should use an array instead and when NULL is passed
it returns the size as we do with properties for example. The naged
list_head pointer looks kind of weird.

> + */
> +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;
> +}
> +
> +/**
> + * pinctrl_acpi_free_group_desc() - free allocated group descriptor

Get the capitalization consistent. Here you have 'free ..' above you
have 'Get ..'.

> + * @group_desc_list: list head for group descriptor to free
> + *
> + * Call this function to free the allocated group descriptors
> + */
> +void pinctrl_acpi_free_group_desc(struct list_head *group_desc_list)
> +{
> +	struct pinctrl_acpi_group_desc *grp, *tmp;
> +
> +	list_for_each_entry_safe(grp, tmp, group_desc_list, list) {
> +		list_del(&grp->list);
> +		kfree_const(grp->name);
> +		kfree(grp->pins);
> +		kfree(grp->vendor_data);
> +		kfree(grp);
> +	}
> +}
> diff --git a/drivers/pinctrl/pinctrl-acpi.h b/drivers/pinctrl/pinctrl-acpi.h
> new file mode 100644
> index 000000000000..e3a6b61bea90
> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-acpi.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * ACPI helpers for PinCtrl API
> + *
> + * Copyright (C) 2022 Linaro Ltd.
> + */
> +
> +/**
> + * struct pinctrl_acpi_group_desc - Descriptor to hold PinGroup resource from ACPI
> + * @name: name of the pin group
> + * @pins: array of pins that belong to the group
> + * @num_pins: number of pins in the group
> + * @vendor_data: vendor data from parsed ACPI resources
> + * @vendor_length: length of vendor data
> + * @list: list head for the descriptor
> + */
> +struct pinctrl_acpi_group_desc {
> +	const char *name;
> +	u16 *pins;
> +	unsigned int num_pins;

size_t?

npins intead of num_pins?

> +	u8 *vendor_data;

void *?

> +	unsigned int vendor_length;

size_t?

vendor_data_size perhaps?

> +	struct list_head list;
> +};
> +
> +#ifdef CONFIG_ACPI
> +int pinctrl_acpi_get_pin_groups(struct acpi_device *adev, struct list_head *group_desc_list);
> +#else
> +static inline int pinctrl_acpi_get_pin_groups(struct acpi_device *adev,
> +			struct list_head *group_desc_list)
> +{
> +	return -ENXIO;
> +}
> +#endif
> -- 
> 2.25.1

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

[Niyas Sait]

On 16/11/2022 09:41, Mika Westerberg wrote:

>> + * pinctrl_acpi_get_pin_groups() - Get ACPI PinGroup Descriptors for the device
>> + * @adev: ACPI device node for retrieving PinGroup descriptors
>> + * @group_desc_list: list head to add PinGroup descriptors
>> + *
>> + * This will parse ACPI PinGroup resources for the given ACPI device
>> + * and will add descriptors to the provided @group_desc_list list
> I would add here what happens to group_desc_list if the function returns
> non-zero.
> 
> Also perhaps the API should use an array instead and when NULL is passed
> it returns the size as we do with properties for example. The naged
> list_head pointer looks kind of weird.

Array would be nice. However I might have to do an additional acpi walk 
to find the number of PinGroups to allocate array and another iteration 
to populate fields. May be two iterations are not as bad as I thought. 
Open to suggestions.

--
Niyas

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

[Mika Westerberg]

On Thu, Nov 17, 2022 at 01:28:01PM +0000, Niyas Sait wrote:
> On 16/11/2022 09:41, Mika Westerberg wrote:
> 
> > > + * pinctrl_acpi_get_pin_groups() - Get ACPI PinGroup Descriptors for the device
> > > + * @adev: ACPI device node for retrieving PinGroup descriptors
> > > + * @group_desc_list: list head to add PinGroup descriptors
> > > + *
> > > + * This will parse ACPI PinGroup resources for the given ACPI device
> > > + * and will add descriptors to the provided @group_desc_list list
> > I would add here what happens to group_desc_list if the function returns
> > non-zero.
> > 
> > Also perhaps the API should use an array instead and when NULL is passed
> > it returns the size as we do with properties for example. The naged
> > list_head pointer looks kind of weird.
> 
> Array would be nice. However I might have to do an additional acpi walk to
> find the number of PinGroups to allocate array and another iteration to
> populate fields. May be two iterations are not as bad as I thought. Open to
> suggestions.

I don't think we need to "optimize" anything at this point and this is
certainly not a hot path. I suggest to emphasize on the API and think
what is easier for the caller (and also if there is simila API already
follow that it does).

[PATCH RFC v2 2/3] pinconf-generic: clarify pull up and pull down config values

[Niyas Sait]

PIN_CONFIG_BIAS_PULL_DOWN and PIN_CONFIG_BIAS_PULL_UP values can
be custom or an SI unit such as ohms

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

diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
index 2422211d6a5a..11e7394bcc70 100644
--- a/include/linux/pinctrl/pinconf-generic.h
+++ b/include/linux/pinctrl/pinconf-generic.h
@@ -35,7 +35,8 @@ struct pinctrl_map;
  *	impedance.
  * @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.
+ *	the value is interpreted by the driver and can be custom or an SI unit
+ *  such as 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
@@ -46,7 +47,8 @@ struct pinctrl_map;
  *	@PIN_CONFIG_BIAS_DISABLE.
  * @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.
+ *	the value is interpreted by the driver and can be custom or an SI unit
+ *  such as 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
-- 
2.25.1

Re: [PATCH RFC v2 2/3] pinconf-generic: clarify pull up and pull down config values

[Linus Walleij]

On Tue, Nov 15, 2022 at 6:54 PM Niyas Sait <niyas.sait@linaro.org> wrote:

> PIN_CONFIG_BIAS_PULL_DOWN and PIN_CONFIG_BIAS_PULL_UP values can
> be custom or an SI unit such as ohms
>
> Signed-off-by: Niyas Sait <niyas.sait@linaro.org>

This patch is good as-is so I just applied it. No need to resend with
the ACPI series.

Yours,
Linus Walleij

Re: [PATCH RFC v2 2/3] pinconf-generic: clarify pull up and pull down config values

[Andy Shevchenko]

On Thu, Nov 17, 2022 at 10:30:37AM +0100, Linus Walleij wrote:
> On Tue, Nov 15, 2022 at 6:54 PM Niyas Sait <niyas.sait@linaro.org> wrote:
> 
> > PIN_CONFIG_BIAS_PULL_DOWN and PIN_CONFIG_BIAS_PULL_UP values can
> > be custom or an SI unit such as ohms
> >
> > Signed-off-by: Niyas Sait <niyas.sait@linaro.org>
> 
> This patch is good as-is so I just applied it. No need to resend with
> the ACPI series.

Is it? I think it's visible that it has TAB vs. space issue...

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFC v2 2/3] pinconf-generic: clarify pull up and pull down config values

[Andy Shevchenko]

On Thu, Nov 17, 2022 at 12:37:44PM +0200, Andy Shevchenko wrote:
> On Thu, Nov 17, 2022 at 10:30:37AM +0100, Linus Walleij wrote:
> > On Tue, Nov 15, 2022 at 6:54 PM Niyas Sait <niyas.sait@linaro.org> wrote:
> > 
> > > PIN_CONFIG_BIAS_PULL_DOWN and PIN_CONFIG_BIAS_PULL_UP values can
> > > be custom or an SI unit such as ohms
> > >
> > > Signed-off-by: Niyas Sait <niyas.sait@linaro.org>
> > 
> > This patch is good as-is so I just applied it. No need to resend with
> > the ACPI series.
> 
> Is it? I think it's visible that it has TAB vs. space issue...

To be more clear, the "such as ohms" have been indented with spaces, while
everything else with TABs. On top of that I would use proper capitalization
for unit, i.e. Ohms. But the latter is minor.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFC v2 2/3] pinconf-generic: clarify pull up and pull down config values

[Niyas Sait]

Sorry for the newbie issues. Somehow I missed the space vs tab issue on 
that patch.

I will send a patch to fix the two issues.

--
Niyas


On 17/11/2022 10:39, Andy Shevchenko wrote:
> On Thu, Nov 17, 2022 at 12:37:44PM +0200, Andy Shevchenko wrote:
>> On Thu, Nov 17, 2022 at 10:30:37AM +0100, Linus Walleij wrote:
>>> On Tue, Nov 15, 2022 at 6:54 PM Niyas Sait <niyas.sait@linaro.org> wrote:
>>>
>>>> PIN_CONFIG_BIAS_PULL_DOWN and PIN_CONFIG_BIAS_PULL_UP values can
>>>> be custom or an SI unit such as ohms
>>>>
>>>> Signed-off-by: Niyas Sait <niyas.sait@linaro.org>
>>>
>>> This patch is good as-is so I just applied it. No need to resend with
>>> the ACPI series.
>>
>> Is it? I think it's visible that it has TAB vs. space issue...
> 
> To be more clear, the "such as ohms" have been indented with spaces, while
> everything else with TABs. On top of that I would use proper capitalization
> for unit, i.e. Ohms. But the latter is minor.
> 

Re: [PATCH RFC v2 2/3] pinconf-generic: clarify pull up and pull down config values

[Andy Shevchenko]

On Thu, Nov 17, 2022 at 11:53:55AM +0000, Niyas Sait wrote:
> Sorry for the newbie issues. Somehow I missed the space vs tab issue on that
> patch.

No problem.

> I will send a patch to fix the two issues.

One more newbie thing you should address in your replies is stop top-posting.

> On 17/11/2022 10:39, Andy Shevchenko wrote:
> > On Thu, Nov 17, 2022 at 12:37:44PM +0200, Andy Shevchenko wrote:
> > > On Thu, Nov 17, 2022 at 10:30:37AM +0100, Linus Walleij wrote:
> > > > On Tue, Nov 15, 2022 at 6:54 PM Niyas Sait <niyas.sait@linaro.org> wrote:
> > > > 
> > > > > PIN_CONFIG_BIAS_PULL_DOWN and PIN_CONFIG_BIAS_PULL_UP values can
> > > > > be custom or an SI unit such as ohms
> > > > > 
> > > > > Signed-off-by: Niyas Sait <niyas.sait@linaro.org>
> > > > 
> > > > This patch is good as-is so I just applied it. No need to resend with
> > > > the ACPI series.
> > > 
> > > Is it? I think it's visible that it has TAB vs. space issue...
> > 
> > To be more clear, the "such as ohms" have been indented with spaces, while
> > everything else with TABs. On top of that I would use proper capitalization
> > for unit, i.e. Ohms. But the latter is minor.

-- 
With Best Regards,
Andy Shevchenko

[PATCH RFC v2 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          |  18 +-
 drivers/pinctrl/core.h          |   3 +
 drivers/pinctrl/pinctrl-acpi.c  | 443 ++++++++++++++++++++++++++++++++
 drivers/pinctrl/pinctrl-acpi.h  |  44 ++++
 include/linux/pinctrl/pinctrl.h |  15 ++
 5 files changed, 519 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 9e57f4c62e60..00e5066c1087 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,10 @@ 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..603e36e175c7 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 cd0d4b2d8868..e337f83e6879 100644
--- a/drivers/pinctrl/pinctrl-acpi.c
+++ b/drivers/pinctrl/pinctrl-acpi.c
@@ -13,6 +13,449 @@
 #include <linux/list.h>
 
 #include "pinctrl-acpi.h"
+#include "core.h"
+
+/**
+ * struct pinctrl_acpi_map - mapping table chunk parsed from ACPI
+ * @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;
+	size_t 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 {
+		kfree(map);
+	}
+}
+
+void pinctrl_acpi_free_maps(struct pinctrl *p)
+{
+	struct pinctrl_acpi_map *acpi_map, *tmp;
+
+	list_for_each_entry_safe(acpi_map, tmp, &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 int num_maps)
+{
+	int i;
+	struct pinctrl_acpi_map *acpi_map;
+
+	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);
+	}
+
+	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;
+}
+
+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_get_acpi_dev(handle);
+	if (!controller_device)
+		return NULL;
+
+	return acpi_dev_name(controller_device);
+}
+
+static int map_acpi_conf_to_general_conf(unsigned int acpi_param,
+			unsigned int acpi_value, unsigned int *pin_config)
+{
+	enum pin_config_param genconf_param;
+
+	switch (acpi_param) {
+	case ACPI_PIN_CONFIG_BIAS_PULL_UP:
+		genconf_param = PIN_CONFIG_BIAS_PULL_UP;
+		break;
+	case ACPI_PIN_CONFIG_BIAS_PULL_DOWN:
+		genconf_param = PIN_CONFIG_BIAS_PULL_DOWN;
+		break;
+	case ACPI_PIN_CONFIG_BIAS_DEFAULT:
+		genconf_param = PIN_CONFIG_BIAS_PULL_PIN_DEFAULT;
+		break;
+	case ACPI_PIN_CONFIG_BIAS_DISABLE:
+		genconf_param = PIN_CONFIG_BIAS_DISABLE;
+		break;
+	case ACPI_PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
+		genconf_param = PIN_CONFIG_BIAS_HIGH_IMPEDANCE;
+		break;
+	case ACPI_PIN_CONFIG_BIAS_BUS_HOLD:
+		genconf_param = PIN_CONFIG_BIAS_BUS_HOLD;
+		break;
+	case ACPI_PIN_CONFIG_DRIVE_OPEN_DRAIN:
+		genconf_param = PIN_CONFIG_DRIVE_OPEN_DRAIN;
+		break;
+	case ACPI_PIN_CONFIG_DRIVE_OPEN_SOURCE:
+		genconf_param = PIN_CONFIG_DRIVE_OPEN_SOURCE;
+		break;
+	case ACPI_PIN_CONFIG_DRIVE_PUSH_PULL:
+		genconf_param = PIN_CONFIG_DRIVE_PUSH_PULL;
+		break;
+	case ACPI_PIN_CONFIG_DRIVE_STRENGTH:
+		genconf_param = PIN_CONFIG_DRIVE_STRENGTH;
+		break;
+	case ACPI_PIN_CONFIG_SLEW_RATE:
+		genconf_param = PIN_CONFIG_SLEW_RATE;
+		break;
+	case ACPI_PIN_CONFIG_INPUT_DEBOUNCE:
+		genconf_param = PIN_CONFIG_INPUT_DEBOUNCE;
+		break;
+	case ACPI_PIN_CONFIG_INPUT_SCHMITT_TRIGGER:
+		genconf_param = PIN_CONFIG_INPUT_SCHMITT_ENABLE;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	*pin_config = pinconf_to_config_packed(genconf_param, acpi_value);
+
+	return 0;
+}
+
+/**
+ * struct pinctrl_acpi_controller_map - internal structure to group pin resources
+ * @pinctrl_dev: pin controller ACPI name
+ * @list: list head for the map
+ * @pin_group_maps: list head for the pin/group maps
+ */
+struct pinctrl_acpi_controller_map {
+	char *pinctrl_dev;
+	struct list_head list;
+	struct list_head pin_group_maps;
+};
+
+static int add_pin_group_node(struct list_head *acpi_map_head,
+							char *pinctrl_dev,
+							char *group,
+							unsigned int pin,
+							bool is_config,
+							unsigned int config_func,
+							void *vendor_data,
+							unsigned int vendor_length)
+{
+	struct pinctrl_acpi_controller_map *acpi_controller_map = NULL;
+	struct pinctrl_acpi_controller_map *acpi_controller_map_iter;
+	struct pinctrl_acpi_pin_group_map *pin_group_map = NULL;
+	struct pinctrl_acpi_pin_group_map *pin_group_map_iter;
+	struct pinctrl_acpi_pin_group_info *info;
+	bool group_pin_match;
+
+	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 (!acpi_controller_map) {
+		acpi_controller_map = kzalloc(sizeof(*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);
+	}
+
+	list_for_each_entry(pin_group_map_iter, &acpi_controller_map->pin_group_maps, list) {
+		if (group)
+			group_pin_match = pin_group_map_iter->group &&
+					!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);
+		list_add(&pin_group_map->list, &acpi_controller_map->pin_group_maps);
+	}
+
+	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;
+	info->vendor_length = vendor_length;
+	INIT_LIST_HEAD(&info->list);
+	list_add(&info->list, &pin_group_map->info_list);
+
+	return 0;
+}
+
+static int populate_pin_function(struct list_head *acpi_map_head,
+		struct acpi_resource_pin_function *ares_pin_function)
+{
+	int i;
+	int ret;
+	unsigned int config;
+
+	ret = map_acpi_conf_to_general_conf(ares_pin_function->pin_config, 1, &config);
+	if (ret < 0)
+		return ret;
+
+	for (i = 0; i < ares_pin_function->pin_table_length; i++) {
+		ret = add_pin_group_node(acpi_map_head,
+				ares_pin_function->resource_source.string_ptr,
+				NULL,
+				ares_pin_function->pin_table[i],
+				false,
+				ares_pin_function->function_number,
+				ares_pin_function->vendor_data,
+				ares_pin_function->vendor_length);
+		if (ret < 0)
+			return ret;
+
+		ret = add_pin_group_node(acpi_map_head,
+				ares_pin_function->resource_source.string_ptr,
+				NULL,
+				ares_pin_function->pin_table[i],
+				true,
+				config,
+				ares_pin_function->vendor_data,
+				ares_pin_function->vendor_length);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 1;
+}
+
+static int populate_pin_config(struct list_head *acpi_map_head,
+						struct acpi_resource_pin_config *ares_pin_config)
+{
+	int i;
+	int ret;
+	unsigned int config;
+
+	ret = map_acpi_conf_to_general_conf(ares_pin_config->pin_config_type,
+					ares_pin_config->pin_config_value, &config);
+	if (ret < 0)
+		return ret;
+
+	for (i = 0; i < ares_pin_config->pin_table_length; i++) {
+		ret = add_pin_group_node(acpi_map_head,
+				ares_pin_config->resource_source.string_ptr,
+				NULL,
+				ares_pin_config->pin_table[i],
+				true,
+				config,
+				ares_pin_config->vendor_data,
+				ares_pin_config->vendor_length);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 1;
+}
+
+static int populate_pin_group_function(struct list_head *acpi_map_head,
+		struct acpi_resource_pin_group_function *ares_pin_group_function)
+{
+	int ret;
+
+	ret = add_pin_group_node(acpi_map_head,
+			ares_pin_group_function->resource_source.string_ptr,
+			ares_pin_group_function->resource_source_label.string_ptr,
+			0,
+			false,
+			ares_pin_group_function->function_number,
+			ares_pin_group_function->vendor_data,
+			ares_pin_group_function->vendor_length);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int populate_pin_group_config(struct list_head *acpi_map_head,
+		struct acpi_resource_pin_group_config *ares_pin_group_config)
+{
+	unsigned int config;
+	int ret;
+
+	ret = map_acpi_conf_to_general_conf(
+			ares_pin_group_config->pin_config_type,
+			ares_pin_group_config->pin_config_value,
+			&config);
+	if (ret < 0)
+		return ret;
+
+	ret = add_pin_group_node(acpi_map_head,
+			ares_pin_group_config->resource_source.string_ptr,
+			ares_pin_group_config->resource_source_label.string_ptr,
+			0,
+			true,
+			config,
+			ares_pin_group_config->vendor_data,
+			ares_pin_group_config->vendor_length);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int pinctrl_acpi_populate_pin_group_map(struct acpi_resource *ares, void *data)
+{
+	struct list_head *acpi_map_head = data;
+
+	switch (ares->type) {
+	case ACPI_RESOURCE_TYPE_PIN_FUNCTION:
+		return populate_pin_function(acpi_map_head, &ares->data.pin_function);
+	case ACPI_RESOURCE_TYPE_PIN_CONFIG:
+		return populate_pin_config(acpi_map_head, &ares->data.pin_config);
+	case ACPI_RESOURCE_TYPE_PIN_GROUP_FUNCTION:
+		return populate_pin_group_function(acpi_map_head, &ares->data.pin_group_function);
+	case ACPI_RESOURCE_TYPE_PIN_GROUP_CONFIG:
+		return populate_pin_group_config(acpi_map_head, &ares->data.pin_group_config);
+	default:
+		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;
+}
+
+/**
+ * pinctrl_acpi_to_map() - pinctrl map from ACPI pin resources for device
+ * @p: pinctrl descriptor for the device
+ *
+ * This will parse the ACPI pin resources for the device and creates
+ * pinctrl map. This functions handles PinFunction, PinFunctionConfig,
+ * PinGroupFunction and PinGroupConfig resources.
+ */
+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;
+
+	INIT_LIST_HEAD(&pin_group_list);
+	ret = pinctrl_acpi_get_pin_group_map(adev, &pin_group_list);
+	if (ret < 0)
+		return ret;
+
+	list_for_each_entry(controller_map, &pin_group_list, list) {
+		const char *pctldev_name = acpi_node_to_device_name(controller_map->pinctrl_dev);
+
+		pctldev = get_pinctrl_dev_from_devname(pctldev_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",
+				pctldev_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 e3a6b61bea90..1b097edb43a8 100644
--- a/drivers/pinctrl/pinctrl-acpi.h
+++ b/drivers/pinctrl/pinctrl-acpi.h
@@ -23,12 +23,56 @@ struct pinctrl_acpi_group_desc {
 	struct list_head list;
 };
 
+/**
+ * struct pinctrl_acpi_pin_group_map - pin/group to config/functions map
+ * @group: name of the pin group. @group is NULL for pin types
+ * @pin: pin number. @pin is valid only if @group is NULL
+ * @is_config: set if @info contains config values
+ * @info_list: list of config or function for the pin/group
+ * @list: list head for the map
+ */
+struct pinctrl_acpi_pin_group_map {
+	const char *group;
+	unsigned int pin;
+	bool is_config;
+	struct list_head info_list;
+	struct list_head list;
+};
+
+/**
+ * struct pinctrl_acpi_pin_group_info - config or function to apply
+ * @config_func: packed config value or function number
+ * @vendor_data: vendor data from ACPI resource
+ * @vendor_length: length of vendor data
+ * @list: list head for the descriptor
+ */
+struct pinctrl_acpi_pin_group_info {
+	unsigned int config_func;
+	u8 *vendor_data;
+	unsigned int 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);
+
+int pinctrl_acpi_to_map(struct pinctrl *p);
+
+void pinctrl_acpi_free_maps(struct pinctrl *p);
 #else
 static inline int pinctrl_acpi_get_pin_groups(struct acpi_device *adev,
 			struct list_head *group_desc_list)
 {
 	return -ENXIO;
 }
+
+static inline int pinctrl_acpi_to_map(struct pinctrl *p)
+{
+	return -ENXIO;
+}
+
+static inline void pinctrl_acpi_free_maps(struct pinctrl *p)
+{
+
+}
 #endif
diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
index 487117ccb1bc..13d43a186df9 100644
--- a/include/linux/pinctrl/pinctrl.h
+++ b/include/linux/pinctrl/pinctrl.h
@@ -104,6 +104,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 @acpi_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);
@@ -120,6 +129,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 v2 3/3] pinctrl: add support for ACPI pin function and config resources

[Mika Westerberg]

On Tue, Nov 15, 2022 at 05:54:15PM +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
> 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          |  18 +-
>  drivers/pinctrl/core.h          |   3 +
>  drivers/pinctrl/pinctrl-acpi.c  | 443 ++++++++++++++++++++++++++++++++
>  drivers/pinctrl/pinctrl-acpi.h  |  44 ++++
>  include/linux/pinctrl/pinctrl.h |  15 ++
>  5 files changed, 519 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> index 9e57f4c62e60..00e5066c1087 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)) {

It looks better of you reverse the branches (and use
has_acpi_companion()):

if (has_acpi_companion(dev)) {
	INIT_LIST_HEAD(&p->acpi_maps);
	ret = pinctrl_acpi_to_map(p);
} else {
	INIT_LIST_HEAD(&p->dt_maps);
	ret = pinctrl_dt_to_map(p, pctldev);
}

> +		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,10 @@ 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);

DItto here.

>  
>  	if (inlist)
>  		list_del(&p->node);
> diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
> index 840103c40c14..603e36e175c7 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 cd0d4b2d8868..e337f83e6879 100644
> --- a/drivers/pinctrl/pinctrl-acpi.c
> +++ b/drivers/pinctrl/pinctrl-acpi.c
> @@ -13,6 +13,449 @@
>  #include <linux/list.h>
>  
>  #include "pinctrl-acpi.h"
> +#include "core.h"
> +
> +/**
> + * struct pinctrl_acpi_map - mapping table chunk parsed from ACPI
> + * @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;
> +	size_t num_maps;

Perhaps nmaps consistent with npins in the previous patch?

> +};
> +
> +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);
> +

Drop this empty line

> +	} else {
> +		kfree(map);
> +	}
> +}
> +

Kernel-doc?

> +void pinctrl_acpi_free_maps(struct pinctrl *p)
> +{
> +	struct pinctrl_acpi_map *acpi_map, *tmp;
> +
> +	list_for_each_entry_safe(acpi_map, tmp, &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 int num_maps)
> +{
> +	int i;
> +	struct pinctrl_acpi_map *acpi_map;

"reverse christmas tree" looks better but up to you:

	struct pinctrl_acpi_map *acpi_map;
	int i;

Ditto everywhere

> +
> +	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);
> +	}
> +
> +	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);

Empty line

> +	return -ENOMEM;
> +}
> +
> +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_get_acpi_dev(handle);
> +	if (!controller_device)
> +		return NULL;
> +
> +	return acpi_dev_name(controller_device);
> +}
> +
> +static int map_acpi_conf_to_general_conf(unsigned int acpi_param,
> +			unsigned int acpi_value, unsigned int *pin_config)
> +{
> +	enum pin_config_param genconf_param;
> +
> +	switch (acpi_param) {
> +	case ACPI_PIN_CONFIG_BIAS_PULL_UP:
> +		genconf_param = PIN_CONFIG_BIAS_PULL_UP;
> +		break;
> +	case ACPI_PIN_CONFIG_BIAS_PULL_DOWN:
> +		genconf_param = PIN_CONFIG_BIAS_PULL_DOWN;
> +		break;
> +	case ACPI_PIN_CONFIG_BIAS_DEFAULT:
> +		genconf_param = PIN_CONFIG_BIAS_PULL_PIN_DEFAULT;
> +		break;
> +	case ACPI_PIN_CONFIG_BIAS_DISABLE:
> +		genconf_param = PIN_CONFIG_BIAS_DISABLE;
> +		break;
> +	case ACPI_PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
> +		genconf_param = PIN_CONFIG_BIAS_HIGH_IMPEDANCE;
> +		break;
> +	case ACPI_PIN_CONFIG_BIAS_BUS_HOLD:
> +		genconf_param = PIN_CONFIG_BIAS_BUS_HOLD;
> +		break;
> +	case ACPI_PIN_CONFIG_DRIVE_OPEN_DRAIN:
> +		genconf_param = PIN_CONFIG_DRIVE_OPEN_DRAIN;
> +		break;
> +	case ACPI_PIN_CONFIG_DRIVE_OPEN_SOURCE:
> +		genconf_param = PIN_CONFIG_DRIVE_OPEN_SOURCE;
> +		break;
> +	case ACPI_PIN_CONFIG_DRIVE_PUSH_PULL:
> +		genconf_param = PIN_CONFIG_DRIVE_PUSH_PULL;
> +		break;
> +	case ACPI_PIN_CONFIG_DRIVE_STRENGTH:
> +		genconf_param = PIN_CONFIG_DRIVE_STRENGTH;
> +		break;
> +	case ACPI_PIN_CONFIG_SLEW_RATE:
> +		genconf_param = PIN_CONFIG_SLEW_RATE;
> +		break;
> +	case ACPI_PIN_CONFIG_INPUT_DEBOUNCE:
> +		genconf_param = PIN_CONFIG_INPUT_DEBOUNCE;
> +		break;
> +	case ACPI_PIN_CONFIG_INPUT_SCHMITT_TRIGGER:
> +		genconf_param = PIN_CONFIG_INPUT_SCHMITT_ENABLE;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	*pin_config = pinconf_to_config_packed(genconf_param, acpi_value);
> +
> +	return 0;
> +}
> +
> +/**
> + * struct pinctrl_acpi_controller_map - internal structure to group pin resources
> + * @pinctrl_dev: pin controller ACPI name
> + * @list: list head for the map
> + * @pin_group_maps: list head for the pin/group maps
> + */
> +struct pinctrl_acpi_controller_map {
> +	char *pinctrl_dev;
> +	struct list_head list;
> +	struct list_head pin_group_maps;
> +};
> +
> +static int add_pin_group_node(struct list_head *acpi_map_head,
> +							char *pinctrl_dev,
> +							char *group,
> +							unsigned int pin,
> +							bool is_config,
> +							unsigned int config_func,
> +							void *vendor_data,
> +							unsigned int vendor_length)

This takes quite many parameters, perhaps you can use a structure or
helper functions to make it look nicer? Also consider formatting:

static int add_pin_group_node(struct list_head *acpi_map_head, char *pinctrl_dev,
			      char *group, unsigned int pin, bool is_config,
			      unsigned int config_func, void *vendor_data,
			      unsigned int vendor_length)

> +{
> +	struct pinctrl_acpi_controller_map *acpi_controller_map = NULL;
> +	struct pinctrl_acpi_controller_map *acpi_controller_map_iter;
> +	struct pinctrl_acpi_pin_group_map *pin_group_map = NULL;
> +	struct pinctrl_acpi_pin_group_map *pin_group_map_iter;
> +	struct pinctrl_acpi_pin_group_info *info;
> +	bool group_pin_match;
> +
> +	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 (!acpi_controller_map) {
> +		acpi_controller_map = kzalloc(sizeof(*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);
> +	}
> +
> +	list_for_each_entry(pin_group_map_iter, &acpi_controller_map->pin_group_maps, list) {
> +		if (group)
> +			group_pin_match = pin_group_map_iter->group &&
> +					!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)

Did you just leak acpi_controller_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);
> +		list_add(&pin_group_map->list, &acpi_controller_map->pin_group_maps);
> +	}
> +
> +	info = kzalloc(sizeof(struct pinctrl_acpi_pin_group_info), GFP_KERNEL);
> +	if (!info)

Does this leak acpi_controller_map and pin_group_map?

> +		return -ENOMEM;
> +
> +	info->config_func = config_func;
> +	info->vendor_data = vendor_data;
> +	info->vendor_length = vendor_length;
> +	INIT_LIST_HEAD(&info->list);
> +	list_add(&info->list, &pin_group_map->info_list);
> +
> +	return 0;
> +}
> +
> +static int populate_pin_function(struct list_head *acpi_map_head,
> +		struct acpi_resource_pin_function *ares_pin_function)
> +{
> +	int i;
> +	int ret;
> +	unsigned int config;
> +
> +	ret = map_acpi_conf_to_general_conf(ares_pin_function->pin_config, 1, &config);
> +	if (ret < 0)
> +		return ret;
> +
> +	for (i = 0; i < ares_pin_function->pin_table_length; i++) {
> +		ret = add_pin_group_node(acpi_map_head,
> +				ares_pin_function->resource_source.string_ptr,
> +				NULL,
> +				ares_pin_function->pin_table[i],
> +				false,
> +				ares_pin_function->function_number,
> +				ares_pin_function->vendor_data,
> +				ares_pin_function->vendor_length);

If you pass here:

		ret = add_pin_group_node(acpi_map_head, ares_pin_function, NULL, false);

looks much nicer. I mean can't you pass "ares_pin_function" as is?


> +		if (ret < 0)
> +			return ret;
> +
> +		ret = add_pin_group_node(acpi_map_head,
> +				ares_pin_function->resource_source.string_ptr,
> +				NULL,
> +				ares_pin_function->pin_table[i],
> +				true,
> +				config,
> +				ares_pin_function->vendor_data,
> +				ares_pin_function->vendor_length);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 1;
> +}
> +
> +static int populate_pin_config(struct list_head *acpi_map_head,
> +						struct acpi_resource_pin_config *ares_pin_config)
> +{
> +	int i;
> +	int ret;
> +	unsigned int config;
> +
> +	ret = map_acpi_conf_to_general_conf(ares_pin_config->pin_config_type,
> +					ares_pin_config->pin_config_value, &config);
> +	if (ret < 0)
> +		return ret;
> +
> +	for (i = 0; i < ares_pin_config->pin_table_length; i++) {
> +		ret = add_pin_group_node(acpi_map_head,
> +				ares_pin_config->resource_source.string_ptr,
> +				NULL,
> +				ares_pin_config->pin_table[i],
> +				true,
> +				config,
> +				ares_pin_config->vendor_data,
> +				ares_pin_config->vendor_length);

Same here

> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 1;
> +}
> +
> +static int populate_pin_group_function(struct list_head *acpi_map_head,
> +		struct acpi_resource_pin_group_function *ares_pin_group_function)
> +{
> +	int ret;
> +
> +	ret = add_pin_group_node(acpi_map_head,
> +			ares_pin_group_function->resource_source.string_ptr,
> +			ares_pin_group_function->resource_source_label.string_ptr,
> +			0,
> +			false,
> +			ares_pin_group_function->function_number,
> +			ares_pin_group_function->vendor_data,
> +			ares_pin_group_function->vendor_length);

And here. Okay the type is different but since they all have the same
common "header" it should be OK, I think.

> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int populate_pin_group_config(struct list_head *acpi_map_head,
> +		struct acpi_resource_pin_group_config *ares_pin_group_config)
> +{
> +	unsigned int config;
> +	int ret;
> +
> +	ret = map_acpi_conf_to_general_conf(
> +			ares_pin_group_config->pin_config_type,
> +			ares_pin_group_config->pin_config_value,
> +			&config);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = add_pin_group_node(acpi_map_head,
> +			ares_pin_group_config->resource_source.string_ptr,
> +			ares_pin_group_config->resource_source_label.string_ptr,
> +			0,
> +			true,
> +			config,
> +			ares_pin_group_config->vendor_data,
> +			ares_pin_group_config->vendor_length);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int pinctrl_acpi_populate_pin_group_map(struct acpi_resource *ares, void *data)
> +{
> +	struct list_head *acpi_map_head = data;
> +
> +	switch (ares->type) {
> +	case ACPI_RESOURCE_TYPE_PIN_FUNCTION:
> +		return populate_pin_function(acpi_map_head, &ares->data.pin_function);
> +	case ACPI_RESOURCE_TYPE_PIN_CONFIG:
> +		return populate_pin_config(acpi_map_head, &ares->data.pin_config);
> +	case ACPI_RESOURCE_TYPE_PIN_GROUP_FUNCTION:
> +		return populate_pin_group_function(acpi_map_head, &ares->data.pin_group_function);
> +	case ACPI_RESOURCE_TYPE_PIN_GROUP_CONFIG:
> +		return populate_pin_group_config(acpi_map_head, &ares->data.pin_group_config);
> +	default:
> +		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);

Still weird formatting. Didn't checkpatch.pl complain about this?

> +	acpi_dev_free_resource_list(&res_list);
> +
> +	return ret;
> +}
> +
> +/**
> + * pinctrl_acpi_to_map() - pinctrl map from ACPI pin resources for device
> + * @p: pinctrl descriptor for the device
> + *
> + * This will parse the ACPI pin resources for the device and creates
> + * pinctrl map. This functions handles PinFunction, PinFunctionConfig,
> + * PinGroupFunction and PinGroupConfig resources.
> + */
> +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;
> +
> +	INIT_LIST_HEAD(&pin_group_list);
> +	ret = pinctrl_acpi_get_pin_group_map(adev, &pin_group_list);
> +	if (ret < 0)
> +		return ret;
> +
> +	list_for_each_entry(controller_map, &pin_group_list, list) {
> +		const char *pctldev_name = acpi_node_to_device_name(controller_map->pinctrl_dev);
> +
> +		pctldev = get_pinctrl_dev_from_devname(pctldev_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) {

!num_maps

> +			dev_info(p->dev, "there is not valid maps for pin controller %s\n",
> +				pctldev_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 e3a6b61bea90..1b097edb43a8 100644
> --- a/drivers/pinctrl/pinctrl-acpi.h
> +++ b/drivers/pinctrl/pinctrl-acpi.h
> @@ -23,12 +23,56 @@ struct pinctrl_acpi_group_desc {
>  	struct list_head list;
>  };
>  
> +/**
> + * struct pinctrl_acpi_pin_group_map - pin/group to config/functions map
> + * @group: name of the pin group. @group is NULL for pin types
> + * @pin: pin number. @pin is valid only if @group is NULL
> + * @is_config: set if @info contains config values
> + * @info_list: list of config or function for the pin/group
> + * @list: list head for the map
> + */
> +struct pinctrl_acpi_pin_group_map {
> +	const char *group;
> +	unsigned int pin;
> +	bool is_config;
> +	struct list_head info_list;
> +	struct list_head list;
> +};
> +
> +/**
> + * struct pinctrl_acpi_pin_group_info - config or function to apply
> + * @config_func: packed config value or function number
> + * @vendor_data: vendor data from ACPI resource
> + * @vendor_length: length of vendor data
> + * @list: list head for the descriptor
> + */
> +struct pinctrl_acpi_pin_group_info {
> +	unsigned int config_func;
> +	u8 *vendor_data;
> +	unsigned int 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);
> +

Drop the empty line

> +int pinctrl_acpi_to_map(struct pinctrl *p);
> +

Ditto

> +void pinctrl_acpi_free_maps(struct pinctrl *p);
>  #else
>  static inline int pinctrl_acpi_get_pin_groups(struct acpi_device *adev,
>  			struct list_head *group_desc_list)
>  {
>  	return -ENXIO;
>  }
> +
> +static inline int pinctrl_acpi_to_map(struct pinctrl *p)
> +{
> +	return -ENXIO;
> +}
> +
> +static inline void pinctrl_acpi_free_maps(struct pinctrl *p)
> +{
> +
> +}
>  #endif
> diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
> index 487117ccb1bc..13d43a186df9 100644
> --- a/include/linux/pinctrl/pinctrl.h
> +++ b/include/linux/pinctrl/pinctrl.h
> @@ -104,6 +104,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 @acpi_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);
> @@ -120,6 +129,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 v2 0/3] pinctrl: add ACPI support to pin controller

[Andy Shevchenko]

On Tue, Nov 15, 2022 at 05:54:12PM +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.

Cover letter doesn't point to the ASL code you are using.

I have some questions about that:
1) why do you need specific _DSD() for the pin mappings?
2) wgy you need vendor data for some of Pin*() resources?


-- 
With Best Regards,
Andy Shevchenko

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

[Niyas Sait]

 > Cover letter doesn't point to the ASL code you are using.

I've built this patch with a prototype ACPI implementation for NXP 
I.MX8MP platform.

You can see the code here: 
https://gitlab.com/nsait-linaro/acpi-patches/-/blob/master/0001-add-acpi-pinctrl-support-for-i2c-controllers.patch.

 > 1) why do you need specific _DSD() for the pin mappings?

_DSD is required to pass the GPIO number to Pin mapping. Is there a 
better way to do it ?

 > 2) wgy you need vendor data for some of Pin*() resources?

Vendor data contains platform specific mux, config and input selection 
register for the pin group.

--
Niyas

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

[Andy Shevchenko]

On Tue, Nov 15, 2022 at 06:29:28PM +0000, Niyas Sait wrote:
> > Cover letter doesn't point to the ASL code you are using.
> 
> I've built this patch with a prototype ACPI implementation for NXP I.MX8MP
> platform.
> 
> You can see the code here: https://gitlab.com/nsait-linaro/acpi-patches/-/blob/master/0001-add-acpi-pinctrl-support-for-i2c-controllers.patch.

Yes, my point that you have to create a cover letter

> > 1) why do you need specific _DSD() for the pin mappings?
> 
> _DSD is required to pass the GPIO number to Pin mapping. Is there a better
> way to do it ?

Don't you have your pins sequential? Doesn't driver know the pin list?
Where are the device tree bindings for those properties? Are they already
present there?

> > 2) wgy you need vendor data for some of Pin*() resources?
> 
> Vendor data contains platform specific mux, config and input selection
> register for the pin group.

Why can't this be in the driver?

...

On top of that, how other hardware can utilize what you are doing without
adding redundancy to the ACPI?

-- 
With Best Regards,
Andy Shevchenko

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

[Niyas Sait]

 >>> 1) why do you need specific _DSD() for the pin mappings?
 >>
 >> _DSD is required to pass the GPIO number to Pin mapping. Is there a 
better
 >> way to do it ?
 >
 > Don't you have your pins sequential? Doesn't driver know the pin list?
 > Where are the device tree bindings for those properties? Are they already
 > present there?

They are not sequential. There are multiple GPIO controllers and GPIO 
pins are mapped to different pin ranges.

Please checkout arch/arm64/boot/dts/freescale/imx8mp.dtsi for device 
tree binding.

Yes I think it can be done in the driver.

 >>> 2) wgy you need vendor data for some of Pin*() resources?
 >>
 >> Vendor data contains platform specific mux, config and input selection
 >> register for the pin group.
 >
 > Why can't this be in the driver?
 >

Yes I think it can be done in driver. But I think we still have to pass 
the vendor data from the ACPI resources to the drivers in case if any 
implementation wants to pass vendor specific data for the driver.

--
Niyas

On 15/11/2022 18:47, Andy Shevchenko wrote:
> On Tue, Nov 15, 2022 at 06:29:28PM +0000, Niyas Sait wrote:
>>> Cover letter doesn't point to the ASL code you are using.
>>
>> I've built this patch with a prototype ACPI implementation for NXP I.MX8MP
>> platform.
>>
>> You can see the code here: https://gitlab.com/nsait-linaro/acpi-patches/-/blob/master/0001-add-acpi-pinctrl-support-for-i2c-controllers.patch.
> 
> Yes, my point that you have to create a cover letter
> 
>>> 1) why do you need specific _DSD() for the pin mappings?
>>
>> _DSD is required to pass the GPIO number to Pin mapping. Is there a better
>> way to do it ?
> 
> Don't you have your pins sequential? Doesn't driver know the pin list?
> Where are the device tree bindings for those properties? Are they already
> present there?
> 
>>> 2) wgy you need vendor data for some of Pin*() resources?
>>
>> Vendor data contains platform specific mux, config and input selection
>> register for the pin group.
> 
> Why can't this be in the driver?
> 
> ...
> 
> On top of that, how other hardware can utilize what you are doing without
> adding redundancy to the ACPI?
> 

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

[Andy Shevchenko]

On Tue, Nov 15, 2022 at 07:15:25PM +0000, Niyas Sait wrote:
> >>> 1) why do you need specific _DSD() for the pin mappings?
> >>
> >> _DSD is required to pass the GPIO number to Pin mapping. Is there a
> better
> >> way to do it ?
> >
> > Don't you have your pins sequential? Doesn't driver know the pin list?
> > Where are the device tree bindings for those properties? Are they already
> > present there?
> 
> They are not sequential. There are multiple GPIO controllers and GPIO pins
> are mapped to different pin ranges.
> 
> Please checkout arch/arm64/boot/dts/freescale/imx8mp.dtsi for device tree
> binding.
> 
> Yes I think it can be done in the driver.
> 
> >>> 2) wgy you need vendor data for some of Pin*() resources?
> >>
> >> Vendor data contains platform specific mux, config and input selection
> >> register for the pin group.
> >
> > Why can't this be in the driver?
> 
> Yes I think it can be done in driver. But I think we still have to pass the
> vendor data from the ACPI resources to the drivers in case if any
> implementation wants to pass vendor specific data for the driver.

I'm afraid that opening this to the drivers will allow to a lot of unknown data
to be encoded in ACPI, so I would really like to see the vendor data to be
absent (ideally).

> On 15/11/2022 18:47, Andy Shevchenko wrote:
> > On Tue, Nov 15, 2022 at 06:29:28PM +0000, Niyas Sait wrote:
> > > > Cover letter doesn't point to the ASL code you are using.
> > > 
> > > I've built this patch with a prototype ACPI implementation for NXP I.MX8MP
> > > platform.
> > > 
> > > You can see the code here: https://gitlab.com/nsait-linaro/acpi-patches/-/blob/master/0001-add-acpi-pinctrl-support-for-i2c-controllers.patch.
> > 
> > Yes, my point that you have to create a cover letter
> > 
> > > > 1) why do you need specific _DSD() for the pin mappings?
> > > 
> > > _DSD is required to pass the GPIO number to Pin mapping. Is there a better
> > > way to do it ?
> > 
> > Don't you have your pins sequential? Doesn't driver know the pin list?
> > Where are the device tree bindings for those properties? Are they already
> > present there?
> > 
> > > > 2) wgy you need vendor data for some of Pin*() resources?
> > > 
> > > Vendor data contains platform specific mux, config and input selection
> > > register for the pin group.
> > 
> > Why can't this be in the driver?
> > 
> > ...
> > 
> > On top of that, how other hardware can utilize what you are doing without
> > adding redundancy to the ACPI?

-- 
With Best Regards,
Andy Shevchenko

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

[Niyas Sait]

On 16/11/2022 10:16, Andy Shevchenko wrote:
> I'm afraid that opening this to the drivers will allow to a lot of unknown data
> to be encoded in ACPI, so I would really like to see the vendor data to be
> absent (ideally).

Ok. I can rework the interface to drop the vendor data and length.

--
Niyas

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

[Niyas Sait]

On 15/11/2022 18:47, Andy Shevchenko wrote:
> On top of that, how other hardware can utilize what you are doing without
> adding redundancy to the ACPI?

Sorry I missed this question.

I am not sure if there is any redundancy to ACPI spec.

ACPI pin resources gives you most of the bits you need to describe the 
pin dependencies and configuration required for the device.

For my particular implementation (not part of this proposal), I had to 
pass extra data to the driver to describe the GPIO number to Pin mapping 
and conf and mux registers. But that can be potentially fully handled in 
the driver. Sorry for the repetition.

Let me know if I missed your question :-)


--
Niyas

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

[Andy Shevchenko]

On Wed, Nov 16, 2022 at 09:50:05AM +0000, Niyas Sait wrote:
> On 15/11/2022 18:47, Andy Shevchenko wrote:
> > On top of that, how other hardware can utilize what you are doing without
> > adding redundancy to the ACPI?
> 
> Sorry I missed this question.
> 
> I am not sure if there is any redundancy to ACPI spec.
> 
> ACPI pin resources gives you most of the bits you need to describe the pin
> dependencies and configuration required for the device.
> 
> For my particular implementation (not part of this proposal), I had to pass
> extra data to the driver to describe the GPIO number to Pin mapping and conf
> and mux registers. But that can be potentially fully handled in the driver.
> Sorry for the repetition.

The GPIO to pin mapping is done by GPIO ranges. This can be made up in the
driver or, IIRC, in DT (so in ACPI) by already registered properties).

Not sure, why you don't use them.

> Let me know if I missed your question :-)

-- 
With Best Regards,
Andy Shevchenko

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

[Niyas Sait]

On 16/11/2022 09:56, Andy Shevchenko wrote:
> The GPIO to pin mapping is done by GPIO ranges. This can be made up in the
> driver or, IIRC, in DT (so in ACPI) by already registered properties).
> 
> Not sure, why you don't use them.

OK. I might have missed it.

In DT, gpio-ranges property is used to describe the mapping, I could not 
find equivalent property in GPIO ACPI properties - 
https://www.kernel.org/doc/html/latest/firmware-guide/acpi/gpio-properties.html

-- 
Niyas

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

[Andy Shevchenko]

On Wed, Nov 16, 2022 at 10:40:19AM +0000, Niyas Sait wrote:
> On 16/11/2022 09:56, Andy Shevchenko wrote:
> > The GPIO to pin mapping is done by GPIO ranges. This can be made up in the
> > driver or, IIRC, in DT (so in ACPI) by already registered properties).
> > 
> > Not sure, why you don't use them.
> 
> OK. I might have missed it.
> 
> In DT, gpio-ranges property is used to describe the mapping, I could not
> find equivalent property in GPIO ACPI properties -
> https://www.kernel.org/doc/html/latest/firmware-guide/acpi/gpio-properties.html

It's the same. You just need to check the code of GPIO library to make sure
that it uses fwnode / device property APIs, so it will be parsed in ACPI case
as well.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Wed, Nov 16, 2022 at 03:27:30PM +0200, Andy Shevchenko wrote:
> On Wed, Nov 16, 2022 at 10:40:19AM +0000, Niyas Sait wrote:
> > On 16/11/2022 09:56, Andy Shevchenko wrote:
> > > The GPIO to pin mapping is done by GPIO ranges. This can be made up in the
> > > driver or, IIRC, in DT (so in ACPI) by already registered properties).
> > > 
> > > Not sure, why you don't use them.
> > 
> > OK. I might have missed it.
> > 
> > In DT, gpio-ranges property is used to describe the mapping, I could not
> > find equivalent property in GPIO ACPI properties -
> > https://www.kernel.org/doc/html/latest/firmware-guide/acpi/gpio-properties.html
> 
> It's the same. You just need to check the code of GPIO library to make sure
> that it uses fwnode / device property APIs, so it will be parsed in ACPI case
> as well.

Ah, and it's always welcome to have Documentation updates to make it up-to-date.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, Nov 15, 2022 at 05:54:12PM +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.

I have just published some old stuff I have [1] for targeting same feature.
Maybe you can find something interesting there.

What I have noticed that your series misses documentation part and it's
hard to see the design behind it.

[1]: https://bitbucket.org/andy-shev/linux/commits/branch/topic%2Feds-acpi-pinctrl

-- 
With Best Regards,
Andy Shevchenko