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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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