Re: [PATCH v5] ASoC: add RT286 CODEC driver

From: Takashi Iwai <tiwai@suse.de>
To: Lars-Peter Clausen <lars@metafoo.de>
Cc: Oder Chiou <oder_chiou@realtek.com>,
	"alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
	"lgirdwood@gmail.com" <lgirdwood@gmail.com>,
	"broonie@kernel.org" <broonie@kernel.org>,
	Bard Liao <bardliao@realtek.com>,
	Gustaw Lewandowski <gustaw.lewandowski@intel.com>,
	Flove <flove@realtek.com>
Subject: Re: [PATCH v5] ASoC: add RT286 CODEC driver
Date: Thu, 13 Mar 2014 09:52:14 +0100	[thread overview]
Message-ID: <s5h61nizdj5.wl%tiwai@suse.de> (raw)
In-Reply-To: <53216DDC.9090502@metafoo.de>

At Thu, 13 Mar 2014 09:35:40 +0100,
Lars-Peter Clausen wrote:
> 
> On 03/13/2014 06:29 AM, Bard Liao wrote:
> >>> +
> >> [...]
> >>> +static int rt286_update_bits(struct snd_soc_codec *codec, unsigned int vid,
> >>> +				unsigned int nid, unsigned int data,
> >>> +				unsigned int mask, unsigned int value) {
> >>> +	struct rt286_priv *rt286 = snd_soc_codec_get_drvdata(codec);
> >>> +	unsigned int old, new, verb;
> >>> +	int change, ret;
> >>> +
> >>> +	verb = VERB_CMD((vid | 0x800), nid, data);
> >>> +	regmap_read(rt286->regmap, verb, &old);
> >>> +	new = (old & ~mask) | (value & mask);
> >>> +	change = old != new;
> >>> +
> >>> +	if (change) {
> >>> +		verb = VERB_CMD(vid, nid, new);
> >>> +		ret = regmap_write(rt286->regmap, verb, 0);
> >>> +		if (ret < 0) {
> >>> +			dev_err(codec->dev,
> >>> +				"Failed to write private reg: %d\n", ret);
> >>> +			goto err;
> >>> +		}
> >>> +	}
> >>
> >> Can't this use regmap_update_bits()?
> >
> > rt286 use different data length for read/write protocol.
> > Also it uses different registers for read/write the same bit.
> >
> > verb = VERB_CMD((vid | 0x800), nid, data);
> > regmap_read(rt286->regmap, verb, &old);
> > ...
> > verb = VERB_CMD(vid, nid, new);
> > ret = regmap_write(rt286->regmap, verb, 0);
> 
> You need to differentiate between logical and physical addresses. If your 
> device uses different physical addresses for read and write then your read 
> and write functions should do the proper translation from the logical 
> address to the physical address. Looking at include/sound/hda_verbs.h it 
> seems that the GET verbs are always the same as the SET verbs but 
> additionally set bit 11. So bit 11 is your read bit that always needs to be 
> set when reading a register. This is nothing special to the rt286, in fact 
> it is so common that regmap as support for this in the core. See the 
> read_flag_mask for the regmap_config struct.

Note that get and set aren't always symmetrical for HD-audio.
You can get up to 32bit, but set only up to 8 bit, thus some verbs are
split to multiple different set verbs.  Also, some verbs expect
different values for get and set.  And, yet more tricky part is the
amplifier verbs.  These aren't suitable with regmap caching,
unfortunately.

Takashi