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

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

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