Skip to contentSkip to conversation

serial: sc16is7xx

PATCH In review [email protected] 5 participants 17 messages Apr 25, 2014

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH v7 1/2 serial: sc16is7xx

From: Jon Ringle <[email protected]>

The SC16IS7xx is a slave I2C-bus/SPI interface to a single-channel high performance UART. The SC16IS7xx's internal register set is backward-compatible with the widely used and widely popular 16C450.

The SC16IS7xx also provides additional advanced features such as auto hardware and software flow control, automatic RS-485 support, and software reset.

---
 drivers/tty/serial/Kconfig       |    9 +
 drivers/tty/serial/Makefile      |    1 +
 drivers/tty/serial/sc16is7xx.c   | 1279 ++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/serial_core.h |    3 +
 4 files changed, 1292 insertions(+)
 create mode 100644 drivers/tty/serial/sc16is7xx.c
drivers/tty/serial/Kconfig +9 -0
index 988fa2b..c3e2b32 100644
@@ -1178,6 +1178,15 @@ config SERIAL_SCCNXP_CONSOLE
 	help
 	  Support for console on SCCNXP serial ports.
 
+config SERIAL_SC16IS7XX
+	tristate "SC16IS7xx serial support"
+	select SERIAL_CORE
+	select REGMAP_I2C if I2C
+	help
+	  This selects support for SC16IS7xx serial ports.
+	  Supported ICs are SC16IS740, SC16IS741, SC16IS750, SC16IS752,
+	  SC16IS760 and SC16IS762.
+
 config SERIAL_BFIN_SPORT
 	tristate "Blackfin SPORT emulate UART"
 	depends on BLACKFIN
drivers/tty/serial/Makefile +1 -0
index 3a5be46..712732b 100644
@@ -51,6 +51,7 @@ obj-$(CONFIG_SERIAL_MPSC) += mpsc.o
 obj-$(CONFIG_SERIAL_SB1250_DUART) += sb1250-duart.o
 obj-$(CONFIG_ETRAX_SERIAL) += crisv10.o
 obj-$(CONFIG_SERIAL_SCCNXP) += sccnxp.o
+obj-$(CONFIG_SERIAL_SC16IS7XX) += sc16is7xx.o
 obj-$(CONFIG_SERIAL_JSM) += jsm/
 obj-$(CONFIG_SERIAL_TXX9) += serial_txx9.o
 obj-$(CONFIG_SERIAL_VR41XX) += vr41xx_siu.o
drivers/tty/serial/sc16is7xx.c new file +1279 -0

1,279 lines not on this page · raw message

include/uapi/linux/serial_core.h +3 -0
index 6e29362..5820269 100644
@@ -241,4 +241,7 @@
 /* MEN 16z135 UART */
 #define PORT_MEN_Z135	107
 
+/* SC16IS74xx */
+#define PORT_SC16IS7XX   108
+
 #endif /* _UAPILINUX_SERIAL_CORE_H */
Signature · 1 line
1.8.5.4
#1 lore raw

PATCH v7 2/2 serial: sc16is7xx: Add bindings documentation for the SC16IS7XX UARTs

From: Jon Ringle <[email protected]>

This patch adds the devicetree documentation for the NXP SC16IS7XX UARTs.

---
 .../devicetree/bindings/serial/nxp,sc16is7xx.txt   | 33 ++++++++++++++++++++++
 1 file changed, 33 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt new file +33 -0
