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

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 <[email protected]>
> Signed-off-by: Frank Li <[email protected]>
> ---
> 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