BDW boards using this machine driver supports only stereo capture and
playback. Implement a constraint to enforce it.
Signed-off-by: Brent Lu <[email protected]>
---
sound/soc/intel/boards/bdw-rt5677.c | 33 +++++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)
diff --git a/sound/soc/intel/boards/bdw-rt5677.c b/sound/soc/intel/boards/bdw-rt5677.c
index 4a4d335..a312b55 100644
--- a/sound/soc/intel/boards/bdw-rt5677.c
+++ b/sound/soc/intel/boards/bdw-rt5677.c
@@ -22,6 +22,8 @@
#include "../../codecs/rt5677.h"
+#define DUAL_CHANNEL 2
+
struct bdw_rt5677_priv {
struct gpio_desc *gpio_hp_en;
struct snd_soc_component *component;
@@ -245,6 +247,36 @@ static int bdw_rt5677_init(struct snd_soc_pcm_runtime *rtd)
return 0;
}
+static const unsigned int channels[] = {
+ DUAL_CHANNEL,
+};
+
+static const struct snd_pcm_hw_constraint_list constraints_channels = {
+ .count = ARRAY_SIZE(channels),
+ .list = channels,
+ .mask = 0,
+};
+
+static int bdw_fe_startup(struct snd_pcm_substream *substream)
+{
+ struct snd_pcm_runtime *runtime = substream->runtime;
+
+ /*
+ * On this platform for PCM device we support,
+ * stereo
+ */
+
+ runtime->hw.channels_max = DUAL_CHANNEL;
+ snd_pcm_hw_constraint_list(runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS,
+ &constraints_channels);
+
+ return 0;
+}
+
+static const struct snd_soc_ops bdw_rt5677_fe_ops = {
+ .startup = bdw_fe_startup,
+};
+
/* broadwell digital audio interface glue - connects codec <--> CPU */
SND_SOC_DAILINK_DEF(dummy,
DAILINK_COMP_ARRAY(COMP_DUMMY()));
@@ -273,6 +305,7 @@ static struct snd_soc_dai_link bdw_rt5677_dais[] = {
},
.dpcm_capture = 1,
.dpcm_playback = 1,
+ .ops = &bdw_rt5677_fe_ops,
SND_SOC_DAILINK_REG(fe, dummy, platform),
},
--
2.7.4
On 9/5/19 8:24 PM, Brent Lu wrote:
> BDW boards using this machine driver supports only stereo capture and
> playback. Implement a constraint to enforce it.
Humm, can you clarify what problem/error this patch fixes?
There are already constraints on the hsw_dais[] where the channels are
stereo only.
Thanks
-Pierre
>
> Signed-off-by: Brent Lu <[email protected]>
> ---
> sound/soc/intel/boards/bdw-rt5677.c | 33 +++++++++++++++++++++++++++++++++
> 1 file changed, 33 insertions(+)
>
> diff --git a/sound/soc/intel/boards/bdw-rt5677.c b/sound/soc/intel/boards/bdw-rt5677.c
> index 4a4d335..a312b55 100644
> --- a/sound/soc/intel/boards/bdw-rt5677.c
> +++ b/sound/soc/intel/boards/bdw-rt5677.c
> @@ -22,6 +22,8 @@
>
> #include "../../codecs/rt5677.h"
>
> +#define DUAL_CHANNEL 2
> +
> struct bdw_rt5677_priv {
> struct gpio_desc *gpio_hp_en;
> struct snd_soc_component *component;
> @@ -245,6 +247,36 @@ static int bdw_rt5677_init(struct snd_soc_pcm_runtime *rtd)
> return 0;
> }
>
> +static const unsigned int channels[] = {
> + DUAL_CHANNEL,
> +};
> +
> +static const struct snd_pcm_hw_constraint_list constraints_channels = {
> + .count = ARRAY_SIZE(channels),
> + .list = channels,
> + .mask = 0,
> +};
> +
> +static int bdw_fe_startup(struct snd_pcm_substream *substream)
> +{
> + struct snd_pcm_runtime *runtime = substream->runtime;
> +
> + /*
> + * On this platform for PCM device we support,
> + * stereo
> + */
> +
> + runtime->hw.channels_max = DUAL_CHANNEL;
> + snd_pcm_hw_constraint_list(runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS,
> + &constraints_channels);
> +
> + return 0;
> +}
> +
> +static const struct snd_soc_ops bdw_rt5677_fe_ops = {
> + .startup = bdw_fe_startup,
> +};
> +
> /* broadwell digital audio interface glue - connects codec <--> CPU */
> SND_SOC_DAILINK_DEF(dummy,
> DAILINK_COMP_ARRAY(COMP_DUMMY()));
> @@ -273,6 +305,7 @@ static struct snd_soc_dai_link bdw_rt5677_dais[] = {
> },
> .dpcm_capture = 1,
> .dpcm_playback = 1,
> + .ops = &bdw_rt5677_fe_ops,
> SND_SOC_DAILINK_REG(fe, dummy, platform),
> },
>
>
Hi Pierre
We are working on a backport 3.14 branch for Chrome projects based on BDW platform. In the branch 4-channel capture is supported on some platforms but not all. So we need to add a constraint in the machine driver for machines don't support this feature.
I checked the for-next branch in the broonie repo. The channels_max of HSW_PCM_DAI_ID_SYSTEM is 4 for capture stream so I think it would have same issue like google's backport tree. I didn't find any constraint for this dai. Would you point out where it is?
.capture = {
.stream_name = "Analog Capture",
.channels_min = 2,
.channels_max = 4,
.rates = SNDRV_PCM_RATE_48000,
.formats = SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S16_LE,
},
Regards,
Brent Lu
-----Original Message-----
From: Pierre-Louis Bossart [mailto:[email protected]]
Sent: Friday, September 6, 2019 10:21 PM
To: Lu, Brent <[email protected]>; [email protected]
Cc: Rojewski, Cezary <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [alsa-devel] [PATCH] ASoC: bdw-rt5677: channel constraint support
On 9/5/19 8:24 PM, Brent Lu wrote:
> BDW boards using this machine driver supports only stereo capture and
> playback. Implement a constraint to enforce it.
Humm, can you clarify what problem/error this patch fixes?
There are already constraints on the hsw_dais[] where the channels are stereo only.
Thanks
-Pierre
>
> Signed-off-by: Brent Lu <[email protected]>
> ---
> sound/soc/intel/boards/bdw-rt5677.c | 33 +++++++++++++++++++++++++++++++++
> 1 file changed, 33 insertions(+)
>
> diff --git a/sound/soc/intel/boards/bdw-rt5677.c
> b/sound/soc/intel/boards/bdw-rt5677.c
> index 4a4d335..a312b55 100644
> --- a/sound/soc/intel/boards/bdw-rt5677.c
> +++ b/sound/soc/intel/boards/bdw-rt5677.c
> @@ -22,6 +22,8 @@
>
> #include "../../codecs/rt5677.h"
>
> +#define DUAL_CHANNEL 2
> +
> struct bdw_rt5677_priv {
> struct gpio_desc *gpio_hp_en;
> struct snd_soc_component *component; @@ -245,6 +247,36 @@ static
> int bdw_rt5677_init(struct snd_soc_pcm_runtime *rtd)
> return 0;
> }
>
> +static const unsigned int channels[] = {
> + DUAL_CHANNEL,
> +};
> +
> +static const struct snd_pcm_hw_constraint_list constraints_channels = {
> + .count = ARRAY_SIZE(channels),
> + .list = channels,
> + .mask = 0,
> +};
> +
> +static int bdw_fe_startup(struct snd_pcm_substream *substream) {
> + struct snd_pcm_runtime *runtime = substream->runtime;
> +
> + /*
> + * On this platform for PCM device we support,
> + * stereo
> + */
> +
> + runtime->hw.channels_max = DUAL_CHANNEL;
> + snd_pcm_hw_constraint_list(runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS,
> + &constraints_channels);
> +
> + return 0;
> +}
> +
> +static const struct snd_soc_ops bdw_rt5677_fe_ops = {
> + .startup = bdw_fe_startup,
> +};
> +
> /* broadwell digital audio interface glue - connects codec <--> CPU */
> SND_SOC_DAILINK_DEF(dummy,
> DAILINK_COMP_ARRAY(COMP_DUMMY()));
> @@ -273,6 +305,7 @@ static struct snd_soc_dai_link bdw_rt5677_dais[] = {
> },
> .dpcm_capture = 1,
> .dpcm_playback = 1,
> + .ops = &bdw_rt5677_fe_ops,
> SND_SOC_DAILINK_REG(fe, dummy, platform),
> },
>
>
Please don't top-post on public mailing lists.
> We are working on a backport 3.14 branch for Chrome projects based on BDW platform. In the branch 4-channel capture is supported on some platforms but not all. So we need to add a constraint in the machine driver for machines don't support this feature.
>
> I checked the for-next branch in the broonie repo. The channels_max of HSW_PCM_DAI_ID_SYSTEM is 4 for capture stream so I think it would have same issue like google's backport tree. I didn't find any constraint for this dai. Would you point out where it is?
>
> .capture = {
> .stream_name = "Analog Capture",
> .channels_min = 2,
> .channels_max = 4,
> .rates = SNDRV_PCM_RATE_48000,
> .formats = SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S16_LE,
> },
ok, I missed this capture case indeed, but when I look at the Chrome
3.14 code I don't see this constraint being added, and we already have
an ssp0_fixup function where 2 channels only are used.
/* The ADSP will covert the FE rate to 48k, stereo */
rate->min = rate->max = 48000;
channels->min = channels->max = 2;
I also don't see any case where we support 4 channels in any broadwell
machine driver?
So again can you point me to an issue or existing backport that requires
the patch below. Not trying to be obtuse but we should only change older
platforms when there is evidence that a change is needed.
-Pierre
>
> Regards,
> Brent Lu
>
> -----Original Message-----
> From: Pierre-Louis Bossart [mailto:[email protected]]
> Sent: Friday, September 6, 2019 10:21 PM
> To: Lu, Brent <[email protected]>; [email protected]
> Cc: Rojewski, Cezary <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
> Subject: Re: [alsa-devel] [PATCH] ASoC: bdw-rt5677: channel constraint support
>
> On 9/5/19 8:24 PM, Brent Lu wrote:
>> BDW boards using this machine driver supports only stereo capture and
>> playback. Implement a constraint to enforce it.
>
> Humm, can you clarify what problem/error this patch fixes?
>
> There are already constraints on the hsw_dais[] where the channels are stereo only.
>
> Thanks
> -Pierre
>
>>
>> Signed-off-by: Brent Lu <[email protected]>
>> ---
>> sound/soc/intel/boards/bdw-rt5677.c | 33 +++++++++++++++++++++++++++++++++
>> 1 file changed, 33 insertions(+)
>>
>> diff --git a/sound/soc/intel/boards/bdw-rt5677.c
>> b/sound/soc/intel/boards/bdw-rt5677.c
>> index 4a4d335..a312b55 100644
>> --- a/sound/soc/intel/boards/bdw-rt5677.c
>> +++ b/sound/soc/intel/boards/bdw-rt5677.c
>> @@ -22,6 +22,8 @@
>>
>> #include "../../codecs/rt5677.h"
>>
>> +#define DUAL_CHANNEL 2
>> +
>> struct bdw_rt5677_priv {
>> struct gpio_desc *gpio_hp_en;
>> struct snd_soc_component *component; @@ -245,6 +247,36 @@ static
>> int bdw_rt5677_init(struct snd_soc_pcm_runtime *rtd)
>> return 0;
>> }
>>
>> +static const unsigned int channels[] = {
>> + DUAL_CHANNEL,
>> +};
>> +
>> +static const struct snd_pcm_hw_constraint_list constraints_channels = {
>> + .count = ARRAY_SIZE(channels),
>> + .list = channels,
>> + .mask = 0,
>> +};
>> +
>> +static int bdw_fe_startup(struct snd_pcm_substream *substream) {
>> + struct snd_pcm_runtime *runtime = substream->runtime;
>> +
>> + /*
>> + * On this platform for PCM device we support,
>> + * stereo
>> + */
>> +
>> + runtime->hw.channels_max = DUAL_CHANNEL;
>> + snd_pcm_hw_constraint_list(runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS,
>> + &constraints_channels);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct snd_soc_ops bdw_rt5677_fe_ops = {
>> + .startup = bdw_fe_startup,
>> +};
>> +
>> /* broadwell digital audio interface glue - connects codec <--> CPU */
>> SND_SOC_DAILINK_DEF(dummy,
>> DAILINK_COMP_ARRAY(COMP_DUMMY()));
>> @@ -273,6 +305,7 @@ static struct snd_soc_dai_link bdw_rt5677_dais[] = {
>> },
>> .dpcm_capture = 1,
>> .dpcm_playback = 1,
>> + .ops = &bdw_rt5677_fe_ops,
>> SND_SOC_DAILINK_REG(fe, dummy, platform),
>> },
>>
>>
>
> _______________________________________________
> Alsa-devel mailing list
> [email protected]
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>
> -----Original Message-----
> From: Pierre-Louis Bossart [mailto:[email protected]]
> Sent: Tuesday, September 10, 2019 1:53 AM
> To: Lu, Brent <[email protected]>; [email protected]
> Cc: Rojewski, Cezary <[email protected]>;
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [alsa-devel] [PATCH] ASoC: bdw-rt5677: channel constraint
> support
>
> Please don't top-post on public mailing lists.
>
> > We are working on a backport 3.14 branch for Chrome projects based on
> BDW platform. In the branch 4-channel capture is supported on some
> platforms but not all. So we need to add a constraint in the machine driver
> for machines don't support this feature.
> >
> > I checked the for-next branch in the broonie repo. The channels_max of
> HSW_PCM_DAI_ID_SYSTEM is 4 for capture stream so I think it would have
> same issue like google's backport tree. I didn't find any constraint for this dai.
> Would you point out where it is?
> >
> > .capture = {
> > .stream_name = "Analog Capture",
> > .channels_min = 2,
> > .channels_max = 4,
> > .rates = SNDRV_PCM_RATE_48000,
> > .formats = SNDRV_PCM_FMTBIT_S24_LE |
> SNDRV_PCM_FMTBIT_S16_LE,
> > },
>
> ok, I missed this capture case indeed, but when I look at the Chrome
> 3.14 code I don't see this constraint being added, and we already have an
> ssp0_fixup function where 2 channels only are used.
>
> /* The ADSP will covert the FE rate to 48k, stereo */
> rate->min = rate->max = 48000;
> channels->min = channels->max = 2;
Yes I noticed it, but the channel max number returned by snd_pcm_hw_params_any()
is still 4 for the port "Capture PCM" so I add a constraint to the FE dai.
>
> I also don't see any case where we support 4 channels in any broadwell
> machine driver?
It's the bdw-rt5650.c which only exists in chrome's 3.14 branch supporting Buddy
project. They submitted the machine driver but not yet merged.
https://patchwork.kernel.org/patch/11050985/
>
> So again can you point me to an issue or existing backport that requires the
> patch below. Not trying to be obtuse but we should only change older
> platforms when there is evidence that a change is needed.
The story is Chrome has a tool called alsa_conformance_test which runs capture or
playback against a PCM port with all possible configurations (channel, format, rate)
then measure if the sample rate is correct. Since the channel max number reported
is 4, it tests the 4-channel 48K capture and reports the actual sample rate is 24000
instead of 48000. That's the reason we want to add a constraint in machine driver to
avoid user space programs trying to do 4 channel recording since this machine does
not support it in the beginning.
>
> -Pierre
>
> >
> > Regards,
> > Brent Lu
> >
> > -----Original Message-----
> > From: Pierre-Louis Bossart
> > [mailto:[email protected]]
> > Sent: Friday, September 6, 2019 10:21 PM
> > To: Lu, Brent <[email protected]>; [email protected]
> > Cc: Rojewski, Cezary <[email protected]>;
> > [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]
> > Subject: Re: [alsa-devel] [PATCH] ASoC: bdw-rt5677: channel constraint
> > support
> >
> > On 9/5/19 8:24 PM, Brent Lu wrote:
> >> BDW boards using this machine driver supports only stereo capture and
> >> playback. Implement a constraint to enforce it.
> >
> > Humm, can you clarify what problem/error this patch fixes?
> >
> > There are already constraints on the hsw_dais[] where the channels are
> stereo only.
> >
> > Thanks
> > -Pierre
> >
> >>
> >> Signed-off-by: Brent Lu <[email protected]>
> >> ---
> >> sound/soc/intel/boards/bdw-rt5677.c | 33
> +++++++++++++++++++++++++++++++++
> >> 1 file changed, 33 insertions(+)
> >>
> >> diff --git a/sound/soc/intel/boards/bdw-rt5677.c
> >> b/sound/soc/intel/boards/bdw-rt5677.c
> >> index 4a4d335..a312b55 100644
> >> --- a/sound/soc/intel/boards/bdw-rt5677.c
> >> +++ b/sound/soc/intel/boards/bdw-rt5677.c
> >> @@ -22,6 +22,8 @@
> >>
> >> #include "../../codecs/rt5677.h"
> >>
> >> +#define DUAL_CHANNEL 2
> >> +
> >> struct bdw_rt5677_priv {
> >> struct gpio_desc *gpio_hp_en;
> >> struct snd_soc_component *component; @@ -245,6 +247,36 @@
> static
> >> int bdw_rt5677_init(struct snd_soc_pcm_runtime *rtd)
> >> return 0;
> >> }
> >>
> >> +static const unsigned int channels[] = {
> >> + DUAL_CHANNEL,
> >> +};
> >> +
> >> +static const struct snd_pcm_hw_constraint_list constraints_channels = {
> >> + .count = ARRAY_SIZE(channels),
> >> + .list = channels,
> >> + .mask = 0,
> >> +};
> >> +
> >> +static int bdw_fe_startup(struct snd_pcm_substream *substream) {
> >> + struct snd_pcm_runtime *runtime = substream->runtime;
> >> +
> >> + /*
> >> + * On this platform for PCM device we support,
> >> + * stereo
> >> + */
> >> +
> >> + runtime->hw.channels_max = DUAL_CHANNEL;
> >> + snd_pcm_hw_constraint_list(runtime, 0,
> SNDRV_PCM_HW_PARAM_CHANNELS,
> >> + &constraints_channels);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static const struct snd_soc_ops bdw_rt5677_fe_ops = {
> >> + .startup = bdw_fe_startup,
> >> +};
> >> +
> >> /* broadwell digital audio interface glue - connects codec <--> CPU */
> >> SND_SOC_DAILINK_DEF(dummy,
> >> DAILINK_COMP_ARRAY(COMP_DUMMY()));
> >> @@ -273,6 +305,7 @@ static struct snd_soc_dai_link bdw_rt5677_dais[] =
> {
> >> },
> >> .dpcm_capture = 1,
> >> .dpcm_playback = 1,
> >> + .ops = &bdw_rt5677_fe_ops,
> >> SND_SOC_DAILINK_REG(fe, dummy, platform),
> >> },
> >>
> >>
> >
> > _______________________________________________
> > Alsa-devel mailing list
> > [email protected]
> > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> >