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

[Mike Frysinger]

On Tue, Mar 10, 2009 at 03:29,  <graff.yang@xxxxxxxxx> 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@xxxxxxxxxx>");

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