new file mode 100644
index 0000000..246c795
@@ -0,0 +1,33 @@
+* NXP SC16IS7xx advanced Universal Asynchronous Receiver-Transmitter (UART)
+
+Required properties:
+- compatible: Should be one of the following:
+  - "nxp,sc16is740" for NXP SC16IS740,
+  - "nxp,sc16is741" for NXP SC16IS741,
+  - "nxp,sc16is750" for NXP SC16IS750,
+  - "nxp,sc16is752" for NXP SC16IS752,
+  - "nxp,sc16is760" for NXP SC16IS760,
+  - "nxp,sc16is762" for NXP SC16IS762.
+- reg: I2C address of the SC16IS7xx device.
+- interrupt-parent: The phandle for the interrupt controller that
+  services interrupts for this IC.
+- interrupts: Should contain the UART interrupt
+- clocks: Reference to the IC source clock.
+
+Optional properties:
+- gpio-controller: Marks the device node as a GPIO controller.
+- #gpio-cells: Should be two. The first cell is the GPIO number and
+  the second cell is used to specify the GPIO polarity:
+    0 = active high,
+    1 = active low.
+
+Example:
+        sc16is750: sc16is750@51 {
+                compatible = "nxp,sc16is750";
+                reg = <0x51>;
+                clocks = <&clk20m>;
+                interrupt-parent = <&gpio3>;
+                interrupts = <7 IRQ_TYPE_EDGE_FALLING>;
+                gpio-controller;
+                #gpio-cells = <2>;
+        };
Signature · 1 line
1.8.5.4
#2 lore raw
In reply to [email protected]
Quoted 8 lines · On Thu, 24 Apr 2014, [email protected] wrote:
drivers/tty/serial/sc16is7xx.c new file +2 -0
new file mode 100644
index 0000000..ed139f5
 
+
+/* SC16IS7XX register definitions */

drivers/tty/serial/sc16is7xx.c line 41

Quoted 10 lines
drivers/tty/serial/sc16is7xx.c new file +10 -0
+#define SC16IS7XX_RHR_REG		(0x00) /* RX FIFO */
+#define SC16IS7XX_THR_REG		(0x00) /* TX FIFO */
+#define SC16IS7XX_IER_REG		(0x01) /* Interrupt enable */
+#define SC16IS7XX_IIR_REG		(0x02) /* Interrupt Identification */
+#define SC16IS7XX_FCR_REG		(0x02) /* FIFO control */
+#define SC16IS7XX_LCR_REG		(0x03) /* Line Control */
+#define SC16IS7XX_MCR_REG		(0x04) /* Modem Control */
+#define SC16IS7XX_LSR_REG		(0x05) /* Line Status */
+#define SC16IS7XX_MSR_REG		(0x06) /* Modem Status */
+#define SC16IS7XX_SPR_REG		(0x07) /* Scratch Pad */

Isn't this a lot of duplication? We have already in

/*
* include/linux/serial_reg.h
*
#define UART_RX 0 /* In: Receive buffer */
#define UART_TX 0 /* Out: Transmit buffer */
#define UART_IER 1 /* Out: Interrupt Enable Register */
#define UART_IIR 2 /* In: Interrupt ID Register */
#define UART_FCR 2 /* Out: FIFO Control Register */
#define UART_LCR 3 /* Out: Line Control Register */
#define UART_MCR 4 /* Out: Modem Control Register */
#define UART_LSR 5 /* In: Line Status Register */
#define UART_MSR 6 /* In: Modem Status Register */
#define UART_SCR 7 /* I/O: Scratch Register */

and the bit definitions, as well.

#3 lore raw

Fri, 25 Apr 2014 09:21:26 -0400 (EDT) от Charles Coldwell <[email protected]>:

Quoted 37 lines · On Thu, 24 Apr 2014, [email protected] wrote:
Quoted 9 lines
drivers/tty/serial/sc16is7xx.c new file +2 -0
new file mode 100644
index 0000000..ed139f5
 
+
+/* SC16IS7XX register definitions */

drivers/tty/serial/sc16is7xx.c line 41

