Re: [PATCH v2 5/9] dmaengine: stm32-dma3: Use bus width capability helpers

[Amelie Delaunay <amelie.delaunay@foss.st.com>]

Hi Nuno,

Thanks for the update to the stm32-dma3 driver. It's good to see that it 
helps demonstrate the dma_slave_caps_clear_xxx API.

For future patches touching STM32-related drivers, please CC the 
linux-stm32 mailing list, as this one went unnoticed on our side.

On 8/10/26 17:06, Nuno Sá wrote:
> Advertise the controller-wide bus width capabilities through the new
> dma_set_src_bus_widths() and dma_set_dst_bus_widths() helpers.
> 
> Also update the per-channel capability callback to clear unsupported
> widths through the dma_slave_caps helpers so the bitmap and
> transitional legacy fields stay in sync.
> 
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> ---
>   drivers/dma/stm32/stm32-dma3.c | 32 ++++++++++++++++++++------------
>   1 file changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/dma/stm32/stm32-dma3.c b/drivers/dma/stm32/stm32-dma3.c
> index 4724e7fa0008..f0781fb1b66f 100644
> --- a/drivers/dma/stm32/stm32-dma3.c
> +++ b/drivers/dma/stm32/stm32-dma3.c
> @@ -1469,14 +1469,14 @@ static void stm32_dma3_caps(struct dma_chan *c, struct dma_slave_caps *caps)
>   
>   	if (!chan->fifo_size) {
>   		caps->max_burst = 0;
> -		caps->src_addr_widths &= ~BIT(DMA_SLAVE_BUSWIDTH_8_BYTES);
> -		caps->dst_addr_widths &= ~BIT(DMA_SLAVE_BUSWIDTH_8_BYTES);
> +		dma_slave_caps_clear_src_width(caps, DMA_SLAVE_BUSWIDTH_8_BYTES);
> +		dma_slave_caps_clear_dst_width(caps, DMA_SLAVE_BUSWIDTH_8_BYTES);
>   	} else {
>   		/* Burst transfer should not exceed half of the fifo size */
>   		caps->max_burst = chan->max_burst;
>   		if (caps->max_burst < DMA_SLAVE_BUSWIDTH_8_BYTES) {
> -			caps->src_addr_widths &= ~BIT(DMA_SLAVE_BUSWIDTH_8_BYTES);
> -			caps->dst_addr_widths &= ~BIT(DMA_SLAVE_BUSWIDTH_8_BYTES);
> +			dma_slave_caps_clear_src_width(caps, DMA_SLAVE_BUSWIDTH_8_BYTES);
> +			dma_slave_caps_clear_dst_width(caps, DMA_SLAVE_BUSWIDTH_8_BYTES);
>   		}
>   	}
>   }
> @@ -1737,6 +1737,12 @@ static int stm32_dma3_probe(struct platform_device *pdev)
>   	u32 master_ports, chan_reserved, i, verr;
>   	u64 hwcfgr;
>   	int ret;
> +	enum dma_slave_buswidth stm32_dma3_buswidths[] = {
> +		DMA_SLAVE_BUSWIDTH_1_BYTE,
> +		DMA_SLAVE_BUSWIDTH_2_BYTES,
> +		DMA_SLAVE_BUSWIDTH_4_BYTES,
> +		DMA_SLAVE_BUSWIDTH_8_BYTES,
> +	};
>   

I'm not fond of having this large block after the "shortest" declaration 
(reversed christmas tree order), and since it is not a global variable, 
I think you can remove the `stm32_dma3` prefix.

By removing the `stm32_dma3` prefix, you could place it after the struct 
declarations, below struct dma_device *dma_dev;

>   	ddata = devm_kzalloc(&pdev->dev, sizeof(*ddata), GFP_KERNEL);
>   	if (!ddata)
> @@ -1775,14 +1781,16 @@ static int stm32_dma3_probe(struct platform_device *pdev)
>   	 * channel, and can only access address at even boundaries, multiple of the buswidth.
>   	 */
>   	dma_dev->copy_align = DMAENGINE_ALIGN_8_BYTES;
> -	dma_dev->src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
> -				   BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
> -				   BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
> -				   BIT(DMA_SLAVE_BUSWIDTH_8_BYTES);
> -	dma_dev->dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
> -				   BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
> -				   BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
> -				   BIT(DMA_SLAVE_BUSWIDTH_8_BYTES);
> +	ret = dma_set_src_bus_widths(dma_dev, stm32_dma3_buswidths,
> +				     ARRAY_SIZE(stm32_dma3_buswidths));

By using `buswidths` instead of `stm32_dma3_buswidths`, this can fit on 
a single line.

> +	if (ret)
> +		goto err_clk_disable;
> +
> +	ret = dma_set_dst_bus_widths(dma_dev, stm32_dma3_buswidths,
> +				     ARRAY_SIZE(stm32_dma3_buswidths));

Ditto

> +	if (ret)
> +		goto err_clk_disable;
> +
>   	dma_dev->directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV) | BIT(DMA_MEM_TO_MEM);
>   
>   	dma_dev->descriptor_reuse = true;
> 

Regards,
Amelie