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

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

On Thu, Feb 13, 2025 at 03:34:42PM +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.

Btw, neither the commit message nor cover letter explain why the new driver
is needed. There are some serial Tegra drivers already. Is this one completely
different from the existing drivers?

...

> +#define TEGRA_UTC_ENABLE			0x0

It would be nice to use fixed width values for the register offsets,
e.g., 0x000 here.

> +#define TEGRA_UTC_ENABLE_CLIENT_ENABLE		BIT(0)
> +
> +#define TEGRA_UTC_FIFO_THRESHOLD		0x8
> +
> +#define TEGRA_UTC_COMMAND			0xc

Ditto.

> +#define TEGRA_UTC_COMMAND_RESET			BIT(0)
> +#define TEGRA_UTC_COMMAND_FLUSH			BIT(1)

> +#define TEGRA_UTC_DATA				0x20

Ditto.

> +#define TEGRA_UTC_FIFO_STATUS			0x100
> +#define TEGRA_UTC_FIFO_EMPTY			BIT(0)
> +#define TEGRA_UTC_FIFO_FULL			BIT(1)
> +#define TEGRA_UTC_FIFO_REQ			BIT(2)
> +#define TEGRA_UTC_FIFO_OVERFLOW			BIT(3)
> +#define TEGRA_UTC_FIFO_TIMEOUT			BIT(4)
> +
> +#define TEGRA_UTC_FIFO_OCCUPANCY		0x104
> +
> +#define TEGRA_UTC_INTR_STATUS			0x108
> +#define TEGRA_UTC_INTR_SET			0x10c
> +#define TEGRA_UTC_INTR_MASK			0x110
> +#define TEGRA_UTC_INTR_CLEAR			0x114
> +#define TEGRA_UTC_INTR_EMPTY			BIT(0)
> +#define TEGRA_UTC_INTR_FULL			BIT(1)
> +#define TEGRA_UTC_INTR_REQ			BIT(2)
> +#define TEGRA_UTC_INTR_OVERFLOW			BIT(3)
> +#define TEGRA_UTC_INTR_TIMEOUT			BIT(4)

...

> +#if IS_ENABLED(CONFIG_SERIAL_TEGRA_UTC_CONSOLE)
> +#define TEGRA_UTC_DEFAULT_FIFO_THRESHOLD	0x4

Hmm... Is this a register offset? If not, why it's in a hexadecimal format?

> +#define TEGRA_UTC_EARLYCON_MAX_BURST_SIZE	128

...

> +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);

Use dev?

> +	if (!tup)
> +		return -ENOMEM;
> +
> +	ret = device_property_read_u32(dev, "tx-threshold", &tup->tx_threshold);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "missing %s property\n", "tx-threshold");
> +
> +	ret = device_property_read_u32(dev, "rx-threshold", &tup->rx_threshold);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "missing %s property\n", "rx-threshold");
> +
> +	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);

Ditto.

> +	if (ret)
> +		dev_err_probe(dev, ret, "failed to setup uart port\n");
> +
> +	platform_set_drvdata(pdev, tup);
> +
> +	return tegra_utc_register_port(tup);
> +}

...

With the above being addressed, FWIW,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko