On Wed, Sep 10, 2014 at 10:42:04AM -0700, Nicolin Chen wrote:
On Wed, Sep 10, 2014 at 04:12:53PM +0800, Shengjiu Wang wrote:
Then we can get a patch like: open() {
- clk_prepare_enable(); ....
}
close() { ....
- clk_disable_unprepare()
}
what is the open() and close()? do you mean the fsl_ssi_startup() and fsl_ssi_shutdown()?
Yea.
probe() { clk_get(); clk_prepare_enable(); .... if (xxx)
goto err_xx;
return ret;- clk_disable_unprepare(); return 0;
-err_xx:
- clk_disable_unprepare()
}
If this probe() is fsl_ssi_imx_probe(), I think no need to add clk_prepare_enable() or clk_disable_unprepare(), seems there is no registers accessing in this probe.
This is trying to be safe, especially for such a driver being used by multiple platforms. You can omit this as long as the patch can pass the test on old imx, PowerPC, and AC97 platforms.
And another risk just came to my mind is that there would be a possibility that a machine driver would call set_dai_fmt() early, after SSI's probe() and before SSI's startup(), if the machine driver contains dai_fmt assignment in its probe(). Then, without regmap_mmio_clk(), it'll be tough for us over here because we may also need to add clock enable/disable for set_dai_fmt/set_sysclk(), even if there might be still tiny risk that we missed something.
Thanks, didn't thought about that. As there are no restrictions on when these functions may be called, it has to be handled.
Then there could be a selfish approach to circumvent it is to use regmap_mmio_clk() with "ipg" at the beginning and call regmap_mmio() without "ipg" if getting a failed return value from regmap_mmio_clk, and meanwhile to keep the clock always enabled for the regmap_mmio() case just like what the current driver is doing. This may result those non-ipg-clk platforms can't benefit from this refinement unless they update their DT bindings -- use "ipg" for core clock This might be the safest and simplest way for us, I'm not sure everyone would be comfortable with this idea though.
I like the "selfish" approach. It would save a lot of clock enabling/disabling and error handling and at the same time it doesn't break the DT compatibility. The platforms with an old DT would have the old behaviour, but that could be changed by updating the devicetrees which should be easy to do for all the imx SoCs.
Best regards,
Markus