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

[Mark Brown]

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.

Attachment: signature.asc
Description: PGP signature