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