| Message ID | 20201003141950.455829-1-peron.clem@gmail.com |
|---|---|
| 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
On Sat, Oct 03, 2020 at 04:19:39PM +0200, Clément Péron wrote: > We are actually using a complex formula to just return a bunch of > simple values. Also this formula is wrong for sun4i when calling > get_wss() the function return 4 instead of 3. > > Replace this with a simpler switch case. > > Also drop the i2s params which is unused and return a simple int as > returning an error code could be out of range for an s8 and there is > no optim to return a s8 here. > > Fixes: 619c15f7fac9 ("ASoC: sun4i-i2s: Change SR and WSS computation") > Reviewed-by: Chen-Yu Tsai <wens@csie.org> > Signed-off-by: Clément Péron <peron.clem@gmail.com> > --- > sound/soc/sunxi/sun4i-i2s.c | 69 +++++++++++++++++++++++-------------- > 1 file changed, 44 insertions(+), 25 deletions(-) > > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c > index 1f577dbc20a6..8e497fb3de09 100644 > --- a/sound/soc/sunxi/sun4i-i2s.c > +++ b/sound/soc/sunxi/sun4i-i2s.c > @@ -175,8 +175,8 @@ struct sun4i_i2s_quirks { > unsigned int num_mclk_dividers; > > 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 (*get_sr)(unsigned int width); > + int (*get_wss)(unsigned int width); > int (*set_chan_cfg)(const struct sun4i_i2s *i2s, > unsigned int channels, unsigned int slots, > unsigned int slot_width); > @@ -381,37 +381,56 @@ static int sun4i_i2s_set_clk_rate(struct snd_soc_dai *dai, > return 0; > } > > -static s8 sun4i_i2s_get_sr(const struct sun4i_i2s *i2s, int width) > +static int sun4i_i2s_get_sr(unsigned int width) > { > - if (width < 16 || width > 24) > - return -EINVAL; > - > - if (width % 4) > - return -EINVAL; > + switch (width) { > + case 16: > + return 0x0; > + case 20: > + return 0x1; > + case 24: > + return 0x2; > + } > > - return (width - 16) / 4; > + return -EINVAL; > } > > -static s8 sun4i_i2s_get_wss(const struct sun4i_i2s *i2s, int width) > +static int sun4i_i2s_get_wss(unsigned int width) > { > - if (width < 16 || width > 32) > - return -EINVAL; > - > - if (width % 4) > - return -EINVAL; > + switch (width) { > + case 16: > + return 0x0; > + case 20: > + return 0x1; > + case 24: > + return 0x2; > + case 32: > + return 0x3; > + } Like I said in the previous version, I'm not really sure why we need to use the hexadecimal representation here? Maxime
On Sat, Oct 03, 2020 at 04:19:44PM +0200, Clément Péron wrote: > Checkpatch script produces warning: > WARNING: function definition argument 'const struct sun4i_i2s *' > should also have an identifier name. > > Let's fix this by adding identifier name to get_bclk_parent_rate() > and set_fmt() callback definition. > > Signed-off-by: Clément Péron <peron.clem@gmail.com> Acked-by: Maxime Ripard <mripard@kernel.org> 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
Hi Maxime, On Mon, 5 Oct 2020 at 14:14, Maxime Ripard <maxime@cerno.tech> wrote: > > On Sat, Oct 03, 2020 at 04:19:39PM +0200, Clément Péron wrote: > > We are actually using a complex formula to just return a bunch of > > simple values. Also this formula is wrong for sun4i when calling > > get_wss() the function return 4 instead of 3. > > > > Replace this with a simpler switch case. > > > > Also drop the i2s params which is unused and return a simple int as > > returning an error code could be out of range for an s8 and there is > > no optim to return a s8 here. > > > > Fixes: 619c15f7fac9 ("ASoC: sun4i-i2s: Change SR and WSS computation") > > Reviewed-by: Chen-Yu Tsai <wens@csie.org> > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > --- > > sound/soc/sunxi/sun4i-i2s.c | 69 +++++++++++++++++++++++-------------- > > 1 file changed, 44 insertions(+), 25 deletions(-) > > > > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c > > index 1f577dbc20a6..8e497fb3de09 100644 > > --- a/sound/soc/sunxi/sun4i-i2s.c > > +++ b/sound/soc/sunxi/sun4i-i2s.c > > @@ -175,8 +175,8 @@ struct sun4i_i2s_quirks { > > unsigned int num_mclk_dividers; > > > > 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 (*get_sr)(unsigned int width); > > + int (*get_wss)(unsigned int width); > > int (*set_chan_cfg)(const struct sun4i_i2s *i2s, > > unsigned int channels, unsigned int slots, > > unsigned int slot_width); > > @@ -381,37 +381,56 @@ static int sun4i_i2s_set_clk_rate(struct snd_soc_dai *dai, > > return 0; > > } > > > > -static s8 sun4i_i2s_get_sr(const struct sun4i_i2s *i2s, int width) > > +static int sun4i_i2s_get_sr(unsigned int width) > > { > > - if (width < 16 || width > 24) > > - return -EINVAL; > > - > > - if (width % 4) > > - return -EINVAL; > > + switch (width) { > > + case 16: > > + return 0x0; > > + case 20: > > + return 0x1; > > + case 24: > > + return 0x2; > > + } > > > > - return (width - 16) / 4; > > + return -EINVAL; > > } > > > > -static s8 sun4i_i2s_get_wss(const struct sun4i_i2s *i2s, int width) > > +static int sun4i_i2s_get_wss(unsigned int width) > > { > > - if (width < 16 || width > 32) > > - return -EINVAL; > > - > > - if (width % 4) > > - return -EINVAL; > > + switch (width) { > > + case 16: > > + return 0x0; > > + case 20: > > + return 0x1; > > + case 24: > > + return 0x2; > > + case 32: > > + return 0x3; > > + } > > Like I said in the previous version, I'm not really sure why we need to > use the hexadecimal representation here? I'm not sure if there is a convention when to use hexa or when not to use it. But these figures are taken from the User Manual where register descriptions are written in Base 2 and default values are written in Base 16. It's easier to read them and check that the code follows the documentation, no ? Indeed with 2 bits this doesn't change anything. Do you want me to change them in decimal ? Clement > > 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
Hi, To avoid using set-tdm property of the simple-soundcard we will introduce a specific soundcard for Allwinner HDMI later. So I have dropped the simple-soundcard, the title of the serie is no more relevent... Regards, Clement Change since v5: - Drop HDMI simple soundcard - Collect Chen-Yu Tsai tags - Configure channels from 9 to 15. - Remove DMA RX for H3/H5 - Fix Documentation for H3/H5 Change since v4: - add more comment on get_wss() and set_channel_cfg() patch - merge soundcard and DAI HDMI patches Change since v3: - add Samuel Holland patch to reconfigure FIFO_TX_REG when suspend is enabled - readd inversion to H6 LRCK sun50i_h6_i2s_set_soc_fmt() - Fix get_wss() for sun4i - Add a commit to fix checkpatch warning Change since v2: - rebase on next-20200918 - drop revert LRCK polarity patch - readd simple-audio-card,frame-inversion in dts - Add patch for changing set_chan_cfg params Change since v1: - rebase on next-20200828 - add revert LRCK polarity - remove all simple-audio-card,frame-inversion in dts - add Ondrej patches for Orange Pi board - Add arm64 defconfig patch *** BLURB HERE *** Clément Péron (6): ASoC: sun4i-i2s: Change set_chan_cfg() params ASoC: sun4i-i2s: Change get_sr() and get_wss() to be more explicit ASoC: sun4i-i2s: Fix sun8i volatile regs ASoC: sun4i-i2s: fix coding-style for callback definition arm64: defconfig: Enable Allwinner i2s driver dt-bindings: sound: sun4i-i2s: Document H3 with missing RX channel possibility Jernej Skrabec (3): ASoC: sun4i-i2s: Add support for H6 I2S dt-bindings: ASoC: sun4i-i2s: Add H6 compatible arm64: dts: allwinner: h6: Add I2S1 node Marcus Cooper (4): ASoC: sun4i-i2s: Set sign extend sample ASoc: sun4i-i2s: Add 20 and 24 bit support arm64: dts: allwinner: a64: Add I2S2 node arm: dts: sunxi: h3/h5: Add I2S2 node Samuel Holland (1): ASoC: sun4i-i2s: Fix setting of FIFO modes .../sound/allwinner,sun4i-a10-i2s.yaml | 6 +- arch/arm/boot/dts/sunxi-h3-h5.dtsi | 13 + arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 14 + arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 13 + arch/arm64/configs/defconfig | 1 + sound/soc/sunxi/sun4i-i2s.c | 376 +++++++++++++++--- 6 files changed, 368 insertions(+), 55 deletions(-)