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.
> +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?
Does `timer_id` or some other equivalent names make more sense for the
variable?
> +static int pseries_wdt_start(struct watchdog_device *wdd)
> +{
[...]
> + rc = plpar_hcall_norets(H_WATCHDOG, flags, pw->num, msecs);
> + if (rc != H_SUCCESS) {
> + dev_crit(dev, "H_WATCHDOG: %ld: failed to start timer %lu",
> + rc, pw->num);
> + return -EIO;
>From `./scripts/checkpatch.pl --strict`:
ERROR: code indent should use tabs where possible
WARNING: please, no space before tabs
> +static struct watchdog_info pseries_wdt_info = {
> + .identity = DRV_NAME,
> + .options = WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE | WDIOF_SETTIMEOUT \
> + | WDIOF_PRETIMEOUT,
>From `./scripts/checkpatch.pl --strict`:
WARNING: Avoid unnecessary line continuations
> +static const struct watchdog_ops pseries_wdt_ops = {
> + .owner = THIS_MODULE,
> + .ping = pseries_wdt_start,
Does this mean: it needs hard restart for every ping?
> +static int pseries_wdt_probe(struct platform_device *pdev)
> +{
[...]
> + rc = plpar_hcall(H_WATCHDOG, ret, PSERIES_WDTF_OP_QUERY);
> + if (rc != H_SUCCESS)
> + return (rc == H_FUNCTION) ? -ENODEV : -EIO;
The parentheses can be dropped.
> + pw = devm_kzalloc(&pdev->dev, sizeof *pw, GFP_KERNEL);
> + if (pw == NULL)
>From `./scripts/checkpatch.pl --strict`:
CHECK: Comparison to NULL could be written "!pw"
> + pw->num = pdev->id + 1; /* 0-based -> 1-based */
Didn't see where the platform device was registered but using the pdev->id as
the timer id could be unreliable (e.g. from auto increment).
