Re: [alsa-devel] [RFC PATCH] ASoC: wm8731: let codec to manage clock by itself

From: Lars-Peter Clausen
Date: Tue Feb 03 2015 - 12:49:09 EST


On 02/03/2015 06:26 PM, Lars-Peter Clausen wrote:
On 02/03/2015 06:17 PM, Russell King - ARM Linux wrote:
On Tue, Feb 03, 2015 at 05:53:48PM +0100, Lars-Peter Clausen wrote:
On 02/03/2015 01:44 PM, Mark Brown wrote:
On Tue, Feb 03, 2015 at 08:54:57AM +0100, Manuel Lauss wrote:

+ wm8731->mclk = devm_clk_get(&spi->dev, "mclk");
+ if (IS_ERR(wm8731->mclk)) {
+ wm8731->mclk = NULL;
+ dev_warn(&spi->dev, "assuming static MCLK\n");
+ }

This is broken for both deferred probe and in the case where the clock
API genuinely returns a NULL clock. Other than that it's the kind of
thing that we've done for some other drivers, though it's not good to
have to do this. Check them for correct behaviour.

Ideally we'd introduce a {devm_}clk_get_optional(), with the same semantics
as gpiod_get_optional(), which handles the finer details of differentiating
between clock specified, but not yet probed, clock specified, but
incorrectly and no clock specified, so this doesn't have to be done over and
over by each driver.

No, we don't need to. It clk_get() already knows this distinction, and
it appropriately returns -ENOENT vs -EPROBE_DEFER according to whether
there's a clock specified in DT or not.

I know, but it returns a error when no clock is specified (-ENOENT), whereas
gpiod_get_optional()-like semantics mean, it would return no error.

What I wanted to say is that pretty much every user of clk_get() that wants a optional clock gets the handling wrong. E.g. they check for PTR_ERR(clk) == -EPROBE_DEFER rather than checking for PTR_ERR(clk) != -ENOENT. Which causes errors when the clock is specified, but incorrectly specified (e.g. invalid phandle or specifier) to be silently ignored.

My hope is that having a explicit API for requesting a optional clock might make it easier for users to gets things right.

If you have coccinelle you can use the following script to find good and bad users:

@@
expression clk;
@@
clk =
(
devm_clk_get
|
clk_get
)
(...);
<+...
(
*PTR_ERR(clk) == -EPROBE_DEFER
|
*PTR_ERR(clk) != -ENOENT
)
...+>


--
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/