Re: [PATCH v2 2/2] mfd: ucb1x00: Convert Assabet gpio-keys to use software nodes

[Lee Jones <lee@kernel.org>]

On Mon, 06 Jul 2026, Dmitry Torokhov wrote:

> Convert the legacy gpio-keys platform device on the StrongARM SA-1100
> Assabet evaluation board to use software nodes and device properties.
> This allows describing the buttons and their GPIO bindings via software
> nodes so that platform data support can eventually be removed from the
> gpio-keys driver.
> 
> Define static software nodes for the gpio-keys device and the six button
> child nodes at file scope using relative pin indexing on the UCB1x00 GPIO
> controller node. In ucb1x00_assabet_add(), register the software node
> group and use platform_device_register_full() to register the device.
> 
> Assisted-by: Antigravity:gemini-3.5-flash
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/mfd/ucb1x00-assabet.c | 124 +++++++++++++++++++++++++++++++++---------
>  1 file changed, 98 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/mfd/ucb1x00-assabet.c b/drivers/mfd/ucb1x00-assabet.c
> index 6a389737c615..ee49ac779d1a 100644
> --- a/drivers/mfd/ucb1x00-assabet.c
> +++ b/drivers/mfd/ucb1x00-assabet.c
> @@ -6,15 +6,18 @@
>   *
>   *  We handle the machine-specific bits of the UCB1x00 driver here.
>   */
> -#include <linux/module.h>
> -#include <linux/init.h>
>  #include <linux/device.h>
>  #include <linux/err.h>
>  #include <linux/fs.h>
> -#include <linux/gpio_keys.h>
> +#include <linux/gpio/machine.h>
> +#include <linux/gpio/property.h>
> +#include <linux/init.h>
>  #include <linux/input.h>
> +#include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/proc_fs.h>
> +#include <linux/property.h>
> +#include <linux/slab.h>
>  #include <linux/mfd/ucb1x00.h>

Should 'linux/mfd/ucb1x00.h' be sorted alphabetically along with the other
header inclusions (e.g. placed before 'linux/module.h')?