Quoted 10 lines
drivers/tty/serial/sc16is7xx.c new file +10 -0
+#define SC16IS7XX_RHR_REG		(0x00) /* RX FIFO */
+#define SC16IS7XX_THR_REG		(0x00) /* TX FIFO */
+#define SC16IS7XX_IER_REG		(0x01) /* Interrupt enable */
+#define SC16IS7XX_IIR_REG		(0x02) /* Interrupt Identification */
+#define SC16IS7XX_FCR_REG		(0x02) /* FIFO control */
+#define SC16IS7XX_LCR_REG		(0x03) /* Line Control */
+#define SC16IS7XX_MCR_REG		(0x04) /* Modem Control */
+#define SC16IS7XX_LSR_REG		(0x05) /* Line Status */
+#define SC16IS7XX_MSR_REG		(0x06) /* Modem Status */
+#define SC16IS7XX_SPR_REG		(0x07) /* Scratch Pad */

Isn't this a lot of duplication? We have already in

/*
* include/linux/serial_reg.h
*
#define UART_RX 0 /* In: Receive buffer */
#define UART_TX 0 /* Out: Transmit buffer */
#define UART_IER 1 /* Out: Interrupt Enable Register */
#define UART_IIR 2 /* In: Interrupt ID Register */
#define UART_FCR 2 /* Out: FIFO Control Register */
#define UART_LCR 3 /* Out: Line Control Register */
#define UART_MCR 4 /* Out: Modem Control Register */
#define UART_LSR 5 /* In: Line Status Register */
#define UART_MSR 6 /* In: Modem Status Register */
#define UART_SCR 7 /* I/O: Scratch Register */

Quoted 1 line

and the bit definitions, as well.

And it would be nice to have a small changelog, because it is unclear what has changed with version 6.

---

author
#4 lore raw
Quoted 2 lines · Alexander Shiyan

And it would be nice to have a small changelog, because it is unclear what has changed with version 6.

v7 was created at Greg's request to refresh the patch. The only conflict that required code change was in serial_core.h changing #define PORT_SC16IS7XX 107
to
#define PORT_SC16IS7XX 108

Jon

#5 lore raw
In reply to Charles Coldwell
Quoted 8 lines · Charles Coldwell
Quoted 5 lines · On Thu, 24 Apr 2014, [email protected] wrote:
drivers/tty/serial/sc16is7xx.c new file +0 -0
new file mode 100644
index 0000000..ed139f5
Quoted 14 lines

drivers/tty/serial/sc16is7xx.c line 41

Quoted 12 lines
drivers/tty/serial/sc16is7xx.c new file +12 -0
+
+/* SC16IS7XX register definitions */
+#define SC16IS7XX_RHR_REG		(0x00) /* RX FIFO */
+#define SC16IS7XX_THR_REG		(0x00) /* TX FIFO */
+#define SC16IS7XX_IER_REG		(0x01) /* Interrupt enable */
+#define SC16IS7XX_IIR_REG		(0x02) /* Interrupt Identification */
+#define SC16IS7XX_FCR_REG		(0x02) /* FIFO control */
+#define SC16IS7XX_LCR_REG		(0x03) /* Line Control */
+#define SC16IS7XX_MCR_REG		(0x04) /* Modem Control */
+#define SC16IS7XX_LSR_REG		(0x05) /* Line Status */
+#define SC16IS7XX_MSR_REG		(0x06) /* Modem Status */
+#define SC16IS7XX_SPR_REG		(0x07) /* Scratch Pad */

Isn't this a lot of duplication?

Actually, the whole thing seems like duplication to me. The SC16IS7X0 parts are designed to be 16550-compatible (hence the duplication in the register map), and so I would vote for putting the driver into the drivers/tty/serial/8250 framework. There's really not much difference between these parts and other 16550 parts except that they are reached over the SPI/I2C bus.

Signature · 6 lines
Charles M. Coldwell, W1CMC
"Turn on, log in, tune out"
Belmont, Massachusetts, New England (FN42jj)

