[PATCH V2 0/4] Fintek F81232 driver improve and fix

[PATCH V2 0/4] Fintek F81232 driver improve and fix

[Ji-Ze Hong (Peter Hong)]

There are 2 patches changed from v1:
	USB: serial: f81232: clear overrun flag
		1: flush the lsr_worker when port closed
		   (noticed by Oliver Neukum).

	USB: serial: f81232: implement break control
		1: Add private is_break to record break status and re-apply
		   when LCR changed in f81232_set_termios().
		2: Fix wrong usage for f81232_set_mask_register() in
		   f81232_break_ctl().
	
Ji-Ze Hong (Peter Hong) (4):
  USB: serial: f81232: clear overrun flag
  USB: serial: f81232: add high baud rate support
  USB: serial: f81232: implement break control
  USB: serial: f81232: fix bulk_in/out size

 drivers/usb/serial/f81232.c | 163 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 144 insertions(+), 19 deletions(-)

-- 
2.7.4

[PATCH V2 1/4] USB: serial: f81232: clear overrun flag

[Ji-Ze Hong (Peter Hong)]

The F81232 will report data and LSR with bulk like following format:
bulk-in data: [LSR(1Byte)+DATA(1Byte)][LSR(1Byte)+DATA(1Byte)]...

LSR will auto clear frame/parity/break error flag when reading by H/W,
but overrrun will only cleared when reading LSR. So this patch add a
worker to read LSR when OE.

Cc: Oliver Neukum <oneukum@suse.com>
Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
---
v2:
        1: Add flush_work(&port_priv->lsr_work) in f81232_close().
           Thanks for notice by Oliver Neukum

 drivers/usb/serial/f81232.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 96036f87b1de..2f595307dd14 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -41,12 +41,14 @@ MODULE_DEVICE_TABLE(usb, id_table);
 #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)
 
 struct f81232_private {
 	struct mutex lock;
 	u8 modem_control;
 	u8 modem_status;
+	struct work_struct lsr_work;
 	struct work_struct interrupt_work;
 	struct usb_serial_port *port;
 };
@@ -282,6 +284,7 @@ static void f81232_read_int_callback(struct urb *urb)
 static void f81232_process_read_urb(struct urb *urb)
 {
 	struct usb_serial_port *port = urb->context;
+	struct f81232_private *priv = usb_get_serial_port_data(port);
 	unsigned char *data = urb->transfer_buffer;
 	char tty_flag;
 	unsigned int i;
@@ -315,6 +318,7 @@ static void f81232_process_read_urb(struct urb *urb)
 
 			if (lsr & UART_LSR_OE) {
 				port->icount.overrun++;
+				schedule_work(&priv->lsr_work);
 				tty_insert_flip_char(&port->port, 0,
 						TTY_OVERRUN);
 			}
@@ -556,9 +560,12 @@ static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
 
 static void f81232_close(struct usb_serial_port *port)
 {
+	struct f81232_private *port_priv = usb_get_serial_port_data(port);
+
 	f81232_port_disable(port);
 	usb_serial_generic_close(port);
 	usb_kill_urb(port->interrupt_in_urb);
+	flush_work(&port_priv->lsr_work);
 }
 
 static void f81232_dtr_rts(struct usb_serial_port *port, int on)
@@ -623,6 +630,21 @@ static void  f81232_interrupt_work(struct work_struct *work)
 	f81232_read_msr(priv->port);
 }
 
