Re: [alsa-devel] FW: [PATCH v3] ASoC: Add support for TI LM49453 Audio codec
-----Original Message----- From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] Sent: Tuesday, February 07, 2012 10:34 PM To: Reddy, MR Swami Cc: Girdwood, Liam; alsa-devel@alsa-project.org Subject: Re: [PATCH v3] ASoC: Add support for TI LM49453 Audio codec
On Mon, Feb 06, 2012 at 06:20:26AM -0800, Reddy, MR Swami wrote:
Changes made in v3: o Updated the lm49453_set_dai_pll() as per review comments in v2 patch. o Removed pll disable code in _set_dai_pll().
This doesn't really seem to address the issue at all - you still have the problems with the set_pll() function not doing anything it's supposed to do with the input and output frequencies, and now there's no way to disable the PLL.
OK. So its ideal to remove the _set_dai_pll(), as its not doing anything here.
Thanks Swami
On Wed, 2012-02-08 at 14:40 +0530, M R Swami Reddy wrote:
-----Original Message----- From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] Sent: Tuesday, February 07, 2012 10:34 PM To: Reddy, MR Swami Cc: Girdwood, Liam; alsa-devel@alsa-project.org Subject: Re: [PATCH v3] ASoC: Add support for TI LM49453 Audio codec
On Mon, Feb 06, 2012 at 06:20:26AM -0800, Reddy, MR Swami wrote:
Changes made in v3: o Updated the lm49453_set_dai_pll() as per review comments in v2 patch. o Removed pll disable code in _set_dai_pll().
This doesn't really seem to address the issue at all - you still have the problems with the set_pll() function not doing anything it's supposed to do with the input and output frequencies, and now there's no way to disable the PLL.
OK. So its ideal to remove the _set_dai_pll(), as its not doing anything here.
The set_pll() function is meant to take the input and output frequencies passed in as parameters and then use these to configure the PLL. If the output frequency is 0 then you should switch OFF your PLL to conserve power.
Your function is just enabling the PLLs. Where do you configure the PLL dividers (to divide the input frequency into the output frequency)? Where do you switch the PLL off when it's not in use ?
Liam
On Wed, 2012-02-08 at 09:18 +0000, Liam Girdwood wrote:
The set_pll() function is meant to take the input and output frequencies passed in as parameters and then use these to configure the PLL. If the output frequency is 0 then you should switch OFF your PLL to conserve power.
Your function is just enabling the PLLs. Where do you configure the PLL dividers (to divide the input frequency into the output frequency)? Where do you switch the PLL off when it's not in use ?
A different question...
how does a codec driver ensure that input clock is ON. Codec doesn't know anything about platform so shouldn't there be a callback to machine/platform to turn on the input clock?
On Wed, 2012-02-08 at 15:07 +0530, Vinod Koul wrote:
On Wed, 2012-02-08 at 09:18 +0000, Liam Girdwood wrote:
The set_pll() function is meant to take the input and output frequencies passed in as parameters and then use these to configure the PLL. If the output frequency is 0 then you should switch OFF your PLL to conserve power.
Your function is just enabling the PLLs. Where do you configure the PLL dividers (to divide the input frequency into the output frequency)? Where do you switch the PLL off when it's not in use ?
A different question...
how does a codec driver ensure that input clock is ON. Codec doesn't know anything about platform so shouldn't there be a callback to machine/platform to turn on the input clock?
Atm, the machine driver would have to enable any system clocks before the codec driver could use them.
However, I think one of the intentions of the clk framework rework is to allow non CPU clocks like CODEC PLLs to register as clocks and have their dependencies worked out. I've no idea of the status of this atm, but maybe Mark knows or you could ask on lkml.
Liam
On Wed, 2012-02-08 at 09:54 +0000, Liam Girdwood wrote:
Atm, the machine driver would have to enable any system clocks before the codec driver could use them.
But how would machine driver know _when_?
We want to dynamical turn off this clock when not is use and turn on only when codec is using. So I was thinking of turning this ON in codecs .set_bias_level callback, but how to propagate this into machine driver?
On Wed, 2012-02-08 at 16:06 +0530, Vinod Koul wrote:
On Wed, 2012-02-08 at 09:54 +0000, Liam Girdwood wrote:
Atm, the machine driver would have to enable any system clocks before the codec driver could use them.
But how would machine driver know _when_?
The machine driver gets all the PCM ops that the codec driver does and is responsible for configuring CODEC PLLs and clocks (usually in it's hw_params()).
We want to dynamical turn off this clock when not is use and turn on only when codec is using. So I was thinking of turning this ON in codecs .set_bias_level callback, but how to propagate this into machine driver?
It's usually done in the machine driver hw_params() since at this point we know the rate, format etc that can have a bearing on the clock configuration. You could switch it OFF in machine hw_free().
Liam
On Wed, 2012-02-08 at 10:57 +0000, Liam Girdwood wrote:
On Wed, 2012-02-08 at 16:06 +0530, Vinod Koul wrote:
On Wed, 2012-02-08 at 09:54 +0000, Liam Girdwood wrote:
Atm, the machine driver would have to enable any system clocks before the codec driver could use them.
But how would machine driver know _when_?
The machine driver gets all the PCM ops that the codec driver does and is responsible for configuring CODEC PLLs and clocks (usually in it's hw_params()).
We want to dynamical turn off this clock when not is use and turn on only when codec is using. So I was thinking of turning this ON in codecs .set_bias_level callback, but how to propagate this into machine driver?
It's usually done in the machine driver hw_params() since at this point we know the rate, format etc that can have a bearing on the clock configuration. You could switch it OFF in machine hw_free().
For DAIs yes. But I also have a Vibra controller on codec, which can be configured and turned On using alsa controls. I treat the Vibra as input (or as recoently as Mark has done a signal generator) so mixer connections ensure loopback from Input to Output and my codec is powered ON. Those cases DAI doesn't help me.
Possibly a .set_machine_clock_input callback which can be called from codec driver in .set_bias_level callback?
On Wed, Feb 08, 2012 at 04:47:00PM +0530, Vinod Koul wrote:
Possibly a .set_machine_clock_input callback which can be called from codec driver in .set_bias_level callback?
The machine driver has set_bias_level() callbacks of its own.
On Wed, 2012-02-08 at 11:21 +0000, Mark Brown wrote:
On Wed, Feb 08, 2012 at 04:47:00PM +0530, Vinod Koul wrote:
Possibly a .set_machine_clock_input callback which can be called from codec driver in .set_bias_level callback?
The machine driver has set_bias_level() callbacks of its own.
Thanks, _exactly_ what I needed :)
On Wed, Feb 08, 2012 at 10:57:30AM +0000, Liam Girdwood wrote:
On Wed, 2012-02-08 at 16:06 +0530, Vinod Koul wrote:
We want to dynamical turn off this clock when not is use and turn on only when codec is using. So I was thinking of turning this ON in codecs .set_bias_level callback, but how to propagate this into machine driver?
It's usually done in the machine driver hw_params() since at this point we know the rate, format etc that can have a bearing on the clock configuration. You could switch it OFF in machine hw_free().
For modern devices set_bias_level() is often a better choice if you're only going to do one of that or hw_params(), especially on the shutdown paths - devices these days typically need the clocks for a lot more stuff. hw_params() by itself obviously has issues for analogue only paths too.
I'd generally expect to see new machine drivers doing this stuff in set_bias_level() and possibly also having additional startup code in hw_params() depending on how flexible the clocking is.
On Wed, 2012-02-08 at 11:20 +0000, Mark Brown wrote:
On Wed, Feb 08, 2012 at 10:57:30AM +0000, Liam Girdwood wrote:
On Wed, 2012-02-08 at 16:06 +0530, Vinod Koul wrote:
We want to dynamical turn off this clock when not is use and turn on only when codec is using. So I was thinking of turning this ON in codecs .set_bias_level callback, but how to propagate this into machine driver?
It's usually done in the machine driver hw_params() since at this point we know the rate, format etc that can have a bearing on the clock configuration. You could switch it OFF in machine hw_free().
For modern devices set_bias_level() is often a better choice if you're only going to do one of that or hw_params(), especially on the shutdown paths - devices these days typically need the clocks for a lot more stuff. hw_params() by itself obviously has issues for analogue only paths too.
I'd generally expect to see new machine drivers doing this stuff in set_bias_level() and possibly also having additional startup code in hw_params() depending on how flexible the clocking is.
The scope of set_bias_level() is now starting to increase to more than just the card bias power. It may be worthwhile at some point to rename it to cover all possibilities.
Liam
On Wed, Feb 08, 2012 at 01:27:30PM +0000, Liam Girdwood wrote:
On Wed, 2012-02-08 at 11:20 +0000, Mark Brown wrote:
I'd generally expect to see new machine drivers doing this stuff in set_bias_level() and possibly also having additional startup code in hw_params() depending on how flexible the clocking is.
The scope of set_bias_level() is now starting to increase to more than just the card bias power. It may be worthwhile at some point to rename it to cover all possibilities.
I'm really not sure it's worth bothering going round and changing everything, and obviously the main candidate name is power which I'd worry is just going to confuse people even more into thinking they don't need to bother with DAPM when really it'd be helpful to made DAPM mandatory.
For many of the things where it's important to have clocks for longer the clocks are being used as part of the biasing - the main reason we need clocks more these days is for things like the charge pumps which are not a million miles off what VMID was on older CODECs.
On Wed, 2012-02-08 at 13:27 +0000, Liam Girdwood wrote:
On Wed, 2012-02-08 at 11:20 +0000, Mark Brown wrote:
On Wed, Feb 08, 2012 at 10:57:30AM +0000, Liam Girdwood wrote:
On Wed, 2012-02-08 at 16:06 +0530, Vinod Koul wrote:
We want to dynamical turn off this clock when not is use and turn on only when codec is using. So I was thinking of turning this ON in codecs .set_bias_level callback, but how to propagate this into machine driver?
It's usually done in the machine driver hw_params() since at this point we know the rate, format etc that can have a bearing on the clock configuration. You could switch it OFF in machine hw_free().
For modern devices set_bias_level() is often a better choice if you're only going to do one of that or hw_params(), especially on the shutdown paths - devices these days typically need the clocks for a lot more stuff. hw_params() by itself obviously has issues for analogue only paths too.
I'd generally expect to see new machine drivers doing this stuff in set_bias_level() and possibly also having additional startup code in hw_params() depending on how flexible the clocking is.
The scope of set_bias_level() is now starting to increase to more than just the card bias power. It may be worthwhile at some point to rename it to cover all possibilities.
Well not really. In order to power up codec and digital logic, we need to feed the input clock. So any activity which is required for this should be performed here. Remember this is not configuring the clock but to ensure that its turned ON so that codec can power up.
On Wed, Feb 08, 2012 at 09:54:37AM +0000, Liam Girdwood wrote:
However, I think one of the intentions of the clk framework rework is to allow non CPU clocks like CODEC PLLs to register as clocks and have their dependencies worked out. I've no idea of the status of this atm, but maybe Mark knows or you could ask on lkml.
It's going nowhere slowly. Seems unlikely for 3.4 at this point, there's been no activity for a month or two except for rmk getting annoyed at the lack of activity. The current block is that very few people are converting the new clk_prepare() APIs, though there's other problems we seem unable to get any traction on beyond that.
I rather suspect that the general lack of any end in sight is not helpful for motivation of random maintainers and none of the folks working on the core API are doing any of that legwork for them.
participants (4)
-
Liam Girdwood -
M R Swami Reddy -
Mark Brown -
Vinod Koul