Messages in this thread | | | Date | Wed, 02 Apr 2014 17:29:33 +0200 | From | Lars-Peter Clausen <> | Subject | Re: [PATCH] ASoC: DAPM: Add support for multi register mux |
| |
On 04/02/2014 05:26 PM, Songhee Baek wrote: >> -----Original Message----- >> From: Lars-Peter Clausen [mailto:lars@metafoo.de] >> Sent: Wednesday, April 02, 2014 12:02 AM >> To: Songhee Baek >> Cc: Arun Shamanna Lakshmi; lgirdwood@gmail.com; broonie@kernel.org; >> swarren@wwwdotorg.org; perex@perex.cz; tiwai@suse.de; alsa- >> devel@alsa-project.org; linux-kernel@vger.kernel.org >> Subject: Re: [PATCH] ASoC: DAPM: Add support for multi register mux >> >> On 04/02/2014 08:56 AM, Songhee Baek wrote: >>> >>> >>>> -----Original Message----- >>>> From: Lars-Peter Clausen [mailto:lars@metafoo.de] >>>> Sent: Tuesday, April 01, 2014 11:47 PM >>>> To: Songhee Baek >>>> Cc: Arun Shamanna Lakshmi; lgirdwood@gmail.com; broonie@kernel.org; >>>> swarren@wwwdotorg.org; perex@perex.cz; tiwai@suse.de; alsa- >>>> devel@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:lars@metafoo.de] >>>>>> Sent: Tuesday, April 01, 2014 11:00 PM >>>>>> To: Arun Shamanna Lakshmi >>>>>> Cc: lgirdwood@gmail.com; broonie@kernel.org; >>>> swarren@wwwdotorg.org; >>>>>> perex@perex.cz; tiwai@suse.de; alsa-devel@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: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? >> >> Right now the update struct does not have support for multiple register >> writes. That's up to you to implement this. I guess making it an array for now >> should be fine. But you need to add some safety checks to make sure that >> num_regs is not larger or equal to the array size. > > I think that the dapm update struct needs to have reg[2]/val[2]/mask[2]. > Because the mux is one-hot coded, only one register has a non-zero value. > So reg[0] will contain the register to be clear and reg[2] has selected register > to be set. > How about your opinion for this?
Should work (in theory).
- Lars
| |