| Message ID | 20210810153759.24333-5-rf@opensource.cirrus.com |
|---|---|
| State | New |
| Headers | show |
| Series | ASoC: cs42l42: Series of bugfixes and improvements | expand |
On 10/08/2021 16:49, Mark Brown wrote: > On Tue, Aug 10, 2021 at 04:37:51PM +0100, Richard Fitzgerald wrote: >> cs42l42_pcm_hw_params() must only configure the PLL if this is the first >> stream to become active, otherwise it will be overwriting the registers >> while the PLL is running. > > Shouldn't the PLL code be noticing problematic attempts to reconfigure > the PLL while it's active rather than the individual callers? > It's wrong for a hw_params() for one stream to try to configure the PLL when the other stream has already called hw_params(), configured the PLL and started it. E.g. if you started a PLAYBACK, configured and started everything, then got another hw_params() for the CAPTURE. 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). hw_params() deals with both playback and capture streams so it makes sense to me that it should contain logic to ensure it isn't stomping on the other stream it already set up, rather than have everything it calls figure out whether it shouldn't have done that.
On Tue, Aug 10, 2021 at 05:27:45PM +0100, Richard Fitzgerald wrote: > On 10/08/2021 16:49, Mark Brown wrote: > > Shouldn't the PLL code be noticing problematic attempts to reconfigure > > the PLL while it's active rather than the individual callers? > It's wrong for a hw_params() for one stream to try to configure the PLL > when the other stream has already called hw_params(), configured the PLL > and started it. E.g. if you started a PLAYBACK, configured and > started everything, then got another hw_params() for the CAPTURE. > 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.
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.
diff --git a/sound/soc/codecs/cs42l42.c b/sound/soc/codecs/cs42l42.c index 5dc3a30272a4..1893d3694570 100644 --- a/sound/soc/codecs/cs42l42.c +++ b/sound/soc/codecs/cs42l42.c @@ -884,7 +884,11 @@ static int cs42l42_pcm_hw_params(struct snd_pcm_substream *substream, break; } - return cs42l42_pll_config(component); + /* Configure the PLL if this is the first active stream */ + if (!cs42l42->stream_use) + return cs42l42_pll_config(component); + else + return 0; } static int cs42l42_set_sysclk(struct snd_soc_dai *dai,
cs42l42_pcm_hw_params() must only configure the PLL if this is the first stream to become active, otherwise it will be overwriting the registers while the PLL is running. Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com> Fixes: 43fc357199f9 ("ASoC: cs42l42: Set clock source for both ways of stream") --- sound/soc/codecs/cs42l42.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)