RE: [PATCH v4 1/3] watchdog: add rza_wdt driver

On Thursday, March 02, 2017, Guenter Roeck worte:

quoted

quoted

The above two lines are unnecessary.

OK.

#I'll assume you mean take out just the last sentence (2 lines), not
both sentences (all 3 lines).

The two empty lines.

Ooops! That makes more sense.

quoted

quoted

quoted

+	rate = clk_get_rate(priv->clk);
+	if (!rate)
+		return -ENOENT;
+
+	/* Assume slowest clock rate possible (CKS=7) */
+	rate /= 16384;
+

The rate check should probably be here to avoid situations where
rate < 16384.

Do I need that if it's technically not possible to have a 'rate' less

than 25MHz?

quoted

These watchdogs HW are always feed directly from the peripheral clock
and there is no such thing as a 16kHz peripheral block an any Renesas

SoC.

quoted

Following that line of argument, can clk_get_rate() ever return 0 ?

In the DT binding, it says that a clock source is required to be present.

If the user leaves out the "clocks =", then devm_clk_get will fail.

If the user puts in some crazy value for "clocks = ", then maybe you could get
0 (assuming there is a valid clock node they made by themselves somewhere that
runs at 0Hz).
But in that extreme case, I think they deserve to have it crash and burn because
who knows what they are doing.

quoted

quoted

quoted

+	priv->wdev.max_hw_heartbeat_ms = (1000 * U8_MAX)/rate;

space before and after /

OK.
#Funny because checkpatch.pl said it didn't like a space on one side
but  not the other, so I choose no spaces and it was happy. I'm way
below 80  characters for that line so it doesn't matter to me.

That would be a bug in checkpatch. coding style, chapter 3.1, still
applies.
Or at least I hope so.

OK. Thank you for the clarification.

quoted

quoted

quoted

+	if (ret < 0)
+		return ret;
+
+	return 0;

Also just
	return ret;

OK.

quoted

quoted

quoted

+static int rza_wdt_remove(struct platform_device *pdev) {
+	struct rza_wdt *priv = platform_get_drvdata(pdev);
+
+	watchdog_unregister_device(&priv->wdev);
+	iounmap(priv->base);

iounmap is unnecessary (and wrong).

Anything mapped with devm_ioremap_resource() automatically gets
unmapped when the drive gets unloaded?

That is the point of devm_ functions. It also means that you won't need a
remove function if you also use devm_watchdog_register_device().

OK.
I see that only 1 driver is using devm_watchdog_register_device (wdat_wdt.c), so
maybe that is a new method.


Thank you,
Chris

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html