> -----Original Message-----
> From: Lars-Peter Clausen [mailto:l...@metafoo.de]
> Sent: Tuesday, April 01, 2014 12:48 AM
> To: Arun Shamanna Lakshmi
> Cc: lgirdw...@gmail.com; broo...@kernel.org;
swar...@wwwdotorg.org;
> pe...@perex.cz; ti...@suse.de; alsa- de...@alsa-project.org;
> linux-kernel@vger.kernel.org; Songhee Baek
> Subject: Re: [PATCH] ASoC: DAPM: Add support for multi register mux
>
> On 04/01/2014 08:21 AM, Arun Shamanna Lakshmi wrote:
> > Modify soc_enum struct to handle pointers for reg and mask. Add
dapm
> > get and put APIs for multi register mux with one hot encoding.
> >
> > Signed-off-by: Arun Shamanna Lakshmi <ar...@nvidia.com>
> > Signed-off-by: Songhee Baek <sb...@nvidia.com>
>
> Looks in my opinion much better than the previous version :) Just a
> few minor issues, comments inline
>
> > ---
> > include/sound/soc-dapm.h | 10 ++++
> > include/sound/soc.h | 22 +++++--
> > sound/soc/soc-core.c | 12 ++--
> > sound/soc/soc-dapm.c | 143
> +++++++++++++++++++++++++++++++++++++++++-----
> > 4 files changed, 162 insertions(+), 25 deletions(-)
> >
> > diff --git a/include/sound/soc-dapm.h b/include/sound/soc-dapm.h
> index
> > ef78f56..983b0ab 100644
> > --- a/include/sound/soc-dapm.h
> > +++ b/include/sound/soc-dapm.h
> > @@ -305,6 +305,12 @@ struct device;
> > .get = snd_soc_dapm_get_enum_double, \
> > .put = snd_soc_dapm_put_enum_double, \
> > .private_value = (unsigned long)&xenum }
> > +#define SOC_DAPM_ENUM_WIDE(xname, xenum) \
>
> maybe just call it ENUM_ONEHOT, since it doesn't actually have to be
> more than one register.
>
> [...]
> > diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index
> > cd52d52..aba0094 100644
> > --- a/sound/soc/soc-core.c
> > +++ b/sound/soc/soc-core.c
> > @@ -2601,12 +2601,12 @@ int snd_soc_get_enum_double(struct
> snd_kcontrol *kcontrol,
> > unsigned int val, item;
> > unsigned int reg_val;
> >
> > - reg_val = snd_soc_read(codec, e->reg);
> > - val = (reg_val >> e->shift_l) & e->mask;
> > + reg_val = snd_soc_read(codec, e->reg[0]);
> > + val = (reg_val >> e->shift_l) & e->mask[0];
> > item = snd_soc_enum_val_to_item(e, val);
> > ucontrol->value.enumerated.item[0] = item;
> > if (e->shift_l != e->shift_r) {
> > - val = (reg_val >> e->shift_l) & e->mask;
> > + val = (reg_val >> e->shift_l) & e->mask[0];
> > item = snd_soc_enum_val_to_item(e, val);
> > ucontrol->value.enumerated.item[1] = item;
> > }
> > @@ -2636,15 +2636,15 @@ int snd_soc_put_enum_double(struct
> snd_kcontrol *kcontrol,
> > if (item[0] >= e->items)
> > return -EINVAL;
> > val = snd_soc_enum_item_to_val(e, item[0]) << e->shift_l;
> > - mask = e->mask << e->shift_l;
> > + mask = e->mask[0] << e->shift_l;
> > if (e->shift_l != e->shift_r) {
> > if (item[1] >= e->items)
> > return -EINVAL;
> > val |= snd_soc_enum_item_to_val(e, item[1]) << e-
shift_r;
> > - mask |= e->mask << e->shift_r;
> > + mask |= e->mask[0] << e->shift_r;
> > }
> >
> > - return snd_soc_update_bits_locked(codec, e->reg, mask, val);
> > + return snd_soc_update_bits_locked(codec, e->reg[0], mask, val);
> > }
> > EXPORT_SYMBOL_GPL(snd_soc_put_enum_double);
> >
> > diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index
> > c8a780d..4d2b35c 100644
> > --- a/sound/soc/soc-dapm.c
> > +++ b/sound/soc/soc-dapm.c
> > @@ -514,9 +514,9 @@ static int dapm_connect_mux(struct
> snd_soc_dapm_context *dapm,
> > unsigned int val, item;
> > int i;
> >
> > - if (e->reg != SND_SOC_NOPM) {
> > - soc_widget_read(dest, e->reg, &val);
> > - val = (val >> e->shift_l) & e->mask;
> > + if (e->reg[0] != SND_SOC_NOPM) {
> > + soc_widget_read(dest, e->reg[0], &val);
> > + val = (val >> e->shift_l) & e->mask[0];
> > item = snd_soc_enum_val_to_item(e, val);
>
> This probably should handle the new enum type as well. You'll probably
> need some kind of flag in the struct to distinguish between the two
> enum types.
Any suggestion on the flag name ?
>
> > } else {
> > /* since a virtual mux has no backing registers to
> [...]
> > /**
> > + * snd_soc_dapm_get_enum_wide - dapm semi enumerated multiple
> > + registers
>
> What's a semi-enumerated register?
>
> > + * mixer get callback
> > + * @kcontrol: mixer control
> > + * @ucontrol: control element information
> > + *
> > + * Callback to get the value of a dapm semi enumerated multiple
> > +register mixer
> > + * control.
> > + *
> > + * semi enumerated multiple registers mixer:
> > + * the mixer has multiple registers to set the enumerated items.
> > +The enumerated
> > + * items are referred as values.
> > + * Can be used for handling bit field coded enumeration for example.
> > + *
> > + * Returns 0 for success.
> > + */
> > +int snd_soc_dapm_get_enum_wide(struct snd_kcontrol *kcontrol,
> > + struct snd_ctl_elem_value *ucontrol) {
> > + struct snd_soc_codec *codec =
> snd_soc_dapm_kcontrol_codec(kcontrol);
> > + struct soc_enum *e = (struct soc_enum *)kcontrol-
> >private_value;
> > + unsigned int reg_val, val, bit_pos = 0, reg_idx;
> > +
> > + for (reg_idx = 0; reg_idx < e->num_regs; reg_idx++) {
> > + reg_val = snd_soc_read(codec, e->reg[reg_idx]);
> > + val = reg_val & e->mask[reg_idx];
> > + if (val != 0) {
> > + bit_pos = ffs(val) + (e->reg_width * reg_idx);
>
> Should be __ffs. __ffs returns the bits zero-indexed and ffs one-indexed.
> That will work better for cases where there is not additional value
> table necessary, since it means bit 1 maps to value 0.
>
> > + break;
> > + }
> > + }
> > +
> > + ucontrol->value.enumerated.item[0] =
> > + snd_soc_enum_val_to_item(e, bit_pos);
> > +
> > + return 0;
> > +}
> [...]
> > +int snd_soc_dapm_put_enum_wide(struct snd_kcontrol *kcontrol,
> > + struct snd_ctl_elem_value *ucontrol) {
> > + struct snd_soc_codec *codec =
> snd_soc_dapm_kcontrol_codec(kcontrol);
> > + struct snd_soc_card *card = codec->card;
> > + struct soc_enum *e = (struct soc_enum *)kcontrol-
> >private_value;
> > + unsigned int *item = ucontrol->value.enumerated.item;
> > + unsigned int change = 0, reg_idx = 0, value, bit_pos;
> > + struct snd_soc_dapm_update update;
> > + int ret = 0, reg_val = 0, i;
> > +
> > + if (item[0] >= e->items)
> > + return -EINVAL;
> > +
> > + value = snd_soc_enum_item_to_val(e, item[0]);
> > +
> > + if (value) {
> > + /* get the register index and value to set */
> > + reg_idx = (value - 1) / e->reg_width;
> > + bit_pos = (value - 1) % e->reg_width;
>
> Changing the ffs to __ffs also means you can drop the ' - 1' here.
>
> Also e->reg_width should be (codec->val_bytes * 8) and reg_width field
> should be dropped from the enum struct.
>
> > + reg_val = BIT(bit_pos);
> > + }
> > +
> > + for (i = 0; i < e->num_regs; i++) {
> > + if (i == reg_idx) {
> > + change = snd_soc_test_bits(codec, e->reg[i],
> > + e->mask[i],
> reg_val);
> > +
> > + } else {
> > + /* accumulate the change to update the DAPM
> path
> > + when none is selected */
> > + change += snd_soc_test_bits(codec, e->reg[i],
> > + e->mask[i], 0);
>
> change |=
>
> > +
> > + /* clear the register when not selected */
> > + snd_soc_write(codec, e->reg[i], 0);
>
> I think this should happen as part of the DAPM update sequence like
> you had earlier. Some special care should probably be take to make
> sure that you de-select the previous mux input before selecting the
> new one if the new one is in a different register than the previous one.
I am not sure I follow this part. We are clearing the 'not selected'
registers before we set the one we want. Do you want us to loop the
logic of soc_dapm_mux_update_power for each register ? or do you
want to change the dapm_update structure so that it takes all the regs,
masks, and values together ?
>
> > + }
> > + }
> > +
> > + mutex_lock_nested(&card->dapm_mutex,
> SND_SOC_DAPM_CLASS_RUNTIME);
> > +
> [...]
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
