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

2014-09-11 Thread Markus Pargmann 
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- |


signature.asc
Description: Digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2014-09-11 Thread Shengjiu Wang 
On Thu, Sep 11, 2014 at 08:36:51AM +0200, Markus Pargmann wrote:
 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

Thanks Markus and Nicolin

I have sent version 2 for this patch. Please review it.

best regards
wang shengjiu 
 -- 
 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- |


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2014-09-10 Thread Markus Pargmann 
On Tue, Sep 09, 2014 at 11:38:05AM -0700, Nicolin Chen wrote:
 On Tue, Sep 09, 2014 at 05:18:07PM +0800, Shengjiu Wang wrote:
  @@ -1321,7 +1333,11 @@ static int fsl_ssi_probe(struct platform_device 
  *pdev)
  return -ENOMEM;
  }
   
  -   ssi_private-regs = devm_regmap_init_mmio(pdev-dev, iomem,
  +   if (ssi_private-soc-imx)
  +   ssi_private-regs = devm_regmap_init_mmio_clk(pdev-dev,
  +   ipg, iomem, fsl_ssi_regconfig);
  +   else
  +   ssi_private-regs = devm_regmap_init_mmio(pdev-dev, iomem,
 
 As Markus mentioned, the key point here is to be compatible with those
 non-clock-name platforms.
 
 I think it would be safer to keep the current code while adding an extra
 clk_disable_unprepare() at the end of probe() as a common routine. And
 meantime, make sure to have the call for imx only because it seems that
 the other platforms do not depend on the clock. //a bit guessing here :)
 
 Then we can get a patch like:
 open() {
 + clk_prepare_enable();
   
 }
 
 close() {
   
 + clk_disable_unprepare()
 }
 
 probe() {
   clk_get();
   clk_prepare_enable();
   
   if (xxx)
 - goto err_xx;
 + return ret;
   
 + clk_disable_unprepare();
   return 0;
 -err_xx:
 - clk_disable_unprepare()
 }
 
 remove() {
   
 - clk_disable_unprepare()
 }

If I remember correctly, there may be AC97 communication with the codec
before any substream is created. That's why we enable the SSI unit right
at the beginning for AC97 in fsl_ssi_setup_reg_vals(). So we need to
check for AC97 before disabling clocks.

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


signature.asc
Description: Digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2014-09-10 Thread Shengjiu Wang 
On Tue, Sep 09, 2014 at 11:38:05AM -0700, Nicolin Chen wrote:
 On Tue, Sep 09, 2014 at 05:18:07PM +0800, Shengjiu Wang wrote:
  @@ -1321,7 +1333,11 @@ static int fsl_ssi_probe(struct platform_device 
  *pdev)
  return -ENOMEM;
  }
   
  -   ssi_private-regs = devm_regmap_init_mmio(pdev-dev, iomem,
  +   if (ssi_private-soc-imx)
  +   ssi_private-regs = devm_regmap_init_mmio_clk(pdev-dev,
  +   ipg, iomem, fsl_ssi_regconfig);
  +   else
  +   ssi_private-regs = devm_regmap_init_mmio(pdev-dev, iomem,
 
 As Markus mentioned, the key point here is to be compatible with those
 non-clock-name platforms.
 
 I think it would be safer to keep the current code while adding an extra
 clk_disable_unprepare() at the end of probe() as a common routine. And
 meantime, make sure to have the call for imx only because it seems that
 the other platforms do not depend on the clock. //a bit guessing here :)
 
 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()?
 
 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.
 
 remove() {
   
 - clk_disable_unprepare()
 }
 
 As long as you make the subject clear as 'Don't enable core/ipg clock
 when SSI's idle', I'm sure you can make them within a single patch.
 
 And an alternative way for open() and close() is to put those code into
 pm_runtime_resume/suspend() instead (since we might have some internal
 code need to be added by using pm_runtime as well), which would make
 the further code neater IMO.
 
 Thank you
 Nicolin
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2014-09-10 Thread Shengjiu Wang 
On Tue, Sep 09, 2014 at 12:59:29PM -0700, Nicolin Chen wrote:
 On Tue, Sep 09, 2014 at 02:37:42PM -0500, Timur Tabi wrote:
  On 09/09/2014 01:38 PM, Nicolin Chen wrote:
  make sure to have the call for imx only because it seems that
  the other platforms do not depend on the clock.
  
  Although I doubt anyone will every add support for clocks to PowerPC side
  of this driver, I would prefer to avoid IMX-specific changes. Instead, the
  code should check if a clock is available.  That's why I suggested this
  change:
  
  -   if (ssi_private-soc-imx)
  +   if (!IS_ERR(ssi_private-clk))
 
 Hmm I think the following change may be better?
 
 probe() {
   
 + /*
 +  * Initially mark the clock to NULL for all platforms so that later
 +  * clk_prepare_enable() will ignore and return 0 for non-clock cases.
 +  */
 + ssi_private-clk = NULL;
   .
   fsl_ssi_imx_probe();
 }
ssi_private is initialized to zero in beginning of probe. I think no need to
add this change here.

wang shengjiu
 
 In this way, all platforms, not confined to imx any more, will be able
 to call clk_prepare_enable(). Then we don't need an extra platform check
 before calling it.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2014-09-10 Thread Shengjiu Wang 
On Wed, Sep 10, 2014 at 08:21:18AM +0200, Markus Pargmann wrote:
 On Tue, Sep 09, 2014 at 11:38:05AM -0700, Nicolin Chen wrote:
  On Tue, Sep 09, 2014 at 05:18:07PM +0800, Shengjiu Wang wrote:
   @@ -1321,7 +1333,11 @@ static int fsl_ssi_probe(struct platform_device 
   *pdev)
 return -ENOMEM;
 }

   - ssi_private-regs = devm_regmap_init_mmio(pdev-dev, iomem,
   + if (ssi_private-soc-imx)
   + ssi_private-regs = devm_regmap_init_mmio_clk(pdev-dev,
   + ipg, iomem, fsl_ssi_regconfig);
   + else
   + ssi_private-regs = devm_regmap_init_mmio(pdev-dev, iomem,
  
  As Markus mentioned, the key point here is to be compatible with those
  non-clock-name platforms.
  
  I think it would be safer to keep the current code while adding an extra
  clk_disable_unprepare() at the end of probe() as a common routine. And
  meantime, make sure to have the call for imx only because it seems that
  the other platforms do not depend on the clock. //a bit guessing here :)
  
  Then we can get a patch like:
  open() {
  +   clk_prepare_enable();
  
  }
  
  close() {
  
  +   clk_disable_unprepare()
  }
  
  probe() {
  clk_get();
  clk_prepare_enable();
  
  if (xxx)
  -   goto err_xx;
  +   return ret;
  
  +   clk_disable_unprepare();
  return 0;
  -err_xx:
  -   clk_disable_unprepare()
  }
  
  remove() {
  
  -   clk_disable_unprepare()
  }
 
 If I remember correctly, there may be AC97 communication with the codec
 before any substream is created. That's why we enable the SSI unit right
 at the beginning for AC97 in fsl_ssi_setup_reg_vals(). So we need to
 check for AC97 before disabling clocks.
 
 Best regards,
 
 Markus

hi Markus

I think if clk_prepare_enable() in startup(), and clk_disable_unprepare()
in shutdown can meet this requirement, right?


done:
if (ssi_private-dai_fmt)
_fsl_ssi_set_dai_fmt(ssi_private, ssi_private-dai_fmt);

I find that in end of probe, there is setting of dai_fmt. Can we remove this?
because this setting need to enable ipg clock, and if ac97, ipg clock can't be
disabled.

wang shengjiu
 
 -- 
 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- |


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2014-09-10 Thread Markus Pargmann 
Hi,

On Wed, Sep 10, 2014 at 06:30:06PM +0800, Shengjiu Wang wrote:
 On Wed, Sep 10, 2014 at 08:21:18AM +0200, Markus Pargmann wrote:
  On Tue, Sep 09, 2014 at 11:38:05AM -0700, Nicolin Chen wrote:
   On Tue, Sep 09, 2014 at 05:18:07PM +0800, Shengjiu Wang wrote:
@@ -1321,7 +1333,11 @@ static int fsl_ssi_probe(struct platform_device 
*pdev)
return -ENOMEM;
}
 
-   ssi_private-regs = devm_regmap_init_mmio(pdev-dev, iomem,
+   if (ssi_private-soc-imx)
+   ssi_private-regs = 
devm_regmap_init_mmio_clk(pdev-dev,
+   ipg, iomem, fsl_ssi_regconfig);
+   else
+   ssi_private-regs = devm_regmap_init_mmio(pdev-dev, 
iomem,
   
   As Markus mentioned, the key point here is to be compatible with those
   non-clock-name platforms.
   
   I think it would be safer to keep the current code while adding an extra
   clk_disable_unprepare() at the end of probe() as a common routine. And
   meantime, make sure to have the call for imx only because it seems that
   the other platforms do not depend on the clock. //a bit guessing here :)
   
   Then we can get a patch like:
   open() {
   + clk_prepare_enable();
 
   }
   
   close() {
 
   + clk_disable_unprepare()
   }
   
   probe() {
 clk_get();
 clk_prepare_enable();
 
 if (xxx)
   - goto err_xx;
   + return ret;
 
   + clk_disable_unprepare();
 return 0;
   -err_xx:
   - clk_disable_unprepare()
   }
   
   remove() {
 
   - clk_disable_unprepare()
   }
  
  If I remember correctly, there may be AC97 communication with the codec
  before any substream is created. That's why we enable the SSI unit right
  at the beginning for AC97 in fsl_ssi_setup_reg_vals(). So we need to
  check for AC97 before disabling clocks.
  
  Best regards,
  
  Markus
 
 hi Markus
 
 I think if clk_prepare_enable() in startup(), and clk_disable_unprepare()
 in shutdown can meet this requirement, right?

Yes that could work.

 
 done:
 if (ssi_private-dai_fmt)
 _fsl_ssi_set_dai_fmt(ssi_private, ssi_private-dai_fmt);
 
 I find that in end of probe, there is setting of dai_fmt. Can we remove this?
 because this setting need to enable ipg clock, and if ac97, ipg clock can't be
 disabled.

No you can't remove it. It is necessary for the DT property fsl,mode.
Most dts do not have this property anymore because the sound cards are
setting the dai-fmt. But there are still some powerpc dts files that
contain that property.

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


signature.asc
Description: Digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2014-09-10 Thread Nicolin Chen 
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
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2014-09-09 Thread li.xi...@freescale.com 
Hi,

 Subject: [PATCH V1] ASoC: fsl_ssi: refine ipg clock usage in this module
 
 Move the ipg clock enable and disable operation to startup and shutdown,
 that is only enable ipg clock when ssi is working. we don't need to enable
 ipg clock in probe.
 Another register accessing need the ipg clock, so use
 devm_regmap_init_mmio_clk
 instead of devm_regmap_init_mmio.


You should split this into two separate patches, which will be more
Easy to be reviewed.
 
Thanks,

BRs
Xiubo

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2014-09-09 Thread Mark Brown 
On Tue, Sep 09, 2014 at 05:18:07PM +0800, Shengjiu Wang wrote:

 + if (ssi_private-soc-imx)
 + clk_prepare_enable(ssi_private-clk);
 +

We're ignoring the error code here.

 - ssi_private-clk = devm_clk_get(pdev-dev, NULL);
 + ssi_private-clk = devm_clk_get(pdev-dev, ipg);
   if (IS_ERR(ssi_private-clk)) {
   ret = PTR_ERR(ssi_private-clk);
 - dev_err(pdev-dev, could not get clock: %d\n, ret);
 - return ret;

Why is this change being made?  It wasn't mentioned in the commit log
and doesn't seem relevant to moving where the enable and disable are
done which is what the patch is supposed to be doing...

Please don't make different changes in the same patch.


signature.asc
Description: Digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2014-09-09 Thread Timur Tabi 

Shengjiu Wang wrote:

+   if (ssi_private-soc-imx)
+   clk_prepare_enable(ssi_private-clk);


How about this instead?

if (ssi_private-clk)
clk_prepare_enable(ssi_private-clk);

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2014-09-09 Thread Mark Brown 
On Tue, Sep 09, 2014 at 08:17:51AM -0500, Timur Tabi wrote:
 Shengjiu Wang wrote:
 +if (ssi_private-soc-imx)
 +clk_prepare_enable(ssi_private-clk);

 How about this instead?

 if (ssi_private-clk)
   clk_prepare_enable(ssi_private-clk);

Should be a !IS_ERR() - NULL is a valid clock.


signature.asc
Description: Digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2014-09-09 Thread Timur Tabi 

On 09/09/2014 10:21 AM, Mark Brown wrote:

if (ssi_private-clk)
clk_prepare_enable(ssi_private-clk);



Should be a !IS_ERR() - NULL is a valid clock.


In that case, ssi_private-clk needs to be initialized to -EINVAL or 
something, so that the check works on systems that don't have any clocks.

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2014-09-09 Thread Nicolin Chen 
On Tue, Sep 09, 2014 at 12:27:50PM +0100, Mark Brown wrote:
 On Tue, Sep 09, 2014 at 05:18:07PM +0800, Shengjiu Wang wrote:
  -   ssi_private-clk = devm_clk_get(pdev-dev, NULL);
  +   ssi_private-clk = devm_clk_get(pdev-dev, ipg);
  if (IS_ERR(ssi_private-clk)) {
  ret = PTR_ERR(ssi_private-clk);
  -   dev_err(pdev-dev, could not get clock: %d\n, ret);
  -   return ret;
 
 Why is this change being made?  It wasn't mentioned in the commit log
 and doesn't seem relevant to moving where the enable and disable are
 done which is what the patch is supposed to be doing...

I think Shengjiu is trying to keep the clock disabled while SSI's idle.
The current driver enables ipg clock anyway even if there's no stream
running.

Apparently, these should be put into the comment log.

Thank you
Nicolin
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2014-09-09 Thread Mark Brown 
On Tue, Sep 09, 2014 at 11:03:10AM -0700, Nicolin Chen wrote:
 On Tue, Sep 09, 2014 at 12:27:50PM +0100, Mark Brown wrote:
  On Tue, Sep 09, 2014 at 05:18:07PM +0800, Shengjiu Wang wrote:
   - ssi_private-clk = devm_clk_get(pdev-dev, NULL);
   + ssi_private-clk = devm_clk_get(pdev-dev, ipg);

  Why is this change being made?  It wasn't mentioned in the commit log
  and doesn't seem relevant to moving where the enable and disable are
  done which is what the patch is supposed to be doing...

 I think Shengjiu is trying to keep the clock disabled while SSI's idle.
 The current driver enables ipg clock anyway even if there's no stream
 running.

 Apparently, these should be put into the comment log.

I got that bit.  However as well as changing where the enable and
disable take place this is also changing from requesting a clock with a
NULL to requesting one called ipg.


signature.asc
Description: Digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2014-09-09 Thread Nicolin Chen 
On Tue, Sep 09, 2014 at 07:15:16PM +0100, Mark Brown wrote:
 On Tue, Sep 09, 2014 at 11:03:10AM -0700, Nicolin Chen wrote:
  On Tue, Sep 09, 2014 at 12:27:50PM +0100, Mark Brown wrote:
   On Tue, Sep 09, 2014 at 05:18:07PM +0800, Shengjiu Wang wrote:
-   ssi_private-clk = devm_clk_get(pdev-dev, NULL);
+   ssi_private-clk = devm_clk_get(pdev-dev, ipg);
 
   Why is this change being made?  It wasn't mentioned in the commit log
   and doesn't seem relevant to moving where the enable and disable are
   done which is what the patch is supposed to be doing...
 
  I think Shengjiu is trying to keep the clock disabled while SSI's idle.
  The current driver enables ipg clock anyway even if there's no stream
  running.
 
  Apparently, these should be put into the comment log.
 
 I got that bit.  However as well as changing where the enable and
 disable take place this is also changing from requesting a clock with a
 NULL to requesting one called ipg.

Understood. Making one patch do one single change is the rule we should
always follow.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2014-09-09 Thread Timur Tabi 

On 09/09/2014 01:38 PM, Nicolin Chen wrote:

make sure to have the call for imx only because it seems that
the other platforms do not depend on the clock.


Although I doubt anyone will every add support for clocks to PowerPC 
side of this driver, I would prefer to avoid IMX-specific changes. 
Instead, the code should check if a clock is available.  That's why I 
suggested this change:


-   if (ssi_private-soc-imx)
+   if (!IS_ERR(ssi_private-clk))
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2014-09-09 Thread Nicolin Chen 
On Tue, Sep 09, 2014 at 02:37:42PM -0500, Timur Tabi wrote:
 On 09/09/2014 01:38 PM, Nicolin Chen wrote:
 make sure to have the call for imx only because it seems that
 the other platforms do not depend on the clock.
 
 Although I doubt anyone will every add support for clocks to PowerPC side
 of this driver, I would prefer to avoid IMX-specific changes. Instead, the
 code should check if a clock is available.  That's why I suggested this
 change:
 
 - if (ssi_private-soc-imx)
 + if (!IS_ERR(ssi_private-clk))

Hmm I think the following change may be better?

probe() {

+   /*
+* Initially mark the clock to NULL for all platforms so that later
+* clk_prepare_enable() will ignore and return 0 for non-clock cases.
+*/
+   ssi_private-clk = NULL;
.
fsl_ssi_imx_probe();
}

In this way, all platforms, not confined to imx any more, will be able
to call clk_prepare_enable(). Then we don't need an extra platform check
before calling it.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2014-09-09 Thread Timur Tabi 

On 09/09/2014 02:59 PM, Nicolin Chen wrote:

+   /*
+* Initially mark the clock to NULL for all platforms so that later
+* clk_prepare_enable() will ignore and return 0 for non-clock cases.
+*/
+   ssi_private-clk = NULL;


According to Mark, NULL is a valid clock, so this should be instead:

ssi_private-clk = PTR_ERR(-EINVAL);

although that doesn't sit well with me.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2014-09-09 Thread Nicolin Chen 
On Tue, Sep 09, 2014 at 03:03:53PM -0500, Timur Tabi wrote:
 On 09/09/2014 02:59 PM, Nicolin Chen wrote:
 +/*
 + * Initially mark the clock to NULL for all platforms so that later
 + * clk_prepare_enable() will ignore and return 0 for non-clock cases.
 + */
 +ssi_private-clk = NULL;
 
 According to Mark, NULL is a valid clock, so this should be instead:
 
   ssi_private-clk = PTR_ERR(-EINVAL);
 
 although that doesn't sit well with me.

I guess Mark's comment is merely against the check for clk validation
because if talking about clk validation, we should check IS_ERR(clk)
rather than check !=NULL directly.

However, my approach doesn't need any check. The open() or pm_resume()
can just call clk_prepare_enable() directly. The __clk_enable() will
then handle the 'clk == NULL' case:

static int __clk_enable(struct clk *clk)
{
int ret = 0;

if (!clk)
return 0;
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2014-09-09 Thread Timur Tabi 

On 09/09/2014 03:27 PM, Nicolin Chen wrote:

I guess Mark's comment is merely against the check for clk validation
because if talking about clk validation, we should check IS_ERR(clk)
rather than check !=NULL directly.


Ah, that makes sense now.


However, my approach doesn't need any check. The open() or pm_resume()
can just call clk_prepare_enable() directly. The __clk_enable() will
then handle the 'clk == NULL' case:


Yes, I was thinking the same thing.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2014-09-09 Thread Nicolin Chen 
On Tue, Sep 09, 2014 at 03:37:26PM -0500, Timur Tabi wrote:
 However, my approach doesn't need any check. The open() or pm_resume()
 can just call clk_prepare_enable() directly. The __clk_enable() will
 then handle the 'clk == NULL' case:
 
 Yes, I was thinking the same thing.

Because that's following your suggestion :)

@Shengjiu
Another thing I forgot to mention is we still need a return check for
clk_prepare_enable() which isn't in the current version. And I said
doesn't need any check is indicating the pre-check of the call.

Thank you
Nicolin
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev