Received-SPF: pass (google.com: domain of linux-kernel+bounces-149173-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Wed, 17 Apr 2024 23:11:46 +0300
From: Serge Semin <fancer.lancer@gmail.com>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Viresh Kumar <vireshk@kernel.org>, Vinod Koul <vkoul@kernel.org>, 
	Ilpo =?utf-8?B?SsOkcnZpbmVu?= <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: <lzcgxh7trwoksd4bx2fsybellbngvpwhgq2a76ou2iufemockp@3dca4bfox2ps>
References: <20240416162908.24180-1-fancer.lancer@gmail.com>
 <20240416162908.24180-4-fancer.lancer@gmail.com>
 <Zh7LyszPd2sNfWRm@smile.fi.intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Zh7LyszPd2sNfWRm@smile.fi.intel.com>

On Tue, Apr 16, 2024 at 10:04:42PM +0300, Andy Shevchenko wrote:
> 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;

Could you please clarify, why? From my point of view it was better to
group the source master ID and the source master burst size inits
together.

> 
> > +	} 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?

As in case of the patch #1 the if-else statement here was designed
like that intentionally: to signify that the else clause implies the
DMA_MEM_TO_MEM transfer. Any other one (like DMA_DEV_TO_DEV) would
need to have the statement alteration. Moreover even though your
version looks smaller, but it causes one redundant store operation.
Do you think it still would be better to use your version despite of
my reasoning? 

-Serge(y)

> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>