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 <[email protected]>
Signed-off-by: Clément Péron <[email protected]>
---
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);
if (ret < 0) {
dev_err(dai->dev, "Invalid channel configuration\n");
return ret;
--
2.25.1
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 <[email protected]>
> Signed-off-by: Cl?ment P?ron <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]>
> > Signed-off-by: Clément Péron <[email protected]>
> > ---
> > 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 <[email protected]>
>> Signed-off-by: Cl?ment P?ron <[email protected]>
>> ---
>> 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 <[email protected]> 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 <[email protected]>
> > > Signed-off-by: Cl?ment P?ron <[email protected]>
> > > ---
> > > 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 <[email protected]>
> >> Signed-off-by: Cl?ment P?ron <[email protected]>
> >> ---
> >> 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 <[email protected]> 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 <[email protected]>
>>>> Signed-off-by: Cl?ment P?ron <[email protected]>
>>>> ---
>>>> 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 <[email protected]> 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 <[email protected]>
> >>>> Signed-off-by: Cl?ment P?ron <[email protected]>
> >>>> ---
> >>>> 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