On Wed, Dec 13, 2006 at 06:21:31PM +0100, Arjan van de Ven wrote:
> Hi,
>
> some review comments below
> > +
> > +struct sm501_devdata {
> > + spinlock_t reg_lock;
> > + struct semaphore clock_lock;
>
> can't this be a mutex instead ?
I wasn't sure what context the callers would be when I originally
wrote this code. I will have a careful think of what this will be
used in, and then have a look at changing it.
> > +#ifdef CONFIG_DEBUG
> > +static unsigned int misc_div[] = {
> > + [0] = 1,
> > + [1] = 2,
>
> can this be const ?
Yes, will change.
> > +
> > +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?
Thanks, mistake when fitting locking in, fixed for next release.
> > +
> > + 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)
Is this a read from an device register to cause the PCI writes
to happen? Would reading SM501_POWER_MODE_CONTROL be ok, or does
it require a different register?
> > + 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
Ok, fixed.
> > + 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 ? ;)
These pointers where meant to be kept for setting new resources'
parent pointers, will check what happened.
>
> the driver looks quite clean otherwise btw, great work!
Thanks for the prompt and useful reply.
--
Ben ([email protected], http://www.fluff.org/)
'a smiley only costs 4 bytes'
-
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]
