[PATCH v6 00/14] Add Allwinner H3/H5/H6/A64 HDMI audio
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(-)
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 --- sound/soc/sunxi/sun4i-i2s.c | 226 ++++++++++++++++++++++++++++++++++++ 1 file changed, 226 insertions(+)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index f23ff29e7c1d..c5ccd423e6d3 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -124,6 +124,21 @@ #define SUN8I_I2S_RX_CHAN_SEL_REG 0x54 #define SUN8I_I2S_RX_CHAN_MAP_REG 0x58
+/* Defines required for sun50i-h6 support */ +#define SUN50I_H6_I2S_TX_CHAN_SEL_OFFSET_MASK GENMASK(21, 20) +#define SUN50I_H6_I2S_TX_CHAN_SEL_OFFSET(offset) ((offset) << 20) +#define SUN50I_H6_I2S_TX_CHAN_SEL_MASK GENMASK(19, 16) +#define SUN50I_H6_I2S_TX_CHAN_SEL(chan) ((chan - 1) << 16) +#define SUN50I_H6_I2S_TX_CHAN_EN_MASK GENMASK(15, 0) +#define SUN50I_H6_I2S_TX_CHAN_EN(num_chan) (((1 << num_chan) - 1)) + +#define SUN50I_H6_I2S_TX_CHAN_MAP0_REG 0x44 +#define SUN50I_H6_I2S_TX_CHAN_MAP1_REG 0x48 + +#define SUN50I_H6_I2S_RX_CHAN_SEL_REG 0x64 +#define SUN50I_H6_I2S_RX_CHAN_MAP0_REG 0x68 +#define SUN50I_H6_I2S_RX_CHAN_MAP1_REG 0x6C + struct sun4i_i2s;
/** @@ -474,6 +489,64 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, return 0; }
+static int sun50i_h6_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, + const struct snd_pcm_hw_params *params) +{ + 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); + regmap_write(i2s->regmap, SUN50I_H6_I2S_RX_CHAN_MAP0_REG, 0xFEDCBA98); + regmap_write(i2s->regmap, SUN50I_H6_I2S_RX_CHAN_MAP1_REG, 0x76543210); + + /* Configure the channels */ + regmap_update_bits(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG, + SUN50I_H6_I2S_TX_CHAN_SEL_MASK, + SUN50I_H6_I2S_TX_CHAN_SEL(channels)); + regmap_update_bits(i2s->regmap, SUN50I_H6_I2S_RX_CHAN_SEL_REG, + SUN50I_H6_I2S_TX_CHAN_SEL_MASK, + SUN50I_H6_I2S_TX_CHAN_SEL(channels)); + + regmap_update_bits(i2s->regmap, SUN8I_I2S_CHAN_CFG_REG, + SUN8I_I2S_CHAN_CFG_TX_SLOT_NUM_MASK, + SUN8I_I2S_CHAN_CFG_TX_SLOT_NUM(channels)); + regmap_update_bits(i2s->regmap, SUN8I_I2S_CHAN_CFG_REG, + SUN8I_I2S_CHAN_CFG_RX_SLOT_NUM_MASK, + SUN8I_I2S_CHAN_CFG_RX_SLOT_NUM(channels)); + + switch (i2s->format & SND_SOC_DAIFMT_FORMAT_MASK) { + case SND_SOC_DAIFMT_DSP_A: + 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; + break; + + case SND_SOC_DAIFMT_I2S: + lrck_period = params_physical_width(params); + break; + + default: + return -EINVAL; + } + + regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT0_REG, + SUN8I_I2S_FMT0_LRCK_PERIOD_MASK, + SUN8I_I2S_FMT0_LRCK_PERIOD(lrck_period)); + + regmap_update_bits(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG, + SUN50I_H6_I2S_TX_CHAN_EN_MASK, + SUN50I_H6_I2S_TX_CHAN_EN(channels)); + + return 0; +} + static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params, struct snd_soc_dai *dai) @@ -699,6 +772,108 @@ static int sun8i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, return 0; }
+static int sun50i_h6_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, + unsigned int fmt) +{ + u32 mode, val; + u8 offset; + + /* + * DAI clock polarity + * + * The setup for LRCK contradicts the datasheet, but under a + * scope it's clear that the LRCK polarity is reversed + * compared to the expected polarity on the bus. + */ + switch (fmt & SND_SOC_DAIFMT_INV_MASK) { + case SND_SOC_DAIFMT_IB_IF: + /* Invert both clocks */ + val = SUN8I_I2S_FMT0_BCLK_POLARITY_INVERTED; + break; + case SND_SOC_DAIFMT_IB_NF: + /* Invert bit clock */ + val = SUN8I_I2S_FMT0_BCLK_POLARITY_INVERTED | + SUN8I_I2S_FMT0_LRCLK_POLARITY_INVERTED; + break; + case SND_SOC_DAIFMT_NB_IF: + /* Invert frame clock */ + val = 0; + break; + case SND_SOC_DAIFMT_NB_NF: + val = SUN8I_I2S_FMT0_LRCLK_POLARITY_INVERTED; + break; + default: + return -EINVAL; + } + + regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT0_REG, + SUN8I_I2S_FMT0_LRCLK_POLARITY_MASK | + SUN8I_I2S_FMT0_BCLK_POLARITY_MASK, + val); + + /* DAI Mode */ + switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) { + case SND_SOC_DAIFMT_DSP_A: + mode = SUN8I_I2S_CTRL_MODE_PCM; + offset = 1; + break; + + case SND_SOC_DAIFMT_DSP_B: + mode = SUN8I_I2S_CTRL_MODE_PCM; + offset = 0; + break; + + case SND_SOC_DAIFMT_I2S: + mode = SUN8I_I2S_CTRL_MODE_LEFT; + offset = 1; + break; + + case SND_SOC_DAIFMT_LEFT_J: + mode = SUN8I_I2S_CTRL_MODE_LEFT; + offset = 0; + break; + + case SND_SOC_DAIFMT_RIGHT_J: + mode = SUN8I_I2S_CTRL_MODE_RIGHT; + offset = 0; + break; + + default: + return -EINVAL; + } + + regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG, + SUN8I_I2S_CTRL_MODE_MASK, mode); + regmap_update_bits(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG, + SUN50I_H6_I2S_TX_CHAN_SEL_OFFSET_MASK, + SUN50I_H6_I2S_TX_CHAN_SEL_OFFSET(offset)); + regmap_update_bits(i2s->regmap, SUN50I_H6_I2S_RX_CHAN_SEL_REG, + SUN50I_H6_I2S_TX_CHAN_SEL_OFFSET_MASK, + SUN50I_H6_I2S_TX_CHAN_SEL_OFFSET(offset)); + + /* DAI clock master masks */ + switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) { + case SND_SOC_DAIFMT_CBS_CFS: + /* BCLK and LRCLK master */ + val = SUN8I_I2S_CTRL_BCLK_OUT | SUN8I_I2S_CTRL_LRCK_OUT; + break; + + case SND_SOC_DAIFMT_CBM_CFM: + /* BCLK and LRCLK slave */ + val = 0; + break; + + default: + return -EINVAL; + } + + regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG, + SUN8I_I2S_CTRL_BCLK_OUT | SUN8I_I2S_CTRL_LRCK_OUT, + val); + + return 0; +} + static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) { struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai); @@ -979,6 +1154,22 @@ static const struct reg_default sun8i_i2s_reg_defaults[] = { { SUN8I_I2S_RX_CHAN_MAP_REG, 0x00000000 }, };
+static const struct reg_default sun50i_h6_i2s_reg_defaults[] = { + { SUN4I_I2S_CTRL_REG, 0x00060000 }, + { SUN4I_I2S_FMT0_REG, 0x00000033 }, + { SUN4I_I2S_FMT1_REG, 0x00000030 }, + { SUN4I_I2S_FIFO_CTRL_REG, 0x000400f0 }, + { SUN4I_I2S_DMA_INT_CTRL_REG, 0x00000000 }, + { SUN4I_I2S_CLK_DIV_REG, 0x00000000 }, + { SUN8I_I2S_CHAN_CFG_REG, 0x00000000 }, + { SUN8I_I2S_TX_CHAN_SEL_REG, 0x00000000 }, + { SUN50I_H6_I2S_TX_CHAN_MAP0_REG, 0x00000000 }, + { SUN50I_H6_I2S_TX_CHAN_MAP1_REG, 0x00000000 }, + { SUN50I_H6_I2S_RX_CHAN_SEL_REG, 0x00000000 }, + { SUN50I_H6_I2S_RX_CHAN_MAP0_REG, 0x00000000 }, + { SUN50I_H6_I2S_RX_CHAN_MAP1_REG, 0x00000000 }, +}; + static const struct regmap_config sun4i_i2s_regmap_config = { .reg_bits = 32, .reg_stride = 4, @@ -1006,6 +1197,19 @@ static const struct regmap_config sun8i_i2s_regmap_config = { .volatile_reg = sun8i_i2s_volatile_reg, };
+static const struct regmap_config sun50i_h6_i2s_regmap_config = { + .reg_bits = 32, + .reg_stride = 4, + .val_bits = 32, + .max_register = SUN50I_H6_I2S_RX_CHAN_MAP1_REG, + .cache_type = REGCACHE_FLAT, + .reg_defaults = sun50i_h6_i2s_reg_defaults, + .num_reg_defaults = ARRAY_SIZE(sun50i_h6_i2s_reg_defaults), + .writeable_reg = sun4i_i2s_wr_reg, + .readable_reg = sun8i_i2s_rd_reg, + .volatile_reg = sun8i_i2s_volatile_reg, +}; + static int sun4i_i2s_runtime_resume(struct device *dev) { struct sun4i_i2s *i2s = dev_get_drvdata(dev); @@ -1164,6 +1368,24 @@ static const struct sun4i_i2s_quirks sun50i_a64_codec_i2s_quirks = { .set_fmt = sun4i_i2s_set_soc_fmt, };
+static const struct sun4i_i2s_quirks sun50i_h6_i2s_quirks = { + .has_reset = true, + .reg_offset_txdata = SUN8I_I2S_FIFO_TX_REG, + .sun4i_i2s_regmap = &sun50i_h6_i2s_regmap_config, + .field_clkdiv_mclk_en = REG_FIELD(SUN4I_I2S_CLK_DIV_REG, 8, 8), + .field_fmt_wss = REG_FIELD(SUN4I_I2S_FMT0_REG, 0, 2), + .field_fmt_sr = REG_FIELD(SUN4I_I2S_FMT0_REG, 4, 6), + .bclk_dividers = sun8i_i2s_clk_div, + .num_bclk_dividers = ARRAY_SIZE(sun8i_i2s_clk_div), + .mclk_dividers = sun8i_i2s_clk_div, + .num_mclk_dividers = ARRAY_SIZE(sun8i_i2s_clk_div), + .get_bclk_parent_rate = sun8i_i2s_get_bclk_parent_rate, + .get_sr = sun8i_i2s_get_sr_wss, + .get_wss = sun8i_i2s_get_sr_wss, + .set_chan_cfg = sun50i_h6_i2s_set_chan_cfg, + .set_fmt = sun50i_h6_i2s_set_soc_fmt, +}; + static int sun4i_i2s_init_regmap_fields(struct device *dev, struct sun4i_i2s *i2s) { @@ -1333,6 +1555,10 @@ static const struct of_device_id sun4i_i2s_match[] = { .compatible = "allwinner,sun50i-a64-codec-i2s", .data = &sun50i_a64_codec_i2s_quirks, }, + { + .compatible = "allwinner,sun50i-h6-i2s", + .data = &sun50i_h6_i2s_quirks, + }, {} }; MODULE_DEVICE_TABLE(of, sun4i_i2s_match);
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); if (ret < 0) { dev_err(dai->dev, "Invalid channel configuration\n"); return ret;
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);};
@@ -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 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,
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 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
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);};
@@ -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
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);};
@@ -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
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; + }
- return (width - 16) / 4; + return -EINVAL; }
-static s8 sun8i_i2s_get_sr_wss(const struct sun4i_i2s *i2s, int width) +static int sun8i_i2s_get_sr_wss(unsigned int width) { - if (width % 4) - return -EINVAL; - - if (width < 8 || width > 32) - return -EINVAL; + switch (width) { + case 8: + return 0x1; + case 12: + return 0x2; + case 16: + return 0x3; + case 20: + return 0x4; + case 24: + return 0x5; + case 28: + return 0x6; + case 32: + return 0x7; + }
- return (width - 8) / 4 + 1; + return -EINVAL; }
static int sun4i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, @@ -574,11 +593,11 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, } i2s->playback_dma_data.addr_width = width;
- sr = i2s->variant->get_sr(i2s, word_size); + sr = i2s->variant->get_sr(word_size); if (sr < 0) return -EINVAL;
- wss = i2s->variant->get_wss(i2s, slot_width); + wss = i2s->variant->get_wss(slot_width); if (wss < 0) return -EINVAL;
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
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
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 --- sound/soc/sunxi/sun4i-i2s.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index 8e497fb3de09..73103673643a 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -48,6 +48,9 @@ #define SUN4I_I2S_FMT0_FMT_I2S (0 << 0)
#define SUN4I_I2S_FMT1_REG 0x08 +#define SUN4I_I2S_FMT1_REG_SEXT_MASK BIT(8) +#define SUN4I_I2S_FMT1_REG_SEXT(sext) ((sext) << 8) + #define SUN4I_I2S_FIFO_TX_REG 0x0c #define SUN4I_I2S_FIFO_RX_REG 0x10
@@ -105,6 +108,9 @@ #define SUN8I_I2S_FMT0_BCLK_POLARITY_INVERTED (1 << 7) #define SUN8I_I2S_FMT0_BCLK_POLARITY_NORMAL (0 << 7)
+#define SUN8I_I2S_FMT1_REG_SEXT_MASK GENMASK(5, 4) +#define SUN8I_I2S_FMT1_REG_SEXT(sext) ((sext) << 4) + #define SUN8I_I2S_INT_STA_REG 0x0c #define SUN8I_I2S_FIFO_TX_REG 0x20
@@ -678,6 +684,7 @@ static int sun4i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, } regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG, SUN4I_I2S_CTRL_MODE_MASK, val); + return 0; }
@@ -780,6 +787,11 @@ static int sun8i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, SUN8I_I2S_CTRL_BCLK_OUT | SUN8I_I2S_CTRL_LRCK_OUT, val);
+ /* Set sign extension to pad out LSB with 0 */ + regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT1_REG, + SUN8I_I2S_FMT1_REG_SEXT_MASK, + SUN8I_I2S_FMT1_REG_SEXT(0)); + return 0; }
@@ -882,6 +894,11 @@ static int sun50i_h6_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, SUN8I_I2S_CTRL_BCLK_OUT | SUN8I_I2S_CTRL_LRCK_OUT, val);
+ /* Set sign extension to pad out LSB with 0 */ + regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT1_REG, + SUN8I_I2S_FMT1_REG_SEXT_MASK, + SUN8I_I2S_FMT1_REG_SEXT(0)); + return 0; }
From: Marcus Cooper codekipper@gmail.com
Extend the functionality of the driver to include support of 20 and 24 bits per sample.
Signed-off-by: Marcus Cooper codekipper@gmail.com Acked-by: Maxime Ripard mripard@kernel.org 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 | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index 73103673643a..ba7514849b73 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -592,6 +592,9 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, case 16: width = DMA_SLAVE_BUSWIDTH_2_BYTES; break; + case 32: + width = DMA_SLAVE_BUSWIDTH_4_BYTES; + break; default: dev_err(dai->dev, "Unsupported physical sample width: %d\n", params_physical_width(params)); @@ -1073,6 +1076,10 @@ static int sun4i_i2s_dai_probe(struct snd_soc_dai *dai) return 0; }
+#define SUN4I_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | \ + SNDRV_PCM_FMTBIT_S20_LE | \ + SNDRV_PCM_FMTBIT_S24_LE) + static struct snd_soc_dai_driver sun4i_i2s_dai = { .probe = sun4i_i2s_dai_probe, .capture = { @@ -1080,14 +1087,14 @@ static struct snd_soc_dai_driver sun4i_i2s_dai = { .channels_min = 1, .channels_max = 8, .rates = SNDRV_PCM_RATE_8000_192000, - .formats = SNDRV_PCM_FMTBIT_S16_LE, + .formats = SUN4I_FORMATS, }, .playback = { .stream_name = "Playback", .channels_min = 1, .channels_max = 8, .rates = SNDRV_PCM_RATE_8000_192000, - .formats = SNDRV_PCM_FMTBIT_S16_LE, + .formats = SUN4I_FORMATS, }, .ops = &sun4i_i2s_dai_ops, .symmetric_rates = 1,
The FIFO TX reg is volatile and sun8i i2s register mapping is different from sun4i.
Even if in this case it's doesn't create an issue, Avoid setting some regs that are undefined in sun8i.
Acked-by: Maxime Ripard mripard@kernel.org 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 | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index ba7514849b73..92671eb94db9 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -1154,12 +1154,19 @@ static bool sun8i_i2s_rd_reg(struct device *dev, unsigned int reg)
static bool sun8i_i2s_volatile_reg(struct device *dev, unsigned int reg) { - if (reg == SUN8I_I2S_INT_STA_REG) + switch (reg) { + case SUN4I_I2S_FIFO_CTRL_REG: + case SUN4I_I2S_FIFO_RX_REG: + case SUN4I_I2S_FIFO_STA_REG: + case SUN4I_I2S_RX_CNT_REG: + case SUN4I_I2S_TX_CNT_REG: + case SUN8I_I2S_FIFO_TX_REG: + case SUN8I_I2S_INT_STA_REG: return true; - if (reg == SUN8I_I2S_FIFO_TX_REG) - return false;
- return sun4i_i2s_volatile_reg(dev, reg); + default: + return false; + } }
static const struct reg_default sun4i_i2s_reg_defaults[] = {
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 --- sound/soc/sunxi/sun4i-i2s.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index 92671eb94db9..fef68146d648 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -588,6 +588,13 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, return ret; }
+ /* Set significant bits in our FIFOs */ + regmap_update_bits(i2s->regmap, SUN4I_I2S_FIFO_CTRL_REG, + SUN4I_I2S_FIFO_CTRL_TX_MODE_MASK | + SUN4I_I2S_FIFO_CTRL_RX_MODE_MASK, + SUN4I_I2S_FIFO_CTRL_TX_MODE(1) | + SUN4I_I2S_FIFO_CTRL_RX_MODE(1)); + switch (params_physical_width(params)) { case 16: width = DMA_SLAVE_BUSWIDTH_2_BYTES; @@ -916,13 +923,6 @@ static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) return ret; }
- /* Set significant bits in our FIFOs */ - regmap_update_bits(i2s->regmap, SUN4I_I2S_FIFO_CTRL_REG, - SUN4I_I2S_FIFO_CTRL_TX_MODE_MASK | - SUN4I_I2S_FIFO_CTRL_RX_MODE_MASK, - SUN4I_I2S_FIFO_CTRL_TX_MODE(1) | - SUN4I_I2S_FIFO_CTRL_RX_MODE(1)); - i2s->format = fmt;
return 0;
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 --- sound/soc/sunxi/sun4i-i2s.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index fef68146d648..86266879d4bc 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -180,13 +180,13 @@ struct sun4i_i2s_quirks { const struct sun4i_i2s_clk_div *mclk_dividers; unsigned int num_mclk_dividers;
- unsigned long (*get_bclk_parent_rate)(const struct sun4i_i2s *); + unsigned long (*get_bclk_parent_rate)(const struct sun4i_i2s *i2s); 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); - int (*set_fmt)(const struct sun4i_i2s *, unsigned int); + int (*set_fmt)(const struct sun4i_i2s *i2s, unsigned int fmt); };
struct sun4i_i2s {
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
From: Jernej Skrabec jernej.skrabec@siol.net
H6 I2S is very similar to H3, except that it supports up to 16 channels and thus few registers have fields on different position.
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net Signed-off-by: Marcus Cooper codekipper@gmail.com Acked-by: Maxime Ripard mripard@kernel.org Acked-by: Rob Herring robh@kernel.org Acked-by: Chen-Yu Tsai wens@csie.org Signed-off-by: Clément Péron peron.clem@gmail.com --- .../devicetree/bindings/sound/allwinner,sun4i-a10-i2s.yaml | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/sound/allwinner,sun4i-a10-i2s.yaml b/Documentation/devicetree/bindings/sound/allwinner,sun4i-a10-i2s.yaml index 112ae00d63c1..606ad2d884a8 100644 --- a/Documentation/devicetree/bindings/sound/allwinner,sun4i-a10-i2s.yaml +++ b/Documentation/devicetree/bindings/sound/allwinner,sun4i-a10-i2s.yaml @@ -24,6 +24,7 @@ properties: - items: - const: allwinner,sun50i-a64-i2s - const: allwinner,sun8i-h3-i2s + - const: allwinner,sun50i-h6-i2s
reg: maxItems: 1 @@ -59,6 +60,7 @@ allOf: - allwinner,sun8i-a83t-i2s - allwinner,sun8i-h3-i2s - allwinner,sun50i-a64-codec-i2s + - allwinner,sun50i-h6-i2s
then: required:
From: Jernej Skrabec jernej.skrabec@siol.net
Add Allwinner H6 I2S1 node connected to HDMI interface.
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net Signed-off-by: Marcus Cooper codekipper@gmail.com Acked-by: Chen-Yu Tsai wens@csie.org Signed-off-by: Clément Péron peron.clem@gmail.com --- arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi index 28c77d6872f6..d915aeb13297 100644 --- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi @@ -609,6 +609,19 @@ mdio: mdio { }; };
+ i2s1: i2s@5091000 { + #sound-dai-cells = <0>; + compatible = "allwinner,sun50i-h6-i2s"; + reg = <0x05091000 0x1000>; + interrupts = <GIC_SPI 19 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&ccu CLK_BUS_I2S1>, <&ccu CLK_I2S1>; + clock-names = "apb", "mod"; + dmas = <&dma 4>, <&dma 4>; + resets = <&ccu RST_BUS_I2S1>; + dma-names = "rx", "tx"; + status = "disabled"; + }; + spdif: spdif@5093000 { #sound-dai-cells = <0>; compatible = "allwinner,sun50i-h6-spdif";
From: Marcus Cooper codekipper@gmail.com
Add the I2S2 node connected to the HDMI interface.
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net Signed-off-by: Marcus Cooper codekipper@gmail.com Acked-by: Chen-Yu Tsai wens@csie.org Signed-off-by: Clément Péron peron.clem@gmail.com --- arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi index dc238814013c..51cc30e84e26 100644 --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi @@ -846,6 +846,20 @@ i2s1: i2s@1c22400 { status = "disabled"; };
+ i2s2: i2s@1c22800 { + #sound-dai-cells = <0>; + compatible = "allwinner,sun50i-a64-i2s", + "allwinner,sun8i-h3-i2s"; + reg = <0x01c22800 0x400>; + interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&ccu CLK_BUS_I2S2>, <&ccu CLK_I2S2>; + clock-names = "apb", "mod"; + resets = <&ccu RST_BUS_I2S2>; + dma-names = "rx", "tx"; + dmas = <&dma 27>, <&dma 27>; + status = "disabled"; + }; + dai: dai@1c22c00 { #sound-dai-cells = <0>; compatible = "allwinner,sun50i-a64-codec-i2s";
Enable Allwinner I2S driver for arm64 defconfig.
Signed-off-by: Clément Péron peron.clem@gmail.com --- arch/arm64/configs/defconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig index 63003ec116ee..9a3c3bbe60e4 100644 --- a/arch/arm64/configs/defconfig +++ b/arch/arm64/configs/defconfig @@ -696,6 +696,7 @@ CONFIG_SND_SOC_ROCKCHIP_RT5645=m CONFIG_SND_SOC_RK3399_GRU_SOUND=m CONFIG_SND_SOC_SAMSUNG=y CONFIG_SND_SOC_RCAR=m +CONFIG_SND_SUN4I_I2S=m CONFIG_SND_SUN4I_SPDIF=m CONFIG_SND_SOC_TEGRA=m CONFIG_SND_SOC_TEGRA210_AHUB=m
Like A83T the Allwinner H3 doesn't have the DMA reception available for some audio interfaces.
As it's already documented for A83T convert this to an enum and add the H3 interface.
Signed-off-by: Clément Péron peron.clem@gmail.com --- .../devicetree/bindings/sound/allwinner,sun4i-a10-i2s.yaml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/sound/allwinner,sun4i-a10-i2s.yaml b/Documentation/devicetree/bindings/sound/allwinner,sun4i-a10-i2s.yaml index 606ad2d884a8..a16e37b01e1d 100644 --- a/Documentation/devicetree/bindings/sound/allwinner,sun4i-a10-i2s.yaml +++ b/Documentation/devicetree/bindings/sound/allwinner,sun4i-a10-i2s.yaml @@ -70,7 +70,9 @@ allOf: properties: compatible: contains: - const: allwinner,sun8i-a83t-i2s + enum: + - allwinner,sun8i-a83t-i2s + - allwinner,sun8i-h3-i2s
then: properties:
On Sat, 03 Oct 2020 16:19:49 +0200, Clément Péron wrote:
Like A83T the Allwinner H3 doesn't have the DMA reception available for some audio interfaces.
As it's already documented for A83T convert this to an enum and add the H3 interface.
Signed-off-by: Clément Péron peron.clem@gmail.com
.../devicetree/bindings/sound/allwinner,sun4i-a10-i2s.yaml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
Acked-by: Rob Herring robh@kernel.org
From: Marcus Cooper codekipper@gmail.com
Add H3/H5 I2S2 node connected to the HDMI interface.
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net Signed-off-by: Marcus Cooper codekipper@gmail.com Acked-by: Chen-Yu Tsai wens@csie.org Signed-off-by: Clément Péron peron.clem@gmail.com --- arch/arm/boot/dts/sunxi-h3-h5.dtsi | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/arch/arm/boot/dts/sunxi-h3-h5.dtsi b/arch/arm/boot/dts/sunxi-h3-h5.dtsi index 22d533d18992..9be13378d4df 100644 --- a/arch/arm/boot/dts/sunxi-h3-h5.dtsi +++ b/arch/arm/boot/dts/sunxi-h3-h5.dtsi @@ -662,6 +662,19 @@ i2s1: i2s@1c22400 { status = "disabled"; };
+ i2s2: i2s@1c22800 { + #sound-dai-cells = <0>; + compatible = "allwinner,sun8i-h3-i2s"; + reg = <0x01c22800 0x400>; + interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&ccu CLK_BUS_I2S2>, <&ccu CLK_I2S2>; + clock-names = "apb", "mod"; + dmas = <&dma 27>; + resets = <&ccu RST_BUS_I2S2>; + dma-names = "tx"; + status = "disabled"; + }; + codec: codec@1c22c00 { #sound-dai-cells = <0>; compatible = "allwinner,sun8i-h3-codec";
participants (4)
-
Clément Péron -
Maxime Ripard -
Rob Herring -
Samuel Holland