Re: [PATCH v3 1/2] ASoC: codecs: add support for ES8326

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

Much improved version, thank you. See additional comments/questions below.
> +static int es8326_set_bias_level(struct snd_soc_component *codec,
> +				 enum snd_soc_bias_level level)
> +{
> +	struct es8326_priv *es8326 = snd_soc_component_get_drvdata(codec);
> +	int ret;
> +
> +	switch (level) {
> +	case SND_SOC_BIAS_ON:
> +		if (!IS_ERR(es8326->mclk)) {

the IS_ERR case is taken care of in the probe, so should this be

if (es8326->mclk)

for symmetry with the BIAS_OFF case?

> +			ret = clk_prepare_enable(es8326->mclk);
> +			if (ret)
> +				return ret;
> +		}
> +		regmap_write(es8326->regmap, ES8326_RESET_00, ES8326_PWRUP_SEQ_EN);
> +		regmap_write(es8326->regmap, ES8326_INTOUT_IO_59, 0x45);
> +		regmap_write(es8326->regmap, ES8326_SDINOUT1_IO_5A,
> +			    (ES8326_IO_DMIC_CLK << ES8326_SDINOUT1_SHIFT));
> +		regmap_write(es8326->regmap, ES8326_SDINOUT23_IO_5B, ES8326_IO_INPUT);
> +		regmap_write(es8326->regmap, ES8326_CLK_RESAMPLE_03, 0x05);
> +		regmap_write(es8326->regmap, ES8326_VMIDSEL_18, 0x02);
> +		regmap_write(es8326->regmap, ES8326_PGA_PDN_17, 0x40);
> +		regmap_write(es8326->regmap, ES8326_DAC2HPMIX_25, 0xAA);
> +		regmap_write(es8326->regmap, ES8326_RESET_00, ES8326_CSM_ON);
> +		break;
> +	case SND_SOC_BIAS_PREPARE:
> +		break;
> +	case SND_SOC_BIAS_STANDBY:
> +		break;
> +	case SND_SOC_BIAS_OFF:
> +		if (es8326->mclk)> +			clk_disable_unprepare(es8326->mclk);
> +		regmap_write(es8326->regmap, ES8326_DAC2HPMIX_25, 0x11);
> +		regmap_write(es8326->regmap, ES8326_RESET_00, ES8326_CSM_OFF);
> +		regmap_write(es8326->regmap, ES8326_PGA_PDN_17, 0xF8);
> +		regmap_write(es8326->regmap, ES8326_VMIDSEL_18, 0x00);
> +		regmap_write(es8326->regmap, ES8326_INT_SOURCE_58, 0x08);
> +		regmap_write(es8326->regmap, ES8326_SDINOUT1_IO_5A, ES8326_IO_INPUT);
> +		regmap_write(es8326->regmap, ES8326_SDINOUT23_IO_5B, ES8326_IO_INPUT);
> +		regmap_write(es8326->regmap, ES8326_RESET_00,
> +			     ES8326_CODEC_RESET | ES8326_PWRUP_SEQ_EN);
> +		break;
> +	}
> +
> +	return 0;
> +}

> +static irqreturn_t es8326_irq(int irq, void *dev_id)
> +{
> +	struct es8326_priv *es8326 = dev_id;
> +	struct snd_soc_component *comp = es8326->component;
> +
> +	if (!es8326->jack)
> +		goto out;
> +
> +	es8326_enable_micbias(comp);
> +
> +	queue_delayed_work(system_wq, &es8326->jack_detect_work,
> +			   msecs_to_jiffies(300));
> +
> +out:
> +	return IRQ_HANDLED;
> +}
> +
> +static int es8326_resume(struct snd_soc_component *component)
> +{
> +	struct es8326_priv *es8326 = snd_soc_component_get_drvdata(component);
> +
> +	regcache_cache_only(es8326->regmap, false);
> +	regcache_sync(es8326->regmap);
> +
> +	regmap_write(es8326->regmap, ES8326_CLK_CTL_01, ES8326_CLK_ON);
> +	/* Two channel ADC */
> +	regmap_write(es8326->regmap, ES8326_PULLUP_CTL_F9, 0x02);
> +	regmap_write(es8326->regmap, ES8326_CLK_INV_02, 0x00);
> +	regmap_write(es8326->regmap, ES8326_CLK_DIV_CPC_0C, 0x1F);
> +	regmap_write(es8326->regmap, ES8326_CLK_VMIDS1_10, 0xC8);
> +	regmap_write(es8326->regmap, ES8326_CLK_VMIDS2_11, 0x88);
> +	regmap_write(es8326->regmap, ES8326_CLK_CAL_TIME_12, 0x20);
> +	regmap_write(es8326->regmap, ES8326_SYS_BIAS_1D, 0x08);
> +	regmap_write(es8326->regmap, ES8326_DAC2HPMIX_25, 0x22);
> +	regmap_write(es8326->regmap, ES8326_ADC1_SRC_2A, es8326->mic1_src);
> +	regmap_write(es8326->regmap, ES8326_ADC2_SRC_2B, es8326->mic2_src);
> +	regmap_write(es8326->regmap, ES8326_HPJACK_TIMER_56, 0x88);
> +	regmap_write(es8326->regmap, ES8326_HP_DET_57,
> +		     ES8326_HP_DET_SRC_PIN9 | es8326->jack_pol);
> +	regmap_write(es8326->regmap, ES8326_INT_SOURCE_58, 0x08);
> +	regmap_write(es8326->regmap, ES8326_INTOUT_IO_59, 0x45);
> +	regmap_write(es8326->regmap, ES8326_RESET_00, ES8326_CSM_ON);
> +	snd_soc_component_update_bits(component, ES8326_PGAGAIN_23,
> +				      ES8326_MIC_SEL_MASK, ES8326_MIC1_SEL);
> +
> +	es8326_irq(es8326->irq, es8326);
> +	return 0;
> +}
> +
> +static int es8326_suspend(struct snd_soc_component *component)
> +{
> +	struct es8326_priv *es8326 = snd_soc_component_get_drvdata(component);
> +
> +	es8326_disable_micbias(component);
> +	cancel_delayed_work_sync(&es8326->jack_detect_work);

I would have swapped the two lines above, first cancel the workqueue
(which means let it run if it started already).

> +
> +	regmap_write(es8326->regmap, ES8326_CLK_CTL_01, ES8326_CLK_OFF);
> +	regcache_cache_only(es8326->regmap, true);
> +	regcache_mark_dirty(es8326->regmap);
> +
> +	return 0;
> +}

