[PATCH V1 0/3] ALSA: hda: add CIX IPBLOQ HDA controller support

[PATCH V1 0/3] ALSA: hda: add CIX IPBLOQ HDA controller support

[joakim.zhang]

From: Joakim Zhang <joakim.zhang@cixtech.com>

This patch set adds CIX IPBLOQ HDA controller support.

Joakim Zhang (3):
  dt-bindings: sound: add binding for CIX IPBLOQ HDA controller
  ALSA: hda: add bus callback for address translation
  ALSA: hda: add CIX IPBLOQ HDA controller support

 .../bindings/sound/cix,ipbloq-hda.yaml        |  69 +++
 include/sound/hdaudio.h                       |   3 +
 sound/hda/controllers/Kconfig                 |  13 +
 sound/hda/controllers/Makefile                |   2 +
 sound/hda/controllers/cix-ipbloq.c            | 470 ++++++++++++++++++
 sound/hda/core/controller.c                   |  25 +-
 sound/hda/core/stream.c                       |  17 +-
 7 files changed, 590 insertions(+), 9 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/cix,ipbloq-hda.yaml
 create mode 100644 sound/hda/controllers/cix-ipbloq.c

-- 
2.49.0

[PATCH V1 1/3] dt-bindings: sound: add binding for CIX IPBLOQ HDA controller

[joakim.zhang]

From: Joakim Zhang <joakim.zhang@cixtech.com>

This patch adds binding for CIX IPBLOQ HDA controller.

Signed-off-by: Joakim Zhang <joakim.zhang@cixtech.com>
---
 .../bindings/sound/cix,ipbloq-hda.yaml        | 69 +++++++++++++++++++
 1 file changed, 69 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/cix,ipbloq-hda.yaml

diff --git a/Documentation/devicetree/bindings/sound/cix,ipbloq-hda.yaml b/Documentation/devicetree/bindings/sound/cix,ipbloq-hda.yaml
new file mode 100644
index 000000000000..a4285d1e0319
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/cix,ipbloq-hda.yaml
@@ -0,0 +1,69 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/cix,ipbloq-hda.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: CIX IPBLOQ HDA controller
+
+description: |
+  CIX IPBLOQ High Definition Audio (HDA) Controller driver.
+
+maintainers:
+  - Joakim Zhang <joakim.zhang@cixtech.com>
+
+properties:
+  compatible:
+    const: cix,sky1-ipbloq-hda
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    description: The interrupt from the HDA controller
+    maxItems: 1
+
+  clocks:
+    maxItems: 2
+
+  clock-names:
+    maxItems: 2
+
+  resets:
+    maxItems: 1
+
+  reset-names:
+    maxItems: 1
+
+  cix,model:
+    $ref: /schemas/types.yaml#/definitions/string
+    description: |
+      The user-visible name of this sound complex. If this property is
+      not specified then boards can use default name provided in hda driver.
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+  - resets
+  - reset-names
+
+additionalProperties: false
+
+examples:
+  - |
+    #include<dt-bindings/interrupt-controller/arm-gic.h>
+
+    audss_ipb_hda: ipb-hda@70c0000 {
+        compatible = "cix,sky1-ipbloq-hda";
+        reg = <0x70c0000 0x10000>;
+        interrupts = <GIC_SPI 234 IRQ_TYPE_LEVEL_HIGH>;
+        clocks = <&audss_clk 7>,
+                 <&audss_clk 8>;
+        clock-names = "sysclk", "clk48m";
+        resets = <&audss_rst 14>;
+        reset-names = "hda";
+        cix,model = "CIX SKY1 EVB HDA";
+    };
-- 
2.49.0

Re: [PATCH V1 1/3] dt-bindings: sound: add binding for CIX IPBLOQ HDA controller

[Krzysztof Kozlowski]

On Thu, Oct 30, 2025 at 07:09:26PM +0800, joakim.zhang@cixtech.com wrote:
> From: Joakim Zhang <joakim.zhang@cixtech.com>
> 
> This patch adds binding for CIX IPBLOQ HDA controller.

Please do not use "This commit/patch/change", but imperative mood. See
longer explanation here:
https://elixir.bootlin.com/linux/v6.16/source/Documentation/process/submitting-patches.rst#L94

Please use standard email subjects, so with the PATCH keyword in the
title. 'git format-patch -vX' helps here to create proper versioned patches.
Another useful tool is b4. Skipping the PATCH keyword makes filtering of
emails more difficult thus making the review process less convenient.

A nit, subject: drop second/last, redundant "binding for". The
"dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.17-rc3/source/Documentation/devicetree/bindings/submitting-patches.rst#L18

> 
> Signed-off-by: Joakim Zhang <joakim.zhang@cixtech.com>
> ---
>  .../bindings/sound/cix,ipbloq-hda.yaml        | 69 +++++++++++++++++++
>  1 file changed, 69 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/cix,ipbloq-hda.yaml
> 
> diff --git a/Documentation/devicetree/bindings/sound/cix,ipbloq-hda.yaml b/Documentation/devicetree/bindings/sound/cix,ipbloq-hda.yaml
> new file mode 100644
> index 000000000000..a4285d1e0319
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/cix,ipbloq-hda.yaml

Filename must match the compatible. See writing bindings doc.

> @@ -0,0 +1,69 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/cix,ipbloq-hda.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: CIX IPBLOQ HDA controller
> +
> +description: |

Do not need '|' unless you need to preserve formatting.

> +  CIX IPBLOQ High Definition Audio (HDA) Controller driver.

driver as in Linux driver? No, please describe here briefly hardware.

> +
> +maintainers:
> +  - Joakim Zhang <joakim.zhang@cixtech.com>
> +
> +properties:
> +  compatible:
> +    const: cix,sky1-ipbloq-hda
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    description: The interrupt from the HDA controller

Drop description, obvious. Cannot be anything else.

> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 2

You should list the items instead with description.

> +
> +  clock-names:
> +    maxItems: 2

No, you need to list the items. There is no syntax like that.

> +
> +  resets:
> +    maxItems: 1
> +
> +  reset-names:
> +    maxItems: 1

No, you need to list the items.

> +
> +  cix,model:
> +    $ref: /schemas/types.yaml#/definitions/string

Use standard properties, see other bindings. Maybe ones from
generic/simple audio card fit.


> +    description: |

Do not need '|' unless you need to preserve formatting.

> +      The user-visible name of this sound complex. If this property is
> +      not specified then boards can use default name provided in hda driver.
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +  - clock-names
> +  - resets
> +  - reset-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include<dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    audss_ipb_hda: ipb-hda@70c0000 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
If you cannot find a name matching your device, please check in kernel
sources for similar cases or you can grow the spec (via pull request to
DT spec repo).

e.g. sound or sound-card. Maybe hda, if it is known enough.

Also drop unused label.

Best regards,
Krzysztof

Re: [PATCH V1 1/3] dt-bindings: sound: add binding for CIX IPBLOQ HDA controller

[Krzysztof Kozlowski]

On 31/10/2025 09:58, Krzysztof Kozlowski wrote:
> On Thu, Oct 30, 2025 at 07:09:26PM +0800, joakim.zhang@cixtech.com wrote:>
> Please use standard email subjects, so with the PATCH keyword in the
> title. 'git format-patch -vX' helps here to create proper versioned patches.
> Another useful tool is b4. Skipping the PATCH keyword makes filtering of
> emails more difficult thus making the review process less convenient.
That's wrong template, sorry. I wanted one about the subject prefixes,
here it goes:

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters

ASoC: dt-bindings: add CIX ...

Best regards,
Krzysztof

Re: [PATCH V1 1/3] dt-bindings: sound: add binding for CIX IPBLOQ HDA controller

[Conor Dooley]

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

On Fri, Oct 31, 2025 at 09:58:48AM +0100, Krzysztof Kozlowski wrote:
> On Thu, Oct 30, 2025 at 07:09:26PM +0800, joakim.zhang@cixtech.com wrote:
> 
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 2
> 
> You should list the items instead with description.
> 
> > +
> > +  clock-names:
> > +    maxItems: 2
> 
> No, you need to list the items. There is no syntax like that.
> 
> > +
> > +  resets:
> > +    maxItems: 1
> > +
> > +  reset-names:
> > +    maxItems: 1
> 
> No, you need to list the items.

Since it wasn't mentioned why the items need to be listed, if you don't
list the items for a -names property, then they can be anything and you
cannot actually use them in drivers.

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

RE: [PATCH V1 1/3] dt-bindings: sound: add binding for CIX IPBLOQ HDA controller

[Joakim Zhang]

Thanks, Krzysztof

I will follow up your detailed comments.

