| Message ID | 20201003141950.455829-3-peron.clem@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Allwinner H3/H5/H6/A64 HDMI audio | expand |
On Sat, Oct 03, 2020 at 04:19:38PM +0200, Clément Péron wrote: > As slots and slot_width can be set manually using set_tdm(). > These values are then kept in sun4i_i2s struct. > So we need to check if these values are setted or not > in the struct. > > Avoid to check for this logic in set_chan_cfg(). This will > duplicate the same check instead pass the required values > as params to set_chan_cfg(). > > This will also avoid a bug when we will enable 20/24bits support, > i2s->slot_width is not actually used in the lrck_period computation. > > Suggested-by: Samuel Holland <samuel@sholland.org> > Signed-off-by: Clément Péron <peron.clem@gmail.com> > --- > sound/soc/sunxi/sun4i-i2s.c | 36 ++++++++++++++---------------------- > 1 file changed, 14 insertions(+), 22 deletions(-) > > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c > index c5ccd423e6d3..1f577dbc20a6 100644 > --- a/sound/soc/sunxi/sun4i-i2s.c > +++ b/sound/soc/sunxi/sun4i-i2s.c > @@ -177,8 +177,9 @@ struct sun4i_i2s_quirks { > unsigned long (*get_bclk_parent_rate)(const struct sun4i_i2s *); > s8 (*get_sr)(const struct sun4i_i2s *, int); > s8 (*get_wss)(const struct sun4i_i2s *, int); > - int (*set_chan_cfg)(const struct sun4i_i2s *, > - const struct snd_pcm_hw_params *); > + int (*set_chan_cfg)(const struct sun4i_i2s *i2s, > + unsigned int channels, unsigned int slots, > + unsigned int slot_width); > int (*set_fmt)(const struct sun4i_i2s *, unsigned int); > }; > > @@ -414,10 +415,9 @@ static s8 sun8i_i2s_get_sr_wss(const struct sun4i_i2s *i2s, int width) > } > > static int sun4i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > - const struct snd_pcm_hw_params *params) > + unsigned int channels, unsigned int slots, > + unsigned int slot_width) > { > - unsigned int channels = params_channels(params); > - > /* Map the channels for playback and capture */ > regmap_write(i2s->regmap, SUN4I_I2S_TX_CHAN_MAP_REG, 0x76543210); > regmap_write(i2s->regmap, SUN4I_I2S_RX_CHAN_MAP_REG, 0x00003210); > @@ -434,15 +434,11 @@ static int sun4i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > } > > static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > - const struct snd_pcm_hw_params *params) > + unsigned int channels, unsigned int slots, > + unsigned int slot_width) > { > - unsigned int channels = params_channels(params); > - unsigned int slots = channels; > unsigned int lrck_period; > > - if (i2s->slots) > - slots = i2s->slots; > - > /* Map the channels for playback and capture */ > regmap_write(i2s->regmap, SUN8I_I2S_TX_CHAN_MAP_REG, 0x76543210); > regmap_write(i2s->regmap, SUN8I_I2S_RX_CHAN_MAP_REG, 0x76543210); > @@ -467,11 +463,11 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > case SND_SOC_DAIFMT_DSP_B: > case SND_SOC_DAIFMT_LEFT_J: > case SND_SOC_DAIFMT_RIGHT_J: > - lrck_period = params_physical_width(params) * slots; > + lrck_period = slot_width * slots; > break; > > case SND_SOC_DAIFMT_I2S: > - lrck_period = params_physical_width(params); > + lrck_period = slot_width; > break; > > default: > @@ -490,15 +486,11 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > } > > static int sun50i_h6_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > - const struct snd_pcm_hw_params *params) > + unsigned int channels, unsigned int slots, > + unsigned int slot_width) > { > - unsigned int channels = params_channels(params); > - unsigned int slots = channels; > unsigned int lrck_period; > > - if (i2s->slots) > - slots = i2s->slots; > - > /* Map the channels for playback and capture */ > regmap_write(i2s->regmap, SUN50I_H6_I2S_TX_CHAN_MAP0_REG, 0xFEDCBA98); > regmap_write(i2s->regmap, SUN50I_H6_I2S_TX_CHAN_MAP1_REG, 0x76543210); > @@ -525,11 +517,11 @@ static int sun50i_h6_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > case SND_SOC_DAIFMT_DSP_B: > case SND_SOC_DAIFMT_LEFT_J: > case SND_SOC_DAIFMT_RIGHT_J: > - lrck_period = params_physical_width(params) * slots; > + lrck_period = slot_width * slots; > break; > > case SND_SOC_DAIFMT_I2S: > - lrck_period = params_physical_width(params); > + lrck_period = slot_width; > break; > > default: > @@ -565,7 +557,7 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, > if (i2s->slot_width) > slot_width = i2s->slot_width; > > - ret = i2s->variant->set_chan_cfg(i2s, params); > + ret = i2s->variant->set_chan_cfg(i2s, channels, slots, slot_width); Isn't slots and slot_width set to 0 here ? And therefore, wouldn't we set lrck_period to 0 if we're using any format but I2S? More importantly, I'm not really convinced this needs to be done, and it introduces some side effects that are not explained. Maxime
Hi Maxime, On Mon, 5 Oct 2020 at 14:13, Maxime Ripard <maxime@cerno.tech> wrote: > > On Sat, Oct 03, 2020 at 04:19:38PM +0200, Clément Péron wrote: > > As slots and slot_width can be set manually using set_tdm(). > > These values are then kept in sun4i_i2s struct. > > So we need to check if these values are setted or not > > in the struct. > > > > Avoid to check for this logic in set_chan_cfg(). This will > > duplicate the same check instead pass the required values > > as params to set_chan_cfg(). > > > > This will also avoid a bug when we will enable 20/24bits support, > > i2s->slot_width is not actually used in the lrck_period computation. > > > > Suggested-by: Samuel Holland <samuel@sholland.org> > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > --- > > sound/soc/sunxi/sun4i-i2s.c | 36 ++++++++++++++---------------------- > > 1 file changed, 14 insertions(+), 22 deletions(-) > > > > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c > > index c5ccd423e6d3..1f577dbc20a6 100644 > > --- a/sound/soc/sunxi/sun4i-i2s.c > > +++ b/sound/soc/sunxi/sun4i-i2s.c > > @@ -177,8 +177,9 @@ struct sun4i_i2s_quirks { > > unsigned long (*get_bclk_parent_rate)(const struct sun4i_i2s *); > > s8 (*get_sr)(const struct sun4i_i2s *, int); > > s8 (*get_wss)(const struct sun4i_i2s *, int); > > - int (*set_chan_cfg)(const struct sun4i_i2s *, > > - const struct snd_pcm_hw_params *); > > + int (*set_chan_cfg)(const struct sun4i_i2s *i2s, > > + unsigned int channels, unsigned int slots, > > + unsigned int slot_width); > > int (*set_fmt)(const struct sun4i_i2s *, unsigned int); > > }; > > > > @@ -414,10 +415,9 @@ static s8 sun8i_i2s_get_sr_wss(const struct sun4i_i2s *i2s, int width) > > } > > > > static int sun4i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > > - const struct snd_pcm_hw_params *params) > > + unsigned int channels, unsigned int slots, > > + unsigned int slot_width) > > { > > - unsigned int channels = params_channels(params); > > - > > /* Map the channels for playback and capture */ > > regmap_write(i2s->regmap, SUN4I_I2S_TX_CHAN_MAP_REG, 0x76543210); > > regmap_write(i2s->regmap, SUN4I_I2S_RX_CHAN_MAP_REG, 0x00003210); > > @@ -434,15 +434,11 @@ static int sun4i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > > } > > > > static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > > - const struct snd_pcm_hw_params *params) > > + unsigned int channels, unsigned int slots, > > + unsigned int slot_width) > > { > > - unsigned int channels = params_channels(params); > > - unsigned int slots = channels; > > unsigned int lrck_period; > > > > - if (i2s->slots) > > - slots = i2s->slots; > > - > > /* Map the channels for playback and capture */ > > regmap_write(i2s->regmap, SUN8I_I2S_TX_CHAN_MAP_REG, 0x76543210); > > regmap_write(i2s->regmap, SUN8I_I2S_RX_CHAN_MAP_REG, 0x76543210); > > @@ -467,11 +463,11 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > > case SND_SOC_DAIFMT_DSP_B: > > case SND_SOC_DAIFMT_LEFT_J: > > case SND_SOC_DAIFMT_RIGHT_J: > > - lrck_period = params_physical_width(params) * slots; > > + lrck_period = slot_width * slots; > > break; > > > > case SND_SOC_DAIFMT_I2S: > > - lrck_period = params_physical_width(params); > > + lrck_period = slot_width; > > break; > > > > default: > > @@ -490,15 +486,11 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > > } > > > > static int sun50i_h6_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > > - const struct snd_pcm_hw_params *params) > > + unsigned int channels, unsigned int slots, > > + unsigned int slot_width) > > { > > - unsigned int channels = params_channels(params); > > - unsigned int slots = channels; > > unsigned int lrck_period; > > > > - if (i2s->slots) > > - slots = i2s->slots; > > - > > /* Map the channels for playback and capture */ > > regmap_write(i2s->regmap, SUN50I_H6_I2S_TX_CHAN_MAP0_REG, 0xFEDCBA98); > > regmap_write(i2s->regmap, SUN50I_H6_I2S_TX_CHAN_MAP1_REG, 0x76543210); > > @@ -525,11 +517,11 @@ static int sun50i_h6_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > > case SND_SOC_DAIFMT_DSP_B: > > case SND_SOC_DAIFMT_LEFT_J: > > case SND_SOC_DAIFMT_RIGHT_J: > > - lrck_period = params_physical_width(params) * slots; > > + lrck_period = slot_width * slots; > > break; > > > > case SND_SOC_DAIFMT_I2S: > > - lrck_period = params_physical_width(params); > > + lrck_period = slot_width; > > break; > > > > default: > > @@ -565,7 +557,7 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, > > if (i2s->slot_width) > > slot_width = i2s->slot_width; > > > > - ret = i2s->variant->set_chan_cfg(i2s, params); > > + ret = i2s->variant->set_chan_cfg(i2s, channels, slots, slot_width); > > Isn't slots and slot_width set to 0 here ? No, there is still a check before calling the set_cfg function. unsigned int slot_width = params_physical_width(params); unsigned int channels = params_channels(params); unsigned int slots = channels; if (i2s->slots) slots = i2s->slots; if (i2s->slot_width) slot_width = i2s->slot_width; ret = i2s->variant->set_chan_cfg(i2s, channels, slots, slot_width); So slot_width will be equal to params_physical_width(params); like before Clement > > And therefore, wouldn't we set lrck_period to 0 if we're using any > format but I2S? > > More importantly, I'm not really convinced this needs to be done, and it > introduces some side effects that are not explained. > > Maxime
On 10/5/20 7:13 AM, Maxime Ripard wrote: > On Sat, Oct 03, 2020 at 04:19:38PM +0200, Clément Péron wrote: >> As slots and slot_width can be set manually using set_tdm(). >> These values are then kept in sun4i_i2s struct. >> So we need to check if these values are setted or not >> in the struct. >> >> Avoid to check for this logic in set_chan_cfg(). This will >> duplicate the same check instead pass the required values >> as params to set_chan_cfg(). >> >> This will also avoid a bug when we will enable 20/24bits support, >> i2s->slot_width is not actually used in the lrck_period computation. >> >> Suggested-by: Samuel Holland <samuel@sholland.org> >> Signed-off-by: Clément Péron <peron.clem@gmail.com> >> --- >> sound/soc/sunxi/sun4i-i2s.c | 36 ++++++++++++++---------------------- >> 1 file changed, 14 insertions(+), 22 deletions(-) >> >> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c >> index c5ccd423e6d3..1f577dbc20a6 100644 >> --- a/sound/soc/sunxi/sun4i-i2s.c >> +++ b/sound/soc/sunxi/sun4i-i2s.c >> @@ -177,8 +177,9 @@ struct sun4i_i2s_quirks { >> unsigned long (*get_bclk_parent_rate)(const struct sun4i_i2s *); >> s8 (*get_sr)(const struct sun4i_i2s *, int); >> s8 (*get_wss)(const struct sun4i_i2s *, int); >> - int (*set_chan_cfg)(const struct sun4i_i2s *, >> - const struct snd_pcm_hw_params *); >> + int (*set_chan_cfg)(const struct sun4i_i2s *i2s, >> + unsigned int channels, unsigned int slots, >> + unsigned int slot_width); >> int (*set_fmt)(const struct sun4i_i2s *, unsigned int); >> }; >> >> @@ -414,10 +415,9 @@ static s8 sun8i_i2s_get_sr_wss(const struct sun4i_i2s *i2s, int width) >> } >> >> static int sun4i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, >> - const struct snd_pcm_hw_params *params) >> + unsigned int channels, unsigned int slots, >> + unsigned int slot_width) >> { >> - unsigned int channels = params_channels(params); >> - >> /* Map the channels for playback and capture */ >> regmap_write(i2s->regmap, SUN4I_I2S_TX_CHAN_MAP_REG, 0x76543210); >> regmap_write(i2s->regmap, SUN4I_I2S_RX_CHAN_MAP_REG, 0x00003210); >> @@ -434,15 +434,11 @@ static int sun4i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, >> } >> >> static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, >> - const struct snd_pcm_hw_params *params) >> + unsigned int channels, unsigned int slots, >> + unsigned int slot_width) >> { >> - unsigned int channels = params_channels(params); >> - unsigned int slots = channels; >> unsigned int lrck_period; >> >> - if (i2s->slots) >> - slots = i2s->slots; >> - >> /* Map the channels for playback and capture */ >> regmap_write(i2s->regmap, SUN8I_I2S_TX_CHAN_MAP_REG, 0x76543210); >> regmap_write(i2s->regmap, SUN8I_I2S_RX_CHAN_MAP_REG, 0x76543210); >> @@ -467,11 +463,11 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, >> case SND_SOC_DAIFMT_DSP_B: >> case SND_SOC_DAIFMT_LEFT_J: >> case SND_SOC_DAIFMT_RIGHT_J: >> - lrck_period = params_physical_width(params) * slots; >> + lrck_period = slot_width * slots; >> break; >> >> case SND_SOC_DAIFMT_I2S: >> - lrck_period = params_physical_width(params); >> + lrck_period = slot_width; >> break; >> >> default: >> @@ -490,15 +486,11 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, >> } >> >> static int sun50i_h6_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, >> - const struct snd_pcm_hw_params *params) >> + unsigned int channels, unsigned int slots, >> + unsigned int slot_width) >> { >> - unsigned int channels = params_channels(params); >> - unsigned int slots = channels; >> unsigned int lrck_period; >> >> - if (i2s->slots) >> - slots = i2s->slots; >> - >> /* Map the channels for playback and capture */ >> regmap_write(i2s->regmap, SUN50I_H6_I2S_TX_CHAN_MAP0_REG, 0xFEDCBA98); >> regmap_write(i2s->regmap, SUN50I_H6_I2S_TX_CHAN_MAP1_REG, 0x76543210); >> @@ -525,11 +517,11 @@ static int sun50i_h6_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, >> case SND_SOC_DAIFMT_DSP_B: >> case SND_SOC_DAIFMT_LEFT_J: >> case SND_SOC_DAIFMT_RIGHT_J: >> - lrck_period = params_physical_width(params) * slots; >> + lrck_period = slot_width * slots; >> break; >> >> case SND_SOC_DAIFMT_I2S: >> - lrck_period = params_physical_width(params); >> + lrck_period = slot_width; >> break; >> >> default: >> @@ -565,7 +557,7 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, >> if (i2s->slot_width) >> slot_width = i2s->slot_width; >> >> - ret = i2s->variant->set_chan_cfg(i2s, params); >> + ret = i2s->variant->set_chan_cfg(i2s, channels, slots, slot_width); > > Isn't slots and slot_width set to 0 here ? > > And therefore, wouldn't we set lrck_period to 0 if we're using any > format but I2S? > > More importantly, I'm not really convinced this needs to be done, and it > introduces some side effects that are not explained. If I set dai-tdm-slot-width = <32> and start a stream using S16_LE, currently we would calculate BCLK for 32-bit slots, but program lrck_period for 16-bit slots, making the sample rate double what we expected. That sounds like a bug to me. (Because of that, as Chen-Yu mentioned in reply to v5, this should be the first patch in the series.) Could you be more specific what side effects you are referring to? > Maxime Cheers, Samuel
Hi, On Mon, Oct 05, 2020 at 03:23:12PM +0200, Clément Péron wrote: > On Mon, 5 Oct 2020 at 14:13, Maxime Ripard <maxime@cerno.tech> wrote: > > > > On Sat, Oct 03, 2020 at 04:19:38PM +0200, Clément Péron wrote: > > > As slots and slot_width can be set manually using set_tdm(). > > > These values are then kept in sun4i_i2s struct. > > > So we need to check if these values are setted or not > > > in the struct. > > > > > > Avoid to check for this logic in set_chan_cfg(). This will > > > duplicate the same check instead pass the required values > > > as params to set_chan_cfg(). > > > > > > This will also avoid a bug when we will enable 20/24bits support, > > > i2s->slot_width is not actually used in the lrck_period computation. > > > > > > Suggested-by: Samuel Holland <samuel@sholland.org> > > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > > --- > > > sound/soc/sunxi/sun4i-i2s.c | 36 ++++++++++++++---------------------- > > > 1 file changed, 14 insertions(+), 22 deletions(-) > > > > > > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c > > > index c5ccd423e6d3..1f577dbc20a6 100644 > > > --- a/sound/soc/sunxi/sun4i-i2s.c > > > +++ b/sound/soc/sunxi/sun4i-i2s.c > > > @@ -177,8 +177,9 @@ struct sun4i_i2s_quirks { > > > unsigned long (*get_bclk_parent_rate)(const struct sun4i_i2s *); > > > s8 (*get_sr)(const struct sun4i_i2s *, int); > > > s8 (*get_wss)(const struct sun4i_i2s *, int); > > > - int (*set_chan_cfg)(const struct sun4i_i2s *, > > > - const struct snd_pcm_hw_params *); > > > + int (*set_chan_cfg)(const struct sun4i_i2s *i2s, > > > + unsigned int channels, unsigned int slots, > > > + unsigned int slot_width); > > > int (*set_fmt)(const struct sun4i_i2s *, unsigned int); > > > }; > > > > > > @@ -414,10 +415,9 @@ static s8 sun8i_i2s_get_sr_wss(const struct sun4i_i2s *i2s, int width) > > > } > > > > > > static int sun4i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > > > - const struct snd_pcm_hw_params *params) > > > + unsigned int channels, unsigned int slots, > > > + unsigned int slot_width) > > > { > > > - unsigned int channels = params_channels(params); > > > - > > > /* Map the channels for playback and capture */ > > > regmap_write(i2s->regmap, SUN4I_I2S_TX_CHAN_MAP_REG, 0x76543210); > > > regmap_write(i2s->regmap, SUN4I_I2S_RX_CHAN_MAP_REG, 0x00003210); > > > @@ -434,15 +434,11 @@ static int sun4i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > > > } > > > > > > static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > > > - const struct snd_pcm_hw_params *params) > > > + unsigned int channels, unsigned int slots, > > > + unsigned int slot_width) > > > { > > > - unsigned int channels = params_channels(params); > > > - unsigned int slots = channels; > > > unsigned int lrck_period; > > > > > > - if (i2s->slots) > > > - slots = i2s->slots; > > > - > > > /* Map the channels for playback and capture */ > > > regmap_write(i2s->regmap, SUN8I_I2S_TX_CHAN_MAP_REG, 0x76543210); > > > regmap_write(i2s->regmap, SUN8I_I2S_RX_CHAN_MAP_REG, 0x76543210); > > > @@ -467,11 +463,11 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > > > case SND_SOC_DAIFMT_DSP_B: > > > case SND_SOC_DAIFMT_LEFT_J: > > > case SND_SOC_DAIFMT_RIGHT_J: > > > - lrck_period = params_physical_width(params) * slots; > > > + lrck_period = slot_width * slots; > > > break; > > > > > > case SND_SOC_DAIFMT_I2S: > > > - lrck_period = params_physical_width(params); > > > + lrck_period = slot_width; > > > break; > > > > > > default: > > > @@ -490,15 +486,11 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > > > } > > > > > > static int sun50i_h6_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > > > - const struct snd_pcm_hw_params *params) > > > + unsigned int channels, unsigned int slots, > > > + unsigned int slot_width) > > > { > > > - unsigned int channels = params_channels(params); > > > - unsigned int slots = channels; > > > unsigned int lrck_period; > > > > > > - if (i2s->slots) > > > - slots = i2s->slots; > > > - > > > /* Map the channels for playback and capture */ > > > regmap_write(i2s->regmap, SUN50I_H6_I2S_TX_CHAN_MAP0_REG, 0xFEDCBA98); > > > regmap_write(i2s->regmap, SUN50I_H6_I2S_TX_CHAN_MAP1_REG, 0x76543210); > > > @@ -525,11 +517,11 @@ static int sun50i_h6_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > > > case SND_SOC_DAIFMT_DSP_B: > > > case SND_SOC_DAIFMT_LEFT_J: > > > case SND_SOC_DAIFMT_RIGHT_J: > > > - lrck_period = params_physical_width(params) * slots; > > > + lrck_period = slot_width * slots; > > > break; > > > > > > case SND_SOC_DAIFMT_I2S: > > > - lrck_period = params_physical_width(params); > > > + lrck_period = slot_width; > > > break; > > > > > > default: > > > @@ -565,7 +557,7 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, > > > if (i2s->slot_width) > > > slot_width = i2s->slot_width; > > > > > > - ret = i2s->variant->set_chan_cfg(i2s, params); > > > + ret = i2s->variant->set_chan_cfg(i2s, channels, slots, slot_width); > > > > Isn't slots and slot_width set to 0 here ? > > No, there is still a check before calling the set_cfg function. > > unsigned int slot_width = params_physical_width(params); > unsigned int channels = params_channels(params); > unsigned int slots = channels; > > if (i2s->slots) > slots = i2s->slots; > > if (i2s->slot_width) > slot_width = i2s->slot_width; > > ret = i2s->variant->set_chan_cfg(i2s, channels, slots, slot_width); > > So slot_width will be equal to params_physical_width(params); > like before Still, it's not obvious what slots and slot_width are going to be set to in a non-TDM setup where that doesn't really make much sense. I assume you want to reduce the boilerplate, then we can make an helper to get the frame size and the number of channels / slots if needed Maxime
On Tue, Oct 06, 2020 at 12:03:14AM -0500, Samuel Holland wrote: > On 10/5/20 7:13 AM, Maxime Ripard wrote: > > On Sat, Oct 03, 2020 at 04:19:38PM +0200, Clément Péron wrote: > >> As slots and slot_width can be set manually using set_tdm(). > >> These values are then kept in sun4i_i2s struct. > >> So we need to check if these values are setted or not > >> in the struct. > >> > >> Avoid to check for this logic in set_chan_cfg(). This will > >> duplicate the same check instead pass the required values > >> as params to set_chan_cfg(). > >> > >> This will also avoid a bug when we will enable 20/24bits support, > >> i2s->slot_width is not actually used in the lrck_period computation. > >> > >> Suggested-by: Samuel Holland <samuel@sholland.org> > >> Signed-off-by: Clément Péron <peron.clem@gmail.com> > >> --- > >> sound/soc/sunxi/sun4i-i2s.c | 36 ++++++++++++++---------------------- > >> 1 file changed, 14 insertions(+), 22 deletions(-) > >> > >> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c > >> index c5ccd423e6d3..1f577dbc20a6 100644 > >> --- a/sound/soc/sunxi/sun4i-i2s.c > >> +++ b/sound/soc/sunxi/sun4i-i2s.c > >> @@ -177,8 +177,9 @@ struct sun4i_i2s_quirks { > >> unsigned long (*get_bclk_parent_rate)(const struct sun4i_i2s *); > >> s8 (*get_sr)(const struct sun4i_i2s *, int); > >> s8 (*get_wss)(const struct sun4i_i2s *, int); > >> - int (*set_chan_cfg)(const struct sun4i_i2s *, > >> - const struct snd_pcm_hw_params *); > >> + int (*set_chan_cfg)(const struct sun4i_i2s *i2s, > >> + unsigned int channels, unsigned int slots, > >> + unsigned int slot_width); > >> int (*set_fmt)(const struct sun4i_i2s *, unsigned int); > >> }; > >> > >> @@ -414,10 +415,9 @@ static s8 sun8i_i2s_get_sr_wss(const struct sun4i_i2s *i2s, int width) > >> } > >> > >> static int sun4i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > >> - const struct snd_pcm_hw_params *params) > >> + unsigned int channels, unsigned int slots, > >> + unsigned int slot_width) > >> { > >> - unsigned int channels = params_channels(params); > >> - > >> /* Map the channels for playback and capture */ > >> regmap_write(i2s->regmap, SUN4I_I2S_TX_CHAN_MAP_REG, 0x76543210); > >> regmap_write(i2s->regmap, SUN4I_I2S_RX_CHAN_MAP_REG, 0x00003210); > >> @@ -434,15 +434,11 @@ static int sun4i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > >> } > >> > >> static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > >> - const struct snd_pcm_hw_params *params) > >> + unsigned int channels, unsigned int slots, > >> + unsigned int slot_width) > >> { > >> - unsigned int channels = params_channels(params); > >> - unsigned int slots = channels; > >> unsigned int lrck_period; > >> > >> - if (i2s->slots) > >> - slots = i2s->slots; > >> - > >> /* Map the channels for playback and capture */ > >> regmap_write(i2s->regmap, SUN8I_I2S_TX_CHAN_MAP_REG, 0x76543210); > >> regmap_write(i2s->regmap, SUN8I_I2S_RX_CHAN_MAP_REG, 0x76543210); > >> @@ -467,11 +463,11 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > >> case SND_SOC_DAIFMT_DSP_B: > >> case SND_SOC_DAIFMT_LEFT_J: > >> case SND_SOC_DAIFMT_RIGHT_J: > >> - lrck_period = params_physical_width(params) * slots; > >> + lrck_period = slot_width * slots; > >> break; > >> > >> case SND_SOC_DAIFMT_I2S: > >> - lrck_period = params_physical_width(params); > >> + lrck_period = slot_width; > >> break; > >> > >> default: > >> @@ -490,15 +486,11 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > >> } > >> > >> static int sun50i_h6_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > >> - const struct snd_pcm_hw_params *params) > >> + unsigned int channels, unsigned int slots, > >> + unsigned int slot_width) > >> { > >> - unsigned int channels = params_channels(params); > >> - unsigned int slots = channels; > >> unsigned int lrck_period; > >> > >> - if (i2s->slots) > >> - slots = i2s->slots; > >> - > >> /* Map the channels for playback and capture */ > >> regmap_write(i2s->regmap, SUN50I_H6_I2S_TX_CHAN_MAP0_REG, 0xFEDCBA98); > >> regmap_write(i2s->regmap, SUN50I_H6_I2S_TX_CHAN_MAP1_REG, 0x76543210); > >> @@ -525,11 +517,11 @@ static int sun50i_h6_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > >> case SND_SOC_DAIFMT_DSP_B: > >> case SND_SOC_DAIFMT_LEFT_J: > >> case SND_SOC_DAIFMT_RIGHT_J: > >> - lrck_period = params_physical_width(params) * slots; > >> + lrck_period = slot_width * slots; > >> break; > >> > >> case SND_SOC_DAIFMT_I2S: > >> - lrck_period = params_physical_width(params); > >> + lrck_period = slot_width; > >> break; > >> > >> default: > >> @@ -565,7 +557,7 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, > >> if (i2s->slot_width) > >> slot_width = i2s->slot_width; > >> > >> - ret = i2s->variant->set_chan_cfg(i2s, params); > >> + ret = i2s->variant->set_chan_cfg(i2s, channels, slots, slot_width); > > > > Isn't slots and slot_width set to 0 here ? > > > > And therefore, wouldn't we set lrck_period to 0 if we're using any > > format but I2S? > > > > More importantly, I'm not really convinced this needs to be done, and it > > introduces some side effects that are not explained. > > If I set dai-tdm-slot-width = <32> and start a stream using S16_LE, > currently we would calculate BCLK for 32-bit slots, but program > lrck_period for 16-bit slots, making the sample rate double what we > expected. That sounds like a bug to me. (Because of that, as Chen-Yu > mentioned in reply to v5, this should be the first patch in the series.) I'm not denying that there's a bug though here :) You've spent way more time than I did with this driver recently, so I definitely take your word for it. > Could you be more specific what side effects you are referring to? I don't really like the change of semantics associated to the new prototype, and it becomes less obvious what we're supposed to do with slots and slot_width. Like, those are very TDM-y terms, are we supposed to honour them when running in I2S, or should we just ignore them? Maxime
On 10/12/20 7:15 AM, Maxime Ripard wrote: > Hi, > > On Mon, Oct 05, 2020 at 03:23:12PM +0200, Clément Péron wrote: >> On Mon, 5 Oct 2020 at 14:13, Maxime Ripard <maxime@cerno.tech> wrote: >>> >>> On Sat, Oct 03, 2020 at 04:19:38PM +0200, Clément Péron wrote: >>>> As slots and slot_width can be set manually using set_tdm(). >>>> These values are then kept in sun4i_i2s struct. >>>> So we need to check if these values are setted or not >>>> in the struct. >>>> >>>> Avoid to check for this logic in set_chan_cfg(). This will >>>> duplicate the same check instead pass the required values >>>> as params to set_chan_cfg(). >>>> >>>> This will also avoid a bug when we will enable 20/24bits support, >>>> i2s->slot_width is not actually used in the lrck_period computation. >>>> >>>> Suggested-by: Samuel Holland <samuel@sholland.org> >>>> Signed-off-by: Clément Péron <peron.clem@gmail.com> >>>> --- >>>> sound/soc/sunxi/sun4i-i2s.c | 36 ++++++++++++++---------------------- >>>> 1 file changed, 14 insertions(+), 22 deletions(-) >>>> >>>> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c >>>> index c5ccd423e6d3..1f577dbc20a6 100644 >>>> --- a/sound/soc/sunxi/sun4i-i2s.c >>>> +++ b/sound/soc/sunxi/sun4i-i2s.c >>>> @@ -177,8 +177,9 @@ struct sun4i_i2s_quirks { >>>> unsigned long (*get_bclk_parent_rate)(const struct sun4i_i2s *); >>>> s8 (*get_sr)(const struct sun4i_i2s *, int); >>>> s8 (*get_wss)(const struct sun4i_i2s *, int); >>>> - int (*set_chan_cfg)(const struct sun4i_i2s *, >>>> - const struct snd_pcm_hw_params *); >>>> + int (*set_chan_cfg)(const struct sun4i_i2s *i2s, >>>> + unsigned int channels, unsigned int slots, >>>> + unsigned int slot_width); >>>> int (*set_fmt)(const struct sun4i_i2s *, unsigned int); >>>> }; >>>> >>>> @@ -414,10 +415,9 @@ static s8 sun8i_i2s_get_sr_wss(const struct sun4i_i2s *i2s, int width) >>>> } >>>> >>>> static int sun4i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, >>>> - const struct snd_pcm_hw_params *params) >>>> + unsigned int channels, unsigned int slots, >>>> + unsigned int slot_width) >>>> { >>>> - unsigned int channels = params_channels(params); >>>> - >>>> /* Map the channels for playback and capture */ >>>> regmap_write(i2s->regmap, SUN4I_I2S_TX_CHAN_MAP_REG, 0x76543210); >>>> regmap_write(i2s->regmap, SUN4I_I2S_RX_CHAN_MAP_REG, 0x00003210); >>>> @@ -434,15 +434,11 @@ static int sun4i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, >>>> } >>>> >>>> static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, >>>> - const struct snd_pcm_hw_params *params) >>>> + unsigned int channels, unsigned int slots, >>>> + unsigned int slot_width) >>>> { >>>> - unsigned int channels = params_channels(params); >>>> - unsigned int slots = channels; >>>> unsigned int lrck_period; >>>> >>>> - if (i2s->slots) >>>> - slots = i2s->slots; >>>> - >>>> /* Map the channels for playback and capture */ >>>> regmap_write(i2s->regmap, SUN8I_I2S_TX_CHAN_MAP_REG, 0x76543210); >>>> regmap_write(i2s->regmap, SUN8I_I2S_RX_CHAN_MAP_REG, 0x76543210); >>>> @@ -467,11 +463,11 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, >>>> case SND_SOC_DAIFMT_DSP_B: >>>> case SND_SOC_DAIFMT_LEFT_J: >>>> case SND_SOC_DAIFMT_RIGHT_J: >>>> - lrck_period = params_physical_width(params) * slots; >>>> + lrck_period = slot_width * slots; >>>> break; >>>> >>>> case SND_SOC_DAIFMT_I2S: >>>> - lrck_period = params_physical_width(params); >>>> + lrck_period = slot_width; >>>> break; >>>> >>>> default: >>>> @@ -490,15 +486,11 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, >>>> } >>>> >>>> static int sun50i_h6_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, >>>> - const struct snd_pcm_hw_params *params) >>>> + unsigned int channels, unsigned int slots, >>>> + unsigned int slot_width) >>>> { >>>> - unsigned int channels = params_channels(params); >>>> - unsigned int slots = channels; >>>> unsigned int lrck_period; >>>> >>>> - if (i2s->slots) >>>> - slots = i2s->slots; >>>> - >>>> /* Map the channels for playback and capture */ >>>> regmap_write(i2s->regmap, SUN50I_H6_I2S_TX_CHAN_MAP0_REG, 0xFEDCBA98); >>>> regmap_write(i2s->regmap, SUN50I_H6_I2S_TX_CHAN_MAP1_REG, 0x76543210); >>>> @@ -525,11 +517,11 @@ static int sun50i_h6_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, >>>> case SND_SOC_DAIFMT_DSP_B: >>>> case SND_SOC_DAIFMT_LEFT_J: >>>> case SND_SOC_DAIFMT_RIGHT_J: >>>> - lrck_period = params_physical_width(params) * slots; >>>> + lrck_period = slot_width * slots; >>>> break; >>>> >>>> case SND_SOC_DAIFMT_I2S: >>>> - lrck_period = params_physical_width(params); >>>> + lrck_period = slot_width; >>>> break; >>>> >>>> default: >>>> @@ -565,7 +557,7 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, >>>> if (i2s->slot_width) >>>> slot_width = i2s->slot_width; >>>> >>>> - ret = i2s->variant->set_chan_cfg(i2s, params); >>>> + ret = i2s->variant->set_chan_cfg(i2s, channels, slots, slot_width); >>> >>> Isn't slots and slot_width set to 0 here ? >> >> No, there is still a check before calling the set_cfg function. >> >> unsigned int slot_width = params_physical_width(params); >> unsigned int channels = params_channels(params); >> unsigned int slots = channels; >> >> if (i2s->slots) >> slots = i2s->slots; >> >> if (i2s->slot_width) >> slot_width = i2s->slot_width; >> >> ret = i2s->variant->set_chan_cfg(i2s, channels, slots, slot_width); >> >> So slot_width will be equal to params_physical_width(params); >> like before > > Still, it's not obvious what slots and slot_width are going to be set to > in a non-TDM setup where that doesn't really make much sense. My understanding is: "slots" is channels per frame + padding slots, regardless of format. "slot_width" is bits per sample + padding bits, regardless of format. Some formats may require or prohibit certain padding, but that has no effect on the definitions. That seems clear to me? At least that's what I implemented (and you acked) in sun8i-codec. > I assume you want to reduce the boilerplate, then we can make an helper > to get the frame size and the number of channels / slots if needed What would you name the return values, if not "slots" and "slot_width"? "channels" and "word_size" would be inaccurate, because those terms refer to the values without padding. > Maxime Cheers, Samuel
Hi Samuel, On Mon, Oct 12, 2020 at 08:15:30PM -0500, Samuel Holland wrote: > On 10/12/20 7:15 AM, Maxime Ripard wrote: > > Hi, > > > > On Mon, Oct 05, 2020 at 03:23:12PM +0200, Clément Péron wrote: > >> On Mon, 5 Oct 2020 at 14:13, Maxime Ripard <maxime@cerno.tech> wrote: > >>> > >>> On Sat, Oct 03, 2020 at 04:19:38PM +0200, Clément Péron wrote: > >>>> As slots and slot_width can be set manually using set_tdm(). > >>>> These values are then kept in sun4i_i2s struct. > >>>> So we need to check if these values are setted or not > >>>> in the struct. > >>>> > >>>> Avoid to check for this logic in set_chan_cfg(). This will > >>>> duplicate the same check instead pass the required values > >>>> as params to set_chan_cfg(). > >>>> > >>>> This will also avoid a bug when we will enable 20/24bits support, > >>>> i2s->slot_width is not actually used in the lrck_period computation. > >>>> > >>>> Suggested-by: Samuel Holland <samuel@sholland.org> > >>>> Signed-off-by: Clément Péron <peron.clem@gmail.com> > >>>> --- > >>>> sound/soc/sunxi/sun4i-i2s.c | 36 ++++++++++++++---------------------- > >>>> 1 file changed, 14 insertions(+), 22 deletions(-) > >>>> > >>>> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c > >>>> index c5ccd423e6d3..1f577dbc20a6 100644 > >>>> --- a/sound/soc/sunxi/sun4i-i2s.c > >>>> +++ b/sound/soc/sunxi/sun4i-i2s.c > >>>> @@ -177,8 +177,9 @@ struct sun4i_i2s_quirks { > >>>> unsigned long (*get_bclk_parent_rate)(const struct sun4i_i2s *); > >>>> s8 (*get_sr)(const struct sun4i_i2s *, int); > >>>> s8 (*get_wss)(const struct sun4i_i2s *, int); > >>>> - int (*set_chan_cfg)(const struct sun4i_i2s *, > >>>> - const struct snd_pcm_hw_params *); > >>>> + int (*set_chan_cfg)(const struct sun4i_i2s *i2s, > >>>> + unsigned int channels, unsigned int slots, > >>>> + unsigned int slot_width); > >>>> int (*set_fmt)(const struct sun4i_i2s *, unsigned int); > >>>> }; > >>>> > >>>> @@ -414,10 +415,9 @@ static s8 sun8i_i2s_get_sr_wss(const struct sun4i_i2s *i2s, int width) > >>>> } > >>>> > >>>> static int sun4i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > >>>> - const struct snd_pcm_hw_params *params) > >>>> + unsigned int channels, unsigned int slots, > >>>> + unsigned int slot_width) > >>>> { > >>>> - unsigned int channels = params_channels(params); > >>>> - > >>>> /* Map the channels for playback and capture */ > >>>> regmap_write(i2s->regmap, SUN4I_I2S_TX_CHAN_MAP_REG, 0x76543210); > >>>> regmap_write(i2s->regmap, SUN4I_I2S_RX_CHAN_MAP_REG, 0x00003210); > >>>> @@ -434,15 +434,11 @@ static int sun4i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > >>>> } > >>>> > >>>> static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > >>>> - const struct snd_pcm_hw_params *params) > >>>> + unsigned int channels, unsigned int slots, > >>>> + unsigned int slot_width) > >>>> { > >>>> - unsigned int channels = params_channels(params); > >>>> - unsigned int slots = channels; > >>>> unsigned int lrck_period; > >>>> > >>>> - if (i2s->slots) > >>>> - slots = i2s->slots; > >>>> - > >>>> /* Map the channels for playback and capture */ > >>>> regmap_write(i2s->regmap, SUN8I_I2S_TX_CHAN_MAP_REG, 0x76543210); > >>>> regmap_write(i2s->regmap, SUN8I_I2S_RX_CHAN_MAP_REG, 0x76543210); > >>>> @@ -467,11 +463,11 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > >>>> case SND_SOC_DAIFMT_DSP_B: > >>>> case SND_SOC_DAIFMT_LEFT_J: > >>>> case SND_SOC_DAIFMT_RIGHT_J: > >>>> - lrck_period = params_physical_width(params) * slots; > >>>> + lrck_period = slot_width * slots; > >>>> break; > >>>> > >>>> case SND_SOC_DAIFMT_I2S: > >>>> - lrck_period = params_physical_width(params); > >>>> + lrck_period = slot_width; > >>>> break; > >>>> > >>>> default: > >>>> @@ -490,15 +486,11 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > >>>> } > >>>> > >>>> static int sun50i_h6_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > >>>> - const struct snd_pcm_hw_params *params) > >>>> + unsigned int channels, unsigned int slots, > >>>> + unsigned int slot_width) > >>>> { > >>>> - unsigned int channels = params_channels(params); > >>>> - unsigned int slots = channels; > >>>> unsigned int lrck_period; > >>>> > >>>> - if (i2s->slots) > >>>> - slots = i2s->slots; > >>>> - > >>>> /* Map the channels for playback and capture */ > >>>> regmap_write(i2s->regmap, SUN50I_H6_I2S_TX_CHAN_MAP0_REG, 0xFEDCBA98); > >>>> regmap_write(i2s->regmap, SUN50I_H6_I2S_TX_CHAN_MAP1_REG, 0x76543210); > >>>> @@ -525,11 +517,11 @@ static int sun50i_h6_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > >>>> case SND_SOC_DAIFMT_DSP_B: > >>>> case SND_SOC_DAIFMT_LEFT_J: > >>>> case SND_SOC_DAIFMT_RIGHT_J: > >>>> - lrck_period = params_physical_width(params) * slots; > >>>> + lrck_period = slot_width * slots; > >>>> break; > >>>> > >>>> case SND_SOC_DAIFMT_I2S: > >>>> - lrck_period = params_physical_width(params); > >>>> + lrck_period = slot_width; > >>>> break; > >>>> > >>>> default: > >>>> @@ -565,7 +557,7 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, > >>>> if (i2s->slot_width) > >>>> slot_width = i2s->slot_width; > >>>> > >>>> - ret = i2s->variant->set_chan_cfg(i2s, params); > >>>> + ret = i2s->variant->set_chan_cfg(i2s, channels, slots, slot_width); > >>> > >>> Isn't slots and slot_width set to 0 here ? > >> > >> No, there is still a check before calling the set_cfg function. > >> > >> unsigned int slot_width = params_physical_width(params); > >> unsigned int channels = params_channels(params); > >> unsigned int slots = channels; > >> > >> if (i2s->slots) > >> slots = i2s->slots; > >> > >> if (i2s->slot_width) > >> slot_width = i2s->slot_width; > >> > >> ret = i2s->variant->set_chan_cfg(i2s, channels, slots, slot_width); > >> > >> So slot_width will be equal to params_physical_width(params); > >> like before > > > > Still, it's not obvious what slots and slot_width are going to be set to > > in a non-TDM setup where that doesn't really make much sense. > > My understanding is: > > "slots" is channels per frame + padding slots, regardless of format. > "slot_width" is bits per sample + padding bits, regardless of format. > > Some formats may require or prohibit certain padding, but that has no > effect on the definitions. > > That seems clear to me? At least that's what I implemented (and you > acked) in sun8i-codec. Yeah I guess you're right. I'd still like at least a comment on top of the function making that clear so that no-one's confused Maxime
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index c5ccd423e6d3..1f577dbc20a6 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -177,8 +177,9 @@ struct sun4i_i2s_quirks { unsigned long (*get_bclk_parent_rate)(const struct sun4i_i2s *); s8 (*get_sr)(const struct sun4i_i2s *, int); s8 (*get_wss)(const struct sun4i_i2s *, int); - int (*set_chan_cfg)(const struct sun4i_i2s *, - const struct snd_pcm_hw_params *); + int (*set_chan_cfg)(const struct sun4i_i2s *i2s, + unsigned int channels, unsigned int slots, + unsigned int slot_width); int (*set_fmt)(const struct sun4i_i2s *, unsigned int); }; @@ -414,10 +415,9 @@ static s8 sun8i_i2s_get_sr_wss(const struct sun4i_i2s *i2s, int width) } static int sun4i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, - const struct snd_pcm_hw_params *params) + unsigned int channels, unsigned int slots, + unsigned int slot_width) { - unsigned int channels = params_channels(params); - /* Map the channels for playback and capture */ regmap_write(i2s->regmap, SUN4I_I2S_TX_CHAN_MAP_REG, 0x76543210); regmap_write(i2s->regmap, SUN4I_I2S_RX_CHAN_MAP_REG, 0x00003210); @@ -434,15 +434,11 @@ static int sun4i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, } static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, - const struct snd_pcm_hw_params *params) + unsigned int channels, unsigned int slots, + unsigned int slot_width) { - unsigned int channels = params_channels(params); - unsigned int slots = channels; unsigned int lrck_period; - if (i2s->slots) - slots = i2s->slots; - /* Map the channels for playback and capture */ regmap_write(i2s->regmap, SUN8I_I2S_TX_CHAN_MAP_REG, 0x76543210); regmap_write(i2s->regmap, SUN8I_I2S_RX_CHAN_MAP_REG, 0x76543210); @@ -467,11 +463,11 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, case SND_SOC_DAIFMT_DSP_B: case SND_SOC_DAIFMT_LEFT_J: case SND_SOC_DAIFMT_RIGHT_J: - lrck_period = params_physical_width(params) * slots; + lrck_period = slot_width * slots; break; case SND_SOC_DAIFMT_I2S: - lrck_period = params_physical_width(params); + lrck_period = slot_width; break; default: @@ -490,15 +486,11 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, } static int sun50i_h6_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, - const struct snd_pcm_hw_params *params) + unsigned int channels, unsigned int slots, + unsigned int slot_width) { - unsigned int channels = params_channels(params); - unsigned int slots = channels; unsigned int lrck_period; - if (i2s->slots) - slots = i2s->slots; - /* Map the channels for playback and capture */ regmap_write(i2s->regmap, SUN50I_H6_I2S_TX_CHAN_MAP0_REG, 0xFEDCBA98); regmap_write(i2s->regmap, SUN50I_H6_I2S_TX_CHAN_MAP1_REG, 0x76543210); @@ -525,11 +517,11 @@ static int sun50i_h6_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, case SND_SOC_DAIFMT_DSP_B: case SND_SOC_DAIFMT_LEFT_J: case SND_SOC_DAIFMT_RIGHT_J: - lrck_period = params_physical_width(params) * slots; + lrck_period = slot_width * slots; break; case SND_SOC_DAIFMT_I2S: - lrck_period = params_physical_width(params); + lrck_period = slot_width; break; default: @@ -565,7 +557,7 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, if (i2s->slot_width) slot_width = i2s->slot_width; - ret = i2s->variant->set_chan_cfg(i2s, params); + ret = i2s->variant->set_chan_cfg(i2s, channels, slots, slot_width); if (ret < 0) { dev_err(dai->dev, "Invalid channel configuration\n"); return ret;
As slots and slot_width can be set manually using set_tdm(). These values are then kept in sun4i_i2s struct. So we need to check if these values are setted or not in the struct. Avoid to check for this logic in set_chan_cfg(). This will duplicate the same check instead pass the required values as params to set_chan_cfg(). This will also avoid a bug when we will enable 20/24bits support, i2s->slot_width is not actually used in the lrck_period computation. Suggested-by: Samuel Holland <samuel@sholland.org> Signed-off-by: Clément Péron <peron.clem@gmail.com> --- sound/soc/sunxi/sun4i-i2s.c | 36 ++++++++++++++---------------------- 1 file changed, 14 insertions(+), 22 deletions(-)