| Message ID | 20201027183149.145165-1-peron.clem@gmail.com |
|---|---|
| Headers | show |
| Series | Add Allwinner H3/H5/H6/A64 HDMI audio | expand |
> @@ -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?
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
On Wed, Oct 28, 2020 at 2:32 AM Clément Péron <peron.clem@gmail.com> wrote: > > Enable Allwinner I2S driver for arm64 defconfig. > > Signed-off-by: Clément Péron <peron.clem@gmail.com> Acked-by: Chen-Yu Tsai <wens@csie.org>
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 >
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 > > >
Hi, This series add H6 I2S support and the I2S node missing to support HDMI audio in different Allwinner SoC. As we first use some TDM property to make the I2S working we the simple soundcard. We have now drop this simple sound card and a proper dedicated soundcard will be introduce later. This make the title of this series wrong now :/. Regards, Clement Change since v8: - move the comment near the function prototype - collect Maxime Ripard tags Change since v7: - rebase on next-20201026 - comment about slots and slot_width Change since v6: - move set_channel_cfg() in first position - convert return value to decimal 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 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 | 384 +++++++++++++++--- 6 files changed, 376 insertions(+), 55 deletions(-)