Joakim

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Friday, October 31, 2025 4:59 PM
> To: Joakim Zhang <joakim.zhang@cixtech.com>
> Cc: lgirdwood@gmail.com; broonie@kernel.org; robh@kernel.org;
> krzk+dt@kernel.org; conor+dt@kernel.org; perex@perex.cz;
> tiwai@suse.com; linux-sound@vger.kernel.org; devicetree@vger.kernel.org;
> cix-kernel-upstream <cix-kernel-upstream@cixtech.com>
> Subject: Re: [PATCH V1 1/3] dt-bindings: sound: add binding for CIX IPBLOQ
> HDA controller
> 
> EXTERNAL EMAIL
> 
> On Thu, Oct 30, 2025 at 07:09:26PM +0800, joakim.zhang@cixtech.com
> wrote:
> > From: Joakim Zhang <joakim.zhang@cixtech.com>
> >
> > This patch adds binding for CIX IPBLOQ HDA controller.
> 
> Please do not use "This commit/patch/change", but imperative mood. See
> longer explanation here:
> https://elixir.bootlin.com/linux/v6.16/source/Documentation/process/submi
> tting-patches.rst#L94
> 
> Please use standard email subjects, so with the PATCH keyword in the title.
> 'git format-patch -vX' helps here to create proper versioned patches.
> Another useful tool is b4. Skipping the PATCH keyword makes filtering of
> emails more difficult thus making the review process less convenient.
> 
> A nit, subject: drop second/last, redundant "binding for". The "dt-bindings"
> prefix is already stating that these are bindings.
> See also:
> https://elixir.bootlin.com/linux/v6.17-
> rc3/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
> 
> >
> > Signed-off-by: Joakim Zhang <joakim.zhang@cixtech.com>
> > ---
> >  .../bindings/sound/cix,ipbloq-hda.yaml        | 69 +++++++++++++++++++
> >  1 file changed, 69 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/sound/cix,ipbloq-hda.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/sound/cix,ipbloq-hda.yaml
> > b/Documentation/devicetree/bindings/sound/cix,ipbloq-hda.yaml
> > new file mode 100644
> > index 000000000000..a4285d1e0319
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/cix,ipbloq-hda.yaml
> 
> Filename must match the compatible. See writing bindings doc.
> 
> > @@ -0,0 +1,69 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/sound/cix,ipbloq-hda.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: CIX IPBLOQ HDA controller
> > +
> > +description: |
> 
> Do not need '|' unless you need to preserve formatting.
> 
> > +  CIX IPBLOQ High Definition Audio (HDA) Controller driver.
> 
> driver as in Linux driver? No, please describe here briefly hardware.
> 
> > +
> > +maintainers:
> > +  - Joakim Zhang <joakim.zhang@cixtech.com>
> > +
> > +properties:
> > +  compatible:
> > +    const: cix,sky1-ipbloq-hda
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    description: The interrupt from the HDA controller
> 
> Drop description, obvious. Cannot be anything else.
> 
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 2
> 
> You should list the items instead with description.
> 
> > +
> > +  clock-names:
> > +    maxItems: 2
> 
> No, you need to list the items. There is no syntax like that.
> 
> > +
> > +  resets:
> > +    maxItems: 1
> > +
> > +  reset-names:
> > +    maxItems: 1
> 
> No, you need to list the items.
> 
> > +
> > +  cix,model:
> > +    $ref: /schemas/types.yaml#/definitions/string
> 
> Use standard properties, see other bindings. Maybe ones from
> generic/simple audio card fit.
> 
> 
> > +    description: |
> 
> Do not need '|' unless you need to preserve formatting.
> 
> > +      The user-visible name of this sound complex. If this property is
> > +      not specified then boards can use default name provided in hda driver.
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - clocks
> > +  - clock-names
> > +  - resets
> > +  - reset-names
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include<dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +    audss_ipb_hda: ipb-hda@70c0000 {
> 
> Node names should be generic. See also an explanation and list of examples
> (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-
> devicetree-basics.html#generic-names-recommendation
> If you cannot find a name matching your device, please check in kernel
> sources for similar cases or you can grow the spec (via pull request to DT
> spec repo).
> 
> e.g. sound or sound-card. Maybe hda, if it is known enough.
> 
> Also drop unused label.
> 
> Best regards,
> Krzysztof

[PATCH V1 2/3] ALSA: hda: add bus callback for address translation

[joakim.zhang]

From: Joakim Zhang <joakim.zhang@cixtech.com>

This patch adds bus callback for address translation, for some
SoCs such as CIX SKY1 which is ARM64, HOST and HDAC has different
memory view, so need to do address translation between HOST and HDAC.

Signed-off-by: Joakim Zhang <joakim.zhang@cixtech.com>
---
 include/sound/hdaudio.h     |  3 +++
 sound/hda/core/controller.c | 25 +++++++++++++++++++------
 sound/hda/core/stream.c     | 17 ++++++++++++++---
 3 files changed, 36 insertions(+), 9 deletions(-)

diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
index 4e0c1d8af09f..61b41a014f4a 100644
--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -293,6 +293,9 @@ struct hdac_bus {
 	const struct hdac_bus_ops *ops;
 	const struct hdac_ext_bus_ops *ext_ops;
 
+	/* address translation from host to hdac */
+	dma_addr_t (*addr_host_to_hdac)(struct hdac_bus *bus, dma_addr_t addr);
+
 	/* h/w resources */
 	unsigned long addr;
 	void __iomem *remap_addr;
diff --git a/sound/hda/core/controller.c b/sound/hda/core/controller.c
index a7c00ad80117..070f05a7eafa 100644
--- a/sound/hda/core/controller.c
+++ b/sound/hda/core/controller.c
@@ -42,14 +42,19 @@ static void azx_clear_corbrp(struct hdac_bus *bus)
  */
 void snd_hdac_bus_init_cmd_io(struct hdac_bus *bus)
 {
+	dma_addr_t corb_addr, rirb_addr;
+
 	WARN_ON_ONCE(!bus->rb.area);
 
 	guard(spinlock_irq)(&bus->reg_lock);
 	/* CORB set up */
 	bus->corb.addr = bus->rb.addr;
 	bus->corb.buf = (__le32 *)bus->rb.area;
-	snd_hdac_chip_writel(bus, CORBLBASE, (u32)bus->corb.addr);
-	snd_hdac_chip_writel(bus, CORBUBASE, upper_32_bits(bus->corb.addr));
+	corb_addr = bus->corb.addr;
+	if (bus->addr_host_to_hdac)
+		corb_addr = bus->addr_host_to_hdac(bus, bus->corb.addr);
+	snd_hdac_chip_writel(bus, CORBLBASE, (u32)corb_addr);
+	snd_hdac_chip_writel(bus, CORBUBASE, upper_32_bits(corb_addr));
 
 	/* set the corb size to 256 entries (ULI requires explicitly) */
 	snd_hdac_chip_writeb(bus, CORBSIZE, 0x02);
@@ -70,8 +75,11 @@ void snd_hdac_bus_init_cmd_io(struct hdac_bus *bus)
 	bus->rirb.buf = (__le32 *)(bus->rb.area + 2048);
 	bus->rirb.wp = bus->rirb.rp = 0;
 	memset(bus->rirb.cmds, 0, sizeof(bus->rirb.cmds));
-	snd_hdac_chip_writel(bus, RIRBLBASE, (u32)bus->rirb.addr);
-	snd_hdac_chip_writel(bus, RIRBUBASE, upper_32_bits(bus->rirb.addr));
+	rirb_addr = bus->rirb.addr;
+	if (bus->addr_host_to_hdac)
+		rirb_addr = bus->addr_host_to_hdac(bus, bus->rirb.addr);
+	snd_hdac_chip_writel(bus, RIRBLBASE, (u32)rirb_addr);
+	snd_hdac_chip_writel(bus, RIRBUBASE, upper_32_bits(rirb_addr));
 
 	/* set the rirb size to 256 entries (ULI requires explicitly) */
 	snd_hdac_chip_writeb(bus, RIRBSIZE, 0x02);
@@ -608,6 +616,8 @@ static void azx_int_clear(struct hdac_bus *bus)
  */
 bool snd_hdac_bus_init_chip(struct hdac_bus *bus, bool full_reset)
 {
+	dma_addr_t posbuf_addr;
+
 	if (bus->chip_init)
 		return false;
 
@@ -625,8 +635,11 @@ bool snd_hdac_bus_init_chip(struct hdac_bus *bus, bool full_reset)
 
 	/* program the position buffer */
 	if (bus->use_posbuf && bus->posbuf.addr) {
-		snd_hdac_chip_writel(bus, DPLBASE, (u32)bus->posbuf.addr);
-		snd_hdac_chip_writel(bus, DPUBASE, upper_32_bits(bus->posbuf.addr));
+		posbuf_addr = bus->posbuf.addr;
+		if (bus->addr_host_to_hdac)
+			posbuf_addr = bus->addr_host_to_hdac(bus, bus->posbuf.addr);
+		snd_hdac_chip_writel(bus, DPLBASE, (u32)posbuf_addr);
+		snd_hdac_chip_writel(bus, DPUBASE, upper_32_bits(posbuf_addr));
 	}
 
 	bus->chip_init = true;
diff --git a/sound/hda/core/stream.c b/sound/hda/core/stream.c
index 579ec544ef4a..0bf181f83b6e 100644
--- a/sound/hda/core/stream.c
+++ b/sound/hda/core/stream.c
@@ -258,6 +258,7 @@ int snd_hdac_stream_setup(struct hdac_stream *azx_dev, bool code_loading)
 {
 	struct hdac_bus *bus = azx_dev->bus;
 	struct snd_pcm_runtime *runtime;
+	dma_addr_t bdl_addr, posbuf_addr;
 	unsigned int val;
 	u16 reg;
 	int ret;
@@ -287,17 +288,24 @@ int snd_hdac_stream_setup(struct hdac_stream *azx_dev, bool code_loading)
 	snd_hdac_stream_writew(azx_dev, SD_LVI, azx_dev->frags - 1);
 
 	/* program the BDL address */
+	bdl_addr = azx_dev->bdl.addr;
+	if (bus->addr_host_to_hdac)
+		bdl_addr = bus->addr_host_to_hdac(bus, azx_dev->bdl.addr);
 	/* lower BDL address */
-	snd_hdac_stream_writel(azx_dev, SD_BDLPL, (u32)azx_dev->bdl.addr);
+	snd_hdac_stream_writel(azx_dev, SD_BDLPL, (u32)bdl_addr);
 	/* upper BDL address */
 	snd_hdac_stream_writel(azx_dev, SD_BDLPU,
-			       upper_32_bits(azx_dev->bdl.addr));
+			       upper_32_bits(bdl_addr));
 
 	/* enable the position buffer */
 	if (bus->use_posbuf && bus->posbuf.addr) {
+		posbuf_addr = bus->posbuf.addr;
+		if (bus->addr_host_to_hdac)
+			posbuf_addr = bus->addr_host_to_hdac(bus, bus->posbuf.addr);
+
 		if (!(snd_hdac_chip_readl(bus, DPLBASE) & AZX_DPLBASE_ENABLE))
 			snd_hdac_chip_writel(bus, DPLBASE,
-				(u32)bus->posbuf.addr | AZX_DPLBASE_ENABLE);
+				(u32)posbuf_addr | AZX_DPLBASE_ENABLE);
 	}
 
 	/* set the interrupt enable bits in the descriptor control register */
@@ -463,6 +471,9 @@ static int setup_bdle(struct hdac_bus *bus,
 			return -EINVAL;
 
 		addr = snd_sgbuf_get_addr(dmab, ofs);
+		if (bus->addr_host_to_hdac)
+			addr = bus->addr_host_to_hdac(bus, addr);
+
 		/* program the address field of the BDL entry */
 		bdl[0] = cpu_to_le32((u32)addr);
 		bdl[1] = cpu_to_le32(upper_32_bits(addr));
-- 
2.49.0

Re: [PATCH V1 2/3] ALSA: hda: add bus callback for address translation

[Takashi Iwai]

On Thu, 30 Oct 2025 12:09:27 +0100,
joakim.zhang@cixtech.com wrote:
> 
> From: Joakim Zhang <joakim.zhang@cixtech.com>
> 
> This patch adds bus callback for address translation, for some
> SoCs such as CIX SKY1 which is ARM64, HOST and HDAC has different
> memory view, so need to do address translation between HOST and HDAC.
> 
> Signed-off-by: Joakim Zhang <joakim.zhang@cixtech.com>
> ---
>  include/sound/hdaudio.h     |  3 +++
>  sound/hda/core/controller.c | 25 +++++++++++++++++++------
>  sound/hda/core/stream.c     | 17 ++++++++++++++---
>  3 files changed, 36 insertions(+), 9 deletions(-)
> 
> diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
> index 4e0c1d8af09f..61b41a014f4a 100644
> --- a/include/sound/hdaudio.h
> +++ b/include/sound/hdaudio.h
> @@ -293,6 +293,9 @@ struct hdac_bus {
>  	const struct hdac_bus_ops *ops;
>  	const struct hdac_ext_bus_ops *ext_ops;
>  
> +	/* address translation from host to hdac */
> +	dma_addr_t (*addr_host_to_hdac)(struct hdac_bus *bus, dma_addr_t addr);

This should be rather added to hdac_bus_ops instead.

Or, we can just add addr_offset field in hdac_bus instead of yet
another callback.  Then the change would be simpler.


thanks,

Takashi

RE: [PATCH V1 2/3] ALSA: hda: add bus callback for address translation

[Joakim Zhang]

> -----Original Message-----
> From: Takashi Iwai <tiwai@suse.de>
> Sent: Friday, October 31, 2025 7:28 PM
> To: Joakim Zhang <joakim.zhang@cixtech.com>
> Cc: lgirdwood@gmail.com; broonie@kernel.org; robh@kernel.org;
> krzk+dt@kernel.org; conor+dt@kernel.org; perex@perex.cz;
> tiwai@suse.com; linux-sound@vger.kernel.org; devicetree@vger.kernel.org;
> cix-kernel-upstream <cix-kernel-upstream@cixtech.com>
> Subject: Re: [PATCH V1 2/3] ALSA: hda: add bus callback for address
> translation
> 
> EXTERNAL EMAIL
> 
> CAUTION: Suspicious Email from unusual domain.
> 
> On Thu, 30 Oct 2025 12:09:27 +0100,
> joakim.zhang@cixtech.com wrote:
> >
> > From: Joakim Zhang <joakim.zhang@cixtech.com>
> >
> > This patch adds bus callback for address translation, for some SoCs
> > such as CIX SKY1 which is ARM64, HOST and HDAC has different memory
> > view, so need to do address translation between HOST and HDAC.
> >
> > Signed-off-by: Joakim Zhang <joakim.zhang@cixtech.com>
> > ---
> >  include/sound/hdaudio.h     |  3 +++
> >  sound/hda/core/controller.c | 25 +++++++++++++++++++------
> >  sound/hda/core/stream.c     | 17 ++++++++++++++---
> >  3 files changed, 36 insertions(+), 9 deletions(-)
> >
> > diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h index
> > 4e0c1d8af09f..61b41a014f4a 100644
> > --- a/include/sound/hdaudio.h
> > +++ b/include/sound/hdaudio.h
> > @@ -293,6 +293,9 @@ struct hdac_bus {
> >       const struct hdac_bus_ops *ops;
> >       const struct hdac_ext_bus_ops *ext_ops;
> >
> > +     /* address translation from host to hdac */
> > +     dma_addr_t (*addr_host_to_hdac)(struct hdac_bus *bus, dma_addr_t
> > + addr);
> 
> This should be rather added to hdac_bus_ops instead.
> 
> Or, we can just add addr_offset field in hdac_bus instead of yet another
> callback.  Then the change would be simpler.

Thanks Takashi, I will take a look.

Joakim
> 
> 
> thanks,
> 
> Takashi

[PATCH V1 3/3] ALSA: hda: add CIX IPBLOQ HDA controller support

[joakim.zhang]

From: Joakim Zhang <joakim.zhang@cixtech.com>

This patch adds support for CIX IPBLOQ HDA controller

Signed-off-by: Joakim Zhang <joakim.zhang@cixtech.com>
---
 sound/hda/controllers/Kconfig      |  13 +
 sound/hda/controllers/Makefile     |   2 +
 sound/hda/controllers/cix-ipbloq.c | 470 +++++++++++++++++++++++++++++
 3 files changed, 485 insertions(+)
 create mode 100644 sound/hda/controllers/cix-ipbloq.c

diff --git a/sound/hda/controllers/Kconfig b/sound/hda/controllers/Kconfig
index 34721f50b055..c6b0666ef18c 100644
--- a/sound/hda/controllers/Kconfig
+++ b/sound/hda/controllers/Kconfig
@@ -30,6 +30,19 @@ config SND_HDA_TEGRA
 	  To compile this driver as a module, choose M here: the module
 	  will be called snd-hda-tegra.
 
+config SND_HDA_CIX_IPBLOQ
+	tristate "CIX IPBLOQ HD Audio"
+	select SND_HDA
+	select SND_HDA_ALIGNED_MMIO
+	help
+	  Say Y here to support the HDA controller present in CIX SoCs
+
+	  This options enables support for the HD Audio controller
+	  present in some CIX SoCs.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called snd-hda-cix-ipbloq.
+
 config SND_HDA_ACPI
 	tristate "HD Audio ACPI"
 	depends on ACPI
diff --git a/sound/hda/controllers/Makefile b/sound/hda/controllers/Makefile
index a4bcd055e9ae..8967b6771d90 100644
--- a/sound/hda/controllers/Makefile
+++ b/sound/hda/controllers/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 snd-hda-intel-y := intel.o
 snd-hda-tegra-y := tegra.o
+snd-hda-cix-ipbloq-y := cix-ipbloq.o
 snd-hda-acpi-y := acpi.o
 
 subdir-ccflags-y += -I$(src)/../common
@@ -10,4 +11,5 @@ CFLAGS_intel.o := -I$(src)
 
 obj-$(CONFIG_SND_HDA_INTEL) += snd-hda-intel.o
 obj-$(CONFIG_SND_HDA_TEGRA) += snd-hda-tegra.o
+obj-$(CONFIG_SND_HDA_CIX_IPBLOQ) += snd-hda-cix-ipbloq.o
 obj-$(CONFIG_SND_HDA_ACPI) += snd-hda-acpi.o
diff --git a/sound/hda/controllers/cix-ipbloq.c b/sound/hda/controllers/cix-ipbloq.c
new file mode 100644
index 000000000000..8f6c9ff09950
--- /dev/null
+++ b/sound/hda/controllers/cix-ipbloq.c
@@ -0,0 +1,470 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright 2025 Cix Technology Group Co., Ltd.
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of.h>
+#include <linux/of_reserved_mem.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/reset.h>
+#include <linux/string.h>
+
+#include <sound/hda_codec.h>
+#include "hda_controller.h"
+
+#define CIX_IPBLOQ_JACKPOLL_DEFAULT_TIME_MS		1000
+#define CIX_IPBLOQ_POWER_SAVE_DEFAULT_TIME_MS		100
+
+#define CIX_IPBLOQ_SKY1_ADDR_HOST_TO_HDAC_OFFSET	0x90000000
+
+struct cix_ipbloq_hda {
+	struct azx chip;
+	struct device *dev;
+	void __iomem *regs;
+
+	struct reset_control_bulk_data resets[1];
+	struct clk_bulk_data clocks[2];
+	unsigned int nresets;
+	unsigned int nclocks;
+
+	struct work_struct probe_work;
+};
+
+static const struct hda_controller_ops cix_ipbloq_hda_ops;
+
+static dma_addr_t cix_ipbloq_hda_addr_host_to_hdac(struct hdac_bus *bus, dma_addr_t addr)
+{
+	dma_addr_t addr_adj;
+
+	addr_adj = addr - CIX_IPBLOQ_SKY1_ADDR_HOST_TO_HDAC_OFFSET;
+
+	return addr_adj;
+}
+
+static int cix_ipbloq_hda_dev_disconnect(struct snd_device *device)
+{
+	struct azx *chip = device->device_data;
+
+	chip->bus.shutdown = 1;
+
+	return 0;
+}
+
+static int cix_ipbloq_hda_dev_free(struct snd_device *device)
+{
+	struct azx *chip = device->device_data;
+	struct cix_ipbloq_hda *hda = container_of(chip, struct cix_ipbloq_hda, chip);
+
+	cancel_work_sync(&hda->probe_work);
+
+	if (azx_bus(chip)->chip_init) {
+		azx_stop_all_streams(chip);
+		azx_stop_chip(chip);
+	}
+
+	azx_free_stream_pages(chip);
+	azx_free_streams(chip);
+	snd_hdac_bus_exit(azx_bus(chip));
+
+	return 0;
+}
+
+static int cix_ipbloq_hda_init_chip(struct azx *chip, struct platform_device *pdev)
+{
+	struct cix_ipbloq_hda *hda = container_of(chip, struct cix_ipbloq_hda, chip);
+	struct hdac_bus *bus = azx_bus(chip);
+	struct resource *res;
+
+	hda->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
+	if (IS_ERR(hda->regs)) {
+		dev_err(&pdev->dev, "failed to get and ioremap resource\n");
+		return PTR_ERR(hda->regs);
+	}
+
+	bus->remap_addr = hda->regs;
+	bus->addr = res->start;
+
+	return 0;
+}
+
+static int cix_ipbloq_hda_init(struct azx *chip, struct platform_device *pdev)
+{
+	struct hdac_bus *bus = azx_bus(chip);
+	struct snd_card *card = chip->card;
+	const char *sname = NULL, *drv_name = "cix-ipbloq-hda";
+	unsigned short gcap;
+	int irq_id, err;
+
+	err = cix_ipbloq_hda_init_chip(chip, pdev);
+	if (err)
+		return err;
+
+	irq_id = platform_get_irq(pdev, 0);
+	if (irq_id < 0) {
+		dev_err(&pdev->dev, "failed to get the irq\n");
+		return irq_id;
+	}
+
+	err = devm_request_irq(chip->card->dev, irq_id, azx_interrupt,
+			     IRQF_SHARED, KBUILD_MODNAME, chip);
+	if (err) {
+		dev_err(chip->card->dev,
+			"unable to request IRQ %d, disabling device\n",
+			irq_id);
+		return err;
+	}
+
+	bus->irq = irq_id;
+	bus->dma_stop_delay = 100;
+	card->sync_irq = bus->irq;
+
+	gcap = azx_readw(chip, GCAP);
+
+	chip->capture_streams = (gcap >> 8) & 0x0f;
+	chip->playback_streams = (gcap >> 12) & 0x0f;
+	chip->capture_index_offset = 0;
+	chip->playback_index_offset = chip->capture_streams;
+	chip->num_streams = chip->playback_streams + chip->capture_streams;
+
+	/* initialize streams */
+	err = azx_init_streams(chip);
+	if (err < 0) {
+		dev_err(card->dev, "failed to initialize streams: %d\n", err);
+		return err;
+	}
+
+	err = azx_alloc_stream_pages(chip);
+	if (err < 0) {
+		dev_err(card->dev, "failed to allocate stream pages: %d\n", err);
+		return err;
+	}
+
+	/* initialize chip */
+	azx_init_chip(chip, 1);
+
+	/* codec detection */
+	if (!bus->codec_mask) {
+		dev_err(card->dev, "no codecs found\n");
+		return -ENODEV;
+	}
+	dev_info(card->dev, "codec detection mask = 0x%lx\n", bus->codec_mask);
+
+	/* driver name */
+	strscpy(card->driver, drv_name, sizeof(card->driver));
+
+	/* shortname for card */
+	sname = of_get_property(pdev->dev.of_node, "cix,model", NULL);
+	if (!sname)
+		sname = drv_name;
+	if (strlen(sname) > sizeof(card->shortname))
+		dev_info(card->dev, "truncating shortname for card\n");
+	strscpy(card->shortname, sname, sizeof(card->shortname));
+
+	/* longname for card */
+	snprintf(card->longname, sizeof(card->longname),
+		 "%s at 0x%lx irq %i",
+		 card->shortname, bus->addr, bus->irq);
+
+	return 0;
+}
+
+static void cix_ipbloq_hda_probe_work(struct work_struct *work)
+{
+	struct cix_ipbloq_hda *hda = container_of(work, struct cix_ipbloq_hda, probe_work);
+	struct platform_device *pdev = to_platform_device(hda->dev);
+	struct azx *chip = &hda->chip;
+	struct hdac_bus *bus = azx_bus(chip);
+	int err;
+
+	err = pm_runtime_resume_and_get(hda->dev);
+	if (err < 0) {
+		dev_err(hda->dev, "pm_runtime_resume_and_get failed, err = %d\n", err);
+		return;
+	}
+
+	to_hda_bus(bus)->bus_probing = 1;
+
+	err = cix_ipbloq_hda_init(chip, pdev);
+	if (err < 0)
+		goto out_free;
+
+	/* create codec instances */
+	err = azx_probe_codecs(chip, 8);
+	if (err < 0)
+		goto out_free;
+
+	err = azx_codec_configure(chip);
+	if (err < 0)
+		goto out_free;
+
+	err = snd_card_register(chip->card);
+	if (err < 0)
+		goto out_free;
+
+	chip->running = 1;
+
+	to_hda_bus(bus)->bus_probing = 0;
+
+	snd_hda_set_power_save(&chip->bus, CIX_IPBLOQ_POWER_SAVE_DEFAULT_TIME_MS);
+
+	dev_info(hda->dev, "cix ipbloq hda probed\n");
+
+ out_free:
+	pm_runtime_put(hda->dev);
+	return; /* no error return from async probe */
+}
+
+static int cix_ipbloq_hda_create(struct snd_card *card,
+				 unsigned int driver_caps,
+				 struct cix_ipbloq_hda *hda)
+{
+	static const struct snd_device_ops ops = {
+		.dev_disconnect = cix_ipbloq_hda_dev_disconnect,
+		.dev_free = cix_ipbloq_hda_dev_free,
+	};
+	struct azx *chip;
+	int err;
+
+	chip = &hda->chip;
+
+	mutex_init(&chip->open_mutex);
+	chip->card = card;
+	chip->ops = &cix_ipbloq_hda_ops;
+	chip->driver_caps = driver_caps;
+	chip->driver_type = driver_caps & 0xff;
+	chip->dev_index = 0;
+	chip->single_cmd = 0;
+	chip->codec_probe_mask = -1;
+	chip->align_buffer_size = 1;
+	chip->jackpoll_interval = msecs_to_jiffies(CIX_IPBLOQ_JACKPOLL_DEFAULT_TIME_MS);
+	INIT_LIST_HEAD(&chip->pcm_list);
+
+	/*
+	 * HD-audio controllers appear pretty inaccurate about the update-IRQ timing.
+	 * The IRQ is issued before actually the data is processed. So use stream
+	 * link position by default instead of dma position buffer.
+	 */
+	chip->get_position[0] = chip->get_position[1] = azx_get_pos_lpib;
+
+	INIT_WORK(&hda->probe_work, cix_ipbloq_hda_probe_work);
+
+	err = azx_bus_init(chip, NULL);
+	if (err < 0) {
+		dev_err(hda->dev, "failed to init bus, err = %d\n", err);
+		return err;
+	}
+
+	/* RIRBSTS.RINTFL cannot be cleared, cause interrupt storm */
+	chip->bus.core.polling_mode = 1;
+	chip->bus.core.not_use_interrupts = 1;
+
+	chip->bus.core.aligned_mmio = 1;
+	chip->bus.jackpoll_in_suspend = 1;
+
+	/* host and hdac has different memory view */
+	chip->bus.core.addr_host_to_hdac = cix_ipbloq_hda_addr_host_to_hdac;
+
+	err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops);
+	if (err < 0) {
+		dev_err(card->dev, "failed to create device, err = %d\n", err);
+		return err;
+	}
+
+	return 0;
+}
+
+static int cix_ipbloq_hda_probe(struct platform_device *pdev)
+{
+	const unsigned int driver_flags = AZX_DCAPS_PM_RUNTIME;
+	struct snd_card *card;
+	struct azx *chip;
+	struct cix_ipbloq_hda *hda;
+	int err;
+
+	hda = devm_kzalloc(&pdev->dev, sizeof(*hda), GFP_KERNEL);
+	if (!hda)
+		return -ENOMEM;
+
+	hda->dev = &pdev->dev;
+	chip = &hda->chip;
+
+	err = snd_card_new(&pdev->dev, SNDRV_DEFAULT_IDX1, SNDRV_DEFAULT_STR1,
+			   THIS_MODULE, 0, &card);
+	if (err < 0) {
+		dev_err(&pdev->dev, "failed to crate card, err = %d\n", err);
+		return err;
+	}
+
+	dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
+
+	err = of_reserved_mem_device_init(&pdev->dev);
+	if (err && err != -ENODEV) {
+		dev_err(&pdev->dev,
+			"failed to init reserved mem for DMA, err = %d\n", err);
+		goto out_free;
+	}
+
+	hda->resets[hda->nresets++].id = "hda";
+	err = devm_reset_control_bulk_get_exclusive(&pdev->dev, hda->nresets,
+						    hda->resets);
+	if (err) {
+		dev_err(&pdev->dev, "failed to get reset, err = %d\n", err);
+		goto out_free;
+	}
+
+	hda->clocks[hda->nclocks++].id = "sysclk";
+	hda->clocks[hda->nclocks++].id = "clk48m";
+	err = devm_clk_bulk_get(&pdev->dev, hda->nclocks, hda->clocks);
+	if (err < 0) {
+		dev_err(&pdev->dev, "failed to get clk, err = %d\n", err);
+		goto out_free;
+	}
+
+	err = cix_ipbloq_hda_create(card, driver_flags, hda);
+	if (err < 0)
+		goto out_free;
+	card->private_data = chip;
+
+	dev_set_drvdata(&pdev->dev, card);
+
+	pm_runtime_enable(hda->dev);
+	if (!azx_has_pm_runtime(chip))
+		pm_runtime_forbid(hda->dev);
+
+	schedule_work(&hda->probe_work);
+
+	return 0;
+
+out_free:
+	snd_card_free(card);
+	return err;
+}
+
+static void cix_ipbloq_hda_remove(struct platform_device *pdev)
+{
+	snd_card_free(dev_get_drvdata(&pdev->dev));
+
+	pm_runtime_disable(&pdev->dev);
+}
+
+static void cix_ipbloq_hda_shutdown(struct platform_device *pdev)
+{
+	struct snd_card *card = dev_get_drvdata(&pdev->dev);
+	struct azx *chip;
+
+	if (!card)
+		return;
+
+	chip = card->private_data;
+	if (chip && chip->running)
+		azx_stop_chip(chip);
+}
+
+static int __maybe_unused cix_ipbloq_hda_suspend(struct device *dev)
+{
+	struct snd_card *card = dev_get_drvdata(dev);
+	int rc;
+
+	rc = pm_runtime_force_suspend(dev);
+	if (rc < 0)
+		return rc;
+	snd_power_change_state(card, SNDRV_CTL_POWER_D3cold);
+
+	return 0;
+}
+
+static int __maybe_unused cix_ipbloq_hda_resume(struct device *dev)
+{
+	struct snd_card *card = dev_get_drvdata(dev);
+	int rc;
+
+	rc = pm_runtime_force_resume(dev);
+	if (rc < 0)
+		return rc;
+	snd_power_change_state(card, SNDRV_CTL_POWER_D0);
+
+	return 0;
+}
+
+static int __maybe_unused cix_ipbloq_hda_runtime_suspend(struct device *dev)
+{
+	struct snd_card *card = dev_get_drvdata(dev);
+	struct azx *chip = card->private_data;
+	struct cix_ipbloq_hda *hda = container_of(chip, struct cix_ipbloq_hda, chip);
+
+	if (chip && chip->running) {
+		azx_stop_chip(chip);
+		azx_enter_link_reset(chip);
+	}
+
+	clk_bulk_disable_unprepare(hda->nclocks, hda->clocks);
+
+	return 0;
+}
+
+static int __maybe_unused cix_ipbloq_hda_runtime_resume(struct device *dev)
+{
+	struct snd_card *card = dev_get_drvdata(dev);
+	struct azx *chip = card->private_data;
+	struct cix_ipbloq_hda *hda = container_of(chip, struct cix_ipbloq_hda, chip);
+	int rc;
+
+	rc = clk_bulk_prepare_enable(hda->nclocks, hda->clocks);
+	if (rc) {
+		dev_err(dev, "failed to enable clk bulk, rc: %d\n", rc);
+		return rc;
+	}
+
+	rc = reset_control_bulk_assert(hda->nresets, hda->resets);
+	if (rc) {
+		dev_err(dev, "failed to assert reset bulk, rc: %d\n", rc);
+		return rc;
+	}
+
+	rc = reset_control_bulk_deassert(hda->nresets, hda->resets);
+	if (rc) {
+		dev_err(dev, "failed to deassert reset bulk, rc: %d\n", rc);
+		return rc;
+	}
+
+	if (chip && chip->running)
+		azx_init_chip(chip, 1);
+
+	return 0;
+}
+
+static const struct dev_pm_ops cix_ipbloq_hda_pm = {
+	SET_SYSTEM_SLEEP_PM_OPS(cix_ipbloq_hda_suspend,
+				cix_ipbloq_hda_resume)
+	SET_RUNTIME_PM_OPS(cix_ipbloq_hda_runtime_suspend,
+			   cix_ipbloq_hda_runtime_resume, NULL)
+};
+
+static const struct of_device_id cix_ipbloq_hda_match[] = {
+	{ .compatible = "cix,sky1-ipbloq-hda" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, cix_ipbloq_hda_match);
+
+static struct platform_driver cix_ipbloq_hda_driver = {
+	.driver = {
+		.name = "cix-ipbloq-hda",
+		.pm = &cix_ipbloq_hda_pm,
+		.of_match_table = cix_ipbloq_hda_match,
+	},
+	.probe = cix_ipbloq_hda_probe,
+	.remove = cix_ipbloq_hda_remove,
+	.shutdown = cix_ipbloq_hda_shutdown,
+};
+module_platform_driver(cix_ipbloq_hda_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("CIX IPBLOQ HDA bus driver");
+MODULE_AUTHOR("Joakim Zhang <joakim.zhang@cixtech.com>");
-- 
2.49.0

Re: [PATCH V1 3/3] ALSA: hda: add CIX IPBLOQ HDA controller support

[Krzysztof Kozlowski]

On Thu, Oct 30, 2025 at 07:09:28PM +0800, joakim.zhang@cixtech.com wrote:
> From: Joakim Zhang <joakim.zhang@cixtech.com>
> 
> This patch adds support for CIX IPBLOQ HDA controller

Please do not use "This commit/patch/change", but imperative mood. See
longer explanation here:
https://elixir.bootlin.com/linux/v6.16/source/Documentation/process/submitting-patches.rst#L94

> 
> Signed-off-by: Joakim Zhang <joakim.zhang@cixtech.com>
> ---
>  sound/hda/controllers/Kconfig      |  13 +
>  sound/hda/controllers/Makefile     |   2 +
>  sound/hda/controllers/cix-ipbloq.c | 470 +++++++++++++++++++++++++++++
>  3 files changed, 485 insertions(+)
>  create mode 100644 sound/hda/controllers/cix-ipbloq.c
> 
> diff --git a/sound/hda/controllers/Kconfig b/sound/hda/controllers/Kconfig
> index 34721f50b055..c6b0666ef18c 100644
> --- a/sound/hda/controllers/Kconfig
> +++ b/sound/hda/controllers/Kconfig
> @@ -30,6 +30,19 @@ config SND_HDA_TEGRA
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called snd-hda-tegra.
>  
> +config SND_HDA_CIX_IPBLOQ
> +	tristate "CIX IPBLOQ HD Audio"
> +	select SND_HDA
> +	select SND_HDA_ALIGNED_MMIO

If this is not usable outside of ARCH CIX then add dependency with
COMPIEL_TEST

> +	help
> +	  Say Y here to support the HDA controller present in CIX SoCs
> +

...

> +static int cix_ipbloq_hda_init(struct azx *chip, struct platform_device *pdev)
> +{
> +	struct hdac_bus *bus = azx_bus(chip);
> +	struct snd_card *card = chip->card;
> +	const char *sname = NULL, *drv_name = "cix-ipbloq-hda";
> +	unsigned short gcap;
> +	int irq_id, err;
> +
> +	err = cix_ipbloq_hda_init_chip(chip, pdev);
> +	if (err)
> +		return err;
> +
> +	irq_id = platform_get_irq(pdev, 0);
> +	if (irq_id < 0) {
> +		dev_err(&pdev->dev, "failed to get the irq\n");
> +		return irq_id;
> +	}
> +
> +	err = devm_request_irq(chip->card->dev, irq_id, azx_interrupt,
> +			     IRQF_SHARED, KBUILD_MODNAME, chip);

You request shared interrupt with devm, which is known patrern for bugs.
You need to carefully investigate it, e.g. provide rationale why this is
safe, or drop devm or shared.

> +	if (err) {
> +		dev_err(chip->card->dev,
> +			"unable to request IRQ %d, disabling device\n",

Why are you requesting resources outside of probe, in work item? You
cannot handle here deferred probe.

How is it supposed to work? How deferred probe would work?

> +			irq_id);
> +		return err;
> +	}
> +
> +	bus->irq = irq_id;
> +	bus->dma_stop_delay = 100;
> +	card->sync_irq = bus->irq;
> +
> +	gcap = azx_readw(chip, GCAP);
> +
> +	chip->capture_streams = (gcap >> 8) & 0x0f;
> +	chip->playback_streams = (gcap >> 12) & 0x0f;
> +	chip->capture_index_offset = 0;
> +	chip->playback_index_offset = chip->capture_streams;
> +	chip->num_streams = chip->playback_streams + chip->capture_streams;
> +
> +	/* initialize streams */
> +	err = azx_init_streams(chip);
> +	if (err < 0) {
> +		dev_err(card->dev, "failed to initialize streams: %d\n", err);
> +		return err;
> +	}
> +
> +	err = azx_alloc_stream_pages(chip);
> +	if (err < 0) {
> +		dev_err(card->dev, "failed to allocate stream pages: %d\n", err);
> +		return err;
> +	}
> +
> +	/* initialize chip */
> +	azx_init_chip(chip, 1);
> +
> +	/* codec detection */
> +	if (!bus->codec_mask) {
> +		dev_err(card->dev, "no codecs found\n");
> +		return -ENODEV;
> +	}
> +	dev_info(card->dev, "codec detection mask = 0x%lx\n", bus->codec_mask);

I guess dev_dbg. Although see Coding style and driver debugging guides.

https://elixir.bootlin.com/linux/v6.15-rc7/source/Documentation/process/coding-style.rst#L913

https://elixir.bootlin.com/linux/v6.15-rc7/source/Documentation/process/debugging/driver_development_debugging_guide.rst#L79

> +
> +	/* driver name */
> +	strscpy(card->driver, drv_name, sizeof(card->driver));
> +
> +	/* shortname for card */
> +	sname = of_get_property(pdev->dev.of_node, "cix,model", NULL);
> +	if (!sname)
> +		sname = drv_name;
> +	if (strlen(sname) > sizeof(card->shortname))
> +		dev_info(card->dev, "truncating shortname for card\n");
> +	strscpy(card->shortname, sname, sizeof(card->shortname));
> +
> +	/* longname for card */
> +	snprintf(card->longname, sizeof(card->longname),
> +		 "%s at 0x%lx irq %i",
> +		 card->shortname, bus->addr, bus->irq);
> +
> +	return 0;
> +}
> +
> +static void cix_ipbloq_hda_probe_work(struct work_struct *work)
> +{
> +	struct cix_ipbloq_hda *hda = container_of(work, struct cix_ipbloq_hda, probe_work);
> +	struct platform_device *pdev = to_platform_device(hda->dev);
> +	struct azx *chip = &hda->chip;
> +	struct hdac_bus *bus = azx_bus(chip);
> +	int err;
> +
> +	err = pm_runtime_resume_and_get(hda->dev);
> +	if (err < 0) {
> +		dev_err(hda->dev, "pm_runtime_resume_and_get failed, err = %d\n", err);
> +		return;
> +	}
> +
> +	to_hda_bus(bus)->bus_probing = 1;
> +
> +	err = cix_ipbloq_hda_init(chip, pdev);
> +	if (err < 0)
> +		goto out_free;
> +
> +	/* create codec instances */
> +	err = azx_probe_codecs(chip, 8);
> +	if (err < 0)
> +		goto out_free;
> +
> +	err = azx_codec_configure(chip);
> +	if (err < 0)
> +		goto out_free;
> +
> +	err = snd_card_register(chip->card);
> +	if (err < 0)
> +		goto out_free;
> +
> +	chip->running = 1;
> +
> +	to_hda_bus(bus)->bus_probing = 0;
> +
> +	snd_hda_set_power_save(&chip->bus, CIX_IPBLOQ_POWER_SAVE_DEFAULT_TIME_MS);
> +
> +	dev_info(hda->dev, "cix ipbloq hda probed\n");

Drop, drivers should be silent on success. Linux kernel already provides
you facilities to know if this probed.


> +
> + out_free:
> +	pm_runtime_put(hda->dev);
> +	return; /* no error return from async probe */
> +}
> +
> +static int cix_ipbloq_hda_create(struct snd_card *card,
> +				 unsigned int driver_caps,
> +				 struct cix_ipbloq_hda *hda)
> +{
> +	static const struct snd_device_ops ops = {
> +		.dev_disconnect = cix_ipbloq_hda_dev_disconnect,
> +		.dev_free = cix_ipbloq_hda_dev_free,
> +	};
> +	struct azx *chip;
> +	int err;
> +
> +	chip = &hda->chip;
> +
> +	mutex_init(&chip->open_mutex);
> +	chip->card = card;
> +	chip->ops = &cix_ipbloq_hda_ops;
> +	chip->driver_caps = driver_caps;
> +	chip->driver_type = driver_caps & 0xff;
> +	chip->dev_index = 0;
> +	chip->single_cmd = 0;
> +	chip->codec_probe_mask = -1;
> +	chip->align_buffer_size = 1;
> +	chip->jackpoll_interval = msecs_to_jiffies(CIX_IPBLOQ_JACKPOLL_DEFAULT_TIME_MS);
> +	INIT_LIST_HEAD(&chip->pcm_list);
> +
> +	/*
> +	 * HD-audio controllers appear pretty inaccurate about the update-IRQ timing.
> +	 * The IRQ is issued before actually the data is processed. So use stream
> +	 * link position by default instead of dma position buffer.
> +	 */
> +	chip->get_position[0] = chip->get_position[1] = azx_get_pos_lpib;
> +
> +	INIT_WORK(&hda->probe_work, cix_ipbloq_hda_probe_work);
> +
> +	err = azx_bus_init(chip, NULL);
> +	if (err < 0) {
> +		dev_err(hda->dev, "failed to init bus, err = %d\n", err);
> +		return err;
> +	}
> +
> +	/* RIRBSTS.RINTFL cannot be cleared, cause interrupt storm */
> +	chip->bus.core.polling_mode = 1;
> +	chip->bus.core.not_use_interrupts = 1;
> +
> +	chip->bus.core.aligned_mmio = 1;
> +	chip->bus.jackpoll_in_suspend = 1;
> +
> +	/* host and hdac has different memory view */
> +	chip->bus.core.addr_host_to_hdac = cix_ipbloq_hda_addr_host_to_hdac;
> +
> +	err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops);
> +	if (err < 0) {
> +		dev_err(card->dev, "failed to create device, err = %d\n", err);
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
> +static int cix_ipbloq_hda_probe(struct platform_device *pdev)
> +{
> +	const unsigned int driver_flags = AZX_DCAPS_PM_RUNTIME;
> +	struct snd_card *card;
> +	struct azx *chip;
> +	struct cix_ipbloq_hda *hda;
> +	int err;
> +
> +	hda = devm_kzalloc(&pdev->dev, sizeof(*hda), GFP_KERNEL);
> +	if (!hda)
> +		return -ENOMEM;
> +
> +	hda->dev = &pdev->dev;
> +	chip = &hda->chip;
> +
> +	err = snd_card_new(&pdev->dev, SNDRV_DEFAULT_IDX1, SNDRV_DEFAULT_STR1,
> +			   THIS_MODULE, 0, &card);
> +	if (err < 0) {
> +		dev_err(&pdev->dev, "failed to crate card, err = %d\n", err);

No, syntax is return dev_err_probe.

> +		return err;
> +	}
> +
> +	dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> +
> +	err = of_reserved_mem_device_init(&pdev->dev);
> +	if (err && err != -ENODEV) {
> +		dev_err(&pdev->dev,
> +			"failed to init reserved mem for DMA, err = %d\n", err);
> +		goto out_free;
> +	}
> +
> +	hda->resets[hda->nresets++].id = "hda";
> +	err = devm_reset_control_bulk_get_exclusive(&pdev->dev, hda->nresets,
> +						    hda->resets);
> +	if (err) {
> +		dev_err(&pdev->dev, "failed to get reset, err = %d\n", err);

Use dev_err_probe, so you will not spam logs on defer.

> +		goto out_free;
> +	}
> +
> +	hda->clocks[hda->nclocks++].id = "sysclk";
> +	hda->clocks[hda->nclocks++].id = "clk48m";
> +	err = devm_clk_bulk_get(&pdev->dev, hda->nclocks, hda->clocks);
> +	if (err < 0) {
> +		dev_err(&pdev->dev, "failed to get clk, err = %d\n", err);

Same here. Please look at recent Linux kernel drivers how they do it.

> +		goto out_free;
> +	}

Best regards,
Krzysztof

Re: [PATCH V1 3/3] ALSA: hda: add CIX IPBLOQ HDA controller support

[Takashi Iwai]

On Fri, 31 Oct 2025 10:05:01 +0100,
Krzysztof Kozlowski wrote:
> 
> On Thu, Oct 30, 2025 at 07:09:28PM +0800, joakim.zhang@cixtech.com wrote:
> > From: Joakim Zhang <joakim.zhang@cixtech.com>
> > +	if (err) {
> > +		dev_err(chip->card->dev,
> > +			"unable to request IRQ %d, disabling device\n",
> 
> Why are you requesting resources outside of probe, in work item? You
> cannot handle here deferred probe.
> 
> How is it supposed to work? How deferred probe would work?

The deferred probe is used for snd-hda-intel as it may require either
the vga_switcheroo binding or the firmware loading before the actual
probe.

OTOH, in this driver case, *_hda_init() call might be better as the
part of the probe callback.  Though, we may still need some deferred
work for probing and configuring the codecs, i.e. azx_probe_codecs()
and azx_codec_configure() calls where explicit module bindings are
involved.

I guess snd-hda-tegra does like snd-hda-intel just because it followed
the code flow of snd-hda-intel.  But it doesn't mean that a new driver
should follow the same, too.  Doing the initialization in the probe
callback would be much easier to handle the errors, for example.


thanks,

Takashi

RE: [PATCH V1 3/3] ALSA: hda: add CIX IPBLOQ HDA controller support

[Joakim Zhang]

Hello,


> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Friday, October 31, 2025 5:05 PM
> To: Joakim Zhang <joakim.zhang@cixtech.com>
> Cc: lgirdwood@gmail.com; broonie@kernel.org; robh@kernel.org;
> krzk+dt@kernel.org; conor+dt@kernel.org; perex@perex.cz;
> tiwai@suse.com; linux-sound@vger.kernel.org; devicetree@vger.kernel.org;
> cix-kernel-upstream <cix-kernel-upstream@cixtech.com>
> Subject: Re: [PATCH V1 3/3] ALSA: hda: add CIX IPBLOQ HDA controller
> support
> 
> EXTERNAL EMAIL
> 
> On Thu, Oct 30, 2025 at 07:09:28PM +0800, joakim.zhang@cixtech.com
> wrote:
> > From: Joakim Zhang <joakim.zhang@cixtech.com>
> >
> > This patch adds support for CIX IPBLOQ HDA controller
> 
> Please do not use "This commit/patch/change", but imperative mood. See
> longer explanation here:
> https://elixir.bootlin.com/linux/v6.16/source/Documentation/process/submi
> tting-patches.rst#L94

OK

> >
> > Signed-off-by: Joakim Zhang <joakim.zhang@cixtech.com>
> > ---
> >  sound/hda/controllers/Kconfig      |  13 +
> >  sound/hda/controllers/Makefile     |   2 +
> >  sound/hda/controllers/cix-ipbloq.c | 470
> > +++++++++++++++++++++++++++++
> >  3 files changed, 485 insertions(+)
> >  create mode 100644 sound/hda/controllers/cix-ipbloq.c
> >
> > diff --git a/sound/hda/controllers/Kconfig
> > b/sound/hda/controllers/Kconfig index 34721f50b055..c6b0666ef18c
> > 100644
> > --- a/sound/hda/controllers/Kconfig
> > +++ b/sound/hda/controllers/Kconfig
> > @@ -30,6 +30,19 @@ config SND_HDA_TEGRA
> >         To compile this driver as a module, choose M here: the module
> >         will be called snd-hda-tegra.
> >
> > +config SND_HDA_CIX_IPBLOQ
> > +     tristate "CIX IPBLOQ HD Audio"
> > +     select SND_HDA
> > +     select SND_HDA_ALIGNED_MMIO
> 
> If this is not usable outside of ARCH CIX then add dependency with
> COMPIEL_TEST

OK

> > +     help
> > +       Say Y here to support the HDA controller present in CIX SoCs
> > +
> 
> ...
> 
> > +static int cix_ipbloq_hda_init(struct azx *chip, struct
> > +platform_device *pdev) {
> > +     struct hdac_bus *bus = azx_bus(chip);
> > +     struct snd_card *card = chip->card;
> > +     const char *sname = NULL, *drv_name = "cix-ipbloq-hda";
> > +     unsigned short gcap;
> > +     int irq_id, err;
> > +
> > +     err = cix_ipbloq_hda_init_chip(chip, pdev);
> > +     if (err)
> > +             return err;
> > +
> > +     irq_id = platform_get_irq(pdev, 0);
> > +     if (irq_id < 0) {
> > +             dev_err(&pdev->dev, "failed to get the irq\n");
> > +             return irq_id;
> > +     }
> > +
> > +     err = devm_request_irq(chip->card->dev, irq_id, azx_interrupt,
> > +                          IRQF_SHARED, KBUILD_MODNAME, chip);
> 
> You request shared interrupt with devm, which is known patrern for bugs.
> You need to carefully investigate it, e.g. provide rationale why this is safe, or
> drop devm or shared.
> 
> > +     if (err) {
> > +             dev_err(chip->card->dev,
> > +                     "unable to request IRQ %d, disabling device\n",
> 
> Why are you requesting resources outside of probe, in work item? You
> cannot handle here deferred probe.
> 
> How is it supposed to work? How deferred probe would work?

OK, will move to probe route.

> > +                     irq_id);
> > +             return err;
> > +     }
> > +
> > +     bus->irq = irq_id;
> > +     bus->dma_stop_delay = 100;
> > +     card->sync_irq = bus->irq;
> > +
> > +     gcap = azx_readw(chip, GCAP);
> > +
> > +     chip->capture_streams = (gcap >> 8) & 0x0f;
> > +     chip->playback_streams = (gcap >> 12) & 0x0f;
> > +     chip->capture_index_offset = 0;
> > +     chip->playback_index_offset = chip->capture_streams;
> > +     chip->num_streams = chip->playback_streams +
> > + chip->capture_streams;
> > +
> > +     /* initialize streams */
> > +     err = azx_init_streams(chip);
> > +     if (err < 0) {
> > +             dev_err(card->dev, "failed to initialize streams: %d\n", err);
> > +             return err;
> > +     }
> > +
> > +     err = azx_alloc_stream_pages(chip);
> > +     if (err < 0) {
> > +             dev_err(card->dev, "failed to allocate stream pages: %d\n", err);
> > +             return err;
> > +     }
> > +
> > +     /* initialize chip */
> > +     azx_init_chip(chip, 1);
> > +
> > +     /* codec detection */
> > +     if (!bus->codec_mask) {
> > +             dev_err(card->dev, "no codecs found\n");
> > +             return -ENODEV;
> > +     }
> > +     dev_info(card->dev, "codec detection mask = 0x%lx\n",
> > + bus->codec_mask);
> 
> I guess dev_dbg. Although see Coding style and driver debugging guides.

OK

> 
> https://elixir.bootlin.com/linux/v6.15-
> rc7/source/Documentation/process/coding-style.rst#L913
> 
> https://elixir.bootlin.com/linux/v6.15-
> rc7/source/Documentation/process/debugging/driver_development_debuggi
> ng_guide.rst#L79
> 
> > +
> > +     /* driver name */
> > +     strscpy(card->driver, drv_name, sizeof(card->driver));
> > +
> > +     /* shortname for card */
> > +     sname = of_get_property(pdev->dev.of_node, "cix,model", NULL);
> > +     if (!sname)
> > +             sname = drv_name;
> > +     if (strlen(sname) > sizeof(card->shortname))
> > +             dev_info(card->dev, "truncating shortname for card\n");
> > +     strscpy(card->shortname, sname, sizeof(card->shortname));
> > +
> > +     /* longname for card */
> > +     snprintf(card->longname, sizeof(card->longname),
> > +              "%s at 0x%lx irq %i",
> > +              card->shortname, bus->addr, bus->irq);
> > +
> > +     return 0;
> > +}
> > +
> > +static void cix_ipbloq_hda_probe_work(struct work_struct *work) {
> > +     struct cix_ipbloq_hda *hda = container_of(work, struct cix_ipbloq_hda,
> probe_work);
> > +     struct platform_device *pdev = to_platform_device(hda->dev);
> > +     struct azx *chip = &hda->chip;
> > +     struct hdac_bus *bus = azx_bus(chip);
> > +     int err;
> > +
> > +     err = pm_runtime_resume_and_get(hda->dev);
> > +     if (err < 0) {
> > +             dev_err(hda->dev, "pm_runtime_resume_and_get failed, err
> = %d\n", err);
> > +             return;
> > +     }
> > +
> > +     to_hda_bus(bus)->bus_probing = 1;
> > +
> > +     err = cix_ipbloq_hda_init(chip, pdev);
> > +     if (err < 0)
> > +             goto out_free;
> > +
> > +     /* create codec instances */
> > +     err = azx_probe_codecs(chip, 8);
> > +     if (err < 0)
> > +             goto out_free;
> > +
> > +     err = azx_codec_configure(chip);
> > +     if (err < 0)
> > +             goto out_free;
> > +
> > +     err = snd_card_register(chip->card);
> > +     if (err < 0)
> > +             goto out_free;
> > +
> > +     chip->running = 1;
> > +
> > +     to_hda_bus(bus)->bus_probing = 0;
> > +
> > +     snd_hda_set_power_save(&chip->bus,
> > + CIX_IPBLOQ_POWER_SAVE_DEFAULT_TIME_MS);
> > +
> > +     dev_info(hda->dev, "cix ipbloq hda probed\n");
> 
> Drop, drivers should be silent on success. Linux kernel already provides you
> facilities to know if this probed.
>

OK

> 
> > +
> > + out_free:
> > +     pm_runtime_put(hda->dev);
> > +     return; /* no error return from async probe */ }
> > +
> > +static int cix_ipbloq_hda_create(struct snd_card *card,
> > +                              unsigned int driver_caps,
> > +                              struct cix_ipbloq_hda *hda) {
> > +     static const struct snd_device_ops ops = {
> > +             .dev_disconnect = cix_ipbloq_hda_dev_disconnect,
> > +             .dev_free = cix_ipbloq_hda_dev_free,
> > +     };
> > +     struct azx *chip;
> > +     int err;
> > +
> > +     chip = &hda->chip;
> > +
> > +     mutex_init(&chip->open_mutex);
> > +     chip->card = card;
> > +     chip->ops = &cix_ipbloq_hda_ops;
> > +     chip->driver_caps = driver_caps;
> > +     chip->driver_type = driver_caps & 0xff;
> > +     chip->dev_index = 0;
> > +     chip->single_cmd = 0;
> > +     chip->codec_probe_mask = -1;
> > +     chip->align_buffer_size = 1;
> > +     chip->jackpoll_interval =
> msecs_to_jiffies(CIX_IPBLOQ_JACKPOLL_DEFAULT_TIME_MS);
> > +     INIT_LIST_HEAD(&chip->pcm_list);
> > +
> > +     /*
> > +      * HD-audio controllers appear pretty inaccurate about the update-IRQ
> timing.
> > +      * The IRQ is issued before actually the data is processed. So use
> stream
> > +      * link position by default instead of dma position buffer.
> > +      */
> > +     chip->get_position[0] = chip->get_position[1] =
> > + azx_get_pos_lpib;
> > +
> > +     INIT_WORK(&hda->probe_work, cix_ipbloq_hda_probe_work);
> > +
> > +     err = azx_bus_init(chip, NULL);
> > +     if (err < 0) {
> > +             dev_err(hda->dev, "failed to init bus, err = %d\n", err);
> > +             return err;
> > +     }
> > +
> > +     /* RIRBSTS.RINTFL cannot be cleared, cause interrupt storm */
> > +     chip->bus.core.polling_mode = 1;
> > +     chip->bus.core.not_use_interrupts = 1;
> > +
> > +     chip->bus.core.aligned_mmio = 1;
> > +     chip->bus.jackpoll_in_suspend = 1;
> > +
> > +     /* host and hdac has different memory view */
> > +     chip->bus.core.addr_host_to_hdac =
> > + cix_ipbloq_hda_addr_host_to_hdac;
> > +
> > +     err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops);
> > +     if (err < 0) {
> > +             dev_err(card->dev, "failed to create device, err = %d\n", err);
> > +             return err;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int cix_ipbloq_hda_probe(struct platform_device *pdev) {
> > +     const unsigned int driver_flags = AZX_DCAPS_PM_RUNTIME;
> > +     struct snd_card *card;
> > +     struct azx *chip;
> > +     struct cix_ipbloq_hda *hda;
> > +     int err;
> > +
> > +     hda = devm_kzalloc(&pdev->dev, sizeof(*hda), GFP_KERNEL);
> > +     if (!hda)
> > +             return -ENOMEM;
> > +
> > +     hda->dev = &pdev->dev;
> > +     chip = &hda->chip;
> > +
> > +     err = snd_card_new(&pdev->dev, SNDRV_DEFAULT_IDX1,
> SNDRV_DEFAULT_STR1,
> > +                        THIS_MODULE, 0, &card);
> > +     if (err < 0) {
> > +             dev_err(&pdev->dev, "failed to crate card, err = %d\n",
> > + err);
> 
> No, syntax is return dev_err_probe.

OK

> > +             return err;
> > +     }
> > +
> > +     dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> > +
> > +     err = of_reserved_mem_device_init(&pdev->dev);
> > +     if (err && err != -ENODEV) {
> > +             dev_err(&pdev->dev,
> > +                     "failed to init reserved mem for DMA, err = %d\n", err);
> > +             goto out_free;
> > +     }
> > +
> > +     hda->resets[hda->nresets++].id = "hda";
> > +     err = devm_reset_control_bulk_get_exclusive(&pdev->dev, hda-
> >nresets,
> > +                                                 hda->resets);
> > +     if (err) {
> > +             dev_err(&pdev->dev, "failed to get reset, err = %d\n",
> > + err);
> 
> Use dev_err_probe, so you will not spam logs on defer.

OK

> > +             goto out_free;
> > +     }
> > +
> > +     hda->clocks[hda->nclocks++].id = "sysclk";
> > +     hda->clocks[hda->nclocks++].id = "clk48m";
> > +     err = devm_clk_bulk_get(&pdev->dev, hda->nclocks, hda->clocks);
> > +     if (err < 0) {
> > +             dev_err(&pdev->dev, "failed to get clk, err = %d\n",
> > + err);
> 
> Same here. Please look at recent Linux kernel drivers how they do it.

OK

> > +             goto out_free;
> > +     }
> 
> Best regards,
> Krzysztof