| Message ID | 20201003141950.455829-1-peron.clem@gmail.com |
|---|---|
| Headers | show |
| Series | Add Allwinner H3/H5/H6/A64 HDMI audio | expand |
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
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
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
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>
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(-)