GPG ID:  852E052F
GPG FPR: 77E5 2B51 4907 F08A 7E92  DE80 AFA9 9A8F 852E 052F
author
#6 lore raw
Quoted 10 lines · Charles Coldwell
Quoted 8 lines · Charles Coldwell
Quoted 5 lines · On Thu, 24 Apr 2014, [email protected] wrote:
drivers/tty/serial/sc16is7xx.c new file +0 -0
new file mode 100644
index 0000000..ed139f5
Quoted 14 lines
Quoted 6 lines
Quoted 6 lines
drivers/tty/serial/sc16is7xx.c new file +6 -0
+
+/* SC16IS7XX register definitions */
+#define SC16IS7XX_RHR_REG          (0x00) /* RX FIFO */
+#define SC16IS7XX_THR_REG          (0x00) /* TX FIFO */
+#define SC16IS7XX_IER_REG          (0x01) /* Interrupt enable */
+#define SC16IS7XX_IIR_REG          (0x02) /* Interrupt Identification */
Quoted 8 lines

drivers/tty/serial/sc16is7xx.c line 41

Quoted 6 lines
drivers/tty/serial/sc16is7xx.c new file +6 -0
+#define SC16IS7XX_FCR_REG          (0x02) /* FIFO control */
+#define SC16IS7XX_LCR_REG          (0x03) /* Line Control */
+#define SC16IS7XX_MCR_REG          (0x04) /* Modem Control */
+#define SC16IS7XX_LSR_REG          (0x05) /* Line Status */
+#define SC16IS7XX_MSR_REG          (0x06) /* Modem Status */
+#define SC16IS7XX_SPR_REG          (0x07) /* Scratch Pad */

Isn't this a lot of duplication?

I started off doing that, but got frustrated enough by incompatibilities between the register bit definitions that I abandoned that and decided to make the driver self contained regarding the register and bit definitions. For example:

serial_reg.h defines:
#define UART_MCR_TCRTLR 0x40 /* Access TCR/TLR (TI16C752, EFR[4]=1) */

but this is wrong for sc16is7xx. I need:

#define SC16IS7XX_MCR_TCRTLR_BIT (1 << 2) /* TCR/TLR register enable */

Quoted 7 lines

Actually, the whole thing seems like duplication to me. The SC16IS7X0 parts are designed to be 16550-compatible (hence the duplication in the register map), and so I would vote for putting the driver into the drivers/tty/serial/8250 framework. There's really not much difference between these parts and other 16550 parts except that they are reached over the SPI/I2C bus.

The fact that we need to reach over the SPI/I2C bus makes a big difference in the way access is handled.

To achieve acceptable throughput, it is necessary to use threaded irq and also bulk i2c transfers for RX and TX using regmap_raw_{read,write}() to optimize the use of the i2c bus.

This is not a good fit for 8250.

Jon

#7 lore raw
Quoted 9 lines · Jon Ringle
Quoted 8 lines · Charles Coldwell
Quoted 5 lines · Charles Coldwell

drivers/tty/serial/sc16is7xx.c

Quoted 1 line · On Thu, 24 Apr 2014, [email protected] wrote:
drivers/tty/serial/sc16is7xx.c +0 -0

Isn't this a lot of duplication?

Quoted 8 lines
Quoted 1 line

Actually, the whole thing seems like duplication to me.

The fact that we need to reach over the SPI/I2C bus makes a big difference in the way access is handled.

To achieve acceptable throughput, it is necessary to use threaded irq and also bulk i2c transfers for RX and TX using regmap_raw_{read,write}() to optimize the use of the i2c bus.

Fair enough, but the 8250 framework does allow you to insert your own irq service routine. "serial8250_default_handle_irq" is the default (unsurprisingly), but if the uart_port has a non-NULL "handle_irq" method it will be faithfully copied into the uart_8250_port "handle_irq" method in 8250_core.c:early_serial_setup.

Quoted 1 line

This is not a good fit for 8250.

If that's really true, then I would say it argues in favor of a revision of the 8250 code. Certainly, this is not the last time that a 16550-compatible UART will appear on a non-PCI, non-ISA bus.

