[PATCH v3] ASoC: wm8524: enable constraints when sysclk is configured.

[PATCH v3] ASoC: wm8524: enable constraints when sysclk is configured.

[Shengjiu Wang]

In some cases, the sysclk won't be configured on init, and sysclk can be
changed in hw_params() according to different sample rate, for example,
for 44kHz sample rate, the sysclk is 11.2896MHz, for 48kHz sample rate,
the sysclk is 12.288MHz.

In order to support the above case, only enable constraints when sysclk
is configured, and check the rate in hw_params.

So overall there are three cases that need to be considered:
- call set_sysclk() on init, then constraints will be initialized.
- don't call set_sysclk() on init, but call it after startup(), then
  constraints will be configured, the constraints can be cleared with
  call set_sysclk() again in shutdown().
- don't call set_sysclk() in the whole flow, then there are no any
  constraints. The clocks depend on cpu dai.

Enlarge the WM8524_NUM_RATES to 12, as the supported rate range is 8kHz
to 192kHz.

Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
changes in v3
- Add rate check in hw_params, and enlarge NUM_RATES to 12.

changes in v2
- Don't remove constraints, but only enable constraints when sysclk
  is configured.

 sound/soc/codecs/wm8524.c | 55 ++++++++++++++++++++++++++++++---------
 1 file changed, 42 insertions(+), 13 deletions(-)

diff --git a/sound/soc/codecs/wm8524.c b/sound/soc/codecs/wm8524.c
index 403e513f3fa8..6b1a7450b0ac 100644
--- a/sound/soc/codecs/wm8524.c
+++ b/sound/soc/codecs/wm8524.c
@@ -21,7 +21,7 @@
 #include <sound/soc.h>
 #include <sound/initval.h>
 
