On Sat, Sep 25, 2010 at 03:05, <[email protected]> wrote:
> Modified: trunk/drivers/char/bfin_sport.c (9162 => 9163)
>
> +static int sport_mode;
> +static int sport_clkdiv;
> +static int irq_notfreed;
you cannot declare any global state in this driver. this driver
supports multiple instances which global state prevents. use the
already existing config structure.
> @@ -282,12 +308,10 @@
> /* Wait for the last byte sent out */
> udelay(500);
> pr_debug("%s status:%x\n", __func__, status);
> -
> dev->regs->tcr1 &= ~TSPEN;
> SSYNC();
> enable_irq(dev->err_irq);
> disable_dma(dev->dma_tx_chan);
> -
> complete(&dev->c);
>
> /* Clear the interrupt status */
please refrain from mixing whitespace changes with functional commits.
imo, this change isnt even wanted as the old spacing was more
readable.
> +static inline void sport_ndso_rx_read(struct sport_dev *dev)
> +static inline void sport_ndso_tx_write(struct sport_dev *dev)
i would refrain from forcing inline on these functions. although, i
see this is an already existing bug in the code we should fix.
> + void *buf;
> ...
> + buf = (void *)dev->rx_buf + dev->rx_received;
> + *(u8 *)buf = dev->regs->rx16;
> ...
> + buf = (void *)dev->rx_buf + dev->rx_received;
> + *(u16 *)buf = dev->regs->rx16;
casting LHS arguments to pointers and then dereferencing them directly
is ugly and in general, forbidden. just look at how the already
existing read/write functions do it in this file.
> +{
> + struct sport_config *cfg = &dev->config;
> + void *buf;
> + int i, dummy;
> +
> + dev->regs->tcr1 |= TSPEN;
> + dev->regs->rcr1 |= RSPEN;
> + for (i = 0; i < 50; i++) {
where is this random "50" coming from ? the driver should exist to
just move data that userspace gives it to the hardware. i cant figure
out why this for loop exists at all.
> @@ -368,9 +455,10 @@
> {
> struct sport_dev *dev = dev_id;
>
> - if (dev->tx_sent < dev->tx_len)
> - sport_tx_write(dev);
> -
> + if (sport_mode != NDSO_MODE) {
> + if (dev->tx_sent < dev->tx_len)
> + sport_tx_write(dev);
> + }
> if (dev->tx_len != 0 && dev->tx_sent >= dev->tx_len
there needs to be a space after your new if statement block and before
the next one like there used to be
> @@ -671,7 +783,7 @@
> {
> struct sport_dev *dev = filp->private_data;
> struct sport_config config;
> -
> + unsigned long value;
> pr_debug("%s: enter, arg:0x%lx\n", __func__, arg);
please retrain the existing style. it should be the local decls, then
a blank line, then the code.
> @@ -681,6 +793,17 @@
> return -EFAULT;
> break;
>
> + case SPORT_IOC_GET_SYSTEMCLOCK:
> + value = get_sclk();
> + copy_to_user((unsigned long *)arg, &value, sizeof(unsigned
> long)) ?
> -EFAULT : 0;
this tertiary is useless as you dont actually set/return EFAULT/0.
fix it so that it does return the right value.
-mike
_______________________________________________
Linux-kernel-commits mailing list
[email protected]
https://blackfin.uclinux.org/mailman/listinfo/linux-kernel-commits