>  
>  #define UCB1X00_ATTR(name,input)\
> @@ -34,50 +37,119 @@ UCB1X00_ATTR(vbatt, UCB_ADC_INP_AD1);
>  UCB1X00_ATTR(vcharger, UCB_ADC_INP_AD0);
>  UCB1X00_ATTR(batt_temp, UCB_ADC_INP_AD2);
>  
> +static const struct property_entry ucb1x00_gpio_keys_props[] = {
> +	PROPERTY_ENTRY_STRING("label", "ucb1x00"),
> +	PROPERTY_ENTRY_U32("poll-interval", 50),
> +	{ }
> +};
> +
> +#define UCB1X00_BTN_PROPS(_idx)						\
> +struct property_entry ucb1x00_btn##_idx##_props[] = {			\
> +	PROPERTY_ENTRY_U32("linux,code", BTN_0 + (_idx)),		\
> +	PROPERTY_ENTRY_GPIO("gpios", &ucb1x00_gpiochip_node,		\

Where is 'ucb1x00_gpiochip_node' defined or declared? Should we add an 'extern'
declaration or include the relevant header to prevent build errors?

> +			    _idx, GPIO_ACTIVE_HIGH),			\
> +	PROPERTY_ENTRY_STRING("label", "btn" #_idx),			\
> +	PROPERTY_ENTRY_BOOL("linux,can-disable"),			\
> +	{ }								\
> +}
> +
> +static const UCB1X00_BTN_PROPS(0);
> +static const UCB1X00_BTN_PROPS(1);
> +static const UCB1X00_BTN_PROPS(2);
> +static const UCB1X00_BTN_PROPS(3);
> +static const UCB1X00_BTN_PROPS(4);
> +static const UCB1X00_BTN_PROPS(5);
> +
> +static const struct property_entry * const ucb1x00_btn_props[] = {
> +	ucb1x00_btn0_props,
> +	ucb1x00_btn1_props,
> +	ucb1x00_btn2_props,
> +	ucb1x00_btn3_props,
> +	ucb1x00_btn4_props,
> +	ucb1x00_btn5_props,
> +};
> +
> +struct ucb1x00_assabet_priv {
> +	struct platform_device *pdev;
> +	struct fwnode_handle *keys_node;
> +	struct fwnode_handle *button_nodes[ARRAY_SIZE(ucb1x00_btn_props)];
> +};
> +
> +static void ucb1x00_assabet_remove_nodes(struct ucb1x00_assabet_priv *priv, int n)
> +{
> +	while (--n >= 0)
> +		fwnode_remove_software_node(priv->button_nodes[n]);
> +
> +	fwnode_remove_software_node(priv->keys_node);
> +}
> +
>  static int ucb1x00_assabet_add(struct ucb1x00_dev *dev)
>  {
>  	struct ucb1x00 *ucb = dev->ucb;
> -	struct platform_device *pdev;
> -	struct gpio_keys_platform_data keys;
> -	static struct gpio_keys_button buttons[6];
> -	unsigned i;
> -
> -	memset(buttons, 0, sizeof(buttons));
> -	memset(&keys, 0, sizeof(keys));
> -
> -	for (i = 0; i < ARRAY_SIZE(buttons); i++) {
> -		buttons[i].code = BTN_0 + i;
> -		buttons[i].gpio = ucb->gpio.base + i;
> -		buttons[i].type = EV_KEY;
> -		buttons[i].can_disable = true;
> +	struct platform_device_info pdevinfo = {
> +		.name = "gpio-keys",
> +		.id = PLATFORM_DEVID_NONE,
> +		.parent = &ucb->dev,
> +	};
> +	int ret;
> +	int i;

Nit: If you re-work this, please declare inside the if ().

> +
> +	struct ucb1x00_assabet_priv *priv;
> +
> +	priv = kzalloc_obj(*priv, GFP_KERNEL);

Why _obj() here instead of the usual candidates?

What about devm_*?

> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->keys_node = fwnode_create_software_node(ucb1x00_gpio_keys_props, NULL);
> +	if (IS_ERR(priv->keys_node)) {
> +		ret = PTR_ERR(priv->keys_node);
> +		goto err_free_priv;
>  	}
>  
> -	keys.buttons = buttons;
> -	keys.nbuttons = ARRAY_SIZE(buttons);
> -	keys.poll_interval = 50;
> -	keys.name = "ucb1x00";
> +	for (i = 0; i < ARRAY_SIZE(ucb1x00_btn_props); i++) {
> +		priv->button_nodes[i] = fwnode_create_software_node(ucb1x00_btn_props[i],
> +								    priv->keys_node);
> +		if (IS_ERR(priv->button_nodes[i])) {
> +			ret = PTR_ERR(priv->button_nodes[i]);
> +			goto err_free_buttons;
> +		}
> +	}
> +
> +	pdevinfo.fwnode = priv->keys_node;
>  
> -	pdev = platform_device_register_data(&ucb->dev, "gpio-keys", -1,
> -		&keys, sizeof(keys));
> +	priv->pdev = platform_device_register_full(&pdevinfo);
> +	ret = PTR_ERR_OR_ZERO(priv->pdev);
> +	if (ret)
> +		goto err_free_buttons;
>  
>  	device_create_file(&ucb->dev, &dev_attr_vbatt);
>  	device_create_file(&ucb->dev, &dev_attr_vcharger);
>  	device_create_file(&ucb->dev, &dev_attr_batt_temp);
>  
> -	dev->priv = pdev;
> +	dev->priv = priv;
>  	return 0;
> +
> +err_free_buttons:
> +	ucb1x00_assabet_remove_nodes(priv, i);
> +err_free_priv:
> +	kfree(priv);
> +	return ret;
>  }
>  
>  static void ucb1x00_assabet_remove(struct ucb1x00_dev *dev)
>  {
> -	struct platform_device *pdev = dev->priv;
> +	struct ucb1x00_assabet_priv *priv = dev->priv;
>  
> -	if (!IS_ERR(pdev))
> -		platform_device_unregister(pdev);
> +	if (!IS_ERR(priv->pdev))
> +		platform_device_unregister(priv->pdev);
> +
> +	ucb1x00_assabet_remove_nodes(priv, ARRAY_SIZE(priv->button_nodes));
>  
>  	device_remove_file(&dev->ucb->dev, &dev_attr_batt_temp);
>  	device_remove_file(&dev->ucb->dev, &dev_attr_vcharger);
>  	device_remove_file(&dev->ucb->dev, &dev_attr_vbatt);
> +
> +	kfree(priv);
>  }
>  
>  static struct ucb1x00_driver ucb1x00_assabet_driver = {
> 
> -- 
> 2.55.0.rc2.803.g1fd1e6609c-goog
> 

-- 
Lee Jones