What is correct way to put conditional stuff in ASoC codec driver?

What is correct way to put conditional stuff in ASoC codec driver?

[Ashish Chavan]

Hi,
   I am working on updating ASoC codec driver for Dialog's DA7210 codec
(sound/soc/codec/da7210.c). This update would be a major functionality update in
nature. In fact we already have a feature complete driver for this codec but
there are two main issues with that driver,

(1) It was developed(and tested) for a quite old kernel version (2.6.28)
(2) It was written in custom(non standard) way and not suitable for direct 
submission to kernel

Our final goal is to pull in all missing features from this custom driver in to 
existing mainline driver. I have already created few patches for basic functions 
but I am bit confused at one point. Some of the features supported by this codec 
have inter dependency, e.g. ALC can be used only if NOISE SUPRESSION is disabled. 
So ideally, all controls related to ALC should be either disabled or not added at 
all, if NOISE SUPRESSION to be used. There are few other features having similar 
kind of dependency.

I just want to know what is the correct way to handle this in ASoC codec driver?
Looking at the existing codec drivers, it seems that having conditional defines 
is not common here. As We just want to support static configuration of such 
features, is it a good idea to add sub menu options in driver's Kconfig to 
enable/disable such features and use them within code?

Any pointer(s) to existing code are most welcome.


Thanks,

-- Ashish


----------------------------------------------------------------------------
|| Linux Is User Friendly, It's Just Selective About Who It's Friends Are ||
----------------------------------------------------------------------------

Re: What is correct way to put conditional stuff in ASoC codec driver?

[Mark Brown]

On Thu, Jun 30, 2011 at 06:16:30PM +0530, Ashish Chavan wrote:

Please fix your mailer to word wrap at less than 80 columns so your mail
is legible.

> Our final goal is to pull in all missing features from this custom
> driver in to existing mainline driver. I have already created few
> patches for basic functions but I am bit confused at one point. Some
> of the features supported by this codec have inter dependency, e.g.
> ALC can be used only if NOISE SUPRESSION is disabled.  So ideally, all
> controls related to ALC should be either disabled or not added at all,
> if NOISE SUPRESSION to be used. There are few other features having
> similar kind of dependency.

You'd need to implement custom controls for the relevant enables which
check to see what is currently enabled and prevents enables if there
are conflicts.  You should do this dynamically and I'd expect that only
the enables actually need to check anything, adjusting parameters for
things that aren't active is usually no problem.

> I just want to know what is the correct way to handle this in ASoC
> codec driver?  Looking at the existing codec drivers, it seems that
> having conditional defines is not common here. As We just want to
> support static configuration of such features, is it a good idea to
> add sub menu options in driver's Kconfig to enable/disable such
> features and use them within code?

Compile time selection is completely uinacceptable for Linux in general,
please see the coding style and development model stuff in Documentation
for discussion of the motivations for this.

Re: What is correct way to put conditional stuff in ASoC codec driver?

[Ashish Chavan]

On Thu, 2011-06-30 at 22:07 +0530, Mark Brown wrote:
> On Thu, Jun 30, 2011 at 06:16:30PM +0530, Ashish Chavan wrote:
> 
> Please fix your mailer to word wrap at less than 80 columns so your mail
> is legible.

Oops! I thought I already have it set in my evolution. Thanks for
pointing it out.

> 
> > Our final goal is to pull in all missing features from this custom
> > driver in to existing mainline driver. I have already created few
> > patches for basic functions but I am bit confused at one point. Some
> > of the features supported by this codec have inter dependency, e.g.
> > ALC can be used only if NOISE SUPRESSION is disabled.  So ideally, all
> > controls related to ALC should be either disabled or not added at all,
> > if NOISE SUPRESSION to be used. There are few other features having
> > similar kind of dependency.
> 
> You'd need to implement custom controls for the relevant enables which
> check to see what is currently enabled and prevents enables if there
> are conflicts.  You should do this dynamically and I'd expect that only
> the enables actually need to check anything, adjusting parameters for
> things that aren't active is usually no problem.

