LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Richard Fitzgerald <rf@opensource.cirrus.com>
To: Mark Brown <broonie@kernel.org>
Cc: <alsa-devel@alsa-project.org>, <patches@opensource.cirrus.com>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 04/12] ASoC: cs42l42: Don't reconfigure the PLL while it is running
Date: Wed, 11 Aug 2021 13:21:24 +0100	[thread overview]
Message-ID: <bae1afcb-d983-c17c-d932-3afac16df501@opensource.cirrus.com> (raw)
In-Reply-To: <20210811115637.GA4167@sirena.org.uk>

On 11/08/2021 12:56, Mark Brown wrote:
> 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.
> 

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.

As in:

hw_params() substream=PLAYBACK
     configure PLL
prepare() substream=PLAYBACK
     PLL is started
hw_params() substream=CAPTURE
     PLAYBACK substream already running so don't rewrite PLL registers

Some of the PLL configurations start with a "safe" configuration and
then switch over to the running configuration once the PLL is stable.
Calling pll_config() again would rewrite back to the startup config,
which would change the clock output.

It's ok if neither stream is started, since the PLL isn't started. This
is needed anyway because it is legal for hw_params() to be called
several times to change parameters without starting a stream.

  reply	other threads:[~2021-08-11 12:36 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-10 15:37 [PATCH 00/12] ASoC: cs42l42: Series of bugfixes and improvements Richard Fitzgerald
2021-08-10 15:37 ` [PATCH 01/12] ASoC: cs42l42: Use PLL for SCLK > 12.188MHz Richard Fitzgerald
2021-08-10 15:37 ` [PATCH 02/12] ASoC: cs42l42: Don't claim to support 192k Richard Fitzgerald
2021-08-10 15:37 ` [PATCH 03/12] ASoC: cs42l42: Always configure both ASP TX channels Richard Fitzgerald
2021-08-10 15:37 ` [PATCH 04/12] ASoC: cs42l42: Don't reconfigure the PLL while it is running Richard Fitzgerald
2021-08-10 15:49   ` Mark Brown
2021-08-10 16:27     ` Richard Fitzgerald
2021-08-11 11:56       ` Mark Brown
2021-08-11 12:21         ` Richard Fitzgerald [this message]
2021-08-11 14:41           ` Mark Brown
2021-08-11 10:56     ` Richard Fitzgerald
2021-08-10 15:37 ` [PATCH 05/12] ASoC: cs42l42: Set correct SRC MCLK Richard Fitzgerald
2021-08-10 15:37 ` [PATCH 06/12] ASoC: cs42l42: Mark OSC_SWITCH_STATUS register volatile Richard Fitzgerald
2021-08-10 15:37 ` [PATCH 07/12] ASoC: cs42l42: Allow time for HP/ADC to power-up after enable Richard Fitzgerald
2021-08-10 15:37 ` [PATCH 08/12] ASoC: cs42l42: Prevent NULL pointer deref in interrupt handler Richard Fitzgerald
2021-08-10 15:37 ` [PATCH 09/12] ASoC: cs42l42: Remove runtime_suspend/runtime_resume callbacks Richard Fitzgerald
2021-08-10 15:37 ` [PATCH 10/12] ASoC: cs42l42: Remove redundant pll_divout member Richard Fitzgerald
2021-08-10 15:37 ` [PATCH 11/12] ASoC: cs42l42: Add HP Volume Scale control Richard Fitzgerald
2021-08-10 15:37 ` [PATCH 12/12] ASoC: cs42l42: Add control for audio slow-start switch Richard Fitzgerald

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=bae1afcb-d983-c17c-d932-3afac16df501@opensource.cirrus.com \
    --to=rf@opensource.cirrus.com \
    --cc=alsa-devel@alsa-project.org \
    --cc=broonie@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=patches@opensource.cirrus.com \
    --subject='Re: [PATCH 04/12] ASoC: cs42l42: Don'\''t reconfigure the PLL while it is running' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).