Hi
On Fri, Jul 31, 2015 at 11:27 AM, Mark Brown broonie@kernel.org wrote:
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.
Done
} 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?
Currently yes. One of the next steps is to look at the power management for this chip (set_bias and suspend/resume). I need to work with Nuvoton HW engineers to find correct sequence of steps.
/* 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?
Created DTS property for it. Thanks.
/* Setup SAR ADC */regmap_write(regmap, NAU8825_REG_SAR_CTRL, 0x0280);Lots of magic numbers in these things...
These init config settings recommended by Nuvoton engineers. I found that chip requires quite a lot of initial preconfiguration to get it into operating state.
some of them I can guess what's going on but this one is a bit obscure, perhaps it should be user controllable?
This setting configures SARADC (ADC for sensing button presses). It sets sample rate, series resistor and ADC voltage range. It does not look like they need to be user-controllable.
/* Setup ADC x128 OSR */Moved toregmap_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.
The oversampling configuration is important for chip audio quality. There is audible hissing without these settings.
My understanding that all users need to set these values and it is better to move it to the driver initialization sequence.
+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).
Moved most of the initialization here. The only part is left in codec_probe() is interruption initialization via I2S master mode toggling. That toggling trick depends on MCLK signal to initialize interruption block correctly. In our case (a tegra SoC device) MCLK is initialized later at audio platform driver probe.