Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <f44fffcf-c592-0c73-6e6c-f5914de82503@kernel.org>
Date:   Tue, 28 Mar 2023 11:23:45 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
Content-Language: en-US
To:     Jacky Huang <ychuang570808@gmail.com>, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, lee@kernel.org,
        mturquette@baylibre.com, sboyd@kernel.org, p.zabel@pengutronix.de,
        gregkh@linuxfoundation.org
Cc:     devicetree@vger.kernel.org, linux-clk@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org,
        arnd@arndb.de, schung@nuvoton.com, mjchen@nuvoton.com,
        Jacky Huang <ychuang3@nuvoton.com>
References: <20230328021912.177301-1-ychuang570808@gmail.com>
 <20230328021912.177301-12-ychuang570808@gmail.com>
From:   Jiri Slaby <jirislaby@kernel.org>
Subject: Re: [PATCH v6 11/12] tty: serial: Add Nuvoton ma35d1 serial driver
 support
In-Reply-To: <20230328021912.177301-12-ychuang570808@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 28. 03. 23, 4:19, Jacky Huang wrote:
> +static void transmit_chars(struct uart_ma35d1_port *up)
> +{
> +	struct circ_buf *xmit = &up->port.state->xmit;
> +	int count;
> +	u8 ch;
> +
> +	if (uart_tx_stopped(&up->port)) {
> +		ma35d1serial_stop_tx(&up->port);
> +		return;
> +	}
> +	if (uart_circ_empty(xmit)) {
> +		__stop_tx(up);
> +		return;
> +	}

Why is this necessary?

> +	count = UART_FIFO_DEPTH - ((serial_in(up, UART_REG_FSR) & FSR_TXPTR_MSK) >> 16);
> +
> +	uart_port_tx_limited(&up->port, ch, count,
> +			     !(serial_in(up, UART_REG_FSR) & FSR_TX_FULL),
> +			     serial_out(up, UART_REG_THR, ch),
> +			     ({}));
> +
> +	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
> +		uart_write_wakeup(&up->port);
> +
> +	if (uart_circ_empty(xmit))
> +		__stop_tx(up);

uart_port_tx_limited() should take care about the above and this too, right?

> +}
...
> +static void receive_chars(struct uart_ma35d1_port *up)
> +{
> +	u8 flag;
> +	u32 fsr;
> +	unsigned int ch;

Shouldn't ch be u8 too?

> +	int max_count = 256;
> +
> +	fsr = serial_in(up, UART_REG_FSR);
> +	do {
> +		flag = TTY_NORMAL;
> +		up->port.icount.rx++;
> +
> +		if (unlikely(fsr & (FSR_BIF | FSR_FEF | FSR_PEF | FSR_RX_OVER_IF))) {
> +			if (fsr & FSR_BIF) {
> +				up->port.icount.brk++;
> +				if (uart_handle_break(&up->port))
> +					continue;
> +			}
> +			if (fsr & FSR_FEF)
> +				up->port.icount.frame++;
> +			if (fsr & FSR_PEF)
> +				up->port.icount.parity++;
> +			if (fsr & FSR_RX_OVER_IF)
> +				up->port.icount.overrun++;
> +
> +			serial_out(up, UART_REG_FSR, fsr &
> +				   (FSR_BIF | FSR_FEF | FSR_PEF | FSR_RX_OVER_IF));
> +
> +			if (fsr & FSR_BIF)
> +				flag = TTY_BREAK;
> +			else if (fsr & FSR_PEF)
> +				flag = TTY_PARITY;
> +			else if (fsr & FSR_FEF)
> +				flag = TTY_FRAME;
> +		}
> +
> +		ch = serial_in(up, UART_REG_RBR);
> +		if (uart_handle_sysrq_char(&up->port, ch))
> +			continue;
> +
> +		spin_lock(&up->port.lock);
> +		uart_insert_char(&up->port, fsr, FSR_RX_OVER_IF, ch, flag);
> +		spin_unlock(&up->port.lock);
> +
> +		fsr = serial_in(up, UART_REG_FSR);
> +	} while (!(fsr & FSR_RX_EMPTY) && (max_count-- > 0));
> +
> +	spin_lock(&up->port.lock);
> +	tty_flip_buffer_push(&up->port.state->port);
> +	spin_unlock(&up->port.lock);
> +}
...
> +static int ma35d1serial_remove(struct platform_device *dev)
> +{
> +	struct uart_port *port = platform_get_drvdata(dev);
> +
> +	if (port) {

Can this ever be NULL?

> +		uart_remove_one_port(&ma35d1serial_reg, port);
> +		free_irq(port->irq, port);
> +	}
> +	return 0;
> +}
> +
> +static int ma35d1serial_suspend(struct platform_device *dev, pm_message_t state)
> +{
> +	int i;
> +	struct uart_ma35d1_port *up;
> +
> +	if (dev->dev.of_node)
> +		i = of_alias_get_id(dev->dev.of_node, "serial");
> +	if (i < 0) {
> +		dev_err(&dev->dev, "failed to get alias/pdev id, errno %d\n", i);
> +		return i;
> +	}
> +	up = &ma35d1serial_ports[i];

platform_get_drvdata(dev) ?

> +	if (i == 0) {
> +		up->console_baud_rate = serial_in(up, UART_REG_BAUD);
> +		up->console_line = serial_in(up, UART_REG_LCR);
> +		up->console_int = serial_in(up, UART_REG_IER);
> +	}
> +	return 0;
> +}
> +
> +static int ma35d1serial_resume(struct platform_device *dev)
> +{
> +	int i;
> +	struct uart_ma35d1_port *up;
> +
> +	if (dev->dev.of_node)
> +		i = of_alias_get_id(dev->dev.of_node, "serial");
> +	if (i < 0) {
> +		dev_err(&dev->dev, "failed to get alias/pdev id, errno %d\n", i);
> +		return i;
> +	}
> +	up = &ma35d1serial_ports[i];
> +	if (i == 0) {
> +		serial_out(up, UART_REG_BAUD, up->console_baud_rate);
> +		serial_out(up, UART_REG_LCR, up->console_line);
> +		serial_out(up, UART_REG_IER, up->console_int);
> +	}
> +	return 0;
> +}

