[PATCH v2 0/5] Allwinner H6 SPDIF support

[PATCH v2 0/5] Allwinner H6 SPDIF support

[Clément Péron]

*H6 DMA support IS REQUIRED*

Allwinner H6 SoC has a SPDIF controller called One Wire Audio (OWA) which
is different from the previous H3 generation and not compatible.

Difference are an increase of fifo sizes, some memory mapping are different
and there is now the possibility to output the master clock on a pin.

Also this series require the DMA working on H6, a first version has been
submitted by Jernej Škrabec but is not yet accepted (as this moment):
https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=89011

This series has been tested on Beelink GS1 on top of sunxi/for-next with
Jernej DMA support patch.

Clément Péron (5):
  dt-bindings: sound: sun4i-spdif: Add Allwinner H6 compatible
  ASoC: sun4i-spdif: Add support for H6 SoC
  arm64: dts: allwinner: Add SPDIF node for Allwinner H6
  arm64: dts: allwinner: h6: Enable SPDIF for Beelink GS1
  arm64: defconfig: Enable Sun4i SPDIF module

 .../bindings/sound/sunxi,sun4i-spdif.txt      |  3 +-
 .../dts/allwinner/sun50i-h6-beelink-gs1.dts   |  4 ++
 arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi  | 38 +++++++++++++++++
 arch/arm64/configs/defconfig                  |  1 +
 sound/soc/sunxi/sun4i-spdif.c                 | 42 ++++++++++++++++---
 5 files changed, 81 insertions(+), 7 deletions(-)

-- 
2.17.1

[PATCH v2 1/5] dt-bindings: sound: sun4i-spdif: Add Allwinner H6 compatible

[Clément Péron]

Allwinner H6 has a SPDIF controller with an increase of the fifo
size and a sligher difference in memory mapping compare which
make it not compatible with the previous generation H3/A64.

Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
 Documentation/devicetree/bindings/sound/sunxi,sun4i-spdif.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/sound/sunxi,sun4i-spdif.txt b/Documentation/devicetree/bindings/sound/sunxi,sun4i-spdif.txt
index 0c64a209c2e9..c0fbb50a4df9 100644
--- a/Documentation/devicetree/bindings/sound/sunxi,sun4i-spdif.txt
+++ b/Documentation/devicetree/bindings/sound/sunxi,sun4i-spdif.txt
@@ -7,10 +7,11 @@ For now only playback is supported.
 
 Required properties:
 
-  - compatible		: should be one of the following:
+  - compatible		: Should be one of the following:
     - "allwinner,sun4i-a10-spdif": for the Allwinner A10 SoC
     - "allwinner,sun6i-a31-spdif": for the Allwinner A31 SoC
     - "allwinner,sun8i-h3-spdif": for the Allwinner H3 SoC
+    - "allwinner,sun50i-h6-spdif": for the allwinner H6 SoC
 
   - reg			: Offset and length of the register set for the device.
 
-- 
2.17.1

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

[Clément Péron]

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;
+};
+
+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;
 };
 
 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,
 };
 
@@ -437,6 +462,10 @@ static const struct of_device_id sun4i_spdif_of_match[] = {
 		.compatible = "allwinner,sun8i-h3-spdif",
 		.data = &sun8i_h3_spdif_quirks,
 	},
+	{
+		.compatible = "allwinner,sun50i-h6-spdif",
+		.data = &sun50i_h6_spdif_quirks,
+	},
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, sun4i_spdif_of_match);
@@ -501,6 +530,7 @@ static int sun4i_spdif_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "Failed to determine the quirks to use\n");
 		return -ENODEV;
 	}
+	host->quirks = quirks;
 
 	host->regmap = devm_regmap_init_mmio(&pdev->dev, base,
 						&sun4i_spdif_regmap_config);
-- 
2.17.1

[PATCH v2 3/5] arm64: dts: allwinner: Add SPDIF node for Allwinner H6

[Clément Péron]

The Allwinner H6 has a SPDIF controller called OWA (One Wire Audio).

Only one pinmuxing is available so set it as default.

Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
 arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 38 ++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
index ad2077830f73..6c1debf11e5c 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
@@ -83,6 +83,24 @@
 		method = "smc";
 	};
 
+	sound_spdif {
+		compatible = "simple-audio-card";
+		simple-audio-card,name = "On-board SPDIF";
+
+		simple-audio-card,cpu {
+			sound-dai = <&spdif>;
+		};
+
+		simple-audio-card,codec {
+			sound-dai = <&spdif_out>;
+		};
+	};
+
+	spdif_out: spdif-out {
+		#sound-dai-cells = <0>;
+		compatible = "linux,spdif-dit";
+	};
+
 	timer {
 		compatible = "arm,armv8-timer";
 		interrupts = <GIC_PPI 13
@@ -264,6 +282,11 @@
 				bias-pull-up;
 			};
 
