| Message ID | 20210215121713.57687-20-marcan@marcan.st (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Apple M1 SoC platform bring-up | expand |
On Mon, Feb 15, 2021 at 09:17:07PM +0900, Hector Martin wrote: > * Split out s3c24xx_serial_tx_chars from s3c24xx_serial_tx_irq, > where only the latter acquires the port lock. I miss here information why you do all this. > > * For S3C64xx, return IRQ_NONE if no flag bits were set, so the > interrupt core can detect IRQ storms. Note that both IRQ handlers > always return IRQ_HANDLED anyway, so 'or' logic isn't necessary here, > if either handler ran we are always going to return IRQ_HANDLED. It looks like separate patch. Your patches should do only one thing at once. The fact that you have here three bullet points is a warning sign. This is even more important if you do some refactorings and cleanups which should not affect functionality. Hiding there changes which could affect functionality is a no-go. > > * Rename s3c24xx_serial_rx_chars to s3c24xx_serial_rx_irq for > consistency with the above. All it does now is call two other > functions anyway. Separate patch for trivial renaming. > > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > drivers/tty/serial/samsung_tty.c | 41 +++++++++++++++++++------------- > 1 file changed, 24 insertions(+), 17 deletions(-) > > diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c > index 21955be680a4..821cd0e4f870 100644 > --- a/drivers/tty/serial/samsung_tty.c > +++ b/drivers/tty/serial/samsung_tty.c > @@ -151,6 +151,9 @@ struct s3c24xx_uart_port { > #endif > }; > > +static void s3c24xx_serial_start_next_tx(struct s3c24xx_uart_port *ourport); > +static void s3c24xx_serial_tx_chars(struct s3c24xx_uart_port *ourport); > + > /* conversion functions */ > > #define s3c24xx_dev_to_port(__dev) dev_get_drvdata(__dev) > @@ -316,8 +319,6 @@ static void s3c24xx_serial_stop_tx(struct uart_port *port) > ourport->tx_mode = 0; > } > > -static void s3c24xx_serial_start_next_tx(struct s3c24xx_uart_port *ourport); > - Why moving this? Why adding s3c24xx_serial_tx_chars() forward declaration? Best regards, Krzysztof
On 16/02/2021 03.40, Krzysztof Kozlowski wrote: > On Mon, Feb 15, 2021 at 09:17:07PM +0900, Hector Martin wrote: >> * Split out s3c24xx_serial_tx_chars from s3c24xx_serial_tx_irq, >> where only the latter acquires the port lock. > > I miss here information why you do all this. Added an explanation for v3. This is used by a subsequent patch in the series. >> >> * For S3C64xx, return IRQ_NONE if no flag bits were set, so the >> interrupt core can detect IRQ storms. Note that both IRQ handlers >> always return IRQ_HANDLED anyway, so 'or' logic isn't necessary here, >> if either handler ran we are always going to return IRQ_HANDLED. > > It looks like separate patch. Your patches should do only one thing at > once. The fact that you have here three bullet points is a warning > sign. This is even more important if you do some refactorings and > cleanups which should not affect functionality. Hiding there changes > which could affect functionality is a no-go. I've reverted this one. I don't think it should affect functionality, but I don't have any way to test on these devices, so I'll leave it to someone else to be safe :) >> >> * Rename s3c24xx_serial_rx_chars to s3c24xx_serial_rx_irq for >> consistency with the above. All it does now is call two other >> functions anyway. > > Separate patch for trivial renaming. I think it makes sense to do this rename together with the first change above, as it keeps both functions symmetric. Otherwise we end up with an inconsistent function naming between both patches. If you really want it separate though, I can do that. > >> >> Signed-off-by: Hector Martin <marcan@marcan.st> >> --- >> drivers/tty/serial/samsung_tty.c | 41 +++++++++++++++++++------------- >> 1 file changed, 24 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c >> index 21955be680a4..821cd0e4f870 100644 >> --- a/drivers/tty/serial/samsung_tty.c >> +++ b/drivers/tty/serial/samsung_tty.c >> @@ -151,6 +151,9 @@ struct s3c24xx_uart_port { >> #endif >> }; >> >> +static void s3c24xx_serial_start_next_tx(struct s3c24xx_uart_port *ourport); >> +static void s3c24xx_serial_tx_chars(struct s3c24xx_uart_port *ourport); >> + >> /* conversion functions */ >> >> #define s3c24xx_dev_to_port(__dev) dev_get_drvdata(__dev) >> @@ -316,8 +319,6 @@ static void s3c24xx_serial_stop_tx(struct uart_port *port) >> ourport->tx_mode = 0; >> } >> >> -static void s3c24xx_serial_start_next_tx(struct s3c24xx_uart_port *ourport); >> - > > Why moving this? Why adding s3c24xx_serial_tx_chars() forward > declaration? This should've gone in the next patch. A previous reviewer told me to put declarations at the top of the file, so I put it there and moved this one along with it, but I'll keep it to the additon only for v3.
On Thu, Feb 18, 2021 at 10:53:10PM +0900, Hector Martin wrote: > > Separate patch for trivial renaming. > > I think it makes sense to do this rename together with the first change > above, as it keeps both functions symmetric. Otherwise we end up with an > inconsistent function naming between both patches. If you really want it > separate though, I can do that. OK, keep your version. > > > > > > > > > Signed-off-by: Hector Martin <marcan@marcan.st> > > > --- > > > drivers/tty/serial/samsung_tty.c | 41 +++++++++++++++++++------------- > > > 1 file changed, 24 insertions(+), 17 deletions(-) > > > > > > diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c > > > index 21955be680a4..821cd0e4f870 100644 > > > --- a/drivers/tty/serial/samsung_tty.c > > > +++ b/drivers/tty/serial/samsung_tty.c > > > @@ -151,6 +151,9 @@ struct s3c24xx_uart_port { > > > #endif > > > }; > > > +static void s3c24xx_serial_start_next_tx(struct s3c24xx_uart_port *ourport); > > > +static void s3c24xx_serial_tx_chars(struct s3c24xx_uart_port *ourport); > > > + > > > /* conversion functions */ > > > #define s3c24xx_dev_to_port(__dev) dev_get_drvdata(__dev) > > > @@ -316,8 +319,6 @@ static void s3c24xx_serial_stop_tx(struct uart_port *port) > > > ourport->tx_mode = 0; > > > } > > > -static void s3c24xx_serial_start_next_tx(struct s3c24xx_uart_port *ourport); > > > - > > > > Why moving this? Why adding s3c24xx_serial_tx_chars() forward > > declaration? > > This should've gone in the next patch. A previous reviewer told me to put > declarations at the top of the file, so I put it there and moved this one > along with it, but I'll keep it to the additon only for v3. Maybe I missed something in the context but it looked like forward declaration s3c24xx_serial_tx_chars() was not needed? In such case no need to move it. Best regards, Krzysztof
On 21/02/2021 04.11, Krzysztof Kozlowski wrote: > On Thu, Feb 18, 2021 at 10:53:10PM +0900, Hector Martin wrote: >> This should've gone in the next patch. A previous reviewer told me to put >> declarations at the top of the file, so I put it there and moved this one >> along with it, but I'll keep it to the additon only for v3. > > Maybe I missed something in the context but it looked like > forward declaration s3c24xx_serial_tx_chars() was not needed? In such > case no need to move it. It's needed in patch #22 in this series; having it in this patch was a mistake I made while splitting up the changes. I have moved that line to the Apple support patch for v3.
diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c index 21955be680a4..821cd0e4f870 100644 --- a/drivers/tty/serial/samsung_tty.c +++ b/drivers/tty/serial/samsung_tty.c @@ -151,6 +151,9 @@ struct s3c24xx_uart_port { #endif }; +static void s3c24xx_serial_start_next_tx(struct s3c24xx_uart_port *ourport); +static void s3c24xx_serial_tx_chars(struct s3c24xx_uart_port *ourport); + /* conversion functions */ #define s3c24xx_dev_to_port(__dev) dev_get_drvdata(__dev) @@ -316,8 +319,6 @@ static void s3c24xx_serial_stop_tx(struct uart_port *port) ourport->tx_mode = 0; } -static void s3c24xx_serial_start_next_tx(struct s3c24xx_uart_port *ourport); - static void s3c24xx_serial_tx_dma_complete(void *args) { struct s3c24xx_uart_port *ourport = args; @@ -825,7 +826,7 @@ static irqreturn_t s3c24xx_serial_rx_chars_pio(void *dev_id) return IRQ_HANDLED; } -static irqreturn_t s3c24xx_serial_rx_chars(int irq, void *dev_id) +static irqreturn_t s3c24xx_serial_rx_irq(int irq, void *dev_id) { struct s3c24xx_uart_port *ourport = dev_id; @@ -834,16 +835,12 @@ static irqreturn_t s3c24xx_serial_rx_chars(int irq, void *dev_id) return s3c24xx_serial_rx_chars_pio(dev_id); } -static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id) +static void s3c24xx_serial_tx_chars(struct s3c24xx_uart_port *ourport) { - struct s3c24xx_uart_port *ourport = id; struct uart_port *port = &ourport->port; struct circ_buf *xmit = &port->state->xmit; - unsigned long flags; int count, dma_count = 0; - spin_lock_irqsave(&port->lock, flags); - count = CIRC_CNT_TO_END(xmit->head, xmit->tail, UART_XMIT_SIZE); if (ourport->dma && ourport->dma->tx_chan && @@ -860,7 +857,7 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id) wr_reg(port, S3C2410_UTXH, port->x_char); port->icount.tx++; port->x_char = 0; - goto out; + return; } /* if there isn't anything more to transmit, or the uart is now @@ -869,7 +866,7 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id) if (uart_circ_empty(xmit) || uart_tx_stopped(port)) { s3c24xx_serial_stop_tx(port); - goto out; + return; } /* try and drain the buffer... */ @@ -891,7 +888,7 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id) if (!count && dma_count) { s3c24xx_serial_start_tx_dma(ourport, dma_count); - goto out; + return; } if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) { @@ -902,8 +899,18 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id) if (uart_circ_empty(xmit)) s3c24xx_serial_stop_tx(port); +} + +static irqreturn_t s3c24xx_serial_tx_irq(int irq, void *id) +{ + struct s3c24xx_uart_port *ourport = id; + struct uart_port *port = &ourport->port; + unsigned long flags; + + spin_lock_irqsave(&port->lock, flags); + + s3c24xx_serial_tx_chars(ourport); -out: spin_unlock_irqrestore(&port->lock, flags); return IRQ_HANDLED; } @@ -914,14 +921,14 @@ static irqreturn_t s3c64xx_serial_handle_irq(int irq, void *id) struct s3c24xx_uart_port *ourport = id; struct uart_port *port = &ourport->port; unsigned int pend = rd_regl(port, S3C64XX_UINTP); - irqreturn_t ret = IRQ_HANDLED; + irqreturn_t ret = IRQ_NONE; if (pend & S3C64XX_UINTM_RXD_MSK) { - ret = s3c24xx_serial_rx_chars(irq, id); + ret = s3c24xx_serial_rx_irq(irq, id); wr_regl(port, S3C64XX_UINTP, S3C64XX_UINTM_RXD_MSK); } if (pend & S3C64XX_UINTM_TXD_MSK) { - ret = s3c24xx_serial_tx_chars(irq, id); + ret = s3c24xx_serial_tx_irq(irq, id); wr_regl(port, S3C64XX_UINTP, S3C64XX_UINTM_TXD_MSK); } return ret; @@ -1153,7 +1160,7 @@ static int s3c24xx_serial_startup(struct uart_port *port) ourport->rx_enabled = 1; - ret = request_irq(ourport->rx_irq, s3c24xx_serial_rx_chars, 0, + ret = request_irq(ourport->rx_irq, s3c24xx_serial_rx_irq, 0, s3c24xx_serial_portname(port), ourport); if (ret != 0) { @@ -1167,7 +1174,7 @@ static int s3c24xx_serial_startup(struct uart_port *port) ourport->tx_enabled = 1; - ret = request_irq(ourport->tx_irq, s3c24xx_serial_tx_chars, 0, + ret = request_irq(ourport->tx_irq, s3c24xx_serial_tx_irq, 0, s3c24xx_serial_portname(port), ourport); if (ret) {
* Split out s3c24xx_serial_tx_chars from s3c24xx_serial_tx_irq, where only the latter acquires the port lock. * For S3C64xx, return IRQ_NONE if no flag bits were set, so the interrupt core can detect IRQ storms. Note that both IRQ handlers always return IRQ_HANDLED anyway, so 'or' logic isn't necessary here, if either handler ran we are always going to return IRQ_HANDLED. * Rename s3c24xx_serial_rx_chars to s3c24xx_serial_rx_irq for consistency with the above. All it does now is call two other functions anyway. Signed-off-by: Hector Martin <marcan@marcan.st> --- drivers/tty/serial/samsung_tty.c | 41 +++++++++++++++++++------------- 1 file changed, 24 insertions(+), 17 deletions(-)