[PATCH v3 5/5] tty/serial: Add Spreadtrum sc9836-uart driver support

Orson Zhai
2014-11-28T10:13:50Z

Hi, Alan,

Thanks for your comments!
Some question for them below,

On 2014?11?26? 20:33, One Thousand Gnomes wrote:
>> + 213 = /dev/ttySPX0 SPRD serial port 0
>> + ...
>> + 216 = /dev/ttySPX3 SPRD serial port 3
> Please use dynamic allocation or give a very very good reason why you
> can't
do we just leave the .major & .minor as NULL in struct uart_driver for
dynamic allocation?
>> +config SERIAL_SPRD_NR
>> + int "Maximum number of sprd serial ports"
>> + depends on SERIAL_SPRD
>> + default "4"
> If you are doing dynamic allocation this should pretty much go away
I think you mean getting the number of uarts from dt and dynamically
allocate their port structure?
>
>> +static int sprd_startup(struct uart_port *port)
>> +{
>> + int ret = 0;
>> + unsigned int ien, ctrl1;
>> + struct sprd_uart_port *sp;
>> +
>> + serial_out(port, SPRD_CTL2, ((THLD_TX_EMPTY << 8) | THLD_RX_FULL));
>> +
>> + /* clear rx fifo */
>> + while (serial_in(port, SPRD_STS1) & 0x00ff)
>> + serial_in(port, SPRD_RXD);
>> +
>> + /* clear tx fifo */
>> + while (serial_in(port, SPRD_STS1) & 0xff00)
>> + ;
> Missing a cpu_relax and I would have thought a timeout on both of these.
We will add in V4
>
>
>> +static void sprd_set_termios(struct uart_port *port,
>> + struct ktermios *termios,
>> + struct ktermios *old)
>> +{
>> + unsigned int baud, quot;
>> + /* calculate parity */
>> + lcr &= ~SPRD_LCR_PARITY;
>> + if (termios->c_cflag & PARENB) {
>> + lcr |= SPRD_LCR_PARITY_EN;
>> + if (termios->c_cflag & PARODD)
>> + lcr |= SPRD_LCR_ODD_PAR;
>> + else
>> + lcr |= SPRD_LCR_EVEN_PAR;
>> + }
> If you don't support mark/space parity then also clear CMSPAR in
> termios->c_cflag.
just simply use code like "termios->c_cflag &= ~CMSPAR" ?

> If you do then it ought to be handled above.
>
>> +
>> + /* clock divider bit16~bit20 */
>> + serial_out(port, SPRD_CLKD1, (quot & 0x1f0000) >> 16);
>> + serial_out(port, SPRD_LCR, lcr);
>> + fc |= 0x3e00 | THLD_RX_FULL;
>> + serial_out(port, SPRD_CTL1, fc);
> Also set the resulting baud back into the termios (see the 8250 termios
> for an example)
you mean something like this part ?

/* Don't rewrite B0 */
if (tty_termios_baud_rate(termios))
tty_termios_encode_baud_rate(termios, baud, baud);

>
>
>> +static int __init sprd_console_setup(struct console *co, char *options)
>> +{
>> + struct uart_port *port;
>> + int baud = 115200;
>> + int bits = 8;
>> + int parity = 'n';
>> + int flow = 'n';
>> +
>> + if (unlikely(co->index >= UART_NR_MAX || co->index < 0))
>> + co->index = 0;
>> +
>> + port = (struct uart_port *)sprd_port[co->index];
>> + if (port == NULL) {
>> + pr_info("srial port %d not yet initialized\n", co->index);
> Typo
it will be fixed :)

Thanks,
Orson
> Looks basically sound to me
>
> Alan