+			spdif_tx_pin: spdif-tx-pin {
+				pins = "PH7";
+				function = "spdif";
+			};
+
 			uart0_ph_pins: uart0-ph-pins {
 				pins = "PH0", "PH1";
 				function = "uart0";
@@ -391,6 +414,21 @@
 			};
 		};
 
+		spdif: spdif@5093000 {
+			#sound-dai-cells = <0>;
+			compatible = "allwinner,sun50i-h6-spdif";
+			reg = <0x05093000 0x400>;
+			interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ccu CLK_BUS_SPDIF>, <&ccu CLK_SPDIF>;
+			clock-names = "apb", "spdif";
+			resets = <&ccu RST_BUS_SPDIF>;
+			dmas = <&dma 2>;
+			dma-names = "tx";
+			pinctrl-names = "default";
+			pinctrl-0 = <&spdif_tx_pin>;
+			status = "disabled";
+		};
+
 		usb2otg: usb@5100000 {
 			compatible = "allwinner,sun50i-h6-musb",
 				     "allwinner,sun8i-a33-musb";
-- 
2.17.1

[PATCH v2 4/5] arm64: dts: allwinner: h6: Enable SPDIF for Beelink GS1

[Clément Péron]

Beelink GS1 board has a SPDIF out connector, so enable it in
the device-tree.

Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
 arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
index 0dc33c90dd60..76a95ad33dc5 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
@@ -243,6 +243,10 @@
 	vcc-pm-supply = <&reg_aldo1>;
 };
 
+&spdif {
+	status = "okay";
+};
+
 &uart0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&uart0_ph_pins>;
-- 
2.17.1

[PATCH v2 5/5] arm64: defconfig: Enable Sun4i SPDIF module

[Clément Péron]

Allwinner A64 and H6 use the Sun4i SPDIF driver.

Enable this to allow a proper support.

Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 2d9c39033c1a..423413768d79 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -531,6 +531,7 @@ CONFIG_SND_SOC_RK3399_GRU_SOUND=m
 CONFIG_SND_MESON_AXG_SOUND_CARD=m
 CONFIG_SND_SOC_SAMSUNG=y
 CONFIG_SND_SOC_RCAR=m
+CONFIG_SND_SUN4I_SPDIF=m
 CONFIG_SND_SOC_AK4613=m
 CONFIG_SND_SOC_PCM3168A_I2C=m
 CONFIG_SND_SIMPLE_CARD=m
-- 
2.17.1

Re: [PATCH v2 1/5] dt-bindings: sound: sun4i-spdif: Add Allwinner H6 compatible

[Rob Herring]

On Fri, 19 Apr 2019 21:17:26 +0200, =?UTF-8?q?Cl=C3=A9ment=20P=C3=A9ron?= wrote:
> Allwinner H6 has a SPDIF controller with an increase of the fifo
> size and a sligher difference in memory mapping compare which
> make it not compatible with the previous generation H3/A64.
> 
> Signed-off-by: Clément Péron <peron.clem@gmail.com>
> ---
>  Documentation/devicetree/bindings/sound/sunxi,sun4i-spdif.txt | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 1/5] dt-bindings: sound: sun4i-spdif: Add Allwinner H6 compatible

[Rob Herring]

On Fri, 19 Apr 2019 21:17:26 +0200, =?UTF-8?q?Cl=C3=A9ment=20P=C3=A9ron?= wrote:
> Allwinner H6 has a SPDIF controller with an increase of the fifo
> size and a sligher difference in memory mapping compare which
> make it not compatible with the previous generation H3/A64.
> 
> Signed-off-by: Clément Péron <peron.clem@gmail.com>
> ---
>  Documentation/devicetree/bindings/sound/sunxi,sun4i-spdif.txt | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 1/5] dt-bindings: sound: sun4i-spdif: Add Allwinner H6 compatible

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 473 bytes --]

On Fri, Apr 19, 2019 at 09:17:26PM +0200, Clément Péron wrote:
> Allwinner H6 has a SPDIF controller with an increase of the fifo
> size and a sligher difference in memory mapping compare which
> make it not compatible with the previous generation H3/A64.
>
> Signed-off-by: Clément Péron <peron.clem@gmail.com>

Acked-by: Maxime Ripard <maxime.ripard@bootlin.com>

Maxime

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3855 bytes --]

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?

> +};
> +
> +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

>  };
>
>  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.

Maxime

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Clément Péron]

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

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2071 bytes --]

On Thu, May 02, 2019 at 11:39:24AM +0200, Clément Péron wrote:
> > > @@ -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.

Damn, I was convinced just declaring a pointer to a structure would
result to a gcc warning. Nevermind then.

> > > @@ -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 ?

Yep

Maxime

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]