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

From: Julia Lawall
Date: Mon Jan 24 2011 - 16:01:57 EST


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/