Signature · 6 lines
Charles M. Coldwell, W1CMC
"Turn on, log in, tune out"
Somerville, Massachusetts, New England (FN42kj)

GPG ID:  852E052F
GPG FPR: 77E5 2B51 4907 F08A 7E92  DE80 AFA9 9A8F 852E 052F
#8 lore raw

Fri, 25 Apr 2014 10:24:30 -0400 (EDT) от Charles Coldwell <[email protected]>:

Quoted 26 lines · Jon Ringle
Quoted 10 lines · Charles Coldwell
Quoted 8 lines · Charles Coldwell
Quoted 6 lines · On Thu, 24 Apr 2014, [email protected] wrote:

drivers/tty/serial/sc16is7xx.c

Quoted 2 lines
drivers/tty/serial/sc16is7xx.c +0 -0

Isn't this a lot of duplication?

Quoted 8 lines
Quoted 1 line

Actually, the whole thing seems like duplication to me.

The fact that we need to reach over the SPI/I2C bus makes a big difference in the way access is handled.

To achieve acceptable throughput, it is necessary to use threaded irq and also bulk i2c transfers for RX and TX using regmap_raw_{read,write}() to optimize the use of the i2c bus.

Fair enough, but the 8250 framework does allow you to insert your own irq service routine. "serial8250_default_handle_irq" is the default (unsurprisingly), but if the uart_port has a non-NULL "handle_irq" method it will be faithfully copied into the uart_8250_port "handle_irq" method in 8250_core.c:early_serial_setup.

Quoted 5 lines
Quoted 1 line

This is not a good fit for 8250.

If that's really true, then I would say it argues in favor of a revision of the 8250 code. Certainly, this is not the last time that a 16550-compatible UART will appear on a non-PCI, non-ISA bus.

At this stage, I would suggest just move the driver code into serial/8250 to indicate the compatibility and then apply this patch and look for ways to optimize similar functions.

---

#9 lore raw
Quoted 4 lines · Alexander Shiyan

At this stage, I would suggest just move the driver code into serial/8250 to indicate the compatibility and then apply this patch and look for ways to optimize similar functions.

I suppose I could live with that. This is really a wonderful UART, and I would like to see it supported in some fashion.

Signature · 6 lines
Charles M. Coldwell, W1CMC
"Turn on, log in, tune out"
Somerville, Massachusetts, New England (FN42kj)

GPG ID:  852E052F
GPG FPR: 77E5 2B51 4907 F08A 7E92  DE80 AFA9 9A8F 852E 052F
author
#10 lore raw
In reply to Charles Coldwell
Quoted 20 lines · Charles Coldwell
Quoted 9 lines · Jon Ringle
Quoted 8 lines · Charles Coldwell
Quoted 5 lines · Charles Coldwell

drivers/tty/serial/sc16is7xx.c

Quoted 1 line · On Thu, 24 Apr 2014, [email protected] wrote:
drivers/tty/serial/sc16is7xx.c +0 -0

Isn't this a lot of duplication?

Quoted 8 lines
Quoted 1 line

Actually, the whole thing seems like duplication to me.

The fact that we need to reach over the SPI/I2C bus makes a big difference in the way access is handled.

To achieve acceptable throughput, it is necessary to use threaded irq and also bulk i2c transfers for RX and TX using regmap_raw_{read,write}() to optimize the use of the i2c bus.

Quoted 5 lines

Fair enough, but the 8250 framework does allow you to insert your own irq service routine. "serial8250_default_handle_irq" is the default (unsurprisingly), but if the uart_port has a non-NULL "handle_irq" method it will be faithfully copied into the uart_8250_port "handle_irq" method in 8250_core.c:early_serial_setup.

However, serial8250_interrupt() which is where port->handle_irq() is called is not a threaded irq.

Quoted 6 lines
Quoted 2 lines

This is not a good fit for 8250.

