Received-SPF: pass (google.com: domain of linux-kernel+bounces-142951-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Fri, 12 Apr 2024 17:28:20 +0200
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Parker Newman <parker@finest.io>
Cc: Ilpo =?iso-8859-1?Q?J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>,
	Jiri Slaby <jirislaby@kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	linux-serial <linux-serial@vger.kernel.org>,
	Parker Newman <pnewman@connecttech.com>
Subject: Re: [PATCH v2 6/7] serial: exar: add CTI board and port setup
 functions
Message-ID: <2024041248-enjoyable-barterer-4f01@gregkh>
References: <cover.1712863999.git.pnewman@connecttech.com>
 <ca94454e54504c1621f17f5e3933cad299f61344.1712863999.git.pnewman@connecttech.com>
 <c73b4fc3-be87-6a6d-408e-634ba915f28e@linux.intel.com>
 <20240412111926.5b4c9953@SWDEV2.connecttech.local>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240412111926.5b4c9953@SWDEV2.connecttech.local>

On Fri, Apr 12, 2024 at 11:19:26AM -0400, Parker Newman wrote:
> On Fri, 12 Apr 2024 13:57:01 +0300 (EEST)
> Ilpo J?rvinen <ilpo.jarvinen@linux.intel.com> wrote:
> 
> > On Thu, 11 Apr 2024, parker@finest.io wrote:
> > 
> > > From: Parker Newman <pnewman@connecttech.com>
> > > 
> > > - Removed old port setup function and replaced with UART specific ones
> > > - Added board setup functions for CTI boards
> > > - Replaced CONNECT_DEVICE macro with CTI_EXAR_DEVICE and CTI_PCI_DEVICE  
> > 
> > In general, you should try to do refactoring in a preparatory patch (one 
> > refactoring thing at a time) and add new stuff in another patch in 
> > the series. I didn't go to figure out how much it applies to those three 
> > items because you likely know the answer immediately.
> > 
> > > - Moved "generic rs485" support up in the file  
> > 
> > Please do this in a separate patch.
> > 
> 
> Will do.
> 
> > 
> > Another general level problem with your series is that it adds functions 
> > x, y, etc. without users, whereas the expected way of doing things would 
> > be to add the functions in the change they are getting used so it's easier 
> > to follow what's going on.
> > 
> > I believe if you separate the refactoring & moving code around into own 
> > changes (no functional change type patches), the new stuff is much 
> > smaller so there is no need to split that illogically into incomplete 
> > fragments in some patches.
> > 
> > --
> >  i.
> > 
> 
> Thanks for the feedback, I am new to the mailing lists and am trying to balance
> what you mention above with not having giant patches. 

It's a fine line, and takes a while to learn, but as a first cut, this
was pretty good, I didn't have any major problems with the structure of
it, so nice work.

thanks,

greg k-h