Re: [PATCH v2 2/5] ASoC: sun4i-spdif: Add support for H6 SoC

From: "Clément Péron" <peron.clem@gmail.com>
To: Maxime Ripard <maxime.ripard@bootlin.com>
Cc: Chen-Yu Tsai <wens@csie.org>, Takashi Iwai <tiwai@suse.com>,
	Jaroslav Kysela <perex@perex.cz>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Mark Brown <broonie@kernel.org>, Rob Herring <robh+dt@kernel.org>,
	Linux-ALSA <alsa-devel@alsa-project.org>,
	devicetree <devicetree@vger.kernel.org>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
	linux-sunxi <linux-sunxi@googlegroups.com>
Subject: Re: [PATCH v2 2/5] ASoC: sun4i-spdif: Add support for H6 SoC
Date: Thu, 2 May 2019 11:39:24 +0200	[thread overview]
Message-ID: <CAJiuCcdFUPBsXfKtDLt-p6Edx-7JrST9d0C=ofCU4CL8ZxwcsA@mail.gmail.com> (raw)
In-Reply-To: <20190502082526.c5zo4uzceqzizbxo@flea>

Hi Maxime,

On Thu, 2 May 2019 at 10:25, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Fri, Apr 19, 2019 at 09:17:27PM +0200, Clément Péron wrote:
> > Allwinner H6 has a different mapping for the fifo register controller.
> >
> > Actually only the fifo tx flush bit is used.
> >
> > Add a new quirk to know the correct fifo tx flush bit.
> >
> > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > ---
> >  sound/soc/sunxi/sun4i-spdif.c | 42 ++++++++++++++++++++++++++++++-----
> >  1 file changed, 36 insertions(+), 6 deletions(-)
> >
> > diff --git a/sound/soc/sunxi/sun4i-spdif.c b/sound/soc/sunxi/sun4i-spdif.c
> > index b4af4aabead1..19e4bf9caa24 100644
> > --- a/sound/soc/sunxi/sun4i-spdif.c
> > +++ b/sound/soc/sunxi/sun4i-spdif.c
> > @@ -75,6 +75,18 @@
> >       #define SUN4I_SPDIF_FCTL_RXOM(v)                ((v) << 0)
> >       #define SUN4I_SPDIF_FCTL_RXOM_MASK              GENMASK(1, 0)
> >
> > +#define SUN50I_H6_SPDIF_FCTL (0x14)
> > +     #define SUN50I_H6_SPDIF_FCTL_HUB_EN             BIT(31)
> > +     #define SUN50I_H6_SPDIF_FCTL_FTX                BIT(30)
> > +     #define SUN50I_H6_SPDIF_FCTL_FRX                BIT(29)
> > +     #define SUN50I_H6_SPDIF_FCTL_TXTL(v)            ((v) << 12)
> > +     #define SUN50I_H6_SPDIF_FCTL_TXTL_MASK          GENMASK(19, 12)
> > +     #define SUN50I_H6_SPDIF_FCTL_RXTL(v)            ((v) << 4)
> > +     #define SUN50I_H6_SPDIF_FCTL_RXTL_MASK          GENMASK(10, 4)
> > +     #define SUN50I_H6_SPDIF_FCTL_TXIM               BIT(2)
> > +     #define SUN50I_H6_SPDIF_FCTL_RXOM(v)            ((v) << 0)
> > +     #define SUN50I_H6_SPDIF_FCTL_RXOM_MASK          GENMASK(1, 0)
> > +
> >  #define SUN4I_SPDIF_FSTA     (0x18)
> >       #define SUN4I_SPDIF_FSTA_TXE                    BIT(14)
> >       #define SUN4I_SPDIF_FSTA_TXECNTSHT              (8)
> > @@ -169,16 +181,25 @@ struct sun4i_spdif_dev {
> >       struct snd_soc_dai_driver cpu_dai_drv;
> >       struct regmap *regmap;
> >       struct snd_dmaengine_dai_dma_data dma_params_tx;
> > +     const struct sun4i_spdif_quirks *quirks;
>
> I guess this will generate a warning since the structure hasn't been
> defined yet?

It's a pointer to a structure so no warning from the compiler.

>
> > +};
> > +
> > +struct sun4i_spdif_quirks {
> > +     unsigned int reg_dac_txdata;    /* TX FIFO offset for DMA config */
> > +     unsigned int reg_fctl_ftx;      /* TX FIFO flush bitmask */
> > +     bool has_reset;
>
> You don't really need to move it around, you can just add the
> structure prototype.
>
> If you do want to move it around, then please do so in a separate patch

I have choose to move it to follow what is done in the sun4i-i2s.
I will put it in a separate patch and make the comment a bit more proper.

>
> >  };
> >
> >  static void sun4i_spdif_configure(struct sun4i_spdif_dev *host)
> >  {
> > +     const struct sun4i_spdif_quirks *quirks = host->quirks;
> > +
> >       /* soft reset SPDIF */
> >       regmap_write(host->regmap, SUN4I_SPDIF_CTL, SUN4I_SPDIF_CTL_RESET);
> >
> >       /* flush TX FIFO */
> >       regmap_update_bits(host->regmap, SUN4I_SPDIF_FCTL,
> > -                        SUN4I_SPDIF_FCTL_FTX, SUN4I_SPDIF_FCTL_FTX);
> > +                        quirks->reg_fctl_ftx, quirks->reg_fctl_ftx);
> >
> >       /* clear TX counter */
> >       regmap_write(host->regmap, SUN4I_SPDIF_TXCNT, 0);
> > @@ -405,22 +426,26 @@ static struct snd_soc_dai_driver sun4i_spdif_dai = {
> >       .name = "spdif",
> >  };
> >
> > -struct sun4i_spdif_quirks {
> > -     unsigned int reg_dac_txdata;    /* TX FIFO offset for DMA config */
> > -     bool has_reset;
> > -};
> > -
> >  static const struct sun4i_spdif_quirks sun4i_a10_spdif_quirks = {
> >       .reg_dac_txdata = SUN4I_SPDIF_TXFIFO,
> > +     .reg_fctl_ftx   = SUN4I_SPDIF_FCTL_FTX,
> >  };
> >
> >  static const struct sun4i_spdif_quirks sun6i_a31_spdif_quirks = {
> >       .reg_dac_txdata = SUN4I_SPDIF_TXFIFO,
> > +     .reg_fctl_ftx   = SUN4I_SPDIF_FCTL_FTX,
> >       .has_reset      = true,
> >  };
> >
> >  static const struct sun4i_spdif_quirks sun8i_h3_spdif_quirks = {
> >       .reg_dac_txdata = SUN8I_SPDIF_TXFIFO,
> > +     .reg_fctl_ftx   = SUN4I_SPDIF_FCTL_FTX,
> > +     .has_reset      = true,
> > +};
> >
> > +static const struct sun4i_spdif_quirks sun50i_h6_spdif_quirks = {
> > +     .reg_dac_txdata = SUN8I_SPDIF_TXFIFO,
> > +     .reg_fctl_ftx   = SUN50I_H6_SPDIF_FCTL_FTX,
> >       .has_reset      = true,
>
> The reg_dac_txdata and reg_fctl_ftx changes here should also be part
> of a separate patch.

You mean the reg_fctl_ftx quirk and the H6 introduction should be split ?

Thanks for the review,
Clement

>
> Maxime
>
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com