Re: [PATCH v2 1/2] serial: 8250: Add proper clock handling for OxSemi PCIe devices

From: "Maciej W. Rozycki" <macro@orcam.me.uk>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>,
	Andy Shevchenko <andy@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jiri Slaby <jirislaby@kernel.org>,
	"open list:SERIAL DRIVERS" <linux-serial@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 1/2] serial: 8250: Add proper clock handling for OxSemi PCIe devices
Date: Wed, 23 Mar 2022 21:59:28 +0000 (GMT)	[thread overview]
Message-ID: <alpine.DEB.2.21.2203232126220.52439@angie.orcam.me.uk> (raw)
In-Reply-To: <YjSL34DkktVVahmy@smile.fi.intel.com>

On Fri, 18 Mar 2022, Andy Shevchenko wrote:

> >  It does allow one to program the full clock divider range of the OxSemi 
> > devices.  I find it appropriate according to my engineer's code of good 
> > practices.  And it doesn't cause any burden for non-OxSemi code.
> 
> How BOTHER does prevent you doing the same?

 It does not allow you to set arbitrary serial port clock rates.  You can 
only set integer baud rates, and then only those that do not exceed the 
[1;65535] clock divisor range.

> >  So I have had a look at how it has been done for other drivers and I have 
> > now convinced myself against such a split.  The primary reason for this 
> > conclusion is that there is no basic infrastructure for such a split and 
> > the ultimate result is code duplication with no clear benefit to justify 
> > it.
> 
> Justification for split is to keep certain quirks out of the scope of the
> generic driver. I'm not sure what duplication you are talking about if the
> LOC statistics shows otherwise.

 All the init/remove code is almost the same across all the devices.  And 
suspend/resume and PCI error handling code has been removed from the split 
off devices, and for the functional regression to be fixed:

1. this code would have to be replicated, or

2. handlers from the generic 8250_pci.c driver exported and referred to, 
   or

3. some kind of a helper library (or a core module) created providing this 
   stuff to 8250_*.c drivers as required.

 I guess the latter is the minimum that could convince me this driver
framework is usable for implementing device-specific drivers (as I find 
the other variants rather miserable hacks).

 Plus there would have to be clear information provided to the users as 
otherwise people will be rather confused as to why 3 out of their 4 16x50 
PCI/e serial cards work with 8250_pci.c while the remaining one does not 
(probably broken, or is it?).

> You may not want to get the idea, it's fine. The rationale is simple:
> isolate quirks for certain platform(s) in one place. Each platform
> in a separate module.

 What is a platform in your terminology?  A PCI/e option card you can 
install in about any modern computer?  I usually think of platforms as 
specific families of computers rather than option cards.  Variants of 
otherwise the same device are usually handled with a single driver in 
Linux.

  Maciej