Re: [PATCH v3 0/4] tty: TX helpers

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2755 bytes --]

On Wed, 7 Sep 2022, Jiri Slaby wrote:

> On 06. 09. 22, 13:30, Johan Hovold wrote:
> > On Tue, Sep 06, 2022 at 12:48:01PM +0200, Jiri Slaby wrote:
> > > This series introduces DEFINE_UART_PORT_TX_HELPER +
> > > DEFINE_UART_PORT_TX_HELPER_LIMITED TX helpers. See PATCH 2/4 for the
> > > details. Comments welcome.
> > > 
> > > Then it switches drivers to use them. First, to
> > > DEFINE_UART_PORT_TX_HELPER() in 3/4 and then
> > > DEFINE_UART_PORT_TX_HELPER_LIMITED() in 4/4.
> > > 
> > > The diffstat of patches 3+4 is as follows:
> > >   26 files changed, 191 insertions(+), 823 deletions(-)
> > > which appears to be nice.
> > 
> > Not really. This is horrid. Quality can't be measured in LoC (only).
> > 
> > The resulting code is unreadable. And for no good reason.
> 
> IMO, it's much more readable than the original ~ 30 various (and buggy -- see
> Ilpo's fixes) copies of this code. Apart from that, it makes further rework
> much easier (I have switch to kfifo in my mind for example).
> 
> > [ And note that you're "saving" something like 20 lines per driver:
> 
> It's not about saving, it's about deduplicating and unifying.
>
> > 	 12 files changed, 84 insertions(+), 349 deletions(-)
> > ]
> > 
> > NAK
> 
> I'd love to come up with something nicer. That would be a function in
> serial-core calling hooks like I had [1] for example. But provided all those
> CPU workarounds/thunks, it'd be quite expensive to call two functions per
> character.
> 
> Or creating a static inline (having ± the macro content) and the hooks as
> parameters and hope for optimizations to eliminate thunks (also suggested in
> the past [1]).
> 
> [1] https://lore.kernel.org/all/20220411105405.9519-1-jslaby@suse.cz/

I second Jiri here.

Saving lines in drivers is not that important compared with all removing 
all the variants of the same thing that have crept there over the years.

I suspect the main reason for the variants is that everybody just used 
other drivers as examples and therefore we've a few "main" variant 
branches depending on which of the drivers was used as an example for the 
other. That is hardly a good enough reason to keep them different and as 
long as each driver keeps its own function for this, it will eventually 
lead to similar differentiation so e.g. a one-time band-aid similarization 
would not help in the long run.

Also, I don't understand why you see it unreadable when the actual code is 
out in the open in that macro. It's formatted much better than e.g. 
read_poll_timeout() if you want an example of something that is hardly 
readable ;-). I agree though there's a learning-curve, albeit small, that 
it actually creates a function but that doesn't seem to me as big of an 
obstacle you seem to think.


-- 
 i.