[PATCH 1/2] ASoC: add api for dapm kcontrol configiuration

[PATCH 1/2] ASoC: add api for dapm kcontrol configiuration

[Vinod Koul]

From: Subhransu S. Prusty <subhransu.s.prusty@intel.com>

For DSPs we need to set and get the value for snd_kcontrol. This is currently
done by dapm_kcontrol_set/get_value, so create a wrapper
snd_soc_dapm_kcontrol_get/set_value APIs to let drivers use this

Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 include/sound/soc-dapm.h |    4 ++++
 sound/soc/soc-dapm.c     |   13 +++++++++++++
 2 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/include/sound/soc-dapm.h b/include/sound/soc-dapm.h
index 75020f5..9e826e9 100644
--- a/include/sound/soc-dapm.h
+++ b/include/sound/soc-dapm.h
@@ -368,6 +368,10 @@ int dapm_regulator_event(struct snd_soc_dapm_widget *w,
 			 struct snd_kcontrol *kcontrol, int event);
 int dapm_clock_event(struct snd_soc_dapm_widget *w,
 			 struct snd_kcontrol *kcontrol, int event);
+bool snd_soc_dapm_kcontrol_set_value(
+		const struct snd_kcontrol *kcontrol, unsigned int value);
+unsigned int snd_soc_dapm_kcontrol_get_value(
+		const struct snd_kcontrol *kcontrol);
 
 /* dapm controls */
 int snd_soc_dapm_put_volsw(struct snd_kcontrol *kcontrol,
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index 98c1dc6..b7dfa1a 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -333,6 +333,12 @@ static unsigned int dapm_kcontrol_get_value(const struct snd_kcontrol *kcontrol)
 
 	return data->value;
 }
+unsigned int snd_soc_dapm_kcontrol_get_value(
+	const struct snd_kcontrol *kcontrol)
+{
+	dapm_kcontrol_get_value(kcontrol);
+}
+EXPORT_SYMBOL_GPL(snd_soc_dapm_kcontrol_get_value);
 
 static bool dapm_kcontrol_set_value(const struct snd_kcontrol *kcontrol,
 	unsigned int value)
@@ -350,6 +356,13 @@ static bool dapm_kcontrol_set_value(const struct snd_kcontrol *kcontrol,
 	return true;
 }
 
+bool snd_soc_dapm_kcontrol_set_value(const struct snd_kcontrol *kcontrol,
+	unsigned int value)
+{
+	return dapm_kcontrol_set_value(kcontrol, value);
+}
+EXPORT_SYMBOL_GPL(snd_soc_dapm_kcontrol_set_value);
+
 /**
  * snd_soc_dapm_kcontrol_codec() - Returns the codec associated to a kcontrol
  * @kcontrol: The kcontrol
-- 
1.7.0.4

[PATCH 2/2] ASoC: dapm: Add a helper to get the platform for DAPM kcontrol

[Vinod Koul]

From: Subhransu S. Prusty <subhransu.s.prusty@intel.com>

commit eee5d7f9 introduced helper for getting codec for DAPM kcontrol, for DSPs
we need similar stuff for platforms as well

Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 include/sound/soc-dapm.h |    2 ++
 sound/soc/soc-dapm.c     |   11 +++++++++++
 2 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/include/sound/soc-dapm.h b/include/sound/soc-dapm.h
index 9e826e9..9f47dee 100644
--- a/include/sound/soc-dapm.h
+++ b/include/sound/soc-dapm.h
@@ -460,6 +460,8 @@ int snd_soc_dapm_dai_get_connected_widgets(struct snd_soc_dai *dai, int stream,
 	struct snd_soc_dapm_widget_list **list);
 
 struct snd_soc_codec *snd_soc_dapm_kcontrol_codec(struct snd_kcontrol *kcontrol);
+struct snd_soc_platform *snd_soc_dapm_kcontrol_platform(
+		struct snd_kcontrol *kcontrol);
 
 /* dapm widget types */
 enum snd_soc_dapm_type {
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index b7dfa1a..f37ca16 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -373,6 +373,17 @@ struct snd_soc_codec *snd_soc_dapm_kcontrol_codec(struct snd_kcontrol *kcontrol)
 }
 EXPORT_SYMBOL_GPL(snd_soc_dapm_kcontrol_codec);
 
