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

[Markus Pargmann <mpa@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 2984 bytes --]

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

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]