If that's really true, then I would say it argues in favor of a revision of the 8250 code. Certainly, this is not the last time that a 16550-compatible UART will appear on a non-PCI, non-ISA bus.

I have re-written this driver 4 times already to get to the version that is being presented. I don't have the time or energy to undergo yet another rewrite of this driver. If you'd like to take on this task, be my guest. I'd be happy to test what you come up with on my hardware.

Jon

#11 lore raw

Fri, 25 Apr 2014 10:41:39 -0400 от Jon Ringle <[email protected]>:

Quoted 37 lines · Charles Coldwell
Quoted 20 lines · Jon Ringle
Quoted 10 lines · Charles Coldwell
Quoted 8 lines · Charles Coldwell
Quoted 6 lines · On Thu, 24 Apr 2014, [email protected] wrote:

drivers/tty/serial/sc16is7xx.c

Quoted 2 lines
drivers/tty/serial/sc16is7xx.c +0 -0

Isn't this a lot of duplication?

Quoted 8 lines
Quoted 1 line

Actually, the whole thing seems like duplication to me.

The fact that we need to reach over the SPI/I2C bus makes a big difference in the way access is handled.

To achieve acceptable throughput, it is necessary to use threaded irq and also bulk i2c transfers for RX and TX using regmap_raw_{read,write}() to optimize the use of the i2c bus.

Quoted 5 lines

Fair enough, but the 8250 framework does allow you to insert your own irq service routine. "serial8250_default_handle_irq" is the default (unsurprisingly), but if the uart_port has a non-NULL "handle_irq" method it will be faithfully copied into the uart_8250_port "handle_irq" method in 8250_core.c:early_serial_setup.

However, serial8250_interrupt() which is where port->handle_irq() is called is not a threaded irq.

Quoted 6 lines
Quoted 2 lines

This is not a good fit for 8250.

If that's really true, then I would say it argues in favor of a revision of the 8250 code. Certainly, this is not the last time that a 16550-compatible UART will appear on a non-PCI, non-ISA bus.

Quoted 5 lines

I have re-written this driver 4 times already to get to the version that is being presented. I don't have the time or energy to undergo yet another rewrite of this driver. If you'd like to take on this task, be my guest. I'd be happy to test what you come up with on my hardware.

Jon, do not panic. I helped you to do every version of this driver, although I do not have such hardware, and you already give up.

I just advise you to take the time to create the next version and listen to all the comments for some time.

Look at your current results, you rewrite the driver 4 times, each time it gets better. Is this not an indicator?

---

#12 lore raw
In reply to Charles Coldwell
Quoted 5 lines

Fair enough, but the 8250 framework does allow you to insert your own irq service routine. "serial8250_default_handle_irq" is the default (unsurprisingly), but if the uart_port has a non-NULL "handle_irq" method it will be faithfully copied into the uart_8250_port "handle_irq" method in 8250_core.c:early_serial_setup.

It's nowhere near sufficient and this was discussed earlier in some depth.

Quoted 3 lines

If that's really true, then I would say it argues in favor of a revision of the 8250 code. Certainly, this is not the last time that a 16550-compatible UART will appear on a non-PCI, non-ISA bus.

The 8250 code has deep seated assumptions about locking and atomicity. Not only would it be very hard to unify the drivers the end result would probably be quite horrible to maintain.

We should just merge the driver as is, as submitted in version 7. It's fine. The fact all the I/O accesses are asynchronous completely changes the way the driver is written. Architecturally from the 8250 driver's point of view its a completely unrelated piece of hardware that shares some constants.

If there are other 16x50 i2c/spi uarts then yes merge them with the sc16is7xx driver.

Alan

maintainer
#13 lore raw
In reply to [email protected]
Quoted 10 lines · On Thu, Apr 24, 2014 at 08:56:06PM -0400, [email protected] wrote:

From: Jon Ringle <[email protected]>

The SC16IS7xx is a slave I2C-bus/SPI interface to a single-channel high performance UART. The SC16IS7xx's internal register set is backward-compatible with the widely used and widely popular 16C450.

