| Message ID | 20221115175415.650690-2-niyas.sait@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: add ACPI support to pin controller | expand |
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
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).
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
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