Received-SPF: pass (google.com: domain of linux-kernel+bounces-147444-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Tue, 16 Apr 2024 22:04:42 +0300
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Serge Semin <fancer.lancer@gmail.com>
Cc: Viresh Kumar <vireshk@kernel.org>, Vinod Koul <vkoul@kernel.org>,
	Ilpo =?iso-8859-1?Q?J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jiri Slaby <jirislaby@kernel.org>, dmaengine@vger.kernel.org,
	linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/4] dmaengine: dw: Simplify prepare CTL_LO methods
Message-ID: <Zh7LyszPd2sNfWRm@smile.fi.intel.com>
References: <20240416162908.24180-1-fancer.lancer@gmail.com>
 <20240416162908.24180-4-fancer.lancer@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240416162908.24180-4-fancer.lancer@gmail.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo

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