Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test

[Julia Lawall]

On Tue, 25 Jan 2011, Ryan Mallon wrote:

> On 01/25/2011 09:28 AM, Julia Lawall wrote:
> >> Julia is correct. Some architectures can return NULL from clk_get, but I
> >> didn't check the at91 before posting :-/. If we can't return NULL from
> >> clk_get then we shouldn't bother checking for it. I do think we should
> >> drop the !IS_ERR(clk_get(dev, func)) check though.
> > 
> > It seems a bit subtle, because the clk manipulated by clk_get in the call 
> > of clk_get(dev, func) is not necessarily the same as the one in 
> > clock_associate.  But perhaps this is the only possibility in practice?
> 
> Not sure I follow. The at91 clk_get does not modify the clk. In
> at91_clock_associate we have:
> 
> 	clk->function = func;
> 	clk->dev = dev;
> 
> and in clk_get we have:
> 
> 	if (clk->function && (dev == clk->dev) &&
> 		strcmp(id, clk->function) == 0)
>             return clk;
> 
> So at91_clock_associate sets the function for a clock, and clk_get
> returns clocks based on the function association if the name lookup
> fails. The only caveat to this is that the the clock function name
> (clk->function) is not the same as any others clock's clk->name.

Right, that was what I was worried about.  That one would find the same 
information already present but somewhere else.  But perhaps it can't 
happen, or it doesn't matter if it does?

julia

> The !IS_ERR(clk_get(dev, func)) check in at91_clock_associate just
> appears to check that the clock is not already associated with the given
> function. We don't really need this check because we are just making the
> same assignment anyway.
> 
> ~Ryan
> 
> -- 
> Bluewater Systems Ltd - ARM Technology Solution Centre
> 
> Ryan Mallon         		5 Amuri Park, 404 Barbadoes St
> ryan@xxxxxxxxxxxxxxxx         	PO Box 13 889, Christchurch 8013
> http://www.bluewatersys.com	New Zealand
> Phone: +64 3 3779127		Freecall: Australia 1800 148 751
> Fax:   +64 3 3779135			  USA 1800 261 2934
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/