Messages in this thread | | | Date | Tue, 24 Oct 2023 07:54:35 +0200 | Subject | Re: [PATCH v6 tty-next 2/2] serial: 8250: add driver for NI UARTs | From | Jiri Slaby <> |
| |
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@ni.com>"); > +MODULE_AUTHOR("Karthik Manamcheri <karthik.manamcheri@ni.com>");
Do the addresses work? Why not CC them?
thanks, -- js suse labs
| |