Received-SPF: pass (google.com: domain of linux-kernel+bounces-151222-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Fri, 19 Apr 2024 12:19:48 +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: <oqyz6fpdxzk5x4xr5rlqwnj3frakjnz24e6bx7mxngh5gm5jom@wsm7qvubqwn3>
References: <20240416162908.24180-1-fancer.lancer@gmail.com>
 <20240416162908.24180-4-fancer.lancer@gmail.com>
 <Zh7LyszPd2sNfWRm@smile.fi.intel.com>
 <lzcgxh7trwoksd4bx2fsybellbngvpwhgq2a76ou2iufemockp@3dca4bfox2ps>
 <ZiEIRluj-50FMIgp@smile.fi.intel.com>
 <xfa7evanbrvdxdoq6473wpymvqogezspwkdoawu2dr6mnyxiwq@zx2schip66wj>
 <ZiGJ-DspJq5R6Dym@smile.fi.intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZiGJ-DspJq5R6Dym@smile.fi.intel.com>

On Fri, Apr 19, 2024 at 12:00:40AM +0300, Andy Shevchenko wrote:
> On Thu, Apr 18, 2024 at 10:00:02PM +0300, Serge Semin wrote:
> > On Thu, Apr 18, 2024 at 02:47:18PM +0300, Andy Shevchenko wrote:
> > > On Wed, Apr 17, 2024 at 11:11:46PM +0300, Serge Semin wrote:
> > > > 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:
> 
> ...
> 
> > > > > > +	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.
> > 
> > > Sure. The point here is that when you look at the DMA channel configuration
> > > usually you operate with the semantically tied fields for source and
> > > destination. At least this is my experience, I always check both sides
> > > of the transfer for the same field, e.g., master setting, hence I want to
> > > have them coupled.
> > 
> > Ok. I see. Thanks for clarification. I normally do that in another
> > order: group the functionally related fields together - all
> > source-related configs first, then all destination-related configs.
> > Honestly I don't have strong opinion about this part, it's just my
> > personal preference. Am I right to think that from your experience in
> > kernel it's normally done in the order you described?
> 

> In this driver I believe I have followed that one, yes.

Agreed then. I'll change the order to the way you ask.

-Serge(y)

> 
> > > > > > +	} 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.
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>