Messages in this thread | | | Date | Tue, 21 Mar 2023 22:23:04 +0800 | Subject | Re: [PATCH 14/15] tty: serial: Add Nuvoton ma35d1 serial driver support | From | Jacky Huang <> |
| |
Dear Ilpo,
On 2023/3/20 下午 06:04, Ilpo Järvinen wrote: > On Mon, 20 Mar 2023, Jacky Huang wrote: > >> Dear Ilpo, >> >> >> Thanks for your advice. >> >> On 2023/3/16 下午 10:54, Ilpo Järvinen wrote: >>> Hi, >>> >>> I'll not note all things below because others have already seemingly >>> commented many things. >>> >>> On Wed, 15 Mar 2023, Jacky Huang wrote: >>> >>>> From: Jacky Huang <ychuang3@nuvoton.com> >>>> >>>> This adds UART and console driver for Nuvoton ma35d1 Soc. >>>> + } >>>> + ch = (u8)serial_in(up, UART_REG_RBR); >>> Drop the case. >> I will fix it. > I meant "cast" in case it wasn't obvious.
I know that, thank you.
>>>> +/* Enable or disable the rs485 support */ >>>> +static int ma35d1serial_config_rs485(struct uart_port *port, >>>> + struct ktermios *termios, >>>> + struct serial_rs485 *rs485conf) >>>> +{ >>>> + struct uart_ma35d1_port *p = to_ma35d1_uart_port(port); >>>> + >>>> + p->rs485 = *rs485conf; >>>> + >>>> + if (p->rs485.delay_rts_before_send >= 1000) >>>> + p->rs485.delay_rts_before_send = 1000; >>> Don't do this in driver, the core handles the delay limits. You don't seem >>> to be using the value anyway for anything??? >>> >>> Please separate the RS485 support into its own patch. >> >> OK, we will remove RS485 support from this initial patch. >> Once this initial patch was merged, we will submit the patch for RS485 >> support. > You could do that but you could just as well include it into the same > series as another patch after the main patch. >>>> + serial_out(p, UART_FUN_SEL, >>>> + (serial_in(p, UART_FUN_SEL) & ~FUN_SEL_MASK)); >>>> + >>>> + if (rs485conf->flags & SER_RS485_ENABLED) { >>>> + serial_out(p, UART_FUN_SEL, >>>> + (serial_in(p, UART_FUN_SEL) | FUN_SEL_RS485)); >>> Does this pair of serial_out()s glitch the RS485 line if ->rs485_config() >>> is called while RS485 mode is already set? >>> >>> Why you need to do serial_in() from the UART_FUN_SEL twice? >> UART_FUN_SEL (2 bits) definition: >> 00 - UART function >> 01 - IrDA function >> 11 - RS485 function >> >> The first searial_in() is used to clear set as UART function. >> The second one is used to set RS485 function if SER_RS485_ENABLED is true. > I got that, but it doesn't answer either of my questions which are: > > Can you clear the UART function without causing a glitch in the RS485? > ->rs485_config() can be called while already in RS485 mode so does it > cause the UART to temporarily switch away from RS485 mode to "UART > function" until the second write. > > Also, you didn't explain why you need to read the register again, does > the HW play with other bits when you do the clearing or to they remain > the same (in which case you can just use a temporary variable to store > the value)? ...It would be better to just write once too so this question > might not matter in the end.
Thank you for the detailed explanation.
OK, the register won't change. I will modify the code to read once and write once only.
>>>> + if (pdev->dev.of_node) { >>>> + ret = of_alias_get_id(pdev->dev.of_node, "serial"); >>>> + if (ret < 0) { >>>> + dev_err(&pdev->dev, >>>> + "failed to get alias/pdev id, errno %d\n", >>>> + ret); >>> Just put error prints to one line if you don't break 100 chars limit. >> But the checkpatch limitation is 80 characters. > No, it isn't. It was changed years ago already.
I have a test on the checkpatch script.
You are right. It won't complain about over 80 characters now.
>>>> +++ b/drivers/tty/serial/ma35d1_serial.h >>>> @@ -0,0 +1,93 @@ >>>> +/* SPDX-License-Identifier: GPL-2.0 */ >>>> +/* >>>> + * MA35D1 serial driver header file >>>> + * Copyright (C) 2023 Nuvoton Technology Corp. >>>> + */ >>>> +#ifndef __MA35D1_SERIAL_H__ >>>> +#define __MA35D1_SERIAL_H__ >>>> + >>>> +/* UART Receive/Transmit Buffer Register */ >>>> +#define UART_REG_RBR 0x00 >>>> +#define UART_REG_THR 0x00 >>>> + >>>> +/* UART Interrupt Enable Register */ >>>> +#define UART_REG_IER 0x04 >>>> +#define RDA_IEN 0x00000001 /* RBR Available Interrupt Enable >>>> */ >>>> +#define THRE_IEN 0x00000002 /* THR Empty Interrupt Enable */ >>>> +#define RLS_IEN 0x00000004 /* RX Line Status Interrupt Enable >>>> */ >>>> +#define RTO_IEN 0x00000010 /* RX Time-out Interrupt Enable */ >>>> +#define BUFERR_IEN 0x00000020 /* Buffer Error Interrupt Enable */ >>>> +#define TIME_OUT_EN 0x00000800 /* RX Buffer Time-out Counter >>>> Enable */ >>>> + >>>> +/* UART FIFO Control Register */ >>>> +#define UART_REG_FCR 0x08 >>>> +#define RFR 0x00000002 /* RX Field Software Reset */ >>>> +#define TFR 0x00000004 /* TX Field Software Reset */ >>>> + >>>> +/* UART Line Control Register */ >>>> +#define UART_REG_LCR 0x0C >>>> +#define NSB 0x00000004 /* Number of “STOP Bit” */ >>>> +#define PBE 0x00000008 /* Parity Bit Enable */ >>>> +#define EPE 0x00000010 /* Even Parity Enable */ >>>> +#define SPE 0x00000020 /* Stick Parity Enable */ >>>> +#define BCB 0x00000040 /* Break Control */ >>>> + >>>> +/* UART Modem Control Register */ >>>> +#define UART_REG_MCR 0x10 >>>> +#define RTS 0x00000020 /* nRTS Signal Control */ >>>> +#define RTSACTLV 0x00000200 /* nRTS Pin Active Level */ >>>> +#define RTSSTS 0x00002000 /* nRTS Pin Status (Read Only) */ >>>> + >>>> +/* UART Modem Status Register */ >>>> +#define UART_REG_MSR 0x14 >>>> +#define CTSDETF 0x00000001 /* Detect nCTS State Change Flag */ >>>> +#define CTSSTS 0x00000010 /* nCTS Pin Status (Read Only) */ >>>> +#define CTSACTLV 0x00000100 /* nCTS Pin Active Level */ >>>> + >>>> +/* UART FIFO Status Register */ >>>> +#define UART_REG_FSR 0x18 >>>> +#define RX_OVER_IF 0x00000001 /* RX Overflow Error Interrupt Flag */ >>>> +#define PEF 0x00000010 /* Parity Error Flag*/ >>>> +#define FEF 0x00000020 /* Framing Error Flag */ >>>> +#define BIF 0x00000040 /* Break Interrupt Flag */ >>>> +#define RX_EMPTY 0x00004000 /* Receiver FIFO Empty (Read Only) */ >>>> +#define RX_FULL 0x00008000 /* Receiver FIFO Full (Read Only) >>>> */ >>>> +#define TX_EMPTY 0x00400000 /* Transmitter FIFO Empty (Read Only) */ >>>> +#define TX_FULL 0x00800000 /* Transmitter FIFO Full (Read >>>> Only) */ >>>> +#define TX_OVER_IF 0x01000000 /* TX Overflow Error Interrupt Flag */ >>>> +#define TE_FLAG 0x10000000 /* Transmitter Empty Flag (Read >>>> Only) */ >>>> + >>>> +/* UART Interrupt Status Register */ >>>> +#define UART_REG_ISR 0x1C >>>> +#define RDA_IF 0x00000001 /* RBR Available Interrupt Flag */ >>>> +#define THRE_IF 0x00000002 /* THR Empty Interrupt Flag */ >>>> +#define RLSIF 0x00000004 /* Receive Line Interrupt Flag */ >>>> +#define MODEMIF 0x00000008 /* MODEM Interrupt Flag */ >>>> +#define RXTO_IF 0x00000010 /* RX Time-out Interrupt Flag */ >>>> +#define BUFEIF 0x00000020 /* Buffer Error Interrupt Flag */ >>>> +#define WK_IF 0x00000040 /* UART Wake-up Interrupt Flag */ >>>> +#define RDAINT 0x00000100 /* RBR Available Interrupt >>>> Indicator */ >>>> +#define THRE_INT 0x00000200 /* THR Empty Interrupt Indicator */ > I forgot to mention earlier, there are many defines above which should use > BIT(). > Sure we will fix them all.
Best regards,
Jacky Huang
| |