lkml.org 
[lkml]   [2006]   [Dec]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: 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/

\
 
 \ /
  Last update: 2006-12-13 18:55    [W:0.049 / U:0.984 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site