Re: [PATCH] ASoC: nau8825: Add driver for headset chip Nuvoton 8825

From: Mark Brown <broonie@kernel.org>
To: Anatol Pomozov <anatol.pomozov@gmail.com>
Cc: alsa-devel@alsa-project.org, lars@metafoo.de,
	lgirdwood@gmail.com, mfelton1@nuvoton.com, kcso@nuvoton.com,
	ccchang12@nuvoton.com, dgreid@google.com, mhkuo@nuvoton.com
Subject: Re: [PATCH] ASoC: nau8825: Add driver for headset chip Nuvoton 8825
Date: Fri, 31 Jul 2015 19:27:16 +0100	[thread overview]
Message-ID: <20150731182716.GO20873@sirena.org.uk> (raw)
In-Reply-To: <1438038837-339-1-git-send-email-anatol.pomozov@gmail.com>

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

On Mon, Jul 27, 2015 at 04:13:57PM -0700, Anatol Pomozov wrote:

Looks mostly good, a few things below though:

> +		break;
> +	case 1:
> +		dev_info(nau8825->dev, "OMTP (micgnd1) mic connected\n");

This is far too noisy - these should be dev_dbg() at most.

> +		} else {
> +			dev_warn(nau8825->dev, "Headset completion IRQ fired but no headset connected\n");
> +			nau8825_eject_jack(nau8825);
> +		}

Things like this that aren't supposed to happen are fine but normal
operation shouild be quiet.

> +	/* VMID Enable and Tieoff */
> +	regmap_write(regmap, NAU8825_REG_BIAS_ADJ, 0x0060);

You're leavinng VMID enabled all the timme?

> +	/* Jack Detect pull up (High=eject, Low=insert) */
> +	regmap_write(regmap, NAU8825_REG_GPIO12_CTRL, 0x0800);

This seems like it should be a board setting?

> +	/* Setup SAR ADC */
> +	regmap_write(regmap, NAU8825_REG_SAR_CTRL, 0x0280);

Lots of magic numbers in these things...  some of them I can guess
what's going on but this one is a bit obscure, perhaps it should be user
controllable?

> +	/* Setup ADC x128 OSR */
> +	regmap_write(regmap, NAU8825_REG_ADC_RATE, 0x0002);
> +	/* Setup DAC x128 OSR */
> +	regmap_write(regmap, NAU8825_REG_DAC_CTRL1, 0x0082);

I'd expect this to be user controllable.

> +static int nau8825_i2c_probe(struct i2c_client *i2c,
> +	const struct i2c_device_id *id)
> +{
> +	struct device *dev = &i2c->dev;
> +	struct nau8825 *nau8825;
> +	int ret, value;
> +
> +	nau8825 = devm_kzalloc(dev, sizeof(*nau8825), GFP_KERNEL);
> +	if (!nau8825)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(i2c, nau8825);
> +
> +	nau8825->regmap = devm_regmap_init_i2c(i2c, &nau8825_regmap_config);
> +	if (IS_ERR(nau8825->regmap))
> +		return PTR_ERR(nau8825->regmap);
> +	nau8825->irq = i2c->irq;
> +	nau8825->dev = dev;
> +
> +	nau8825_reset_chip(nau8825->regmap);
> +	ret = regmap_read(nau8825->regmap, NAU8825_REG_I2C_DEVICE_ID, &value);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to read device id from the NAU8825: %d\n",
> +			ret);
> +		return ret;
> +	}
> +	if ((value & NAU8825_SOFTWARE_ID_MASK) !=
> +			NAU8825_SOFTWARE_ID_NAU8825) {
> +		dev_err(dev, "Not a NAU8825 chip\n");
> +		return -ENODEV;
> +	}
> +
> +	return snd_soc_register_codec(&i2c->dev, &nau8825_codec_driver,
> +			&nau8825_dai, 1);
> +}

I'd expect any initial register initialistion to happen here (if only so
we save power until the card registers).

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

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