Re: [PATCH V11 1/1] usb:serial: Add Fintek F81532/534 driver

2016-11-09 Thread Ji-Ze Hong (Peter Hong) 

Hi Johan,

Johan Hovold 於 2016/11/2 下午 08:37 寫道:

On Fri, Oct 14, 2016 at 04:20:46PM +0800, Ji-Ze Hong (Peter Hong) wrote:


Reviewed-by: Johan Hovold 


You must never add other peoples' Reviewed-by tags unless you've
explicitly been given permission to do so (e.g. "fix this minor thing up
and then you can add...").

Please make sure to read the section about Reviewed-by tags in
Documentation/SubmittingPatches.


Sorry for misusing "Reviewed-by" tags.


Changelog:
V11
1. Reduce F81534_MAX_BUS_RETRY from 2000 to 20. We are only using
   internal SPI bus to read flash when attach() & calc_num_ports()
   and never read flash when the F81532/534 in full loading, so we
   can reduce retry count.


Does this mean you can remove that retry mechanism completely?


The mechanism is for checking SPI Bus busy status. We need to perform
read/write operation only when the bus idle. So we need remain the check
mechanism.




2. Modify attach() & calc_num_ports() read default value only when
   can't find the custom setting.
3. Change tx_empty protect method from spinlock to set_bit()/
   clear_bit()/test_and_clear_bit().
4. Move calculate tty_idx[i] from port_probe() to attach().
5. Add f81534_tx_empty()



+#define DRIVER_DESC"Fintek F81532/F81534"
+#define FINTEK_VENDOR_ID_1 0x1934
+#define FINTEK_VENDOR_ID_2 0x2C42
+#define FINTEK_DEVICE_ID   0x1202
+#define F81534_MAX_TX_SIZE 100


You never replies to my question about why this is not 124 as for rx.


The limit for TX with 100 bytes is tuned for high-speed TX performance.
But this patch is not contained for high-baudrate, so we'll change it
for 124 byte with next patch.






+static int f81534_set_normal_register(struct usb_serial *serial, u16 reg,


What do mean by "normal" here? Could you give this a more descriptive
name?

Perhaps just call this one f81534_set_register and add a "port" or
"uart" infix to the current f81534_set_register below (e.g. rename it
f81534_set_uart_register, and similar for get).
Or simply replace "normal" with "generic" above.


OK, I'll rename such functions with following:

Generic read/write USB:
  f81534_set/get_normal_register -> f81534_set/get_register

UART port read/write:
  f81534_set/get_register -> f81534_set/get_port_register

SPI bus read/write
  f81534_set_normal_register_with_delay -> f81534_set/get_spi_register




+{
+   int status;
+
+   status = f81534_get_normal_register(serial, reg, data);
+   if (status)
+   return status;
+
+   status = f81534_command_delay(serial);
+   if (status)
+   return status;


Why do you need a delay after reading?


Sorry for misleading.
I'll rename f81534_command_delay to f81534_wait_for_spi_idle.



+static void f81534_prepare_write_buffer(struct usb_serial_port *port, u8 *buf,
+   size_t size)


You never use size in this function. You need to make sure you never
overwrite the provided buffer using some sanity checks.


OK, I'll add probe() to check bulk_in/out endpoints count & packet size.




+   /* Check H/W is TXEMPTY */
+   if (!test_and_clear_bit(F81534_TX_EMPTY_BIT, _priv->tx_empty))
+   return 0;
+
+   urb = port->write_urbs[0];
+   f81534_prepare_write_buffer(port, port->bulk_out_buffers[0],
+   port->bulk_out_size);
+   urb->transfer_buffer_length = F81534_WRITE_BUFFER_SIZE;


You need to make sure the buffers have the expected size. They are
currently set to the endpoint size, but you can you can make sure they
are never smaller than F81534_WRITE_BUFFER_SIZE by setting bulk_out_size
in the usb_serial_driver struct.


Thanks for your notice. We had tested it on USB 2.0 Full-speed, the
packet size will reduce to 64, it will cause buffer overflow. So I'll
reconfirm the packet size in probe().


Thanks for your comments.
--
With Best Regards,
Peter Hong
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V11 1/1] usb:serial: Add Fintek F81532/534 driver

2016-11-02 Thread Johan Hovold 
On Fri, Oct 14, 2016 at 04:20:46PM +0800, Ji-Ze Hong (Peter Hong) wrote:
> This driver is for Fintek F81532/F81534 USB to Serial Ports IC.
> 
> F81532 spec:
> https://drive.google.com/file/d/0B8vRwwYO7aMFOTRRMmhWQVNvajQ/view?usp=
> sharing
> 
> F81534 spec:
> https://drive.google.com/file/d/0B8vRwwYO7aMFV29pQWJqbVBNc00/view?usp=
> sharing
> 
> Features:
> 1. F81532 is 1-to-2 & F81534 is 1-to-4 serial ports IC
> 2. Support Baudrate from B50 to B115200.
> 
> Reviewed-by: Johan Hovold 

You must never add other peoples' Reviewed-by tags unless you've
explicitly been given permission to do so (e.g. "fix this minor thing up
and then you can add...").

Please make sure to read the section about Reviewed-by tags in
Documentation/SubmittingPatches.

> Signed-off-by: Ji-Ze Hong (Peter Hong) 
> ---
> Changelog:
> V11
> 1. Reduce F81534_MAX_BUS_RETRY from 2000 to 20. We are only using
>internal SPI bus to read flash when attach() & calc_num_ports()
>and never read flash when the F81532/534 in full loading, so we
>can reduce retry count.

Does this mean you can remove that retry mechanism completely?

> 2. Modify attach() & calc_num_ports() read default value only when
>can't find the custom setting.
> 3. Change tx_empty protect method from spinlock to set_bit()/
>clear_bit()/test_and_clear_bit().
> 4. Move calculate tty_idx[i] from port_probe() to attach().
> 5. Add f81534_tx_empty()

> +#define DRIVER_DESC  "Fintek F81532/F81534"
> +#define FINTEK_VENDOR_ID_1   0x1934
> +#define FINTEK_VENDOR_ID_2   0x2C42
> +#define FINTEK_DEVICE_ID 0x1202
> +#define F81534_MAX_TX_SIZE   100

You never replies to my question about why this is not 124 as for rx.

> +#define F81534_MAX_RX_SIZE   124
> +#define F81534_RECEIVE_BLOCK_SIZE128
> +
> +#define F81534_TOKEN_RECEIVE 0x01
> +#define F81534_TOKEN_WRITE   0x02
> +#define F81534_TOKEN_TX_EMPTY0x03
> +#define F81534_TOKEN_MSR_CHANGE  0x04
> +
> +/*
> + * We used interal SPI bus to access FLASH section. We must wait the SPI bus 
> to
> + * idle if we performed any command.
> + *
> + * SPI Bus status register: F81534_BUS_REG_STATUS
> + *   Bit 0/1 : BUSY
> + *   Bit 2   : IDLE
> + */
> +#define F81534_BUS_BUSY  (BIT(0) | BIT(1))
> +#define F81534_BUS_IDLE  BIT(2)
> +#define F81534_BUS_READ_DATA 0x1004
> +#define F81534_BUS_REG_STATUS0x1003
> +#define F81534_BUS_REG_START 0x1002
> +#define F81534_BUS_REG_END   0x1001
> +
> +#define F81534_CMD_READ  0x03
> +
> +#define F81534_DEFAULT_BAUD_RATE 9600
> +#define F81534_MAX_BAUDRATE  115200
> +
> +#define F81534_PORT_CONF_DISABLE_PORTBIT(3)
> +#define F81534_PORT_CONF_NOT_EXIST_PORT  BIT(7)
> +#define F81534_PORT_UNAVAILABLE  \
> + (F81534_PORT_CONF_DISABLE_PORT | F81534_PORT_CONF_NOT_EXIST_PORT)
> +
> +#define F81534_1X_RXTRIGGER  0xc3
> +#define F81534_8X_RXTRIGGER  0xcf
> +
> +static const struct usb_device_id f81534_id_table[] = {
> + {USB_DEVICE(FINTEK_VENDOR_ID_1, FINTEK_DEVICE_ID)},
> + {USB_DEVICE(FINTEK_VENDOR_ID_2, FINTEK_DEVICE_ID)},

Add a space after { and before }.

> + {}  /* Terminating entry */
> +};
> +
> +#define F81534_TX_EMPTY_BIT  0
> +
> +struct f81534_serial_private {
> + u8 conf_data[F81534_DEF_CONF_SIZE];
> + int tty_idx[F81534_NUM_PORT];
> + u8 setting_idx;
> + int opened_port;
> + struct mutex urb_mutex;
> +};
> +
> +struct f81534_port_private {
> + struct mutex mcr_mutex;
> + unsigned long tx_empty;
> + spinlock_t msr_lock;
> + u8 shadow_mcr;
> + u8 shadow_msr;
> + u8 phy_num;
> +};


> +static int f81534_set_normal_register(struct usb_serial *serial, u16 reg,

What do mean by "normal" here? Could you give this a more descriptive
name?

Perhaps just call this one f81534_set_register and add a "port" or
"uart" infix to the current f81534_set_register below (e.g. rename it
f81534_set_uart_register, and similar for get).

Or simply replace "normal" with "generic" above.

> + u8 data)
> +{
> + struct usb_interface *interface = serial->interface;
> + struct usb_device *dev = serial->dev;
> + size_t count = F81534_USB_MAX_RETRY;
> + int status;
> + u8 *tmp;
> +
> + tmp = kmalloc(sizeof(u8), GFP_KERNEL);
> + if (!tmp)
> + return -ENOMEM;
> +
> + *tmp = data;
> +
> + /*
> +  * 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_sndctrlpipe(dev, 0),
> +  F81534_SET_GET_REGISTER,
> +

[PATCH V11 1/1] usb:serial: Add Fintek F81532/534 driver

2016-10-14 Thread Ji-Ze Hong (Peter Hong) 
This driver is for Fintek F81532/F81534 USB to Serial Ports IC.

F81532 spec:
https://drive.google.com/file/d/0B8vRwwYO7aMFOTRRMmhWQVNvajQ/view?usp=
sharing

F81534 spec:
https://drive.google.com/file/d/0B8vRwwYO7aMFV29pQWJqbVBNc00/view?usp=
sharing

Features:
1. F81532 is 1-to-2 & F81534 is 1-to-4 serial ports IC
2. Support Baudrate from B50 to B115200.

Reviewed-by: Johan Hovold 
Signed-off-by: Ji-Ze Hong (Peter Hong) 
---
Changelog:
V11
1. Reduce F81534_MAX_BUS_RETRY from 2000 to 20. We are only using
   internal SPI bus to read flash when attach() & calc_num_ports()
   and never read flash when the F81532/534 in full loading, so we
   can reduce retry count.
2. Modify attach() & calc_num_ports() read default value only when
   can't find the custom setting.
3. Change tx_empty protect method from spinlock to set_bit()/
   clear_bit()/test_and_clear_bit().
4. Move calculate tty_idx[i] from port_probe() to attach().
5. Add f81534_tx_empty()

V10
1. Change the submit/kill timming for read URBs, submit when first
   serial port open and kill when final port close.
2. Remove all source code about controlling GPIOs.
3. Change the f81534_tiocmget() from reading shadow MSR with delay
   20ms to read MSR register directly.
4. Using tty_port_initialized() to check port opened/closed.
5. Add sanity check for variables.

v9
1. Remove lots of code to make more generic for F81532/534. e.g.,
   high baud rate support, RS485/422 mode switch, most of GPIO
   control and internal storage write functional.
2. Change f81534_tiocmget() MSR delay from schedule_timeout_killable()
   to wait_for_completion_killable_timeout(). This IC will delayed
   receiving MSR change when doing loop-back test e.g., BurnInTest.
   We'll reset completion "msr_done" in f81534_update_mctrl(). If we
   changed MCR, the next f81534_tiocmget() will delay for 20ms or
   continue with new MSR arrived.
3. Fix for non-zero buffer allocated in f81534_setup_ports(). It'll
   make device malfunctional with incorrect tx length for other ports.

v8
1. Remove driver mode GPIOLIB & RS485 control support, the driver will
   only load GPIO/UART Mode when driver attach() & port_probe().
2. Add more documents for 3 generation IC with f81534_calc_num_ports().
3. Simplify the GPIO register structure "f81534_pin_control".
4. Change all counter type from int to size_t.
5. Change some failed message with failed: "status code" and remove all
   exclamation mark in messages.
6. Change all save blocks to block0 due to the driver is only used 1
   block (block0) to save data.
7. Change read MSR in open() instead of port_probe().
8. use GFP_ATOMIC kmalloc mode in write().
9. Maintain old style with 1 read URBs and 4 write URBs like mxuports.c
   I had tested with submit 4 read URBs, but it'll make some port freeze
   when doing BurnInTest Port test.

v7
1. Make all gpiolib function with #ifdef CONFIG_GPIOLIB marco block.
   Due to F81532/534 could run without gpiolib, we implements
   f81534_prepare_gpio()/f81534_release_gpio() always success without
   CONFIG_GPIOLIB.
2. Fix crash when receiving MSR change on driver load/unload. It's cause
   by f81534_process_read_urb() get read URB callback data, but port
   private data is not init complete or released. We solve with 2
   modifications.

   1. add null pointer check with f81534_process_read_urb(). We'll skip
  this report when port_priv = NULL.
   2. when "one" port f81534_port_remove() is called, kill the port-0
  read URB before kfree port_priv.

v6
1. Re-implement the write()/resume() function. Due to this device cant be
   suitable with generic write(), we'll do the submit write URB when
   write()/received tx empty/set_termios()/resume()
2. Logic/Phy Port mapping rewrite in f81534_port_probe() &
   f81534_phy_to_logic_port().
3. Introduced "Port Hide" function. Some customer use F81532 reference
   design for HW layout, but really use F81534 IC. We'll check
   F81534_PORT_CONF_DISABLE_PORT flag with in uart mode field to do
   port hide with port not used. It can be avoid end-user to use not
   layouted port.
4. The 4x3 output-only open-drain pins for F81532/534 is designed for
   control outer devices (with our EVB for examples, the 4 sets of pins
   are designed to control transceiver mode). So we decide to implement
   with gpiolib interface.
5. Add device vendor id with 0x2c42

v5
1. Change f81534_port_disable/enable() from H/W mode to S/W mode
   It'll skip all rx data when port is not opened.
2. Some function modifier add with static (Thanks for Paul Bolle)
3. It's will direct return when count=0 in f81534_write() to
   reduce spin_lock usage.

v4
1.