diff mbox series

[v9,01/14] ASoC: sun4i-i2s: Change set_chan_cfg() params

Message ID 20201027183149.145165-2-peron.clem@gmail.com (mailing list archive)
State Superseded
Headers show
Series Add Allwinner H3/H5/H6/A64 HDMI audio | expand

Commit Message

Clément Péron Oct. 27, 2020, 6:31 p.m. UTC 
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 set or not.

This is not done actually and will trigger a bug.
For example, if we set to the simple soundcard in the device-tree
dai-tdm-slot-width = <32> and then 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.

To fix this, we need to check if these values are set or not but as
this logic is already done by the caller. Avoid duplicating this
logic and just pass the required values as params to set_chan_cfg().

Suggested-by: Samuel Holland <samuel@sholland.org>
Acked-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
 sound/soc/sunxi/sun4i-i2s.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

Comments

Pierre-Louis Bossart Oct. 27, 2020, 6:58 p.m. UTC | #1 
> @@ -452,11 +454,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;

Aren't I2S, LEFT_J and RIGHT_J pretty much the same in terms of lrclk 
rate/period? the only thing that can change is the polarity, no?

Not sure why it's handled differently here?
Clément Péron Oct. 27, 2020, 9:43 p.m. UTC | #2 
Hi Pierre-Louis,

On Tue, 27 Oct 2020 at 19:59, Pierre-Louis Bossart
<pierre-louis.bossart@linux.intel.com> wrote:
>
>
> > @@ -452,11 +454,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;
>
> Aren't I2S, LEFT_J and RIGHT_J pretty much the same in terms of lrclk
> rate/period? the only thing that can change is the polarity, no?
>
> Not sure why it's handled differently here?

I just had a look at the User Manual for H3 and H6 and I didn't find
any reason why LEFT_J and RIGHT_J should be computed in a different
way as I2S.

Also the commit introducing this doesn't mention it.
7ae7834ec446 ("ASoC: sun4i-i2s: Add support for DSP formats")

I can't test it with my board but if nobody complains about it, I will
introduce a fix for this in the next version and change this also for
H6.

Thanks for your review,
Clement
Samuel Holland Oct. 30, 2020, 1:20 a.m. UTC | #3 
On 10/27/20 4:43 PM, Clément Péron wrote:
> Hi Pierre-Louis,
> 
> On Tue, 27 Oct 2020 at 19:59, Pierre-Louis Bossart
> <pierre-louis.bossart@linux.intel.com> wrote:
>>
>>
>>> @@ -452,11 +454,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;
>>
>> Aren't I2S, LEFT_J and RIGHT_J pretty much the same in terms of lrclk
>> rate/period? the only thing that can change is the polarity, no?
>>
>> Not sure why it's handled differently here?
> 
> I just had a look at the User Manual for H3 and H6 and I didn't find
> any reason why LEFT_J and RIGHT_J should be computed in a different
> way as I2S.

Yes, and the manual explicitly groups LEFT_J and RIGHT_J with I2S when
describing this field:

   PCM Mode: Number of BCLKs within (Left + Right) channel width.
   I2S/Left-Justified/Right-Justified Mode: Number of BCLKs within each
individual channel width(Left or Right)

So I agree that the code is wrong here.

Regards,
Samuel

> Also the commit introducing this doesn't mention it.
> 7ae7834ec446 ("ASoC: sun4i-i2s: Add support for DSP formats")
> 
> I can't test it with my board but if nobody complains about it, I will
> introduce a fix for this in the next version and change this also for
> H6.
> 
> Thanks for your review,
> Clement
>
Clément Péron Oct. 30, 2020, 12:58 p.m. UTC | #4 
Hi Samuel

On Fri, 30 Oct 2020 at 02:20, Samuel Holland <samuel@sholland.org> wrote:
>
> On 10/27/20 4:43 PM, Clément Péron wrote:
> > Hi Pierre-Louis,
> >
> > On Tue, 27 Oct 2020 at 19:59, Pierre-Louis Bossart
> > <pierre-louis.bossart@linux.intel.com> wrote:
> >>
> >>
> >>> @@ -452,11 +454,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;
> >>
> >> Aren't I2S, LEFT_J and RIGHT_J pretty much the same in terms of lrclk
> >> rate/period? the only thing that can change is the polarity, no?
> >>
> >> Not sure why it's handled differently here?
> >
> > I just had a look at the User Manual for H3 and H6 and I didn't find
> > any reason why LEFT_J and RIGHT_J should be computed in a different
> > way as I2S.
>
> Yes, and the manual explicitly groups LEFT_J and RIGHT_J with I2S when
> describing this field:
>
>    PCM Mode: Number of BCLKs within (Left + Right) channel width.
>    I2S/Left-Justified/Right-Justified Mode: Number of BCLKs within each
> individual channel width(Left or Right)
>
> So I agree that the code is wrong here.

Thanks, I will send a fix in the v10.

Regards,
Clement

>
> Regards,
> Samuel
>
> > Also the commit introducing this doesn't mention it.
> > 7ae7834ec446 ("ASoC: sun4i-i2s: Add support for DSP formats")
> >
> > I can't test it with my board but if nobody complains about it, I will
> > introduce a fix for this in the next version and change this also for
> > H6.
> >
> > Thanks for your review,
> > Clement
> >
>
diff mbox series

Patch

diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
index f23ff29e7c1d..7c1f57eb2462 100644
--- a/sound/soc/sunxi/sun4i-i2s.c
+++ b/sound/soc/sunxi/sun4i-i2s.c
@@ -162,8 +162,15 @@  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 *);
+
+	/*
+	 * In the set_chan_cfg() function pointer:
+	 * @slots: channels per frame + padding slots, regardless of format
+	 * @slot_width: bits per sample + padding bits, regardless of format
+	 */
+	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);
 };
 
@@ -399,10 +406,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);
@@ -419,15 +425,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);
@@ -452,11 +454,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:
@@ -482,7 +484,9 @@  static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
 	unsigned int word_size = params_width(params);
 	unsigned int slot_width = params_physical_width(params);
 	unsigned int channels = params_channels(params);
+
 	unsigned int slots = channels;
+
 	int ret, sr, wss;
 	u32 width;
 
@@ -492,7 +496,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;