Re: [PATCH 3/4] dmaengine: dw: Simplify prepare CTL_LO methods

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Tue, Apr 16, 2024 at 07:28:57PM +0300, Serge Semin wrote:
> Currently the CTL LO fields are calculated on the platform-specific basis.
> It's implemented by means of the prepare_ctllo() callbacks using the
> ternary operator within the local variables init block at the beginning of
> the block scope. The functions code currently is relatively hard to
> comprehend and isn't that optimal since implies four conditional
> statements executed and two additional local variables defined. Let's
> simplify the DW AHB DMA prepare_ctllo() method by unrolling the ternary
> operators into the normal if-else statement, dropping redundant
> master-interface ID variables and initializing the local variables based
> on the singly evaluated DMA-transfer direction check. Thus the method will
> look much more readable since now the fields content can be easily
> inferred right from the if-else branch. Provide the same update in the
> Intel DMA32 core driver for sake of the driver code unification.
> 
> Note besides of the effects described above this update is basically a
> preparation before dropping the max burst encoding callback. It will
> require calling the burst fields calculation methods right in the
> prepare_ctllo() callbacks, which would have made the later function code
> even more complex.

Yeah, this is inherited from the original driver where it used to be a macro.

...

> +	if (dwc->direction == DMA_MEM_TO_DEV) {
> +		sms = dwc->dws.m_master;
> +		smsize = 0;
> +		dms = dwc->dws.p_master;
> +		dmsize = sconfig->dst_maxburst;

I would group it differently, i.e.

		sms = dwc->dws.m_master;
		dms = dwc->dws.p_master;
		smsize = 0;
		dmsize = sconfig->dst_maxburst;

> +	} else if (dwc->direction == DMA_DEV_TO_MEM) {
> +		sms = dwc->dws.p_master;
> +		smsize = sconfig->src_maxburst;
> +		dms = dwc->dws.m_master;
> +		dmsize = 0;
> +	} else /* DMA_MEM_TO_MEM */ {
> +		sms = dwc->dws.m_master;
> +		smsize = 0;
> +		dms = dwc->dws.m_master;
> +		dmsize = 0;
> +	}

Ditto for two above cases.

>  static u32 idma32_prepare_ctllo(struct dw_dma_chan *dwc)
>  {
>  	struct dma_slave_config	*sconfig = &dwc->dma_sconfig;
> -	u8 smsize = (dwc->direction == DMA_DEV_TO_MEM) ? sconfig->src_maxburst : 0;
> -	u8 dmsize = (dwc->direction == DMA_MEM_TO_DEV) ? sconfig->dst_maxburst : 0;
> +	u8 smsize, dmsize;
> +
> +	if (dwc->direction == DMA_MEM_TO_DEV) {
> +		smsize = 0;
> +		dmsize = sconfig->dst_maxburst;
> +	} else if (dwc->direction == DMA_DEV_TO_MEM) {
> +		smsize = sconfig->src_maxburst;
> +		dmsize = 0;
> +	} else /* DMA_MEM_TO_MEM */ {
> +		smsize = 0;
> +		dmsize = 0;
> +	}

	u8 smsize = 0, dmsize = 0;

	if (dwc->direction == DMA_MEM_TO_DEV)
		dmsize = sconfig->dst_maxburst;
	else if (dwc->direction == DMA_DEV_TO_MEM)
		smsize = sconfig->src_maxburst;

?

Something similar also can be done in the Synopsys case above, no?

-- 
With Best Regards,
Andy Shevchenko