I see. That means it's ok to allow setting up values of five band
equalizers even when overall equalizer functionality is disabled.
Can you point me to any existing code that has such custom control(s)
which need to check for similar conditions? I am sure that many existing
codecs would have this kind of inter dependent functions.

> > I just want to know what is the correct way to handle this in ASoC
> > codec driver?  Looking at the existing codec drivers, it seems that
> > having conditional defines is not common here. As We just want to
> > support static configuration of such features, is it a good idea to
> > add sub menu options in driver's Kconfig to enable/disable such
> > features and use them within code?
> 
> Compile time selection is completely uinacceptable for Linux in general,
> please see the coding style and development model stuff in Documentation
> for discussion of the motivations for this.

Yes, guessed this. In fact that's why I posted the query here :-)

Re: What is correct way to put conditional stuff in ASoC codec driver?

[Mark Brown]

On Fri, Jul 01, 2011 at 02:03:25PM +0530, Ashish Chavan wrote:
> On Thu, 2011-06-30 at 22:07 +0530, Mark Brown wrote:

> > You'd need to implement custom controls for the relevant enables which
> > check to see what is currently enabled and prevents enables if there
> > are conflicts.  You should do this dynamically and I'd expect that only
> > the enables actually need to check anything, adjusting parameters for
> > things that aren't active is usually no problem.

> I see. That means it's ok to allow setting up values of five band
> equalizers even when overall equalizer functionality is disabled.
> Can you point me to any existing code that has such custom control(s)
> which need to check for similar conditions? I am sure that many existing
> codecs would have this kind of inter dependent functions.

This requirement is actually fairly unusual, but there's plenty of
drivers with open coded controls for various reasons.  Have you tried
looking at the existing drivers in mainline?

Re: What is correct way to put conditional stuff in ASoC codec driver?

[Ashish Chavan]

On Fri, 2011-07-01 at 21:47 +0530, Mark Brown wrote:
> On Fri, Jul 01, 2011 at 02:03:25PM +0530, Ashish Chavan wrote:
> > On Thu, 2011-06-30 at 22:07 +0530, Mark Brown wrote:
> 
> > > You'd need to implement custom controls for the relevant enables which
> > > check to see what is currently enabled and prevents enables if there
> > > are conflicts.  You should do this dynamically and I'd expect that only
> > > the enables actually need to check anything, adjusting parameters for
> > > things that aren't active is usually no problem.
> 
> > I see. That means it's ok to allow setting up values of five band
> > equalizers even when overall equalizer functionality is disabled.
> > Can you point me to any existing code that has such custom control(s)
> > which need to check for similar conditions? I am sure that many existing
> > codecs would have this kind of inter dependent functions.
> 
> This requirement is actually fairly unusual, but there's plenty of
> drivers with open coded controls for various reasons.  Have you tried
> looking at the existing drivers in mainline?
> 

Do you mean the requirement of having interdependent codec functionality
is unusual? If yes, then it is something different that what I expected
and I need to do some homework before asking further questions. By
"Homework" I roughly mean, 
(1) Going through data sheets of some of Wolfsons' codecs to find out if
they have similar inter dependent functional blocks.
(2) If something is found, have a detailed look at that codec's driver
in mainline. Find out how these functions are handled.

Do you think that this is the correct way to move forward?

I haven't done this exercise yet, mainly because I assumed that my
requirement is common and I could easily find a way to handle it from
existing drivers or experts!

In general, I have gone through some of the existing codec drivers in
mainline to get a feel of them but couldn't find something that handles
conditional part. May be I am missing something.

Re: What is correct way to put conditional stuff in ASoC codec driver?

[Mark Brown]

On Mon, Jul 04, 2011 at 12:26:50PM +0530, Ashish Chavan wrote:
> On Fri, 2011-07-01 at 21:47 +0530, Mark Brown wrote:

> > This requirement is actually fairly unusual, but there's plenty of
> > drivers with open coded controls for various reasons.  Have you tried
> > looking at the existing drivers in mainline?

> Do you mean the requirement of having interdependent codec functionality
> is unusual? If yes, then it is something different that what I expected

Yes, or at least the need to actually worry about it is.

