| Message ID | 1395186692-11735-1-git-send-email-aruns@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Mar 18, 2014 at 04:51:32PM -0700, Arun Shamanna Lakshmi wrote: > Currently soc_enum structure supports only 2 registers (reg, reg2) > for kcontrol. However, it is possible to have multiple registers > per mux. This change allows us to control these multiple registers. I'd want to see a user along with this and... > @@ -1093,6 +1093,9 @@ struct soc_enum { > unsigned int mask; > const char * const *texts; > const unsigned int *values; > + unsigned int *regs; > + unsigned int *masks; > + unsigned int num_regs; ...it duplicates and generally isn't joined up with the existing members of the structure, and has no support in the helpers (for example, converting the existing stereo controls to be two element arrays which I'd expect to see). Helpers would count as users here. Note that we don't support double register enums or muxes - only numerical controls are supported. It's not clear what a multi-register enum would mean.
If each bit of a 32 bit register maps to an input of a mux, then with the current 'soc_enum' structure we cannot have more than 64 inputs for the mux (because of reg and reg2 only).
In such cases, we need more than 2 registers to select the input of the mux. This is referred to as 'multi register mux'
For instance, the audio xbar (AXBAR) module acts as a mux selecting various inputs (reference: Tegra K1 manual).
The number of such inputs increases with future Tegra chips and so will be the need to control multiple registers per mux in DAPM. We have 2 options to achieve that.
Option 1: Using custom get and put functions something similar to below inside AXBAR tegra driver.
int tegra_xbar_get_value_enum(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol)
{
struct snd_soc_dapm_widget_list *wlist = snd_kcontrol_chip(kcontrol);
struct snd_soc_dapm_widget *widget = wlist->widgets[0];
struct soc_enum *e = (struct soc_enum *)kcontrol->private_value;
unsigned int reg_val, mux, find, reg_idx;
unsigned int num_regs = 3, regs[3];
/* control 3 registers that has a common STRIDE */
regs[0] = e-> reg;
regs[1] = regs[0] + MUX_REG_STRIDE;
regs[2] = regs[1] + MUX_REG_STRIDE;
for (mux = 0; mux < e->max; mux++) {
find = 0;
for (reg_idx = 0; reg_idx < num_regs; reg_idx++) {
regmap_read(widget->codec->control_data,
regs[reg_idx], ®_val);
if (reg_val == e->values[mux * num_regs + reg_idx])
find++;
}
if (find == num_regs)
break;
}
ucontrol->value.enumerated.item[0] = mux;
return 0;
}
int tegra_xbar_put_value_enum(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol)
{
struct snd_soc_dapm_widget_list *wlist = snd_kcontrol_chip(kcontrol);
struct snd_soc_dapm_widget *widget = wlist->widgets[0];
struct soc_enum *e = (struct soc_enum *)kcontrol->private_value;
unsigned int value, mux, old, reg_idx;
struct snd_soc_dapm_update update;
unsigned int num_regs = 3, regs[3], masks[3] = { 0xf1f03ff, 0x3f30031f, 0xff1cf313};
int wi;
regs[0] = e-> reg;
regs[1] = regs[0] + MUX_REG_STRIDE;
regs[2] = regs[1] + MUX_REG_STRIDE;
if (ucontrol->value.enumerated.item[0] > e->max - 1)
return -EINVAL;
mux = ucontrol->value.enumerated.item[0];
for (reg_idx = 0; reg_idx < num_regs; reg_idx++) {
value = e->values[ucontrol->value.enumerated.item[0] * num_regs + reg_idx];
regmap_read(widget->codec->control_data, regs[reg_idx], &old);
if (value != old) {
for (wi = 0; wi < wlist->num_widgets; wi++) {
widget = wlist->widgets[wi];
widget->value = value;
update.kcontrol = kcontrol;
update.widget = widget;
update.reg = regs[reg_idx];
update.mask = masks[reg_idx];
update.val = value;
widget->dapm->update = &update;
snd_soc_dapm_mux_update_power(widget, kcontrol, mux, e);
widget->dapm->update = NULL;
}
}
}
return 0;
}
Option 2: Modify soc_enum structure and make 'reg' variable as reg[MAX_REG]
This would also mean that we should edit all the macros in soc.h, soc-dapm.c and soc-core.c to use 'reg[0]' instead of 'reg'
Our goal is to eventually add support to do multi register mux in get and put handlers inside soc-dapm.c upstream.
With Option1, we don't need to change any code in upstream as each register among the multiple registers has a common STRIDE (address offset). Thus, option1 is not generic enough.
If you suggest Option1, we wanted to check if upstream will be okay with such a structure. (it will be tegra specific though).
With Option2, it becomes easy to add new macros for multi register mux in soc.h and then, add new get and put handlers in dapm.c
Thanks,
Arun
-----Original Message-----
From: Mark Brown [mailto:broonie@kernel.org]
Sent: Tuesday, March 18, 2014 5:00 PM
To: Arun Shamanna Lakshmi
Cc: lgirdwood@gmail.com; perex@perex.cz; tiwai@suse.de; alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org; Songhee Baek
Subject: Re: [PATCH] ASoC: Add support for multi register mux
* PGP Signed by an unknown key
On Tue, Mar 18, 2014 at 04:51:32PM -0700, Arun Shamanna Lakshmi wrote:
> Currently soc_enum structure supports only 2 registers (reg, reg2) for
> kcontrol. However, it is possible to have multiple registers per mux.
> This change allows us to control these multiple registers.
I'd want to see a user along with this and...
> @@ -1093,6 +1093,9 @@ struct soc_enum {
> unsigned int mask;
> const char * const *texts;
> const unsigned int *values;
> + unsigned int *regs;
> + unsigned int *masks;
> + unsigned int num_regs;
...it duplicates and generally isn't joined up with the existing members of the structure, and has no support in the helpers (for example, converting the existing stereo controls to be two element arrays which I'd expect to see). Helpers would count as users here.
Note that we don't support double register enums or muxes - only numerical controls are supported. It's not clear what a multi-register enum would mean.
* Unknown Key
* 0x7EA229BD
On Wed, Mar 19, 2014 at 04:44:00PM -0700, Arun Shamanna Lakshmi wrote: Don't top post and fix your mailer to word wrap within paragraphs, your mail is very hard to read. > If each bit of a 32 bit register maps to an input of a mux, then with > the current 'soc_enum' structure we cannot have more than 64 inputs > for the mux (because of reg and reg2 only). What makes you say that? We currently have devices in mainline which have well over 32 inputs to muxes. > For instance, the audio xbar (AXBAR) module acts as a mux selecting > various inputs (reference: Tegra K1 manual). I don't have access to non-public nVidia documents... > The number of such inputs increases with future Tegra chips and so > will be the need to control multiple registers per mux in DAPM. We > have 2 options to achieve that. Like I said in my previous reply I would expect to see some users along with the code, extending the standard helpers to support this would be a much better idea than doing something driver custom.
On 03/20/2014 05:48 AM, Mark Brown wrote: > On Wed, Mar 19, 2014 at 04:44:00PM -0700, Arun Shamanna Lakshmi wrote: > > Don't top post and fix your mailer to word wrap within paragraphs, your > mail is very hard to read. > >> If each bit of a 32 bit register maps to an input of a mux, then with >> the current 'soc_enum' structure we cannot have more than 64 inputs >> for the mux (because of reg and reg2 only). > > What makes you say that? We currently have devices in mainline which > have well over 32 inputs to muxes. I think their register layout is different. I found a number of large muxes where the register stores a 'integer' indicating which mux input to select, e.g. Arizona, WM2200, etc. In this case, an N-bit register could support up to 2^N inputs. However, the registers in the Tegra AHUB use 1 bit position per input, and require you to set one single bit at a time. Hence, an N bit register (or string of registers) can support up to N inputs. In more recent Tegra chips, we have at least >32 inputs and I think Arun was saying even >64 inputs. That requires 2 or 3 or more .reg fields in struct soc_enum.
On Thu, Mar 20, 2014 at 12:20:17PM -0600, Stephen Warren wrote: > On 03/20/2014 05:48 AM, Mark Brown wrote: > > On Wed, Mar 19, 2014 at 04:44:00PM -0700, Arun Shamanna Lakshmi wrote: > >> If each bit of a 32 bit register maps to an input of a mux, then with > >> the current 'soc_enum' structure we cannot have more than 64 inputs > >> for the mux (because of reg and reg2 only). > > What makes you say that? We currently have devices in mainline which > > have well over 32 inputs to muxes. > I think their register layout is different. > I found a number of large muxes where the register stores a 'integer' > indicating which mux input to select, e.g. Arizona, WM2200, etc. In this > case, an N-bit register could support up to 2^N inputs. > However, the registers in the Tegra AHUB use 1 bit position per input, > and require you to set one single bit at a time. Hence, an N bit > register (or string of registers) can support up to N inputs. In more > recent Tegra chips, we have at least >32 inputs and I think Arun was > saying even >64 inputs. That requires 2 or 3 or more .reg fields in > struct soc_enum. Right, that was my guess too (the mail wasn't terribly clear with the formatting, references to unpublished documents and so on) but that's not a straight mux, it's a value mux, and the limit with the current code is much lower on 32 bit systems (like at least some of the K1s) since muxes only use one of the current register fields.
On 03/20/2014 07:36 PM, Mark Brown wrote: > On Thu, Mar 20, 2014 at 12:20:17PM -0600, Stephen Warren wrote: >> On 03/20/2014 05:48 AM, Mark Brown wrote: >>> On Wed, Mar 19, 2014 at 04:44:00PM -0700, Arun Shamanna Lakshmi wrote: > >>>> If each bit of a 32 bit register maps to an input of a mux, then with >>>> the current 'soc_enum' structure we cannot have more than 64 inputs >>>> for the mux (because of reg and reg2 only). > >>> What makes you say that? We currently have devices in mainline which >>> have well over 32 inputs to muxes. > >> I think their register layout is different. > >> I found a number of large muxes where the register stores a 'integer' >> indicating which mux input to select, e.g. Arizona, WM2200, etc. In this >> case, an N-bit register could support up to 2^N inputs. > >> However, the registers in the Tegra AHUB use 1 bit position per input, >> and require you to set one single bit at a time. Hence, an N bit >> register (or string of registers) can support up to N inputs. In more >> recent Tegra chips, we have at least >32 inputs and I think Arun was >> saying even >64 inputs. That requires 2 or 3 or more .reg fields in >> struct soc_enum. > > Right, that was my guess too (the mail wasn't terribly clear with the > formatting, references to unpublished documents and so on) but that's > not a straight mux, it's a value mux, and the limit with the current > code is much lower on 32 bit systems (like at least some of the K1s) > since muxes only use one of the current register fields. It might make sense to add special code for supported muxes with a one-hot encoding instead of using a value mux. Having an large array where each entry is just 1<<n is a bit ugly in my opinion, especially if the value needs to be able to be larger than 2**64. But anyway the patch that modifies the soc_enum struct should also add the code that makes use of the new struct layout. - Lars
On 03/20/2014 08:05 PM, Lars-Peter Clausen wrote: > On 03/20/2014 07:36 PM, Mark Brown wrote: >> On Thu, Mar 20, 2014 at 12:20:17PM -0600, Stephen Warren wrote: >>> On 03/20/2014 05:48 AM, Mark Brown wrote: >>>> On Wed, Mar 19, 2014 at 04:44:00PM -0700, Arun Shamanna Lakshmi wrote: >> >>>>> If each bit of a 32 bit register maps to an input of a mux, then with >>>>> the current 'soc_enum' structure we cannot have more than 64 inputs >>>>> for the mux (because of reg and reg2 only). >> >>>> What makes you say that? We currently have devices in mainline which >>>> have well over 32 inputs to muxes. >> >>> I think their register layout is different. >> >>> I found a number of large muxes where the register stores a 'integer' >>> indicating which mux input to select, e.g. Arizona, WM2200, etc. In this >>> case, an N-bit register could support up to 2^N inputs. >> >>> However, the registers in the Tegra AHUB use 1 bit position per input, >>> and require you to set one single bit at a time. Hence, an N bit >>> register (or string of registers) can support up to N inputs. In more >>> recent Tegra chips, we have at least >32 inputs and I think Arun was >>> saying even >64 inputs. That requires 2 or 3 or more .reg fields in >>> struct soc_enum. >> >> Right, that was my guess too (the mail wasn't terribly clear with the >> formatting, references to unpublished documents and so on) but that's >> not a straight mux, it's a value mux, and the limit with the current >> code is much lower on 32 bit systems (like at least some of the K1s) >> since muxes only use one of the current register fields. > > It might make sense to add special code for supported muxes with a one-hot > encoding instead of using a value mux. Having an large array where each > entry is just 1<<n is a bit ugly in my opinion, especially if the value > needs to be able to be larger than 2**64. But anyway the patch that modifies > the soc_enum struct should also add the code that makes use of the new > struct layout. On the other hand this can actually already be implemented without any core changes by using a virtual mux and connecting a supply widget to each input which sets the bit for that input. DAPM will take care that only one of the supply widgets is enabled at a time. - Lars
On Thu, Mar 20, 2014 at 08:40:54PM +0100, Lars-Peter Clausen wrote: > On 03/20/2014 08:05 PM, Lars-Peter Clausen wrote: > >It might make sense to add special code for supported muxes with a one-hot > >encoding instead of using a value mux. Having an large array where each > >entry is just 1<<n is a bit ugly in my opinion, especially if the value > >needs to be able to be larger than 2**64. But anyway the patch that modifies > >the soc_enum struct should also add the code that makes use of the new > >struct layout. > On the other hand this can actually already be implemented without any core > changes by using a virtual mux and connecting a supply widget to each input > which sets the bit for that input. DAPM will take care that only one of the > supply widgets is enabled at a time. Yes, each works - either way I think we can probably come up with some helpers in the header which hide the actual implementation from the drivers or at least makes it obvious that this is the way to do things. The supply widgets approach wouldn't need any changes in the core but then generalising the two register code isn't a bad thing anyway.
diff --git a/include/sound/soc.h b/include/sound/soc.h index 9a00147..ddedfb4 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -1093,6 +1093,9 @@ struct soc_enum { unsigned int mask; const char * const *texts; const unsigned int *values; + unsigned int *regs; + unsigned int *masks; + unsigned int num_regs; }; /* codec IO */