RE: [PATCH] ASoC: DAPM: Add support for multi register mux

[Arun Shamanna Lakshmi]

> -----Original Message-----
> From: Lars-Peter Clausen [mailto:lars@xxxxxxxxxx]
> Sent: Tuesday, April 01, 2014 12:48 AM
> To: Arun Shamanna Lakshmi
> Cc: lgirdwood@xxxxxxxxx; broonie@xxxxxxxxxx;
swarren@xxxxxxxxxxxxx;
> perex@xxxxxxxx; tiwai@xxxxxxx; alsa- devel@xxxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; 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 <aruns@xxxxxxxxxx>
> > Signed-off-by: Songhee Baek <sbaek@xxxxxxxxxx>
>
> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/