+/**
+ * snd_soc_dapm_kcontrol_platform() - Returns the platform associated to a kcontrol
+ * @kcontrol: The kcontrol
+ */
+struct snd_soc_platform *snd_soc_dapm_kcontrol_platform(
+		struct snd_kcontrol *kcontrol)
+{
+	return dapm_kcontrol_get_wlist(kcontrol)->widgets[0]->platform;
+}
+EXPORT_SYMBOL_GPL(snd_soc_dapm_kcontrol_platform);
+
 static void dapm_reset(struct snd_soc_card *card)
 {
 	struct snd_soc_dapm_widget *w;
-- 
1.7.0.4

Re: [PATCH 2/2] ASoC: dapm: Add a helper to get the platform for DAPM kcontrol

[Jarkko Nikula]

On 05/26/2014 03:08 PM, Vinod Koul wrote:
> From: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
>
> commit eee5d7f9 introduced helper for getting codec for DAPM kcontrol, for DSPs
> we need similar stuff for platforms as well
>
Please refer not only the commit id but also commit title ("ASoC: dapm: 
Add a helper to get the CODEC for DAPM kcontrol").

Makes commit log more clear and helps cases where original commit is 
backported to older tree.

-- 
Jarkko

Re: [PATCH 2/2] ASoC: dapm: Add a helper to get the platform for DAPM kcontrol

[Vinod Koul]

On Mon, May 26, 2014 at 03:44:22PM +0300, Jarkko Nikula wrote:
> On 05/26/2014 03:08 PM, Vinod Koul wrote:
> >From: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> >
> >commit eee5d7f9 introduced helper for getting codec for DAPM kcontrol, for DSPs
> >we need similar stuff for platforms as well
> >
> Please refer not only the commit id but also commit title ("ASoC:
> dapm: Add a helper to get the CODEC for DAPM kcontrol").
> 
> Makes commit log more clear and helps cases where original commit is
> backported to older tree.
ah yes, my bad :(

Mark, do you want a patch or can edit the commit with title "ASoC: dapm: Add a
helper to get the CODEC for DAPM kcontrol" before applying, if no further
comments

-- 
~Vinod

Re: [PATCH 1/2] ASoC: add api for dapm kcontrol configiuration

[Lars-Peter Clausen]

On 05/26/2014 02:08 PM, Vinod Koul wrote:
> From: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
>
> For DSPs we need to set and get the value for snd_kcontrol. This is currently
> done by dapm_kcontrol_set/get_value, so create a wrapper
> snd_soc_dapm_kcontrol_get/set_value APIs to let drivers use this

Can you go a bit more into detail how you intend to use this?

> [...]
> +unsigned int snd_soc_dapm_kcontrol_get_value(
> +	const struct snd_kcontrol *kcontrol)
> +{
> +	dapm_kcontrol_get_value(kcontrol);

The compiler should have complained about this.

> +}
> +EXPORT_SYMBOL_GPL(snd_soc_dapm_kcontrol_get_value);

I'm also not sure how much sense it makes to have a single line wrapper 
function for exporting things? Why not just rename the original function and 
then export it?

Re: [PATCH 2/2] ASoC: dapm: Add a helper to get the platform for DAPM kcontrol

[Lars-Peter Clausen]

On 05/26/2014 02:08 PM, Vinod Koul wrote:
[...]
> + * snd_soc_dapm_kcontrol_platform() - Returns the platform associated to a kcontrol
> + * @kcontrol: The kcontrol
> + */
> +struct snd_soc_platform *snd_soc_dapm_kcontrol_platform(
> +		struct snd_kcontrol *kcontrol)
> +{
> +	return dapm_kcontrol_get_wlist(kcontrol)->widgets[0]->platform;
> +}
> +EXPORT_SYMBOL_GPL(snd_soc_dapm_kcontrol_platform);

This conflicts with the series that moves DAPM support to the component 
level [1].

- Lars

[1] http://mailman.alsa-project.org/pipermail/alsa-devel/2014-May/076718.html

Re: [PATCH 2/2] ASoC: dapm: Add a helper to get the platform for DAPM kcontrol

[Mark Brown]

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

On Mon, May 26, 2014 at 06:21:46PM +0530, Vinod Koul wrote:

> Mark, do you want a patch or can edit the commit with title "ASoC: dapm: Add a
> helper to get the CODEC for DAPM kcontrol" before applying, if no further
> comments

It's fine, I can edit.

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

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

Re: [PATCH 1/2] ASoC: add api for dapm kcontrol configiuration

[Mark Brown]

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

On Mon, May 26, 2014 at 05:38:55PM +0530, Vinod Koul wrote:
> From: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> 
> For DSPs we need to set and get the value for snd_kcontrol. This is currently
> done by dapm_kcontrol_set/get_value, so create a wrapper
> snd_soc_dapm_kcontrol_get/set_value APIs to let drivers use this

Can you provide any more detail on this than just simply stating that
this is needed for DSPs - why do DSPs need it, what are they going to do
with it?

>  }
> +unsigned int snd_soc_dapm_kcontrol_get_value(

Missing blank line.

> +	const struct snd_kcontrol *kcontrol)
> +{
> +	dapm_kcontrol_get_value(kcontrol);
> +}
> +EXPORT_SYMBOL_GPL(snd_soc_dapm_kcontrol_get_value);

This function has a return value but no return statement, I'm surprised
the compiler didn't tell you this.  To be honest I'm not sure why
dapm_kcontrol_get_value() wasn't just exported, the wrapper isn't adding
much and the function name is already so long that snd_soc_ isn't going
to hurt.

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

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

Re: [PATCH 1/2] ASoC: add api for dapm kcontrol configiuration

[Vinod Koul]

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

On Mon, May 26, 2014 at 04:05:07PM +0100, Mark Brown wrote:
> On Mon, May 26, 2014 at 05:38:55PM +0530, Vinod Koul wrote:
> > From: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > 
> > For DSPs we need to set and get the value for snd_kcontrol. This is currently
> > done by dapm_kcontrol_set/get_value, so create a wrapper
> > snd_soc_dapm_kcontrol_get/set_value APIs to let drivers use this
> 
> Can you provide any more detail on this than just simply stating that
> this is needed for DSPs - why do DSPs need it, what are they going to do
> with it?
Well,... planning to provide a patch too :)

For our DSPs we have mixers and we need to send messages to DSP on mixer enable
and disable by DAPM. So this fn helps us to get the value of widget and pass on
to the DSP.

The usage will be in my comming series for our DSP model using DAPM and DPCM.

> 
> >  }
> > +unsigned int snd_soc_dapm_kcontrol_get_value(
> 
> Missing blank line.
> 
> > +	const struct snd_kcontrol *kcontrol)
> > +{
> > +	dapm_kcontrol_get_value(kcontrol);
> > +}
> > +EXPORT_SYMBOL_GPL(snd_soc_dapm_kcontrol_get_value);
> 
> This function has a return value but no return statement, I'm surprised
> the compiler didn't tell you this.  To be honest I'm not sure why
> dapm_kcontrol_get_value() wasn't just exported, the wrapper isn't adding
> much and the function name is already so long that snd_soc_ isn't going
> to hurt.
Looks like patch got mangled, I will fix it up.

And agree to both your and Lar's comment that we dont need a wrapper so will
export the current funtion.

Thanks
-- 
~Vinod

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

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

Re: [PATCH 2/2] ASoC: dapm: Add a helper to get the platform for DAPM kcontrol

[Vinod Koul]

On Mon, May 26, 2014 at 03:29:03PM +0200, Lars-Peter Clausen wrote:
> On 05/26/2014 02:08 PM, Vinod Koul wrote:
> [...]
> >+ * snd_soc_dapm_kcontrol_platform() - Returns the platform associated to a kcontrol
> >+ * @kcontrol: The kcontrol
> >+ */
> >+struct snd_soc_platform *snd_soc_dapm_kcontrol_platform(
> >+		struct snd_kcontrol *kcontrol)
> >+{
> >+	return dapm_kcontrol_get_wlist(kcontrol)->widgets[0]->platform;
> >+}
> >+EXPORT_SYMBOL_GPL(snd_soc_dapm_kcontrol_platform);
> 
> This conflicts with the series that moves DAPM support to the
> component level [1].

Thanks for the pointer, has this series been merged?
Dont see it in topic/core in Mark's tree.

I will take a look but fwiw this statement is not entirely true in the cover
letter of patch:

"This will allow any component to have DAPM widgets and routes, which was
previously only possible for CODECs, and will allow any component to have DAPM
widgets with controls (i.e. Mixers and MUXs), which was previously only possible
for CODECs"

I am already running a system which models platform and has Mixers,
Muxes and works fine (tested on 3.10 and 3.14).

Few bits of code is in RFC I sent earlier and will post these in detail over
next few weeks
> 
> - Lars
> 
> [1] http://mailman.alsa-project.org/pipermail/alsa-devel/2014-May/076718.html
> 

-- 
~Vinod

Re: [PATCH 2/2] ASoC: dapm: Add a helper to get the platform for DAPM kcontrol

[Vinod Koul]

On Mon, May 26, 2014 at 06:52:28PM +0200, Lars-Peter Clausen wrote:
> On 05/26/2014 06:23 PM, Vinod Koul wrote:
> >On Mon, May 26, 2014 at 03:29:03PM +0200, Lars-Peter Clausen wrote:
> >>On 05/26/2014 02:08 PM, Vinod Koul wrote:
> >>[...]
> >>>+ * snd_soc_dapm_kcontrol_platform() - Returns the platform associated to a kcontrol
> >>>+ * @kcontrol: The kcontrol
> >>>+ */
> >>>+struct snd_soc_platform *snd_soc_dapm_kcontrol_platform(
> >>>+		struct snd_kcontrol *kcontrol)
> >>>+{
> >>>+	return dapm_kcontrol_get_wlist(kcontrol)->widgets[0]->platform;
> >>>+}
> >>>+EXPORT_SYMBOL_GPL(snd_soc_dapm_kcontrol_platform);
> >>
> >>This conflicts with the series that moves DAPM support to the
> >>component level [1].
> >
> >Thanks for the pointer, has this series been merged?
> >Dont see it in topic/core in Mark's tree.
> 
> It hasn't been merged yet.
Okay, what is the state of the series? Would it merged soonish? If this is
approach going to be taken then we need to plan for intercepting this

> >
> >I will take a look but fwiw this statement is not entirely true in the cover
> >letter of patch:
> >
> >"This will allow any component to have DAPM widgets and routes, which was
> >previously only possible for CODECs, and will allow any component to have DAPM
> >widgets with controls (i.e. Mixers and MUXs), which was previously only possible
> >for CODECs"
> >
> >I am already running a system which models platform and has Mixers,
> >Muxes and works fine (tested on 3.10 and 3.14).
> 
> It works if you use custom controls. But it does not work with the
> standard SOC_DAPM_* controls.
Not sure as we are using SOC_DAPM but yes with *_E versions and our own get/put
handlers with additional dapm event handlers.
I have also tested with a version which used non _E versions and only dapm event
handlers. So not sure where you found it difficult. I was able to model a fiarly
complex DSP and able to do both playback as well as loopback tests.

We haven't wrriten our own SOC_DAPM_* controls yet.

> >Few bits of code is in RFC I sent earlier and will post these in detail over
> >next few weeks
> 
> Do you think it is necessary that these two patches get merged
> before you send the other patches? It would be good to see things in
> context. That will make it easier to properly review patch 1 of this
> series.
Actually yes, as our mixer/mux get and put handlers would need to find the
platform pointer as well as set and get the values.

-- 
~Vinod

Re: [PATCH 2/2] ASoC: dapm: Add a helper to get the platform for DAPM kcontrol

[Lars-Peter Clausen]

On 05/26/2014 06:23 PM, Vinod Koul wrote:
> On Mon, May 26, 2014 at 03:29:03PM +0200, Lars-Peter Clausen wrote:
>> On 05/26/2014 02:08 PM, Vinod Koul wrote:
>> [...]
>>> + * snd_soc_dapm_kcontrol_platform() - Returns the platform associated to a kcontrol
>>> + * @kcontrol: The kcontrol
>>> + */
>>> +struct snd_soc_platform *snd_soc_dapm_kcontrol_platform(
>>> +		struct snd_kcontrol *kcontrol)
>>> +{
>>> +	return dapm_kcontrol_get_wlist(kcontrol)->widgets[0]->platform;
>>> +}
>>> +EXPORT_SYMBOL_GPL(snd_soc_dapm_kcontrol_platform);
>>
>> This conflicts with the series that moves DAPM support to the
>> component level [1].
>
> Thanks for the pointer, has this series been merged?
> Dont see it in topic/core in Mark's tree.

It hasn't been merged yet.

>
> I will take a look but fwiw this statement is not entirely true in the cover
> letter of patch:
>
> "This will allow any component to have DAPM widgets and routes, which was
> previously only possible for CODECs, and will allow any component to have DAPM
> widgets with controls (i.e. Mixers and MUXs), which was previously only possible
> for CODECs"
>
> I am already running a system which models platform and has Mixers,
> Muxes and works fine (tested on 3.10 and 3.14).

It works if you use custom controls. But it does not work with the standard 
SOC_DAPM_* controls.

>
> Few bits of code is in RFC I sent earlier and will post these in detail over
> next few weeks

Do you think it is necessary that these two patches get merged before you 
send the other patches? It would be good to see things in context. That will 
make it easier to properly review patch 1 of this series.

- Lars

Re: [PATCH 2/2] ASoC: dapm: Add a helper to get the platform for DAPM kcontrol

[Lars-Peter Clausen]

On 05/26/2014 06:47 PM, Vinod Koul wrote:
> On Mon, May 26, 2014 at 06:52:28PM +0200, Lars-Peter Clausen wrote:
>> On 05/26/2014 06:23 PM, Vinod Koul wrote:
>>> On Mon, May 26, 2014 at 03:29:03PM +0200, Lars-Peter Clausen wrote:
>>>> On 05/26/2014 02:08 PM, Vinod Koul wrote:
>>>> [...]
>>>>> + * snd_soc_dapm_kcontrol_platform() - Returns the platform associated to a kcontrol
>>>>> + * @kcontrol: The kcontrol
>>>>> + */
>>>>> +struct snd_soc_platform *snd_soc_dapm_kcontrol_platform(
>>>>> +		struct snd_kcontrol *kcontrol)
>>>>> +{
>>>>> +	return dapm_kcontrol_get_wlist(kcontrol)->widgets[0]->platform;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(snd_soc_dapm_kcontrol_platform);
>>>>
>>>> This conflicts with the series that moves DAPM support to the
>>>> component level [1].
>>>
>>> Thanks for the pointer, has this series been merged?
>>> Dont see it in topic/core in Mark's tree.
>>
>> It hasn't been merged yet.
> Okay, what is the state of the series? Would it merged soonish?

Maybe, depends on how well the review goes. But it doesn't really matter which 
patch is merged first, we just need to be aware that the other one needs to be 
reworked and rebased on top of it.

> If this is approach going to be taken then we need to plan for intercepting this
>
>>>
>>> I will take a look but fwiw this statement is not entirely true in the cover
>>> letter of patch:
>>>
>>> "This will allow any component to have DAPM widgets and routes, which was
>>> previously only possible for CODECs, and will allow any component to have DAPM
>>> widgets with controls (i.e. Mixers and MUXs), which was previously only possible
>>> for CODECs"
>>>
>>> I am already running a system which models platform and has Mixers,
>>> Muxes and works fine (tested on 3.10 and 3.14).
>>
>> It works if you use custom controls. But it does not work with the
>> standard SOC_DAPM_* controls.
> Not sure as we are using SOC_DAPM but yes with *_E versions and our own get/put
> handlers with additional dapm event handlers.
> I have also tested with a version which used non _E versions and only dapm event
> handlers. So not sure where you found it difficult. I was able to model a fiarly
> complex DSP and able to do both playback as well as loopback tests.
>
> We haven't wrriten our own SOC_DAPM_* controls yet.

The standard DAPM get/put handlers will crash if you use them with a platform.

>
>>> Few bits of code is in RFC I sent earlier and will post these in detail over
>>> next few weeks
>>
>> Do you think it is necessary that these two patches get merged
>> before you send the other patches? It would be good to see things in
>> context. That will make it easier to properly review patch 1 of this
>> series.
> Actually yes, as our mixer/mux get and put handlers would need to find the
> platform pointer as well as set and get the values.
>

Yes, but can these two patches be sent in the same series as the first user? 
That makes review easier.

Re: [PATCH 2/2] ASoC: dapm: Add a helper to get the platform for DAPM kcontrol

[Mark Brown]

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

On Mon, May 26, 2014 at 08:07:45PM +0200, Lars-Peter Clausen wrote:
> On 05/26/2014 06:47 PM, Vinod Koul wrote:

> >>It hasn't been merged yet.
> >Okay, what is the state of the series? Would it merged soonish?

> Maybe, depends on how well the review goes. But it doesn't really matter
> which patch is merged first, we just need to be aware that the other one
> needs to be reworked and rebased on top of it.

Probably not until after the merge window now; that should open next
week and it's a bit invasive to be doing right now.

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

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