| Message ID | 1401357817-17942-1-git-send-email-vinod.koul@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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; }
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
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()
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
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.
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...
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".
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
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.
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
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(-)