> and I need to do some homework before asking further questions. By
> "Homework" I roughly mean, 

> (1) Going through data sheets of some of Wolfsons' codecs to find out if
> they have similar inter dependent functional blocks.

There should be no need to look at one particular vendor.

> (2) If something is found, have a detailed look at that codec's driver
> in mainline. Find out how these functions are handled.

I really don't understand the difficulties you are having here.  There
are many examples of drivers implementing custom handling for controls
in the kernel, some of which do things that sound fairly similar to what
you're doing (though not exactly the same thing mostly).

> I haven't done this exercise yet, mainly because I assumed that my
> requirement is common and I could easily find a way to handle it from
> existing drivers or experts!

So when you looked at the drivers what did you see?  I already told you
what you need to do:

| You'd need to implement custom controls for the relevant enables which
| check to see what is currently enabled and prevents enables if there
| are conflicts.  You should do this dynamically and I'd expect that only

All you're doing is trying to restrict the values that can be set on a
control dynamically.

> In general, I have gone through some of the existing codec drivers in
> mainline to get a feel of them but couldn't find something that handles
> conditional part. May be I am missing something.

Please be more concrete about what you looked at and what you didn't
find which you were expecting.  Any custom control should at least
include some bounds checking so it's very surprising you weren't able to
find any examples of how you could prevent the user setting invalid
settings.

Re: What is correct way to put conditional stuff in ASoC codec driver?

[Ashish Chavan]

On Tue, 2011-07-05 at 04:04 +0530, Mark Brown wrote:
> > Do you mean the requirement of having interdependent codec functionality
> > is unusual? If yes, then it is something different that what I expected
> 
> Yes, or at least the need to actually worry about it is.

OK, I will stop worrying about it as of now and concentrate on other
stuff :-)

> > (1) Going through data sheets of some of Wolfsons' codecs to find out if
> > they have similar inter dependent functional blocks.
> 
> There should be no need to look at one particular vendor.

I just selected Wolfson because their ASoC codec drivers are in majority
if I look at sound/soc/codecs dir. So in my opinion, it just raises the
probability of finding the required thing.

> | You'd need to implement custom controls for the relevant enables which
> | check to see what is currently enabled and prevents enables if there
> | are conflicts.  You should do this dynamically and I'd expect that only

Yes, I got what you meant here and after some homework also found the
example of this. You are trying to convey something similar to what is
done within "outmixer_event()" function of sound/soc/codecs/wm8991.c (at
lest the condition checking part), right?

> All you're doing is trying to restrict the values that can be set on a
> control dynamically.

I think, it is more about restricting access to a set of controls
dynamically based of enable/disable of some other control(s).

I guess now I have enough details to start off implementation part. I
will pop up a query again, if I am stuck somewhere.

BTW thanks for helping me out in getting things clear. 

Re: What is correct way to put conditional stuff in ASoC codec driver?

[Mark Brown]

On Tue, Jul 05, 2011 at 07:55:22PM +0530, Ashish Chavan wrote:
> On Tue, 2011-07-05 at 04:04 +0530, Mark Brown wrote:

> > | You'd need to implement custom controls for the relevant enables which
> > | check to see what is currently enabled and prevents enables if there
> > | are conflicts.  You should do this dynamically and I'd expect that only

> Yes, I got what you meant here and after some homework also found the
> example of this. You are trying to convey something similar to what is
> done within "outmixer_event()" function of sound/soc/codecs/wm8991.c (at
> lest the condition checking part), right?

No, that's not a user visible control that's part of the internal DAPM
power management stuff.  You're looking for struct snd_kcontrol_new
stuff.

> > All you're doing is trying to restrict the values that can be set on a
> > control dynamically.

> I think, it is more about restricting access to a set of controls
> dynamically based of enable/disable of some other control(s).

The state of another control is one example of a dynamic source of
information.

Re: What is correct way to put conditional stuff in ASoC codec driver?

[Ashish Chavan]

On Wed, 2011-07-06 at 01:07 +0530, Mark Brown wrote:

> No, that's not a user visible control that's part of the internal DAPM
> power management stuff.  You're looking for struct snd_kcontrol_new
> stuff.

