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

[Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>]

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@xxxxxxxxxx>
> ---
>  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