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

[Vinod Koul <vkoul@kernel.org>]

On 18-12-25, 10: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
> +    adjust burst length or FIFO address.
> +
>    - peripheral_dma_vec: DMA an array of scatter gather buffers from/to a
>      peripheral. Similar to slave_sg, but uses an array of dma_vec
>      structures instead of a scatterlist.
> @@ -106,6 +110,11 @@ The details of these operations are:
>  		unsigned int sg_len, enum dma_data_direction direction,
>  		unsigned long flags);
>  
> +     struct dma_async_tx_descriptor *dmaengine_prep_config_sg(
> +		struct dma_chan *chan, struct scatterlist *sgl,
> +		unsigned int sg_len, enum dma_transfer_direction dir,
> +		unsigned long flags, struct dma_slave_config *config);
> +
>       struct dma_async_tx_descriptor *dmaengine_prep_peripheral_dma_vec(
>  		struct dma_chan *chan, const struct dma_vec *vecs,
>  		size_t nents, enum dma_data_direction direction,
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index 99efe2b9b4ea9844ca6161208362ef18ef111d96..276dca760f95e1131f5ff5bf69752c4c9cb1bcad 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -835,6 +835,8 @@ struct dma_filter {
>   *	where the address and size of each segment is located in one entry of
>   *	the dma_vec array.
>   * @device_prep_slave_sg: prepares a slave dma operation
> + *	(Deprecated, use @device_prep_config_sg)

Sorry that is _not_ deprecated, we are adding another way to do this in
a single shot

> + * @device_prep_config_sg: prepares a slave DMA operation with dma_slave_config
>   * @device_prep_dma_cyclic: prepare a cyclic dma operation suitable for audio.
>   *	The function takes a buffer of size buf_len. The callback function will
>   *	be called after period_len bytes have been transferred.
> @@ -934,6 +936,11 @@ struct dma_device {
>  		struct dma_chan *chan, struct scatterlist *sgl,
>  		unsigned int sg_len, enum dma_transfer_direction direction,
>  		unsigned long flags, void *context);
> +	struct dma_async_tx_descriptor *(*device_prep_config_sg)(
> +		struct dma_chan *chan, struct scatterlist *sgl,
> +		unsigned int sg_len, enum dma_transfer_direction direction,
> +		unsigned long flags, struct dma_slave_config *config,
> +		void *context);
>  	struct dma_async_tx_descriptor *(*device_prep_dma_cyclic)(
>  		struct dma_chan *chan, dma_addr_t buf_addr, size_t buf_len,
>  		size_t period_len, enum dma_transfer_direction direction,
> @@ -974,22 +981,85 @@ static inline bool is_slave_direction(enum dma_transfer_direction direction)
>  	       (direction == DMA_DEV_TO_DEV);
>  }
>  
> -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)
> +/*
> + * Re-entrancy and locking considerations for callers:
> + *
> + * dmaengine_prep_config_single(sg)_safe() is re-entrant and requires the
> + * DMA engine driver to implement device_prep_config_sg(). It returns NULL
> + * if device_prep_config_sg() is not implemented.
> + *
> + * The unsafe variant (without the _safe suffix) falls back to calling
> + * dmaengine_slave_config() and dmaengine_prep_slave_sg() separately.
> + * In this case, additional locking may be required, depending on the
> + * DMA consumer's usage.
> + */
> +static inline struct dma_async_tx_descriptor *
> +dmaengine_prep_config_sg_safe(struct dma_chan *chan, struct scatterlist *sgl,
> +	unsigned int sg_len, enum dma_transfer_direction dir,
> +	unsigned long flags, struct dma_slave_config *config)
> +{
> +	if (!chan || !chan->device || !chan->device->device_prep_config_sg)
> +		return NULL;
> +
> +	return chan->device->device_prep_config_sg(chan, sgl, sg_len,
> +						   dir, flags, config, NULL);
> +}
> +
> +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)

Agree with Damien, this could look better!

-- 
~Vinod