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@xxxxxxxxx>
> > ---
> >   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
> >