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

[Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]

On Mar 09, Guenter Roeck wrote:
> On 03/04/2014 08:07 AM, Ezequiel Garcia wrote:
[..]
> >  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.
> 
> >+		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