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

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);
  

+}