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

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

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