Oh, that's correct. You may be suggesting something similar to what is
done in sound/soc/codecs/wm8985.c for "Equalizer Function", i.e. using
SOC_xxx_EXT version for defining custom control and putting condition
checks in respective "_put()" function.

> The state of another control is one example of a dynamic source of
> information.

Can you point me to any example that shows correct way to inquire
state/value of a custom control? I guess snd_soc_get_xxx() functions
need to be used here but I could find only handful of usages of these
functions in entire sound/soc/codecs dir.

Re: What is correct way to put conditional stuff in ASoC codec driver?

[Mark Brown]

On Thu, Jul 07, 2011 at 05:20:07PM +0530, Ashish Chavan wrote:
> On Wed, 2011-07-06 at 01:07 +0530, Mark Brown wrote:

> > No, that's not a user visible control that's part of the internal DAPM
> > power management stuff.  You're looking for struct snd_kcontrol_new
> > stuff.

> Oh, that's correct. You may be suggesting something similar to what is
> done in sound/soc/codecs/wm8985.c for "Equalizer Function", i.e. using
> SOC_xxx_EXT version for defining custom control and putting condition
> checks in respective "_put()" function.

Or just writing a control directly.

> > The state of another control is one example of a dynamic source of
> > information.

> Can you point me to any example that shows correct way to inquire
> state/value of a custom control? I guess snd_soc_get_xxx() functions
> need to be used here but I could find only handful of usages of these
> functions in entire sound/soc/codecs dir.

Those functions are used by the core for providing readback of the
controls to userspace.  You need to provide them but your driver can use
whatever it likes to read its own state (providing it's tasteful), it
doesn't need to go through external APIs.  Any driver with a custom
control will have an example of how it chooses to read its own state.

Re: What is correct way to put conditional stuff in ASoC codec driver?

[Ashish Chavan]

On Thu, 2011-07-07 at 21:19 +0530, Mark Brown wrote:
> On Thu, Jul 07, 2011 at 05:20:07PM +0530, Ashish Chavan wrote:

> > Oh, that's correct. You may be suggesting something similar to what is
> > done in sound/soc/codecs/wm8985.c for "Equalizer Function", i.e. using
> > SOC_xxx_EXT version for defining custom control and putting condition
> > checks in respective "_put()" function.
> 
> Or just writing a control directly.

I guess both of these methods are acceptable and there is no inherent
preference for one over another.
 
> Those functions are used by the core for providing readback of the
> controls to userspace.  You need to provide them but your driver can use
> whatever it likes to read its own state (providing it's tasteful), it
> doesn't need to go through external APIs.  Any driver with a custom
> control will have an example of how it chooses to read its own state.

I see. Thanks for the insight.

I am looking at SOC_DAPM_SINGLE_W in wm8903.c, SOC_WM8350_DOUBLE_R_TLV
in wm8350.c and SOC_TWL6040_DOUBLE_TLV in twl6040.c as reference
examples. Respective xxx_get_xxx() methods are what you are trying to
point me to, right?

Re: What is correct way to put conditional stuff in ASoC codec driver?

[Mark Brown]

On Mon, Jul 11, 2011 at 07:46:43PM +0530, Ashish Chavan wrote:
> On Thu, 2011-07-07 at 21:19 +0530, Mark Brown wrote:

> > Those functions are used by the core for providing readback of the
> > controls to userspace.  You need to provide them but your driver can use
> > whatever it likes to read its own state (providing it's tasteful), it
> > doesn't need to go through external APIs.  Any driver with a custom
> > control will have an example of how it chooses to read its own state.

> I see. Thanks for the insight.

> I am looking at SOC_DAPM_SINGLE_W in wm8903.c, SOC_WM8350_DOUBLE_R_TLV
> in wm8350.c and SOC_TWL6040_DOUBLE_TLV in twl6040.c as reference
> examples. Respective xxx_get_xxx() methods are what you are trying to
> point me to, right?

No.  Like I say in the text you quote above these are functions used to
report the control state to userspace and there's no reason why you'd
need to use those interfaces from within the driver itself.