[PATCH 0/3] add channel constraint for BDW machine drivers

[PATCH 0/3] add channel constraint for BDW machine drivers

[Brent Lu]

The machine driver bdw-rt5650 (for Google buddy) supports 2 or 4-channel
recording while other two drivers support only 2-channel recording. HW
constraints are implemented to reflect the hardware limitation on BDW
platform.

Brent Lu (3):
  ASoC: bdw-rt5677: channel constraint support
  ASoC: bdw-rt5650: channel constraint support
  ASoC: broadwell: channel constraint support

 sound/soc/intel/boards/bdw-rt5650.c | 34 ++++++++++++++++++++++++++++++++++
 sound/soc/intel/boards/bdw-rt5677.c | 33 +++++++++++++++++++++++++++++++++
 sound/soc/intel/boards/broadwell.c  | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 100 insertions(+)

-- 
2.7.4

[PATCH 1/3] ASoC: bdw-rt5677: channel constraint support

[Brent Lu]

BDW boards using this machine driver supports only stereo capture and
playback. Implement a constraint to enforce it.

Signed-off-by: Brent Lu <brent.lu@intel.com>
---
 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 cc41a34..7963cae 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;
@@ -222,6 +224,36 @@ static int bdw_rt5677_rtd_init(struct snd_soc_pcm_runtime *rtd)
 }
 #endif
 