-#define WM8524_NUM_RATES 7
+#define WM8524_NUM_RATES 12
 
 /* codec private data */
 struct wm8524_priv {
@@ -46,7 +46,7 @@ static const struct snd_soc_dapm_route wm8524_dapm_routes[] = {
 static const struct {
 	int value;
 	int ratio;
-} lrclk_ratios[WM8524_NUM_RATES] = {
+} lrclk_ratios[] = {
 	{ 1, 128 },
 	{ 2, 192 },
 	{ 3, 256 },
@@ -63,17 +63,12 @@ static int wm8524_startup(struct snd_pcm_substream *substream,
 	struct wm8524_priv *wm8524 = snd_soc_component_get_drvdata(component);
 
 	/* The set of sample rates that can be supported depends on the
-	 * MCLK supplied to the CODEC - enforce this.
+	 * MCLK supplied to the CODEC.
 	 */
-	if (!wm8524->sysclk) {
-		dev_err(component->dev,
-			"No MCLK configured, call set_sysclk() on init\n");
-		return -EINVAL;
-	}
-
-	snd_pcm_hw_constraint_list(substream->runtime, 0,
-				   SNDRV_PCM_HW_PARAM_RATE,
-				   &wm8524->rate_constraint);
+	if (wm8524->sysclk)
+		snd_pcm_hw_constraint_list(substream->runtime, 0,
+					   SNDRV_PCM_HW_PARAM_RATE,
+					   &wm8524->rate_constraint);
 
 	gpiod_set_value_cansleep(wm8524->mute, 1);
 
@@ -97,9 +92,11 @@ static int wm8524_set_dai_sysclk(struct snd_soc_dai *codec_dai,
 	unsigned int val;
 	int i, j = 0;
 
+	wm8524->rate_constraint.count = 0;
 	wm8524->sysclk = freq;
+	if (!wm8524->sysclk)
+		return 0;
 
-	wm8524->rate_constraint.count = 0;
 	for (i = 0; i < ARRAY_SIZE(lrclk_ratios); i++) {
 		val = freq / lrclk_ratios[i].ratio;
 		/* Check that it's a standard rate since core can't
@@ -108,9 +105,13 @@ static int wm8524_set_dai_sysclk(struct snd_soc_dai *codec_dai,
 		 */
 		switch (val) {
 		case 8000:
+		case 11025:
+		case 16000:
+		case 22050:
 		case 32000:
 		case 44100:
 		case 48000:
+		case 64000:
 		case 88200:
 		case 96000:
 		case 176400:
@@ -157,6 +158,33 @@ static int wm8524_mute_stream(struct snd_soc_dai *dai, int mute, int stream)
 	return 0;
 }
 
+static int wm8524_hw_params(struct snd_pcm_substream *substream,
+			    struct snd_pcm_hw_params *params,
+			    struct snd_soc_dai *dai)
+{
+	struct snd_soc_component *component = dai->component;
+	struct wm8524_priv *wm8524 = snd_soc_component_get_drvdata(component);
+	int i;
+
+	/* If sysclk is not configured, no need to check the rate */
+	if (!wm8524->sysclk)
+		return 0;
+
+	/* Find a supported LRCLK rate */
+	for (i = 0; i < wm8524->rate_constraint.count; i++) {
+		if (wm8524->rate_constraint.list[i] == params_rate(params))
+			break;
+	}
+
+	if (i == wm8524->rate_constraint.count) {
+		dev_err(component->dev, "LRCLK %d unsupported with MCLK %d\n",
+			params_rate(params), wm8524->sysclk);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 #define WM8524_RATES SNDRV_PCM_RATE_8000_192000
 
 #define WM8524_FORMATS (SNDRV_PCM_FMTBIT_S16_LE |\
@@ -169,6 +197,7 @@ static const struct snd_soc_dai_ops wm8524_dai_ops = {
 	.set_sysclk	= wm8524_set_dai_sysclk,
 	.set_fmt	= wm8524_set_fmt,
 	.mute_stream	= wm8524_mute_stream,
+	.hw_params	= wm8524_hw_params,
 };
 
 static struct snd_soc_dai_driver wm8524_dai = {
-- 
2.34.1

Re: [PATCH v3] ASoC: wm8524: enable constraints when sysclk is configured.

[Charles Keepax]

On Fri, Jun 20, 2025 at 10:14:03AM +0800, Shengjiu Wang wrote:
> In some cases, the sysclk won't be configured on init, and sysclk can be
> changed in hw_params() according to different sample rate, for example,
> for 44kHz sample rate, the sysclk is 11.2896MHz, for 48kHz sample rate,
> the sysclk is 12.288MHz.
> 
> In order to support the above case, only enable constraints when sysclk
> is configured, and check the rate in hw_params.
> 
> So overall there are three cases that need to be considered:
> - call set_sysclk() on init, then constraints will be initialized.
> - don't call set_sysclk() on init, but call it after startup(), then
>   constraints will be configured, the constraints can be cleared with
>   call set_sysclk() again in shutdown().
> - don't call set_sysclk() in the whole flow, then there are no any
>   constraints. The clocks depend on cpu dai.
> 
> Enlarge the WM8524_NUM_RATES to 12, as the supported rate range is 8kHz
> to 192kHz.
> 
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> ---
> +static int wm8524_hw_params(struct snd_pcm_substream *substream,
> +			    struct snd_pcm_hw_params *params,
> +			    struct snd_soc_dai *dai)
> +{
> +	struct snd_soc_component *component = dai->component;
> +	struct wm8524_priv *wm8524 = snd_soc_component_get_drvdata(component);
> +	int i;
> +
> +	/* If sysclk is not configured, no need to check the rate */
> +	if (!wm8524->sysclk)
> +		return 0;

This is kinda the opposite of what I was hoping we could do. The
idea was to make sure we returned an error if we can't support
the given rate. So if we don't have the constraint, we check the
value in hw_params. This looks like it checks in hw_params only
in the case the constraint existed, but in that case there is no
need to check because we had the constraint.

Thanks,
Charles

> +	/* Find a supported LRCLK rate */
> +	for (i = 0; i < wm8524->rate_constraint.count; i++) {
> +		if (wm8524->rate_constraint.list[i] == params_rate(params))
> +			break;
> +	}
> +
> +	if (i == wm8524->rate_constraint.count) {
> +		dev_err(component->dev, "LRCLK %d unsupported with MCLK %d\n",
> +			params_rate(params), wm8524->sysclk);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}

Re: [PATCH v3] ASoC: wm8524: enable constraints when sysclk is configured.

[Charles Keepax]

On Fri, Jun 20, 2025 at 09:26:00AM +0100, Charles Keepax wrote:
> On Fri, Jun 20, 2025 at 10:14:03AM +0800, Shengjiu Wang wrote:
> > In some cases, the sysclk won't be configured on init, and sysclk can be
> > changed in hw_params() according to different sample rate, for example,
> > for 44kHz sample rate, the sysclk is 11.2896MHz, for 48kHz sample rate,
> > the sysclk is 12.288MHz.
> > 
> > In order to support the above case, only enable constraints when sysclk
> > is configured, and check the rate in hw_params.
> > 
> > So overall there are three cases that need to be considered:
> > - call set_sysclk() on init, then constraints will be initialized.
> > - don't call set_sysclk() on init, but call it after startup(), then
> >   constraints will be configured, the constraints can be cleared with
> >   call set_sysclk() again in shutdown().
> > - don't call set_sysclk() in the whole flow, then there are no any
> >   constraints. The clocks depend on cpu dai.
> > 
> > Enlarge the WM8524_NUM_RATES to 12, as the supported rate range is 8kHz
> > to 192kHz.
> > 
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > ---
> > +static int wm8524_hw_params(struct snd_pcm_substream *substream,
> > +			    struct snd_pcm_hw_params *params,
> > +			    struct snd_soc_dai *dai)
> > +{
> > +	struct snd_soc_component *component = dai->component;
> > +	struct wm8524_priv *wm8524 = snd_soc_component_get_drvdata(component);
> > +	int i;
> > +
> > +	/* If sysclk is not configured, no need to check the rate */
> > +	if (!wm8524->sysclk)
> > +		return 0;
> 
> This is kinda the opposite of what I was hoping we could do. The
> idea was to make sure we returned an error if we can't support
> the given rate. So if we don't have the constraint, we check the
> value in hw_params. This looks like it checks in hw_params only
> in the case the constraint existed, but in that case there is no
> need to check because we had the constraint.
> 

Although perhaps I am mistaken here, I guess is the clock has
been set by the machine driver then we would pass this check.
Would it perhaps make more sense to return an error rather than
zero here?

Thanks,
Charles

Re: [PATCH v3] ASoC: wm8524: enable constraints when sysclk is configured.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1175 bytes --]

On Fri, Jun 20, 2025 at 09:42:58AM +0100, Charles Keepax wrote:
> On Fri, Jun 20, 2025 at 09:26:00AM +0100, Charles Keepax wrote:

> > This is kinda the opposite of what I was hoping we could do. The
> > idea was to make sure we returned an error if we can't support
> > the given rate. So if we don't have the constraint, we check the
> > value in hw_params. This looks like it checks in hw_params only
> > in the case the constraint existed, but in that case there is no
> > need to check because we had the constraint.

> Although perhaps I am mistaken here, I guess is the clock has
> been set by the machine driver then we would pass this check.
> Would it perhaps make more sense to return an error rather than
> zero here?

The link hw_params() should be run before the CODEC ones so we would be
able to insist on the clocks having been configured first.  

Or I wonder if it might be easier to just implement clock API support in
the driver and if we get a MCLK we set it to a sensible value here?
That wouldn't work if the MCLK is coming from the other DAI though.
Also I'm really not sure how this bikeshed fits into the design concept
here.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3] ASoC: wm8524: enable constraints when sysclk is configured.

[Charles Keepax]

On Fri, Jun 20, 2025 at 01:42:20PM +0100, Mark Brown wrote:
> On Fri, Jun 20, 2025 at 09:42:58AM +0100, Charles Keepax wrote:
> > On Fri, Jun 20, 2025 at 09:26:00AM +0100, Charles Keepax wrote:
> 
> > > This is kinda the opposite of what I was hoping we could do. The
> > > idea was to make sure we returned an error if we can't support
> > > the given rate. So if we don't have the constraint, we check the
> > > value in hw_params. This looks like it checks in hw_params only
> > > in the case the constraint existed, but in that case there is no
> > > need to check because we had the constraint.
> 
> > Although perhaps I am mistaken here, I guess is the clock has
> > been set by the machine driver then we would pass this check.
> > Would it perhaps make more sense to return an error rather than
> > zero here?
> 
> The link hw_params() should be run before the CODEC ones so we would be
> able to insist on the clocks having been configured first.  
> 
> Or I wonder if it might be easier to just implement clock API support in
> the driver and if we get a MCLK we set it to a sensible value here?
> That wouldn't work if the MCLK is coming from the other DAI though.
> Also I'm really not sure how this bikeshed fits into the design concept
> here.

I think clock framework stuff is probably more work than makes
sense here.

If you are happy with this as is I don't object to it getting
merged, ultimately if the machine driver doesn't configure the
clock that is a bug in the machine driver and it will likely be
relatively obvious audibily so the returning an error is really
just a nice to have.

Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>

Thanks,
Charles

Re: [PATCH v3] ASoC: wm8524: enable constraints when sysclk is configured.

[Shengjiu Wang]

On Sat, Jun 21, 2025 at 12:02 AM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
>
> On Fri, Jun 20, 2025 at 01:42:20PM +0100, Mark Brown wrote:
> > On Fri, Jun 20, 2025 at 09:42:58AM +0100, Charles Keepax wrote:
> > > On Fri, Jun 20, 2025 at 09:26:00AM +0100, Charles Keepax wrote:
> >
> > > > This is kinda the opposite of what I was hoping we could do. The
> > > > idea was to make sure we returned an error if we can't support
> > > > the given rate. So if we don't have the constraint, we check the
> > > > value in hw_params. This looks like it checks in hw_params only
> > > > in the case the constraint existed, but in that case there is no
> > > > need to check because we had the constraint.
> >
> > > Although perhaps I am mistaken here, I guess is the clock has
> > > been set by the machine driver then we would pass this check.
> > > Would it perhaps make more sense to return an error rather than
> > > zero here?
> >
> > The link hw_params() should be run before the CODEC ones so we would be
> > able to insist on the clocks having been configured first.
> >
> > Or I wonder if it might be easier to just implement clock API support in
> > the driver and if we get a MCLK we set it to a sensible value here?
> > That wouldn't work if the MCLK is coming from the other DAI though.
> > Also I'm really not sure how this bikeshed fits into the design concept
> > here.
>
> I think clock framework stuff is probably more work than makes
> sense here.
>
> If you are happy with this as is I don't object to it getting
> merged, ultimately if the machine driver doesn't configure the
> clock that is a bug in the machine driver and it will likely be
> relatively obvious audibily so the returning an error is really
> just a nice to have.

The code here wants to avoid returning an error when the machine driver
doesn't configure the sysclk. As wm8524 is a slave codec, it don't have
any registers to be configured, it can work without sysclk configured.

Maybe we can release this flexibility for wm8524?

Best regards
Shengjiu Wang

>
> Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>
>
> Thanks,
> Charles

Re: [PATCH v3] ASoC: wm8524: enable constraints when sysclk is configured.

[Mark Brown]

On Fri, 20 Jun 2025 10:14:03 +0800, Shengjiu Wang wrote:
> In some cases, the sysclk won't be configured on init, and sysclk can be
> changed in hw_params() according to different sample rate, for example,
> for 44kHz sample rate, the sysclk is 11.2896MHz, for 48kHz sample rate,
> the sysclk is 12.288MHz.
> 
> In order to support the above case, only enable constraints when sysclk
> is configured, and check the rate in hw_params.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/1] ASoC: wm8524: enable constraints when sysclk is configured.
      commit: 17cc308b183308bf5ada36e164284fff7eb064ba

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark