linux-kernel - Re: [PATCH v2 6/7] serial: exar: add CTI board and port setup functions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20240412113943.7a2bf0c6@SWDEV2.connecttech.local>
Date: Fri, 12 Apr 2024 11:39:43 -0400
From: Parker Newman <parker@...est.io>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>, Jiri
 Slaby <jirislaby@...nel.org>, LKML <linux-kernel@...r.kernel.org>,
 linux-serial <linux-serial@...r.kernel.org>, Parker Newman
 <pnewman@...necttech.com>
Subject: Re: [PATCH v2 6/7] serial: exar: add CTI board and port setup
 functions

On Fri, 12 Apr 2024 17:28:20 +0200
Greg Kroah-Hartman <gregkh@...uxfoundation.org> wrote:

> 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@...ux.intel.com> wrote:
> >   
> > > On Thu, 11 Apr 2024, parker@...est.io wrote:
> > >   
> > > > From: Parker Newman <pnewman@...necttech.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

Thanks, I appreciate both your feedback I think I have a better handle on it.
-Parker