[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ade1f1a4-0fa9-4233-9061-5a76178c94fb@kernel.org>
Date: Tue, 24 Oct 2023 07:54:35 +0200
From: Jiri Slaby <jirislaby@...nel.org>
To: Brenda Streiff <brenda.streiff@...com>
Cc: Gratian Crisan <gratian.crisan@...com>,
Jason Smith <jason.smith@...com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org
Subject: Re: [PATCH v6 tty-next 2/2] serial: 8250: add driver for NI UARTs
On 23. 10. 23, 23:04, Brenda Streiff wrote:
> The National Instruments (NI) 16550 is a 16550-like UART with larger
> FIFOs and embedded RS-232/RS-485 transceiver control circuitry. This
> patch adds a driver that can operate this UART, which is used for
> onboard serial ports in several NI embedded controller designs.
>
> Portions of this driver were originally written by Jaeden Amero and
> Karthik Manamcheri, with extensive cleanups and refactors since.
...
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_ni.c
...
> +static int ni16550_probe(struct platform_device *pdev)
> +{
> + const struct ni16550_device_info *info;
> + struct device *dev = &pdev->dev;
> + struct uart_8250_port uart = {};
> + unsigned int prescaler = 0;
> + struct ni16550_data *data;
> + const char *portmode;
> + int txfifosz, rxfifosz;
Why not unsigned?
> + int rs232_property;
bool
> + int ret;
> + int irq;
> +
> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + spin_lock_init(&uart.port.lock);
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0)
> + return irq;
> +
> + ret = ni16550_get_regs(pdev, &uart.port);
> + if (ret < 0)
> + return ret;
> +
> + /* early setup so that serial_in()/serial_out() work */
> + serial8250_set_defaults(&uart);
> +
> + info = device_get_match_data(dev);
> +
> + uart.port.dev = dev;
> + uart.port.irq = irq;
> + uart.port.irqflags = IRQF_SHARED;
> + uart.port.flags = UPF_SHARE_IRQ | UPF_BOOT_AUTOCONF
> + | UPF_FIXED_PORT | UPF_FIXED_TYPE;
> + uart.port.startup = ni16550_port_startup;
> + uart.port.shutdown = ni16550_port_shutdown;
> +
> + /*
> + * Hardware instantiation of FIFO sizes are held in registers.
> + */
> + txfifosz = ni16550_read_fifo_size(&uart, NI16550_TFS_OFFSET);
> + rxfifosz = ni16550_read_fifo_size(&uart, NI16550_RFS_OFFSET);
> +
> + dev_dbg(dev, "NI 16550 has TX FIFO size %d, RX FIFO size %d\n",
> + txfifosz, rxfifosz);
> +
> + uart.port.type = PORT_16550A;
> + uart.port.fifosize = txfifosz;
> + uart.tx_loadsz = txfifosz;
> + uart.fcr = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10;
> + uart.capabilities = UART_CAP_FIFO | UART_CAP_AFE | UART_CAP_EFR;
> +
> + /*
> + * Declaration of the base clock frequency can come from one of:
> + * - static declaration in this driver (for older ACPI IDs)
> + * - a "clock-frquency" ACPI or OF device property
> + * - an associated OF clock definition
> + */
> + if (info->uartclk)
> + uart.port.uartclk = info->uartclk;
> + if (device_property_read_u32(dev, "clock-frequency",
> + &uart.port.uartclk)) {
> + data->clk = devm_clk_get_optional_enabled(dev, NULL);
> + if (data->clk)
!IS_ERR(data->clk)
> + uart.port.uartclk = clk_get_rate(data->clk);
> + }
> +
> + if (!uart.port.uartclk) {
> + dev_err(dev, "unable to determine clock frequency!\n");
> + ret = -ENODEV;
> + goto err;
> + }
> +
> + if (info->prescaler)
> + prescaler = info->prescaler;
> + device_property_read_u32(dev, "clock-prescaler", &prescaler);
> +
> + if (prescaler != 0) {
> + uart.port.set_mctrl = ni16550_set_mctrl;
> + ni16550_config_prescaler(&uart, (u8)prescaler);
> + }
> +
> + /*
> + * The determination of whether or not this is an RS-485 or RS-232 port
> + * can come from a device property (if present), or it can come from
> + * the PMR (if present), and otherwise we're solely an RS-485 port.
> + *
> + * This is a device-specific property, and thus has a vendor-prefixed
> + * "ni,serial-port-mode" form as a devicetree binding. However, there
> + * are old devices in the field using "transceiver" as an ACPI device
> + * property, so we have to check for that as well.
> + */
> + if (!device_property_read_string(dev, "ni,serial-port-mode", &portmode) ||
> + !device_property_read_string(dev, "transceiver", &portmode)) {
> + rs232_property = strncmp(portmode, "RS-232", 6) == 0;
> +
> + dev_dbg(dev, "port is in %s mode (via device property)",
\n missing here and there.
> + rs232_property ? "RS-232" : "RS-485");
> + } else if (info->flags & NI_HAS_PMR) {
> + rs232_property = is_pmr_rs232_mode(&uart);
> +
> + dev_dbg(dev, "port is in %s mode (via PMR)",
> + rs232_property ? "RS-232" : "RS-485");
> + } else {
> + rs232_property = 0;
> +
> + dev_dbg(dev, "port is fixed as RS-485");
> + }
> +
> + if (!rs232_property) {
> + /*
> + * Neither the 'transceiver' property nor the PMR indicate
> + * that this is an RS-232 port, so it must be an RS-485 one.
> + */
> + ni16550_rs485_setup(&uart.port);
> + }
> +
> + ret = serial8250_register_8250_port(&uart);
> + if (ret < 0)
> + goto err;
> + data->line = ret;
> +
> + platform_set_drvdata(pdev, data);
> + return 0;
> +
> +err:
> + clk_disable_unprepare(data->clk);
But you use devm_?
> + return ret;
> +}
> +
> +static int ni16550_remove(struct platform_device *pdev)
> +{
> + struct ni16550_data *data = platform_get_drvdata(pdev);
> +
> + clk_disable_unprepare(data->clk);
> + serial8250_unregister_port(data->line);
This should be likely inverted. But I don't know if it matters here at all.
Actually, devm_ again, so the first is unnecessary?
> + return 0;
> +}
...
> +MODULE_AUTHOR("Jaeden Amero <jaeden.amero@...com>");
> +MODULE_AUTHOR("Karthik Manamcheri <karthik.manamcheri@...com>");
Do the addresses work? Why not CC them?
thanks,
--
js
suse labs
Powered by blists - more mailing lists