Hi Maxime,
On Thu, 2 May 2019 at 10:25, Maxime Ripard <maxime.ripard@xxxxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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
