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

[Johan Hovold <johan@kernel.org>]

On Fri, Sep 09, 2022 at 12:53:04PM +0200, Jiri Slaby wrote:
> On 07. 09. 22, 16:56, Arnd Bergmann wrote:
> > On Wed, Sep 7, 2022, at 3:52 PM, Russell King (Oracle) wrote:
> >> On Wed, Sep 07, 2022 at 02:36:37PM +0200, Greg Kroah-Hartman wrote:
> >>
> >> Of course, it would have been nicer to see the definition of this
> >> macro, because then we can understand what the "ch" argument is to
> >> this macro, and how that relates to the macro argument that is
> >> shown in the example as a writel().
> > 
> > I pulled out the 'ch' variable from the macro to avoid having
> > the macro define local variables that are then passed to the
> > inner expressions.
> 
> Note that I had "port" and "ch" as a part of the macro parameters in 
> [v2], but it didn't help the situation much.
> >> Maybe a more complete example would help clear up the confusion?
> >> Arnd?
> > 
> > Here is a patch on top of the series that would implement the
> > uart_port_tx_helper_limited() and uart_port_tx_helper()
> > macros that can be used directly from drivers in place of defining
> > local functions, with the (alphabetically) first two drivers
> > converted to that.
> 
> If there are no objections, I will push the patches this directorin. I 
> like this more than [v2] or [v3] (the helper macros). Actually, I 
> mentioned this wait_event() style in [v1], but I perhaps simplified the 
> concept too much to completely eliminate the need of a wrapper function. 
> And that made it too complicated/too hard to understand.

This sounds much better. You also had some users that needed some
preamble which could now go in the same function (e.g.
altera_jtaguart_tx_chars()).

> Except I'd drop the "_helper" part from the name. Originally (in [v1]), 
> I had uart_port_tx() and uart_port_tx_limited() functions. In [v2+v3], I 
> added _helper to avoid confusion as we were generating a helpers using 
> the macros. Yes, technically, uart_port_tx() is still a helper, but I 
> think it's superfluous to have it in the name now.

That would also be an in improvement. For the altera example you could
end up with something like:

static void altera_jtaguart_tx_chars(struct altera_jtaguart *pp)
{
	struct uart_port *port = &pp->port;
	unsigned int space;

	space = (readl(port->membase + ALTERA_JTAGUART_CONTROL_REG) &
			ALTERA_JTAGUART_CONTROL_WSPACE_MSK) >>
			ALTERA_JTAGUART_CONTROL_WSPACE_OFF;

	uart_port_tx_chars(port, space,
			   writel(ch, port->membase + ALTERA_JTAGUART_DATA_REG));
}

which would be understandable.

If there are too many arguments to be passed in, then perhaps you can
explore Arnd's (and you own) suggestion to use inline helpers so that
the arguments are named.

Johan