Re: [PATCH 3/3] Blackfin serial driver: this driver enable SPORTs on Blackfin emulate UART
On Feb 4, 2008 11:35 AM, Andrew Morton [EMAIL PROTECTED] wrote: My review comments for this patch remain unaddressed so I have put it on hold. -- I forgot to send out the patch. It will be there soon. Thanks -Bryan
Re: [PATCH 3/3] Blackfin serial driver: this driver enable SPORTs on Blackfin emulate UART
My review comments for this patch remain unaddressed so I have put it on hold.
Re: [PATCH 3/3] Blackfin serial driver: this driver enable SPORTs on Blackfin emulate UART
Subject: [PATCH 3/3] Blackfin serial driver: this driver enable SPORTs on
Blackfin emulate UART
That's a bit hard to parse.
On Thu, 11 Oct 2007 18:23:40 +0800
Bryan Wu [EMAIL PROTECTED] wrote:
Signed-off-by: Bryan Wu [EMAIL PROTECTED]
---
drivers/serial/Kconfig | 43 +++
drivers/serial/Makefile |1 +
drivers/serial/bfin_sport_uart.c | 614
++
drivers/serial/bfin_sport_uart.h | 63
include/linux/serial_core.h |2 +
5 files changed, 723 insertions(+), 0 deletions(-)
create mode 100644 drivers/serial/bfin_sport_uart.c
create mode 100644 drivers/serial/bfin_sport_uart.h
diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
index 81b52b7..f3bfbe1 100644
--- a/drivers/serial/Kconfig
+++ b/drivers/serial/Kconfig
@@ -1259,4 +1259,47 @@ config SERIAL_OF_PLATFORM
Currently, only 8250 compatible ports are supported, but
others can easily be added.
+config SERIAL_BFIN_SPORT
+ tristate Blackfin SPORT emulate UART (EXPERIMENTAL)
+ depends on BFIN EXPERIMENTAL
+ select SERIAL_CORE
+ help
+ Enble support SPORT emulate UART on Blackfin series.
There's a typo there. Also the text is pretty meaningless - I'd fix it up
but I'm not sure what it's trying to tell us!
+//#define DEBUG
Probably this shouldn't be here at all - DEBUG is passed in from the gcc
command line.
+#include linux/module.h
+#include linux/ioport.h
+#include linux/init.h
+#include linux/console.h
+#include linux/sysrq.h
+#include linux/platform_device.h
+#include linux/tty.h
+#include linux/tty_flip.h
+#include linux/serial_core.h
+
+#include asm/delay.h
+#include asm/portmux.h
+
+#include bfin_sport_uart.h
+
+unsigned short bfin_uart_pin_req_sport0[] =
+ {P_SPORT0_TFS, P_SPORT0_DTPRI, P_SPORT0_TSCLK, P_SPORT0_RFS, \
+ P_SPORT0_DRPRI, P_SPORT0_RSCLK, P_SPORT0_DRSEC, P_SPORT0_DTSEC, 0};
+
+unsigned short bfin_uart_pin_req_sport1[] =
+ {P_SPORT1_TFS, P_SPORT1_DTPRI, P_SPORT1_TSCLK, P_SPORT1_RFS, \
+ P_SPORT1_DRPRI, P_SPORT1_RSCLK, P_SPORT1_DRSEC, P_SPORT1_DTSEC, 0};
I don't think these need global scope?
...
+/* Reqeust IRQ, Setup clock */
+static int sport_startup(struct uart_port *port)
+{
+ struct sport_uart_port *up = (struct sport_uart_port *)port;
+ char buffer[20];
+ int retval;
+
+ pr_debug(%s enter\n, __FUNCTION__);
+ memset(buffer, 20, '\0');
I don't think this memset is needed.
+ snprintf(buffer, 20, %s rx, up-name);
+ retval = request_irq(up-rx_irq, sport_uart_rx_irq, IRQF_SAMPLE_RANDOM,
buffer, up);
+ if (retval) {
+ printk(KERN_ERR Unable to request interrupt %s\n, buffer);
+ return retval;
+ }
+
+ snprintf(buffer, 20, %s tx, up-name);
+ retval = request_irq(up-tx_irq, sport_uart_tx_irq, IRQF_SAMPLE_RANDOM,
buffer, up);
+ if (retval) {
+ printk(KERN_ERR Unable to request interrupt %s\n, buffer);
+ goto fail1;
+ }
+
+ snprintf(buffer, 20, %s err, up-name);
+ retval = request_irq(up-err_irq, sport_uart_err_irq,
IRQF_SAMPLE_RANDOM, buffer, up);
+ if (retval) {
+ printk(KERN_ERR Unable to request interrupt %s\n, buffer);
+ goto fail2;
+ }
It is not a good idea to create files in /proc which have spaces in their
names. Yes, userspace _should_ be able to cope with that in all cases, but
all software sucks, even including userspace ;)
I'd suggest that we be defensive here, and avoid using spaces in filenames.
+ if (port-line) {
+ if (peripheral_request_list(bfin_uart_pin_req_sport1, DRV_NAME))
+ goto fail3;
+ } else {
+ if (peripheral_request_list(bfin_uart_pin_req_sport0, DRV_NAME))
+ goto fail3;
+ }
+
+ sport_uart_setup(up, get_sclk(), port-uartclk);
+
+ /* Enable receive interrupt */
+ SPORT_PUT_RCR1(up, (SPORT_GET_RCR1(up) | RSPEN));
+ SSYNC();
+
+ return 0;
+
+
+fail3:
+ printk(KERN_ERR DRV_NAME
+ : Requesting Peripherals failed\n);
s/P/p/
+ free_irq(up-err_irq, up);
+fail2:
+ free_irq(up-tx_irq, up);
+fail1:
+ free_irq(up-rx_irq, up);
+
+ return retval;
+
+}
...
+static void sport_shutdown(struct uart_port *port)
+{
+ struct sport_uart_port *up = (struct sport_uart_port *)port;
That's a bit ugly. Perhaps using container_of() would be clearer. it
would also remove the requirement that uart_port be the first member of
sport_uart_port.
+ pr_debug(%s enter\n, __FUNCTION__);
+
+ /* Disable sport */
+ SPORT_PUT_TCR1(up, (SPORT_GET_TCR1(up) ~TSPEN));
+ SPORT_PUT_RCR1(up, (SPORT_GET_RCR1(up) ~RSPEN));
+ SSYNC();
+
+ if (port-line) {
+ peripheral_free_list(bfin_uart_pin_req_sport1);
+ } else {
+ peripheral_free_list
Re: [PATCH 3/3] Blackfin serial driver: this driver enable SPORTs on Blackfin emulate UART
On 10/15/07, Andrew Morton [EMAIL PROTECTED] wrote:
+config SERIAL_BFIN_SPORT
+ tristate Blackfin SPORT emulate UART (EXPERIMENTAL)
+ depends on BFIN EXPERIMENTAL
+ select SERIAL_CORE
+ help
+ Enble support SPORT emulate UART on Blackfin series.
There's a typo there. Also the text is pretty meaningless - I'd fix it up
but I'm not sure what it's trying to tell us!
here it is in english ;)
This driver emulates a standard UART using the SPORT peripherals on a
Blackfin processor.
+ snprintf(buffer, 20, %s rx, up-name);
+ retval = request_irq(up-rx_irq, sport_uart_rx_irq,
IRQF_SAMPLE_RANDOM, buffer, up);
+ if (retval) {
+ printk(KERN_ERR Unable to request interrupt %s\n, buffer);
+ return retval;
+ }
+
+ snprintf(buffer, 20, %s tx, up-name);
+ retval = request_irq(up-tx_irq, sport_uart_tx_irq,
IRQF_SAMPLE_RANDOM, buffer, up);
+ if (retval) {
+ printk(KERN_ERR Unable to request interrupt %s\n, buffer);
+ goto fail1;
+ }
+
+ snprintf(buffer, 20, %s err, up-name);
+ retval = request_irq(up-err_irq, sport_uart_err_irq,
IRQF_SAMPLE_RANDOM, buffer, up);
+ if (retval) {
+ printk(KERN_ERR Unable to request interrupt %s\n, buffer);
+ goto fail2;
+ }
It is not a good idea to create files in /proc which have spaces in their
names. Yes, userspace _should_ be able to cope with that in all cases, but
all software sucks, even including userspace ;)
I'd suggest that we be defensive here, and avoid using spaces in filenames.
i'm not sure i follow ... these are the names given to request_irq()
which means this is what shows up in /proc/interrupts ... does this
function also create an actual file somewhere in /proc that i am not
aware of ?
the rest of the comments look good to me, thanks !
-mike
Re: [PATCH 3/3] Blackfin serial driver: this driver enable SPORTs on Blackfin emulate UART
On 10/11/07, Bryan Wu [EMAIL PROTECTED] wrote: --- a/drivers/serial/Kconfig +++ b/drivers/serial/Kconfig +config SERIAL_BFIN_SPORT + tristate Blackfin SPORT emulate UART (EXPERIMENTAL) + depends on BFIN EXPERIMENTAL + select SERIAL_CORE + help + Enble support SPORT emulate UART on Blackfin series. + + To compile this driver as a module, choose M here: the + module will be called bfin_sport_uart. + +choice + prompt Baud rate for Blackfin SPORT UART + depends on SERIAL_BFIN_SPORT + default SERIAL_SPORT_BAUD_RATE_57600 + help + Choose a baud rate for the SPORT UART, other uart settings are + 8 bit, 1 stop bit, no parity, no flow control. this looks wrong to me ... why cant the standard infrastructure for managing baud rate be used ? -mike
Re: [PATCH 3/3] Blackfin serial driver: this driver enable SPORTs on Blackfin emulate UART
On Mon 15 Oct 2007 16:33, Andrew Morton pondered: Subject: [PATCH 3/3] Blackfin serial driver: this driver enable SPORTs on Blackfin emulate UART That's a bit hard to parse. Blackfin's have a synchronous Serial Peripheral pORT (SPORT). Unlike SPI, UART, I2C, or CAN interfaces which are designed for specific industry standard compatible communication only, the SPORT support a variety (software programmable) serial data communication protocols: - A-law or ยต-law companding according to G.711 specification - Multichannel or Time-Division-Multiplexed (TDM) modes - Stereo Audio I2S Mode - TDM Modes for Multi-Channel audio codecs - H.100 Telephony standard support - others, but if anyone really cares, they need to read the chip specs... Bryan's patch takes the SPORT, and makes a standard UART out of it (exposing /dev/ttySS0) for those people who don't have enough hardware UARTs in their system. Is it a SPORT driver that emulates a UART, or a UART driver on the SPORT? I think it is the latter... Maybe: [PATCH 3/3] Blackfin serial driver: enables a UART interface for the SPORT Which still doesn't make any sense, until you know what a SPORT is :) -Robin
Re: [PATCH 3/3] Blackfin serial driver: this driver enable SPORTs on Blackfin emulate UART
On Mon, 15 Oct 2007 18:03:35 -0400 Mike Frysinger [EMAIL PROTECTED] wrote: It is not a good idea to create files in /proc which have spaces in their names. Yes, userspace _should_ be able to cope with that in all cases, but all software sucks, even including userspace ;) I'd suggest that we be defensive here, and avoid using spaces in filenames. i'm not sure i follow ... these are the names given to request_irq() which means this is what shows up in /proc/interrupts ... does this function also create an actual file somewhere in /proc that i am not aware of ? err, umm, yeah. But the same argument applies: it is imprudent to have space-containing records in /proc/interrupts. However it seems that we've already done that in several places so I guess any /proc/interrupts-parsing programs are already coping with it OK.
