lkml.org 
[lkml]   [2007]   [Oct]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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 <bryan.wu@analog.com> wrote:

    > Signed-off-by: Bryan Wu <bryan.wu@analog.com>
    > ---
    > 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?


    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2007-10-15 22:37    [W:0.054 / U:59.408 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site