The SC16IS7xx also provides additional advanced features such as auto hardware and software flow control, automatic RS-485 support, and software reset.

Quoted 8 lines
---
 drivers/tty/serial/Kconfig       |    9 +
 drivers/tty/serial/Makefile      |    1 +
 drivers/tty/serial/sc16is7xx.c   | 1279 ++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/serial_core.h |    3 +
 4 files changed, 1292 insertions(+)
 create mode 100644 drivers/tty/serial/sc16is7xx.c

I get a few build warnings with this patch applied:

drivers/tty/serial/sc16is7xx.c:1059:12: warning: ‘sc16is7xx_probe’ defined but not used [-Wunused-function]
static int sc16is7xx_probe(struct device *dev,

            ^
drivers/tty/serial/sc16is7xx.c:1175:12: warning: ‘sc16is7xx_remove’ defined but not used [-Wunused-function]
 static int sc16is7xx_remove(struct device *dev)
            ^
drivers/tty/serial/sc16is7xx.c:1214:29: warning: ‘regcfg’ defined but not used [-Wunused-variable]
 static struct regmap_config regcfg = {
                             ^

Can you send a follow-on patch to fix them?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#14 lore raw

Fri, 25 Apr 2014 10:26:40 -0700 от Greg KH <[email protected]>:

Quoted 32 lines · On Thu, Apr 24, 2014 at 08:56:06PM -0400, [email protected] wrote:
Quoted 10 lines

From: Jon Ringle <[email protected]>

The SC16IS7xx is a slave I2C-bus/SPI interface to a single-channel high performance UART. The SC16IS7xx's internal register set is backward-compatible with the widely used and widely popular 16C450.

The SC16IS7xx also provides additional advanced features such as auto hardware and software flow control, automatic RS-485 support, and software reset.

Quoted 8 lines
---
 drivers/tty/serial/Kconfig       |    9 +
 drivers/tty/serial/Makefile      |    1 +
 drivers/tty/serial/sc16is7xx.c   | 1279 ++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/serial_core.h |    3 +
 4 files changed, 1292 insertions(+)
 create mode 100644 drivers/tty/serial/sc16is7xx.c

I get a few build warnings with this patch applied:

drivers/tty/serial/sc16is7xx.c:1059:12: warning: ‘sc16is7xx_probe’ defined but not used [-Wunused-function]
static int sc16is7xx_probe(struct device *dev,

            ^
drivers/tty/serial/sc16is7xx.c:1175:12: warning: ‘sc16is7xx_remove’ defined but not used [-Wunused-function]
 static int sc16is7xx_remove(struct device *dev)
            ^
drivers/tty/serial/sc16is7xx.c:1214:29: warning: ‘regcfg’ defined but not used [-Wunused-variable]
 static struct regmap_config regcfg = {
                             ^
Quoted 1 line

Can you send a follow-on patch to fix them?

Seems this just should contain: "depends on I2C" in the Kconfig.

---

���{.n�+�������+%�����ݶ��w��{.n�+����{��G�����{ay�ʇڙ�,j��f���h��������z_�(�階�ݢj"���m������G������?����&���~��iO��z��v�^�m���� ������?�I�

author
#15 lore raw
Quoted 33 lines · Alexander Shiyan

Fri, 25 Apr 2014 10:26:40 -0700 от Greg KH <[email protected]>:

Quoted 32 lines · On Thu, Apr 24, 2014 at 08:56:06PM -0400, [email protected] wrote:
Quoted 10 lines

From: Jon Ringle <[email protected]>

The SC16IS7xx is a slave I2C-bus/SPI interface to a single-channel high performance UART. The SC16IS7xx's internal register set is backward-compatible with the widely used and widely popular 16C450.

The SC16IS7xx also provides additional advanced features such as auto hardware and software flow control, automatic RS-485 support, and software reset.

Quoted 8 lines
---
 drivers/tty/serial/Kconfig       |    9 +
 drivers/tty/serial/Makefile      |    1 +
 drivers/tty/serial/sc16is7xx.c   | 1279 ++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/serial_core.h |    3 +
 4 files changed, 1292 insertions(+)
 create mode 100644 drivers/tty/serial/sc16is7xx.c

I get a few build warnings with this patch applied:

drivers/tty/serial/sc16is7xx.c:1059:12: warning: ‘sc16is7xx_probe’ defined but not used [-Wunused-function]
static int sc16is7xx_probe(struct device *dev,

            ^
drivers/tty/serial/sc16is7xx.c:1175:12: warning: ‘sc16is7xx_remove’ defined but not used [-Wunused-function]
 static int sc16is7xx_remove(struct device *dev)
            ^
drivers/tty/serial/sc16is7xx.c:1214:29: warning: ‘regcfg’ defined but not used [-Wunused-variable]
 static struct regmap_config regcfg = {
                             ^
Quoted 3 lines
Quoted 1 line

Can you send a follow-on patch to fix them?

Seems this just should contain: "depends on I2C" in the Kconfig.

I can add "depends on I2C" in Kconfig for now, but I'm just wondering how would this be dealt with when sometime in the future this driver supports either I2C or SPI? Can the "depends on" be conditional somehow?

Jon

#16 lore raw

Fri, 25 Apr 2014 14:24:23 -0400 от Jon Ringle <[email protected]>:

Quoted 34 lines · Alexander Shiyan
Quoted 33 lines

Fri, 25 Apr 2014 10:26:40 -0700 от Greg KH <[email protected]>:

Quoted 32 lines · On Thu, Apr 24, 2014 at 08:56:06PM -0400, [email protected] wrote:
Quoted 10 lines

From: Jon Ringle <[email protected]>

The SC16IS7xx is a slave I2C-bus/SPI interface to a single-channel high performance UART. The SC16IS7xx's internal register set is backward-compatible with the widely used and widely popular 16C450.

The SC16IS7xx also provides additional advanced features such as auto hardware and software flow control, automatic RS-485 support, and software reset.

Quoted 8 lines
---
 drivers/tty/serial/Kconfig       |    9 +
 drivers/tty/serial/Makefile      |    1 +
 drivers/tty/serial/sc16is7xx.c   | 1279 ++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/serial_core.h |    3 +
 4 files changed, 1292 insertions(+)
 create mode 100644 drivers/tty/serial/sc16is7xx.c

I get a few build warnings with this patch applied:

drivers/tty/serial/sc16is7xx.c:1059:12: warning: ‘sc16is7xx_probe’ defined but not used [-Wunused-function]
static int sc16is7xx_probe(struct device *dev,

            ^
drivers/tty/serial/sc16is7xx.c:1175:12: warning: ‘sc16is7xx_remove’ defined but not used [-Wunused-function]
 static int sc16is7xx_remove(struct device *dev)
            ^
drivers/tty/serial/sc16is7xx.c:1214:29: warning: ‘regcfg’ defined but not used [-Wunused-variable]
 static struct regmap_config regcfg = {
                             ^
Quoted 8 lines
Quoted 3 lines
Quoted 1 line

Can you send a follow-on patch to fix them?

Seems this just should contain: "depends on I2C" in the Kconfig.

I can add "depends on I2C" in Kconfig for now, but I'm just wondering how would this be dealt with when sometime in the future this driver supports either I2C or SPI? Can the "depends on" be conditional somehow?

If anyone adds SPI support later, this can be represented as:
depends on I2C || SPI_MASTER
select REGMAP_I2C if I2C
select REGMAP_SPI if SPI_MASTER

---

���{.n�+�������+%�����ݶ��w��{.n�+����{��G�����{ay�ʇڙ�,j��f���h��������z_�(�階�ݢj"���m������G������?����&���~��iO��z��v�^�m���� ������?�I