[PATCH 0/7] serial: 8250_fintek: Fix the IRQ mode and code refactoring

[PATCH 0/7] serial: 8250_fintek: Fix the IRQ mode and code refactoring

[Ji-Ze Hong (Peter Hong)]

The following patches will fix the Fintek LPC to UARTs IRQ mode mismatch
issue, code refactoring and this series patches should follow by patch
rename IRQ_MODE macro on
'commit 87a713c8ffca ("8250/fintek: rename IRQ_MODE macro")'.
with tty-linus branch.

Some BIOS only use _OSI("Linux") to distinguish between Linux & Windows.
Apply Level/Low to UART trigger mode if Windows, Edge/High otherwise.
But since 2.6.23 the mainline kernel no longer returns true for
_OSI(“Linux”). The BIOS ASL should avoid to use _OSI() to check system
type and use ACPI MADT override IRQ mode instead.

We'll try to refactoring the source code more readable with SuperIO
read/write register functions.

Ricardo Ribalda Delgado suggest fixing a potential NULL pointer
dereference with applied patch
fix the mismatched IRQ mode on
'commit 4da22f1418cb ("serial: 8250_fintek: fix the mismatched IRQ mode")'.
had been implemented with 8250_fintek: Set IRQ Mode when port probed.

Ji-Ze Hong (Peter Hong) (7):
  serial: 8250_fintek: Refactoring read/write method
  serial: 8250_fintek: Set IRQ Mode when port probed
  serial: 8250_fintek: Set maximum FIFO of F81216H
  serial: 8250_fintek: Rearrange function
  serial: 8250_fintek: Add F81216 Support
  serial: 8250_fintek: Add F81866 Support
  serial: 8250_fintek: Add F81865 Support

 drivers/tty/serial/8250/8250_fintek.c | 233 +++++++++++++++++++++++++---------
 1 file changed, 175 insertions(+), 58 deletions(-)

-- 
1.9.1

[PATCH 1/7] serial: 8250_fintek: Refactoring read/write method

[Ji-Ze Hong (Peter Hong)]

If we need to access SuperIO registers, It should write register offset
to base_addr and read/write value to base_addr + 1 to perform read/write.
We can make it more simply with write/read functions.

This patch add sio_read_reg()/sio_write_reg()/sio_write_mask_reg() to
reduce SuperIO register operation with lot of outb()/inb().

Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
---
 drivers/tty/serial/8250/8250_fintek.c | 73 ++++++++++++++++++-----------------
 1 file changed, 38 insertions(+), 35 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_fintek.c b/drivers/tty/serial/8250/8250_fintek.c
index 0facc78..2ae846e 100644
--- a/drivers/tty/serial/8250/8250_fintek.c
+++ b/drivers/tty/serial/8250/8250_fintek.c
@@ -49,6 +49,27 @@ struct fintek_8250 {
 	u8 key;
 };
 
+static u8 sio_read_reg(struct fintek_8250 *pdata, u8 reg)
+{
+	outb(reg, pdata->base_port + ADDR_PORT);
+	return inb(pdata->base_port + DATA_PORT);
+}
+
+static void sio_write_reg(struct fintek_8250 *pdata, u8 reg, u8 data)
+{
+	outb(reg, pdata->base_port + ADDR_PORT);
+	outb(data, pdata->base_port + DATA_PORT);
+}
+
+static void sio_write_mask_reg(struct fintek_8250 *pdata, u8 reg, u8 mask,
+			       u8 data)
+{
+	u8 tmp;
+
+	tmp = (sio_read_reg(pdata, reg) & ~mask) | (mask & data);
+	sio_write_reg(pdata, reg, tmp);
+}
+
 static int fintek_8250_enter_key(u16 base_port, u8 key)
 {
 	if (!request_muxed_region(base_port, 2, "8250_fintek"))
@@ -66,22 +87,18 @@ static void fintek_8250_exit_key(u16 base_port)
 	release_region(base_port + ADDR_PORT, 2);
 }
 
