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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
