[alsa-devel] What is correct way to put conditional stuff in ASoC codec driver?
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 || ----------------------------------------------------------------------------
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.
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 :-)
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?
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.
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.
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.
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.
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.
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.
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?
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.
participants (2)
-
Ashish Chavan -
Mark Brown