linux-kernel - Re: SM501: core (mfd) driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 13 Dec 2006 18:21:31 +0100
From:	Arjan van de Ven <arjan@...radead.org>
To:	Ben Dooks <ben-fbdev@...ff.org>
Cc:	linux-kernel@...r.kernel.org,
	linux-fbdev-devel@...ts.sourceforge.net
Subject: Re: SM501: core (mfd) driver

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 majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/