[PATCH 1/3] Input: soc_button_array - use resources to set up IRQ info for gpio-keys

[PATCH 1/3] Input: soc_button_array - use resources to set up IRQ info for gpio-keys

[Dmitry Torokhov]

In preparation for transitioning the driver to use static device
properties instead of platform data to configure gpio-keys driver,
switch to using IRQ resources attached to the gpio-keys platform device
to communicate the IRQ selection.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/soc_button_array.c | 33 +++++++++++++++++++++------
 1 file changed, 26 insertions(+), 7 deletions(-)

diff --git a/drivers/input/misc/soc_button_array.c b/drivers/input/misc/soc_button_array.c
index dbf86ea778e6..d4dc4e7116a7 100644
--- a/drivers/input/misc/soc_button_array.c
+++ b/drivers/input/misc/soc_button_array.c
@@ -17,6 +17,7 @@
 #include <linux/gpio_keys.h>
 #include <linux/platform_data/x86/soc.h>
 #include <linux/platform_device.h>
+#include <linux/slab.h>
 
 static bool use_low_level_irq;
 module_param(use_low_level_irq, bool, 0444);
@@ -154,6 +155,11 @@ soc_button_device_create(struct platform_device *pdev,
 			 const struct soc_button_info *button_info,
 			 bool autorepeat)
 {
+	struct platform_device_info pdevinfo = {
+		.parent = &pdev->dev,
+		.name = "gpio-keys",
+		.id = PLATFORM_DEVID_AUTO,
+	};
 	const struct soc_button_info *info;
 	struct platform_device *pd;
 	struct gpio_keys_button *gpio_keys;
@@ -167,6 +173,15 @@ soc_button_device_create(struct platform_device *pdev,
 		if (info->autorepeat == autorepeat)
 			n_buttons++;
 
+	struct resource *irq_resources __free(kfree) = NULL;
+	/* See dmi_use_low_level_irq[] comment */
+	if (!autorepeat && (use_low_level_irq ||
+			    dmi_check_system(dmi_use_low_level_irq))) {
+		irq_resources = kzalloc_objs(*irq_resources, n_buttons);
+		if (!irq_resources)
+			return ERR_PTR(-ENOMEM);
+	}
+
 	gpio_keys_pdata = devm_kzalloc(&pdev->dev,
 				       sizeof(*gpio_keys_pdata) +
 					sizeof(*gpio_keys) * n_buttons,
@@ -174,6 +189,9 @@ soc_button_device_create(struct platform_device *pdev,
 	if (!gpio_keys_pdata)
 		return ERR_PTR(-ENOMEM);
 
+	pdevinfo.data = gpio_keys_pdata;
+	pdevinfo.size_data = sizeof(*gpio_keys_pdata);
+
 	gpio_keys = (void *)(gpio_keys_pdata + 1);
 	n_buttons = 0;
 
@@ -212,10 +230,9 @@ soc_button_device_create(struct platform_device *pdev,
 		}
 
 		/* See dmi_use_low_level_irq[] comment */
-		if (!autorepeat && (use_low_level_irq ||
-				    dmi_check_system(dmi_use_low_level_irq))) {
+		if (irq_resources) {
 			irq_set_irq_type(irq, IRQ_TYPE_LEVEL_LOW);
-			gpio_keys[n_buttons].irq = irq;
+			irq_resources[n_buttons] = DEFINE_RES_IRQ(irq);
 			gpio_keys[n_buttons].gpio = -ENOENT;
 		} else {
 			gpio_keys[n_buttons].gpio = gpio;
@@ -240,10 +257,12 @@ soc_button_device_create(struct platform_device *pdev,
 	gpio_keys_pdata->nbuttons = n_buttons;
 	gpio_keys_pdata->rep = autorepeat;
 
-	pd = platform_device_register_resndata(&pdev->dev, "gpio-keys",
-					       PLATFORM_DEVID_AUTO, NULL, 0,
-					       gpio_keys_pdata,
-					       sizeof(*gpio_keys_pdata));
+	if (irq_resources) {
+		pdevinfo.res = irq_resources;
+		pdevinfo.num_res = n_buttons;
+	}
+
+	pd = platform_device_register_full(&pdevinfo);
 	error = PTR_ERR_OR_ZERO(pd);
 	if (error) {
 		dev_err(&pdev->dev,
-- 
2.56.0.212.g129ede20d9-goog

[PATCH 2/3] Input: soc_button_array - use __free() cleanup facilities to free memory

[Dmitry Torokhov]

Switch to using __free(kfree) cleanup facilities instead of doing manual
cleanup or abusing devm for allocations that are only needed during
probing.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/soc_button_array.c | 52 ++++++++++++---------------
 1 file changed, 23 insertions(+), 29 deletions(-)

diff --git a/drivers/input/misc/soc_button_array.c b/drivers/input/misc/soc_button_array.c
index d4dc4e7116a7..8d782ef4f78b 100644
--- a/drivers/input/misc/soc_button_array.c
+++ b/drivers/input/misc/soc_button_array.c
@@ -366,10 +366,10 @@ static const u8 btns_desc_uuid[16] = {
 static struct soc_button_info *soc_button_get_button_info(struct device *dev)
 {
 	struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER };
-	const union acpi_object *desc, *el0, *uuid, *btns_desc = NULL;
-	struct soc_button_info *button_info;
+	const union acpi_object *el0, *uuid, *btns_desc = NULL;
 	acpi_status status;
 	int i, btn, collection_uid = -1;
+	int error;
 
 	status = acpi_evaluate_object_typed(ACPI_HANDLE(dev), "_DSD", NULL,
 					    &buf, ACPI_TYPE_PACKAGE);
@@ -378,8 +378,9 @@ static struct soc_button_info *soc_button_get_button_info(struct device *dev)
 		return ERR_PTR(-ENODEV);
 	}
 
+	const union acpi_object *desc __free(kfree) = buf.pointer;
+
 	/* Look for the Button Descriptors UUID */
-	desc = buf.pointer;
 	for (i = 0; (i + 1) < desc->package.count; i += 2) {
 		uuid = &desc->package.elements[i];
 
@@ -397,8 +398,7 @@ static struct soc_button_info *soc_button_get_button_info(struct device *dev)
 
 	if (!btns_desc || !btns_desc->package.count) {
 		dev_err(dev, "ACPI Button Descriptors not found\n");
-		button_info = ERR_PTR(-ENODEV);
-		goto out;
+		return ERR_PTR(-ENODEV);
 	}
 
 	/* The first package describes the collection */
@@ -414,32 +414,26 @@ static struct soc_button_info *soc_button_get_button_info(struct device *dev)
 	}
 	if (collection_uid == -1) {
 		dev_err(dev, "Invalid Button Collection Descriptor\n");
-		button_info = ERR_PTR(-ENODEV);
-		goto out;
+		return ERR_PTR(-ENODEV);
 	}
 
 	/* There are package.count - 1 buttons + 1 terminating empty entry */
-	button_info = devm_kcalloc(dev, btns_desc->package.count,
-				   sizeof(*button_info), GFP_KERNEL);
-	if (!button_info) {
-		button_info = ERR_PTR(-ENOMEM);
-		goto out;
-	}
+	struct soc_button_info *button_info __free(kfree) =
+		kzalloc_objs(*button_info, btns_desc->package.count);
+	if (!button_info)
+		return ERR_PTR(-ENOMEM);
 
 	/* Parse the button descriptors */
 	for (i = 1, btn = 0; i < btns_desc->package.count; i++, btn++) {
-		if (soc_button_parse_btn_desc(dev,
-					      &btns_desc->package.elements[i],
-					      collection_uid,
-					      &button_info[btn])) {
-			button_info = ERR_PTR(-ENODEV);
-			goto out;
-		}
+		error = soc_button_parse_btn_desc(dev,
+						  &btns_desc->package.elements[i],
+						  collection_uid,
+						  &button_info[btn]);
+		if (error)
+			return ERR_PTR(error);
 	}
 
-out:
-	kfree(buf.pointer);
-	return button_info;
+	return_ptr(button_info);
 }
 
 static void soc_button_remove(struct platform_device *pdev)
@@ -458,6 +452,7 @@ static int soc_button_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	const struct soc_device_data *device_data;
 	const struct soc_button_info *button_info;
+	const struct soc_button_info *dynamic_button_info __free(kfree) = NULL;
 	struct soc_button_data *priv;
 	struct platform_device *pd;
 	int i;
@@ -473,9 +468,11 @@ static int soc_button_probe(struct platform_device *pdev)
 	if (device_data && device_data->button_info) {
 		button_info = device_data->button_info;
 	} else {
-		button_info = soc_button_get_button_info(dev);
-		if (IS_ERR(button_info))
-			return PTR_ERR(button_info);
+		dynamic_button_info = soc_button_get_button_info(dev);
+		if (IS_ERR(dynamic_button_info))
+			return PTR_ERR(dynamic_button_info);
+
+		button_info = dynamic_button_info;
 	}
 
 	error = gpiod_count(dev, NULL);
@@ -507,9 +504,6 @@ static int soc_button_probe(struct platform_device *pdev)
 	if (!priv->children[0] && !priv->children[1])
 		return -ENODEV;
 
-	if (!device_data || !device_data->button_info)
-		devm_kfree(dev, button_info);
-
 	return 0;
 }
 
-- 
2.56.0.212.g129ede20d9-goog

[PATCH 3/3] Input: soc_button_array - convert to software nodes

[Dmitry Torokhov]

Convert the driver from using legacy platform data to using software
nodes for registering 'gpio-keys' devices. This allows us to
transition away from platform data and use generic device properties.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/Kconfig            |   1 -
 drivers/input/misc/soc_button_array.c | 134 ++++++++++++++++++--------
 2 files changed, 92 insertions(+), 43 deletions(-)

diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 169768e3d5bb..a2984c1048f3 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -906,7 +906,6 @@ config INPUT_SOC_BUTTON_ARRAY
 	depends on KEYBOARD_GPIO && ACPI
 	depends on X86
 	depends on GPIOLIB
-	select GPIOLIB_LEGACY
 	help
 	  Say Y here if you have a SoC-based tablet that originally runs
 	  Windows 8 or a Microsoft Surface Book 2, Pro 5, Laptop 1 or later.
diff --git a/drivers/input/misc/soc_button_array.c b/drivers/input/misc/soc_button_array.c
index 8d782ef4f78b..aa7838b864c2 100644
--- a/drivers/input/misc/soc_button_array.c
+++ b/drivers/input/misc/soc_button_array.c
@@ -14,9 +14,11 @@
 #include <linux/acpi.h>
 #include <linux/dmi.h>
 #include <linux/gpio/consumer.h>
-#include <linux/gpio_keys.h>
+#include <linux/gpio/driver.h>
+#include <linux/gpio/machine.h>
 #include <linux/platform_data/x86/soc.h>
 #include <linux/platform_device.h>
+#include <linux/property.h>
 #include <linux/slab.h>
 
 static bool use_low_level_irq;
@@ -59,10 +61,10 @@ struct soc_button_data {
  * gets called with the lid open (e.g. on boot and on resume), it sets the
  * irq-type to IRQ_TYPE_LEVEL_LOW. Where as the gpio-keys driver programs the
  * type to, and expects it to be, IRQ_TYPE_EDGE_BOTH.
- * To work around this we don't set gpio_keys_button.gpio on these 2-in-1s,
+ * To work around this we don't set "gpios" property on these 2-in-1s,
  * instead we get the irq for the GPIO ourselves, configure it as
  * IRQ_TYPE_LEVEL_LOW (to match how the _LID AML code configures it) and pass
- * the irq in gpio_keys_button.irq. Below is a list of affected devices.
+ * the irq as a device resource. Below is a list of affected devices.
  */
 static const struct dmi_system_id dmi_use_low_level_irq[] = {
 	{
@@ -131,18 +133,31 @@ static const struct dmi_system_id dmi_invalid_acpi_index[] = {
 };
 
 /*
- * Get the Nth GPIO number from the ACPI object.
+ * Get the Nth GPIO information from the ACPI object.
  */
 static int soc_button_lookup_gpio(struct device *dev, int acpi_index,
-				  int *gpio_ret, int *irq_ret)
+				  bool active_low,
+				  struct software_node_ref_args *gpio_ref,
+				  int *irq_ret)
 {
+	struct gpio_device *gdev;
+	struct fwnode_handle *fwnode;
 	struct gpio_desc *desc;
 
 	desc = gpiod_get_index(dev, NULL, acpi_index, GPIOD_ASIS);
 	if (IS_ERR(desc))
 		return PTR_ERR(desc);
 
-	*gpio_ret = desc_to_gpio(desc);
+	gdev = gpiod_to_gpio_device(desc);
+	fwnode = dev_fwnode(gpio_device_to_device(gdev));
+	if (!fwnode) {
+		gpiod_put(desc);
+		return -ENODEV;
+	}
+
+	*gpio_ref = SOFTWARE_NODE_REFERENCE(fwnode, gpiod_hwgpio(desc),
+					    active_low ? GPIO_ACTIVE_LOW :
+							 GPIO_ACTIVE_HIGH);
 	*irq_ret = gpiod_to_irq(desc);
 
 	gpiod_put(desc);
@@ -150,11 +165,28 @@ static int soc_button_lookup_gpio(struct device *dev, int acpi_index,
 	return 0;
 }
 
+static void soc_button_remove_swnodes(struct fwnode_handle *parent)
+{
+	struct fwnode_handle *child;
+
+	if (IS_ERR_OR_NULL(parent))
+		return;
+
+	fwnode_for_each_child_node(parent, child)
+		fwnode_remove_software_node(child);
+
+	fwnode_remove_software_node(parent);
+}
+
 static struct platform_device *
 soc_button_device_create(struct platform_device *pdev,
 			 const struct soc_button_info *button_info,
 			 bool autorepeat)
 {
+	const struct property_entry parent_props[] = {
+		PROPERTY_ENTRY_BOOL("autorepeat"),
+		{ }
+	};
 	struct platform_device_info pdevinfo = {
 		.parent = &pdev->dev,
 		.name = "gpio-keys",
@@ -162,17 +194,19 @@ soc_button_device_create(struct platform_device *pdev,
 	};
 	const struct soc_button_info *info;
 	struct platform_device *pd;
-	struct gpio_keys_button *gpio_keys;
-	struct gpio_keys_platform_data *gpio_keys_pdata;
+	struct fwnode_handle *parent_node;
 	const struct dmi_system_id *dmi_id;
 	int invalid_acpi_index = -1;
-	int error, gpio, irq = 0;
+	int error, irq = 0;
 	int n_buttons = 0;
 
 	for (info = button_info; info->name; info++)
 		if (info->autorepeat == autorepeat)
 			n_buttons++;
 
+	if (n_buttons == 0)
+		return ERR_PTR(-ENODEV);
+
 	struct resource *irq_resources __free(kfree) = NULL;
 	/* See dmi_use_low_level_irq[] comment */
 	if (!autorepeat && (use_low_level_irq ||
@@ -182,17 +216,10 @@ soc_button_device_create(struct platform_device *pdev,
 			return ERR_PTR(-ENOMEM);
 	}
 
-	gpio_keys_pdata = devm_kzalloc(&pdev->dev,
-				       sizeof(*gpio_keys_pdata) +
-					sizeof(*gpio_keys) * n_buttons,
-				       GFP_KERNEL);
-	if (!gpio_keys_pdata)
-		return ERR_PTR(-ENOMEM);
-
-	pdevinfo.data = gpio_keys_pdata;
-	pdevinfo.size_data = sizeof(*gpio_keys_pdata);
+	parent_node = fwnode_create_software_node(autorepeat ? parent_props : NULL, NULL);
+	if (IS_ERR(parent_node))
+		return ERR_CAST(parent_node);
 
-	gpio_keys = (void *)(gpio_keys_pdata + 1);
 	n_buttons = 0;
 
 	dmi_id = dmi_first_match(dmi_invalid_acpi_index);
@@ -200,13 +227,19 @@ soc_button_device_create(struct platform_device *pdev,
 		invalid_acpi_index = (long)dmi_id->driver_data;
 
 	for (info = button_info; info->name; info++) {
+		struct software_node_ref_args gpio_ref;
+		struct property_entry child_props[7] = { };
+		struct fwnode_handle *child_node;
+		int prop_idx = 0;
+
 		if (info->autorepeat != autorepeat)
 			continue;
 
 		if (info->acpi_index == invalid_acpi_index)
 			continue;
 
-		error = soc_button_lookup_gpio(&pdev->dev, info->acpi_index, &gpio, &irq);
+		error = soc_button_lookup_gpio(&pdev->dev, info->acpi_index,
+					       info->active_low, &gpio_ref, &irq);
 		if (error || irq < 0) {
 			/*
 			 * Propagate -EPROBE_DEFER, skip button on other errors.
@@ -223,57 +256,68 @@ soc_button_device_create(struct platform_device *pdev,
 			if ((error == -EPROBE_DEFER || irq == -EPROBE_DEFER) &&
 			    !(soc_intel_is_byt() || soc_intel_is_cht())) {
 				error = -EPROBE_DEFER;
-				goto err_free_mem;
+				goto err_remove_swnodes;
 			}
 
 			continue;
 		}
 
+		child_props[prop_idx++] = PROPERTY_ENTRY_U32("linux,code", info->event_code);
+		child_props[prop_idx++] = PROPERTY_ENTRY_STRING("label", info->name);
+
+		if (info->event_type != EV_KEY)
+			child_props[prop_idx++] =
+				PROPERTY_ENTRY_U32("linux,input-type",
+						   info->event_type);
+
+		if (info->wakeup)
+			child_props[prop_idx++] = PROPERTY_ENTRY_BOOL("wakeup-source");
+
+		/* These devices often use cheap buttons, use 50 ms debounce */
+		child_props[prop_idx++] = PROPERTY_ENTRY_U32("debounce-interval", 50);
+
 		/* See dmi_use_low_level_irq[] comment */
 		if (irq_resources) {
 			irq_set_irq_type(irq, IRQ_TYPE_LEVEL_LOW);
 			irq_resources[n_buttons] = DEFINE_RES_IRQ(irq);
-			gpio_keys[n_buttons].gpio = -ENOENT;
 		} else {
-			gpio_keys[n_buttons].gpio = gpio;
+			child_props[prop_idx++] =
+				PROPERTY_ENTRY_REF("gpios", &gpio_ref);
+		}
+
+		child_node = fwnode_create_software_node(child_props, parent_node);
+		if (IS_ERR(child_node)) {
+			error = PTR_ERR(child_node);
+			goto err_remove_swnodes;
 		}
 
-		gpio_keys[n_buttons].type = info->event_type;
-		gpio_keys[n_buttons].code = info->event_code;
-		gpio_keys[n_buttons].active_low = info->active_low;
-		gpio_keys[n_buttons].desc = info->name;
-		gpio_keys[n_buttons].wakeup = info->wakeup;
-		/* These devices often use cheap buttons, use 50 ms debounce */
-		gpio_keys[n_buttons].debounce_interval = 50;
 		n_buttons++;
 	}
 
 	if (n_buttons == 0) {
 		error = -ENODEV;
-		goto err_free_mem;
+		goto err_remove_swnodes;
 	}
 
-	gpio_keys_pdata->buttons = gpio_keys;
-	gpio_keys_pdata->nbuttons = n_buttons;
-	gpio_keys_pdata->rep = autorepeat;
-
 	if (irq_resources) {
 		pdevinfo.res = irq_resources;
 		pdevinfo.num_res = n_buttons;
 	}
 
+	pdevinfo.fwnode = parent_node;
+
 	pd = platform_device_register_full(&pdevinfo);
 	error = PTR_ERR_OR_ZERO(pd);
 	if (error) {
 		dev_err(&pdev->dev,
 			"failed registering gpio-keys: %d\n", error);
-		goto err_free_mem;
+		goto err_remove_swnodes;
 	}
 
 	return pd;
 
-err_free_mem:
-	devm_kfree(&pdev->dev, gpio_keys_pdata);
+err_remove_swnodes:
+	soc_button_remove_swnodes(parent_node);
 	return ERR_PTR(error);
 }
 
@@ -439,12 +483,18 @@ static struct soc_button_info *soc_button_get_button_info(struct device *dev)
 static void soc_button_remove(struct platform_device *pdev)
 {
 	struct soc_button_data *priv = platform_get_drvdata(pdev);
-
+	struct platform_device *pd;
+	struct fwnode_handle *fwnode;
 	int i;
 
-	for (i = 0; i < BUTTON_TYPES; i++)
-		if (priv->children[i])
-			platform_device_unregister(priv->children[i]);
+	for (i = 0; i < BUTTON_TYPES; i++) {
+		pd = priv->children[i];
+		if (pd) {
+			fwnode = dev_fwnode(&pd->dev);
+			platform_device_unregister(pd);
+			soc_button_remove_swnodes(fwnode);
+		}
+	}
 }
 
 static int soc_button_probe(struct platform_device *pdev)
-- 
2.56.0.212.g129ede20d9-goog

Re: [PATCH 1/3] Input: soc_button_array - use resources to set up IRQ info for gpio-keys

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Input: mix of __free() and goto cleanup in soc_button_device_create()
--

commit 135ab963100f947c993746f76110c2ca70438cec
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Input: soc_button_array - use resources to set up IRQ info for gpio-keys

This commit switches soc_button_device_create() to pass IRQ information to
the gpio-keys platform device via platform device IRQ resources instead of
platform data, in preparation for converting the driver to static device
properties.

> diff --git a/drivers/input/misc/soc_button_array.c b/drivers/input/misc/soc_button_array.c
> index dbf86ea778e67..d4dc4e7116a7d 100644
> --- a/drivers/input/misc/soc_button_array.c
> +++ b/drivers/input/misc/soc_button_array.c
[ ... ]
> @@ -167,6 +173,15 @@ soc_button_device_create(struct platform_device *pdev,
>  		if (info->autorepeat == autorepeat)
>  			n_buttons++;
>  
> +	struct resource *irq_resources __free(kfree) = NULL;
> +	/* See dmi_use_low_level_irq[] comment */
> +	if (!autorepeat && (use_low_level_irq ||
> +			    dmi_check_system(dmi_use_low_level_irq))) {
> +		irq_resources = kzalloc_objs(*irq_resources, n_buttons);
> +		if (!irq_resources)
> +			return ERR_PTR(-ENOMEM);
> +	}
> +

[Severity: Low]
In drivers/input/misc/soc_button_array.c, soc_button_device_create()
introduces irq_resources with __free(kfree) while still using goto-based
cleanup via err_free_mem to free gpio_keys_pdata on later failure paths:

drivers/input/misc/soc_button_array.c:soc_button_device_create() {
	...
	if (n_buttons == 0) {
		error = -ENODEV;
		goto err_free_mem;
	}
	...
	pd = platform_device_register_full(&pdevinfo);
	error = PTR_ERR_OR_ZERO(pd);
	if (error) {
		dev_err(&pdev->dev,
			"failed registering gpio-keys: %d\n", error);
		goto err_free_mem;
	}

	return pd;

err_free_mem:
	devm_kfree(&pdev->dev, gpio_keys_pdata);
	return ERR_PTR(error);
}

Although irq_resources is declared and initialized before all of the
goto err_free_mem jumps, include/linux/cleanup.h recommends against mixing
__free() scope-based cleanup and goto cleanup labels within the same
function. Later in the series (in commit "Input: soc_button_array - convert
to software nodes"), err_free_mem is replaced by goto err_remove_swnodes
while keeping irq_resources __free(kfree).

Should soc_button_device_create() avoid mixing __free(kfree) with goto
cleanup here?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021015004.1537738-1-dmitry.torokhov@gmail.com?part=1

Re: [PATCH 2/3] Input: soc_button_array - use __free() cleanup facilities to free memory

[Bartosz Golaszewski]

On Wed, 21 Oct 2026 03:50:00 +0200, Dmitry Torokhov
<dmitry.torokhov@gmail.com> said:
> Switch to using __free(kfree) cleanup facilities instead of doing manual
> cleanup or abusing devm for allocations that are only needed during
> probing.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---

Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

Re: [PATCH 3/3] Input: soc_button_array - convert to software nodes

[Bartosz Golaszewski]

On Wed, 21 Oct 2026 03:50:01 +0200, Dmitry Torokhov
<dmitry.torokhov@gmail.com> said:
> Convert the driver from using legacy platform data to using software
> nodes for registering 'gpio-keys' devices. This allows us to
> transition away from platform data and use generic device properties.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/misc/Kconfig            |   1 -
>  drivers/input/misc/soc_button_array.c | 134 ++++++++++++++++++--------
>  2 files changed, 92 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index 169768e3d5bb..a2984c1048f3 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -906,7 +906,6 @@ config INPUT_SOC_BUTTON_ARRAY
>  	depends on KEYBOARD_GPIO && ACPI
>  	depends on X86
>  	depends on GPIOLIB
> -	select GPIOLIB_LEGACY
>  	help
>  	  Say Y here if you have a SoC-based tablet that originally runs
>  	  Windows 8 or a Microsoft Surface Book 2, Pro 5, Laptop 1 or later.
> diff --git a/drivers/input/misc/soc_button_array.c b/drivers/input/misc/soc_button_array.c
> index 8d782ef4f78b..aa7838b864c2 100644
> --- a/drivers/input/misc/soc_button_array.c
> +++ b/drivers/input/misc/soc_button_array.c
> @@ -14,9 +14,11 @@
>  #include <linux/acpi.h>
>  #include <linux/dmi.h>
>  #include <linux/gpio/consumer.h>
> -#include <linux/gpio_keys.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/gpio/machine.h>
>  #include <linux/platform_data/x86/soc.h>
>  #include <linux/platform_device.h>
> +#include <linux/property.h>
>  #include <linux/slab.h>
>
>  static bool use_low_level_irq;
> @@ -59,10 +61,10 @@ struct soc_button_data {
>   * gets called with the lid open (e.g. on boot and on resume), it sets the
>   * irq-type to IRQ_TYPE_LEVEL_LOW. Where as the gpio-keys driver programs the
>   * type to, and expects it to be, IRQ_TYPE_EDGE_BOTH.
> - * To work around this we don't set gpio_keys_button.gpio on these 2-in-1s,
> + * To work around this we don't set "gpios" property on these 2-in-1s,
>   * instead we get the irq for the GPIO ourselves, configure it as
>   * IRQ_TYPE_LEVEL_LOW (to match how the _LID AML code configures it) and pass
> - * the irq in gpio_keys_button.irq. Below is a list of affected devices.
> + * the irq as a device resource. Below is a list of affected devices.
>   */
>  static const struct dmi_system_id dmi_use_low_level_irq[] = {
>  	{
> @@ -131,18 +133,31 @@ static const struct dmi_system_id dmi_invalid_acpi_index[] = {
>  };
>
>  /*
> - * Get the Nth GPIO number from the ACPI object.
> + * Get the Nth GPIO information from the ACPI object.
>   */
>  static int soc_button_lookup_gpio(struct device *dev, int acpi_index,
> -				  int *gpio_ret, int *irq_ret)
> +				  bool active_low,
> +				  struct software_node_ref_args *gpio_ref,
> +				  int *irq_ret)
>  {
> +	struct gpio_device *gdev;
> +	struct fwnode_handle *fwnode;
>  	struct gpio_desc *desc;
>
>  	desc = gpiod_get_index(dev, NULL, acpi_index, GPIOD_ASIS);
>  	if (IS_ERR(desc))
>  		return PTR_ERR(desc);

This GPIO is requested for a short time and then released. Do we run into
a risk of the actual target user requesting this pin at the same time and
failing? We don't support shared GPIO pins for ACPI yet (though I want to
add that eventually).

I know this has always been like this but I'm wondering how it works exactly.

>
> -	*gpio_ret = desc_to_gpio(desc);
> +	gdev = gpiod_to_gpio_device(desc);
> +	fwnode = dev_fwnode(gpio_device_to_device(gdev));
> +	if (!fwnode) {
> +		gpiod_put(desc);
> +		return -ENODEV;
> +	}
> +
> +	*gpio_ref = SOFTWARE_NODE_REFERENCE(fwnode, gpiod_hwgpio(desc),
> +					    active_low ? GPIO_ACTIVE_LOW :
> +							 GPIO_ACTIVE_HIGH);

Sigh... I hate it and it looks hacky but I also don't have a better idea. :(

Bart

Re: [PATCH 3/3] Input: soc_button_array - convert to software nodes

[Dmitry Torokhov]

On Wed, Oct 21, 2026 at 05:11:32AM -0500, Bartosz Golaszewski wrote:
> On Wed, 21 Oct 2026 03:50:01 +0200, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> said:
> > Convert the driver from using legacy platform data to using software
> > nodes for registering 'gpio-keys' devices. This allows us to
> > transition away from platform data and use generic device properties.
> >
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> >  drivers/input/misc/Kconfig            |   1 -
> >  drivers/input/misc/soc_button_array.c | 134 ++++++++++++++++++--------
> >  2 files changed, 92 insertions(+), 43 deletions(-)
> >
> > diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> > index 169768e3d5bb..a2984c1048f3 100644
> > --- a/drivers/input/misc/Kconfig
> > +++ b/drivers/input/misc/Kconfig
> > @@ -906,7 +906,6 @@ config INPUT_SOC_BUTTON_ARRAY
> >  	depends on KEYBOARD_GPIO && ACPI
> >  	depends on X86
> >  	depends on GPIOLIB
> > -	select GPIOLIB_LEGACY
> >  	help
> >  	  Say Y here if you have a SoC-based tablet that originally runs
> >  	  Windows 8 or a Microsoft Surface Book 2, Pro 5, Laptop 1 or later.
> > diff --git a/drivers/input/misc/soc_button_array.c b/drivers/input/misc/soc_button_array.c
> > index 8d782ef4f78b..aa7838b864c2 100644
> > --- a/drivers/input/misc/soc_button_array.c
> > +++ b/drivers/input/misc/soc_button_array.c
> > @@ -14,9 +14,11 @@
> >  #include <linux/acpi.h>
> >  #include <linux/dmi.h>
> >  #include <linux/gpio/consumer.h>
> > -#include <linux/gpio_keys.h>
> > +#include <linux/gpio/driver.h>
> > +#include <linux/gpio/machine.h>
> >  #include <linux/platform_data/x86/soc.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/property.h>
> >  #include <linux/slab.h>
> >
> >  static bool use_low_level_irq;
> > @@ -59,10 +61,10 @@ struct soc_button_data {
> >   * gets called with the lid open (e.g. on boot and on resume), it sets the
> >   * irq-type to IRQ_TYPE_LEVEL_LOW. Where as the gpio-keys driver programs the
> >   * type to, and expects it to be, IRQ_TYPE_EDGE_BOTH.
> > - * To work around this we don't set gpio_keys_button.gpio on these 2-in-1s,
> > + * To work around this we don't set "gpios" property on these 2-in-1s,
> >   * instead we get the irq for the GPIO ourselves, configure it as
> >   * IRQ_TYPE_LEVEL_LOW (to match how the _LID AML code configures it) and pass
> > - * the irq in gpio_keys_button.irq. Below is a list of affected devices.
> > + * the irq as a device resource. Below is a list of affected devices.
> >   */
> >  static const struct dmi_system_id dmi_use_low_level_irq[] = {
> >  	{
> > @@ -131,18 +133,31 @@ static const struct dmi_system_id dmi_invalid_acpi_index[] = {
> >  };
> >
> >  /*
> > - * Get the Nth GPIO number from the ACPI object.
> > + * Get the Nth GPIO information from the ACPI object.
> >   */
> >  static int soc_button_lookup_gpio(struct device *dev, int acpi_index,
> > -				  int *gpio_ret, int *irq_ret)
> > +				  bool active_low,
> > +				  struct software_node_ref_args *gpio_ref,
> > +				  int *irq_ret)
> >  {
> > +	struct gpio_device *gdev;
> > +	struct fwnode_handle *fwnode;
> >  	struct gpio_desc *desc;
> >
> >  	desc = gpiod_get_index(dev, NULL, acpi_index, GPIOD_ASIS);
> >  	if (IS_ERR(desc))
> >  		return PTR_ERR(desc);
> 
> This GPIO is requested for a short time and then released. Do we run into
> a risk of the actual target user requesting this pin at the same time and
> failing? We don't support shared GPIO pins for ACPI yet (though I want to
> add that eventually).
> 
> I know this has always been like this but I'm wondering how it works exactly.

The soc_button_array driver controls instantiation of devices that
consume these GPIOs, so there is no concern of them being consumed too
early.

> 
> >
> > -	*gpio_ret = desc_to_gpio(desc);
> > +	gdev = gpiod_to_gpio_device(desc);
> > +	fwnode = dev_fwnode(gpio_device_to_device(gdev));
> > +	if (!fwnode) {
> > +		gpiod_put(desc);
> > +		return -ENODEV;
> > +	}
> > +
> > +	*gpio_ref = SOFTWARE_NODE_REFERENCE(fwnode, gpiod_hwgpio(desc),
> > +					    active_low ? GPIO_ACTIVE_LOW :
> > +							 GPIO_ACTIVE_HIGH);
> 
> Sigh... I hate it and it looks hacky but I also don't have a better idea. :(

Thanks, that is quite common reaction to the code I write ;)

-- 
Dmitry

Re: [PATCH 3/3] Input: soc_button_array - convert to software nodes

[Arnd Bergmann]

On Wed, Oct 21, 2026, at 18:56, Dmitry Torokhov wrote:
> On Wed, Oct 21, 2026 at 05:11:32AM -0500, Bartosz Golaszewski wrote:
>> >
>> > -	*gpio_ret = desc_to_gpio(desc);
>> > +	gdev = gpiod_to_gpio_device(desc);
>> > +	fwnode = dev_fwnode(gpio_device_to_device(gdev));
>> > +	if (!fwnode) {
>> > +		gpiod_put(desc);
>> > +		return -ENODEV;
>> > +	}
>> > +
>> > +	*gpio_ref = SOFTWARE_NODE_REFERENCE(fwnode, gpiod_hwgpio(desc),
>> > +					    active_low ? GPIO_ACTIVE_LOW :
>> > +							 GPIO_ACTIVE_HIGH);
>> 
>> Sigh... I hate it and it looks hacky but I also don't have a better idea. :(
>
> Thanks, that is quite common reaction to the code I write ;)

When I looked at this one in the past, the only other idea I had was to
completely separate it from the regular gpio_keys driver and just make
it a separate input driver. I think that would make this driver a bit
cleaner, but it would also require a bit of code duplication and I
can't tell if that is worth it.

      Arnd

Re: [PATCH 2/3] Input: soc_button_array - use __free() cleanup facilities to free memory

[Andy Shevchenko]

On Tue, Oct 20, 2026 at 06:50:00PM -0700, Dmitry Torokhov wrote:
> Switch to using __free(kfree) cleanup facilities instead of doing manual
> cleanup or abusing devm for allocations that are only needed during
> probing.

...

> +	const union acpi_object *desc __free(kfree) = buf.pointer;

Rafael added 'acpi_object_free' for __free() (pun wasn't intended :-).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3] Input: soc_button_array - convert to software nodes

[Andy Shevchenko]

On Tue, Oct 20, 2026 at 06:50:01PM -0700, Dmitry Torokhov wrote:
> Convert the driver from using legacy platform data to using software
> nodes for registering 'gpio-keys' devices. This allows us to
> transition away from platform data and use generic device properties.

...

> +static void soc_button_remove_swnodes(struct fwnode_handle *parent)
> +{
> +	struct fwnode_handle *child;

> +	if (IS_ERR_OR_NULL(parent))
> +		return;

Isn't this redundant? Because the following has it inside already.

> +	fwnode_for_each_child_node(parent, child)
> +		fwnode_remove_software_node(child);
> +
> +	fwnode_remove_software_node(parent);
> +}

...

>  	const struct soc_button_info *info;
>  	struct platform_device *pd;
> -	struct gpio_keys_button *gpio_keys;
> -	struct gpio_keys_platform_data *gpio_keys_pdata;
> +	struct fwnode_handle *parent_node;

Can it be one line down, so we keep reversed xmas tree order?

>  	const struct dmi_system_id *dmi_id;
>  	int invalid_acpi_index = -1;
> -	int error, gpio, irq = 0;
> +	int error, irq = 0;
>  	int n_buttons = 0;

...

> +		child_props[prop_idx++] = PROPERTY_ENTRY_U32("linux,code", info->event_code);
> +		child_props[prop_idx++] = PROPERTY_ENTRY_STRING("label", info->name);
> +
> +		if (info->event_type != EV_KEY)
> +			child_props[prop_idx++] =

> +				PROPERTY_ENTRY_U32("linux,input-type",
> +						   info->event_type);

Make it a single line, you seem use 100 limit anyway.

> +
> +		if (info->wakeup)
> +			child_props[prop_idx++] = PROPERTY_ENTRY_BOOL("wakeup-source");
> +
> +		/* These devices often use cheap buttons, use 50 ms debounce */
> +		child_props[prop_idx++] = PROPERTY_ENTRY_U32("debounce-interval", 50);

...

>  static void soc_button_remove(struct platform_device *pdev)
>  {
>  	struct soc_button_data *priv = platform_get_drvdata(pdev);
> -
> +	struct platform_device *pd;
> +	struct fwnode_handle *fwnode;
>  	int i;
>  
> -	for (i = 0; i < BUTTON_TYPES; i++)
> -		if (priv->children[i])
> -			platform_device_unregister(priv->children[i]);
> +	for (i = 0; i < BUTTON_TYPES; i++) {

At the same time this can be transformed to

	for (int i = 0; i < BUTTON_TYPES; i++) {

> +		pd = priv->children[i];
> +		if (pd) {

		if (!pd)
			continue;

?

> +			fwnode = dev_fwnode(&pd->dev);
> +			platform_device_unregister(pd);
> +			soc_button_remove_swnodes(fwnode);
> +		}
> +	}
>  }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] Input: soc_button_array - use __free() cleanup facilities to free memory

[Dmitry Torokhov]

On Wed, Oct 21, 2026 at 08:05:37PM +0300, Andy Shevchenko wrote:
> On Tue, Oct 20, 2026 at 06:50:00PM -0700, Dmitry Torokhov wrote:
> > Switch to using __free(kfree) cleanup facilities instead of doing manual
> > cleanup or abusing devm for allocations that are only needed during
> > probing.
> 
> ...
> 
> > +	const union acpi_object *desc __free(kfree) = buf.pointer;
> 
> Rafael added 'acpi_object_free' for __free() (pun wasn't intended :-).

Not sure why it is better... It goes through ACPI_FREE() ->
acpi_os_free() wrappers which might be nice for ACPICA code, but has
limited utility in the rest of the kernel code...

Thanks.

-- 
Dmitry

Re: [PATCH 3/3] Input: soc_button_array - convert to software nodes

[Dmitry Torokhov]

On Wed, Oct 21, 2026 at 07:02:32PM +0200, Arnd Bergmann wrote:
> On Wed, Oct 21, 2026, at 18:56, Dmitry Torokhov wrote:
> > On Wed, Oct 21, 2026 at 05:11:32AM -0500, Bartosz Golaszewski wrote:
> >> >
> >> > -	*gpio_ret = desc_to_gpio(desc);
> >> > +	gdev = gpiod_to_gpio_device(desc);
> >> > +	fwnode = dev_fwnode(gpio_device_to_device(gdev));
> >> > +	if (!fwnode) {
> >> > +		gpiod_put(desc);
> >> > +		return -ENODEV;
> >> > +	}
> >> > +
> >> > +	*gpio_ref = SOFTWARE_NODE_REFERENCE(fwnode, gpiod_hwgpio(desc),
> >> > +					    active_low ? GPIO_ACTIVE_LOW :
> >> > +							 GPIO_ACTIVE_HIGH);
> >> 
> >> Sigh... I hate it and it looks hacky but I also don't have a better idea. :(
> >
> > Thanks, that is quite common reaction to the code I write ;)
> 
> When I looked at this one in the past, the only other idea I had was to
> completely separate it from the regular gpio_keys driver and just make
> it a separate input driver. I think that would make this driver a bit
> cleaner, but it would also require a bit of code duplication and I
> can't tell if that is worth it.

Yes, I had the same idea and tried doing this. And pretty much ended
duplicating gpio-keys in soc-button-array...

Thanks.

-- 
Dmitry

Re: [PATCH 3/3] Input: soc_button_array - convert to software nodes

[Dmitry Torokhov]

On Wed, Oct 21, 2026 at 08:13:08PM +0300, Andy Shevchenko wrote:
> On Tue, Oct 20, 2026 at 06:50:01PM -0700, Dmitry Torokhov wrote:
> > Convert the driver from using legacy platform data to using software
> > nodes for registering 'gpio-keys' devices. This allows us to
> > transition away from platform data and use generic device properties.
> 
> ...
> 
> > +static void soc_button_remove_swnodes(struct fwnode_handle *parent)
> > +{
> > +	struct fwnode_handle *child;
> 
> > +	if (IS_ERR_OR_NULL(parent))
> > +		return;
> 
> Isn't this redundant? Because the following has it inside already.

Maybe but it really is not obvious whether the latest flavor of some API
handles error pointers or NULLs transparently or not and whether it will
continue doing this.

This is not a hot path so I can think we can spare an extra check.


> 
> > +	fwnode_for_each_child_node(parent, child)
> > +		fwnode_remove_software_node(child);
> > +
> > +	fwnode_remove_software_node(parent);
> > +}
> 
> ...
> 
> >  	const struct soc_button_info *info;
> >  	struct platform_device *pd;
> > -	struct gpio_keys_button *gpio_keys;
> > -	struct gpio_keys_platform_data *gpio_keys_pdata;
> > +	struct fwnode_handle *parent_node;
> 
> Can it be one line down, so we keep reversed xmas tree order?

Sure.

> 
> >  	const struct dmi_system_id *dmi_id;
> >  	int invalid_acpi_index = -1;
> > -	int error, gpio, irq = 0;
> > +	int error, irq = 0;
> >  	int n_buttons = 0;
> 
> ...
> 
> > +		child_props[prop_idx++] = PROPERTY_ENTRY_U32("linux,code", info->event_code);
> > +		child_props[prop_idx++] = PROPERTY_ENTRY_STRING("label", info->name);
> > +
> > +		if (info->event_type != EV_KEY)
> > +			child_props[prop_idx++] =
> 
> > +				PROPERTY_ENTRY_U32("linux,input-type",
> > +						   info->event_type);
> 
> Make it a single line, you seem use 100 limit anyway.

OK.

> 
> > +
> > +		if (info->wakeup)
> > +			child_props[prop_idx++] = PROPERTY_ENTRY_BOOL("wakeup-source");
> > +
> > +		/* These devices often use cheap buttons, use 50 ms debounce */
> > +		child_props[prop_idx++] = PROPERTY_ENTRY_U32("debounce-interval", 50);
> 
> ...
> 
> >  static void soc_button_remove(struct platform_device *pdev)
> >  {
> >  	struct soc_button_data *priv = platform_get_drvdata(pdev);
> > -
> > +	struct platform_device *pd;
> > +	struct fwnode_handle *fwnode;
> >  	int i;
> >  
> > -	for (i = 0; i < BUTTON_TYPES; i++)
> > -		if (priv->children[i])
> > -			platform_device_unregister(priv->children[i]);
> > +	for (i = 0; i < BUTTON_TYPES; i++) {
> 
> At the same time this can be transformed to
> 
> 	for (int i = 0; i < BUTTON_TYPES; i++) {
> 
> > +		pd = priv->children[i];
> > +		if (pd) {
> 
> 		if (!pd)
> 			continue;
> 
> ?

Sure, why not...

Thanks.

-- 
Dmitry

Re: [PATCH 2/3] Input: soc_button_array - use __free() cleanup facilities to free memory

[Andy Shevchenko]

On Wed, Oct 21, 2026 at 10:15:43AM -0700, Dmitry Torokhov wrote:
> On Wed, Oct 21, 2026 at 08:05:37PM +0300, Andy Shevchenko wrote:
> > On Tue, Oct 20, 2026 at 06:50:00PM -0700, Dmitry Torokhov wrote:

...

> > > +	const union acpi_object *desc __free(kfree) = buf.pointer;
> > 
> > Rafael added 'acpi_object_free' for __free() (pun wasn't intended :-).
> 
> Not sure why it is better... It goes through ACPI_FREE() ->
> acpi_os_free() wrappers which might be nice for ACPICA code, but has
> limited utility in the rest of the kernel code...

Today it might be similar, tomorrow it might be different. ACPI objects should
be freed with their proper APIs at least for the sake of consistency, no?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] Input: soc_button_array - use __free() cleanup facilities to free memory

[Dmitry Torokhov]

On Wed, Oct 21, 2026 at 08:43:02PM +0300, Andy Shevchenko wrote:
> On Wed, Oct 21, 2026 at 10:15:43AM -0700, Dmitry Torokhov wrote:
> > On Wed, Oct 21, 2026 at 08:05:37PM +0300, Andy Shevchenko wrote:
> > > On Tue, Oct 20, 2026 at 06:50:00PM -0700, Dmitry Torokhov wrote:
> 
> ...
> 
> > > > +	const union acpi_object *desc __free(kfree) = buf.pointer;
> > > 
> > > Rafael added 'acpi_object_free' for __free() (pun wasn't intended :-).
> > 
> > Not sure why it is better... It goes through ACPI_FREE() ->
> > acpi_os_free() wrappers which might be nice for ACPICA code, but has
> > limited utility in the rest of the kernel code...
> 
> Today it might be similar, tomorrow it might be different. ACPI objects should
> be freed with their proper APIs at least for the sake of consistency, no?

Fair enough. I'll add a todo to switch once it hits mainline.

Thanks.

-- 
Dmitry

Re: [PATCH 2/3] Input: soc_button_array - use __free() cleanup facilities to free memory

[Andy Shevchenko]

On Wed, Oct 21, 2026 at 10:53:06AM -0700, Dmitry Torokhov wrote:
> On Wed, Oct 21, 2026 at 08:43:02PM +0300, Andy Shevchenko wrote:
> > On Wed, Oct 21, 2026 at 10:15:43AM -0700, Dmitry Torokhov wrote:
> > > On Wed, Oct 21, 2026 at 08:05:37PM +0300, Andy Shevchenko wrote:
> > > > On Tue, Oct 20, 2026 at 06:50:00PM -0700, Dmitry Torokhov wrote:

...

> > > > > +	const union acpi_object *desc __free(kfree) = buf.pointer;
> > > > 
> > > > Rafael added 'acpi_object_free' for __free() (pun wasn't intended :-).
> > > 
> > > Not sure why it is better... It goes through ACPI_FREE() ->
> > > acpi_os_free() wrappers which might be nice for ACPICA code, but has
> > > limited utility in the rest of the kernel code...
> > 
> > Today it might be similar, tomorrow it might be different. ACPI objects should
> > be freed with their proper APIs at least for the sake of consistency, no?
> 
> Fair enough. I'll add a todo to switch once it hits mainline.

I believe it's pending for v7.4-rc1. Are you planning to push this series
to upstream before that one is out?

-- 
With Best Regards,
Andy Shevchenko