linux-kernel - Re: [PATCH 9/9] ASoC: codecs: wm8753: Fix register cache incoherency

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 24 Dec 2010 15:50:41 +0000
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	Lars-Peter Clausen <lars@...afoo.de>
Cc:	Liam Girdwood <lrg@...mlogic.co.uk>, alsa-devel@...a-project.org,
	linux-kernel@...r.kernel.org,
	Ian Lartey <ian@...nsource.wolfsonmicro.com>,
	Dimitris Papastamos <dp@...nsource.wolfsonmicro.com>
Subject: Re: [PATCH 9/9] ASoC: codecs: wm8753: Fix register cache
 incoherency

On Fri, Dec 24, 2010 at 02:48:04PM +0100, Lars-Peter Clausen wrote:
> The multi-component patch(commit f0fba2ad1) introduced a generic register cache
> framework. But the wm8753 driver still uses its own register cache for its

So, the actual relevant thing that the multi component patch did was to
introduce a new way of initialising the cache and setting up the pointer
that soc-cache uses to reference it, with some of the drivers being
partially converted.  

Since most of the drivers were already using the soc-cache code they
mostly have some initialisation errors, others (like WM8753) weren't
using soc-cache at all and probably shouldn't have been flipped over.

>  	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
>  	case SND_SOC_DAIFMT_I2S:
> -		voice |= 0x0002;
> +		voice = 0x0002;
>  		break;

This is more than a straight conversion of the driver - since changes
like this aren't simple substitutions they're much harder to review, if
you want to make changes such as this they should be split out into
separate patches.  There's quite a few other bits like this in the
patch, some of which make it pretty confusing.

> -	u16 voice, ioctl;
> +	u16 voice, ioctl = 0;

Please don't mix variable declarations of initialised and uninitialised
variables like this.

> -	wm8753_write(codec, WM8753_PCM, voice);
> -	wm8753_write(codec, WM8753_IOCTL, ioctl);
> +	snd_soc_write(codec, WM8753_PCM, voice);
> +	snd_soc_update_bits(codec, WM8753_IOCTL, 0x00a2, ioctl);

This bit is especially confusing, we've now got a mixture of both
writing the full register and use of update_bits.

> +	for (i = 1; i < ARRAY_SIZE(wm8753_reg); i++) {
> +		if (i == WM8753_RESET)
> +			continue;
> +

If you make the register reset function write the default value of the
reset register you can use the standard cache sync implementation.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/