lkml.org 
[lkml]   [2021]   [Feb]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 05/18] tty: serial: samsung_tty: add support for Apple UARTs
Date
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

\
 
 \ /
  Last update: 2021-02-08 19:32    [W:0.225 / U:0.424 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site