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.
Attachment:
signature.asc
Description: PGP signature
