| 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: <20150914133301.GC19567@localhost>
Date: Mon, 14 Sep 2015 15:33:01 +0200
From: Johan Hovold <johan@...nel.org>
To: Peter Hung <hpeter@...il.com>
Cc: johan@...nel.org, gregkh@...uxfoundation.org,
linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
tom_tsai@...tek.com.tw, peter_hong@...tek.com.tw,
Peter Hung <hpeter+linux_kernel@...il.com>
Subject: Re: [PATCH V5 1/1] usb:serial add Fintek F81532/534 driver
On Tue, Jul 21, 2015 at 09:58:19AM +0800, Peter Hung wrote:
> This driver is for Fintek F81532/F81534 USB to Serial Ports IC.
>
> Features:
> 1. F81534 is 1-to-4 & F81532 is 1-to-2 serial ports IC
> 2. Support Baudrate from B50 to B1500000 (excluding B1000000).
> 3. The RTS signal can be transformed their behavior with
> configuration by default ioctl TIOCGRS485/TIOCSRS485
> (for RS232/RS485/RS422 with transceiver)
>
> If the driver setting with SER_RS485_ENABLED, the RTS signal will
> high with not in TX and low with in TX.
>
> If the driver setting with SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND,
> the RTS signal will low with not in TX and high with in TX.
>
> 4. There are 4x3 output-only ic pins to control transceiver mode.
Are these pins only used to control the transceiver mode, and not for
general purpose IO?
> It's can be controlled via gpiolib. We could found the gpio
> number from /sys/class/tty/ttyUSB[x]/device/gpiochap[yyy] where
> x is F81532/534 serial port and yyy is gpiochip number.
>
> After we found chip number, we can export 3 gpios(M2/M1/M0) per
> serial port by
> echo yyy > /sys/class/gpio/export
> echo yyy+1 > /sys/class/gpio/export
> echo yyy+2 > /sys/class/gpio/export
>
> then we can control it with
> echo [M0 value] > /sys/class/gpio/gpio[yyy]/value
> echo [M1 value] > /sys/class/gpio/gpio[yyy+1]/value
> echo [M2 value] > /sys/class/gpio/gpio[yyy+2]/value
> which M0/M1/M2 as your desired value, value is only 0 or 1.
>
> When configure complete, It's a must to free all gpio by
> echo yyy > /sys/class/gpio/unexport
> echo yyy+1 > /sys/class/gpio/unexport
> echo yyy+2 > /sys/class/gpio/unexport
You don't need to document how to use the gpiolib sysfs interface here.
Just mention that the driver exports these gpios.
> The driver will "save" gpio configure after we release
> all gpio of a serial port.
>
> For examples to change mode & gpio with F81532/534
> Evalaution Board.
>
> F81532 EVB
> port0: F81437 (RS232 only)
> port1: F81439 (RS232/RS485/RS422 ... etc.)
> F81534 EVB
> port0/1: F81437 (RS232 only)
> port2/3: F81439 (RS232/RS485/RS422 ... etc.)
>
> 1. RS232 Mode (Default IC Mode)
> 1. Set struct serial_rs485 flags "without" SER_RS485_ENABLED
> (control F81532/534 RTS control)
> 2. Set M2/M1/M0 as 0/0/1
> (control F81532/534 output pin to control transceiver mode)
>
> 2. RS485 Mode (RTS Low when TX Mode)
> 1. Set struct serial_rs485 flags with SER_RS485_ENABLED
> 2. Set M2/M1/M0 as 0/1/0
>
> 3. RS485 Mode (RTS High when TX Mode)
> 1. Set struct serial_rs485 flags with SER_RS485_ENABLED and
> SER_RS485_RTS_ON_SEND
> 2. Set M2/M1/M0 as 0/1/1
>
> 4. RS422 Mode
> 1. The RTS mode is dont care.
> 2. Set M2/M1/M0 as 0/0/0
I don't think all gpios should be exported for these ports if they have
special functions that the driver could control transparently (e.g. for
SER_RS485_RTS_ON_SEND).
We need to come up with a way to deal with rs422 mode though.
> Please reference https://bitbucket.org/hpeter/fintek-general/src/
> with f81534/tools to get set_gpio.c & set_mode.c. Please use it
> carefully.
>
> Changelog
> v5
> 1. Change the configure data address F81534_CUSTOM_ADDRESS_START
> from 0x4000 to 0x2f00 (for MP F/W ver:AA66)
> 2. Change f81534_port_disable/enable() from H/W mode to S/W mode
> It'll skip all rx data when port is not opened.
> 3. Some function modifier add with static (Thanks for Paul Bolle)
> 4. It's will direct return when count=0 in f81534_write() to
> reduce spin_lock usage.
>
> v4
> 1. clearify f81534_process_read_urb() with
> f81534_process_per_serial_block(). (referenced from mxuport.c)
> 2. We limited f81534_write() max tx kfifo with 124Bytes.
> Original subsystem is designed for auto tranmiting fifo data
> if available. But we must wait for tx_empty for next tx data
> (H/W design).
>
> With this kfifo size limit, we can use generic subsystem api with
> f81534_write(). When usb_serial_generic_write_start() called after
> first write URB complete, the fifo will no data. The generic
> subsystem of write will go to idle state. Until we received
> TX_EMPTY and release write spinlock, the fifo will fill max
> 124Bytes by following f81534_write().
>
> v3
> 1. Migrate read, write and some routine from custom code to usbserial
> subsystem callback function.
> 2. Use more defines to replece magic numbers to make it meaningful
> 3. Make more comments as document in source code.
>
> v2
> 1. v1 version submit to staging tree, but Greg KH advised me to
> cleanup source code & re-submit it to correct subsystem
> 2. Remove all custom ioctl commands
It's very nice to have this changelog but please put it below the cutoff
line below as it does not need to go into the commit message itself once
applied.
> Signed-off-by: Peter Hung <hpeter+linux_kernel@...il.com>
> ---
That is, put the change log here.
> drivers/usb/serial/Kconfig | 10 +
> drivers/usb/serial/Makefile | 1 +
> drivers/usb/serial/f81534.c | 3279 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 3290 insertions(+)
> create mode 100644 drivers/usb/serial/f81534.c
>
> diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
> index 56ecb8b..0642864 100644
> --- a/drivers/usb/serial/Kconfig
> +++ b/drivers/usb/serial/Kconfig
> @@ -255,6 +255,16 @@ config USB_SERIAL_F81232
> To compile this driver as a module, choose M here: the
> module will be called f81232.
>
> +config USB_SERIAL_F8153X
> + tristate "USB Fintek F81532/534 Multi-Ports Serial Driver"
> + help
> + Say Y here if you want to use the Fintek F81532/534 Multi-Ports
> + usb to serial adapter.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called f81534.
> +
> +
> config USB_SERIAL_GARMIN
> tristate "USB Garmin GPS driver"
> help
> diff --git a/drivers/usb/serial/Makefile b/drivers/usb/serial/Makefile
> index 349d9df..9e43b7b 100644
> --- a/drivers/usb/serial/Makefile
> +++ b/drivers/usb/serial/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_USB_SERIAL_EDGEPORT) += io_edgeport.o
> obj-$(CONFIG_USB_SERIAL_EDGEPORT_TI) += io_ti.o
> obj-$(CONFIG_USB_SERIAL_EMPEG) += empeg.o
> obj-$(CONFIG_USB_SERIAL_F81232) += f81232.o
> +obj-$(CONFIG_USB_SERIAL_F8153X) += f81534.o
> obj-$(CONFIG_USB_SERIAL_FTDI_SIO) += ftdi_sio.o
> obj-$(CONFIG_USB_SERIAL_GARMIN) += garmin_gps.o
> obj-$(CONFIG_USB_SERIAL_IPAQ) += ipaq.o
> diff --git a/drivers/usb/serial/f81534.c b/drivers/usb/serial/f81534.c
> new file mode 100644
> index 0000000..ee4b1f8
> --- /dev/null
> +++ b/drivers/usb/serial/f81534.c
> @@ -0,0 +1,3279 @@
> +/*
> + * F81532/F81534 USB to Serial Ports Bridge
> + *
> + * F81532 => 2 Serial Ports
> + * F81534 => 4 Serial Ports
> + *
> + * Copyright (C) 2014 Tom Tsai (Tom_Tsai@...tek.com.tw)
> + *
> + * The F81532/F81534 had 1 control endpoint for setting,
> + * 1 endport bulk-out for all serial port write out and
> + * 1 endport bulk-in for all serial port read in.
s/endport/endpoint/
Please run this this patch through a spell-checker, there seems to be
quite a few typos.
> + *
> + * write urb is fixed with 512bytes, per serial port used 128Bytes.
> + * is can be described by f81534_prepare_write_buffer()
> + *
> + * read urb is 512Bytes max. per serial port used 128Bytes.
> + * is can be described by f81534_process_read_urb(), it' maybe
> + * received with 128x1,2,3,4 bytes.
> + *
> + * We can control M0/M1/M2 per ports by gpiolib, it's recommend
> + * sequence to request 3 pins, change value and free 3 pins. We'll
> + * save it when M0/M1/M2 pin all released this port to reduce write
> + * operations.
What do you mean by this?
> + *
> + *
> + * Features:
> + * 1. F81534 is 1-to-4 & F81532 is 1-to-2 serial ports IC
> + * 2. Support Baudrate from B50 to B1500000 (excluding B1000000).
> + * 3. The RTS signal can be transformed their behavior with
> + * configuration by default ioctl TIOCGRS485/TIOCSRS485
> + * (for RS232/RS485/RS422 with transceiver)
> + *
> + * If the driver setting with SER_RS485_ENABLED, the RTS signal will
> + * high with not in TX and low with in TX.
> + *
> + * If the driver setting with SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND,
> + * the RTS signal will low with not in TX and high with in TX.
> + *
> + * 4. There are 4x3 output-only ic pins to control transceiver mode.
> + * It's can be controlled via gpiolib. We could found the gpio
> + * number from /sys/class/tty/ttyUSB[x]/device/gpiochap[yyy] where
> + * x is F81532/534 serial port and yyy is gpiochip number.
> + *
> + * After we found chip number, we can export 3 gpios(M2/M1/M0) per
> + * serial port by
> + * echo yyy > /sys/class/gpio/export
> + * echo yyy+1 > /sys/class/gpio/export
> + * echo yyy+2 > /sys/class/gpio/export
> + *
> + * then we can control it with
> + * echo [M0 value] > /sys/class/gpio/gpio[yyy]/value
> + * echo [M1 value] > /sys/class/gpio/gpio[yyy+1]/value
> + * echo [M2 value] > /sys/class/gpio/gpio[yyy+2]/value
> + * which M0/M1/M2 as your desired value, value is only 0 or 1.
> + *
> + * When configure complete, It's a must to free all gpio by
> + * echo yyy > /sys/class/gpio/unexport
> + * echo yyy+1 > /sys/class/gpio/unexport
> + * echo yyy+2 > /sys/class/gpio/unexport
> + *
> + * The driver will "save" gpio configure after we release
> + * all gpio of a serial port.
> + *
> + * For examples to change mode & gpio with F81532/534
> + * Evalaution Board.
> + *
> + * F81532 EVB
> + * port0: F81437 (RS232 only)
> + * port1: F81439 (RS232/RS485/RS422 ... etc.)
> + * F81534 EVB
> + * port0/1: F81437 (RS232 only)
> + * port2/3: F81439 (RS232/RS485/RS422 ... etc.)
> + *
> + * 1. RS232 Mode (Default IC Mode)
> + * 1. Set struct serial_rs485 flags "without" SER_RS485_ENABLED
> + * (control F81532/534 RTS control)
> + * 2. Set M2/M1/M0 as 0/0/1
> + * (control F81532/534 output pin to control transceiver mode)
> + *
> + * 2. RS485 Mode (RTS Low when TX Mode)
> + * 1. Set struct serial_rs485 flags with SER_RS485_ENABLED
> + * 2. Set M2/M1/M0 as 0/1/0
> + *
> + * 3. RS485 Mode (RTS High when TX Mode)
> + * 1. Set struct serial_rs485 flags with SER_RS485_ENABLED and
> + * SER_RS485_RTS_ON_SEND
> + * 2. Set M2/M1/M0 as 0/1/1
> + *
> + * 4. RS422 Mode
> + * 1. The RTS mode is dont care.
> + * 2. Set M2/M1/M0 as 0/0/0
> + *
> + * Please reference https://bitbucket.org/hpeter/fintek-general/src/
> + * with f81534/tools to get set_gpio.c & set_mode.c. Please use it
> + * carefully.
> + */
> +#include <asm/unaligned.h>
You don't need this include it seems.
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/tty.h>
> +#include <linux/tty_driver.h>
> +#include <linux/tty_flip.h>
> +#include <linux/module.h>
> +#include <linux/serial.h>
> +#include <linux/usb.h>
> +#include <linux/usb/serial.h>
> +#include <linux/serial_reg.h>
> +#include <linux/uaccess.h>
> +#include <linux/version.h>
> +#include <linux/workqueue.h>
You don't need the above two includes.
> +#include <linux/kfifo.h>
> +#include <linux/fs.h>
Why do you include fs.h?
> +#include <linux/mutex.h>
> +#include <linux/gpio.h>
> +
> +#define F81534_TIOCGET_RETRY 5
> +
> +/* Serial Port register Address */
> +#define SERIAL_BASE_ADDRESS 0x1200
> +#define RECEIVE_BUFFER_REGISTER (0x00 + SERIAL_BASE_ADDRESS)
> +#define TRANSMIT_HOLDING_REGISTER (0x00 + SERIAL_BASE_ADDRESS)
> +#define INTERRUPT_ENABLE_REGISTER (0x01 + SERIAL_BASE_ADDRESS)
> +#define INTERRUPT_IDENT_REGISTER (0x02 + SERIAL_BASE_ADDRESS)
> +#define FIFO_CONTROL_REGISTER (0x02 + SERIAL_BASE_ADDRESS)
> +#define LINE_CONTROL_REGISTER (0x03 + SERIAL_BASE_ADDRESS)
> +#define MODEM_CONTROL_REGISTER (0x04 + SERIAL_BASE_ADDRESS)
> +#define LINE_STATUS_REGISTER (0x05 + SERIAL_BASE_ADDRESS)
> +#define MODEM_STATUS_REGISTER (0x06 + SERIAL_BASE_ADDRESS)
> +#define CLK_SEL_REGISTER (0x08 + SERIAL_BASE_ADDRESS)
> +#define CONFIG1_REGISTER (0x09 + SERIAL_BASE_ADDRESS)
> +#define SADDRESS_REGISTER (0x0a + SERIAL_BASE_ADDRESS)
> +#define SADEN_REGISTER (0x0b + SERIAL_BASE_ADDRESS)
> +#define DIVISOR_LATCH_LSB (0x00 + SERIAL_BASE_ADDRESS)
> +#define DIVISOR_LATCH_MSB (0x01 + SERIAL_BASE_ADDRESS)
> +#define SCRATCH_PAD_REGISTER (0x07 + SERIAL_BASE_ADDRESS)
How about keeping these sorted by offset.
> +#define MSR_ACCESS_SHADOW_REG 0x5a00
> +
> +#define F81534_RESERVE_ADDRESS_START 0x3000
> +#define F81534_RESERVE_SIZE 8
> +
> +#define F81534_CUSTOM_ADDRESS_START 0x2f00
> +#define F81534_CUSTOM_TOTAL_SIZE 0x10
> +#define F81534_CUSTOM_DATA_SIZE 0x10
> +#define F81534_CUSTOM_MAX_IDX \
> + (F81534_CUSTOM_TOTAL_SIZE/F81534_CUSTOM_DATA_SIZE)
> +#define F81534_CUSTOM_NO_CUSTOM_DATA (-1)
> +#define F81534_CUSTOM_VALID_TOKEN 0xf0
> +#define F81534_CONF_OFFSET 1
> +#define F81534_CONF_SIZE 4
> +
> +#define F81534_MAX_DATA_BLOCK 64
> +#define F81534_MAX_BUS_RETRY 2000
> +
> +/* default urb timeout for usb operations */
> +#define F81534_USB_MAX_RETRY 5
> +#define F81534_USB_TIMEOUT 1000
> +#define F81534_CONTROL_BYTE 0x1B
> +#define F81534_SET_GET_REGISTER 0xA0
> +
> +#define F81534_NUM_PORT 4
> +#define F81534_UNUSED_PORT 0xff
> +
> +#define F81534_WRITE_BUFFER_SIZE (512L) /* size of write buffer */
> +#define F81534_READ_BUFFER_SIZE (512L)
No need for L here.
The read buffer size is never used?
> +#define IC_NAME "f81534"
> +#define DRIVER_DESC \
> + "Fintek USB to Serial Ports Driver (F81532/F81534-Evaluation Board)"
> +#define FINTEK_VENDOR_ID 0x1934
> +#define FINTEK_DEVICE_ID 0x1202 /* RS232 four port */
> +#define F81534_MAX_TX_SIZE 124L
> +#define F81534_FIFO_SIZE 128L
Drop the L.
> +#define F81534_RECEIVE_BLOCK_SIZE 128
> +
> +#define F81534_TOKEN_RECEIVE 0x01
> +#define F81534_TOKEN_WRITE 0x02
> +#define F81534_TOKEN_TX_EMPTY 0x03
> +#define F81534_TOKEN_MSR_CHANGE 0x04
> +
> +#define F81534_BUS_BUSY 0x03
> +#define F81534_BUS_IDLE 0x04
> +#define F81534_BUS_READ_DATA 0x1004
> +#define F81534_BUS_REG_STATUS 0x1003
> +#define F81534_BUS_REG_START 0x1002
> +#define F81534_BUS_REG_END 0x1001
> +
> +#define F81534_CMD_READ 0x03
> +#define F81534_CMD_ENABLE_WR 0x06
> +#define F81534_CMD_PROGRAM 0x02
> +#define F81534_CMD_ERASE 0x20
> +#define F81534_CMD_READ_STATUS 0x05
> +
> +#define F81534_MEDIA_BUSY_STATUS 0x03
> +
> +#define F81534_1X_RXTRIGGER 0xc3
> +#define F81534_8X_RXTRIGGER 0xcf
> +
> +#define F81534_DEFAULT_BAUD_RATE 9600
> +#define F81534_MAX_BAUDRATE 1500000
> +
> +#define F81534_RS232_FLAG 0x00
> +#define F81534_RS485_FLAG 0x03
> +#define F81534_RS485_1_FLAG 0x01
> +#define F81534_MODE_MASK 0x03
> +
> +static int m_f81534_max_tx_size = F81534_MAX_TX_SIZE;
This should not be static, either use the define directly or put it
in the private data.
> +
> +enum eUartMode {
Don't use CamelCase.
> + eModeRS422,
> + eModeRS232,
> + eModeRS485,
> + eModeRS485_1,
> + eModeRS422_term,
> + eModeRS232_coexist,
> + eModeRS485_1_term,
> + eModeShutdown,
> + eModeInvalid,
> +};
> +
> +struct internal_data {
> + unsigned int address;
> + unsigned int size;
> + unsigned char buf[F81534_MAX_DATA_BLOCK];
> +};
This struct is never used.
Please go though this driver and remove structures and code that you
don't use.
> +
> +struct f81534_pin_config_data {
> + enum eUartMode eForceUartMode;
> + enum eUartMode eGPIOMode;
> + int address[9];
> + int offset[9];
> +};
> +
> +struct reg_value {
> + int reg_address;
> + int reg_offset;
> + int reg_bit;
Looks like this should be u16 is some other appropriate size.
> +};
> +
> +struct pin_data {
> + struct reg_value port_mode_1;
> + struct reg_value port_mode_0;
> + struct reg_value port_io;
> +};
> +
> +struct out_pin {
> + struct pin_data m1;
> + struct pin_data m2;
> + struct pin_data sd;
> +};
> +
> +struct io_map_value {
> + int product_id;
> + int max_port;
> + enum eUartMode mode;
> + struct out_pin port[MAX_NUM_PORTS + 1];
> +};
> +
> +/* The following magic numbers is F81532/534 output pin
> + * register maps
> + */
Please document this (and the above structures) better.
Preferred comment style for multi-line comments is
/*
* ...
*/
> +static struct io_map_value f81534_rs232_control = {
> + FINTEK_DEVICE_ID, F81534_NUM_PORT, eModeRS232,
> + {
> + /* please reference f81439 io port */
> + {
> + {{0x2ad5, 4, 0}, {0x2ad4, 4, 1}, {0x2a90, 4, 0},},
> + {{0x2ad5, 5, 0}, {0x2ad4, 5, 1}, {0x2a90, 5, 0},},
> + {{0x2add, 7, 0}, {0x2adc, 7, 1}, {0x2ae8, 7, 1},},
> + },
> + {
> + {{0x2add, 3, 0}, {0x2adc, 3, 1}, {0x2ae8, 3, 0},},
> + {{0x2add, 0, 0}, {0x2adc, 0, 1}, {0x2ae8, 0, 0},},
> + {{0x2add, 6, 0}, {0x2adc, 6, 1}, {0x2ae8, 6, 1},},
> + },
> + {
> + {{0x2ad3, 6, 0}, {0x2ad2, 6, 1}, {0x2a80, 6, 0},},
> + {{0x2add, 2, 0}, {0x2adc, 2, 1}, {0x2ae8, 2, 0},},
> + {{0x2ad5, 0, 0}, {0x2ad4, 0, 1}, {0x2a90, 0, 1},},
> + },
> + {
> + {{0x2ad5, 1, 0}, {0x2ad4, 1, 1}, {0x2a90, 1, 0},},
> + {{0x2ad5, 2, 0}, {0x2ad4, 2, 1}, {0x2a90, 2, 0},},
> + {{0x2ad5, 3, 0}, {0x2ad4, 3, 1}, {0x2a90, 3, 1},},
> + },
> + },
> +
> +};
<snip>
> +static struct io_map_value *f81534_mode_control[eModeInvalid] = {
> + &f81534_rs422_control,
> + &f81534_rs232_control,
> + &f81534_rs485_control,
> + &f81534_rs485_1_control,
> + &f81534_rs422_term_control,
> + &f81534_rs232_coexist_control,
> + &f81534_rs485_1_term_control,
> + &f81534_shutdown_control,
> +};
Looks like all of the above should be const.
> +
> +
> +static const struct usb_device_id id_table[] = {
> + {USB_DEVICE(FINTEK_VENDOR_ID, FINTEK_DEVICE_ID)},
> + {} /* Terminating entry */
> +};
> +
> +MODULE_DEVICE_TABLE(usb, id_table);
> +
> +enum eIC_Type {
> + eIC_F81530,
> + eIC_F81531,
> + eIC_F81532,
> + eIC_F81533,
> + eIC_F81534,
> +};
> +
> +static const char * const m_ic_name[] = {
> + "F81530",
> + "F81531",
> + "F81532",
> + "F81533",
> + "F81534",
> +};
Drop the eXXX and m_xxx notations. Just just a f81534 prefix.
> +
> +struct f81534_serial_private {
> + enum eIC_Type ic_type;
> + struct usb_serial *serial;
> + bool phy_port_in_use[F81534_NUM_PORT];
> + spinlock_t write_urb_lock;
> + spinlock_t msr_lock;
> + u8 shadowMSR[F81534_NUM_PORT];
Avoid CamelCase, just call it shadow_msr.
Why aren't these in the port private data?
> + unsigned long msr_time[F81534_NUM_PORT];
> + u8 port_mapping[F81534_NUM_PORT];
> + struct mutex msr_mutex;
> + struct mutex change_mode_mutex;
> + u8 reserve_data[F81534_RESERVE_SIZE];
> + u32 custom_idx;
> +}
> +
> +struct f81534_port_private {
> + u8 shadowMCR;
> + u8 shadowLCR;
> + struct usb_serial_port *port;
> + u32 tx, rx;
> + u32 currentBaudRate;
> + struct f81534_pin_config_data port_pin_data;
> + struct gpio_chip f81534_gpio_chip;
> + atomic_t gpio_active;
> + atomic_t port_active;
> +};
> +
> +static void f81534_update_msr(struct usb_serial_port *port,
> + unsigned char *ch);
> +
> +static int f81534_setup_urbs(struct usb_serial *serial);
> +
> +static int f81534_set_normal_register(struct usb_device *dev,
> + u16 reg, u8 data);
> +
> +static int f81534_get_normal_register(struct usb_device *dev,
> + u16 reg, u8 *data);
> +
> +static int f81534_getregister(struct usb_device *dev,
> + u8 uart, u16 reg, u8 *data);
> +
> +static void f81534_dtr_rts(struct usb_serial_port *port, int on);
> +
> +static int f81534_set_port_mode(struct usb_serial_port *port,
> + enum eUartMode eMode);
> +
> +static int f81534_save_configure_data(struct usb_serial_port *port);
> +
> +static int f81534_switch_gpio_mode(struct usb_serial_port
> + *serial_port, int mode);
> +
> +static void f81534_wakeup_all_port(struct usb_serial *serial);
> +
> +static int f81534_logic_to_phy_port(struct usb_serial *usbserial, int logic)
> +{
> + int index;
> + struct f81534_serial_private *serial_priv =
> + usb_get_serial_data(usbserial);
Odd line break; indent continuation lines at least two tabs further.
> +
> + for (index = 0; index < F81534_NUM_PORT; ++index, --logic) {
> + if ((serial_priv->port_mapping[index] != F81534_UNUSED_PORT)
> + && !logic)
> + return serial_priv->port_mapping[index];
> + }
> +
> + dev_err(&usbserial->dev->dev, "%s could found mapping: logic: %x\n",
> + __func__, logic);
"could not find"?
> + return F81534_UNUSED_PORT;
> +}
> +
> +static int f81534_phy_to_logic_port(struct usb_serial *usbserial, int phy)
> +{
> + int index;
> + struct f81534_serial_private *serial_priv =
> + usb_get_serial_data(usbserial);
> +
> + for (index = 0; index < F81534_NUM_PORT; ++index)
> + if (serial_priv->port_mapping[index] == phy)
> + return index;
> +
> + dev_err(&usbserial->dev->dev, "%s could found mapping: phy: %x\n",
> + __func__, phy);
> + return F81534_UNUSED_PORT;
> +}
> +
> +static int f81534_port_to_phy_index(struct usb_serial_port *port)
> +{
> + return f81534_logic_to_phy_port(port->serial, port->port_number);
> +}
> +
> +static int f81534_port_index(struct usb_serial_port *port)
> +{
> + return port->port_number;
> +}
> +
Please describe in a comment what this function does here.
> +static int f81534_command_delay(struct usb_serial *usbserial)
> +{
> + unsigned int count = F81534_MAX_BUS_RETRY;
> + unsigned char tmp;
> + int status;
> + struct usb_device *dev = usbserial->dev;
> +
> + do {
> + status = f81534_get_normal_register(dev,
> + F81534_BUS_REG_STATUS, &tmp);
> + if (status) {
> + dev_err(&dev->dev,
> + "%s: failed, %d\n",
> + __func__, __LINE__);
Please spell out what went wrong, for example,
dev_err(..., "failed to get bus status: %d", status);
and include the error code instead of the line number (fix this
throughout).
> + return status;
> + }
> +
> + if (tmp & F81534_BUS_BUSY)
> + continue;
> +
> + if (tmp & F81534_BUS_IDLE)
> + break;
> +
> + } while (--count);
> +
> + if (!count) {
> + dev_err(&usbserial->dev->dev, "%s: max retry exceed !!!\n",
> + __func__);
There's rarely any need for exclamation marks in error messages.
> + return -EIO;
> + }
> +
> + status = f81534_set_normal_register(dev, F81534_BUS_REG_STATUS,
> + tmp & ~F81534_BUS_IDLE);
> + if (status) {
> + dev_err(&dev->dev, "%s: failed, %d\n", __func__, __LINE__);
> + return status;
> + }
> +
> + return 0;
> +}
> +
> +static int f81534_read_data(struct usb_serial *usbserial, int address,
> + int size, unsigned char *buf)
Use an appropriate size (e.g. u32) for addresses and an unsigned type
(e.g. size_t) for size.
> +{
> + unsigned int count = 0;
> + unsigned int read_size = 0;
> + unsigned int block = 0;
> + unsigned char *tmp_buf;
> + int status;
> + int offset;
> + struct usb_device *dev = usbserial->dev;
> +
> + tmp_buf = kzalloc(F81534_MAX_DATA_BLOCK, GFP_KERNEL);
> + if (!tmp_buf)
> + return -ENOMEM;
> +
> + status = f81534_command_delay(usbserial);
> + if (status) {
> + dev_err(&dev->dev, "%s: failed, %d\n",
> + __func__, __LINE__);
> + goto error;
> + }
> +
> + status = f81534_set_normal_register(dev,
> + F81534_BUS_REG_START, F81534_CMD_READ);
> + if (status) {
> + dev_err(&dev->dev, "%s: failed, %d\n",
> + __func__, __LINE__);
> + goto error;
> + }
If you really need the delay before/after every read then why not do
that in a helper function?
> +
> + status = f81534_command_delay(usbserial);
> + if (status) {
> + dev_err(&dev->dev, "%s: failed, %d\n",
> + __func__, __LINE__);
> + goto error;
> + }
> +static int f81534_write_data(struct usb_serial *usbserial, int address,
> + int size, unsigned char *buf)
> +{
> + unsigned int count = 0;
> + unsigned int write_size = 0;
> + unsigned int block = 0;
> + int offset;
> + int status;
> + struct usb_device *dev = usbserial->dev;
> +
> + /* This function maybe cause IC no workable
> + * Please take this carefully
> + */
Put this above the function, and expand on what it does (e.g. what is it
you're updating etc.).
> +static int f81534_erase_sector(struct usb_serial *usbserial, int address)
> +{
> + u8 current_status = 0;
> + int status;
> + unsigned int count = F81534_MAX_BUS_RETRY;
> + struct usb_device *dev = usbserial->dev;
> +
> + /* This function maybe cause IC no workable
> + * Please take this carefully
> + */
Same here.
> +static int f81534_gpio_get(struct gpio_chip *chip, unsigned gpio_num)
> +{
> + struct usb_serial_port *port = container_of(
Break before container_of or initialise after declarations.
> + chip->dev, struct usb_serial_port, dev);
> + struct f81534_port_private *port_priv =
> + usb_get_serial_port_data(port);
> + struct f81534_serial_private *serial_priv =
> + usb_get_serial_data(port->serial);
> + int current_mode;
> + int status;
> +
> + status = mutex_lock_interruptible(
> + &serial_priv->change_mode_mutex);
> + if (status) {
> + dev_err(&port->dev, "%s: interrupted!\n", __func__);
> + return status;
> + }
> +
> + current_mode =
> + port_priv->port_pin_data.eGPIOMode;
Odd line break.
> + current_mode &= BIT(gpio_num);
> +
> + mutex_unlock(&serial_priv->change_mode_mutex);
> + f81534_wakeup_all_port(port->serial);
> +
> + return !!current_mode;
> +}
Your gpio implementation looks wrong, but let's get back to that after
you explain how these pins are used.
If they are not really general purpose pins, then this isn't the right
interface.
> +static int f81534_calc_baud_divisor(u32 baudrate, u32 clockrate, u32 *remain)
> +{
> + u32 divisor, rem;
> +
> + if (!baudrate)
> + return 0;
> +
> + divisor = clockrate / baudrate;
> + rem = clockrate % baudrate;
> +
> + if (remain)
> + *remain = rem;
> +
> + /* Round to nearest divisor */
> + divisor = DIV_ROUND_CLOSEST(clockrate, baudrate);
> +
> + return divisor;
> +}
> +
> +static int f81534_get_normal_register(struct usb_device *dev,
> + u16 reg, u8 *data)
> +{
> + int count = F81534_USB_MAX_RETRY;
> + int status;
> + u8 *tmp;
> +
> + /* avoid smatch report: error: doing dma on the stack*/
No need for this comment.
> + tmp = kmalloc(sizeof(u8), GFP_KERNEL);
> + if (!tmp)
> + return -ENOMEM;
> +
> + /* Our device maybe not reply when heavily loading,
> + * We'll retry for F81534_USB_MAX_RETRY times
> + */
> +
> + while (count--) {
> + status = usb_control_msg(dev,
> + usb_rcvctrlpipe(dev, 0),
> + F81534_SET_GET_REGISTER,
> + USB_TYPE_VENDOR | USB_DIR_IN,
> + reg,
> + 0, tmp, sizeof(u8),
> + F81534_USB_TIMEOUT);
> + if (status <= 0) {
> + if (status == 0)
> + status = -EIO;
> + } else {
> + break;
> + }
> + }
> +
> + if ((count <= 0) && (status <= 0)) {
> + dev_err(&dev->dev,
> + "%s ERROR reg:%x status:%i failed\n",
> + __func__, reg, status);
> + kfree(tmp);
> + return status;
> + }
> +
> + *data = *tmp;
> + kfree(tmp);
> + return 0;
> +}
> +
> +static int f81534_set_normal_register(struct usb_device *dev,
> + u16 reg, u8 data)
> +{
> + /* Our device maybe not reply when heavily loading,
> + * We'll retry for F81534_USB_MAX_RETRY times
> + */
Move this before the loop.
> +
> + int count = F81534_USB_MAX_RETRY;
> + int status = 0;
> + u8 *tmp;
> +
> + /* avoid smatch report: error: doing dma on the stack*/
Not needed.
> + tmp = kmalloc(sizeof(u8), GFP_KERNEL);
> + if (!tmp)
> + return -ENOMEM;
> +
> + *tmp = data;
> +
> + while (count--) {
> + status = usb_control_msg(dev,
> + usb_sndctrlpipe(dev, 0),
> + F81534_SET_GET_REGISTER,
> + USB_TYPE_VENDOR | USB_DIR_OUT,
> + reg, 0, tmp, sizeof(u8),
> + F81534_USB_TIMEOUT);
> + if (status <= 0) {
> + if (status == 0)
> + status = -EIO;
> + } else {
> + break;
> + }
> + }
> +
> + if ((count <= 0) && status) {
> + dev_err(&dev->dev,
> + "%s ERROR reg:%x data:0x%x status:%i failed\n",
> + __func__, reg, data, status);
> + kfree(tmp);
> + return status;
> + }
> +
> + kfree(tmp);
> + return 0;
> +}
> +
> +static int f81534_setregister(struct usb_device *dev,
> + u8 uart, u16 reg, u8 data)
> +{
> + /* Our device maybe not reply when heavily loading,
> + * We'll retry for F81534_USB_MAX_RETRY times
> + */
> +
> + int count = F81534_USB_MAX_RETRY;
> + int status;
> + u8 *tmp;
> +
> + /* avoid smatch report: error: doing dma on the stack*/
> + tmp = kmalloc(sizeof(u8), GFP_KERNEL);
> + if (!tmp)
> + return -ENOMEM;
> +
> + *tmp = data;
> +
> + while (count--) {
> + status = usb_control_msg(dev,
> + usb_sndctrlpipe(dev, 0),
> + F81534_SET_GET_REGISTER,
> + USB_TYPE_VENDOR | USB_DIR_OUT,
> + reg + uart * 0x10,
> + 0, tmp, sizeof(u8),
> + F81534_USB_TIMEOUT);
Looks like this one should be implemented using
f81534_set_normal_register if the register offset is all that differs.
> +static int f81534_set_port_config(struct usb_device *dev,
> + unsigned char port_number,
> + struct usb_serial_port *port,
> + u32 baudrate, u16 lcr)
> +{
> + struct usb_serial *serial = port->serial;
> + struct f81534_port_private *port_priv = usb_get_serial_port_data(port);
> + u16 device_port = f81534_port_to_phy_index(port);
> + u32 divisor[3] = {0, 0, 0};
> + u32 rem[3] = {0, 0, 0};
> + int status;
> + u8 value;
> + bool is485Mode = false;
> + bool needInvert = false;
Make sure you remove all CamelCase from this driver.
> +static int f81534_getregister(struct usb_device *dev,
> + u8 uart, u16 reg, u8 *data)
> +{
> + int count = F81534_USB_MAX_RETRY;
> + int status;
> + u8 *tmp;
> +
> + /* avoid smatch report: error: doing dma on the stack*/
> + tmp = kmalloc(sizeof(u8), GFP_KERNEL);
> + if (!tmp)
> + return -ENOMEM;
> +
> + /* Our device maybe not reply when heavily loading,
> + * We'll retry for F81534_USB_MAX_RETRY times
> + */
> +
> + while (count--) {
> + status = usb_control_msg(dev,
> + usb_rcvctrlpipe(dev, 0),
> + F81534_SET_GET_REGISTER,
> + USB_TYPE_VENDOR | USB_DIR_IN,
> + reg + uart * 0x10,
> + 0, tmp, sizeof(u8),
> + F81534_USB_TIMEOUT);
Should be implemented using f81534_get_normal_register if the offset is
all that differs.
> + if (status <= 0) {
> + if (status == 0)
> + status = -EIO;
> + } else {
> + break;
> + }
> + }
> +
> + if ((count <= 0) && (status <= 0)) {
> + dev_err(&dev->dev,
> + "%s ERROR port_number:%d reg:%x status:%i failed\n",
> + __func__, uart, reg + uart * 0x10, status);
> + kfree(tmp);
> + return status;
> + }
> +
> + *data = *tmp;
> + kfree(tmp);
> + return 0;
> +}
> +static int f81534_calc_custom_idx(struct usb_serial *serial, u32 *index)
What is this index used for? Please document it.
> +{
> + int idx, status;
> + u8 custom_data;
> + int offset;
> +
> + for (idx = F81534_CUSTOM_MAX_IDX - 1; idx >= 0; --idx) {
> + offset = F81534_CUSTOM_ADDRESS_START +
> + F81534_CUSTOM_DATA_SIZE * idx;
> + status =
> + f81534_read_data(serial, offset, 1,
> + &custom_data);
Odd line breaks again.
> + if (status) {
> + dev_err(&serial->dev->dev,
> + "%s: read error, idx:%d, status:%d\n",
> + __func__, idx, status);
> + return status;
> + }
> +
> + /* if had custom setting, override
> + * 1st byte is a indicater, 0xff is empty, 0xf0 is had data
> + */
> +
> + /* found */
> + if (custom_data == F81534_CUSTOM_VALID_TOKEN)
> + break;
> + }
> +
> + *index = idx;
> + return 0;
> +}
> +
> +static int f81534_calc_num_ports(struct usb_serial *serial)
> +{
> + struct f81534_serial_private *serial_priv = NULL;
> + int index;
> + u8 num_port = 0;
> + int status;
> + unsigned char reserve[F81534_CUSTOM_DATA_SIZE + 1];
> +
> + serial_priv = kzalloc(sizeof(*serial_priv), GFP_KERNEL);
> + if (!serial_priv)
> + return 0;
You must allocate the port private data in attach, which is called after
this function. Then release it a new release callback instead of at
disconnect.
Note that otherwise you may leak this memory if there are errors during
probe.
> +
> + usb_set_serial_data(serial, serial_priv);
> +
> + /* check had custom setting */
> + status = f81534_calc_custom_idx(serial, &serial_priv->custom_idx);
> + if (status) {
> + dev_err(&serial->dev->dev,
> + "%s: f81534_calc_custom_idx read failed!!\n",
> + __func__);
> + return 0;
> + }
You could just determine the index (please document it better) again in
attach, or possibly use usb_set_serial_data to store it in the private
pointer.
> +
> + /* read default board setting */
> + status = f81534_read_data(serial, F81534_RESERVE_ADDRESS_START,
> + F81534_NUM_PORT, reserve);
> + if (status) {
> + dev_err(&serial->dev->dev,
> + "%s: f81534_read_data read failed!!\n", __func__);
> + return 0;
> + }
> +
> + /* if had custom setting, override
> + * 1st byte is a indicater, 0xff is empty, 0xf0f is had data
> + * skip with 1st data
> + */
> +
> + if (serial_priv->custom_idx != F81534_CUSTOM_NO_CUSTOM_DATA) {
> + status = f81534_read_data(serial,
> + F81534_CUSTOM_ADDRESS_START +
> + F81534_CUSTOM_DATA_SIZE *
> + serial_priv->custom_idx + 1,
> + sizeof(reserve), reserve);
> + if (status) {
> + dev_err(&serial->dev->dev,
> + "%s: get custom data failed!!\n", __func__);
> + return 0;
> + }
> +
> + dev_info(&serial->dev->dev,
> + "%s: read configure from block:%d\n", __func__,
> + serial_priv->custom_idx);
> + } else
> + dev_info(&serial->dev->dev, "%s: read configure default\n",
> + __func__);
> +
> + for (index = 0; index < F81534_NUM_PORT; ++index) {
> + switch (reserve[index]) {
> + case 0x37:
> + case 0x38:
> + case 0x39:
> + num_port += 1;
> + break;
> + }
> + }
What's going on here? What are those magic numbers?
> +
> + /* old style */
> + if (num_port) {
> + dev_dbg(&serial->dev->dev, "%s: old style wtih %d ports",
> + __func__, num_port);
> + return num_port;
> + }
> +
> + /*new style, find all possible ports */
> + num_port = 0;
> + for (index = 0; index < F81534_NUM_PORT; ++index) {
> + if (reserve[index] & BIT(7))
> + continue;
> +
> + num_port += 1;
> + }
> +
> + if (num_port)
> + return num_port;
> +
> + dev_err(&serial->dev->dev, "Read Failed!!, default 4 ports\n");
> + return 4; /* nothing found */
> +}
> +
> +static void f81534_set_termios(struct tty_struct *tty,
> + struct usb_serial_port *port,
> + struct ktermios *old_termios)
> +{
> + struct usb_device *dev = port->serial->dev;
> + struct f81534_port_private *port_priv;
> + u32 baud;
> + u16 new_lcr = 0;
> + int status;
> +
> + port_priv = usb_get_serial_port_data(port);
> +
> + if (C_BAUD(tty) == B0)
> + f81534_update_mctrl(port, 0, TIOCM_DTR | TIOCM_RTS);
> + else if (old_termios && (old_termios->c_cflag & CBAUD) == B0)
> + f81534_update_mctrl(port, TIOCM_DTR | TIOCM_RTS, 0);
> +
> + if (C_PARENB(tty)) {
> + new_lcr |= UART_LCR_PARITY;
> +
> + if (!C_PARODD(tty))
> + new_lcr |= UART_LCR_EPAR;
> +
> + if (C_CMSPAR(tty))
> + new_lcr |= UART_LCR_SPAR;
> + }
> +
> + if (C_CSTOPB(tty))
> + new_lcr |= UART_LCR_STOP;
> +
> + switch (C_CSIZE(tty)) {
> + case CS5:
> + new_lcr |= UART_LCR_WLEN5;
> + break;
> + case CS6:
> + new_lcr |= UART_LCR_WLEN6;
> + break;
> + case CS7:
> + new_lcr |= UART_LCR_WLEN7;
> + break;
> + default:
> + case CS8:
> + new_lcr |= UART_LCR_WLEN8;
> + break;
> + }
> +
> + baud = tty_get_baud_rate(tty);
> +
> + if (baud) {
> + if ((baud == 1000000) || (baud > F81534_MAX_BAUDRATE)) {
Why do you treat 1Mbaud differently here?
> + if (old_termios)
> + baud = old_termios->c_ospeed;
> + else
> + baud = F81534_DEFAULT_BAUD_RATE;
> + }
> +
> + dev_dbg(&dev->dev, "%s-baud: %d\n", __func__, baud);
> + tty_encode_baud_rate(tty, baud, baud);
> +
> + port_priv->currentBaudRate = baud;
> + }
> +
> + if (C_CRTSCTS(tty) && baud) {
> + dev_dbg(&dev->dev, "%s: port:%d CRTSCTS\n", __func__,
> + f81534_port_to_phy_index(port));
> + f81534_update_mctrl(port, TIOCM_RTS, 0);
This looks wrong. You never enable hardware flow control AFAICS.
> + }
> +
> + port_priv->shadowLCR = new_lcr;
> + status =
> + f81534_set_port_config(dev, f81534_port_to_phy_index(port), port,
> + port_priv->currentBaudRate, new_lcr);
> + if (status < 0)
> + dev_err(&port->dev, "%s - f81534_set_port_config failed: %i\n",
> + __func__, status);
> +
> +}
> +
> +static int f81534_port_enable(struct usb_serial_port *port)
> +{
> + struct f81534_port_private *port_priv =
> + usb_get_serial_port_data(port);
> +
> + atomic_inc(&port_priv->port_active);
> + return 0;
> +}
> +
> +static int f81534_port_disable(struct usb_serial_port *port)
> +{
> + struct f81534_port_private *port_priv =
> + usb_get_serial_port_data(port);
> +
> + atomic_dec(&port_priv->port_active);
> + return 0;
> +}
Move these above open and close.
> +
> +static int f81534_prepare_gpio(struct usb_serial_port *port)
> +{
> + struct f81534_port_private *port_priv =
> + usb_get_serial_port_data(port);
> + char *name = kmalloc(32, GFP_KERNEL);
Split declaration and initialisation.
> + int idx = port->minor;
> + int rc;
> +
> + if (!name)
> + return -ENOMEM;
> +
> + memcpy(&port_priv->f81534_gpio_chip,
> + &f81534_gpio_chip_templete,
> + sizeof(f81534_gpio_chip_templete));
> +
> + sprintf(name, "%s-%d", IC_NAME, idx);
Use snprintf
> +
> + port_priv->f81534_gpio_chip.label = name;
> + port_priv->f81534_gpio_chip.dev = &port->dev;
> +
> + rc = gpiochip_add(&port_priv->f81534_gpio_chip);
> + if (rc) {
> + kfree(name);
> + dev_err(&port->dev, "%s: f81534_prepare_gpio failed:%d\n",
> + __func__, rc);
> + return rc;
> + }
> +
> + return 0;
> +}
> +
> +static int f81534_release_gpio(struct usb_serial_port *port)
> +{
> + struct f81534_port_private *port_priv =
> + usb_get_serial_port_data(port);
> +
> + gpiochip_remove(&port_priv->f81534_gpio_chip);
> + kfree(port_priv->f81534_gpio_chip.names);
Looks like you leak memory here as you never release label that you
allocated above, but instead you free this NULL-pointer.
> + return 0;
> +}
> +
> +static int f81534_open(struct tty_struct *tty, struct usb_serial_port *port)
> +{
> + int status;
> +
> + status = f81534_port_enable(port);
> + if (status)
> + return status;
> +
> + if (tty)
> + f81534_set_termios(tty, port, &tty->termios);
> +
> + return 0;
> +}
> +
> +static void f81534_close(struct usb_serial_port *port)
> +{
> + int i;
> + unsigned long flags;
> +
> + f81534_port_disable(port);
> +
> + /* referenced from usb_serial_generic_close() */
> + for (i = 0; i < ARRAY_SIZE(port->write_urbs); ++i)
> + usb_kill_urb(port->write_urbs[i]);
> +
> + spin_lock_irqsave(&port->lock, flags);
> + kfifo_reset_out(&port->write_fifo);
> + spin_unlock_irqrestore(&port->lock, flags);
> +}
> +
> +static void f81534_disconnect(struct usb_serial *serial)
> +{
> + int i;
> + struct usb_serial_port *port0 = serial->port[0];
> + struct f81534_serial_private *serial_priv =
> + usb_get_serial_data(serial);
> +
> + dev_dbg(&serial->dev->dev, "%s\n", __func__);
Please remove all debug statements like this one that only track what
functions is being called.
> + kfree(serial_priv);
This should be freed in a .release callback.
> +
> + for (i = 0; i < ARRAY_SIZE(port0->read_urbs); ++i)
> + usb_kill_urb(port0->read_urbs[i]);
> +}
> +
> +static int f81534_get_serial_info(struct usb_serial_port *port,
> + struct serial_struct __user *retinfo)
> +{
> + struct serial_struct tmp;
> +
> + if (!retinfo)
> + return -EFAULT;
> +
> + memset(&tmp, 0, sizeof(tmp));
> +
> + tmp.type = PORT_16550A;
> + tmp.port = port->port_number;
> + tmp.line = port->minor;
> + tmp.baud_base = 115200;
115200?
> +
> + if (copy_to_user(retinfo, &tmp, sizeof(*retinfo)))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> +#define READ_AND_SET_NORMAL(dev, register, mask, value) \
> + ({ \
> + int status_tmp = 0 ; \
> + do { \
> + char *err_str = "%s - error: %x, status: %d\n"; \
> + u8 urb_value_tmp = 0; \
> + status_tmp = f81534_get_normal_register(dev, \
> + register, &urb_value_tmp); \
> + if (status_tmp < 0) { \
> + dev_err(&dev->dev, \
> + err_str, \
> + __func__, \
> + register, \
> + status_tmp); \
> + break; \
> + }; \
> + \
> + if ((value) != 0) \
> + urb_value_tmp |= \
> + ((1L << mask) & (value)) ; \
> + else \
> + urb_value_tmp &= \
> + ~(1L << mask); \
> + \
> + status_tmp = f81534_set_normal_register(dev, \
> + register, \
> + urb_value_tmp); \
> + \
> + if (status_tmp < 0) { \
> + dev_err(&dev->dev, \
> + err_str, \
> + __func__, \
> + register, status_tmp); \
> + break; \
> + }; \
> + } while (0); \
> + status_tmp; \
> + })
This should be a proper function.
> +
> +static int f81534_switch_gpio_mode(struct usb_serial_port
> + *serial_port, int mode)
Odd line break.
Why not pass an enum for mode?
> +{
> + int x = f81534_port_to_phy_index(serial_port);
> + int y = 0;
> + int val;
> + int status;
> + struct usb_device *dev = serial_port->serial->dev;
> + struct io_map_value *request_mode =
> + f81534_mode_control[(mode >= eModeInvalid) ? eModeRS232 : mode];
> +
> + struct pin_data *m1 = &request_mode->port[x].m1;
> + struct pin_data *m2 = &request_mode->port[x].m2;
> + struct pin_data *sd = &request_mode->port[x].sd;
> +
> + struct pin_data *pins[3] = { m1, m2, sd };
Why not use an array for the three pins as you never seem to access them
directly (e.g. ->m2)?
Also why are sometimes referring to "m0" and sometimes "sd"?
> +
> + if (mode >= 8)
> + return -EINVAL;
> +
> + for (y = 0; y < 3; ++y) {
> + val = pins[y]->port_io.reg_bit ? 0xff : 0x00;
> + status = READ_AND_SET_NORMAL(dev,
> + pins[y]->port_io.reg_address,
> + pins[y]->port_io.reg_offset,
> + val);
> + if (status) {
> + dev_err(&serial_port->dev,
> + "%s: failed, %d\n",
> + __func__, __LINE__);
> + return status;
> + }
> + }
> +
> + return 0;
> +}
> +
> +
> +
Stray newlines.
> +static int f81534_set_port_mode(struct usb_serial_port *port,
> + enum eUartMode eMode)
> +{
> + int status;
> + u8 urb_value;
urb_value?
> + struct f81534_port_private *port_priv = usb_get_serial_port_data(port);
> +
> + if (eMode > eModeInvalid)
> + return -1;
> +
> + if (eMode != eModeInvalid) {
> + int data = 1;
> +
> + status = f81534_getregister(port->serial->dev,
> + f81534_port_to_phy_index(port),
> + CLK_SEL_REGISTER,
> + &urb_value);
> + if (status) {
> + dev_err(&port->dev,
> + "%s: failed, %d\n",
> + __func__, __LINE__);
> + return status;
> + }
> +
> + urb_value &= ~(data << 4);
> + urb_value &= ~(data << 5);
What is this? Why not clear it in one mask operation?
> +static void f81534_process_per_serial_block(struct usb_serial_port *port,
> + unsigned char *data)
> +{
> + unsigned char lsr = 0;
> + unsigned char msr, old_msr;
> + char tty_flag;
> + struct tty_struct *tty;
> + struct usb_serial *serial = port->serial;
> + struct f81534_serial_private *serial_priv =
> + usb_get_serial_data(serial);
> + struct f81534_port_private *port_priv =
> + usb_get_serial_port_data(port);
> + int phy_port_num = data[0];
> + int read_size = 0;
> + int i;
> +
> + /* The block layout is 128 Bytes
> + * index 0: port phy idx (e.g., 0,1,2,3),
> + * index 1: It's could be
> + * F81534_TOKEN_RECEIVE
> + * F81534_TOKEN_TX_EMPTY
> + * F81534_TOKEN_MSR_CHANGE
> + * index 2: serial in size (data+lsr, must be even)
> + * meanful for F81534_TOKEN_RECEIVE only
> + * index 3: current MSR with device read
> + * index 4~127: serial in data block (data+lsr, must be even)
> + */
> +
> + switch (data[1]) {
> + case F81534_TOKEN_TX_EMPTY:
> + spin_lock(&serial_priv->write_urb_lock);
> + serial_priv->phy_port_in_use[phy_port_num] =
> + false;
> + spin_unlock(&serial_priv->write_urb_lock);
> + usb_serial_port_softint(port);
> + break;
> +
> + case F81534_TOKEN_MSR_CHANGE:
> + break;
> +
> + case F81534_TOKEN_RECEIVE:
> + read_size = data[2];
> + break;
> +
> + default:
> + dev_info(&port->dev, "%s: unknown token:%02x\n",
> + __func__, data[1]);
dev_warn
> + return;
> + }
> +
> + do {
> + msr = data[3];
> + spin_lock(&serial_priv->msr_lock);
> +
> + /* The MSR information can be get with any
> + * 'correct' block data, but dont directly read MSR
> + * from 16550A, our MCU will reported newest
> + * or current data.
> + *
> + * We maybe received the same MSR. e.g.,
> + * received MSR 0x11 and received 0x11 again.
> + *
> + * So we'll not use UART_MSR_ANY_DELTA to do check,
> + * we directly compare their value.
> + */
> + if (msr ==
> + serial_priv->shadowMSR[phy_port_num]) {
> + spin_unlock(&serial_priv->msr_lock);
> + break;
> + }
> +
> + old_msr = serial_priv->shadowMSR[phy_port_num];
> + serial_priv->shadowMSR[phy_port_num] = msr;
> + serial_priv->msr_time[phy_port_num] = jiffies;
> +
> + spin_unlock(&serial_priv->msr_lock);
> +
> + dev_dbg(&port->dev,
> + "%s: DCD Changed: port %d from %x to %x.\n",
> + __func__, phy_port_num, old_msr,
> + msr);
> +
> + f81534_update_msr(port, &msr);
> +
> + tty = tty_port_tty_get(&port->port);
> + if (!tty)
> + break;
> +
> + usb_serial_handle_dcd_change(port,
> + tty, msr & UART_MSR_DCD);
> + tty_kref_put(tty);
> + } while (0);
Move this to a helper function instead.
> +
> + if (data[1] != F81534_TOKEN_RECEIVE)
> + return;
> +
> + /* if the open not had open, skip this data */
> + if (atomic_read(&port_priv->port_active))
> + return;
You should not process MSR changes for a closed port above either.
> +
> + for (i = 0; i < read_size; i += 2) {
Start counting at 4 instead.
> + tty_flag = TTY_NORMAL;
> + lsr = data[i + 5];
> +
> + if (lsr & UART_LSR_BRK_ERROR_BITS) {
> + if (lsr & UART_LSR_BI) {
> + tty_flag = TTY_BREAK;
> + port->icount.brk++;
> + usb_serial_handle_break(port);
> + } else if (lsr & UART_LSR_PE) {
> + tty_flag = TTY_PARITY;
> + port->icount.parity++;
> + } else if (lsr & UART_LSR_FE) {
> + tty_flag = TTY_FRAME;
> + port->icount.frame++;
> + }
> +
> + if (lsr & UART_LSR_OE) {
> + port->icount.overrun++;
> + tty_insert_flip_char(&port->port, 0,
> + TTY_OVERRUN);
> + }
> + }
> +
> + if (port->port.console && port->sysrq) {
> + if (usb_serial_handle_sysrq_char(port, data[i + 4]))
> + continue;
> + }
> +
> + tty_insert_flip_char(&port->port, data[i + 4], tty_flag);
> + }
> +
> + tty_flip_buffer_push(&port->port);
> +}
> +
> +static void f81534_process_read_urb(struct urb *urb)
> +{
> + int i;
> + int phy_port_num;
> + int tty_port_num;
> + unsigned char *ch;
> + struct usb_serial *serial;
> + struct usb_serial_port *port = NULL;
> +
> + if (!urb->actual_length)
> + return;
> +
> + port = urb->context;
> + serial = port->serial;
> + ch = urb->transfer_buffer;
> +
> + for (i = 0; i < urb->actual_length / F81534_RECEIVE_BLOCK_SIZE;
> + ++i) {
Increment using F81534_RECEIVE_BLOCK_SIZE instead and remove the
multiplications below.
> + phy_port_num = ch[i * F81534_RECEIVE_BLOCK_SIZE];
> + tty_port_num =
> + f81534_phy_to_logic_port(serial, phy_port_num);
> + port = serial->port[tty_port_num];
> + f81534_process_per_serial_block(port,
> + &ch[i * F81534_RECEIVE_BLOCK_SIZE]);
> + }
> +}
> +
> +static int f81534_setup_urbs(struct usb_serial *serial)
> +{
> + int i = 0;
> + u8 port0_out_address;
> + int j;
> + int buffer_size;
> + struct usb_serial_port *port = NULL;
> +
> + /* In our system architecture, we had 4 or 2 serial ports,
> + * but only get 1 set of bulk in/out endpoints.
> + *
> + * The usb-serial subsystem will generate port 0 data,
> + * but port 1/2/3 will not. It's will generate write urb and buffer
> + * by following code
> + */
> + for (i = 1; i < serial->num_ports; ++i) {
> + port0_out_address =
> + serial->port[0]->bulk_out_endpointAddress;
> + buffer_size = serial->port[0]->bulk_out_size;
> + port = serial->port[i];
> +
> + if (kfifo_alloc(&port->write_fifo, PAGE_SIZE, GFP_KERNEL))
> + goto failed;
> +
> + port->bulk_out_size = buffer_size;
> + port->bulk_out_endpointAddress = port0_out_address;
> +
> + for (j = 0; j < ARRAY_SIZE(port->write_urbs); ++j) {
> + set_bit(j, &port->write_urbs_free);
> + port->write_urbs[j] = usb_alloc_urb(0, GFP_KERNEL);
> + if (!port->write_urbs[j])
> + goto failed;
> + port->bulk_out_buffers[j] = kmalloc(buffer_size,
> + GFP_KERNEL);
> + if (!port->bulk_out_buffers[j])
> + goto failed;
> +
> + usb_fill_bulk_urb(port->write_urbs[j],
> + serial->dev,
> + usb_sndbulkpipe(serial->dev,
> + port0_out_address),
> + port->bulk_out_buffers[j],
> + buffer_size,
> + serial->type->write_bulk_callback,
> + port);
> + }
> +
> + port->write_urb = port->write_urbs[0];
> + port->bulk_out_buffer = port->bulk_out_buffers[0];
> + }
> + return 0;
> +
> +failed:
> + return -ENOMEM;
> +}
I understand better now how your firmware work. It looks like this is a
bad fit for the generic implementation due to the TX_EMPTY requirement.
You need to implement your own write handling, and there's no need to
have more than one write urb per port. You still want the fifo though to
cache outgoing data. If you implement write combining you only need urb
per device, but I'm not sure if it's worth it.
> +static int f81534_save_configure_data(struct usb_serial_port *port)
What does thing function do? And why do you need to call it on every
port probe? Please add some comments.
> + /* token of data exist */
> + data[0] = F81534_CUSTOM_VALID_TOKEN;
> + memcpy(&data[1], serial_priv->reserve_data,
> + F81534_RESERVE_SIZE);
This line does not need to be broken. There are a lot of such
unnecessarily broken lines that only hurt readability. Please go through
the file and fix that up.
> +static int f81534_load_configure_data(struct usb_serial_port *port)
> +{
> + int status;
> + int offset;
> + unsigned char uart_flag, gpio_mode;
> + int device_port = f81534_port_to_phy_index(port);
> + struct f81534_port_private *port_priv = usb_get_serial_port_data(port);
> + struct f81534_serial_private *serial_priv =
> + usb_get_serial_data(port->serial);
> +
> + uart_flag = serial_priv->reserve_data[device_port];
> + gpio_mode = serial_priv->reserve_data[device_port + F81534_CONF_SIZE];
> +
> + switch (uart_flag) {
> + /* old style setting */
> + case 0x37:
> + case 0x38:
> + case 0x39:
No magic constants please.
> +static int f81534_attach(struct usb_serial *serial)
> +{
> + struct f81534_serial_private *serial_priv =
> + usb_get_serial_data(serial);
> + int status;
> + int i;
> + int offset;
> + int num_port = serial->num_ports;
> +
> + serial_priv->serial = serial;
> + memset(serial_priv->port_mapping, F81534_UNUSED_PORT,
> + sizeof(serial_priv->port_mapping));
> +
> + switch (num_port) {
> + case eIC_F81532:
> + case eIC_F81534:
> + serial_priv->ic_type = num_port;
> + dev_info(&serial->dev->dev, "%s detected\n",
> + m_ic_name[num_port]);
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + for (i = 0; i < F81534_NUM_PORT; ++i)
> + serial_priv->port_mapping[i] = i;
> +
> + switch (num_port) {
> + case 4:
> + break;
> + case 2:
> + serial_priv->port_mapping[1] = 3;
> + serial_priv->port_mapping[2] = F81534_UNUSED_PORT;
> + serial_priv->port_mapping[3] = F81534_UNUSED_PORT;
> + break;
> + case 1:
> + serial_priv->port_mapping[1] = F81534_UNUSED_PORT;
> + serial_priv->port_mapping[2] = F81534_UNUSED_PORT;
> + serial_priv->port_mapping[3] = F81534_UNUSED_PORT;
> + break;
> + default:
> + dev_err(&serial->dev->dev,
> + "%s: Cant determine ports: %d, error!!!\n", __func__,
> + num_port);
> + status = -EINVAL;
> + goto failed;
> + }
You should not need this port_mapping, but rather have calc_num_ports
return the actual number of ports available so that resource are not
unnecessarily allocated. You can then store the mapping in the port
private data instead.
> + for (i = 0; i < F81534_NUM_PORT; ++i) {
> + /* clear fifo when plug in */
> + status = f81534_setregister(serial->dev,
> + i, FIFO_CONTROL_REGISTER,
> + UART_FCR_R_TRIG_11 |
> + UART_FCR_ENABLE_FIFO |
> + UART_FCR_CLEAR_RCVR |
> + UART_FCR_CLEAR_XMIT);
> + if (status) {
> + dev_err(&serial->dev->dev,
> + "%s FCR init error:%x failed\n", __func__, i);
> + goto failed;
> + }
> +
> + /* Get MSR initial value*/
> + status = f81534_get_normal_register(serial->dev,
> + MSR_ACCESS_SHADOW_REG + i,
> + &serial_priv->shadowMSR[i]);
> + if (status) {
> + dev_err(&serial->dev->dev,
> + "%s f81534_get_normal_register:%x failed\n",
> + __func__, MSR_ACCESS_SHADOW_REG + i);
> + goto failed;
> + }
> +
> + serial_priv->msr_time[i] = jiffies;
> + }
This should be done per port in open instead.
> + spin_lock_init(&serial_priv->write_urb_lock);
> + spin_lock_init(&serial_priv->msr_lock);
> + mutex_init(&serial_priv->msr_mutex);
> + mutex_init(&serial_priv->change_mode_mutex);
> +
> + status = f81534_setup_urbs(serial);
> + if (status != 0)
> + goto failed;
> +
> + status = f81534_read_data(serial,
> + F81534_RESERVE_ADDRESS_START,
> + F81534_RESERVE_SIZE,
> + serial_priv->reserve_data);
> + if (status) {
> + dev_err(&serial->dev->dev, "%s read reserve data failed\n",
> + __func__);
> + goto failed;
> + }
Please document this "reserve" data.
> +
> + /* if had custom setting, override
> + * 1st byte is a indicater, 0xff is empty, 0xf0 is had data
> + * skip with 1st data
> + */
> +
> + if (serial_priv->custom_idx == F81534_CUSTOM_NO_CUSTOM_DATA)
> + return 0;
What does this mean?
> +
> + offset = F81534_CUSTOM_ADDRESS_START +
> + F81534_CUSTOM_DATA_SIZE * serial_priv->custom_idx + 1;
> + /* only read 8 bytes for mode & GPIO */
> + status = f81534_read_data(serial,
> + offset,
> + sizeof(serial_priv->reserve_data),
> + serial_priv->reserve_data);
> + if (status) {
> + dev_err(&serial->dev->dev,
> + "%s: get custom data failed, idx:%d, status:%d!!\n",
> + __func__, serial_priv->custom_idx, status);
> + goto failed;
> + }
> +
> + /* We'll register port 0 bulkin only once, It'll take all port read
> + * and information.
> + */
> + status = usb_serial_generic_submit_read_urbs(serial->port[0],
> + GFP_KERNEL);
> + if (status) {
> + dev_err(&serial->dev->dev,
> + "%s: submit read urbs failed!! status:%d!!\n",
> + __func__, status);
> + goto failed;
> + }
> +
> + return 0;
> +
> +failed:
> + kfree(serial_priv);
> + return status;
> +}
> +
> +static int f81534_tiocmget(struct tty_struct *tty)
> +{
> + struct usb_serial_port *port = tty->driver_data;
> + struct f81534_port_private *port_priv = usb_get_serial_port_data(port);
> + struct f81534_serial_private *serial_priv =
> + usb_get_serial_data(port->serial);
> + int r = 0;
> + int index = f81534_port_to_phy_index(port);
> + int count = F81534_TIOCGET_RETRY, result;
> + unsigned long current_jiffies = jiffies + 1;
> + unsigned long flags = 0;
> + u8 msr, mcr;
> +
> + /* We'll not directly to read MSR/Shadow Register.
Why not?
> + * If MSR has any change, the device will notice by bulk-in
> + * Please reference f81534_process_read_urb()
> + *
> + * This section will wait for 10*F81534_TIOCGET_RETRY msec to
> + * receive MSR change. We'll use old MSR value if H/W not change
> + * or not noticed.
> + */
This seems like a bad idea. Why not simply return the cached value if
you can not get the current state? Is it only sent when there's incoming
data available?
> +static int f81534_tiocmset(struct tty_struct *tty,
> + unsigned int set, unsigned int clear)
> +{
> + struct usb_serial_port *port = tty->driver_data;
> +
> + return f81534_update_mctrl(port, set, clear);
> +}
> +
> +static void f81534_dtr_rts(struct usb_serial_port *port, int on)
> +{
> + /* drop RTS and DTR */
> + if (on)
> + f81534_update_mctrl(port, TIOCM_DTR | TIOCM_RTS, 0);
> + else
> + f81534_update_mctrl(port, 0, TIOCM_DTR | TIOCM_RTS);
> +}
> +
> +static void f81534_update_msr(struct usb_serial_port *port, unsigned char *ch)
> +{
> + u8 newMSR = (u8) *ch;
Cast not needed.
> +
> + if (newMSR & UART_MSR_ANY_DELTA) {
> + /* update input line counters */
> + if (newMSR & UART_MSR_DCTS)
> + port->icount.cts++;
> + if (newMSR & UART_MSR_DDSR)
> + port->icount.dsr++;
> + if (newMSR & UART_MSR_DDCD)
> + port->icount.dcd++;
> + if (newMSR & UART_MSR_TERI)
> + port->icount.rng++;
> +
> + wake_up_interruptible(&port->port.delta_msr_wait);
> + }
> +}
> +
> +static int f81534_write_room(struct tty_struct *tty)
> +{
> + struct usb_serial_port *port = tty->driver_data;
> + struct f81534_port_private *port_priv;
> + struct f81534_serial_private *serial_priv =
> + usb_get_serial_data(port->serial);
> + int port_num = f81534_port_to_phy_index(port);
> + unsigned long flags = 0;
> + int r;
> +
> + port_priv = usb_get_serial_port_data(port);
> +
> + spin_lock_irqsave(&serial_priv->write_urb_lock, flags);
> +
> + if (serial_priv->phy_port_in_use[port_num])
> + r = 0;
> + else
> + r = m_f81534_max_tx_size;
> +
> + spin_unlock_irqrestore(&serial_priv->write_urb_lock, flags);
> +
> + return r;
> +}
You should always accept data if there's room in the per port fifo, but
then you let TX_EMPTY pace how fast the fifo is emptied in your custom
write implementation.
> +
> +static int f81534_prepare_write_buffer(struct usb_serial_port *port,
> + void *dest, size_t size)
> +{
> + unsigned char *ptr = (unsigned char *) dest;
> + int port_num = f81534_port_to_phy_index(port);
> + struct usb_serial *serial = port->serial;
> +
> + WARN_ON(size != serial->port[0]->bulk_out_size);
> +
> + if (size != F81534_WRITE_BUFFER_SIZE) {
> + WARN_ON(size != F81534_WRITE_BUFFER_SIZE);
> + return 0;
> + }
> +
> + /* The block layout is fixed with 4x128 Bytes, per 128 Bytes
> + * for a port.
> + * index 0: port phy idx (e.g., 0,1,2,3)
> + * index 1: only F81534_TOKEN_WRITE
> + * index 2: serial out size
> + * index 3: fix to 0
> + * index 4~127: serial out data block
> + */
> +
> + ptr[0] = 0;
> + ptr[128] = 1;
> + ptr[256] = 2;
> + ptr[384] = 3;
> + ptr[128 * port_num + 0] = port_num;
> + ptr[128 * port_num + 1] = F81534_TOKEN_WRITE;
> + ptr[128 * port_num + 3] = 0;
> + ptr[128 * port_num + 2] =
> + kfifo_out_locked(&port->write_fifo,
> + &ptr[128 * port_num + 4],
> + m_f81534_max_tx_size,
> + &port->lock);
> +
> + return F81534_WRITE_BUFFER_SIZE;
> +}
> +
> +static int f81534_write(struct tty_struct *tty,
> + struct usb_serial_port *port,
> + const unsigned char *buf, int count)
> +{
> + int bytes_out = 0;
> + unsigned long flags;
> + bool bCouldWrite = true;
> + int port_num = f81534_port_to_phy_index(port);
> + int updating_data;
> + struct f81534_serial_private *serial_priv =
> + usb_get_serial_data(port->serial);
> +
> + if (!count)
> + return 0;
> +
> + updating_data = mutex_is_locked(&serial_priv->change_mode_mutex);
> + if (updating_data) /*someone is changing setting */
> + return 0;
> +
> + /* We suppress tx data max up to m_f81534_max_tx_size(124)
> + * to fit generic subsystem. Our device's max tx data per port is
> + * 124 and We should wait for the H/W reply "TX_EMPTY" to
> + * submit next URB for this port.
> + *
> + * If we submit data without wait for H/W report "TX_EMPTY",
> + * the H/W DMA engine will halt.
> + */
So as I mentioned above, always accept data if there's room in the fifo.
Then kick of a write urb, if TX_EMPTY is set for any of the ports and
fill the urb based on those flags as well.
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists