Re: [PATCH] platform/x86: intel-vbtn: Fix code style issues

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Sun, 22 Jun 2025, Xiang Shen wrote:
> On Fri, Jun 20, 2025 at 12:00:03PM +1000, Hans de Goede wrote:
> > On 20-Jun-25 2:38 AM, Xiang Shen wrote:
> > > Fix checkpatch code style errors:
> > > 
> > > ERROR: do not use assignment in if condition
> > > +	if ((ke = sparse_keymap_entry_from_scancode(priv->buttons_dev, event))) {
> > > 
> > > ERROR: do not use assignment in if condition
> > > +	} else if ((ke = sparse_keymap_entry_from_scancode(priv->switches_dev, event))) {
> > > 
> > > Signed-off-by: Xiang Shen <turyshen@gmail.com>
> > 
> > Thank you for your patch, but this change really does not make
> > the code more readable.
> > 
> > The contrary the suggested changes are making the code harder
> > to read, so NACK.
> > 
> > Note checkpatch is just a tool, sometimes there are good reasons
> > to deviate from the style checks done by checkpatch.
> > 
> > Next time when submitting a patch to fix checkpatch issues please
> > take a look at the resulting code after the patch and only submit
> > the patch upstream if it actually is an improvement.
> > 
> > Regards,
> > 
> > Hans
> > 
> Hi Hans,
> 
> Thanks for the feedback. 
> 
> That's fine if breaking the "rule" is the only way to keep the file readable.
> 
> However, there are only three files (x86/sony-laptop.c and 
> x86/dell/dell_rbu.c) out of 273 files in the whole drivers/platform 
> folder that have such an error. 

Hi,

Please don't call correct code "error" even if checkpatch may label it as 
such. The goal is NOT and will never be to have zero checkpatch warnings.

The fact that the checkpatch "rule" is broken only a few times does not 
mean those 3 places have a problem, it just tells it's good rule for the
general case. So I won't accept using such numbers as a leverage against 
the few places just for the sake of silencing checkpatch.

> Perhaps there are other approaches to make them more readable without 
> breaking the rule.

Perhaps, but I'm not sure the effort spent to find one is worthwhile 
investment.

> > > ---
> > >  drivers/platform/x86/intel/vbtn.c | 38 +++++++++++++++++--------------
> > >  1 file changed, 21 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/drivers/platform/x86/intel/vbtn.c b/drivers/platform/x86/intel/vbtn.c
> > > index 232cd12e3c9f..bcc97b06844e 100644
> > > --- a/drivers/platform/x86/intel/vbtn.c
> > > +++ b/drivers/platform/x86/intel/vbtn.c
> > > @@ -160,30 +160,34 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
> > >  
> > >  	guard(mutex)(&priv->mutex);
> > >  
> > > -	if ((ke = sparse_keymap_entry_from_scancode(priv->buttons_dev, event))) {
> > > +	ke = sparse_keymap_entry_from_scancode(priv->buttons_dev, event);
> > > +	if (ke) {
> > >  		if (!priv->has_buttons) {
> > >  			dev_warn(&device->dev, "Warning: received 0x%02x button event on a device without buttons, please report this.\n",
> > >  				 event);
> > >  			return;
> > >  		}
> > >  		input_dev = priv->buttons_dev;
> > > -	} else if ((ke = sparse_keymap_entry_from_scancode(priv->switches_dev, event))) {
> > > -		if (!priv->has_switches) {
> > > -			/* See dual_accel_detect.h for more info */
> > > -			if (priv->dual_accel)
> > > -				return;
> > > -
> > > -			dev_info(&device->dev, "Registering Intel Virtual Switches input-dev after receiving a switch event\n");
> > > -			ret = input_register_device(priv->switches_dev);
> > > -			if (ret)
> > > -				return;
> > > -
> > > -			priv->has_switches = true;
> > > -		}
> > > -		input_dev = priv->switches_dev;
> > >  	} else {
> > > -		dev_dbg(&device->dev, "unknown event index 0x%x\n", event);
> > > -		return;
> > > +		ke = sparse_keymap_entry_from_scancode(priv->switches_dev, event);
> > > +		if (ke) {
> > > +			if (!priv->has_switches) {
> > > +				/* See dual_accel_detect.h for more info */
> > > +				if (priv->dual_accel)
> > > +					return;
> > > +
> > > +				dev_info(&device->dev, "Registering Intel Virtual Switches input-dev after receiving a switch event\n");
> > > +				ret = input_register_device(priv->switches_dev);
> > > +				if (ret)
> > > +					return;
> > > +
> > > +				priv->has_switches = true;
> > > +			}
> > > +			input_dev = priv->switches_dev;
> > > +		} else {
> > > +			dev_dbg(&device->dev, "unknown event index 0x%x\n", event);
> > > +			return;
> > > +		}
> > >  	}
> > >  
> > >  	if (priv->wakeup_mode) {
> > 
> 

-- 
 i.