> -----Original Message----- > From: Lars-Peter Clausen [mailto:l...@metafoo.de] > Sent: Tuesday, April 01, 2014 11:47 PM > To: Songhee Baek > Cc: Arun Shamanna Lakshmi; 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 > Subject: Re: [PATCH] ASoC: DAPM: Add support for multi register mux > > On 04/02/2014 08:17 AM, Songhee Baek wrote: > >> -----Original Message----- > >> From: Lars-Peter Clausen [mailto:l...@metafoo.de] > >> Sent: Tuesday, April 01, 2014 11:00 PM > >> 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- > >> ker...@vger.kernel.org; Songhee Baek > >> Subject: Re: [PATCH] ASoC: DAPM: Add support for multi register mux > >> > >> On 04/01/2014 08:26 PM, Arun Shamanna Lakshmi wrote: > >> [...] > >>>>> 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 ? > >>> > >> > >> How about 'onehot'? > >> > >> [...] > >>>>> + 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 ? > >> > >> The idea with the dapm_update struct is that the register updates are > >> done in the middle of the power-down and power-up sequence. So yes, > >> change the dapm_update struct to be able to hold all register updates > >> and do all register updates in dapm_widget_update. I think an earlier > >> version of your patch already had this. > > > > Is the change similar to as shown below? > > > > for (reg_idx = 0; reg_idx < e->num_regs; reg_idx++) { > > val = e->values[item * e->num_regs + reg_idx]; > > ret = snd_soc_update_bits_locked(codec, e->reg[reg_idx], > > e->mask[reg_idx], val); > > if (ret) > > return ret; > > } > > > > During updating of the register's value, the above change can create > > non-zero value in two different registers (very short transition) as > > Mark mentioned for that change so we need to clear register first > > before writing the desired value in the register. > > > > Should we add the clearing all registers and write the mux value in > > desired register in the update function? > > > > In dapm_update_widget() you have this line: > > ret = soc_widget_update_bits(w, update->reg, update->mask, update- > >val); > > That needs to be done for every register update. When you setup the > update struct you need to make sure that the register clears come before > the register set. > > E.g. if you have register 0x3, 0x4, 0x5 and you select a bit in register 0x4 > it > should look like this. > > update->reg[0] = 0x3; > update->val[0] = 0x0; > update->reg[1] = 0x5; > update->val[1] = 0x0; > update->reg[2] = 0x4; > update->val[2] = 0x8; > > When you set a bit in register 0x3 it should look like this: > > update->reg[0] = 0x4; > update->val[0] = 0x0; > update->reg[1] = 0x5; > update->val[1] = 0x0; > update->reg[2] = 0x3; > update->val[2] = 0x1; > > So basically the write operation goes into update->reg[e->num_regs-1] the > clear operations go into the other slots before that.
Does update reg/val array have the writing sequence, is it correct? And can I assume that update struct has reg/val/mask arrays not pointers? > > - Lars -- 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/