Re: [v3,04/11] watchdog/hpwdt: white space changes

[Guenter Roeck <linux@roeck-us.net>]

On 02/17/2018 11:32 AM, Jerry Hoemann wrote:
> On Sat, Feb 17, 2018 at 08:17:34AM -0800, Guenter Roeck wrote:
>> On Thu, Feb 15, 2018 at 04:43:53PM -0700, Jerry Hoemann wrote:
>>> Minor white space changes and some name clean up.
>>>
>>> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
>>> ---
>>>   MODULE_DEVICE_TABLE(pci, hpwdt_devices);
>>>   
>>> @@ -102,7 +100,7 @@ static int hpwdt_time_left(void)
>>>   	return TICKS_TO_SECS(ioread16(hpwdt_timer_reg));
>>>   }
>>>   
>>> -#ifdef CONFIG_HPWDT_NMI_DECODING
>>> +#ifdef CONFIG_HPWDT_NMI_DECODING	/* { */
>>>   static int hpwdt_my_nmi(void)
>>>   {
>>>   	return ioread8(hpwdt_nmistat) & 0x6;
>>> @@ -133,7 +131,7 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
>>>   
>>>   	return NMI_HANDLED;
>>>   }
>>> -#endif /* CONFIG_HPWDT_NMI_DECODING */
>>> +#endif					/* } */
>>
>> I disagree with those changes. While I don't object to adding the '{'
>> per se, I find it very useful to have the 'CONFIG_HPWDT_NMI_DECODING'
>> with an endif to be able to associate it with the matching #ifdef.
> 
> The matching /* { */ and /* } */ allow for quickly the finding of the
> matching ifdef/endif.
> 
> In the "vim" editor, the command '%' will take one from one curly paren to its
> matching curly paren...
> 
> There is a similar sequence for emacs.
> 
This isn't about you, it is about the community.

Looking again into this patch, the only acceptable change is the copyright update.
The driver is still based on softdog. The other changes are your personal preference
only and don't fix any checkpatch issues (and even those would be arguable for
some maintainers). This means that, long term, we might get another patch from
someone else changing indentations and variable names again ... and again ...
and again. This adds no value.

Guenter