Re: [PATCH] [net/irda]: new Blackfin on-chip SIR IrDA driver

[Mike Frysinger <vapier.adi@gmail.com>]

On Tue, Mar 10, 2009 at 03:29,  <graff.yang@gmail.com> wrote:
> +config BFIN_SIR3
> +       bool "Blackfin SIR on UART3"
> +config BFIN_SIR1
> +       bool "Blackfin SIR on UART1"
> +config BFIN_SIR0
> +       bool "Blackfin SIR on UART0"
> +config BFIN_SIR2
> +       bool "Blackfin SIR on UART2"

looks like an odd order for things.  or maybe you count to 3
differently from me :).

> +static int bfin_sir_set_speed(struct bfin_sir_port *port, int speed)
> +{
> +       int ret = -EINVAL;
> +       unsigned int quot;
> +       unsigned short val, lsr, lcr = 0;
> +
> +       lcr = WLS(8);

the lcr init to 0 looks pretty pointless to me ...

> +               quot = (port->clk + (8 * speed)) / (16 * speed);

isnt the SIR affected by the same anomalies as the UART ?  in other
words, you just made that adjustment to the UART recently ...

> +               do {
> +                       lsr = SIR_UART_GET_LSR(port);
> +               } while (!(lsr & TEMT));

i'm pretty sure we determined that it is not the job of the kernel to
make sure the line is clear before we go changing speeds.  plus, we
had a few bugs on the UART driver when polling for bits on a
peripheral that wasnt enabled yet ...

> +               SIR_UART_PUT_DLL(port, quot & 0xFF);
> +               SSYNC();
> +               SIR_UART_PUT_DLH(port, (quot >> 8) & 0xFF);
> +               SSYNC();

i'm pretty sure that first SSYNC is not needed

> +               /*printk(KERN_DEBUG "bfin_sir: Set new speed %d\n", speed);*/

pr_debug() ?

> +static irqreturn_t bfin_sir_dma_tx_int(int irq, void *dev_id)
> +{
> +       struct net_device *dev = dev_id;
> +       struct bfin_sir_self *self = netdev_priv(dev);
> +       struct bfin_sir_port *port = self->sir_port;
> +
> +       spin_lock(&self->lock);
> +       if (!(get_dma_curr_irqstat(port->tx_dma_channel)&DMA_RUN)) {

really should be whitespace around that &

> +static irqreturn_t bfin_sir_dma_rx_int(int irq, void *dev_id)
> +{
> +....
> +       mod_timer(&(port->rx_dma_timer), jiffies + DMA_SIR_RX_FLUSH_JIFS);

those paren are unnecessary

> +static int bfin_sir_startup(struct bfin_sir_port *port, struct net_device *dev)
> +{
> +#ifdef CONFIG_SIR_BFIN_DMA
> +       dma_addr_t dma_handle;
> +#endif /* CONFIG_SIR_BFIN_DMA */
> +
> +       if (request_dma(port->rx_dma_channel, "BFIN_UART_RX") < 0) {
> +               printk(KERN_WARNING "bfin_sir: Unable to attach SIR RX DMA channel\n");

should be dev_warn(&dev->dev, ...) ?  if so, i'd check all the places
where printk() is used when a net_device is available ...

> +static int __devinit bfin_sir_probe(struct platform_device *pdev)
> +{
> +       struct net_device *dev;
> +       struct bfin_sir_self *self;
> +       unsigned int baudrate_mask;
> +       struct bfin_sir_port *sir_port;
> +       int err = 0;
> +
> +       err = peripheral_request(per[pdev->id][0], DRIVER_NAME);
> +       if (err)
> +               return err;
> +       err = peripheral_request(per[pdev->id][1], DRIVER_NAME);
> +       if (err)
> +               return err;

the first per list was just leaked ...

> +       sir_port = kmalloc(sizeof(struct bfin_sir_port), GFP_KERNEL);

sizeof(*sir_port)

> +       dev = alloc_irdadev(sizeof(struct bfin_sir_self));

sizeof(*dev)

> +       baudrate_mask = IR_9600;
> +
> +       switch (max_rate) {
> +       case 115200:
> +               baudrate_mask |= IR_115200;
> +       case 57600:
> +               baudrate_mask |= IR_57600;
> +       case 38400:
> +               baudrate_mask |= IR_38400;
> +       case 19200:
> +               baudrate_mask |= IR_19200;
> +       }

what if someone specified max_rate = 1231245 ?

> +       if (err) {
> +err_mem_3:
> +               kfree(self->tx_buff.head);
> +err_mem_2:
> +               kfree(self->rx_buff.head);
> +err_mem_1:
> +               free_netdev(dev);
> +err_mem_0:
> +               kfree(sir_port);
> +       }

the peripheral pins are leaked here as well

> +       if (err == 0)
> +               platform_set_drvdata(pdev, sir_port);

considering you tested "if (err)" right before, an "} else" would make
more sense

> +MODULE_AUTHOR("Graf.Yang <graf.yang@analog.com>");

i think your name has no "." ? :)

> --- /dev/null
> +++ b/drivers/net/irda/bfin_sir.h
> +#ifdef CONFIG_SIR_BFIN_DMA
> +struct dma_rx_buf {
> +       char *buf;
> +       int head;
> +       int tail;
> +       };

no indentation in closing brace

> +static unsigned short per[][2] = {
> +       {P_UART0_RX, P_UART0_TX},
> +       {P_UART1_RX, P_UART1_TX},
> +       {P_UART2_RX, P_UART2_TX},
> +       {P_UART3_RX, P_UART3_TX},
> +       };

no indentation in closing brace and really this should be const
-mike