Re: [PATCH v3 2/2] serial: tegra-utc: Add driver for Tegra UART Trace Controller (UTC)

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, Feb 12, 2025 at 04:11:32PM +0530, Kartik Rajput wrote:
> The Tegra264 SoC supports the UART Trace Controller (UTC), which allows
> multiple firmware clients (up to 16) to share a single physical UART.
> Each client is provided with its own interrupt and has access to a
> 128-character wide FIFO for both transmit (TX) and receive (RX)
> operations.
> 
> Add tegra-utc driver to support Tegra UART Trace Controller (UTC)
> client.

...

> +/*
> + * NVIDIA Tegra UTC (UART Trace Controller) driver.
> + */

Can be a single line.

...

> +#include <linux/bits.h>
> +#include <linux/console.h>
> +#include <linux/container_of.h>
> +#include <linux/device.h>
> +#include <linux/err.h>

> +#include <linux/io.h>
> +#include <linux/iopoll.h>

iopoll.h guarantees to include io.h in case you want to have less lines here.
(yeah, I know that the header guarantees is a tribal knowledge, it's undocumented)

> +#include <linux/kfifo.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>

> +#include <linux/of.h>

Is this being used now?

> +#include <linux/property.h>
> +#include <linux/platform_device.h>
> +#include <linux/serial.h>
> +#include <linux/serial_core.h>
> +#include <linux/slab.h>
> +#include <linux/tty.h>
> +#include <linux/tty_flip.h>
> +#include <linux/types.h>

...

> +#define UART_NR					16

Bad naming, calling for collisions. Move it to the driver's namespace.

...

> +static void tegra_utc_init_tx(struct tegra_utc_port *tup)
> +{
> +	/* Disable TX. */
> +	tegra_utc_tx_writel(tup, 0x0, TEGRA_UTC_ENABLE);
> +
> +	/* Update the FIFO Threshold. */
> +	tegra_utc_tx_writel(tup, tup->tx_threshold, TEGRA_UTC_FIFO_THRESHOLD);
> +
> +	/* Clear and mask all the interrupts. */
> +	tegra_utc_tx_writel(tup, TEGRA_UTC_INTR_REQ | TEGRA_UTC_INTR_FULL | TEGRA_UTC_INTR_EMPTY,
> +			    TEGRA_UTC_INTR_CLEAR);

Here...

> +	tegra_utc_disable_tx_irq(tup);
> +
> +	/* Enable TX. */
> +	tegra_utc_tx_writel(tup, TEGRA_UTC_ENABLE_CLIENT_ENABLE, TEGRA_UTC_ENABLE);
> +}
> +
> +static void tegra_utc_init_rx(struct tegra_utc_port *tup)
> +{
> +	tup->rx_irqmask = TEGRA_UTC_INTR_REQ | TEGRA_UTC_INTR_TIMEOUT;
> +
> +	tegra_utc_rx_writel(tup, TEGRA_UTC_COMMAND_RESET, TEGRA_UTC_COMMAND);
> +	tegra_utc_rx_writel(tup, tup->rx_threshold, TEGRA_UTC_FIFO_THRESHOLD);
> +
> +	/* Clear all the pending interrupts. */
> +	tegra_utc_rx_writel(tup, TEGRA_UTC_INTR_TIMEOUT | TEGRA_UTC_INTR_OVERFLOW |
> +			    TEGRA_UTC_INTR_REQ | TEGRA_UTC_INTR_FULL |
> +			    TEGRA_UTC_INTR_EMPTY, TEGRA_UTC_INTR_CLEAR);

...and here the potential of deduplication by introducing an additional constant:

#define TEGRA_UTC_INTR_COMMON	\
	(...)

(choose better name)

> +	tegra_utc_rx_writel(tup, tup->rx_irqmask, TEGRA_UTC_INTR_MASK);
> +	tegra_utc_rx_writel(tup, tup->rx_irqmask, TEGRA_UTC_INTR_SET);
> +
> +	/* Enable RX. */
> +	tegra_utc_rx_writel(tup, TEGRA_UTC_ENABLE_CLIENT_ENABLE, TEGRA_UTC_ENABLE);
> +}

...

> +static bool tegra_utc_tx_chars(struct tegra_utc_port *tup)
> +{
> +	struct uart_port *port = &tup->port;
> +	unsigned int pending;
> +	u8 c;
> +
> +	pending = uart_port_tx(port, c,
> +		     !(tegra_utc_tx_readl(tup, TEGRA_UTC_FIFO_STATUS) & TEGRA_UTC_FIFO_FULL),
> +		     tegra_utc_tx_writel(tup, c, TEGRA_UTC_DATA));

Make the last two to reside in temporary variables with self-explanatory names.

> +

Redundant blank line.

> +	if (pending)
> +		return true;
> +
> +	return false;

	return pending;

> +}

...

> +static int tegra_utc_startup(struct uart_port *port)
> +{
> +	struct tegra_utc_port *tup = container_of(port, struct tegra_utc_port, port);
> +	int ret;
> +
> +	tegra_utc_hw_init(tup);
> +
> +	ret = request_irq(port->irq, tegra_utc_isr, 0, dev_name(port->dev), tup);

Seems the same Q stands about sharing, perhaps a comment why it's expected to
be always exclusive?

> +	if (ret < 0)
> +		dev_err(port->dev, "failed to register interrupt handler\n");
> +
> +	return ret;
> +}

...

> +	for (i = 0; i < len; i++) {
> +		if (!nbcon_enter_unsafe(wctxt))
> +			break;
> +
> +		read_poll_timeout_atomic(tegra_utc_tx_readl, val, !(val & TEGRA_UTC_FIFO_FULL),
> +					 0, USEC_PER_SEC, false, tup, TEGRA_UTC_FIFO_STATUS);

No error check?

> +		uart_console_write(&tup->port, wctxt->outbuf + i, 1, tegra_utc_console_putchar);
> +
> +		if (!nbcon_exit_unsafe(wctxt))
> +			break;
> +	}

> +

Unneeded blank line.

> +}