One question on the interrupt handling: should there be an interrupt
disable on suspend and conversely an interrupt enable on resume?

> +
> +static int es8326_probe(struct snd_soc_component *component)
> +{
> +	struct es8326_priv *es8326 = snd_soc_component_get_drvdata(component);
> +	int ret;
> +
> +	es8326->component = component;
> +	es8326->jd_inverted = device_property_read_bool(component->dev,
> +							"everest,jack-detect-inverted");
> +
> +	ret = device_property_read_u8(component->dev, "everest,mic1-src", &es8326->mic1_src);
> +	if (ret != 0) {
> +		dev_dbg(component->dev, "mic1-src return %d", ret);
> +		es8326->mic1_src = ES8326_ADC_AMIC;
> +	}
> +	dev_dbg(component->dev, "mic1-src %x", es8326->mic1_src);
> +
> +	ret = device_property_read_u8(component->dev, "everest,mic2-src", &es8326->mic2_src);
> +	if (ret != 0) {
> +		dev_dbg(component->dev, "mic2-src return %d", ret);
> +		es8326->mic2_src = ES8326_ADC_DMIC;
> +	}
> +	dev_dbg(component->dev, "mic2-src %x", es8326->mic2_src);
> +
> +	ret = device_property_read_u8(component->dev, "everest,jack-pol", &es8326->jack_pol);
> +	if (ret != 0) {
> +		dev_dbg(component->dev, "jack-pol return %d", ret);
> +		es8326->jack_pol = ES8326_HP_DET_BUTTON_POL | ES8326_HP_TYPE_OMTP;
> +	}
> +	dev_dbg(component->dev, "jack-pol %x", es8326->jack_pol);
> +
> +	es8326_resume(component);
> +	return 0;
> +}

> +static int es8326_i2c_probe(struct i2c_client *i2c,
> +			    const struct i2c_device_id *id)
> +{
> +	struct es8326_priv *es8326;
> +	int ret;
> +
> +	es8326 = devm_kzalloc(&i2c->dev, sizeof(struct es8326_priv), GFP_KERNEL);
> +	if (!es8326)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(i2c, es8326);
> +	es8326->i2c = i2c;
> +	mutex_init(&es8326->lock);
> +	es8326->regmap = devm_regmap_init_i2c(i2c, &es8326_regmap_config);
> +	if (IS_ERR(es8326->regmap)) {
> +		ret = PTR_ERR(es8326->regmap);
> +		dev_err(&i2c->dev, "Failed to init regmap: %d\n", ret);
> +		return ret;
> +	}
> +
> +	es8326->irq = i2c->irq;
> +	INIT_DELAYED_WORK(&es8326->jack_detect_work,
> +			  es8326_jack_detect_handler);
> +	/* ES8316 is level-based while ES8326 is edge-based */
> +	ret = devm_request_threaded_irq(&i2c->dev, es8326->irq, NULL, es8326_irq,
> +					IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> +					"es8326", es8326);
> +	if (ret) {
> +		dev_warn(&i2c->dev, "Failed to reguest IRQ: %d: %d\n",

typo: request

> +		es8326->irq, ret);
> +		es8326->irq = -ENXIO;
> +	}
> +
> +	es8326->mclk = devm_clk_get_optional(&i2c->dev, "mclk");
> +	if (IS_ERR(es8326->mclk)) {
> +		dev_err(&i2c->dev, "unable to get mclk\n");
> +		return PTR_ERR(es8326->mclk);
> +	}
> +	if (!es8326->mclk)
> +		dev_warn(&i2c->dev, "assuming static mclk\n");
> +
> +	ret = clk_prepare_enable(es8326->mclk);
> +	if (ret) {
> +		dev_err(&i2c->dev, "unable to enable mclk\n");
> +		return ret;
> +	}

I am not really following what happens if es8326->mclk is NULL. Why
would you call clk_prepare_enable() with a NULL pointer? If you look at
the code in es8326_set_bias_level(), you do test for that case, so why
not here? Something's not right here.

Could it be that this is a scope issue? This block should be moved under
the scope of the if (!es8236->mclk) test, no?


> +	return devm_snd_soc_register_component(&i2c->dev,
> +					&soc_component_dev_es8326,
> +					&es8326_dai, 1);
> +}