[PATCH v2 1/2] ASoC: export dapm_kcontrol_set/get_value functions

[PATCH v2 1/2] ASoC: export dapm_kcontrol_set/get_value functions

[Vinod Koul]

The DSPs like Intel ones use the DPCM to represent the DSP toplogy. So when DAPM
triggers the widget, the DSP needs to know the kcontrol values and pass on to DSP
firmware, so export these for driver use

Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 include/sound/soc-dapm.h |    3 +++
 sound/soc/soc-dapm.c     |    6 ++++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/include/sound/soc-dapm.h b/include/sound/soc-dapm.h
index 75020f5..abda467 100644
--- a/include/sound/soc-dapm.h
+++ b/include/sound/soc-dapm.h
@@ -368,6 +368,9 @@ 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 dapm_kcontrol_set_value(const struct snd_kcontrol *kcontrol,
+			unsigned int value);
+unsigned int 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..d831e73 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -327,14 +327,15 @@ static struct list_head *dapm_kcontrol_get_path_list(
 	list_for_each_entry(path, dapm_kcontrol_get_path_list(kcontrol), \
 		list_kcontrol)
 
-static unsigned int dapm_kcontrol_get_value(const struct snd_kcontrol *kcontrol)
+unsigned int dapm_kcontrol_get_value(const struct snd_kcontrol *kcontrol)
 {
 	struct dapm_kcontrol_data *data = snd_kcontrol_chip(kcontrol);
 
 	return data->value;
 }
+EXPORT_SYMBOL_GPL(dapm_kcontrol_get_value);
 
-static bool dapm_kcontrol_set_value(const struct snd_kcontrol *kcontrol,
+bool dapm_kcontrol_set_value(const struct snd_kcontrol *kcontrol,
 	unsigned int value)
 {
 	struct dapm_kcontrol_data *data = snd_kcontrol_chip(kcontrol);
@@ -349,6 +350,7 @@ static bool dapm_kcontrol_set_value(const struct snd_kcontrol *kcontrol,
 
 	return true;
 }
+EXPORT_SYMBOL_GPL(dapm_kcontrol_set_value);
 
 /**
  * snd_soc_dapm_kcontrol_codec() - Returns the codec associated to a kcontrol
-- 
1.7.0.4

[PATCH v2 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 - "ASoC: dapm: Add a helper to get the CODEC for DAPM kcontrol"
introduced helper for getting codec for DAPM kcontrol. For DSPs we need similar
method to get the platform pointer while handling the widgets.

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 abda467..247cd5e 100644
--- a/include/sound/soc-dapm.h
+++ b/include/sound/soc-dapm.h
@@ -459,6 +459,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 d831e73..8a4a733 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -362,6 +362,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 v2 1/2] ASoC: export dapm_kcontrol_set/get_value functions

[Lars-Peter Clausen]

On 05/29/2014 12:03 PM, Vinod Koul wrote:
> The DSPs like Intel ones use the DPCM to represent the DSP toplogy. So when DAPM
> triggers the widget, the DSP needs to know the kcontrol values and pass on to DSP
> firmware, so export these for driver use
>

As I said I feel a bit uneasy about exporting these and I'd rather much 
prefer to see this patch in context (i.e. in the same series as its user).

- Lars

Re: [PATCH v2 1/2] ASoC: export dapm_kcontrol_set/get_value functions

[Vinod Koul]

On Thu, May 29, 2014 at 12:55:07PM +0200, Lars-Peter Clausen wrote:
> On 05/29/2014 12:03 PM, Vinod Koul wrote:
> >The DSPs like Intel ones use the DPCM to represent the DSP toplogy. So when DAPM
> >triggers the widget, the DSP needs to know the kcontrol values and pass on to DSP
> >firmware, so export these for driver use
> >
> 
> As I said I feel a bit uneasy about exporting these and I'd rather
> much prefer to see this patch in context (i.e. in the same series as
> its user).
I am still working on that series. The core patches could have gone ahead so
posted.

Here is the code snippet that will come in subsequent series:

int sst_mix_put(struct snd_kcontrol *kcontrol,
        struct snd_ctl_elem_value *ucontrol)
{
        struct snd_soc_dapm_widget_list *wlist =
			dapm_kcontrol_get_wlist(kcontrol);
        struct snd_soc_dapm_widget *widget = wlist->widgets[0];
        struct soc_mixer_control *mc =
                (struct soc_mixer_control *)kcontrol->private_value;
        struct sst_data *sst = snd_soc_platform_get_drvdata(widget->platform);
        unsigned int mask = (1 << fls(mc->max)) - 1;
        unsigned int val;
        int connect;
        struct snd_soc_dapm_update update;

        val = sst_reg_write(sst, mc->reg, mc->shift, mc->max,
				ucontrol->value.integer.value[0]);
        connect = !!val;

        dapm_kcontrol_set_value(kcontrol, val);
        update.kcontrol = kcontrol;
        update.reg = mc->reg;
        update.mask = mask;
        update.val = val;

        snd_soc_dapm_mixer_update_power(widget->dapm, kcontrol, connect, &update);
        return 0;
}

-- 
~Vinod

Re: [PATCH v2 1/2] ASoC: export dapm_kcontrol_set/get_value functions

[Lars-Peter Clausen]

On 05/30/2014 08:09 AM, Vinod Koul wrote:
> On Thu, May 29, 2014 at 12:55:07PM +0200, Lars-Peter Clausen wrote:
>> On 05/29/2014 12:03 PM, Vinod Koul wrote:
>>> The DSPs like Intel ones use the DPCM to represent the DSP toplogy. So when DAPM
>>> triggers the widget, the DSP needs to know the kcontrol values and pass on to DSP
>>> firmware, so export these for driver use
>>>
>>
>> As I said I feel a bit uneasy about exporting these and I'd rather
>> much prefer to see this patch in context (i.e. in the same series as
>> its user).
> I am still working on that series. The core patches could have gone ahead so
> posted.
>
> Here is the code snippet that will come in subsequent series:
>
> int sst_mix_put(struct snd_kcontrol *kcontrol,
>          struct snd_ctl_elem_value *ucontrol)
> {
>          struct snd_soc_dapm_widget_list *wlist =
> 			dapm_kcontrol_get_wlist(kcontrol);
>          struct snd_soc_dapm_widget *widget = wlist->widgets[0];
>          struct soc_mixer_control *mc =
>                  (struct soc_mixer_control *)kcontrol->private_value;
>          struct sst_data *sst = snd_soc_platform_get_drvdata(widget->platform);
>          unsigned int mask = (1 << fls(mc->max)) - 1;
>          unsigned int val;
>          int connect;
>          struct snd_soc_dapm_update update;
>
>          val = sst_reg_write(sst, mc->reg, mc->shift, mc->max,
> 				ucontrol->value.integer.value[0]);
>          connect = !!val;
>
>          dapm_kcontrol_set_value(kcontrol, val);

This looks similar to what I discussed with Peter the other day. You do not 
really need to call dapm_kcontrol_set_value() for controls that are not a 
virtual control or a auto-muted control. Neither is probably the case here. 
The conclusion of our discussion also was to move the 
dapm_kcontrol_set_value() call into snd_soc_dapm_mixer_update_power(), in 
case it ever becomes mandatory for other types of controls as well, because 
this means that not every custom put callback has to call the function on 
it's own.

Btw. in this case I think you should just use the default DAPM kcontrol put 
callback. There doesn't seem to be anything special in here and with the 
DAPM componentization series the default handler is also able to handle 
platforms.

- Lars

Re: [PATCH v2 1/2] ASoC: export dapm_kcontrol_set/get_value functions

[Vinod Koul]

On Fri, May 30, 2014 at 09:22:07AM +0200, Lars-Peter Clausen wrote:
> On 05/30/2014 08:09 AM, Vinod Koul wrote:
> >On Thu, May 29, 2014 at 12:55:07PM +0200, Lars-Peter Clausen wrote:
> >>On 05/29/2014 12:03 PM, Vinod Koul wrote:
> >>>The DSPs like Intel ones use the DPCM to represent the DSP toplogy. So when DAPM
> >>>triggers the widget, the DSP needs to know the kcontrol values and pass on to DSP
> >>>firmware, so export these for driver use
> >>>
> >>
> >>As I said I feel a bit uneasy about exporting these and I'd rather
> >>much prefer to see this patch in context (i.e. in the same series as
> >>its user).
> >I am still working on that series. The core patches could have gone ahead so
> >posted.
> >
> >Here is the code snippet that will come in subsequent series:
> >
> >int sst_mix_put(struct snd_kcontrol *kcontrol,
> >         struct snd_ctl_elem_value *ucontrol)
> >{
> >         struct snd_soc_dapm_widget_list *wlist =
> >			dapm_kcontrol_get_wlist(kcontrol);
> >         struct snd_soc_dapm_widget *widget = wlist->widgets[0];
> >         struct soc_mixer_control *mc =
> >                 (struct soc_mixer_control *)kcontrol->private_value;
> >         struct sst_data *sst = snd_soc_platform_get_drvdata(widget->platform);
> >         unsigned int mask = (1 << fls(mc->max)) - 1;
> >         unsigned int val;
> >         int connect;
> >         struct snd_soc_dapm_update update;
> >
> >         val = sst_reg_write(sst, mc->reg, mc->shift, mc->max,
> >				ucontrol->value.integer.value[0]);
> >         connect = !!val;
> >
> >         dapm_kcontrol_set_value(kcontrol, val);
> 
> This looks similar to what I discussed with Peter the other day. You
> do not really need to call dapm_kcontrol_set_value() for controls
> that are not a virtual control or a auto-muted control. Neither is
> probably the case here. The conclusion of our discussion also was to
> move the dapm_kcontrol_set_value() call into
> snd_soc_dapm_mixer_update_power(), in case it ever becomes mandatory
> for other types of controls as well, because this means that not
> every custom put callback has to call the function on it's own.
Yes I agree that if this is the approach we take we can remove the code snippet
here, which would make sense IMO
>
> Btw. in this case I think you should just use the default DAPM
> kcontrol put callback. There doesn't seem to be anything special in
> here and with the DAPM componentization series the default handler
> is also able to handle platforms.
Not for this version sure, but future platforms of our need some custom stuff to
be done. I will try to minimize those ones and once your component work is
complete redo these bits. Would be too happy to remove if it works out that way,
which is not the case with today. So till then we need these bits and exporting
of the dapm_kcontrol_set_value()

-- 
~Vinod

Re: [PATCH v2 1/2] ASoC: export dapm_kcontrol_set/get_value functions

[Lars-Peter Clausen]

On 05/30/2014 12:29 PM, Vinod Koul wrote:
[...]
>> Btw. in this case I think you should just use the default DAPM
>> kcontrol put callback. There doesn't seem to be anything special in
>> here and with the DAPM componentization series the default handler
>> is also able to handle platforms.
> Not for this version sure, but future platforms of our need some custom stuff to
> be done. I will try to minimize those ones and once your component work is
> complete redo these bits. Would be too happy to remove if it works out that way,
> which is not the case with today. So till then we need these bits and exporting
> of the dapm_kcontrol_set_value()
>

You do not need to call dapm_kcontrol_set_value() from your custom put 
handler unless the control is auto-muted or a virtual control, of which 
neither seems to be the case here. So we do not need to export the function.

- Lars

Re: [PATCH v2 1/2] ASoC: export dapm_kcontrol_set/get_value functions

[Mark Brown]

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

On Thu, May 29, 2014 at 03:33:36PM +0530, Vinod Koul wrote:
> The DSPs like Intel ones use the DPCM to represent the DSP toplogy. So when DAPM
> triggers the widget, the DSP needs to know the kcontrol values and pass on to DSP
> firmware, so export these for driver use

I have to say that I share Lars' concerns with this one, especially for
the set function (which doesn't quite gel with the description above) -
it may be that this really is the best solution for the problem but it's
not entirely clear what the problem is and like Lars says there's some
other stuff in play here.  If there's something that the core just isn't
doing that any driver trying to do similar things is going to want we
should fix the core instead of requring drivers to do work.

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

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

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

[Mark Brown]

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

On Thu, May 29, 2014 at 03:33:37PM +0530, Vinod Koul wrote:
> From: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> 
> commit eee5d7f9 - "ASoC: dapm: Add a helper to get the CODEC for DAPM kcontrol"
> introduced helper for getting codec for DAPM kcontrol. For DSPs we need similar
> method to get the platform pointer while handling the widgets.

This is fine if we need it but hopefully soon we'll be able to just use
a component for everything instead of a platform and so it won't be
needed at all so I'd rather wait until there's a user.

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

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

Re: [PATCH v2 1/2] ASoC: export dapm_kcontrol_set/get_value functions

[Vinod Koul]

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

On Sun, Jun 01, 2014 at 08:26:49PM +0100, Mark Brown wrote:
> On Thu, May 29, 2014 at 03:33:36PM +0530, Vinod Koul wrote:
> > The DSPs like Intel ones use the DPCM to represent the DSP toplogy. So when DAPM
> > triggers the widget, the DSP needs to know the kcontrol values and pass on to DSP
> > firmware, so export these for driver use
> 
> I have to say that I share Lars' concerns with this one, especially for
> the set function (which doesn't quite gel with the description above) -
> it may be that this really is the best solution for the problem but it's
> not entirely clear what the problem is and like Lars says there's some
> other stuff in play here.  If there's something that the core just isn't
> doing that any driver trying to do similar things is going to want we
> should fix the core instead of requring drivers to do work.

I don't disagree to the concern but as things seem to be today we need these
handlers. Yes with the componentization work by Lar's this may not be required, so
I guess we need to get the componentization series done first and then revisit
this topic.

In parallel we are trying to test bits to see if we can do away with this
assumption. Stay tuned...

-- 
~Vinod

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

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

Re: [PATCH v2 1/2] ASoC: export dapm_kcontrol_set/get_value functions

[Mark Brown]

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

On Tue, Jun 03, 2014 at 11:14:33AM +0530, Vinod Koul wrote:

> I don't disagree to the concern but as things seem to be today we need these
> handlers. Yes with the componentization work by Lar's this may not be required, so
> I guess we need to get the componentization series done first and then revisit
> this topic.

> In parallel we are trying to test bits to see if we can do away with this
> assumption. Stay tuned...

OK, or at least post the code using them - it's a lot easier to say "we
need this now for this driver" than to say "we need this for this out of
tree driver but we plan to remove the need for it and perhaps that'll
happen before the driver gets upstreamed".

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

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

Re: [PATCH v2 1/2] ASoC: export dapm_kcontrol_set/get_value functions

[Vinod Koul]

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

On Sun, Jun 01, 2014 at 08:26:49PM +0100, Mark Brown wrote:
> On Thu, May 29, 2014 at 03:33:36PM +0530, Vinod Koul wrote:
> > The DSPs like Intel ones use the DPCM to represent the DSP toplogy. So when DAPM
> > triggers the widget, the DSP needs to know the kcontrol values and pass on to DSP
> > firmware, so export these for driver use
> 
> I have to say that I share Lars' concerns with this one, especially for
> the set function (which doesn't quite gel with the description above) -
> it may be that this really is the best solution for the problem but it's
> not entirely clear what the problem is and like Lars says there's some
> other stuff in play here.  If there's something that the core just isn't
> doing that any driver trying to do similar things is going to want we
> should fix the core instead of requring drivers to do work.
Well the problem is that we can't use the current ASoC handlers for platform side
mixers etc. The current code assumes that mixer is only present in codecs so in
snd_soc_put_volsw() it does snd_soc_update_bits_locked(codec,...) which wont
work for platform side.

Hence we went ahead and did our own handler for platform side but need the
dapm_kcontrol_set/get_value exported out. And yes I wouldn't have bothered you
if this was to be kept out of tree, the plan is to get this merged :)

If we go ahead with Lar's series we may not need this, but am yet to test
that bit.

Thanks
-- 
~Vinod

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

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

Re: [PATCH v2 1/2] ASoC: export dapm_kcontrol_set/get_value functions

[Mark Brown]

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

On Fri, Jun 06, 2014 at 11:24:08AM +0530, Vinod Koul wrote:

> Well the problem is that we can't use the current ASoC handlers for platform side
> mixers etc. The current code assumes that mixer is only present in codecs so in
> snd_soc_put_volsw() it does snd_soc_update_bits_locked(codec,...) which wont
> work for platform side.
> 
> Hence we went ahead and did our own handler for platform side but need the
> dapm_kcontrol_set/get_value exported out. And yes I wouldn't have bothered you
> if this was to be kept out of tree, the plan is to get this merged :)

> If we go ahead with Lar's series we may not need this, but am yet to test
> that bit.

Right, that's definitely where we want to end up - convert everything to
components.  Failing that we should make helpers that work with
platforms instead of CODECs so the code isn't in the core.

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

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