-static int fintek_8250_check_id(u16 base_port)
+static int fintek_8250_check_id(struct fintek_8250 *pdata)
 {
 	u16 chip;
 
-	outb(VENDOR_ID1, base_port + ADDR_PORT);
-	if (inb(base_port + DATA_PORT) != VENDOR_ID1_VAL)
+	if (sio_read_reg(pdata, VENDOR_ID1) != VENDOR_ID1_VAL)
 		return -ENODEV;
 
-	outb(VENDOR_ID2, base_port + ADDR_PORT);
-	if (inb(base_port + DATA_PORT) != VENDOR_ID2_VAL)
+	if (sio_read_reg(pdata, VENDOR_ID2) != VENDOR_ID2_VAL)
 		return -ENODEV;
 
-	outb(CHIP_ID1, base_port + ADDR_PORT);
-	chip = inb(base_port + DATA_PORT);
-	outb(CHIP_ID2, base_port + ADDR_PORT);
-	chip |= inb(base_port + DATA_PORT) << 8;
+	chip = sio_read_reg(pdata, CHIP_ID1);
+	chip |= sio_read_reg(pdata, CHIP_ID2) << 8;
 
 	if (chip != CHIP_ID_0 && chip != CHIP_ID_1)
 		return -ENODEV;
@@ -128,10 +145,8 @@ static int fintek_8250_rs485_config(struct uart_port *port,
 	if (fintek_8250_enter_key(pdata->base_port, pdata->key))
 		return -EBUSY;
 
-	outb(LDN, pdata->base_port + ADDR_PORT);
-	outb(pdata->index, pdata->base_port + DATA_PORT);
-	outb(RS485, pdata->base_port + ADDR_PORT);
-	outb(config, pdata->base_port + DATA_PORT);
+	sio_write_reg(pdata, LDN, pdata->index);
+	sio_write_reg(pdata, RS485, config);
 	fintek_8250_exit_key(pdata->base_port);
 
 	port->rs485 = *rs485;
@@ -147,10 +162,12 @@ static int find_base_port(struct fintek_8250 *pdata, u16 io_address)
 
 	for (i = 0; i < ARRAY_SIZE(addr); i++) {
 		for (j = 0; j < ARRAY_SIZE(keys); j++) {
+			pdata->base_port = addr[i];
+			pdata->key = keys[j];
 
 			if (fintek_8250_enter_key(addr[i], keys[j]))
 				continue;
-			if (fintek_8250_check_id(addr[i])) {
+			if (fintek_8250_check_id(pdata)) {
 				fintek_8250_exit_key(addr[i]);
 				continue;
 			}
@@ -158,19 +175,13 @@ static int find_base_port(struct fintek_8250 *pdata, u16 io_address)
 			for (k = 0; k < 4; k++) {
 				u16 aux;
 
-				outb(LDN, addr[i] + ADDR_PORT);
-				outb(k, addr[i] + DATA_PORT);
-
-				outb(IO_ADDR1, addr[i] + ADDR_PORT);
-				aux = inb(addr[i] + DATA_PORT);
-				outb(IO_ADDR2, addr[i] + ADDR_PORT);
-				aux |= inb(addr[i] + DATA_PORT) << 8;
+				sio_write_reg(pdata, LDN, k);
+				aux = sio_read_reg(pdata, IO_ADDR1);
+				aux |= sio_read_reg(pdata, IO_ADDR2) << 8;
 				if (aux != io_address)
 					continue;
 
 				fintek_8250_exit_key(addr[i]);
-				pdata->key = keys[j];
-				pdata->base_port = addr[i];
 				pdata->index = k;
 
 				return 0;
@@ -186,24 +197,16 @@ static int find_base_port(struct fintek_8250 *pdata, u16 io_address)
 static int fintek_8250_set_irq_mode(struct fintek_8250 *pdata, bool level_mode)
 {
 	int status;
-	u8 tmp;
 
 	status = fintek_8250_enter_key(pdata->base_port, pdata->key);
 	if (status)
 		return status;
 
-	outb(LDN, pdata->base_port + ADDR_PORT);
-	outb(pdata->index, pdata->base_port + DATA_PORT);
-
-	outb(FINTEK_IRQ_MODE, pdata->base_port + ADDR_PORT);
-	tmp = inb(pdata->base_port + DATA_PORT);
-
-	tmp &= ~IRQ_MODE_MASK;
-	tmp |= IRQ_SHARE;
-	if (!level_mode)
-		tmp |= IRQ_EDGE_HIGH;
+	sio_write_reg(pdata, LDN, pdata->index);
+	sio_write_mask_reg(pdata, FINTEK_IRQ_MODE, IRQ_SHARE, IRQ_SHARE);
+	sio_write_mask_reg(pdata, FINTEK_IRQ_MODE, IRQ_MODE_MASK,
+			   level_mode ? IRQ_LEVEL_LOW : IRQ_EDGE_HIGH);
 
-	outb(tmp, pdata->base_port + DATA_PORT);
 	fintek_8250_exit_key(pdata->base_port);
 	return 0;
 }
-- 
1.9.1

[PATCH 2/7] serial: 8250_fintek: Set IRQ Mode when port probed

[Ji-Ze Hong (Peter Hong)]

Set IRQ Mode when port probed in find_base_port()

It should hold the IO port premission via fintek_8250_enter_key() and
release via fintek_8250_exit_key() when we configure the SuperIO.

This patch will move all SuperIO configure operations to find_base_port()
to reduce fintek_8250_enter_key()/fintek_8250_exit_key() usage.

Suggested-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
---
 drivers/tty/serial/8250/8250_fintek.c | 36 ++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_fintek.c b/drivers/tty/serial/8250/8250_fintek.c
index 2ae846e..cd8903f 100644
--- a/drivers/tty/serial/8250/8250_fintek.c
+++ b/drivers/tty/serial/8250/8250_fintek.c
@@ -49,6 +49,9 @@ struct fintek_8250 {
 	u8 key;
 };
 
+static void fintek_8250_set_irq_mode(struct fintek_8250 *pdata,
+				     bool level_mode);
+
 static u8 sio_read_reg(struct fintek_8250 *pdata, u8 reg)
 {
 	outb(reg, pdata->base_port + ADDR_PORT);
@@ -154,10 +157,13 @@ static int fintek_8250_rs485_config(struct uart_port *port,
 	return 0;
 }
 
-static int find_base_port(struct fintek_8250 *pdata, u16 io_address)
+static int find_base_port(struct fintek_8250 *pdata, u16 io_address,
+			  unsigned int irq)
 {
 	static const u16 addr[] = {0x4e, 0x2e};
 	static const u8 keys[] = {0x77, 0xa0, 0x87, 0x67};
+	struct irq_data *irq_data;
+	bool level_mode = false;
 	int i, j, k;
 
 	for (i = 0; i < ARRAY_SIZE(addr); i++) {
@@ -181,9 +187,16 @@ static int find_base_port(struct fintek_8250 *pdata, u16 io_address)
 				if (aux != io_address)
 					continue;
 
-				fintek_8250_exit_key(addr[i]);
 				pdata->index = k;
 
+				irq_data = irq_get_irq_data(irq);
+				if (irq_data)
+					level_mode =
+						irqd_is_level_type(irq_data);
+
+				fintek_8250_set_irq_mode(pdata, level_mode);
+				fintek_8250_exit_key(addr[i]);
+
 				return 0;
 			}
 
@@ -194,31 +207,20 @@ static int find_base_port(struct fintek_8250 *pdata, u16 io_address)
 	return -ENODEV;
 }
 
-static int fintek_8250_set_irq_mode(struct fintek_8250 *pdata, bool level_mode)
+static void fintek_8250_set_irq_mode(struct fintek_8250 *pdata, bool is_level)
 {
-	int status;
-
-	status = fintek_8250_enter_key(pdata->base_port, pdata->key);
-	if (status)
-		return status;
-
 	sio_write_reg(pdata, LDN, pdata->index);
 	sio_write_mask_reg(pdata, FINTEK_IRQ_MODE, IRQ_SHARE, IRQ_SHARE);
 	sio_write_mask_reg(pdata, FINTEK_IRQ_MODE, IRQ_MODE_MASK,
-			   level_mode ? IRQ_LEVEL_LOW : IRQ_EDGE_HIGH);
-
-	fintek_8250_exit_key(pdata->base_port);
-	return 0;
+			   is_level ? IRQ_LEVEL_LOW : IRQ_EDGE_HIGH);
 }
 
 int fintek_8250_probe(struct uart_8250_port *uart)
 {
 	struct fintek_8250 *pdata;
 	struct fintek_8250 probe_data;
-	struct irq_data *irq_data = irq_get_irq_data(uart->port.irq);
-	bool level_mode = irqd_is_level_type(irq_data);
 
-	if (find_base_port(&probe_data, uart->port.iobase))
+	if (find_base_port(&probe_data, uart->port.iobase, uart->port.irq))
 		return -ENODEV;
 
 	pdata = devm_kzalloc(uart->port.dev, sizeof(*pdata), GFP_KERNEL);
@@ -229,5 +231,5 @@ int fintek_8250_probe(struct uart_8250_port *uart)
 	uart->port.rs485_config = fintek_8250_rs485_config;
 	uart->port.private_data = pdata;
 
-	return fintek_8250_set_irq_mode(pdata, level_mode);
+	return 0;
 }
-- 
1.9.1

[PATCH 3/7] serial: 8250_fintek: Set maximum FIFO of F81216H

[Ji-Ze Hong (Peter Hong)]

The Fintek F81216H had maximum 128Bytes FIFO, but some BIOS configurated
as normal 16Bytes FIFO. This patch will set 128Bytes FIFO and trigger
level multiplier as 4x when F81216H detected.

Default 16550A trigger level is 8Bytes. When this patch applied, the
trigger level will change to 8Byte x 4 = 32Byte. It can be reduce the RX
incoming interrupts.

Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
---
 drivers/tty/serial/8250/8250_fintek.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_fintek.c b/drivers/tty/serial/8250/8250_fintek.c
index cd8903f..5625203 100644
--- a/drivers/tty/serial/8250/8250_fintek.c
+++ b/drivers/tty/serial/8250/8250_fintek.c
@@ -21,8 +21,8 @@
 #define EXIT_KEY 0xAA
 #define CHIP_ID1  0x20
 #define CHIP_ID2  0x21
-#define CHIP_ID_0 0x1602
-#define CHIP_ID_1 0x0501
+#define CHIP_ID_F81216AD 0x1602
+#define CHIP_ID_F81216H 0x0501
 #define VENDOR_ID1 0x23
 #define VENDOR_ID1_VAL 0x19
 #define VENDOR_ID2 0x24
@@ -43,7 +43,14 @@
 #define RXW4C_IRA BIT(3)
 #define TXW4C_IRA BIT(2)
 
+#define FIFO_CTRL		0xF6
+#define FIFO_MODE_MASK		(BIT(1) | BIT(0))
+#define FIFO_MODE_128		(BIT(1) | BIT(0))
+#define RXFTHR_MODE_MASK	(BIT(5) | BIT(4))
+#define RXFTHR_MODE_4X		BIT(5)
+
 struct fintek_8250 {
+	u16 pid;
 	u16 base_port;
 	u8 index;
 	u8 key;
@@ -103,9 +110,10 @@ static int fintek_8250_check_id(struct fintek_8250 *pdata)
 	chip = sio_read_reg(pdata, CHIP_ID1);
 	chip |= sio_read_reg(pdata, CHIP_ID2) << 8;
 
-	if (chip != CHIP_ID_0 && chip != CHIP_ID_1)
+	if (chip != CHIP_ID_F81216AD && chip != CHIP_ID_F81216H)
 		return -ENODEV;
 
+	pdata->pid = chip;
 	return 0;
 }
 
@@ -157,6 +165,20 @@ static int fintek_8250_rs485_config(struct uart_port *port,
 	return 0;
 }
 
+static void fintek_8250_set_max_fifo(struct fintek_8250 *pdata)
+{
+	switch (pdata->pid) {
+	default: /* Default 16Bytes FIFO */
+		return;
+
+	case CHIP_ID_F81216H: /* 128Bytes FIFO */
+		sio_write_mask_reg(pdata, FIFO_CTRL,
+				   FIFO_MODE_MASK | RXFTHR_MODE_MASK,
+				   FIFO_MODE_128 | RXFTHR_MODE_4X);
+		break;
+	}
+}
+
 static int find_base_port(struct fintek_8250 *pdata, u16 io_address,
 			  unsigned int irq)
 {
@@ -195,6 +217,7 @@ static int find_base_port(struct fintek_8250 *pdata, u16 io_address,
 						irqd_is_level_type(irq_data);
 
 				fintek_8250_set_irq_mode(pdata, level_mode);
+				fintek_8250_set_max_fifo(pdata);
 				fintek_8250_exit_key(addr[i]);
 
 				return 0;
-- 
1.9.1

Re: [PATCH 3/7] serial: 8250_fintek: Set maximum FIFO of F81216H

[Ricardo Ribalda Delgado]

Hi Peter

On Thu, Sep 1, 2016 at 5:39 AM, Ji-Ze Hong (Peter Hong)
<hpeter@gmail.com> wrote:

> +static void fintek_8250_set_max_fifo(struct fintek_8250 *pdata)
> +{
> +       switch (pdata->pid) {
> +       default: /* Default 16Bytes FIFO */
> +               return;
> +
> +       case CHIP_ID_F81216H: /* 128Bytes FIFO */
> +               sio_write_mask_reg(pdata, FIFO_CTRL,
> +                                  FIFO_MODE_MASK | RXFTHR_MODE_MASK,
> +                                  FIFO_MODE_128 | RXFTHR_MODE_4X);
> +               break;
> +       }

I think it is more clear to have the default case as the ast one;
> +}
> +
>  static int find_base_port(struct fintek_8250 *pdata, u16 io_address,
>                           unsigned int irq)

I think that we should rename find_base_port to something else like
probe_setup_port(),  we are not only finding the port but also setting
it up.

>  {
> @@ -195,6 +217,7 @@ static int find_base_port(struct fintek_8250 *pdata, u16 io_address,
>                                                 irqd_is_level_type(irq_data);
>
>                                 fintek_8250_set_irq_mode(pdata, level_mode);
> +                               fintek_8250_set_max_fifo(pdata);
>                                 fintek_8250_exit_key(addr[i]);
>
>                                 return 0;
> --
> 1.9.1
>

Regrds!


-- 
Ricardo Ribalda

Re: [PATCH 3/7] serial: 8250_fintek: Set maximum FIFO of F81216H

[Ji-Ze Hong (Peter Hong)]

Hi Ricardo,

Ricardo Ribalda Delgado 於 2016/9/1 下午 07:16 寫道:
>> +static void fintek_8250_set_max_fifo(struct fintek_8250 *pdata)
>> +{
>> +       switch (pdata->pid) {
>> +       default: /* Default 16Bytes FIFO */
>> +               return;
>> +
>> +       case CHIP_ID_F81216H: /* 128Bytes FIFO */
>> +               sio_write_mask_reg(pdata, FIFO_CTRL,
>> +                                  FIFO_MODE_MASK | RXFTHR_MODE_MASK,
>> +                                  FIFO_MODE_128 | RXFTHR_MODE_4X);
>> +               break;
>> +       }
>
> I think it is more clear to have the default case as the ast one;

OK, I'll modify all default label to the last one.

>> +}
>> +
>>  static int find_base_port(struct fintek_8250 *pdata, u16 io_address,
>>                           unsigned int irq)
>
> I think that we should rename find_base_port to something else like
> probe_setup_port(),  we are not only finding the port but also setting
> it up.
>

OK, I'll modify it with next version.

-- 
With Best Regards,
Peter Hong

[PATCH 4/7] serial: 8250_fintek: Rearrange function

[Ji-Ze Hong (Peter Hong)]

We change the position of fintek_8250_set_irq_mode() above the
find_base_port() to eliminate the prototype define.

Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
---
 drivers/tty/serial/8250/8250_fintek.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_fintek.c b/drivers/tty/serial/8250/8250_fintek.c
index 5625203..921f742 100644
--- a/drivers/tty/serial/8250/8250_fintek.c
+++ b/drivers/tty/serial/8250/8250_fintek.c
@@ -56,9 +56,6 @@ struct fintek_8250 {
 	u8 key;
 };
 
-static void fintek_8250_set_irq_mode(struct fintek_8250 *pdata,
-				     bool level_mode);
-
 static u8 sio_read_reg(struct fintek_8250 *pdata, u8 reg)
 {
 	outb(reg, pdata->base_port + ADDR_PORT);
@@ -179,6 +176,14 @@ static void fintek_8250_set_max_fifo(struct fintek_8250 *pdata)
 	}
 }
 
+static void fintek_8250_set_irq_mode(struct fintek_8250 *pdata, bool is_level)
+{
+	sio_write_reg(pdata, LDN, pdata->index);
+	sio_write_mask_reg(pdata, FINTEK_IRQ_MODE, IRQ_SHARE, IRQ_SHARE);
+	sio_write_mask_reg(pdata, FINTEK_IRQ_MODE, IRQ_MODE_MASK,
+			   is_level ? IRQ_LEVEL_LOW : IRQ_EDGE_HIGH);
+}
+
 static int find_base_port(struct fintek_8250 *pdata, u16 io_address,
 			  unsigned int irq)
 {
@@ -230,14 +235,6 @@ static int find_base_port(struct fintek_8250 *pdata, u16 io_address,
 	return -ENODEV;
 }
 
-static void fintek_8250_set_irq_mode(struct fintek_8250 *pdata, bool is_level)
-{
-	sio_write_reg(pdata, LDN, pdata->index);
-	sio_write_mask_reg(pdata, FINTEK_IRQ_MODE, IRQ_SHARE, IRQ_SHARE);
-	sio_write_mask_reg(pdata, FINTEK_IRQ_MODE, IRQ_MODE_MASK,
-			   is_level ? IRQ_LEVEL_LOW : IRQ_EDGE_HIGH);
-}
-
 int fintek_8250_probe(struct uart_8250_port *uart)
 {
 	struct fintek_8250 *pdata;
-- 
1.9.1

Re: [PATCH 4/7] serial: 8250_fintek: Rearrange function

[Ricardo Ribalda Delgado]

On one previous patch you almost rewrote theset_irq_mode function and
added the prototype. It might be a good moment to move the whole
function up.

On Thu, Sep 1, 2016 at 5:39 AM, Ji-Ze Hong (Peter Hong)
<hpeter@gmail.com> wrote:
> We change the position of fintek_8250_set_irq_mode() above the
> find_base_port() to eliminate the prototype define.
>
> Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
> ---
>  drivers/tty/serial/8250/8250_fintek.c | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_fintek.c b/drivers/tty/serial/8250/8250_fintek.c
> index 5625203..921f742 100644
> --- a/drivers/tty/serial/8250/8250_fintek.c
> +++ b/drivers/tty/serial/8250/8250_fintek.c
> @@ -56,9 +56,6 @@ struct fintek_8250 {
>         u8 key;
>  };
>
> -static void fintek_8250_set_irq_mode(struct fintek_8250 *pdata,
> -                                    bool level_mode);
> -
>  static u8 sio_read_reg(struct fintek_8250 *pdata, u8 reg)
>  {
>         outb(reg, pdata->base_port + ADDR_PORT);
> @@ -179,6 +176,14 @@ static void fintek_8250_set_max_fifo(struct fintek_8250 *pdata)
>         }
>  }
>
> +static void fintek_8250_set_irq_mode(struct fintek_8250 *pdata, bool is_level)
> +{
> +       sio_write_reg(pdata, LDN, pdata->index);
> +       sio_write_mask_reg(pdata, FINTEK_IRQ_MODE, IRQ_SHARE, IRQ_SHARE);
> +       sio_write_mask_reg(pdata, FINTEK_IRQ_MODE, IRQ_MODE_MASK,
> +                          is_level ? IRQ_LEVEL_LOW : IRQ_EDGE_HIGH);
> +}
> +
>  static int find_base_port(struct fintek_8250 *pdata, u16 io_address,
>                           unsigned int irq)
>  {
> @@ -230,14 +235,6 @@ static int find_base_port(struct fintek_8250 *pdata, u16 io_address,
>         return -ENODEV;
>  }
>
> -static void fintek_8250_set_irq_mode(struct fintek_8250 *pdata, bool is_level)
> -{
> -       sio_write_reg(pdata, LDN, pdata->index);
> -       sio_write_mask_reg(pdata, FINTEK_IRQ_MODE, IRQ_SHARE, IRQ_SHARE);
> -       sio_write_mask_reg(pdata, FINTEK_IRQ_MODE, IRQ_MODE_MASK,
> -                          is_level ? IRQ_LEVEL_LOW : IRQ_EDGE_HIGH);
> -}
> -
>  int fintek_8250_probe(struct uart_8250_port *uart)
>  {
>         struct fintek_8250 *pdata;
> --
> 1.9.1
>



-- 
Ricardo Ribalda

Re: [PATCH 4/7] serial: 8250_fintek: Rearrange function

[Ji-Ze Hong (Peter Hong)]

Hi Ricardo, Greg,

Ricardo Ribalda Delgado 於 2016/9/1 下午 07:17 寫道:
> On one previous patch you almost rewrote theset_irq_mode function and
> added the prototype. It might be a good moment to move the whole
> function up.
>

I had try with merge the following 2 patches in local,
  1: serial: 8250_fintek: Set IRQ Mode when port probed
  2: serial: 8250_fintek: Rearrange function

but it made the patch less readable. So I'll try to add
set_irq() with prototype first, and change this function location
with other patch.

Could you give me some comments, Greg? Should I merge it?

Thanks
-- 
With Best Regards,
Peter Hong

[PATCH 5/7] serial: 8250_fintek: Add F81216 Support

[Ji-Ze Hong (Peter Hong)]

Fintek F81216 is a LPC to 4 UARTs device. It's the F81216 series but
support less functional than F81216AD/F81216H

The following list is brief descriptions of F81216 series:

F81216H (0105)
	9Bit/High baud rate(not implements with mainline)
	RS485, 128Bytes FIFO (implemented)

F81216AD (0216)
	9Bit(not implements with mainline)
	RS485(implemented)

F81216 (0208)
	basically 16550A

Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
---
 drivers/tty/serial/8250/8250_fintek.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_fintek.c b/drivers/tty/serial/8250/8250_fintek.c
index 921f742..a62cfdd 100644
--- a/drivers/tty/serial/8250/8250_fintek.c
+++ b/drivers/tty/serial/8250/8250_fintek.c
@@ -23,6 +23,7 @@
 #define CHIP_ID2  0x21
 #define CHIP_ID_F81216AD 0x1602
 #define CHIP_ID_F81216H 0x0501
+#define CHIP_ID_F81216 0x0802
 #define VENDOR_ID1 0x23
 #define VENDOR_ID1_VAL 0x19
 #define VENDOR_ID2 0x24
@@ -107,8 +108,14 @@ static int fintek_8250_check_id(struct fintek_8250 *pdata)
 	chip = sio_read_reg(pdata, CHIP_ID1);
 	chip |= sio_read_reg(pdata, CHIP_ID2) << 8;
 
-	if (chip != CHIP_ID_F81216AD && chip != CHIP_ID_F81216H)
+	switch (chip) {
+	case CHIP_ID_F81216AD:
+	case CHIP_ID_F81216H:
+	case CHIP_ID_F81216:
+		break;
+	default:
 		return -ENODEV;
+	}
 
 	pdata->pid = chip;
 	return 0;
@@ -235,6 +242,21 @@ static int find_base_port(struct fintek_8250 *pdata, u16 io_address,
 	return -ENODEV;
 }
 
+static void fintek_8250_set_rs485_handler(struct uart_8250_port *uart)
+{
+	struct fintek_8250 *pdata = uart->port.private_data;
+
+	switch (pdata->pid) {
+	default: /* No RS485 Auto direction functional */
+		break;
+
+	case CHIP_ID_F81216AD:
+	case CHIP_ID_F81216H:
+		uart->port.rs485_config = fintek_8250_rs485_config;
+		break;
+	}
+}
+
 int fintek_8250_probe(struct uart_8250_port *uart)
 {
 	struct fintek_8250 *pdata;
@@ -248,8 +270,8 @@ int fintek_8250_probe(struct uart_8250_port *uart)
 		return -ENOMEM;
 
 	memcpy(pdata, &probe_data, sizeof(probe_data));
-	uart->port.rs485_config = fintek_8250_rs485_config;
 	uart->port.private_data = pdata;
+	fintek_8250_set_rs485_handler(uart);
 
 	return 0;
 }
-- 
1.9.1

Re: [PATCH 5/7] serial: 8250_fintek: Add F81216 Support

[Ricardo Ribalda Delgado]

Hi Peter

On Thu, Sep 1, 2016 at 5:39 AM, Ji-Ze Hong (Peter Hong)
<hpeter@gmail.com> wrote:

>  int fintek_8250_probe(struct uart_8250_port *uart)
>  {
>         struct fintek_8250 *pdata;
> @@ -248,8 +270,8 @@ int fintek_8250_probe(struct uart_8250_port *uart)
>                 return -ENOMEM;
>
>         memcpy(pdata, &probe_data, sizeof(probe_data));
> -       uart->port.rs485_config = fintek_8250_rs485_config;

Maybe just:


if (pdata->id != CHIP_ID_F81216)
   uart->port.rs485_config = fintek_8250_rs485_config;


Instead of the whole function

>         uart->port.private_data = pdata;
> +       fintek_8250_set_rs485_handler(uart);
>
>         return 0;
>  }
> --
> 1.9.1
>



-- 
Ricardo Ribalda

Re: [PATCH 5/7] serial: 8250_fintek: Add F81216 Support

[Ji-Ze Hong (Peter Hong)]

Hi Ricardo,

Ricardo Ribalda Delgado 於 2016/9/1 下午 07:22 寫道:
>>
>>         memcpy(pdata, &probe_data, sizeof(probe_data));
>> -       uart->port.rs485_config = fintek_8250_rs485_config;
>
> Maybe just:
>
>
> if (pdata->id != CHIP_ID_F81216)
>    uart->port.rs485_config = fintek_8250_rs485_config;
>
>
> Instead of the whole function
>
>>         uart->port.private_data = pdata;
>> +       fintek_8250_set_rs485_handler(uart);
>>
>>         return 0;
>>  }

Your comments is good for this patch. But we'll still try to add more 
SuperIO with this. So we'll separate it as a function to control
individually.

-- 
With Best Regards,
Peter Hong

[PATCH 6/7] serial: 8250_fintek: Add F81866 Support

[Ji-Ze Hong (Peter Hong)]

Fintek F81866 is a LPC to 6 UARTs SuperIO. It has fully functional UARTs
likes F81216H. It's also need check the IRQ mode with system assigned,
but the configuration is not the same with F81216 series.

F81866 IRQ Mode setting:
	0xf0
		Bit1: IRQ_MODE0
		Bit0: Share mode (always on)
	0xf6
		Bit3: IRQ_MODE1

	Level/Low: IRQ_MODE0:0, IRQ_MODE1:0
	Edge/High: IRQ_MODE0:1, IRQ_MODE1:0

The following list is brief descriptions of F81866:

F81866 (1010)
	9Bit/High baud rate(not implements with mainline)
	RS485, 128Bytes FIFO (implemented)

Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
---
 drivers/tty/serial/8250/8250_fintek.c | 75 ++++++++++++++++++++++++++++++++---
 1 file changed, 69 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_fintek.c b/drivers/tty/serial/8250/8250_fintek.c
index a62cfdd..1194e57 100644
--- a/drivers/tty/serial/8250/8250_fintek.c
+++ b/drivers/tty/serial/8250/8250_fintek.c
@@ -21,6 +21,7 @@
 #define EXIT_KEY 0xAA
 #define CHIP_ID1  0x20
 #define CHIP_ID2  0x21
+#define CHIP_ID_F81866 0x1010
 #define CHIP_ID_F81216AD 0x1602
 #define CHIP_ID_F81216H 0x0501
 #define CHIP_ID_F81216 0x0802
@@ -50,6 +51,26 @@
 #define RXFTHR_MODE_MASK	(BIT(5) | BIT(4))
 #define RXFTHR_MODE_4X		BIT(5)
 
+#define F81216_LDN_LOW	0x0
+#define F81216_LDN_HIGH	0x4
+
+/*
+ * F81866 registers
+ *
+ * The IRQ setting mode of F81866 is not the same with F81216 series.
+ *	Level/Low: IRQ_MODE0:0, IRQ_MODE1:0
+ *	Edge/High: IRQ_MODE0:1, IRQ_MODE1:0
+ */
+#define F81866_IRQ_MODE		0xf0
+#define F81866_IRQ_SHARE	BIT(0)
+#define F81866_IRQ_MODE0	BIT(1)
+
+#define F81866_FIFO_CTRL	FIFO_CTRL
+#define F81866_IRQ_MODE1	BIT(3)
+
+#define F81866_LDN_LOW		0x10
+#define F81866_LDN_HIGH		0x16
+
 struct fintek_8250 {
 	u16 pid;
 	u16 base_port;
@@ -109,6 +130,7 @@ static int fintek_8250_check_id(struct fintek_8250 *pdata)
 	chip |= sio_read_reg(pdata, CHIP_ID2) << 8;
 
 	switch (chip) {
+	case CHIP_ID_F81866:
 	case CHIP_ID_F81216AD:
 	case CHIP_ID_F81216H:
 	case CHIP_ID_F81216:
@@ -121,6 +143,26 @@ static int fintek_8250_check_id(struct fintek_8250 *pdata)
 	return 0;
 }
 
+static int fintek_8250_get_ldn_range(struct fintek_8250 *pdata, int *min,
+				     int *max)
+{
+	switch (pdata->pid) {
+	case CHIP_ID_F81866:
+		*min = F81866_LDN_LOW;
+		*max = F81866_LDN_HIGH;
+		return 0;
+
+	case CHIP_ID_F81216AD:
+	case CHIP_ID_F81216H:
+	case CHIP_ID_F81216:
+		*min = F81216_LDN_LOW;
+		*max = F81216_LDN_HIGH;
+		return 0;
+	}
+
+	return -ENODEV;
+}
+
 static int fintek_8250_rs485_config(struct uart_port *port,
 			      struct serial_rs485 *rs485)
 {
@@ -175,6 +217,7 @@ static void fintek_8250_set_max_fifo(struct fintek_8250 *pdata)
 	default: /* Default 16Bytes FIFO */
 		return;
 
+	case CHIP_ID_F81866:
 	case CHIP_ID_F81216H: /* 128Bytes FIFO */
 		sio_write_mask_reg(pdata, FIFO_CTRL,
 				   FIFO_MODE_MASK | RXFTHR_MODE_MASK,
@@ -186,9 +229,27 @@ static void fintek_8250_set_max_fifo(struct fintek_8250 *pdata)
 static void fintek_8250_set_irq_mode(struct fintek_8250 *pdata, bool is_level)
 {
 	sio_write_reg(pdata, LDN, pdata->index);
-	sio_write_mask_reg(pdata, FINTEK_IRQ_MODE, IRQ_SHARE, IRQ_SHARE);
-	sio_write_mask_reg(pdata, FINTEK_IRQ_MODE, IRQ_MODE_MASK,
-			   is_level ? IRQ_LEVEL_LOW : IRQ_EDGE_HIGH);
+
+	switch (pdata->pid) {
+	case CHIP_ID_F81866:
+		sio_write_mask_reg(pdata, F81866_FIFO_CTRL, F81866_IRQ_MODE1,
+				   0);
+		sio_write_mask_reg(pdata, F81866_IRQ_MODE, F81866_IRQ_SHARE,
+				   F81866_IRQ_SHARE);
+		sio_write_mask_reg(pdata, F81866_IRQ_MODE,
+				   F81866_IRQ_MODE0,
+				   is_level ? 0 : F81866_IRQ_MODE0);
+		break;
+
+	case CHIP_ID_F81216AD:
+	case CHIP_ID_F81216H:
+	case CHIP_ID_F81216:
+		sio_write_mask_reg(pdata, FINTEK_IRQ_MODE, IRQ_SHARE,
+				   IRQ_SHARE);
+		sio_write_mask_reg(pdata, FINTEK_IRQ_MODE, IRQ_MODE_MASK,
+				   is_level ? IRQ_LEVEL_LOW : IRQ_EDGE_HIGH);
+		break;
+	}
 }
 
 static int find_base_port(struct fintek_8250 *pdata, u16 io_address,
@@ -198,7 +259,7 @@ static int find_base_port(struct fintek_8250 *pdata, u16 io_address,
 	static const u8 keys[] = {0x77, 0xa0, 0x87, 0x67};
 	struct irq_data *irq_data;
 	bool level_mode = false;
-	int i, j, k;
+	int i, j, k, min, max;
 
 	for (i = 0; i < ARRAY_SIZE(addr); i++) {
 		for (j = 0; j < ARRAY_SIZE(keys); j++) {
@@ -207,12 +268,13 @@ static int find_base_port(struct fintek_8250 *pdata, u16 io_address,
 
 			if (fintek_8250_enter_key(addr[i], keys[j]))
 				continue;
-			if (fintek_8250_check_id(pdata)) {
+			if (fintek_8250_check_id(pdata) ||
+			    fintek_8250_get_ldn_range(pdata, &min, &max)) {
 				fintek_8250_exit_key(addr[i]);
 				continue;
 			}
 
-			for (k = 0; k < 4; k++) {
+			for (k = min; k < max; k++) {
 				u16 aux;
 
 				sio_write_reg(pdata, LDN, k);
@@ -250,6 +312,7 @@ static void fintek_8250_set_rs485_handler(struct uart_8250_port *uart)
 	default: /* No RS485 Auto direction functional */
 		break;
 
+	case CHIP_ID_F81866:
 	case CHIP_ID_F81216AD:
 	case CHIP_ID_F81216H:
 		uart->port.rs485_config = fintek_8250_rs485_config;
-- 
1.9.1

[PATCH 7/7] serial: 8250_fintek: Add F81865 Support

[Ji-Ze Hong (Peter Hong)]

Fintek F81865 is a LPC to 6 UARTs SuperIO. It has less functional UARTs
likes F81866. It's also need check the IRQ mode with system assigned,
but the configuration is not the same with F81216 series.

F81865 IRQ Mode setting:
    0xf0
            Bit1: IRQ_MODE0
            Bit0: Share mode (always on)

    Level/Low: IRQ_MODE0:0
    Edge/High: IRQ_MODE0:1

The following list is brief descriptions of F81865:

F81865 (0704)
    9Bit(not implements with mainline)
    RS485(implemented)

Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
---
 drivers/tty/serial/8250/8250_fintek.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_fintek.c b/drivers/tty/serial/8250/8250_fintek.c
index 1194e57..3fb9ab6 100644
--- a/drivers/tty/serial/8250/8250_fintek.c
+++ b/drivers/tty/serial/8250/8250_fintek.c
@@ -21,6 +21,7 @@
 #define EXIT_KEY 0xAA
 #define CHIP_ID1  0x20
 #define CHIP_ID2  0x21
+#define CHIP_ID_F81865 0x0407
 #define CHIP_ID_F81866 0x1010
 #define CHIP_ID_F81216AD 0x1602
 #define CHIP_ID_F81216H 0x0501
@@ -130,6 +131,7 @@ static int fintek_8250_check_id(struct fintek_8250 *pdata)
 	chip |= sio_read_reg(pdata, CHIP_ID2) << 8;
 
 	switch (chip) {
+	case CHIP_ID_F81865:
 	case CHIP_ID_F81866:
 	case CHIP_ID_F81216AD:
 	case CHIP_ID_F81216H:
@@ -147,6 +149,7 @@ static int fintek_8250_get_ldn_range(struct fintek_8250 *pdata, int *min,
 				     int *max)
 {
 	switch (pdata->pid) {
+	case CHIP_ID_F81865:
 	case CHIP_ID_F81866:
 		*min = F81866_LDN_LOW;
 		*max = F81866_LDN_HIGH;
@@ -231,9 +234,12 @@ static void fintek_8250_set_irq_mode(struct fintek_8250 *pdata, bool is_level)
 	sio_write_reg(pdata, LDN, pdata->index);
 
 	switch (pdata->pid) {
+	case CHIP_ID_F81865:
 	case CHIP_ID_F81866:
-		sio_write_mask_reg(pdata, F81866_FIFO_CTRL, F81866_IRQ_MODE1,
-				   0);
+		if (pdata->pid == CHIP_ID_F81866)
+			sio_write_mask_reg(pdata, F81866_FIFO_CTRL,
+					   F81866_IRQ_MODE1, 0);
+
 		sio_write_mask_reg(pdata, F81866_IRQ_MODE, F81866_IRQ_SHARE,
 				   F81866_IRQ_SHARE);
 		sio_write_mask_reg(pdata, F81866_IRQ_MODE,
@@ -312,6 +318,7 @@ static void fintek_8250_set_rs485_handler(struct uart_8250_port *uart)
 	default: /* No RS485 Auto direction functional */
 		break;
 
+	case CHIP_ID_F81865:
 	case CHIP_ID_F81866:
 	case CHIP_ID_F81216AD:
 	case CHIP_ID_F81216H:
-- 
1.9.1

Re: [PATCH 7/7] serial: 8250_fintek: Add F81865 Support

[Ricardo Ribalda Delgado]

Hi Peter



On Thu, Sep 1, 2016 at 5:39 AM, Ji-Ze Hong (Peter Hong)
<hpeter@gmail.com> wrote:

>
>         switch (pdata->pid) {
> +       case CHIP_ID_F81865:
>         case CHIP_ID_F81866:
> -               sio_write_mask_reg(pdata, F81866_FIFO_CTRL, F81866_IRQ_MODE1,
> -                                  0);
> +               if (pdata->pid == CHIP_ID_F81866)
> +                       sio_write_mask_reg(pdata, F81866_FIFO_CTRL,
> +                                          F81866_IRQ_MODE1, 0);
> +
>                 sio_write_mask_reg(pdata, F81866_IRQ_MODE, F81866_IRQ_SHARE,
>                                    F81866_IRQ_SHARE);
>                 sio_write_mask_reg(pdata, F81866_IRQ_MODE,


What about:


         case CHIP_ID_F81866:
               sio_write_mask_reg(pdata, F81866_FIFO_CTRL, F81866_IRQ_MODE1, 0);
                /* fall through */
        case CHIP_ID_F81865:
              sio_write_mask_reg(pdata, F81
.....

> @@ -312,6 +318,7 @@ static void fintek_8250_set_rs485_handler(struct uart_8250_port *uart)
>         default: /* No RS485 Auto direction functional */
>                 break;
>
> +       case CHIP_ID_F81865:
>         case CHIP_ID_F81866:
>         case CHIP_ID_F81216AD:
>         case CHIP_ID_F81216H:
> --
> 1.9.1
>



-- 
Ricardo Ribalda