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