On Tue, May 29, 2012 at 04:39:46PM +0530, MR.Swami.Reddy@ti.com wrote:
he below patch is a basic driver code for TI Isabelle audio codec. The functionalities like headset detection, etc., will be included incrementally in the up-coming patches.
Overall this is very good, there's a few issues below but they're pretty minor and ought to be easy to fix.
Signed-off-by: Vishwas A Deshpande vishwas.a.deshpande@ti.com
You should include signoffs from both of you - it's especailly important that whoever sends the mail signs it off from a legal point of view.
+/* codec private data */ +struct isabelle_priv {
- struct regmap *regmap;
+};
If this is all you need then you should be able to use the newly introduced dev_get_regmap() to get the regmap back (other drivers should be being updated for this soon).
+/*
- MICGAIN volume control:
- from 0 to 30 dB in 1 dB steps
- */
+static const DECLARE_TLV_DB_SCALE(mic_amp_tlv, 0, 100, 0);
These comments are a bit on the verbose side...
case SND_SOC_BIAS_STANDBY:if (codec->dapm.bias_level == SND_SOC_BIAS_OFF)regcache_sync(isabelle->regmap);
Nothing ever marks the cache as dirty so this'll never actually do anything.
+static int isabelle_resume(struct snd_soc_codec *codec) +{
- isabelle_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
- isabelle_set_bias_level(codec, codec->dapm.suspend_bias_level);
As with your previous driver there's no need to use suspend_bias_level the framework will ensure that you're in your idle state before it suspends. Indeed, since your driver sets idle_bias_off there's no need for these suspend and resume functions at all except possibly to invalidate the cache in the suspend.
+static int isabelle_remove(struct snd_soc_codec *codec) +{
- isabelle_set_bias_level(codec, SND_SOC_BIAS_OFF);
- return 0;
+}
No need for this, your driver sets idle_bias_off so it'll be powered down before the driver is removed.
- isabelle->regmap = regmap_init_i2c(i2c, &isabelle_regmap_config);
- if (IS_ERR(isabelle->regmap)) {
devm_regmap_init_i2c().
+static const struct i2c_device_id isabelle_i2c_id[] = {
- { "isabelle", 0 },
- { }
+}; +MODULE_DEVICE_TABLE(i2c, isabelle_i2c_id);
This should really include a list of part numbers - the general expecation people have is that they can just register the part number.