Re: [PATCH RESEND] ASoC: Support TI Isabelle Audio driver

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2421 bytes --]

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.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]