Re: [PATCH v2 1/8] dmaengine: Add API to combine configuration and preparation (sg and single)

Damien Le Moal
2025-12-19T10:33:58Z

On 12/19/25 00:56, Frank Li wrote:
> Previously, configuration and preparation required two separate calls. This
> works well when configuration is done only once during initialization.
>
> However, in cases where the burst length or source/destination address must
> be adjusted for each transfer, calling two functions is verbose and
> requires additional locking to ensure both steps complete atomically.
>
> Add a new API dmaengine_prep_config_single() and dmaengine_prep_config_sg()
> and callback device_prep_config_sg() that combines configuration and
> preparation into a single operation. If the configuration argument is
> passed as NULL, fall back to the existing implementation.
>
> Add a new API dmaengine_prep_config_single_safe() and
> dmaengine_prep_config_sg_safe() for re-entrancy, which require driver
> implement callback device_prep_config_sg().
>
> Tested-by: Niklas Cassel <cassel@kernel.org>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> change in v2
> - add () for function
> - use short name device_prep_sg(), remove "slave" and "config". the 'slave'
> is reduntant. after remove slave, the function name is difference existed
> one, so remove _config suffix.
> ---
> Documentation/driver-api/dmaengine/client.rst | 9 +++
> include/linux/dmaengine.h | 103 ++++++++++++++++++++++++--
> 2 files changed, 105 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/driver-api/dmaengine/client.rst b/Documentation/driver-api/dmaengine/client.rst
> index d491e385d61a98b8a804cd823caf254a2dc62cf4..02c45b7d7a779421411eb9c68325cdedafcfe3b1 100644
> --- a/Documentation/driver-api/dmaengine/client.rst
> +++ b/Documentation/driver-api/dmaengine/client.rst
> @@ -80,6 +80,10 @@ The details of these operations are:
>
> - slave_sg: DMA a list of scatter gather buffers from/to a peripheral
>
> + - config_sg: Similar with slave_sg, just pass down dma_slave_config
> + struct to avoid call dmaengine_slave_config() every time if need

...struct to avoid calling dmaengine_slave_config() every time adjusting the
burst length or the FIFO address is needed.

> + adjust burst length or FIFO address.
> +

> +static inline struct dma_async_tx_descriptor *
> +dmaengine_prep_config_single_safe(struct dma_chan *chan, dma_addr_t buf,
> + size_t len, enum dma_transfer_direction dir, unsigned long flags,
> + struct dma_slave_config *config)
> {
> struct scatterlist sg;
> +
> sg_init_table(&sg, 1);
> sg_dma_address(&sg) = buf;
> sg_dma_len(&sg) = len;
>
> - if (!chan || !chan->device || !chan->device->device_prep_slave_sg)
> + if (!chan || !chan->device || !chan->device->device_prep_config_sg)
> + return NULL;

While at it, please move this arguments check before the sg initialization.
Otherwise, this is a little odd (argument checks generally are done first).

> +
> + return chan->device->device_prep_config_sg(chan, &sg, 1, dir,
> + flags, config, NULL);
> +}
> +
> +static inline struct dma_async_tx_descriptor *
> +dmaengine_prep_config_single(struct dma_chan *chan, dma_addr_t buf, size_t len,
> + enum dma_transfer_direction dir, unsigned long flags,
> + struct dma_slave_config *config)
> +{
> + struct scatterlist sg;
> +
> + sg_init_table(&sg, 1);
> + sg_dma_address(&sg) = buf;
> + sg_dma_len(&sg) = len;
> +
> + if (!chan || !chan->device)
> + return NULL;

Same here. Check these before initializing sg.

> +
> + if (chan->device->device_prep_config_sg)
> + return dmaengine_prep_config_sg_safe(chan, &sg, 1, dir,
> + flags, config);
> +
> + if (config)
> + if (dmaengine_slave_config(chan, config))
> + return NULL;
> +
> + if (!chan->device->device_prep_slave_sg)
> return NULL;
>
> return chan->device->device_prep_slave_sg(chan, &sg, 1,
> dir, flags, NULL);
> }
>
> +static inline struct dma_async_tx_descriptor *
> +dmaengine_prep_slave_single(struct dma_chan *chan, dma_addr_t buf, size_t len,
> + enum dma_transfer_direction dir,
> + unsigned long flags)
> +{
> + return dmaengine_prep_config_single(chan, buf, len, dir, flags, NULL);
> +}
> +
> /**
> * dmaengine_prep_peripheral_dma_vec() - Prepare a DMA scatter-gather descriptor
> * @chan: The channel to be used for this descriptor
> @@ -1009,17 +1079,36 @@ static inline struct dma_async_tx_descriptor *dmaengine_prep_peripheral_dma_vec(
> dir, flags);
> }
>
> -static inline struct dma_async_tx_descriptor *dmaengine_prep_slave_sg(
> +static inline struct dma_async_tx_descriptor *dmaengine_prep_config_sg(

Very odd line split. Please split this after the "*" of the return type.

> struct dma_chan *chan, struct scatterlist *sgl, unsigned int sg_len,
> - enum dma_transfer_direction dir, unsigned long flags)
> + enum dma_transfer_direction dir, unsigned long flags,
> + struct dma_slave_config *config)
> {
> - if (!chan || !chan->device || !chan->device->device_prep_slave_sg)
> + if (!chan || !chan->device)
> + return NULL;
> +
> + if (chan->device->device_prep_config_sg)
> + return dmaengine_prep_config_sg_safe(chan, sgl, sg_len,
> + dir, flags, config);
> +
> + if (config)
> + if (dmaengine_slave_config(chan, config))
> + return NULL;
> +
> + if (!chan->device->device_prep_slave_sg)
> return NULL;
>
> return chan->device->device_prep_slave_sg(chan, sgl, sg_len,
> dir, flags, NULL);
> }
>
> +static inline struct dma_async_tx_descriptor *dmaengine_prep_slave_sg(

Same comment here.

> + struct dma_chan *chan, struct scatterlist *sgl, unsigned int sg_len,
> + enum dma_transfer_direction dir, unsigned long flags)
> +{
> + return dmaengine_prep_config_sg(chan, sgl, sg_len, dir, flags, NULL);
> +}
> +
> #ifdef CONFIG_RAPIDIO_DMA_ENGINE
> struct rio_dma_ext;
> static inline struct dma_async_tx_descriptor *dmaengine_prep_rio_sg(
>

--
Damien Le Moal
Western Digital Research