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.
