Re: [PATCH v2 1/7] watchdog: orion: Introduce a SoC-specific RSTOUT mapping

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

Hi Guenter,

Thanks for reviewing this patchset.

On Mar 09, Guenter Roeck wrote:
[..]
> >@@ -317,6 +313,7 @@ MODULE_DEVICE_TABLE(of, orion_wdt_of_match_table);
> >  static int orion_wdt_probe(struct platform_device *pdev)
> >  {
> >  	struct orion_watchdog *dev;
> >+	struct device_node *node = pdev->dev.of_node;
> >  	const struct of_device_id *match;
> >  	unsigned int wdt_max_duration;	/* (seconds) */
> >  	struct resource *res;
> >@@ -346,10 +343,27 @@ static int orion_wdt_probe(struct platform_device *pdev)
> >  	if (!dev->reg)
> >  		return -ENOMEM;
> >
> >-	dev->rstout = orion_wdt_ioremap_rstout(pdev, res->start &
> >-						     INTERNAL_REGS_MASK);
> >-	if (!dev->rstout)
> >+	if (of_device_is_compatible(node, "marvell,orion-wdt")) {
> >+
> >+		dev->rstout = orion_wdt_ioremap_rstout(pdev, res->start &
> >+						       INTERNAL_REGS_MASK);
> >+		if (!dev->rstout)
> >+			return -ENODEV;
> >+
> >+	} else if (of_device_is_compatible(node, "marvell,armada-370-wdt") ||
> >+		   of_device_is_compatible(node, "marvell,armada-xp-wdt")) {
> >+
> >+		res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> >+		if (!res)
> >+			return -ENODEV;
> >+		dev->rstout = devm_ioremap(&pdev->dev, res->start,
> >+					   resource_size(res));
> 
> Better use devm_ioremap_resource, and then you don't have to check for
> the error from platform_get_resource() since devm_ioremap_resource()
> takes care of it.
> 
> The same change should be made for the other calls call to devm_ioremap;
> different patch though.
> 

Hm... well, could be. However it's not that simple! devm_ioremap_resource()
calls request_region() and since the RSTOUT register is shared, we can't
request it for this driver. This applies on orion-wdt only, though,
and not on armada-{370,xp}-wdt, so we can do it as long as we keep two
different paths.

> On a side note, the resource is always assigned, only a workaround exists
> for "marvell,orion-wdt" if it isn't. Wonder if it would make sense
> to move the calls to platform_get_resource and devm_ioremap[_resource]
> further up and have it just in this function.
> 

Aside from the above discussion about requesting the resource, I think it's
more readable this way, hiding the complexity of the firmware bug fallback
on a function, and use different paths.

> >+		if (!dev->rstout)
> >+			return -ENOMEM;
> >+
> >+	} else {
> >  		return -ENODEV;
> 
> Is this stricter than the original code on purpose ?
> 
> Previously the driver would instantiate successfully in this case
> as long as IORESOURCE_MEM, 1 was defined.
> 

Uh? This patch is not changing that. We're just splitting the way we
ioremap the RSTOUT register, as preparation work to add another compatible
string. Unless I've done something wrong, this is not *stricter* in any
way.
-- 
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