> On May 9, 2022, at 9:35 PM, Tzung-Bi Shih <tzungbi@xxxxxxxxxx> wrote:
>
> On Mon, May 09, 2022 at 12:43:57PM -0500, Scott Cheloha wrote:
>> +#define SETFIELD(_v, _b, _e) \
>> + (((unsigned long)(_v) << PPC_BITLSHIFT(_e)) & PPC_BITMASK((_b), (_e)))
>> +#define GETFIELD(_v, _b, _e) \
>> + (((unsigned long)(_v) & PPC_BITMASK((_b), (_e))) >> PPC_BITLSHIFT(_e))
>
> From `./scripts/checkpatch.pl --strict`:
> WARNING: please, no spaces at the start of a line
>
>> +#define PSERIES_WDTQL_MUST_STOP 1
>
> From `./scripts/checkpatch.pl --strict`:
> WARNING: please, no space before tabs
>
>> +static const struct kernel_param_ops action_ops = { .set = action_set };
>> +module_param_cb(action, &action_ops, NULL, S_IRUGO);
>
> From `./scripts/checkpatch.pl --strict`:
> WARNING: Symbolic permissions 'S_IRUGO' are not preferred. Consider using
> octal permissions '0444'.
>
>> +MODULE_PARM_DESC(action, "Action taken when watchdog expires: \"hard-poweroff\", \"hard-restart\", or \"dump-restart\" (default=\"hard-restart\")");
>
> The line exceeds 100 columns.
I was under the impression that strings were an exception to the
line-length rule. Is that not the case?
`scripts/checkpatch.pl --strict` complains if I break the string up:
WARNING: quoted string split across lines
#162: FILE: drivers/watchdog/pseries-wdt.c:162:
+MODULE_PARM_DESC(action, "Action taken when watchdog expires: "
+ "\"hard-poweroff\", \"hard-restart\", or \"dump-restart\" "
WARNING: quoted string split across lines
#163: FILE: drivers/watchdog/pseries-wdt.c:163:
+ "\"hard-poweroff\", \"hard-restart\", or \"dump-restart\" "
+ "(default=\"hard-restart\")");
>> +static bool nowayout = WATCHDOG_NOWAYOUT;
>> +module_param(nowayout, bool, S_IRUGO);
>
> From `./scripts/checkpatch.pl --strict`:
> WARNING: Symbolic permissions 'S_IRUGO' are not preferred. Consider using
> octal permissions '0444'.
>
>> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>
> From `./scripts/checkpatch.pl --strict`, the line exceeds 100 columns.
>
>> +#define WATCHDOG_TIMEOUT 60
>> +static unsigned int timeout = WATCHDOG_TIMEOUT;
>> +module_param(timeout, uint, S_IRUGO);
>
> From `./scripts/checkpatch.pl --strict`:
> WARNING: Symbolic permissions 'S_IRUGO' are not preferred. Consider using
> octal permissions '0444'.
>
>> +MODULE_PARM_DESC(timeout, "Initial watchdog timeout in seconds (default=" __MODULE_STRING(WATCHDOG_TIMEOUT) ")");
>
> From `./scripts/checkpatch.pl --strict`, the line exceeds 100 columns.
>
>> +struct pseries_wdt {
>> + struct watchdog_device wd;
>> + unsigned long num; /* NB: Watchdog numbers are 1-based */
>
> What does NB stand for? Could it be removed from the comment?
Sure, removed.
> Does `timer_id` or some other equivalent names make more sense for the
> variable?
The hardware documentation calls the parameter "watchdogNumber", so
I think "num" is better than "id" in this case.
