Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Hardcoded channel-specific bitmask written to global registers during per-channel configuration.
--
--- Patch [2]: [PATCH v4 2/2] dmaengine: apple-admac: Add M3 generation ADMACs ---
commit 64ebea6bdd03b6d431f7550dfd48b4169f531022
Author: Sasha Finkelstein <[email protected]>
dmaengine: apple-admac: Add M3 generation ADMACs
This commit adds support for M3 generation ADMACs present on t8122 and t603x
SoCs. It introduces additional writes to unknown registers required for
these hardware variants to operate correctly.
> diff --git a/drivers/dma/apple-admac.c b/drivers/dma/apple-admac.c
> --- a/drivers/dma/apple-admac.c
> +++ b/drivers/dma/apple-admac.c
> @@ -39,10 +39,14 @@
>
> #define FLAG_DESC_NOTIFY BIT(16)
>
> +#define T8122_UNK_28_VAL 0x200000
> +
[ ... ]
> @@ -747,6 +756,11 @@ static int admac_device_config(struct dma_chan *chan,
> u32 bus_width = readl_relaxed(ad->base + REG_BUS_WIDTH(adchan->no)) &
> ~(BUS_WIDTH_WORD_SIZE | BUS_WIDTH_FRAME_SIZE);
>
> + if (ad->hw->set_unk28) {
> + writel_relaxed(T8122_UNK_28_VAL, ad->base + REG_UNK_28);
> + writel_relaxed(T8122_UNK_28_VAL, ad->base + REG_UNK_2C);
> + }
> +
[Severity: High]
Does this hardcode the configuration for a specific channel pair?
T8122_UNK_28_VAL is defined as 0x200000, which equals BIT(21). In this DMA
controller, global registers typically operate using a channel pair bitmask
calculated as 1 << (ch / 2), making BIT(21) correspond to channel pair 21
(channels 42 and 43).
Because this is called in the per-channel admac_device_config(),
configuring any channel will unconditionally write the bitmask for channel
pair 21 to these global registers.
Should this value be dynamically calculated based on adchan->no, or if it
is genuinely a one-time global constant, should it be written once during
admac_probe() instead?
--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.