On 08/01/2013 12:48 PM, Mark Brown wrote:
On Mon, Jul 29, 2013 at 05:14:04PM +0200, Lars-Peter Clausen wrote:
- ucontrol->value.integer.value[0] =
(snd_soc_read(codec, reg) >> shift) & mask;
- mutex_lock_nested(&card->dapm_mutex, SND_SOC_DAPM_CLASS_RUNTIME);
- if (dapm_kcontrol_is_powered(kcontrol))
val = (snd_soc_read(codec, reg) >> shift) & mask;- else
val = dapm_kcontrol_get_value(kcontrol);- mutex_unlock(&card->dapm_mutex);
My first thought looking at this is that I would expect this to be encapsulated in kcntrol_get_value(), though at the minute it's actually only returning the virtual value which makes sense for the existing use.
Equally well I'd expect the value to always be a functioning cache of the real value so I think what I'm really saying here is that I don't think we should really be checking if the control is powered at all. We do need the I/O path but the power isn't the reason for it, the fact that we have the value stashed locally is.
There might be some corner cases where the driver directly modifies the register value outside of put_volsw() which would break. But otherwise I agree.
Another thing that's bothering me here is that this only works for mono controls but many of the uses are stereo mutes and/or volumes. We'd need to add back the support for those.
But that's a general limitation for DAPM controls. Right now snd_soc_dapm_put_volsw() only supports mono controls. If we have a left and a right mixer we usually have one mono control for each side. But I of course it would be nice to have support for stereo controls here. I guess with the shared control infrastructure we actually have most of what we need in place. The only thing missing is a way to express which channel of the control controls which path. E.g. an extra field per path or per mixer widget selecting the index.
- Lars