[alsa-devel] [PATCH v5] ASoC: add RT286 CODEC driver

[Bard Liao]

> -----Original Message-----
> From: Lars-Peter Clausen [mailto:lars at metafoo.de]
> Sent: Thursday, March 13, 2014 5:16 AM
> To: Bard Liao
> Cc: broonie at kernel.org; lgirdwood at gmail.com; Oder Chiou;
> alsa-devel at alsa-project.org; Gustaw Lewandowski; Flove
> Subject: Re: [alsa-devel] [PATCH v5] ASoC: add RT286 CODEC driver
> 
> On 03/11/2014 08:11 AM, bardliao at realtek.com wrote:
> [...]
> > +static unsigned int rt286_reg_cache[] = {
> > +	[RT286_AUDIO_FUNCTION_GROUP] = 0x0000,
> > +	[RT286_DAC_OUT1] = 0x7f7f,
> > +	[RT286_DAC_OUT2] = 0x7f7f,
> > +	[RT286_SPDIF] = 0x0000,
> > +	[RT286_ADC_IN1] = 0x4343,
> > +	[RT286_ADC_IN2] = 0x4343,
> > +	[RT286_MIC1] = 0x0000,
> > +	[RT286_MIXER_IN] = 0x000b,
> > +	[RT286_MIXER_OUT1] = 0x0002,
> > +	[RT286_MIXER_OUT2] = 0x0000,
> > +	[RT286_DMIC1] = 0x0000,
> > +	[RT286_DMIC2] = 0x0000,
> > +	[RT286_LINE1] = 0x0000,
> > +	[RT286_BEEP] = 0x0000,
> > +	[RT286_VENDOR_REGISTERS] = 0x0000,
> > +	[RT286_SPK_OUT] = 0x8080,
> > +	[RT286_HP_OUT] = 0x8080,
> > +	[RT286_MIXER_IN1] = 0x0000,
> > +	[RT286_MIXER_IN2] = 0x0000,
> > +};
> > +
> > +static int rt286_hw_read(void *context, unsigned int reg, unsigned
> > +int *value) {
> > +	struct i2c_client *client = context;
> > +	struct i2c_msg xfer[2];
> > +	int ret;
> > +	unsigned int buf = 0x0;
> > +
> > +	if (reg <= 0xff) { /*read cache*/
> > +		*value = rt286_reg_cache[reg];
> 
> Any reason particular reason to not use regmap caching? Also your cache is
> driver global which means different device instances will use the same cache,
> which will probably have very weired effects. Also you cache has less than 0xff
> entries, so the check is completely bogus.

Because rt286 uses different registers for read and write the same bits.
So, I can't read the real value from cache.
The cache I am using is for dummy registers actually.
These dummy registers are mainly used for dapm to get the route information.
We don't touch the codec when read/write these dummy registers.
I will try to put the cache into rt286_priv.
And will change if (reg <= 0xff) to if (reg <= RT286_MIXER_IN2).

> 
> > +	/* Write register */
> > +	xfer[0].addr = client->addr;
> > +	xfer[0].flags = 0;
> > +	xfer[0].len = 4;
> > +	xfer[0].buf = (u8 *)®
> > +
> > +	/* Read data */
> > +	xfer[1].addr = client->addr;
> > +	xfer[1].flags = I2C_M_RD;
> > +	xfer[1].len = 4;
> > +	xfer[1].buf = (u8 *)&buf;
> > +
> > +	ret = i2c_transfer(client->adapter, xfer, 2);
> > +	if (ret < 0)
> > +		return ret;
> > +	else if (ret != 2)
> > +		return -EIO;
> > +
> > +	*value = be32_to_cpu(buf);
> > +
> > +	return 0;
> > +}
> > +
> [...]
> > +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);

I use different reg(verb) for regmap_read and regmap_write.
And set regmap_write's val variable to 0.
I think regmap_update_bits() will do things like
regmap_read(rt286->regmap, verb, &old);
regmap_write(rt286->regmap, verb, new);
with the same verb value.

> 
> > +	return change;
> > +
> > +err:
> > +	return ret;
> > +}
> [...]
> > +static int rt286_index_update_bits(struct snd_soc_codec *codec,
> > +	unsigned int wid, unsigned int index,
> > +	unsigned int mask, unsigned int data) {
> > +	unsigned int old, new;
> > +	int change, ret;
> > +
> > +	old =  rt286_index_read(codec, wid, index);
> > +	new = (old & ~mask) | (data & mask);
> > +	change = old != new;
> > +
> > +	if (change) {
> > +		ret = rt286_index_write(codec, wid, index, new);
> > +		if (ret < 0) {
> > +			dev_err(codec->dev,
> > +				"Failed to write private reg: %d\n", ret);
> > +			goto err;
> > +		}
> > +	}
> 
> Same here.

Same reason as above.

> 
> > +	0, 5,
> > +};
> [...]
> > +static int rt286_set_bclk_ratio(struct snd_soc_dai *dai, unsigned int
> > +ratio) {
> > +	struct snd_soc_codec *codec = dai->codec;
> > +
> > +	dev_dbg(codec->dev, "%s ratio=%d\n", __func__, ratio);
> > +	if (50 == ratio)
> 
> The ratio is the number of bit-clock cycles per lr-clock cycle.

That is exactly the information we need to know.


> 
> > +				GFP_KERNEL);
> > +	if (NULL == rt286)
> > +		return -ENOMEM;
> > +
> > +	rt286->regmap = devm_regmap_init(dev, NULL, i2c, &rt286_regmap);
> > +	if (IS_ERR(rt286->regmap)) {
> > +		ret = PTR_ERR(rt286->regmap);
> > +		dev_err(&i2c->dev, "Failed to allocate register map: %d\n",
> > +			ret);
> > +		return ret;
> > +	}
> > +
> > +	rt286->i2c = i2c;
> > +	i2c_set_clientdata(i2c, rt286);
> > +
> > +	if (pdata)
> > +		rt286->pdata = *pdata;
> > +
> > +	ret = devm_snd_soc_register_codec(&i2c->dev, &soc_codec_dev_rt286,
> > +				     rt286_dai, ARRAY_SIZE(rt286_dai));
> 
> There is no such thing as devm_snd_soc_register_codec().

That is Mark's suggestion.
I suppose devm_snd_soc_register_codec() will be upstreaming soon.
Should I use snd_soc_register_codec() in this patch?

> 
> > +
> > +	return ret;
> > +}
> 
> 
> ------Please consider the environment before printing this e-mail.