Messages in this thread | | | From | Hector Martin <> | Subject | Re: [PATCH 05/18] tty: serial: samsung_tty: add support for Apple UARTs | Date | Tue, 9 Feb 2021 01:10:02 +0900 |
| |
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?
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.
>> /* 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.
Ack on all the other comments, I'll make the changes for v2.
-- Hector Martin (marcan@marcan.st) Public Key: https://mrcn.st/pub
| |