On Wed, Aug 11, 2021 at 01:21:24PM +0100, Richard Fitzgerald wrote: > On 11/08/2021 12:56, Mark Brown wrote: > > On Tue, Aug 10, 2021 at 05:27:45PM +0100, Richard Fitzgerald wrote: > > > cs42l42_pll_config() could check whether it is already running and skip > > > configuration in that case, but that seems to me a rather opaque > > > implementation. In my opinion this doesn't really fall into the case of > > > ignoring-bad-stuff-to-be-helpful (like free() accepting a NULL). > > This doesn't treat the situation as an error though, it just ignores it, > > and there's nothing to stop _pll_config() generating a warning if that > > makes sense. > It isn't an error. hw_params() will be called for both substreams > (PLAYBACK and CAPTURE) and if one is already running we mustn't > reconfigure the things we already configured. The DAI is marked > symmetric so both substreams will always produce the same I2C BCLK. If it's a noop reconfiguration then there's a case for saying that _pll_config() should just silently do nothing anyway regardless of issues with reconfiguring, though you might also want to warn dpeending on other expectations. If it's not a noop reconfiguration then presumably the new configuration not taking effect might mean that other things aren't going to see the clocks they expect. Either way if a reconfiguration gets introduced via a path other than hw_params(), either now or later, having the check in the _pll_config() would catch it.
Attachment:
signature.asc
Description: PGP signature
