Subject: Re: [PATCH V1] ASoC: fsl_ssi: refine ipg clock usage in this module

From: Nicolin Chen <[email-masked]>
Date: Wed, 10 Sep 2014 10:42:04 -0700

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.

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.

Best regards,
Nicolin