linux-kernel - Re: [RFC v1 4/9] ASoC: msm8x16: add ranges for default, readonly

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:	Tue, 16 Feb 2016 19:16:56 +0000
From:	Mark Brown <broonie@...nel.org>
To:	Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
Cc:	alsa-devel@...a-project.org, Rob Herring <robh+dt@...nel.org>,
	Mark Rutland <mark.rutland@....com>,
	Pawel Moll <pawel.moll@....com>,
	Patrick Lai <plai@...eaurora.org>,
	Liam Girdwood <lgirdwood@...il.com>,
	Jaroslav Kysela <perex@...ex.cz>,
	Takashi Iwai <tiwai@...e.com>, linux-kernel@...r.kernel.org,
	linux-arm-msm@...r.kernel.org, kwestfie@...eaurora.org
Subject: Re: [RFC v1 4/9] ASoC: msm8x16: add ranges for default, readonly

On Tue, Feb 16, 2016 at 05:32:56PM +0000, Srinivas Kandagatla wrote:

> This patch add register ranges for readonly, default and reset register
> values.

This split of the patches is really not helping at all.  The patches
cross reference each other which makes things harder to follow and it's
not like things can be treated independently or there are detaile
changelogs explaining everything separately.

> +static int __msm8x16_wcd_reg_write(struct snd_soc_codec *codec,
> +			unsigned short reg, u8 val)
> +{
> +	int ret = -EINVAL;
> +	struct msm8x16_wcd_chip *chip = dev_get_drvdata(codec->dev);
> +
> +	if (MSM8X16_WCD_IS_TOMBAK_REG(reg)) {
> +		ret = regmap_write(chip->analog_map,
> +				   chip->analog_base + reg, val);
> +	} else if (MSM8X16_WCD_IS_DIGITAL_REG(reg)) {
> +		u32 temp = val & 0x000000FF;
> +		u16 offset = (reg ^ 0x0200) & 0x0FFF;
> +
> +		ret = regmap_write(chip->digital_map, offset, temp);
> +	}
> +
> +	return ret;
> +}

I don't really know what this is supposed to be doing but it looks like
something is wrong.  It seems that it's trying to munge two different
register maps together in some undocumented reason.

> +static int msm8x16_wcd_write(struct snd_soc_codec *codec, unsigned int reg,
> +			     unsigned int value)
> +{
> +	if (reg == SND_SOC_NOPM)
> +		return 0;
> +
> +	WARN_ON(reg > MSM8X16_WCD_MAX_REGISTER);
> +	if (!msm8x16_wcd_volatile(codec, reg))
> +		msm8x16_wcd_reset_reg_defaults[reg] = value;

This appears to be obviously confused.  We're writing to a global
variable as part of the write routine, and worse that global variable is
called _reg_defaults which suggests that it's supposed to be the
register default values not what appears to be a cache implemented
outside of the existing generic cache code.

> +
> +	return __msm8x16_wcd_reg_write(codec, reg, (u8)value);
> +}

It's also not clear why this is a separate wrapper function.

Download attachment "signature.asc" of type "application/pgp-signature" (474 bytes)