Message ID | 20201026185239.379417-1-peron.clem@gmail.com |
---|---|
Headers | show |
Series | Add Allwinner H3/H5/H6/A64 HDMI audio | expand |
On Mon, Oct 26, 2020 at 07:52:26PM +0100, 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 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> > Signed-off-by: Clément Péron <peron.clem@gmail.com> > --- > sound/soc/sunxi/sun4i-i2s.c | 33 ++++++++++++++++++--------------- > 1 file changed, 18 insertions(+), 15 deletions(-) > > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c > index f23ff29e7c1d..6c10f810b114 100644 > --- a/sound/soc/sunxi/sun4i-i2s.c > +++ b/sound/soc/sunxi/sun4i-i2s.c > @@ -162,8 +162,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); > }; > > @@ -399,10 +400,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 +419,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 +448,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: > @@ -480,9 +476,16 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, > { > struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai); > unsigned int word_size = params_width(params); > - unsigned int slot_width = params_physical_width(params); > unsigned int channels = params_channels(params); > + > + /* > + * Here and in set_chan_cfg(), "slots" means channels per frame + > + * padding slots, regardless of format. "slot_width" means bits > + * per sample + padding bits, regardless of format. > + */ > unsigned int slots = channels; > + unsigned int slot_width = params_physical_width(params); > + what I meant was to put that comment next to the function pointer in the structure sun4i_i2s_quirks, it would be fairly easy to miss here. With that fixed, Acked-by: Maxime Ripard <mripard@kernel.org> Maxime
On Mon, Oct 26, 2020 at 07:52:27PM +0100, Clément Péron wrote: > From: Jernej Skrabec <jernej.skrabec@siol.net> > > H6 I2S is very similar to that in H3, except it supports up to 16 > channels. > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > Signed-off-by: Marcus Cooper <codekipper@gmail.com> > Reviewed-by: Chen-Yu Tsai <wens@csie.org> > Signed-off-by: Clément Péron <peron.clem@gmail.com> Acked-by: Maxime Ripard <mripard@kernel.org> Maxime
On Mon, Oct 26, 2020 at 07:52:28PM +0100, 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> Acked-by: Maxime Ripard <mripard@kernel.org> Maxime
On Mon, Oct 26, 2020 at 07:52:29PM +0100, Clément Péron wrote: > From: Marcus Cooper <codekipper@gmail.com> > > On the newer SoCs such as the H3 and A64 this is set by default > to transfer a 0 after each sample in each slot. However the A10 > and A20 SoCs that this driver was developed on had a default > setting where it padded the audio gain with zeros. > > This isn't a problem while we have only support for 16bit audio > but with larger sample resolution rates in the pipeline then SEXT > bits should be cleared so that they also pad at the LSB. Without > this the audio gets distorted. > > Set sign extend sample for all the sunxi generations even if they > are not affected. This will keep consistency and avoid relying on > default. > > Signed-off-by: Marcus Cooper <codekipper@gmail.com> > Reviewed-by: Chen-Yu Tsai <wens@csie.org> > Signed-off-by: Clément Péron <peron.clem@gmail.com> Acked-by: Maxime Ripard <mripard@kernel.org> Maxime
On Mon, Oct 26, 2020 at 07:52:32PM +0100, Clément Péron wrote: > From: Samuel Holland <samuel@sholland.org> > > Because SUN4I_I2S_FIFO_CTRL_REG is volatile, writes done while the > regmap is cache-only are ignored. To work around this, move the > configuration to a callback that runs while the ASoC core has a > runtime PM reference to the device. > > Signed-off-by: Samuel Holland <samuel@sholland.org> > Reviewed-by: Chen-Yu Tsai <wens@csie.org> > Signed-off-by: Clément Péron <peron.clem@gmail.com> Acked-by: Maxime Ripard <mripard@kernel.org> Maxime