Re: [linux-sunxi] [PATCH v5 01/20] ASoC: sun4i-i2s: Add support for H6 I2S

[Chen-Yu Tsai]

On Mon, Sep 28, 2020 at 3:29 AM Clément Péron <peron.clem@xxxxxxxxx> wrote:
>
> From: Jernej Skrabec <jernej.skrabec@xxxxxxxx>
>
> H6 I2S is very similar to that in H3, except it supports up to 16
> channels.
>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@xxxxxxxx>
> Signed-off-by: Marcus Cooper <codekipper@xxxxxxxxx>
> Signed-off-by: Clément Péron <peron.clem@xxxxxxxxx>
> ---
>  sound/soc/sunxi/sun4i-i2s.c | 224 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 224 insertions(+)
>
> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> index f23ff29e7c1d..2baf6c276280 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,62 @@ 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_MAP1_REG, 0x76543210);
> +       regmap_write(i2s->regmap, SUN50I_H6_I2S_RX_CHAN_MAP1_REG, 0x76543210);

Nit, since it supports up to 16 channels, you might want to map all 16 of them
now, instead of having to come back and fix it later.

Code wise, this patch is

Reviewed-by: Chen-Yu Tsai <wens@xxxxxxxx>

I don't have a scope nor logic analyzer, so I wasn't able to participate in the
LRCK discussion.