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.