>From: Alsa-devel [mailto:alsa-devel-bounces@xxxxxxxxxxxxxxxx] On Behalf Of >Maxime Ripard >Sent: Wednesday, June 5, 2019 12:08 PM >To: Mark Brown <broonie@xxxxxxxxxx>; Liam Girdwood ><lgirdwood@xxxxxxxxx>; Mark Rutland <mark.rutland@xxxxxxx>; Rob >Herring <robh+dt@xxxxxxxxxx>; Frank Rowand <frowand.list@xxxxxxxxx> >Cc: devicetree@xxxxxxxxxxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx; Maxime Ripard ><maxime.ripard@xxxxxxxxxxx>; Marcus Cooper <codekipper@xxxxxxxxx>; >Chen-Yu Tsai <wens@xxxxxxxx>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx >Subject: [alsa-devel] [PATCH v2] ASoC: sun4i-i2s: Change SR and WSS >computation > >The current computation for the SR (sample resolution) and the WSS (word >slot size) register parameters is based on a switch returning the matching >parameters for a given params width. > >Later SoCs (A83t, H3, A64) changed that calculation, which was loosely the >same with an offset. Therefore, an offset was added to adjust those >parameters. > >However, the calculation is a bit less trivial than initially thought. >Indeed, while we assumed that SR and WSS were always the same, on older >SoCs, SR will max at 24 (since those SoCs do not support 32 bits formats), >but the word size can be 32. > >Newer SoCs can also support a much larger range (8 bits to 32 bits, by >increments of 4) of size than the older SoCs could. > >Finally, the A64 and A83t were never adjusted to have that offset in the >first place, and were therefore broken from that point of view. > >In order to fix all those issues, let's introduce two functions, get_wss >and get_sr, with their respective implementations for all the SoCs >supported so far. > >Fixes: 21faaea1343f ("ASoC: sun4i-i2s: Add support for A83T") >Fixes: 66ecce332538 ("ASoC: sun4i-i2s: Add compatibility with A64 codec I2S") >Signed-off-by: Maxime Ripard <maxime.ripard@xxxxxxxxxxx> > >--- > >Changes from v1: > - Declare the structure sun4i_i2s to fix compilation errors >--- > sound/soc/sunxi/sun4i-i2s.c | 71 ++++++++++++++++++++++++++++--------- > 1 file changed, 55 insertions(+), 16 deletions(-) > >diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c >index c53bfed8d4c2..78d44dbc6373 100644 >--- a/sound/soc/sunxi/sun4i-i2s.c >+++ b/sound/soc/sunxi/sun4i-i2s.c >@@ -114,6 +114,8 @@ > #define SUN8I_I2S_RX_CHAN_SEL_REG 0x54 > #define SUN8I_I2S_RX_CHAN_MAP_REG 0x58 > >+struct sun4i_i2s; >+ > /** > * struct sun4i_i2s_quirks - Differences between SoC variants. > * >@@ -127,7 +129,6 @@ > * @sun4i_i2s_regmap: regmap config to use. > * @mclk_offset: Value by which mclkdiv needs to be adjusted. > * @bclk_offset: Value by which bclkdiv needs to be adjusted. >- * @fmt_offset: Value by which wss and sr needs to be adjusted. > * @field_clkdiv_mclk_en: regmap field to enable mclk output. > * @field_fmt_wss: regmap field to set word select size. > * @field_fmt_sr: regmap field to set sample resolution. >@@ -150,7 +151,6 @@ struct sun4i_i2s_quirks { > const struct regmap_config *sun4i_i2s_regmap; > unsigned int mclk_offset; > unsigned int bclk_offset; >- unsigned int fmt_offset; > > /* Register fields for i2s */ > struct reg_field field_clkdiv_mclk_en; >@@ -163,6 +163,9 @@ struct sun4i_i2s_quirks { > struct reg_field field_rxchanmap; > struct reg_field field_txchansel; > struct reg_field field_rxchansel; >+ >+ s8 (*get_sr)(const struct sun4i_i2s *, int); >+ s8 (*get_wss)(const struct sun4i_i2s *, int); > }; > > struct sun4i_i2s { >@@ -345,6 +348,39 @@ 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) >+{ >+ if (width < 16 || width > 24) >+ return -EINVAL; >+ >+ if (width % 4) >+ return -EINVAL; >+ >+ return (width - 16) / 4; >+} >+ >+static s8 sun4i_i2s_get_wss(const struct sun4i_i2s *i2s, int width) >+{ >+ if (width < 16 || width > 32) >+ return -EINVAL; >+ >+ if (width % 4) >+ return -EINVAL; >+ >+ return (width - 16) / 4; >+} >+ >+static s8 sun8i_i2s_get_sr_wss(const struct sun4i_i2s *i2s, int width) >+{ >+ if (width % 4) >+ return -EINVAL; >+ In the two above you start with boundary check before mod yet in this one the order is reversed. Keeping the same order should prove more cohesive. >+ if (width < 8 || width > 32) >+ return -EINVAL; >+ >+ return (width - 8) / 4 + 1; >+} >+ Other, probably less welcome suggestion is introduction of unified function which ones listed here would simply invoke. All of these "computations" differ in fact only in: min and max boundary. The +1 for _sr_wss is negligible, you can append it on return. > static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, > struct snd_pcm_hw_params *params, > struct snd_soc_dai *dai) >@@ -396,22 +432,16 @@ static int sun4i_i2s_hw_params(struct >snd_pcm_substream *substream, > } > i2s->playback_dma_data.addr_width = width; > >- switch (params_width(params)) { >- case 16: >- sr = 0; >- wss = 0; >- break; >+ sr = i2s->variant->get_sr(i2s, params_width(params)); >+ if (sr < 0) >+ return -EINVAL; > >- default: >- dev_err(dai->dev, "Unsupported sample width: %d\n", >- params_width(params)); >+ wss = i2s->variant->get_wss(i2s, params_width(params)); >+ if (wss < 0) > return -EINVAL; >- } > >- regmap_field_write(i2s->field_fmt_wss, >- wss + i2s->variant->fmt_offset); >- regmap_field_write(i2s->field_fmt_sr, >- sr + i2s->variant->fmt_offset); >+ regmap_field_write(i2s->field_fmt_wss, wss); >+ regmap_field_write(i2s->field_fmt_sr, sr); > > return sun4i_i2s_set_clk_rate(dai, params_rate(params), > params_width(params)); >@@ -887,6 +917,8 @@ static const struct sun4i_i2s_quirks >sun4i_a10_i2s_quirks = { > .field_rxchanmap = >REG_FIELD(SUN4I_I2S_RX_CHAN_MAP_REG, 0, 31), > .field_txchansel = REG_FIELD(SUN4I_I2S_TX_CHAN_SEL_REG, >0, 2), > .field_rxchansel = REG_FIELD(SUN4I_I2S_RX_CHAN_SEL_REG, >0, 2), >+ .get_sr = sun4i_i2s_get_sr, >+ .get_wss = sun4i_i2s_get_wss, > }; > > static const struct sun4i_i2s_quirks sun6i_a31_i2s_quirks = { >@@ -904,6 +936,8 @@ static const struct sun4i_i2s_quirks >sun6i_a31_i2s_quirks = { > .field_rxchanmap = >REG_FIELD(SUN4I_I2S_RX_CHAN_MAP_REG, 0, 31), > .field_txchansel = REG_FIELD(SUN4I_I2S_TX_CHAN_SEL_REG, >0, 2), > .field_rxchansel = REG_FIELD(SUN4I_I2S_RX_CHAN_SEL_REG, >0, 2), >+ .get_sr = sun4i_i2s_get_sr, >+ .get_wss = sun4i_i2s_get_wss, > }; > > static const struct sun4i_i2s_quirks sun8i_a83t_i2s_quirks = { >@@ -921,6 +955,8 @@ static const struct sun4i_i2s_quirks >sun8i_a83t_i2s_quirks = { > .field_rxchanmap = >REG_FIELD(SUN4I_I2S_RX_CHAN_MAP_REG, 0, 31), > .field_txchansel = REG_FIELD(SUN4I_I2S_TX_CHAN_SEL_REG, >0, 2), > .field_rxchansel = REG_FIELD(SUN4I_I2S_RX_CHAN_SEL_REG, >0, 2), >+ .get_sr = sun8i_i2s_get_sr_wss, >+ .get_wss = sun8i_i2s_get_sr_wss, > }; > > static const struct sun4i_i2s_quirks sun8i_h3_i2s_quirks = { >@@ -929,7 +965,6 @@ static const struct sun4i_i2s_quirks >sun8i_h3_i2s_quirks = { > .sun4i_i2s_regmap = &sun8i_i2s_regmap_config, > .mclk_offset = 1, > .bclk_offset = 2, >- .fmt_offset = 3, > .has_fmt_set_lrck_period = true, > .has_chcfg = true, > .has_chsel_tx_chen = true, >@@ -944,6 +979,8 @@ static const struct sun4i_i2s_quirks >sun8i_h3_i2s_quirks = { > .field_rxchanmap = >REG_FIELD(SUN8I_I2S_RX_CHAN_MAP_REG, 0, 31), > .field_txchansel = REG_FIELD(SUN8I_I2S_TX_CHAN_SEL_REG, >0, 2), > .field_rxchansel = REG_FIELD(SUN8I_I2S_RX_CHAN_SEL_REG, >0, 2), >+ .get_sr = sun8i_i2s_get_sr_wss, >+ .get_wss = sun8i_i2s_get_sr_wss, > }; > > static const struct sun4i_i2s_quirks sun50i_a64_codec_i2s_quirks = { >@@ -961,6 +998,8 @@ static const struct sun4i_i2s_quirks >sun50i_a64_codec_i2s_quirks = { > .field_rxchanmap = >REG_FIELD(SUN4I_I2S_RX_CHAN_MAP_REG, 0, 31), > .field_txchansel = REG_FIELD(SUN4I_I2S_TX_CHAN_SEL_REG, >0, 2), > .field_rxchansel = REG_FIELD(SUN4I_I2S_RX_CHAN_SEL_REG, >0, 2), >+ .get_sr = sun8i_i2s_get_sr_wss, >+ .get_wss = sun8i_i2s_get_sr_wss, > }; > > static int sun4i_i2s_init_regmap_fields(struct device *dev, >-- >2.21.0 > >_______________________________________________ >Alsa-devel mailing list >Alsa-devel@xxxxxxxxxxxxxxxx >https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
