Re: FW: [PATCH v3] ASoC: Add support for TI LM49453 Audio codec

Re: FW: [PATCH v3] ASoC: Add support for TI LM49453 Audio codec

[M R Swami Reddy]

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

Re: FW: [PATCH v3] ASoC: Add support for TI LM49453 Audio codec

[Liam Girdwood]

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

Re: FW: [PATCH v3] ASoC: Add support for TI LM49453 Audio codec

[Vinod Koul]

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?

-- 
~Vinod

Re: FW: [PATCH v3] ASoC: Add support for TI LM49453 Audio codec

[Liam Girdwood]

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

Re: FW: [PATCH v3] ASoC: Add support for TI LM49453 Audio codec

[Vinod Koul]

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?

-- 
~Vinod

Re: FW: [PATCH v3] ASoC: Add support for TI LM49453 Audio codec

[Liam Girdwood]

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

Re: FW: [PATCH v3] ASoC: Add support for TI LM49453 Audio codec

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 863 bytes --]

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.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: FW: [PATCH v3] ASoC: Add support for TI LM49453 Audio codec

[Vinod Koul]

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?


-- 
~Vinod

Re: FW: [PATCH v3] ASoC: Add support for TI LM49453 Audio codec

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1041 bytes --]

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.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: FW: [PATCH v3] ASoC: Add support for TI LM49453 Audio codec

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 240 bytes --]

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.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: FW: [PATCH v3] ASoC: Add support for TI LM49453 Audio codec

[Liam Girdwood]

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

Re: FW: [PATCH v3] ASoC: Add support for TI LM49453 Audio codec

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1059 bytes --]

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.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: FW: [PATCH v3] ASoC: Add support for TI LM49453 Audio codec

[Vinod Koul]

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

-- 
~Vinod

Re: FW: [PATCH v3] ASoC: Add support for TI LM49453 Audio codec

[Vinod Koul]

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.

-- 
~Vinod