Hi,
some review comments below
> +
> +struct sm501_devdata {
> + spinlock_t reg_lock;
> + struct semaphore clock_lock;
can't this be a mutex instead ?
> +#ifdef CONFIG_DEBUG
> +static unsigned int misc_div[] = {
> + [0] = 1,
> + [1] = 2,
can this be const ?
> +
> +int sm501_unit_power(struct device *dev, unsigned int unit, unsigned int to)
> +{
> + struct sm501_devdata *sm = dev_get_drvdata(dev);
> + unsigned long mode = readl(sm->regs + SM501_POWER_MODE_CONTROL);
> + unsigned long gate = readl(sm->regs + SM501_CURRENT_GATE);
> + unsigned long clock = readl(sm->regs + SM501_CURRENT_CLOCK);
> +
> + mode &= 3; /* get current power mode */
> +
> + down(&sm->clock_lock);
eh shouldn't you do the readl()'s inside the semaphore (or mutex) area?
> +
> + writel(mode, sm->regs + SM501_POWER_MODE_CONTROL);
> +
> + dev_dbg(sm->dev, "gate %08lx, clock %08lx, mode %08lx\n",
> + gate, clock, mode);
> +
> + msleep(16);
you're missing a PCI posting flush here
(if you don't know what this is please ask)
> + sm->dev = &dev->dev;
> + sm->irq = dev->irq;
you shouldn't look at dev->irq ...
> +
> + /* set a hopefully unique id for our child platform devices */
> + sm->pdev_id = 32 + dev->devfn;
> +
> + pci_set_drvdata(dev, sm);
> +
> + err = pci_enable_device(dev);
.. before calling pci_enable_device() since pci_enable_device() may be
the one that sets the dev->irq value to it's final value in the first
place
> + sm->io_res = &dev->resource[1];
> + sm->mem_res = &dev->resource[0];
> +
> + sm->regs = ioremap(pci_resource_start(dev, 1),
> + pci_resource_len(dev, 1));
you know how to use pci_resource_start() and co.. why not use them 3
lines higher ? ;)
the driver looks quite clean otherwise btw, great work!
-
if you want to mail me at work (you don't), use arjan (at) linux.intel.com
Test the interaction between Linux and your BIOS via http://www.linuxfirmwarekit.org
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
