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.
I use different reg(verb) for regmap_read and regmap_write. And set regmap_write's val variable to 0.
So you changed the regmap semantics and do pass the value of the register write via address and set value always to zero. In that it would probably been better to not use regmap at all. But as I said, there doesn't seem to be anything special about the device, you just need to implement the read and write callbacks correctly, then it is no problem to use regmap_update_bits and also regmap level caching.
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.
rt286_index_read and rt286_index_write seem to implement a paging mechanism. regmap has native support for paging, see regmap_range_cfg. If you can't use regmap for paging implement the paging mechanism in your read/write callbacks.
- 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?
Yes, otherwise the driver won't compile.