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

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

On 7/26/22 09:56, Mark Brown wrote:
> On Tue, Jul 26, 2022 at 09:06:10AM -0500, Pierre-Louis Bossart wrote:
>> Much improved version, thank you. See additional comments/questions below.
> 
>>> +	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?
> 
> That shouldn't be needed (in general the interrupt enable/disable stuff
> shouldn't be needed at all).

isn't there a risk of an interrupt being triggered after all the jack
detection resources are disabled?

> 
>>> +	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?
> 
> The clock API will happily consume NULL clocks (it uses NULL as an
> equivalent to the regulator API's dummy regulator), there's no *need* to
> skip clock handling if you've got a NULL clock.

Right, so the code in es8326_set_bias_level() can remove the tests for a
NULL mclk.