* RE: [PATCH] ASoC: AMD: Configure master codec on all playback/capture cases
2019-02-15 7:06 [PATCH] ASoC: AMD: Configure master codec on all playback/capture cases Agrawal, Akshu
@ 2019-02-15 9:58 ` Adam Thomson
2019-02-18 18:18 ` Mark Brown
2019-03-04 17:49 ` Guenter Roeck
2 siblings, 0 replies; 5+ messages in thread
From: Adam Thomson @ 2019-02-15 9:58 UTC (permalink / raw)
To: Agrawal, Akshu
Cc: djkurtz, Deucher, Alexander, Adam Thomson, Liam Girdwood,
Mark Brown, Jaroslav Kysela, Takashi Iwai, Mukunda, Vijendar,
Wei Yongjun, Kuninori Morimoto,
moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...,
open list
On 15 February 2019 07:06, Akshu Agrawal wrote:
> In the system design da7219 is the master codec and clocks are generated by it.
> Bclk is to be generated at the required rate for other codecs used when
> da7219 is acting only as clock master. For this call hw_params of da7219 during
> playback/capture on non da7219 codecs.
>
> Being able to set bclk at lower rate also fixes noise issue observed on some
> dmics.
>
> Signed-off-by: Akshu Agrawal <akshu.agrawal@amd.com>
Reviewed-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
> ---
> sound/soc/amd/acp-da7219-max98357a.c | 20 ++++++++++++++++++--
> 1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/sound/soc/amd/acp-da7219-max98357a.c b/sound/soc/amd/acp-
> da7219-max98357a.c
> index a5daad973ce5..ad327415290a 100644
> --- a/sound/soc/amd/acp-da7219-max98357a.c
> +++ b/sound/soc/amd/acp-da7219-max98357a.c
> @@ -45,6 +45,7 @@
> #define CZ_PLAT_CLK 48000000
> #define DUAL_CHANNEL 2
>
> +static struct snd_soc_dai *codec_dai;
> static struct snd_soc_jack cz_jack;
> static struct clk *da7219_dai_clk;
> extern int bt_uart_enable;
> @@ -53,8 +54,10 @@ static int cz_da7219_init(struct snd_soc_pcm_runtime *rtd)
> {
> int ret;
> struct snd_soc_card *card = rtd->card;
> - struct snd_soc_dai *codec_dai = rtd->codec_dai;
> - struct snd_soc_component *component = codec_dai->component;
> + struct snd_soc_component *component;
> +
> + codec_dai = rtd->codec_dai;
> + component = codec_dai->component;
>
> dev_info(rtd->dev, "codec dai name = %s\n", codec_dai->name);
>
> @@ -255,6 +258,16 @@ static void cz_dmic_shutdown(struct
> snd_pcm_substream *substream)
> da7219_clk_disable();
> }
>
> +static static int cz_da7219_hw_params(struct snd_pcm_substream *substream,
> + struct snd_pcm_hw_params *params) {
> + /* da7219 Codec is clock master so setup as per the needs */
> + if (codec_dai->driver->ops->hw_params)
> + return codec_dai->driver->ops->hw_params(substream, params,
> + codec_dai);
> + return 0;
> +}
> +
> static const struct snd_soc_ops cz_da7219_play_ops = {
> .startup = cz_da7219_play_startup,
> .shutdown = cz_da7219_shutdown,
> @@ -268,16 +281,19 @@ static const struct snd_soc_ops cz_da7219_cap_ops = {
> static const struct snd_soc_ops cz_max_play_ops = {
> .startup = cz_max_startup,
> .shutdown = cz_max_shutdown,
> + .hw_params = cz_da7219_hw_params,
> };
>
> static const struct snd_soc_ops cz_dmic0_cap_ops = {
> .startup = cz_dmic0_startup,
> .shutdown = cz_dmic_shutdown,
> + .hw_params = cz_da7219_hw_params,
> };
>
> static const struct snd_soc_ops cz_dmic1_cap_ops = {
> .startup = cz_dmic1_startup,
> .shutdown = cz_dmic_shutdown,
> + .hw_params = cz_da7219_hw_params,
> };
>
> static struct snd_soc_dai_link cz_dai_7219_98357[] = {
> --
> 2.17.1
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] ASoC: AMD: Configure master codec on all playback/capture cases
2019-02-15 7:06 [PATCH] ASoC: AMD: Configure master codec on all playback/capture cases Agrawal, Akshu
2019-02-15 9:58 ` Adam Thomson
@ 2019-02-18 18:18 ` Mark Brown
2019-02-20 9:30 ` [alsa-devel] " Adam Thomson
2019-03-04 17:49 ` Guenter Roeck
2 siblings, 1 reply; 5+ messages in thread
From: Mark Brown @ 2019-02-18 18:18 UTC (permalink / raw)
To: Agrawal, Akshu
Cc: djkurtz, Deucher, Alexander, Adam.Thomson.Opensource,
Liam Girdwood, Jaroslav Kysela, Takashi Iwai, Mukunda, Vijendar,
Wei Yongjun, Kuninori Morimoto,
moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...,
open list
[-- Attachment #1: Type: text/plain, Size: 1152 bytes --]
On Fri, Feb 15, 2019 at 07:06:15AM +0000, Agrawal, Akshu wrote:
> +static static int cz_da7219_hw_params(struct snd_pcm_substream *substream,
> + struct snd_pcm_hw_params *params)
> +{
> + /* da7219 Codec is clock master so setup as per the needs */
> + if (codec_dai->driver->ops->hw_params)
> + return codec_dai->driver->ops->hw_params(substream, params,
> + codec_dai);
> + return 0;
> +}
No, this is a bad idea - you shouldn't be callings ops in drivers
outside of the framework, something is going to break on you there.
Either the drivers or the frameworks are going to get surprised by
things getting called out of sequence, for example I'd not rely on the
fact that the CODEC would be outputing a clock without active audio.
This probably means that you're going to have to extend the framework in
some way, or refactor things so that the BCLK is exported as a clock by
the CODEC and reflect the relationships via the clock API. I need to
think a bit more but I think the clock API is going to be the the best
solution.
I also suspect that some work is needed to propagate constraints between
the various DAIs in the system.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread* RE: [alsa-devel] [PATCH] ASoC: AMD: Configure master codec on all playback/capture cases
2019-02-18 18:18 ` Mark Brown
@ 2019-02-20 9:30 ` Adam Thomson
0 siblings, 0 replies; 5+ messages in thread
From: Adam Thomson @ 2019-02-20 9:30 UTC (permalink / raw)
To: Mark Brown, Agrawal, Akshu
Cc: moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...,
Kuninori Morimoto, open list, Takashi Iwai, djkurtz,
Liam Girdwood, Wei Yongjun, Mukunda, Vijendar, Deucher,
Alexander, Adam Thomson
On 8 February 2019 18:18, Mark Brown wrote:
> On Fri, Feb 15, 2019 at 07:06:15AM +0000, Agrawal, Akshu wrote:
>
> > +static static int cz_da7219_hw_params(struct snd_pcm_substream
> *substream,
> > + struct snd_pcm_hw_params *params) {
> > + /* da7219 Codec is clock master so setup as per the needs */
> > + if (codec_dai->driver->ops->hw_params)
> > + return codec_dai->driver->ops->hw_params(substream, params,
> > + codec_dai);
> > + return 0;
> > +}
>
> No, this is a bad idea - you shouldn't be callings ops in drivers outside of the
> framework, something is going to break on you there.
> Either the drivers or the frameworks are going to get surprised by things getting
> called out of sequence, for example I'd not rely on the fact that the CODEC would
> be outputing a clock without active audio.
> This probably means that you're going to have to extend the framework in some
> way, or refactor things so that the BCLK is exported as a clock by the CODEC and
> reflect the relationships via the clock API. I need to think a bit more but I think
> the clock API is going to be the the best solution.
>
> I also suspect that some work is needed to propagate constraints between the
> various DAIs in the system.
So this was a suggestion from my side to avoid the scenario where the system
starts, the DA7219 codec hasn't yet been used (hw_params() not been called), and
one of the other audio devices is needed for playback/capture. The codec will be
at a default DAI configuration rather than the configuration that's required. If
the DA7219 codec had been used first then of course there's no problem and the
system runs as expected from then on.
This AMD platform limits all DAIs to 48KHz, 2 channel so the configuration is
fixed, as I understand it. DA7219 will output DAI clocks without data so there's
no problem there. I initially suggested using 'soc_dai_hw_params()' but the
function isn't a global symbol, probably intentionally, so for this more unusual
config I felt the above approach might be an option. Absolutely understand your
concerns though.
I considered the CCF approach but I'm not 100% confident that the clocking
mechanism of the codec will fit ideally into that framework. There's already
a clock gate for the DAI clocks, in the driver, but nothing more. The problem
here is that the BCLK rate is intrinsically linked to the WCLK rate, as well as
the word length, so changing the desired BCLK rate would affect the WCLK rate,
and vice versa. I will take a look again, but if you have any suggestions then
they'd be greatly received.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [alsa-devel] [PATCH] ASoC: AMD: Configure master codec on all playback/capture cases
2019-02-15 7:06 [PATCH] ASoC: AMD: Configure master codec on all playback/capture cases Agrawal, Akshu
2019-02-15 9:58 ` Adam Thomson
2019-02-18 18:18 ` Mark Brown
@ 2019-03-04 17:49 ` Guenter Roeck
2 siblings, 0 replies; 5+ messages in thread
From: Guenter Roeck @ 2019-03-04 17:49 UTC (permalink / raw)
To: Agrawal, Akshu
Cc: moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...,
Kuninori Morimoto, open list, Takashi Iwai, Liam Girdwood,
djkurtz, Mark Brown, Wei Yongjun, Mukunda, Vijendar, Deucher,
Alexander, Adam.Thomson.Opensource
On Fri, Feb 15, 2019 at 07:06:15AM +0000, Agrawal, Akshu wrote:
> In the system design da7219 is the master codec and clocks are
> generated by it.
> Bclk is to be generated at the required rate for other codecs used when
> da7219 is acting only as clock master. For this call hw_params of da7219
> during playback/capture on non da7219 codecs.
>
> Being able to set bclk at lower rate also fixes noise issue observed on
> some dmics.
>
> Signed-off-by: Akshu Agrawal <akshu.agrawal@amd.com>
> Reviewed-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
> ---
> sound/soc/amd/acp-da7219-max98357a.c | 20 ++++++++++++++++++--
> 1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/sound/soc/amd/acp-da7219-max98357a.c b/sound/soc/amd/acp-da7219-max98357a.c
> index a5daad973ce5..ad327415290a 100644
> --- a/sound/soc/amd/acp-da7219-max98357a.c
> +++ b/sound/soc/amd/acp-da7219-max98357a.c
> @@ -45,6 +45,7 @@
> #define CZ_PLAT_CLK 48000000
> #define DUAL_CHANNEL 2
>
> +static struct snd_soc_dai *codec_dai;
> static struct snd_soc_jack cz_jack;
> static struct clk *da7219_dai_clk;
> extern int bt_uart_enable;
> @@ -53,8 +54,10 @@ static int cz_da7219_init(struct snd_soc_pcm_runtime *rtd)
> {
> int ret;
> struct snd_soc_card *card = rtd->card;
> - struct snd_soc_dai *codec_dai = rtd->codec_dai;
> - struct snd_soc_component *component = codec_dai->component;
> + struct snd_soc_component *component;
> +
> + codec_dai = rtd->codec_dai;
> + component = codec_dai->component;
>
> dev_info(rtd->dev, "codec dai name = %s\n", codec_dai->name);
>
> @@ -255,6 +258,16 @@ static void cz_dmic_shutdown(struct snd_pcm_substream *substream)
> da7219_clk_disable();
> }
>
> +static static int cz_da7219_hw_params(struct snd_pcm_substream *substream,
s/static static/static/
gcc doesn't like the double static (at least neither gcc 6.5.0 nor 8.1.0
do), and it doesn't seem to be syntactically correct (even though,
oddly enough, clang accepts it w/o error message).
Guenter
> + struct snd_pcm_hw_params *params)
> +{
> + /* da7219 Codec is clock master so setup as per the needs */
> + if (codec_dai->driver->ops->hw_params)
> + return codec_dai->driver->ops->hw_params(substream, params,
> + codec_dai);
> + return 0;
> +}
> +
> static const struct snd_soc_ops cz_da7219_play_ops = {
> .startup = cz_da7219_play_startup,
> .shutdown = cz_da7219_shutdown,
> @@ -268,16 +281,19 @@ static const struct snd_soc_ops cz_da7219_cap_ops = {
> static const struct snd_soc_ops cz_max_play_ops = {
> .startup = cz_max_startup,
> .shutdown = cz_max_shutdown,
> + .hw_params = cz_da7219_hw_params,
> };
>
> static const struct snd_soc_ops cz_dmic0_cap_ops = {
> .startup = cz_dmic0_startup,
> .shutdown = cz_dmic_shutdown,
> + .hw_params = cz_da7219_hw_params,
> };
>
> static const struct snd_soc_ops cz_dmic1_cap_ops = {
> .startup = cz_dmic1_startup,
> .shutdown = cz_dmic_shutdown,
> + .hw_params = cz_da7219_hw_params,
> };
>
> static struct snd_soc_dai_link cz_dai_7219_98357[] = {
^ permalink raw reply [flat|nested] 5+ messages in thread