| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1455613899.31169.149.camel@linux.intel.com>
Date: Tue, 16 Feb 2016 11:11:39 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Peter Hung <hpeter@...il.com>, linus.walleij@...aro.org,
gnurou@...il.com, gregkh@...uxfoundation.org,
paul.gortmaker@...driver.com, lee.jones@...aro.org,
jslaby@...e.com, gnomes@...rguk.ukuu.org.uk,
peter_hong@...tek.com.tw
Cc: heikki.krogerus@...ux.intel.com, peter@...leysoftware.com,
soeren.grunewald@...y.de, udknight@...il.com,
adam.lee@...onical.com, arnd@...db.de, manabian@...il.com,
scottwood@...escale.com, yamada.masahiro@...ionext.com,
paul.burton@...tec.com, mans@...sr.com, matthias.bgg@...il.com,
ralf@...ux-mips.org, linux-kernel@...r.kernel.org,
linux-gpio@...r.kernel.org, linux-serial@...r.kernel.org,
tom_tsai@...tek.com.tw, Peter Hung <hpeter+linux_kernel@...il.com>
Subject: Re: [PATCH V3 3/4] 8250: 8250_f81504: Add Fintek F81504/508/512
PCIE-to-UART/GPIO UART support
On Tue, 2016-02-16 at 14:55 +0800, Peter Hung wrote:
> This driver is 8250 driver for F81504/508/512, it'll handle the
> serial
> port operation of this device. This module will depend on
> MFD_FINTEK_F81504_CORE.
>
> The serial ports support from 50bps to 1.5Mbps with Linux baudrate
> define excluding 1.0Mbps due to not support 16MHz clock source.
>
> PCI Configuration Space Registers, set:0~11(Max):
> 40h + 8 * set:
> bit7~6: Clock source selector
> 00: 1.8432MHz
> 01: 18.432MHz
> 10: 24MHz
> 11: 14.769MHz
> bit0: UART enable
> 41h + 8 * set:
> bit5~4: RX trigger multiple
> 00: 1x * trigger level
> 01: 2x * trigger level
> 10: 4x * trigger level
> 11: 8x * trigger level
> bit1~0: FIFO Size
> 11: 128Bytes
> 44h + 8 * set: UART IO address (LSB)
> 45h + 8 * set: UART IO address (MSB)
> 47h + 8 * set:
> bit5: RTS invert (bit4 must enable)
> bit4: RTS auto direction enable
> 0: RTS control by MCR
> 1: RTS driven high when TX, otherwise low
>
Few my comments below.
> +++ b/drivers/tty/serial/8250/8250_f81504.c
> @@ -0,0 +1,254 @@
> +#include <linux/pci.h>
> +#include <linux/serial_8250.h>
> +#include <linux/module.h>
> +#include <linux/version.h>
> +#include <linux/mfd/f81504.h>
> +
> +#include "8250.h"
> +
> +static u32 baudrate_table[] = { 1500000, 1152000, 921600 };
> +static u8 clock_table[] = { F81504_CLKSEL_24_MHZ,
> F81504_CLKSEL_18DOT46_MHZ,
> + F81504_CLKSEL_14DOT77_MHZ };
I suggest to replace DOT by _.
> +
> +/* We should do proper H/W transceiver setting before change to
> RS485 mode */
> +static int f81504_rs485_config(struct uart_port *port,
> + struct serial_rs485 *rs485)
> +{
> + u8 setting;
> + u8 *index = (u8 *) port->private_data;
private_data is a type of void *, therefore no need to have an explicit
casting.
> +static int f81504_check_baudrate(u32 baud, size_t *idx)
> +{
> + size_t index;
> + u32 quot, rem;
> +
> + for (index = 0; index < ARRAY_SIZE(baudrate_table); ++index)
Post-increment is also okay.
> {
> + /* Clock source must largeer than desire baudrate */
> + if (baud > baudrate_table[index])
> + continue;
> +
> + quot = DIV_ROUND_CLOSEST(baudrate_table[index],
> baud);
So, how quot is used and is it possible to set, for example, baud rate
as 1000000 or 576000?
> + /* find divisible clock source */
> + rem = baudrate_table[index] % baud;
> +
> + if (quot && !rem) {
> + if (idx)
> + *idx = index;
> + return 0;
> + }
> + }
> +
> + return -EINVAL;
> +}
> +
> +static void f81504_set_termios(struct uart_port *port,
> + struct ktermios *termios, struct ktermios *old)
> +{
> + struct platform_device *pdev = container_of(port->dev,
> + struct platform_device,
> dev);
> + struct pci_dev *dev = to_pci_dev(pdev->dev.parent);
> + unsigned int baud = tty_termios_baud_rate(termios);
> + u8 tmp, *offset = (u8 *) port->private_data;
Same for provate_data as above.
> + /* read current clock source (masked with
> CLOCK_RATE_MASK) */
...
> + /*
> + * direct use 1.8432MHz when baudrate
> smaller then or
> + * equal 115200bps
Check your style of comments in a _whole_ your series.
/*
* Start sentence with Capital letter and end with a period.
*/
> + */
>
> + if (!f81504_check_baudrate(baud, &i)) {
> + /* had optimize value */
/* For one line comment */
> + /*
> + * If it can't found suitable clock source
> but had old
> + * accpetable baudrate, we'll use it
Typo: acceptable.
Baudrate -> baud rate.
> + */
> + baud = tty_termios_baud_rate(old);
> + } else {
> + /*
> + * If it can't found suitable clock source
> and not old
> + * config, we'll direct set 115200bps for
> future use
> + */
> +static int f81504_register_port(struct platform_device *dev,
> + unsigned long address, int idx)
> +{
> + struct pci_dev *pci_dev = to_pci_dev(dev->dev.parent);
> + struct uart_8250_port port;
> + u8 *data;
> +
> + memset(&port, 0, sizeof(port));
> + data = devm_kzalloc(&dev->dev, sizeof(u8), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + *data = idx;
> + port.port.iotype = UPIO_PORT;
> + port.port.irq = pci_dev->irq;
> + port.port.flags = UPF_SKIP_TEST | UPF_FIXED_TYPE |
> UPF_BOOT_AUTOCONF |
> + UPF_SHARE_IRQ;
> + port.port.uartclk = 1843200;
> + port.port.dev = &dev->dev;
> + port.port.iobase = address;
> + port.port.type = PORT_16550A;
> + port.port.fifosize = 128;
> + port.port.rs485_config = f81504_rs485_config;
> + port.port.set_termios = f81504_set_termios;
> + port.tx_loadsz = 32;
> + port.port.private_data = data; /* save current idx */
Not sure you need to allocate memory for that at all, or maybe use a
struct with one member (for now).
> +static SIMPLE_DEV_PM_OPS(f81504_serial_pm_ops,
> f81504_serial_suspend,
> + f81504_serial_resume);
> +
> +static struct platform_driver f81504_serial_driver = {
> + .driver = {
> + .name = F81504_SERIAL_NAME,
> + .owner = THIS_MODULE,
You perhaps don't need this. Check the rest of the modules.
> + .pm = &f81504_serial_pm_ops,
> + },
>
> --- a/drivers/tty/serial/8250/Kconfig
> +++ b/drivers/tty/serial/8250/Kconfig
> @@ -116,6 +116,17 @@ config SERIAL_8250_PCI
> Note that serial ports on NetMos 9835 Multi-I/O cards are
> handled
> by the parport_serial driver, enabled with
> CONFIG_PARPORT_SERIAL.
>
> +config SERIAL_8250_F81504
> + tristate "Fintek F81504/508/512 16550 PCIE device support"
> if EXPERT
> + depends on SERIAL_8250 && MFD_FINTEK_F81504_CORE
> + default SERIAL_8250
> + select RATIONAL
It seems RATIONAL API is not used here.
--
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Intel Finland Oy
Powered by blists - more mailing lists