Re: [PATCH v2 4/7] watchdog: orion: Add Armada 375/380 SoC support

On Mar 09, Guenter Roeck wrote:

On 03/04/2014 08:07 AM, Ezequiel Garcia wrote:

[..]

quoted

 MODULE_DEVICE_TABLE(of, orion_wdt_of_match_table);

@@ -396,6 +475,25 @@ static int orion_wdt_probe(struct platform_device *pdev)
 		if (!dev->rstout)
 			return -ENOMEM;

+	} else if (of_device_is_compatible(node, "marvell,armada-375-wdt") ||
+		   of_device_is_compatible(node, "marvell,armada-380-wdt")) {
+
+		res = platform_get_resource(pdev, IORESOURCE_MEM, 1);

Looks like each supported watchdog needs this call. Might as well do it earlier
and just once, unless you have a good reason for doing it this way.
To me this just looks like a lot of code replication.

Well... it seemed to me as cleaner to have one 'if' block per compatible
group and do all the ioremap'ing in it.

Otherwise, it would be like this (seudo-code):

rstout = foo_request_ioremap();
if (!rsout)
	if (compatible(1)) {
		/* Missing RSTOUT! Do backwards compatibility hack */
		rstout = fw_bug_fallback();
		if (!rstout)
			return -ENODEV;
	} else
		return -ENODEV;

if (compatible(3))
	rstout_mask = foo_request_ioremap();
	if (!rstout_mask)
		return -ENODEV;

Maybe it's a matter of taste, but I think this looks better:

if (compatible(1))
	request_ioremap all registers ...
else if (compatible(2))
	request_ioremap all registers ...
else if (compatible(3))
	request_ioremap all registers ...
else
	return -ENODEV;

At the price of a little code duplication.

It might also possibly make sense to move all the memory initializations
into a separate function; the probe function gets a bit large.

quoted

+		if (!res)
+			return -ENODEV;
+		dev->rstout = devm_ioremap(&pdev->dev, res->start,
+					   resource_size(res));
+		if (!dev->rstout)
+			return -ENOMEM;
+
+		res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
+		if (!res)
+			return -ENODEV;
+		dev->rstout_mask = devm_ioremap(&pdev->dev, res->start,
+						resource_size(res));

devm_ioremap_resource() is better here.

True. We are currently confusing the shared and non-shared registers,
and treating them all as shared. I'll fix that.

Thanks for the review,
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html