Re: [PATCH v6 03/10] ASoC: qcom: Add QAIF hardware register map

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Incorrect register offset for QAIF_EE_CODEC_WRDMA_MAP_REG due to likely hex addition error.
- [Low] Missing '_REG' suffix on ERR_RSP_IRQ_RAW_STAT macros.
- [Low] Misleading parameter name `i` in IRQ register macros creates an API trap.
--

commit 408d3c905e0252b19f0ed0b72abd0544e15097a3
Author: Harendra Gautam <harendra.gautam@oss.qualcomm.com>

ASoC: qcom: Add QAIF hardware register map

Add qaif-reg.h to define the MMIO register offsets for the Qualcomm Audio
Interface (QAIF) hardware block. This driver needs a common register map
to program the core registers, audio interfaces, DMA channels, interrupts, 
and SID maps based on the DAI ID.

> diff --git a/sound/soc/qcom/qaif-reg.h b/sound/soc/qcom/qaif-reg.h
> new file mode 100644
> index 0000000000000..84a9c0285289e
> --- /dev/null
> +++ b/sound/soc/qcom/qaif-reg.h

[ ... ]

> +#define QAIF_EE_RDDMA_MAP_REG(v)			(0x1920 + (0x1000 * ((v)->ee)))
> +#define QAIF_EE_WRDMA_MAP_REG(v)			(0x1940 + (0x1000 * ((v)->ee)))
> +#define QAIF_EE_INTF_MAP_REG(v)				(0x1960 + (0x1000 * ((v)->ee)))
> +#define QAIF_EE_CODEC_RDDMA_MAP_REG(v)			(0x1980 + (0x1000 * ((v)->ee)))
> +#define QAIF_EE_CODEC_WRDMA_MAP_REG(v)			(0x1A00 + (0x1000 * ((v)->ee)))

[Severity: Medium]
Is this offset correct? The previous registers in this block use a stride of
0x20, which would make the next offset 0x19A0. If this is a hexadecimal
addition error (mixing up 0x80 + 0x20 = 0xA0 with decimal 80 + 20 = 100 ->
0x100), writing to 0x1A00 could silently fail to map codec capture channels
and potentially corrupt unrelated hardware state during device probe.

[ ... ]

> +/* RDDMA Period Interrupts */
> +#define QAIF_EE_RDDMA_PERIOD_IRQ_EN_REG(v, i)		qaif_rddma_irq_reg_addr(v, i, 0x00)

[Severity: Low]
Does the parameter name i create a potential API trap here? In this macro,
i is passed as the dma_type parameter to qaif_rddma_irq_reg_addr(). Using
i typically implies a channel index, which might lead future developers to
accidentally pass a DMA channel index instead of the expected qaif_irq_type.

> +#define QAIF_EE_RDDMA_PERIOD_IRQ_STAT_REG(v, i)		qaif_rddma_irq_reg_addr(v, i, 0x08)
> +#define QAIF_EE_RDDMA_PERIOD_IRQ_RAW_STAT_REG(v, i)	qaif_rddma_irq_reg_addr(v, i, 0x10)
> +#define QAIF_EE_RDDMA_PERIOD_IRQ_CLR_REG(v, i)		qaif_rddma_irq_reg_addr(v, i, 0x18)
> +#define QAIF_EE_RDDMA_PERIOD_IRQ_FORCE_REG(v, i)	qaif_rddma_irq_reg_addr(v, i, 0x20)

[ ... ]

> +/* RDDMA Error Response Interrupts */
> +#define QAIF_EE_RDDMA_ERR_RSP_IRQ_EN_REG(v, i)		qaif_rddma_irq_reg_addr(v, i, 0x50)
> +#define QAIF_EE_RDDMA_ERR_RSP_IRQ_STAT_REG(v, i)	qaif_rddma_irq_reg_addr(v, i, 0x58)
> +#define QAIF_EE_RDDMA_ERR_RSP_IRQ_RAW_STAT(v, i)	qaif_rddma_irq_reg_addr(v, i, 0x60)

[Severity: Low]
Should this macro be named QAIF_EE_RDDMA_ERR_RSP_IRQ_RAW_STAT_REG? It
appears to be missing the _REG suffix used by the other macros in this file,
which could cause naming inconsistencies for future development.

> +#define QAIF_EE_RDDMA_ERR_RSP_IRQ_CLR_REG(v, i)		qaif_rddma_irq_reg_addr(v, i, 0x68)
> +#define QAIF_EE_RDDMA_ERR_RSP_IRQ_FORCE_REG(v, i)	qaif_rddma_irq_reg_addr(v, i, 0x70)

[ ... ]

> +/* WRDMA Error Response Interrupts */
> +#define QAIF_EE_WRDMA_ERR_RSP_IRQ_EN_REG(v, i)		qaif_wrdma_irq_reg_addr(v, i, 0x50)
> +#define QAIF_EE_WRDMA_ERR_RSP_IRQ_STAT_REG(v, i)	qaif_wrdma_irq_reg_addr(v, i, 0x58)
> +#define QAIF_EE_WRDMA_ERR_RSP_IRQ_RAW_STAT(v, i)	qaif_wrdma_irq_reg_addr(v, i, 0x60)

[Severity: Low]
Should this macro also be named QAIF_EE_WRDMA_ERR_RSP_IRQ_RAW_STAT_REG to
maintain the consistent _REG suffix convention?

> +#define QAIF_EE_WRDMA_ERR_RSP_IRQ_CLR_REG(v, i)		qaif_wrdma_irq_reg_addr(v, i, 0x68)
> +#define QAIF_EE_WRDMA_ERR_RSP_IRQ_FORCE_REG(v, i)	qaif_wrdma_irq_reg_addr(v, i, 0x70)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911-b4-qaif-shikra-audio-v6-0-e9addb358201@oss.qualcomm.com?part=3