+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 audio
+	 */
+
+	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,
+};
+
 static int bdw_rt5677_init(struct snd_soc_pcm_runtime *rtd)
 {
 	struct bdw_rt5677_priv *bdw_rt5677 =
@@ -321,6 +353,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

[PATCH 2/3] ASoC: bdw-rt5650: channel constraint support

[Brent Lu]

BDW boards using this machine driver supports only 2 or 4-channel capture.
Implement a constraint to enforce it.

Signed-off-by: Brent Lu <brent.lu@intel.com>
---
 sound/soc/intel/boards/bdw-rt5650.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/sound/soc/intel/boards/bdw-rt5650.c b/sound/soc/intel/boards/bdw-rt5650.c
index af2f502..dd4f219 100644
--- a/sound/soc/intel/boards/bdw-rt5650.c
+++ b/sound/soc/intel/boards/bdw-rt5650.c
@@ -21,6 +21,9 @@
 
 #include "../../codecs/rt5645.h"
 
+#define DUAL_CHANNEL 2
+#define QUAD_CHANNEL 4
+
 struct bdw_rt5650_priv {
 	struct gpio_desc *gpio_hp_en;
 	struct snd_soc_component *component;
@@ -162,6 +165,36 @@ static int bdw_rt5650_rtd_init(struct snd_soc_pcm_runtime *rtd)
 }
 #endif
 
+static const unsigned int channels[] = {
+	DUAL_CHANNEL, QUAD_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,
+	 * 2 or 4 channel capture
+	 */
+	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
+		snd_pcm_hw_constraint_list(runtime, 0,
+					   SNDRV_PCM_HW_PARAM_CHANNELS,
+					   &constraints_channels);
+
+	return 0;
+}
+
+static const struct snd_soc_ops bdw_rt5650_fe_ops = {
+	.startup = bdw_fe_startup,
+};
+
 static int bdw_rt5650_init(struct snd_soc_pcm_runtime *rtd)
 {
 	struct bdw_rt5650_priv *bdw_rt5650 =
@@ -234,6 +267,7 @@ static struct snd_soc_dai_link bdw_rt5650_dais[] = {
 		.name = "System PCM",
 		.stream_name = "System Playback",
 		.dynamic = 1,
+		.ops = &bdw_rt5650_fe_ops,
 #if !IS_ENABLED(CONFIG_SND_SOC_SOF_BROADWELL)
 		.init = bdw_rt5650_rtd_init,
 #endif
-- 
2.7.4

[PATCH 3/3] ASoC: broadwell: channel constraint support

[Brent Lu]

BDW boards using this machine driver supports only stereo capture and
playback. Implement a constraint to enforce it.

Signed-off-by: Brent Lu <brent.lu@intel.com>
---
 sound/soc/intel/boards/broadwell.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/sound/soc/intel/boards/broadwell.c b/sound/soc/intel/boards/broadwell.c
index f9a8336..09347f2 100644
--- a/sound/soc/intel/boards/broadwell.c
+++ b/sound/soc/intel/boards/broadwell.c
@@ -19,6 +19,8 @@
 
 #include "../../codecs/rt286.h"
 
+#define DUAL_CHANNEL 2
+
 static struct snd_soc_jack broadwell_headset;
 /* Headset jack detection DAPM pins */
 static struct snd_soc_jack_pin broadwell_headset_pins[] = {
@@ -143,6 +145,36 @@ static int broadwell_rtd_init(struct snd_soc_pcm_runtime *rtd)
 }
 #endif
 
+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 broadwell_fe_startup(struct snd_pcm_substream *substream)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+
+	/*
+	 * On this platform for PCM device we support,
+	 * stereo audio
+	 */
+
+	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 broadwell_fe_ops = {
+	.startup = broadwell_fe_startup,
+};
+
 SND_SOC_DAILINK_DEF(system,
 	DAILINK_COMP_ARRAY(COMP_CPU("System Pin")));
 
@@ -180,6 +212,7 @@ static struct snd_soc_dai_link broadwell_rt286_dais[] = {
 		.init = broadwell_rtd_init,
 #endif
 		.trigger = {SND_SOC_DPCM_TRIGGER_POST, SND_SOC_DPCM_TRIGGER_POST},
+		.ops = &broadwell_fe_ops,
 		.dpcm_playback = 1,
 		.dpcm_capture = 1,
 		SND_SOC_DAILINK_REG(system, dummy, platform),
-- 
2.7.4

Re: [PATCH 1/3] ASoC: bdw-rt5677: channel constraint support

[Cezary Rojewski]

On 2020-04-27 10:37, Brent Lu wrote:
> BDW boards using this machine driver supports only stereo capture and
> playback. Implement a constraint to enforce it.
> 

Title for the overall series fits better than the one chosen for actual 
patches. "channel constraint support" is misleading. Constraints are 
added or removed but certainly not supported.

> Signed-off-by: Brent Lu <brent.lu@intel.com>
> ---
>   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 cc41a34..7963cae 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
> +

Remove, we need not additional too-obvious macro. One could argue 
'STEREO' is a better choice too.

>   struct bdw_rt5677_priv {
>   	struct gpio_desc *gpio_hp_en;
>   	struct snd_soc_component *component;
> @@ -222,6 +224,36 @@ static int bdw_rt5677_rtd_init(struct snd_soc_pcm_runtime *rtd)
>   }
>   #endif
>   
> +static const unsigned int channels[] = {
> +	DUAL_CHANNEL,

Inline as stated above.

> +};
> +
> +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)

Entire file uses 'bdw_rt5677_' naming convention. Let's not stray away 
from that path now.

> +{
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +
> +	/*
> +	 * On this platform for PCM device we support,
> +	 * stereo audio
> +	 */

Sometimes you add a newline add and before, while other times just one, 
before the comment. Please streamline the format across all patches in 
the series. Comment can be more strict too
	/* Board supports stereo configuration only */
> +
> +	runtime->hw.channels_max = DUAL_CHANNEL;

Inline. If you really want to avoid using 2, make use of 0-entry of 
constrains_channels array.

> +	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,
> +};
> +
>   static int bdw_rt5677_init(struct snd_soc_pcm_runtime *rtd)
>   {
>   	struct bdw_rt5677_priv *bdw_rt5677 =
> @@ -321,6 +353,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),
>   	},
>   
> 

Re: [PATCH 2/3] ASoC: bdw-rt5650: channel constraint support

[Cezary Rojewski]

On 2020-04-27 10:37, Brent Lu wrote:
> BDW boards using this machine driver supports only 2 or 4-channel capture.
> Implement a constraint to enforce it.
> 

What about playback configurations?

Title for the overall series fits better than the one chosen for actual 
patches. "channel constraint support" is misleading. Constraints are 
added or removed but certainly not supported.

> Signed-off-by: Brent Lu <brent.lu@intel.com>
> ---
>   sound/soc/intel/boards/bdw-rt5650.c | 34 ++++++++++++++++++++++++++++++++++
>   1 file changed, 34 insertions(+)
> 
> diff --git a/sound/soc/intel/boards/bdw-rt5650.c b/sound/soc/intel/boards/bdw-rt5650.c
> index af2f502..dd4f219 100644
> --- a/sound/soc/intel/boards/bdw-rt5650.c
> +++ b/sound/soc/intel/boards/bdw-rt5650.c
> @@ -21,6 +21,9 @@
>   
>   #include "../../codecs/rt5645.h"
>   
> +#define DUAL_CHANNEL 2
> +#define QUAD_CHANNEL 4
> +

Remove, we need not additional too-obvious macro. One could argue 
'STEREO' is a better choice for '2' channel configuration too.

>   struct bdw_rt5650_priv {
>   	struct gpio_desc *gpio_hp_en;
>   	struct snd_soc_component *component;
> @@ -162,6 +165,36 @@ static int bdw_rt5650_rtd_init(struct snd_soc_pcm_runtime *rtd)
>   }
>   #endif
>   
> +static const unsigned int channels[] = {
> +	DUAL_CHANNEL, QUAD_CHANNEL,

Inline as stated above.

> +};
> +
> +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)

Entire file uses 'bdw_rt5650_' naming convention. Let's not stray away 
from that path now.

> +{
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +

Missing hw.channels_max assignment from rt5677 - inconsistency/ copy error?

> +	/*
> +	 * On this platform for PCM device we support,
> +	 * 2 or 4 channel capture
> +	 */

Sometimes you add a newline add and before, while other times just one, 
before the comment. Please streamline the format across all patches in 
the series. Comment can be more strict too
	/* Board supports stereo and quad configurations */

> +	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
> +		snd_pcm_hw_constraint_list(runtime, 0,
> +					   SNDRV_PCM_HW_PARAM_CHANNELS,
> +					   &constraints_channels);

Redesign to reduce indentation and improve readability -
	if (stream != capture)
		return 0;

	return snd_pcm_hw_contraint_list(...);

> +
> +	return 0;
> +}
> +
> +static const struct snd_soc_ops bdw_rt5650_fe_ops = {
> +	.startup = bdw_fe_startup,
> +};
> +
>   static int bdw_rt5650_init(struct snd_soc_pcm_runtime *rtd)
>   {
>   	struct bdw_rt5650_priv *bdw_rt5650 =
> @@ -234,6 +267,7 @@ static struct snd_soc_dai_link bdw_rt5650_dais[] = {
>   		.name = "System PCM",
>   		.stream_name = "System Playback",
>   		.dynamic = 1,
> +		.ops = &bdw_rt5650_fe_ops,
>   #if !IS_ENABLED(CONFIG_SND_SOC_SOF_BROADWELL)
>   		.init = bdw_rt5650_rtd_init,
>   #endif
> 

Re: [PATCH 3/3] ASoC: broadwell: channel constraint support

[Cezary Rojewski]

On 2020-04-27 10:37, Brent Lu wrote:
> BDW boards using this machine driver supports only stereo capture and
> playback. Implement a constraint to enforce it.
> 

Title for the overall series fits better than the one chosen for actual 
patches. "channel constraint support" is misleading. Constraints are 
added or removed but certainly not supported.

> Signed-off-by: Brent Lu <brent.lu@intel.com>
> ---
>   sound/soc/intel/boards/broadwell.c | 33 +++++++++++++++++++++++++++++++++
>   1 file changed, 33 insertions(+)
> 
> diff --git a/sound/soc/intel/boards/broadwell.c b/sound/soc/intel/boards/broadwell.c
> index f9a8336..09347f2 100644
> --- a/sound/soc/intel/boards/broadwell.c
> +++ b/sound/soc/intel/boards/broadwell.c
> @@ -19,6 +19,8 @@
>   
>   #include "../../codecs/rt286.h"
>   
> +#define DUAL_CHANNEL 2
> +

Remove, we need not additional too-obvious macro. One could argue 
'STEREO' is a better choice too.

>   static struct snd_soc_jack broadwell_headset;
>   /* Headset jack detection DAPM pins */
>   static struct snd_soc_jack_pin broadwell_headset_pins[] = {
> @@ -143,6 +145,36 @@ static int broadwell_rtd_init(struct snd_soc_pcm_runtime *rtd)
>   }
>   #endif
>   
> +static const unsigned int channels[] = {
> +	DUAL_CHANNEL,

Inline as stated above.

> +};
> +
> +static const struct snd_pcm_hw_constraint_list constraints_channels = {
> +	.count = ARRAY_SIZE(channels),
> +	.list = channels,
> +	.mask = 0,
> +};
> +
> +static int broadwell_fe_startup(struct snd_pcm_substream *substream)
> +{
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +
> +	/*
> +	 * On this platform for PCM device we support,
> +	 * stereo audio
> +	 */
> +

Sometimes you add a newline add and before, while other times just one, 
before the comment. Please streamline the format across all patches in 
the series. Comment can be more strict too
	/* Board supports stereo configuration only */

> +	runtime->hw.channels_max = DUAL_CHANNEL;

Inline. If you really want to avoid using 2, make use of 0-entry of 
constrains_channels array.

> +	snd_pcm_hw_constraint_list(runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS,
> +					   &constraints_channels);
> +
> +	return 0;
> +}
> +
> +static const struct snd_soc_ops broadwell_fe_ops = {
> +	.startup = broadwell_fe_startup,
> +};
> +
>   SND_SOC_DAILINK_DEF(system,
>   	DAILINK_COMP_ARRAY(COMP_CPU("System Pin")));
>   
> @@ -180,6 +212,7 @@ static struct snd_soc_dai_link broadwell_rt286_dais[] = {
>   		.init = broadwell_rtd_init,
>   #endif
>   		.trigger = {SND_SOC_DPCM_TRIGGER_POST, SND_SOC_DPCM_TRIGGER_POST},
> +		.ops = &broadwell_fe_ops,
>   		.dpcm_playback = 1,
>   		.dpcm_capture = 1,
>   		SND_SOC_DAILINK_REG(system, dummy, platform),
> 

Re: [PATCH 0/3] add channel constraint for BDW machine drivers

[Cezary Rojewski]

On 2020-04-27 10:37, Brent Lu wrote:
> The machine driver bdw-rt5650 (for Google buddy) supports 2 or 4-channel
> recording while other two drivers support only 2-channel recording. HW
> constraints are implemented to reflect the hardware limitation on BDW
> platform.
> 

Message body relates to bdw-rt5650 while the series touches every of BDW 
boards.

Apart from review given for each and every patch (although most issues 
are shared so there is not as much to address) my question is:
- are these hw limitations or software (machine board) limitations?

Czarek

> Brent Lu (3):
>    ASoC: bdw-rt5677: channel constraint support
>    ASoC: bdw-rt5650: channel constraint support
>    ASoC: broadwell: channel constraint support
> 
>   sound/soc/intel/boards/bdw-rt5650.c | 34 ++++++++++++++++++++++++++++++++++
>   sound/soc/intel/boards/bdw-rt5677.c | 33 +++++++++++++++++++++++++++++++++
>   sound/soc/intel/boards/broadwell.c  | 33 +++++++++++++++++++++++++++++++++
>   3 files changed, 100 insertions(+)
> 

Re: [PATCH 0/3] add channel constraint for BDW machine drivers

[Cezary Rojewski]

On 2020-04-27 13:01, Cezary Rojewski wrote:
> On 2020-04-27 10:37, Brent Lu wrote:
>> The machine driver bdw-rt5650 (for Google buddy) supports 2 or 4-channel
>> recording while other two drivers support only 2-channel recording. HW
>> constraints are implemented to reflect the hardware limitation on BDW
>> platform.
>>
> 
> Message body relates to bdw-rt5650 while the series touches every of BDW 
> boards.

Ignore my first sentence :-) Second still applies though

> 
> Apart from review given for each and every patch (although most issues 
> are shared so there is not as much to address) my question is:
> - are these hw limitations or software (machine board) limitations?
> 
> Czarek
> 
>> Brent Lu (3):
>>    ASoC: bdw-rt5677: channel constraint support
>>    ASoC: bdw-rt5650: channel constraint support
>>    ASoC: broadwell: channel constraint support
>>
>>   sound/soc/intel/boards/bdw-rt5650.c | 34 
>> ++++++++++++++++++++++++++++++++++
>>   sound/soc/intel/boards/bdw-rt5677.c | 33 
>> +++++++++++++++++++++++++++++++++
>>   sound/soc/intel/boards/broadwell.c  | 33 
>> +++++++++++++++++++++++++++++++++
>>   3 files changed, 100 insertions(+)
>>

RE: [PATCH 0/3] add channel constraint for BDW machine drivers

[Lu, Brent]

> >
> > Apart from review given for each and every patch (although most issues
> > are shared so there is not as much to address) my question is:
> > - are these hw limitations or software (machine board) limitations?

The limitation comes from board. Bdw-rt5677 and Broadwell are using I2S with
2 microphones while Bdw-rt5650 is using PCM TDM with 4 microphones. Our
DSP supports stereo playback and 2 or 4-channel capture (haswell/sst-haswell-pcm.c).


> >
> > Czarek
> >
> >> Brent Lu (3):
> >>    ASoC: bdw-rt5677: channel constraint support
> >>    ASoC: bdw-rt5650: channel constraint support
> >>    ASoC: broadwell: channel constraint support
> >>
> >>   sound/soc/intel/boards/bdw-rt5650.c | 34
> >> ++++++++++++++++++++++++++++++++++
> >>   sound/soc/intel/boards/bdw-rt5677.c | 33
> >> +++++++++++++++++++++++++++++++++
> >>   sound/soc/intel/boards/broadwell.c  | 33
> >> +++++++++++++++++++++++++++++++++
> >>   3 files changed, 100 insertions(+)
> >>

RE: [PATCH 2/3] ASoC: bdw-rt5650: channel constraint support

[Lu, Brent]

> 
> What about playback configurations?
> 
> Title for the overall series fits better than the one chosen for actual patches.
> "channel constraint support" is misleading. Constraints are added or removed
> but certainly not supported.
> 

The FE DAI supports stereo playback only so I don't install the constraint for playback stream.

static struct snd_soc_dai_driver hsw_dais[] = {
	{
		.name  = "System Pin",
		.id = HSW_PCM_DAI_ID_SYSTEM,
		.playback = {
			.stream_name = "System Playback",
			.channels_min = 2,
			.channels_max = 2,
			.rates = SNDRV_PCM_RATE_48000,
			.formats = SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S16_LE,
		},

> > Signed-off-by: Brent Lu <brent.lu@intel.com>
> > ---
> >   sound/soc/intel/boards/bdw-rt5650.c | 34
> ++++++++++++++++++++++++++++++++++
> >   1 file changed, 34 insertions(+)
> >
> > diff --git a/sound/soc/intel/boards/bdw-rt5650.c
> > b/sound/soc/intel/boards/bdw-rt5650.c
> > index af2f502..dd4f219 100644
> > --- a/sound/soc/intel/boards/bdw-rt5650.c
> > +++ b/sound/soc/intel/boards/bdw-rt5650.c
> > @@ -21,6 +21,9 @@
> >
> >   #include "../../codecs/rt5645.h"
> >
> > +#define DUAL_CHANNEL 2
> > +#define QUAD_CHANNEL 4
> > +
> 
> Remove, we need not additional too-obvious macro. One could argue 'STEREO'
> is a better choice for '2' channel configuration too.
> 
> >   struct bdw_rt5650_priv {
> >   	struct gpio_desc *gpio_hp_en;
> >   	struct snd_soc_component *component; @@ -162,6 +165,36 @@
> static
> > int bdw_rt5650_rtd_init(struct snd_soc_pcm_runtime *rtd)
> >   }
> >   #endif
> >
> > +static const unsigned int channels[] = {
> > +	DUAL_CHANNEL, QUAD_CHANNEL,
> 
> Inline as stated above.
> 
> > +};
> > +
> > +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)
> 
> Entire file uses 'bdw_rt5650_' naming convention. Let's not stray away from that
> path now.
> 
> > +{
> > +	struct snd_pcm_runtime *runtime = substream->runtime;
> > +
> 
> Missing hw.channels_max assignment from rt5677 - inconsistency/ copy error?
> 
I just copy the assignment from other machine driver but I think the assignment is
redundant. The runtime->hw will be initialized in the dpcm_init_runtime_hw()
function and the channel_max will be overwritten.

dpcm_fe_dai_startup
-> dpcm_be_dai_startup
-> soc_pcm_open
    => here our bdw_fe_startup () is called
-> dpcm_set_fe_runtime
---> dpcm_init_runtime_hw
    => here runtime->hw is initialized

> > +	/*
> > +	 * On this platform for PCM device we support,
> > +	 * 2 or 4 channel capture
> > +	 */
> 
> Sometimes you add a newline add and before, while other times just one, before
> the comment. Please streamline the format across all patches in the series.
> Comment can be more strict too
> 	/* Board supports stereo and quad configurations */
> 
> > +	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
> > +		snd_pcm_hw_constraint_list(runtime, 0,
> > +
> SNDRV_PCM_HW_PARAM_CHANNELS,
> > +					   &constraints_channels);
> 
> Redesign to reduce indentation and improve readability -
> 	if (stream != capture)
> 		return 0;
> 
> 	return snd_pcm_hw_contraint_list(...);
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct snd_soc_ops bdw_rt5650_fe_ops = {
> > +	.startup = bdw_fe_startup,
> > +};
> > +
> >   static int bdw_rt5650_init(struct snd_soc_pcm_runtime *rtd)
> >   {
> >   	struct bdw_rt5650_priv *bdw_rt5650 = @@ -234,6 +267,7 @@ static
> > struct snd_soc_dai_link bdw_rt5650_dais[] = {
> >   		.name = "System PCM",
> >   		.stream_name = "System Playback",
> >   		.dynamic = 1,
> > +		.ops = &bdw_rt5650_fe_ops,
> >   #if !IS_ENABLED(CONFIG_SND_SOC_SOF_BROADWELL)
> >   		.init = bdw_rt5650_rtd_init,
> >   #endif
> >