Re: [PATCH 05/18] tty: serial: samsung_tty: add support for Apple UARTs

[Krzysztof Kozlowski]

On Tue, Feb 09, 2021 at 01:10:02AM +0900, Hector Martin wrote:
> On 08/02/2021 19.54, Krzysztof Kozlowski wrote:
> > > +enum s3c24xx_irq_type {
> > > +	IRQ_DISCRETE = 0,
> > > +	IRQ_S3C6400 = 1,
> > > +	IRQ_APPLE = 2,
> > 
> > It seems you add the third structure to differentiate type of UART.
> > There is already port type and s3c24xx_serial_drv_data, no need for
> > third structure (kind of similar to tries of Tamseel Shams in recent
> > patches). It's too much. Instead, differentiate by port type or prepare
> > own set of uart_ops if it's really different (like you did with startup
> > op).
> 
> This ties into little changes in a bunch of places, so separate uart_ops
> callbacks for every one of those would end up duplicating a lot of code :(
> 
> That list is just used to map the port type to something that only
> represents the type of IRQs, but its only real purpose for the indirection
> is so I can do "== IRQ_DISCRETE" in some codepaths to mean "not apple or
> S3C6400".
> 
> e.g.
> 
>     if (s3c24xx_irq_type(port) == IRQ_DISCRETE)
>         free_irq(ourport->rx_irq, ourport);
> 
> Would have to become
> 
>     if (port->type != IRQ_S3C6400 && port->type != IRQ_APPLE)
>         free_irq(ourport->rx_irq, ourport);
> 
> or
> 
>     switch (port->type) {
>     case IRQ_S3C6400:
>     case IRQ_APPLE:
>         break;
>     default:
>         free_irq(ourport->rx_irq, ourport);
>     }
> 
> Which one do you prefer?

The latter, especially that switches will appear quite a lot in such
case.

However this pattern (== IRQ_DISCRETE) appears only in three places so
it should not be the main case considered here.

However I saw later you actually replaced the
s3c24xx_serial_has_interrupt_mask(), so it's not that bad.

In most of your code, there will be actually a switch with all three
cases.

> 
> Aside: Marc didn't like introducing new port types into uapi, but if we
> don't do that then we need a "real" port type in drv_data that isn't the
> uapi-exposed port or something along those lines, with a separate enum
> containing all the true port type values for that.

Looking at Greg's comment, we can get rid of the PORT_ stuff entirely.
First of all, PORT_S3C2410 == PORT_S3C2412, so this define is not
accurate.

This leaves us with three types (s3c2400, s3c2440, s3c6410 and Apple).
The s3c2440 could be removed with adding a new "ucon_mask" field to
s3c24xx_serial_drv_data.

This would end with s3c24xx, s3c6410 and Apple - quite nice choice.

> 
> > >   	/* Startup sequence is different for s3c64xx and higher SoC's */
> > > -	if (s3c24xx_serial_has_interrupt_mask(port))
> > > +	case IRQ_S3C6400:
> > >   		s3c24xx_serial_ops.startup = s3c64xx_serial_startup;
> > 
> > Don't overwrite specific ops. It's difficult to see then which ops are
> > being used. Instead create a new set of uart_ops matching the needs.
> 
> s3c24xx_serial_ops was already doing that here, but I can move that to a a
> separate uart_ops too.

You're right. This one would have to be improved before your change.
Instead of replacing specific op calls in startup, I think it's better
to have entirely separate ops instance for each variant:

static const struct uart_ops s3c24xx_serial_ops;
static const struct uart_ops s3c64xx_serial_ops;
static const struct uart_ops s5l_serial_ops;

This allows to add a "const", since uart_port takes such const pointer.

Still during s3c24xx_serial_probe() correct ops would have to be
assigned, but at least all ops are easily visible.

Best regards,
Krzysztof