...

> +static int tegra_utc_probe(struct platform_device *pdev)
> +{
> +	const unsigned int *soc_fifosize;
> +	struct device *dev = &pdev->dev;
> +	struct tegra_utc_port *tup;
> +	int ret;
> +
> +	tup = devm_kzalloc(&pdev->dev, sizeof(*tup), GFP_KERNEL);
> +	if (!tup)
> +		return -ENOMEM;
> +
> +	ret = device_property_read_u32(dev, "tx-threshold", &tup->tx_threshold);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "missing tx-threshold device-tree property\n");

' device-tree' is redundant part.

> +	ret = device_property_read_u32(dev, "rx-threshold", &tup->rx_threshold);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "missing rx-threshold device-tree property\n");

Ditto.

Also in a form of

		return dev_err_probe(dev, ret, "missing %s property\n", "rx-threshold");

in both cases the size of the object file will be smaller by a couple of dozens
of bytes.

> +	soc_fifosize = device_get_match_data(&pdev->dev);
> +	tup->fifosize = *soc_fifosize;
> +
> +	tup->tx_base = devm_platform_ioremap_resource_byname(pdev, "tx");
> +	if (IS_ERR(tup->tx_base))
> +		return PTR_ERR(tup->tx_base);
> +
> +	tup->rx_base = devm_platform_ioremap_resource_byname(pdev, "rx");
> +	if (IS_ERR(tup->rx_base))
> +		return PTR_ERR(tup->rx_base);
> +
> +	ret = tegra_utc_setup_port(&pdev->dev, tup);
> +	if (ret)
> +		dev_err_probe(dev, ret, "failed to setup uart port\n");
> +
> +	platform_set_drvdata(pdev, tup);
> +
> +	return tegra_utc_register_port(tup);
> +}

...

> +static int __init tegra_utc_init(void)
> +{
> +	int ret;
> +
> +	ret = uart_register_driver(&tegra_utc_driver);
> +	if (ret)
> +		return ret;
> +
> +	ret = platform_driver_register(&tegra_utc_platform_driver);
> +	if (ret) {
> +		uart_unregister_driver(&tegra_utc_driver);

> +		return ret;
> +	}
> +
> +	return 0;

Just

	return ret;

will be good instead of the above 4 LoCs.

> +}

-- 
With Best Regards,
Andy Shevchenko