No uart_suspend_port()/uart_resume_port()? You don't wait for 
transmitter to be empty in suspend. You don't stop tx etc.

> +static struct platform_driver ma35d1serial_driver = {
> +	.probe      = ma35d1serial_probe,
> +	.remove     = ma35d1serial_remove,
> +	.suspend    = ma35d1serial_suspend,
> +	.resume     = ma35d1serial_resume,
> +	.driver     = {
> +		.name   = "ma35d1-uart",
> +		.owner  = THIS_MODULE,
> +		.of_match_table = of_match_ptr(ma35d1_serial_of_match),
> +	},
> +};
> +
> +static int __init ma35d1serial_init(void)
> +{
> +	int ret;
> +
> +	ret = uart_register_driver(&ma35d1serial_reg);
> +	if (ret)
> +		return ret;
> +	ret = platform_driver_register(&ma35d1serial_driver);
> +	if (ret)
> +		uart_unregister_driver(&ma35d1serial_reg);
> +	return ret;
> +}
> +
> +static void __exit ma35d1serial_exit(void)
> +{
> +	platform_driver_unregister(&ma35d1serial_driver);
> +	uart_unregister_driver(&ma35d1serial_reg);
> +}
> +
> +module_init(ma35d1serial_init);
> +module_exit(ma35d1serial_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("MA35D1 serial driver");


> +MODULE_ALIAS_CHARDEV_MAJOR(TTY_MAJOR);

Why is this needed? How are other platform drivers autoloaded?

thanks,
-- 
js
suse labs