Re: [alsa-devel] [PATCH] ASoC: dapm: Add support for multi register mux

On 04/03/2014 05:11 AM, Arun Shamanna Lakshmi wrote:

This looks essentially good to me. A few minor issues, once those are fixed things should be good to go.

[...]

struct snd_soc_dapm_update { struct snd_kcontrol *kcontrol;

• int reg;
• int mask;
• int val;

• int reg[3];
• int mask[3];
• int val[3];

Make the 3 a define and check against it in the put handler.

• int num_regs;

unsigned int

};

[...]

+/*

• • Soc Enum Type
• • @NONE: soc_enum type for SINGLE, DOUBLE or VIRTUAL mux
• • @ONEHOT: soc_enum type for one hot encoding mux
• */

This should be kernel doc style so

/** * enum snd_soc_enum_type - Type of the ASoC enum control * @SND_SOC_ENUM_NONE: ... * ... */

+enum snd_soc_enum_type {

• SND_SOC_ENUM_NONE = 0,

I'm not sure if NONE is the right term. Maybe BINARY is better.

• SND_SOC_ENUM_ONEHOT = 1,

+};

[...]

diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index c8a780d..19b004a 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -511,13 +511,26 @@ static int dapm_connect_mux(struct snd_soc_dapm_context *dapm, const struct snd_kcontrol_new *kcontrol) { struct soc_enum *e = (struct soc_enum *)kcontrol->private_value;

• unsigned int val, item;

• unsigned int val, item, bit_pos = -1;

default for bit_pos should probably be 0.

[...]

@@ -1575,8 +1588,12 @@ static void dapm_widget_update(struct snd_soc_card *card) if (!w) return;

• ret = soc_widget_update_bits_locked(w, update->reg, update->mask,

• 		  update->val);
  

• /* dapm update for multiple registers */
• for (i = 0; i < update->num_regs; i++) {

• ret |= soc_widget_update_bits_locked(w, update->reg[i],
  

• 			update->mask[i], update->val[i]);
  

I'd prefer ret = soc_widget_update_bits_locked(...); if (ret < 0) break;

• }
• if (ret < 0) dev_err(w->dapm->dev, "ASoC: %s DAPM update failed: %d\n", w->name, ret);

[...]

@@ -2984,6 +3002,112 @@ int snd_soc_dapm_put_enum_double(struct snd_kcontrol *kcontrol, EXPORT_SYMBOL_GPL(snd_soc_dapm_put_enum_double);

/**

• • snd_soc_dapm_get_enum_onehot - dapm enumerated onehot mixer get callback
• • @kcontrol: mixer control
• • @ucontrol: control element information
• • Callback to get the value of a dapm enumerated onehot encoded mixer control
• • Returns 0 for success.
• */

+int snd_soc_dapm_get_enum_onehot(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 = -1, reg_idx;

Here as well, default for bit_pos should be 0.

• 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) + (8 * codec->val_bytes * reg_idx);
  

• 	break;
  

• }
  

• }
• ucontrol->value.enumerated.item[0] =

• 	snd_soc_enum_val_to_item(e, bit_pos);
  

• return 0;

+} +EXPORT_SYMBOL_GPL(snd_soc_dapm_get_enum_onehot);

+/**

• • snd_soc_dapm_put_enum_onehot - dapm enumerated onehot mixer put callback
• • @kcontrol: mixer control
• • @ucontrol: control element information
• • Callback to put the value of a dapm enumerated onehot encoded mixer control
• • Returns 0 for success.
• */

+int snd_soc_dapm_put_enum_onehot(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, update_idx = 0;
• if (item[0] >= e->items)

• return -EINVAL;
  

• value = snd_soc_enum_item_to_val(e, item[0]);
• if (value >= 0) {

value is unsigned int, so this is never false.

• /* get the register index and value to set */
  

• reg_idx = value / (8 * codec->val_bytes);
  

• bit_pos = value % (8 * codec->val_bytes);
  

• 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);
  

change |=

• 	/* set the selected register */
  

• 	update.reg[e->num_regs - 1] = e->reg[reg_idx];
  

• 	update.mask[e->num_regs - 1] = e->mask[reg_idx];
  

• 	update.val[e->num_regs - 1] = 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);
  

• 	/* clear the register when not selected */
  

• 	update.reg[update_idx] = e->reg[i];
  

• 	update.mask[update_idx] = e->mask[i];
  

• 	update.val[update_idx++] = 0;
  

• }
  

• }
• mutex_lock_nested(&card->dapm_mutex, SND_SOC_DAPM_CLASS_RUNTIME);
• if (change) {

• update.kcontrol = kcontrol;
  

• update.num_regs = 3;
  

Should be e->num_regs;

• card->update = &update;
  

• ret = soc_dapm_mux_update_power(card, kcontrol, item[0], e);
  

• card->update = NULL;
  

• }