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

[Xiang Shen]

On Tue, Jun 24, 2025 at 01:35:57PM +1000, Ilpo Järvinen wrote:
> 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@xxxxxxxxx>
> > >
> > > 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.

Hi,

I just thought there must be a reason that the checkpatch categories 
findings as "ERROR", "WARNING" and "CHECK".

Sometimes the number does make sense and means the vast majority
follow the widely accepted "rule".

Curiously, isn't it the contributor's due diligence to pass checkpatch
in the first place before sending? 
Should any objection, submit a patch to checkpatch itself,
instead of sneaking into the upstream quietly for the sake of "readability".

> > 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.

Indeed, that's precisely why it might be worth sacrificing a bit of "readability".