| Message ID | 1372666571-17276-3-git-send-email-shawn.guo@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jul 01, 2013 at 04:16:09PM +0800, Shawn Guo wrote: > It's not always the case that clock is already available when sgtl5000 > get probed at the first time, e.g. the clock is provided by CPU DAI > which may be probed after sgtl5000. So let's defer the probe when > devm_clk_get() call fails and give it chance to try later. Why not fix this in the clock API - the same logic is going to apply to a great proportion of clocks?
On Mon, Jul 01, 2013 at 10:45:25AM +0100, Mark Brown wrote: > On Mon, Jul 01, 2013 at 04:16:09PM +0800, Shawn Guo wrote: > > It's not always the case that clock is already available when sgtl5000 > > get probed at the first time, e.g. the clock is provided by CPU DAI > > which may be probed after sgtl5000. So let's defer the probe when > > devm_clk_get() call fails and give it chance to try later. > > Why not fix this in the clock API - the same logic is going to apply to > a great proportion of clocks? It's not ever clear whether a missing clock is because it's just not provided or whether it's because it hasn't been registered yet. The decision to defer on missing resources is something which a _driver_ using the resource should make, not the subsystem providing the resource - because the driver should know better whether that resource is optional, whether it can continue to initialise without it and maybe try again later, or whether it does need to defer. Take for instance a video driver. :) It may be that it can't do all resolutions, but it can do some without an external clock, so it may decide that it can initialise without the external clock and allow one of the possible modes to be set - and when the external clock does appear, it can then allow the other modes. That kind of information is unknown at the subsystem level, and so the subsystem level should only ever return "I don't know about this" not "defer probe".
On Mon, Jul 01, 2013 at 12:33:10PM +0100, Russell King - ARM Linux wrote: > On Mon, Jul 01, 2013 at 10:45:25AM +0100, Mark Brown wrote: > > Why not fix this in the clock API - the same logic is going to apply to > > a great proportion of clocks? > It's not ever clear whether a missing clock is because it's just not > provided or whether it's because it hasn't been registered yet. The > decision to defer on missing resources is something which a _driver_ > using the resource should make, not the subsystem providing the > resource - because the driver should know better whether that resource > is optional, whether it can continue to initialise without it and maybe > try again later, or whether it does need to defer. Given how common it is to have a hard requirement for a clock I think we should at least have a helper which implements deferral, even if it's not part of the default request function, since so many drivers ought to be using it. Doing this in by default doesn't prevent the driver overriding, though - the driver can choose to squash down the PROBE_DEFER into a fatal error. One nice thing you can sometimes do here in the DT case is to have the subsystem tell the driver the difference between "this clock is mapped but not yet registered" and "this clock is not physically present" though there's never any guarantee that the provided resource will actually load. I don't think the idiom for the clock API is to provide every single clock in DT though so that might not be doable. > Take for instance a video driver. :) It may be that it can't do all > resolutions, but it can do some without an external clock, so it may > decide that it can initialise without the external clock and allow one > of the possible modes to be set - and when the external clock does > appear, it can then allow the other modes. Yes, that's the sort of case where you do definitely want to just carry on (though there is then some vulnerability to changes in init ordering with things like modular kernels).
diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c index 20bca03..1b7003c 100644 --- a/sound/soc/codecs/sgtl5000.c +++ b/sound/soc/codecs/sgtl5000.c @@ -1527,7 +1527,8 @@ static int sgtl5000_i2c_probe(struct i2c_client *client, if (IS_ERR(sgtl5000->mclk)) { ret = PTR_ERR(sgtl5000->mclk); dev_err(&client->dev, "Failed to get mclock: %d\n", ret); - return ret; + /* Defer the probe to see if the clk will be provided later */ + return ret == -ENOENT ? -EPROBE_DEFER : ret; } ret = clk_prepare_enable(sgtl5000->mclk);
It's not always the case that clock is already available when sgtl5000 get probed at the first time, e.g. the clock is provided by CPU DAI which may be probed after sgtl5000. So let's defer the probe when devm_clk_get() call fails and give it chance to try later. Signed-off-by: Shawn Guo <shawn.guo@linaro.org> --- sound/soc/codecs/sgtl5000.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)