> 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(bfin_uart_pin_req_sport0);
> + }
Please use checkpatch. This patch generates a world record number of
warnings.
> +struct uart_ops sport_uart_ops = {
> + .tx_empty = sport_tx_empty,
> + .set_mctrl = sport_set_mctrl,
> + .get_mctrl = sport_get_mctrl,
> + .stop_tx = sport_stop_tx,
> + .start_tx = sport_start_tx,
> + .stop_rx = sport_stop_rx,
> + .enable_ms = sport_enable_ms,
> + .break_ctl = sport_break_ctl,
> + .startup = sport_startup,
> + .shutdown = sport_shutdown,
> + .set_termios = sport_set_termios,
> + .type = sport_type,
> + .release_port = sport_release_port,
> + .request_port = sport_request_port,
> + .config_port = sport_config_port,
> + .verify_port = sport_verify_port,
> +};
Does it need to have global scope?