+static void f81232_lsr_worker(struct work_struct *work)
+{
+	struct f81232_private *priv;
+	struct usb_serial_port *port;
+	int status;
+	u8 tmp;
+
+	priv = container_of(work, struct f81232_private, lsr_work);
+	port = priv->port;
+
+	status = f81232_get_register(port, LINE_STATUS_REGISTER, &tmp);
+	if (status)
+		dev_warn(&port->dev, "read LSR failed: %d\n", status);
+}
+
 static int f81232_port_probe(struct usb_serial_port *port)
 {
 	struct f81232_private *priv;
@@ -633,6 +655,7 @@ static int f81232_port_probe(struct usb_serial_port *port)
 
 	mutex_init(&priv->lock);
 	INIT_WORK(&priv->interrupt_work,  f81232_interrupt_work);
+	INIT_WORK(&priv->lsr_work, f81232_lsr_worker);
 
 	usb_set_serial_port_data(port, priv);
 
-- 
2.7.4

[PATCH V2 2/4] USB: serial: f81232: add high baud rate support

[Ji-Ze Hong (Peter Hong)]

The F81232 had 4 clocksource 1.846/18.46/14.77/24MHz and baud rates
can be up to 1.5Mbits with 24MHz.

F81232 Clock registers (106h)

Bit1-0:     Clock source selector
                    00: 1.846MHz.
                    01: 18.46MHz.
                    10: 24MHz.
                    11: 14.77MHz.

Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
---
 drivers/usb/serial/f81232.c | 105 +++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 94 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 2f595307dd14..56adae8a9ee0 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -28,7 +28,8 @@ static const struct usb_device_id id_table[] = {
 MODULE_DEVICE_TABLE(usb, id_table);
 
 /* Maximum baudrate for F81232 */
-#define F81232_MAX_BAUDRATE		115200
+#define F81232_MAX_BAUDRATE		1500000
+#define F81232_DEF_BAUDRATE		9600
 
 /* USB Control EP parameter */
 #define F81232_REGISTER_REQUEST		0xa0
@@ -44,18 +45,42 @@ MODULE_DEVICE_TABLE(usb, id_table);
 #define LINE_STATUS_REGISTER		(0x05 + SERIAL_BASE_ADDRESS)
 #define MODEM_STATUS_REGISTER		(0x06 + SERIAL_BASE_ADDRESS)
 
+/*
+ * F81232 Clock registers (106h)
+ *
+ * Bit1-0:	Clock source selector
+ *			00: 1.846MHz.
+ *			01: 18.46MHz.
+ *			10: 24MHz.
+ *			11: 14.77MHz.
+ */
+#define F81232_CLK_REGISTER		0x106
+#define F81232_CLK_1_846_MHZ		0
+#define F81232_CLK_18_46_MHZ		BIT(0)
+#define F81232_CLK_24_MHZ		BIT(1)
+#define F81232_CLK_14_77_MHZ		(BIT(1) | BIT(0))
+#define F81232_CLK_MASK			GENMASK(1, 0)
+
 struct f81232_private {
 	struct mutex lock;
 	u8 modem_control;
 	u8 modem_status;
+	speed_t baud_base;
 	struct work_struct lsr_work;
 	struct work_struct interrupt_work;
 	struct usb_serial_port *port;
 };
 
-static int calc_baud_divisor(speed_t baudrate)
+static u32 const baudrate_table[] = { 115200, 921600, 1152000, 1500000 };
+static u8 const clock_table[] = { F81232_CLK_1_846_MHZ, F81232_CLK_14_77_MHZ,
+				F81232_CLK_18_46_MHZ, F81232_CLK_24_MHZ };
+
+static int calc_baud_divisor(speed_t baudrate, speed_t clockrate)
 {
-	return DIV_ROUND_CLOSEST(F81232_MAX_BAUDRATE, baudrate);
+	if (!baudrate)
+		return 0;
+
+	return DIV_ROUND_CLOSEST(clockrate, baudrate);
 }
 
 static int f81232_get_register(struct usb_serial_port *port, u16 reg, u8 *val)
@@ -129,6 +154,21 @@ static int f81232_set_register(struct usb_serial_port *port, u16 reg, u8 val)
 	return status;
 }
 
+static int f81232_set_mask_register(struct usb_serial_port *port, u16 reg,
+					u8 mask, u8 val)
+{
+	int status;
+	u8 tmp;
+
+	status = f81232_get_register(port, reg, &tmp);
+	if (status)
+		return status;
+
+	tmp = (tmp & ~mask) | (val & mask);
+
+	return f81232_set_register(port, reg, tmp);
+}
+
 static void f81232_read_msr(struct usb_serial_port *port)
 {
 	int status;
@@ -346,13 +386,53 @@ static void f81232_break_ctl(struct tty_struct *tty, int break_state)
 	 */
 }
 
-static void f81232_set_baudrate(struct usb_serial_port *port, speed_t baudrate)
+static int f81232_find_clk(speed_t baudrate)
+{
+	int idx;
+
+	for (idx = 0; idx < ARRAY_SIZE(baudrate_table); ++idx) {
+		if (baudrate <= baudrate_table[idx] &&
+				baudrate_table[idx] % baudrate == 0)
+			return idx;
+	}
+
+	return -EINVAL;
+}
+
+static void f81232_set_baudrate(struct tty_struct *tty,
+				struct usb_serial_port *port, speed_t baudrate,
+				speed_t old_baudrate)
 {
+	struct f81232_private *priv = usb_get_serial_port_data(port);
 	u8 lcr;
 	int divisor;
 	int status = 0;
+	int i;
+	int idx;
+	speed_t baud_list[] = {baudrate, old_baudrate, F81232_DEF_BAUDRATE};
+
+	for (i = 0; i < ARRAY_SIZE(baud_list); ++i) {
+		idx = f81232_find_clk(baud_list[i]);
+		if (idx >= 0) {
+			baudrate = baud_list[i];
+			tty_encode_baud_rate(tty, baudrate, baudrate);
+			break;
+		}
+	}
 
-	divisor = calc_baud_divisor(baudrate);
+	if (idx < 0)
+		return;
+
+	priv->baud_base = baudrate_table[idx];
+	divisor = calc_baud_divisor(baudrate, priv->baud_base);
+
+	status = f81232_set_mask_register(port, F81232_CLK_REGISTER,
+			F81232_CLK_MASK, clock_table[idx]);
+	if (status) {
+		dev_err(&port->dev, "%s failed to set CLK_REG: %d\n",
+			__func__, status);
+		return;
+	}
 
 	status = f81232_get_register(port, LINE_CONTROL_REGISTER,
 			 &lcr); /* get LCR */
@@ -442,6 +522,7 @@ static void f81232_set_termios(struct tty_struct *tty,
 	u8 new_lcr = 0;
 	int status = 0;
 	speed_t baudrate;
+	speed_t old_baud;
 
 	/* Don't change anything if nothing has changed */
 	if (old_termios && !tty_termios_hw_change(&tty->termios, old_termios))
@@ -454,11 +535,12 @@ static void f81232_set_termios(struct tty_struct *tty,
 
 	baudrate = tty_get_baud_rate(tty);
 	if (baudrate > 0) {
-		if (baudrate > F81232_MAX_BAUDRATE) {
-			baudrate = F81232_MAX_BAUDRATE;
-			tty_encode_baud_rate(tty, baudrate, baudrate);
-		}
-		f81232_set_baudrate(port, baudrate);
+		if (old_termios)
+			old_baud = tty_termios_baud_rate(old_termios);
+		else
+			old_baud = F81232_DEF_BAUDRATE;
+
+		f81232_set_baudrate(tty, port, baudrate, old_baud);
 	}
 
 	if (C_PARENB(tty)) {
@@ -593,6 +675,7 @@ static int f81232_carrier_raised(struct usb_serial_port *port)
 static int f81232_get_serial_info(struct usb_serial_port *port,
 		unsigned long arg)
 {
+	struct f81232_private *priv = usb_get_serial_port_data(port);
 	struct serial_struct ser;
 
 	memset(&ser, 0, sizeof(ser));
@@ -600,7 +683,7 @@ static int f81232_get_serial_info(struct usb_serial_port *port,
 	ser.type = PORT_16550A;
 	ser.line = port->minor;
 	ser.port = port->port_number;
-	ser.baud_base = F81232_MAX_BAUDRATE;
+	ser.baud_base = priv->baud_base;
 
 	if (copy_to_user((void __user *)arg, &ser, sizeof(ser)))
 		return -EFAULT;
-- 
2.7.4

[PATCH V2 3/4] serial: f81232: implement break control

[Ji-Ze Hong (Peter Hong)]

Implement Fintek F81232 break on/off with LCR register.
It's the same with 16550A LCR register layout.

Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
---
v2:
	1: Fix f81232_break_ctl() for mis-operated set_mask_register().
	2: Add set break control bit in f81232_set_termios().

 drivers/usb/serial/f81232.c | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 56adae8a9ee0..863430d7535f 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -65,6 +65,7 @@ struct f81232_private {
 	struct mutex lock;
 	u8 modem_control;
 	u8 modem_status;
+	bool is_break;
 	speed_t baud_base;
 	struct work_struct lsr_work;
 	struct work_struct interrupt_work;
@@ -377,13 +378,24 @@ static void f81232_process_read_urb(struct urb *urb)
 
 static void f81232_break_ctl(struct tty_struct *tty, int break_state)
 {
-	/* FIXME - Stubbed out for now */
+	struct usb_serial_port *port = tty->driver_data;
+	struct f81232_private *priv = usb_get_serial_port_data(port);
+	int status;
+	u8 tmp = 0;
 
-	/*
-	 * break_state = -1 to turn on break, and 0 to turn off break
-	 * see drivers/char/tty_io.c to see it used.
-	 * last_set_data_urb_value NEVER has the break bit set in it.
-	 */
+	mutex_lock(&priv->lock);
+
+	if (break_state) {
+		priv->is_break = !!break_state;
+		tmp = UART_LCR_SBC;
+	}
+
+	status = f81232_set_mask_register(port, LINE_CONTROL_REGISTER,
+						UART_LCR_SBC, tmp);
+	if (status)
+		dev_err(&port->dev, "set break failed: %d\n", status);
+
+	mutex_unlock(&priv->lock);
 }
 
 static int f81232_find_clk(speed_t baudrate)
@@ -519,6 +531,7 @@ static int f81232_port_disable(struct usb_serial_port *port)
 static void f81232_set_termios(struct tty_struct *tty,
 		struct usb_serial_port *port, struct ktermios *old_termios)
 {
+	struct f81232_private *priv = usb_get_serial_port_data(port);
 	u8 new_lcr = 0;
 	int status = 0;
 	speed_t baudrate;
@@ -572,11 +585,18 @@ static void f81232_set_termios(struct tty_struct *tty,
 		break;
 	}
 
+	mutex_lock(&priv->lock);
+
+	if (priv->is_break)
+		new_lcr |= UART_LCR_SBC;
+
 	status = f81232_set_register(port, LINE_CONTROL_REGISTER, new_lcr);
 	if (status) {
 		dev_err(&port->dev, "%s failed to set LCR: %d\n",
 			__func__, status);
 	}
+
+	mutex_unlock(&priv->lock);
 }
 
 static int f81232_tiocmget(struct tty_struct *tty)
-- 
2.7.4

[PATCH V2 3/4] USB: serial: f81232: implement break control

[Ji-Ze Hong (Peter Hong)]

Implement Fintek F81232 break on/off with LCR register.
It's the same with 16550A LCR register layout.

Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
---
v2:
        1: Fix f81232_break_ctl() for mis-operated set_mask_register().
        2: Add set break control bit in f81232_set_termios().

 drivers/usb/serial/f81232.c | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 56adae8a9ee0..863430d7535f 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -65,6 +65,7 @@ struct f81232_private {
 	struct mutex lock;
 	u8 modem_control;
 	u8 modem_status;
+	bool is_break;
 	speed_t baud_base;
 	struct work_struct lsr_work;
 	struct work_struct interrupt_work;
@@ -377,13 +378,24 @@ static void f81232_process_read_urb(struct urb *urb)
 
 static void f81232_break_ctl(struct tty_struct *tty, int break_state)
 {
-	/* FIXME - Stubbed out for now */
+	struct usb_serial_port *port = tty->driver_data;
+	struct f81232_private *priv = usb_get_serial_port_data(port);
+	int status;
+	u8 tmp = 0;
 
-	/*
-	 * break_state = -1 to turn on break, and 0 to turn off break
-	 * see drivers/char/tty_io.c to see it used.
-	 * last_set_data_urb_value NEVER has the break bit set in it.
-	 */
+	mutex_lock(&priv->lock);
+
+	if (break_state) {
+		priv->is_break = !!break_state;
+		tmp = UART_LCR_SBC;
+	}
+
+	status = f81232_set_mask_register(port, LINE_CONTROL_REGISTER,
+						UART_LCR_SBC, tmp);
+	if (status)
+		dev_err(&port->dev, "set break failed: %d\n", status);
+
+	mutex_unlock(&priv->lock);
 }
 
 static int f81232_find_clk(speed_t baudrate)
@@ -519,6 +531,7 @@ static int f81232_port_disable(struct usb_serial_port *port)
 static void f81232_set_termios(struct tty_struct *tty,
 		struct usb_serial_port *port, struct ktermios *old_termios)
 {
+	struct f81232_private *priv = usb_get_serial_port_data(port);
 	u8 new_lcr = 0;
 	int status = 0;
 	speed_t baudrate;
@@ -572,11 +585,18 @@ static void f81232_set_termios(struct tty_struct *tty,
 		break;
 	}
 
+	mutex_lock(&priv->lock);
+
+	if (priv->is_break)
+		new_lcr |= UART_LCR_SBC;
+
 	status = f81232_set_register(port, LINE_CONTROL_REGISTER, new_lcr);
 	if (status) {
 		dev_err(&port->dev, "%s failed to set LCR: %d\n",
 			__func__, status);
 	}
+
+	mutex_unlock(&priv->lock);
 }
 
 static int f81232_tiocmget(struct tty_struct *tty)
-- 
2.7.4

[PATCH V2 4/4] USB: serial: f81232: fix bulk_in/out size

[Ji-Ze Hong (Peter Hong)]

Fix Fintek F81232 bulk_in/out size to 64/16 according to the spec.
http://html.alldatasheet.com/html-pdf/406315/FINTEK/F81232/1762/8/F81232.html

Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
---
 drivers/usb/serial/f81232.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 863430d7535f..e3ab1646a693 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -785,8 +785,7 @@ static struct usb_serial_driver f81232_device = {
 	},
 	.id_table =		id_table,
 	.num_ports =		1,
-	.bulk_in_size =		256,
-	.bulk_out_size =	256,
+	.bulk_out_size =	16,
 	.open =			f81232_open,
 	.close =		f81232_close,
 	.dtr_rts =		f81232_dtr_rts,
-- 
2.7.4

Re: [PATCH V2 3/4] serial: f81232: implement break control

[Ji-Ze Hong (Peter Hong)]

Hi Johan,

This article is duplication with
[PATCH V2 3/4] USB: serial: f81232: implement break control
[PATCH V2 3/4] serial: f81232: implement break control
	(Title not start with USB token)

Should I discard this PATCH_V2 series and re-send PATCH_V3 ?
or could you discard the patch - serial: f81232: implement break control
only ?

Thanks

Ji-Ze Hong (Peter Hong) 於 2018/8/30 上午 09:47 寫道:
> Implement Fintek F81232 break on/off with LCR register.
> It's the same with 16550A LCR register layout.
> 
> Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
> ---
> v2:
> 	1: Fix f81232_break_ctl() for mis-operated set_mask_register().
> 	2: Add set break control bit in f81232_set_termios().
> 
>   drivers/usb/serial/f81232.c | 32 ++++++++++++++++++++++++++------
>   1 file changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index 56adae8a9ee0..863430d7535f 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -65,6 +65,7 @@ struct f81232_private {
>   	struct mutex lock;
>   	u8 modem_control;
>   	u8 modem_status;
> +	bool is_break;
>   	speed_t baud_base;
>   	struct work_struct lsr_work;
>   	struct work_struct interrupt_work;
> @@ -377,13 +378,24 @@ static void f81232_process_read_urb(struct urb *urb)
>   
>   static void f81232_break_ctl(struct tty_struct *tty, int break_state)
>   {
> -	/* FIXME - Stubbed out for now */
> +	struct usb_serial_port *port = tty->driver_data;
> +	struct f81232_private *priv = usb_get_serial_port_data(port);
> +	int status;
> +	u8 tmp = 0;
>   
> -	/*
> -	 * break_state = -1 to turn on break, and 0 to turn off break
> -	 * see drivers/char/tty_io.c to see it used.
> -	 * last_set_data_urb_value NEVER has the break bit set in it.
> -	 */
> +	mutex_lock(&priv->lock);
> +
> +	if (break_state) {
> +		priv->is_break = !!break_state;
> +		tmp = UART_LCR_SBC;
> +	}
> +
> +	status = f81232_set_mask_register(port, LINE_CONTROL_REGISTER,
> +						UART_LCR_SBC, tmp);
> +	if (status)
> +		dev_err(&port->dev, "set break failed: %d\n", status);
> +
> +	mutex_unlock(&priv->lock);
>   }
>   
>   static int f81232_find_clk(speed_t baudrate)
> @@ -519,6 +531,7 @@ static int f81232_port_disable(struct usb_serial_port *port)
>   static void f81232_set_termios(struct tty_struct *tty,
>   		struct usb_serial_port *port, struct ktermios *old_termios)
>   {
> +	struct f81232_private *priv = usb_get_serial_port_data(port);
>   	u8 new_lcr = 0;
>   	int status = 0;
>   	speed_t baudrate;
> @@ -572,11 +585,18 @@ static void f81232_set_termios(struct tty_struct *tty,
>   		break;
>   	}
>   
> +	mutex_lock(&priv->lock);
> +
> +	if (priv->is_break)
> +		new_lcr |= UART_LCR_SBC;
> +
>   	status = f81232_set_register(port, LINE_CONTROL_REGISTER, new_lcr);
>   	if (status) {
>   		dev_err(&port->dev, "%s failed to set LCR: %d\n",
>   			__func__, status);
>   	}
> +
> +	mutex_unlock(&priv->lock);
>   }
>   
>   static int f81232_tiocmget(struct tty_struct *tty)
> 

-- 
With Best Regards,
Peter Hong

Re: [PATCH V2 3/4] serial: f81232: implement break control

[Johan Hovold]

On Thu, Aug 30, 2018 at 09:55:46AM +0800, Ji-Ze Hong (Peter Hong) wrote:
> Hi Johan,
> 
> This article is duplication with
> [PATCH V2 3/4] USB: serial: f81232: implement break control
> [PATCH V2 3/4] serial: f81232: implement break control
> 	(Title not start with USB token)
> 
> Should I discard this PATCH_V2 series and re-send PATCH_V3 ?
> or could you discard the patch - serial: f81232: implement break control
> only ?

Yes, no problem, so no need to resend. But thanks for highlighting this.

I'll try to look at these next week.

Johan

Re: [PATCH V2 1/4] USB: serial: f81232: clear overrun flag

[Johan Hovold]

On Thu, Aug 30, 2018 at 09:47:10AM +0800, Ji-Ze Hong (Peter Hong) wrote:
> The F81232 will report data and LSR with bulk like following format:
> bulk-in data: [LSR(1Byte)+DATA(1Byte)][LSR(1Byte)+DATA(1Byte)]...
> 
> LSR will auto clear frame/parity/break error flag when reading by H/W,
> but overrrun will only cleared when reading LSR. So this patch add a
> worker to read LSR when OE.
> 
> Cc: Oliver Neukum <oneukum@suse.com>
> Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
> ---
> v2:
>         1: Add flush_work(&port_priv->lsr_work) in f81232_close().
>            Thanks for notice by Oliver Neukum

Looks like you ignored Oliver's comment that you needed to take care of
any pending lsr work both and close and suspend.

Johan

Re: [PATCH V2 3/4] USB: serial: f81232: implement break control

[Johan Hovold]

On Thu, Aug 30, 2018 at 09:47:13AM +0800, Ji-Ze Hong (Peter Hong) wrote:
> Implement Fintek F81232 break on/off with LCR register.
> It's the same with 16550A LCR register layout.
> 
> Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
> ---
> v2:
>         1: Fix f81232_break_ctl() for mis-operated set_mask_register().
>         2: Add set break control bit in f81232_set_termios().
> 
>  drivers/usb/serial/f81232.c | 32 ++++++++++++++++++++++++++------
>  1 file changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index 56adae8a9ee0..863430d7535f 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -65,6 +65,7 @@ struct f81232_private {
>  	struct mutex lock;
>  	u8 modem_control;
>  	u8 modem_status;
> +	bool is_break;
>  	speed_t baud_base;
>  	struct work_struct lsr_work;
>  	struct work_struct interrupt_work;
> @@ -377,13 +378,24 @@ static void f81232_process_read_urb(struct urb *urb)
>  
>  static void f81232_break_ctl(struct tty_struct *tty, int break_state)
>  {
> -	/* FIXME - Stubbed out for now */
> +	struct usb_serial_port *port = tty->driver_data;
> +	struct f81232_private *priv = usb_get_serial_port_data(port);
> +	int status;
> +	u8 tmp = 0;
>  
> -	/*
> -	 * break_state = -1 to turn on break, and 0 to turn off break
> -	 * see drivers/char/tty_io.c to see it used.
> -	 * last_set_data_urb_value NEVER has the break bit set in it.
> -	 */
> +	mutex_lock(&priv->lock);
> +
> +	if (break_state) {
> +		priv->is_break = !!break_state;
> +		tmp = UART_LCR_SBC;
> +	}
> +
> +	status = f81232_set_mask_register(port, LINE_CONTROL_REGISTER,
> +						UART_LCR_SBC, tmp);
> +	if (status)
> +		dev_err(&port->dev, "set break failed: %d\n", status);

Why not maintain a shadow lsr value so that you don't have to retrieve
it every time you want to change the break state? It looks like you do
it this way in f81534.

Johan

Re: [PATCH V2 4/4] USB: serial: f81232: fix bulk_in/out size

[Johan Hovold]

On Thu, Aug 30, 2018 at 09:47:14AM +0800, Ji-Ze Hong (Peter Hong) wrote:
> Fix Fintek F81232 bulk_in/out size to 64/16 according to the spec.
> http://html.alldatasheet.com/html-pdf/406315/FINTEK/F81232/1762/8/F81232.html

I know we've discussed this before, but please explain here in the
commit message why you want to do this.

The bulk_in/out_size fields only set the buffer sizes used by the
driver and using buffers larger than the endpoint size is typically only
more efficient even if there may be hardware reasons for not wanting to
use that.

You said last time, you datasheets were incorrect and that the devices
were in fact using 64 b endpoints, right?

> Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
> ---
>  drivers/usb/serial/f81232.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index 863430d7535f..e3ab1646a693 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -785,8 +785,7 @@ static struct usb_serial_driver f81232_device = {
>  	},
>  	.id_table =		id_table,
>  	.num_ports =		1,
> -	.bulk_in_size =		256,
> -	.bulk_out_size =	256,
> +	.bulk_out_size =	16,
>  	.open =			f81232_open,
>  	.close =		f81232_close,
>  	.dtr_rts =		f81232_dtr_rts,

Johan

Re: [PATCH V2 1/4] USB: serial: f81232: clear overrun flag

[Ji-Ze Hong (Peter Hong)]

Hi,

Johan Hovold 於 2018/9/5 下午 10:39 寫道:
> Looks like you ignored Oliver's comment that you needed to take care of
> any pending lsr work both and close and suspend.
> 

I'll add the cancel operation when suspend() with next version.

Thanks
-- 
With Best Regards,
Peter Hong