mbox series

[v2,0/3] ASoC: wm8782: Allow higher audio rates

Message ID 20230913171552.92252-1-contact@jookia.org
Headers show
Series ASoC: wm8782: Allow higher audio rates | expand

Message

John Watts Sept. 13, 2023, 5:15 p.m. UTC 
The wm8782 supports higher audio rates than just 48kHz. This is
configured by setting the FSAMPEN pin on the codec chip.

This patch series introduces the 'wlf,fsampen' device tree property
to indicate the pin status and control the maximum rate available
when using the codec.

v1 -> v2:
- Switched from max-rate property to wlf,fsampen property
- Clarified property is optional, not required

John Watts (3):
  ASoC: wm8782: Handle maximum audio rate at runtime
  ASoC: wm8782: Use wlf,fsampen device tree property
  ASoC: dt-bindings: wlf,wm8782: Add wlf,fsampen property

 .../devicetree/bindings/sound/wm8782.txt      |  5 ++
 sound/soc/codecs/wm8782.c                     | 66 +++++++++++++++----
 2 files changed, 58 insertions(+), 13 deletions(-)

Comments

John Watts Sept. 14, 2023, 9:27 a.m. UTC | #1 
On Thu, Sep 14, 2023 at 09:21:07AM +0000, Charles Keepax wrote:
> On Thu, Sep 14, 2023 at 03:15:50AM +1000, John Watts wrote:
> > The wm8782 supports up to 192kHz audio when pins are set correctly.
> > Instead of hardcoding which rates are supported enable them all
> > then refer to a max_rate variable at runtime.
> > 
> > Signed-off-by: John Watts <contact@jookia.org>
> > ---
> > +static int wm8782_dai_hw_params(struct snd_pcm_substream *component,
> > +			    struct snd_pcm_hw_params *params,
> > +			    struct snd_soc_dai *dai)
> > +{
> > +	struct wm8782_priv *priv =
> > +		snd_soc_component_get_drvdata(dai->component);
> > +
> > +	if (params_rate(params) > priv->max_rate)
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> 
> We should be setting this as a constraint in startup, rather
> than returning an error in hw_params. That will let user-space
> know the supported rates and allow it to resample if necessary.

How do you do this? The struct with the rate is statically defined.

> 
> Thanks,
> Charles

John.
Charles Keepax Sept. 14, 2023, 9:37 a.m. UTC | #2 
On Thu, Sep 14, 2023 at 07:27:03PM +1000, John Watts wrote:
> On Thu, Sep 14, 2023 at 09:21:07AM +0000, Charles Keepax wrote:
> > On Thu, Sep 14, 2023 at 03:15:50AM +1000, John Watts wrote:
> > > The wm8782 supports up to 192kHz audio when pins are set correctly.
> > > Instead of hardcoding which rates are supported enable them all
> > > then refer to a max_rate variable at runtime.
> > > 
> > > Signed-off-by: John Watts <contact@jookia.org>
> > > ---
> > > +static int wm8782_dai_hw_params(struct snd_pcm_substream *component,
> > > +			    struct snd_pcm_hw_params *params,
> > > +			    struct snd_soc_dai *dai)
> > > +{
> > > +	struct wm8782_priv *priv =
> > > +		snd_soc_component_get_drvdata(dai->component);
> > > +
> > > +	if (params_rate(params) > priv->max_rate)
> > > +		return -EINVAL;
> > > +
> > > +	return 0;
> > > +}
> > 
> > We should be setting this as a constraint in startup, rather
> > than returning an error in hw_params. That will let user-space
> > know the supported rates and allow it to resample if necessary.
> 
> How do you do this? The struct with the rate is statically defined.
> 

You can programmatically add additional constraints, commonly
this will be done from the startup callback on the DAI. See
something like arizona_startup in sound/soc/codecs/arizona.c for
an example, that enables 44.1/48k rates based on clocks but the
principle should be similar.

Thanks,
Charles
Charles Keepax Sept. 14, 2023, 9:44 a.m. UTC | #3 
On Thu, Sep 14, 2023 at 09:37:31AM +0000, Charles Keepax wrote:
> On Thu, Sep 14, 2023 at 07:27:03PM +1000, John Watts wrote:
> > On Thu, Sep 14, 2023 at 09:21:07AM +0000, Charles Keepax wrote:
> > > On Thu, Sep 14, 2023 at 03:15:50AM +1000, John Watts wrote:
> > > > The wm8782 supports up to 192kHz audio when pins are set correctly.
> > > > Instead of hardcoding which rates are supported enable them all
> > > > then refer to a max_rate variable at runtime.
> > > > 
> > > > Signed-off-by: John Watts <contact@jookia.org>
> > > > ---
> > > > +static int wm8782_dai_hw_params(struct snd_pcm_substream *component,
> > > > +			    struct snd_pcm_hw_params *params,
> > > > +			    struct snd_soc_dai *dai)
> > > > +{
> > > > +	struct wm8782_priv *priv =
> > > > +		snd_soc_component_get_drvdata(dai->component);
> > > > +
> > > > +	if (params_rate(params) > priv->max_rate)
> > > > +		return -EINVAL;
> > > > +
> > > > +	return 0;
> > > > +}
> > > 
> > > We should be setting this as a constraint in startup, rather
> > > than returning an error in hw_params. That will let user-space
> > > know the supported rates and allow it to resample if necessary.
> > 
> > How do you do this? The struct with the rate is statically defined.
> > 
> 
> You can programmatically add additional constraints, commonly
> this will be done from the startup callback on the DAI. See
> something like arizona_startup in sound/soc/codecs/arizona.c for
> an example, that enables 44.1/48k rates based on clocks but the
> principle should be similar.
> 

Although I would also imagine snd_pcm_hw_constraint_minmax is
going to be more appropriate in your case.

Thanks,
Charles