Messages in this thread | | | Date | Wed, 13 Dec 2006 17:51:43 +0000 | From | Ben Dooks <> | Subject | Re: SM501: core (mfd) driver |
| |
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 (ben@fluff.org, 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |