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

On Thu, Jul 16, 2026 at 02:06:04PM +0100, Lee Jones wrote:
> 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 <[email protected]>
> > ---
> >  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')?

Maybe, but I did not add it here...

> 
> >  
> >  #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?

We did. It is introduced in the previous patch and is declared in
linux/mfd/ucb1x00.h

> 
> > +			    _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 ().

You man inside "for ()"?

> 
> > +
> > +	struct ucb1x00_assabet_priv *priv;
> > +
> > +	priv = kzalloc_obj(*priv, GFP_KERNEL);
> 
> Why _obj() here instead of the usual candidates?
> 
> What about devm_*?

This is not a driver in the sense of device core driver, so devm would
not work.

Thanks.

-- 
Dmitry