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@xxxxxxx/ 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.
