Re: [PATCH v4 2/2] serial: 8250: Add new 8250-core based Broadcom STB driver

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Mon, Mar 08, 2021 at 12:05:22PM -0500, Al Cooper wrote:
> +static void brcmuart_init_debugfs(struct brcmuart_priv *priv,
> +				  const char *device)
> +{
> +	priv->debugfs_dir = debugfs_create_dir(device, brcmuart_debugfs_root);
> +	priv->debugfs_file = debugfs_create_file("stats", 0444,
> +						 priv->debugfs_dir,
> +						 priv,
> +						 &debugfs_stats_fops);

No need to save this file *, you never do anything with it.

> +}
> +
> +
> +static int brcmuart_probe(struct platform_device *pdev)
> +{
> +	struct resource *regs;
> +	struct device_node *np = pdev->dev.of_node;
> +	const struct of_device_id *of_id = NULL;
> +	struct uart_8250_port *new_port;
> +	struct device *dev = &pdev->dev;
> +	struct brcmuart_priv *priv;
> +	struct clk *baud_mux_clk;
> +	struct uart_8250_port up;
> +	struct resource *irq;
> +	void __iomem *membase = 0;
> +	resource_size_t mapbase = 0;
> +	u32 clk_rate = 0;
> +	int ret;
> +	int x;
> +	int dma_irq;
> +	static const char * const reg_names[REGS_MAX] = {
> +		"uart", "dma_rx", "dma_tx", "dma_intr2", "dma_arb"
> +	};
> +
> +	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> +	if (!irq) {
> +		dev_err(dev, "missing irq\n");
> +		return -EINVAL;
> +	}
> +	priv = devm_kzalloc(dev, sizeof(struct brcmuart_priv),
> +			GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	of_id = of_match_node(brcmuart_dt_ids, np);
> +	if (!of_id || !of_id->data)
> +		priv->rate_table = brcmstb_rate_table;
> +	else
> +		priv->rate_table = of_id->data;
> +
> +	for (x = 0; x < REGS_MAX; x++) {
> +		regs = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +						reg_names[x]);
> +		if (!regs)
> +			break;
> +		priv->regs[x] =	devm_ioremap(dev, regs->start,
> +					     resource_size(regs));
> +		if (IS_ERR(priv->regs[x]))
> +			return PTR_ERR(priv->regs[x]);
> +		if (x == REGS_8250) {
> +			mapbase = regs->start;
> +			membase = priv->regs[x];
> +		}
> +	}
> +
> +	/* We should have just the uart base registers or all the registers */
> +	if (x != 1 && x != REGS_MAX) {
> +		dev_warn(dev, "%s registers not specified\n", reg_names[x]);
> +		return -EINVAL;
> +	}
> +
> +	/* if the DMA registers were specified, try to enable DMA */
> +	if (x > REGS_DMA_RX) {
> +		if (brcmuart_arbitration(priv, 1) == 0) {
> +			u32 txrev = 0;
> +			u32 rxrev = 0;
> +
> +			txrev = udma_readl(priv, REGS_DMA_RX, UDMA_RX_REVISION);
> +			rxrev = udma_readl(priv, REGS_DMA_TX, UDMA_TX_REVISION);
> +			if ((txrev >= UDMA_TX_REVISION_REQUIRED) &&
> +				(rxrev >= UDMA_RX_REVISION_REQUIRED)) {
> +
> +				/* Enable the use of the DMA hardware */
> +				priv->dma_enabled = true;
> +			} else {
> +				brcmuart_arbitration(priv, 0);
> +				dev_err(dev,
> +					"Unsupported DMA Hardware Revision\n");
> +			}
> +		} else {
> +			dev_err(dev,
> +				"Timeout arbitrating for UART DMA hardware\n");
> +		}
> +	}
> +
> +	of_property_read_u32(np, "clock-frequency", &clk_rate);
> +
> +	/* See if a Baud clock has been specified */
> +	baud_mux_clk = of_clk_get_by_name(np, "sw_baud");
> +	if (IS_ERR(baud_mux_clk)) {
> +		if (PTR_ERR(baud_mux_clk) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;
> +		dev_info(dev, "BAUD MUX clock not specified\n");
> +	} else {
> +		dev_info(dev, "BAUD MUX clock found\n");

These are not "info" messages, make them dev_dbg() please.

> +		ret = clk_prepare_enable(baud_mux_clk);
> +		if (ret)
> +			return ret;
> +		priv->baud_mux_clk = baud_mux_clk;
> +		init_real_clk_rates(dev, priv);
> +		clk_rate = priv->default_mux_rate;
> +	}
> +
> +	if (clk_rate == 0) {
> +		dev_err(dev, "clock-frequency or clk not defined\n");
> +		return -EINVAL;
> +	}
> +
> +	dev_info(dev, "DMA is %senabled\n", priv->dma_enabled ? "" : "not ");

When a driver is working properly, no need to say anything in the kernel
log.

This should be dev_info().

Much better overall, thanks for making the changes.

thanks,

greg k-h