[PATCH 1/3] Misc: phantom, synchronize_irq() on suspend

[PATCH 1/3] Misc: phantom, synchronize_irq() on suspend

[Jiri Slaby]

phantom, synchronize_irq() on suspend

Wait after disabling device's interrupt until the handler finishes its
work if still in progress.

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

---
commit 7e792ef384190b517f2fb27cd0237fa30dbe0775
tree 17b15e5ab7c90eef0e7ae57e532839e81b831d58
parent 5c008a5651ee92ebe020dd5108a66a7db74fe41d
author Jiri Slaby <jirislaby@gmail.com> Mon, 15 Oct 2007 15:52:21 +0200
committer Jiri Slaby <jirislaby@gmail.com> Mon, 15 Oct 2007 15:52:21 +0200

 drivers/misc/phantom.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/misc/phantom.c b/drivers/misc/phantom.c
index 5108b7c..6e61a79 100644
--- a/drivers/misc/phantom.c
+++ b/drivers/misc/phantom.c
@@ -378,6 +378,8 @@ static int phantom_suspend(struct pci_dev *pdev, pm_message_t state)
 	iowrite32(0, dev->caddr + PHN_IRQCTL);
 	ioread32(dev->caddr + PHN_IRQCTL); /* PCI posting */
 
+	synchronize_irq(pdev->irq);
+
 	return 0;
 }
 

[PATCH 2/3] Misc: phantom, add comment about openhaptics

[Jiri Slaby]

phantom, add comment about openhaptics

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

---
commit 931e139b87d4fdb9fcae6a39cc0a157ff2fc07e1
tree f0f98cff767ed04a2289a5c8ebbcad0d5757b23b
parent 7e792ef384190b517f2fb27cd0237fa30dbe0775
author Jiri Slaby <jirislaby@gmail.com> Mon, 15 Oct 2007 15:53:06 +0200
committer Jiri Slaby <jirislaby@gmail.com> Mon, 15 Oct 2007 15:53:06 +0200

 drivers/misc/phantom.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/misc/phantom.c b/drivers/misc/phantom.c
index 6e61a79..bc532ac 100644
--- a/drivers/misc/phantom.c
+++ b/drivers/misc/phantom.c
@@ -9,6 +9,7 @@
  *  You need an userspace library to cooperate with this driver. It (and other
  *  info) may be obtained here:
  *  http://www.fi.muni.cz/~xslaby/phantom.html
+ *  or alternatively, you might use OpenHaptics provided by Sensable.
  */
 
 #include <linux/kernel.h>

[PATCH 3/3] Misc: phantom, improved data passing

[Jiri Slaby]

phantom, improved data passing

this new version guarantees amb_bit switch in small enough intervals, so
that the device won't stop working in the middle of a movement anymore.
However it preserves old (openhaptics) functionality.

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

---
commit e820ffda8d5f6c8a1d463955f6df7cf18544ca92
tree bba23481215cfcb6f3b7d6fec1d57d6902dc1098
parent 931e139b87d4fdb9fcae6a39cc0a157ff2fc07e1
author Jiri Slaby <jirislaby@gmail.com> Mon, 15 Oct 2007 16:03:38 +0200
committer Jiri Slaby <jirislaby@gmail.com> Mon, 15 Oct 2007 16:03:38 +0200

 drivers/misc/phantom.c  |   79 +++++++++++++++++++++++++++++++++++++----------
 include/linux/phantom.h |    6 +++-
 2 files changed, 68 insertions(+), 17 deletions(-)

diff --git a/drivers/misc/phantom.c b/drivers/misc/phantom.c
index bc532ac..4a610cc 100644
--- a/drivers/misc/phantom.c
+++ b/drivers/misc/phantom.c
@@ -25,13 +25,14 @@
 #include <asm/atomic.h>
 #include <asm/io.h>
 
-#define PHANTOM_VERSION		"n0.9.5"
+#define PHANTOM_VERSION		"n0.9.7"
 
 #define PHANTOM_MAX_MINORS	8
 
 #define PHN_IRQCTL		0x4c    /* irq control in caddr space */
 
 #define PHB_RUNNING		1
+#define PHB_NOT_OH		2
 
 static struct class *phantom_class;
 static int phantom_major;
@@ -48,7 +49,11 @@ struct phantom_device {
 	struct cdev cdev;
 
 	struct mutex open_lock;
-	spinlock_t ioctl_lock;
+	spinlock_t regs_lock;
+
+	/* used in NOT_OH mode */
+	struct phm_regs oregs;
+	u32 ctl_reg;
 };
 
 static unsigned char phantom_devices[PHANTOM_MAX_MINORS];
@@ -83,6 +88,7 @@ static long phantom_ioctl(struct file *file, unsigned int cmd,
 	struct phm_regs rs;
 	struct phm_reg r;
 	void __user *argp = (void __user *)arg;
+	unsigned long flags;
 	unsigned int i;
 
 	if (_IOC_TYPE(cmd) != PH_IOC_MAGIC ||
@@ -97,32 +103,42 @@ static long phantom_ioctl(struct file *file, unsigned int cmd,
 		if (r.reg > 7)
 			return -EINVAL;
 
-		spin_lock(&dev->ioctl_lock);
+		spin_lock_irqsave(&dev->regs_lock, flags);
 		if (r.reg == PHN_CONTROL && (r.value & PHN_CTL_IRQ) &&
 				phantom_status(dev, dev->status | PHB_RUNNING)){
-			spin_unlock(&dev->ioctl_lock);
+			spin_unlock_irqrestore(&dev->regs_lock, flags);
 			return -ENODEV;
 		}
 
 		pr_debug("phantom: writing %x to %u\n", r.value, r.reg);
+
+		/* preserve amp bit (don't allow to change it when in NOT_OH) */
+		if (r.reg == PHN_CONTROL && (dev->status & PHB_NOT_OH))
+			dev->ctl_reg = r.value = (r.value & ~PHN_CTL_AMP) |
+				(dev->ctl_reg & PHN_CTL_AMP);
+
 		iowrite32(r.value, dev->iaddr + r.reg);
 		ioread32(dev->iaddr); /* PCI posting */
 
 		if (r.reg == PHN_CONTROL && !(r.value & PHN_CTL_IRQ))
 			phantom_status(dev, dev->status & ~PHB_RUNNING);
-		spin_unlock(&dev->ioctl_lock);
+		spin_unlock_irqrestore(&dev->regs_lock, flags);
 		break;
 	case PHN_SET_REGS:
 		if (copy_from_user(&rs, argp, sizeof(rs)))
 			return -EFAULT;
 
 		pr_debug("phantom: SRS %u regs %x\n", rs.count, rs.mask);
-		spin_lock(&dev->ioctl_lock);
-		for (i = 0; i < min(rs.count, 8U); i++)
-			if ((1 << i) & rs.mask)
-				iowrite32(rs.values[i], dev->oaddr + i);
-		ioread32(dev->iaddr); /* PCI posting */
-		spin_unlock(&dev->ioctl_lock);
+		spin_lock_irqsave(&dev->regs_lock, flags);
+		if (dev->status & PHB_NOT_OH)
+			memcpy(&dev->oregs, &rs, sizeof(rs));
+		else {
+			for (i = 0; i < min(rs.count, 8U); i++)
+				if (rs.mask & BIT(i))
+					iowrite32(rs.values[i], dev->oaddr + i);
+			ioread32(dev->iaddr); /* PCI posting */
+		}
+		spin_unlock_irqrestore(&dev->regs_lock, flags);
 		break;
 	case PHN_GET_REG:
 		if (copy_from_user(&r, argp, sizeof(r)))
@@ -141,15 +157,26 @@ static long phantom_ioctl(struct file *file, unsigned int cmd,
 			return -EFAULT;
 
 		pr_debug("phantom: GRS %u regs %x\n", rs.count, rs.mask);
-		spin_lock(&dev->ioctl_lock);
+		spin_lock_irqsave(&dev->regs_lock, flags);
 		for (i = 0; i < min(rs.count, 8U); i++)
-			if ((1 << i) & rs.mask)
+			if (rs.mask & BIT(i))
 				rs.values[i] = ioread32(dev->iaddr + i);
-		spin_unlock(&dev->ioctl_lock);
+		spin_unlock_irqrestore(&dev->regs_lock, flags);
 
 		if (copy_to_user(argp, &rs, sizeof(rs)))
 			return -EFAULT;
 		break;
+	case PHN_NOT_OH:
+		spin_lock_irqsave(&dev->regs_lock, flags);
+		if (dev->status & PHB_RUNNING) {
+			printk(KERN_ERR "phantom: you need to set NOT_OH "
+					"before you start the device!\n");
+			spin_unlock_irqrestore(&dev->regs_lock, flags);
+			return -EINVAL;
+		}
+		dev->status |= PHB_NOT_OH;
+		spin_unlock_irqrestore(&dev->regs_lock, flags);
+		break;
 	default:
 		return -ENOTTY;
 	}
@@ -172,8 +199,11 @@ static int phantom_open(struct inode *inode, struct file *file)
 		return -EINVAL;
 	}
 
+	WARN_ON(dev->status & PHB_NOT_OH);
+
 	file->private_data = dev;
 
+	atomic_set(&dev->counter, 0);
 	dev->opened++;
 	mutex_unlock(&dev->open_lock);
 
@@ -188,6 +218,7 @@ static int phantom_release(struct inode *inode, struct file *file)
 
 	dev->opened = 0;
 	phantom_status(dev, dev->status & ~PHB_RUNNING);
+	dev->status &= ~PHB_NOT_OH;
 
 	mutex_unlock(&dev->open_lock);
 
@@ -221,12 +252,28 @@ static struct file_operations phantom_file_ops = {
 static irqreturn_t phantom_isr(int irq, void *data)
 {
 	struct phantom_device *dev = data;
+	unsigned int i;
+	u32 ctl;
 
-	if (!(ioread32(dev->iaddr + PHN_CONTROL) & PHN_CTL_IRQ))
+	spin_lock(&dev->regs_lock);
+	ctl = ioread32(dev->iaddr + PHN_CONTROL);
+	if (!(ctl & PHN_CTL_IRQ)) {
+		spin_unlock(&dev->regs_lock);
 		return IRQ_NONE;
+	}
 
 	iowrite32(0, dev->iaddr);
 	iowrite32(0xc0, dev->iaddr);
+
+	if (dev->status & PHB_NOT_OH)
+		for (i = 0; i < min(dev->oregs.count, 8U); i++)
+			if (dev->oregs.mask & BIT(i))
+				iowrite32(dev->oregs.values[i], dev->oaddr + i);
+
+	dev->ctl_reg ^= PHN_CTL_AMP;
+	iowrite32(dev->ctl_reg, dev->iaddr + PHN_CONTROL);
+	spin_unlock(&dev->regs_lock);
+
 	ioread32(dev->iaddr); /* PCI posting */
 
 	atomic_inc(&dev->counter);
@@ -298,7 +345,7 @@ static int __devinit phantom_probe(struct pci_dev *pdev,
 	}
 
 	mutex_init(&pht->open_lock);
-	spin_lock_init(&pht->ioctl_lock);
+	spin_lock_init(&pht->regs_lock);
 	init_waitqueue_head(&pht->wait);
 	cdev_init(&pht->cdev, &phantom_file_ops);
 	pht->cdev.owner = THIS_MODULE;
diff --git a/include/linux/phantom.h b/include/linux/phantom.h
index d3ebbfa..96f4048 100644
--- a/include/linux/phantom.h
+++ b/include/linux/phantom.h
@@ -30,7 +30,11 @@ struct phm_regs {
 #define PHN_SET_REG		_IOW (PH_IOC_MAGIC, 1, struct phm_reg *)
 #define PHN_GET_REGS		_IOWR(PH_IOC_MAGIC, 2, struct phm_regs *)
 #define PHN_SET_REGS		_IOW (PH_IOC_MAGIC, 3, struct phm_regs *)
-#define PH_IOC_MAXNR		3
+/* this ioctl tells the driver, that the caller is not OpenHaptics and might
+ * use improved registers update (no more phantom switchoffs when using
+ * libphantom) */
+#define PHN_NOT_OH		_IO  (PH_IOC_MAGIC, 4)
+#define PH_IOC_MAXNR		4
 
 #define PHN_CONTROL		0x6     /* control byte in iaddr space */
 #define PHN_CTL_AMP		0x1     /*   switch after torques change */

Re: [PATCH 1/3] Misc: phantom, synchronize_irq() on suspend

[Andrew Morton]

On Mon, 15 Oct 2007 09:32:28 -0700
Jiri Slaby <jirislaby@gmail.com> wrote:

> phantom, synchronize_irq() on suspend
> 
> Wait after disabling device's interrupt until the handler finishes its
> work if still in progress.
> 
> Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
> 
> ---
> commit 7e792ef384190b517f2fb27cd0237fa30dbe0775
> tree 17b15e5ab7c90eef0e7ae57e532839e81b831d58
> parent 5c008a5651ee92ebe020dd5108a66a7db74fe41d
> author Jiri Slaby <jirislaby@gmail.com> Mon, 15 Oct 2007 15:52:21 +0200
> committer Jiri Slaby <jirislaby@gmail.com> Mon, 15 Oct 2007 15:52:21 +0200
> 
>  drivers/misc/phantom.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/misc/phantom.c b/drivers/misc/phantom.c
> index 5108b7c..6e61a79 100644
> --- a/drivers/misc/phantom.c
> +++ b/drivers/misc/phantom.c
> @@ -378,6 +378,8 @@ static int phantom_suspend(struct pci_dev *pdev, pm_message_t state)
>  	iowrite32(0, dev->caddr + PHN_IRQCTL);
>  	ioread32(dev->caddr + PHN_IRQCTL); /* PCI posting */
>  
> +	synchronize_irq(pdev->irq);
> +
>  	return 0;
>  }
>  

What inspired this change?  Some bug report, or does it just seem the right
thing to do?

Would it be logical to do this operation from the PCI core somewhere, on
behalf of all PCI drivers?

Re: [PATCH 3/3] Misc: phantom, improved data passing

[Andrew Morton]

On Mon, 15 Oct 2007 09:33:49 -0700
Jiri Slaby <jirislaby@gmail.com> wrote:

> phantom, improved data passing
> 
> this new version guarantees amb_bit switch in small enough intervals, so
> that the device won't stop working in the middle of a movement anymore.
> However it preserves old (openhaptics) functionality.
> 
> ...
>
> +
> +		/* preserve amp bit (don't allow to change it when in NOT_OH) */
> +		if (r.reg == PHN_CONTROL && (dev->status & PHB_NOT_OH))
> +			dev->ctl_reg = r.value = (r.value & ~PHN_CTL_AMP) |
> +				(dev->ctl_reg & PHN_CTL_AMP);

hm, is that coded as clearly and as simply as possible?  Don't think so :(


> +		spin_lock_irqsave(&dev->regs_lock, flags);
> +		if (dev->status & PHB_NOT_OH)
> +			memcpy(&dev->oregs, &rs, sizeof(rs));
> +		else {
> +			for (i = 0; i < min(rs.count, 8U); i++)

We evaluate min() each time around the loop.  Doesn't matter much here I guess.

> +				if (rs.mask & BIT(i))
> +					iowrite32(rs.values[i], dev->oaddr + i);
> +			ioread32(dev->iaddr); /* PCI posting */
> +		}
> +		spin_unlock_irqrestore(&dev->regs_lock, flags);
>  		break;
>  	case PHN_GET_REG:
>  		if (copy_from_user(&r, argp, sizeof(r)))
>
> ...
>
>  static irqreturn_t phantom_isr(int irq, void *data)
>  {
>  	struct phantom_device *dev = data;
> +	unsigned int i;
> +	u32 ctl;
>  
> -	if (!(ioread32(dev->iaddr + PHN_CONTROL) & PHN_CTL_IRQ))
> +	spin_lock(&dev->regs_lock);
> +	ctl = ioread32(dev->iaddr + PHN_CONTROL);
> +	if (!(ctl & PHN_CTL_IRQ)) {
> +		spin_unlock(&dev->regs_lock);
>  		return IRQ_NONE;
> +	}
>  
>  	iowrite32(0, dev->iaddr);
>  	iowrite32(0xc0, dev->iaddr);
> +
> +	if (dev->status & PHB_NOT_OH)
> +		for (i = 0; i < min(dev->oregs.count, 8U); i++)

But this is the interrupt handler, and there's a pointer chase as well as
the min() each time around the loop (I assume).  I doubt if the world will
end, but it seems a bit lazy?

> +			if (dev->oregs.mask & BIT(i))
> +				iowrite32(dev->oregs.values[i], dev->oaddr + i);
> +
> +	dev->ctl_reg ^= PHN_CTL_AMP;
> +	iowrite32(dev->ctl_reg, dev->iaddr + PHN_CONTROL);
> +	spin_unlock(&dev->regs_lock);
> +

Common .suspend()/.resume() template for PCI devices (was: Re: [PATCH 1/3] Misc: phantom, synchronize_irq() on suspend)

[Rafael J. Wysocki]

On Tuesday, 16 October 2007 01:23, Andrew Morton wrote:
> On Mon, 15 Oct 2007 09:32:28 -0700
> Jiri Slaby <jirislaby@gmail.com> wrote:
> 
> > phantom, synchronize_irq() on suspend
> > 
> > Wait after disabling device's interrupt until the handler finishes its
> > work if still in progress.
> > 
> > Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
> > 
> > ---
> > commit 7e792ef384190b517f2fb27cd0237fa30dbe0775
> > tree 17b15e5ab7c90eef0e7ae57e532839e81b831d58
> > parent 5c008a5651ee92ebe020dd5108a66a7db74fe41d
> > author Jiri Slaby <jirislaby@gmail.com> Mon, 15 Oct 2007 15:52:21 +0200
> > committer Jiri Slaby <jirislaby@gmail.com> Mon, 15 Oct 2007 15:52:21 +0200
> > 
> >  drivers/misc/phantom.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/misc/phantom.c b/drivers/misc/phantom.c
> > index 5108b7c..6e61a79 100644
> > --- a/drivers/misc/phantom.c
> > +++ b/drivers/misc/phantom.c
> > @@ -378,6 +378,8 @@ static int phantom_suspend(struct pci_dev *pdev, pm_message_t state)
> >  	iowrite32(0, dev->caddr + PHN_IRQCTL);
> >  	ioread32(dev->caddr + PHN_IRQCTL); /* PCI posting */
> >  
> > +	synchronize_irq(pdev->irq);
> > +
> >  	return 0;
> >  }
> >  
> 
> What inspired this change?  Some bug report, or does it just seem the right
> thing to do?

Probably this thread: http://lkml.org/lkml/2007/10/10/261

> Would it be logical to do this operation from the PCI core somewhere, on
> behalf of all PCI drivers?

Yes, it would.

The problem is that we don't have a common template for PCI devices' .suspend()
and .resume() callbacks and I don't feel confident enough to propose one.

[PATCH 1/1] misc-phantom-improved-data-passing-fix

[Jiri Slaby]

Andrew Morton wrote:
> On Mon, 15 Oct 2007 09:33:49 -0700
> Jiri Slaby <jirislaby@gmail.com> wrote:
> 
> > phantom, improved data passing
> > 
> > this new version guarantees amb_bit switch in small enough intervals, so
> > that the device won't stop working in the middle of a movement anymore.
> > However it preserves old (openhaptics) functionality.
> > 
> > ...
> >
> > +
> > +		/* preserve amp bit (don't allow to change it when in NOT_OH) */
> > +		if (r.reg == PHN_CONTROL && (dev->status & PHB_NOT_OH))
> > +			dev->ctl_reg = r.value = (r.value & ~PHN_CTL_AMP) |
> > +				(dev->ctl_reg & PHN_CTL_AMP);
> 
> hm, is that coded as clearly and as simply as possible?  Don't think so :(

Changed below.

> >  static irqreturn_t phantom_isr(int irq, void *data)
> >  {
> >  	struct phantom_device *dev = data;
> > +	unsigned int i;
> > +	u32 ctl;
> >  
> > -	if (!(ioread32(dev->iaddr + PHN_CONTROL) & PHN_CTL_IRQ))
> > +	spin_lock(&dev->regs_lock);
> > +	ctl = ioread32(dev->iaddr + PHN_CONTROL);
> > +	if (!(ctl & PHN_CTL_IRQ)) {
> > +		spin_unlock(&dev->regs_lock);
> >  		return IRQ_NONE;
> > +	}
> >  
> >  	iowrite32(0, dev->iaddr);
> >  	iowrite32(0xc0, dev->iaddr);
> > +
> > +	if (dev->status & PHB_NOT_OH)
> > +		for (i = 0; i < min(dev->oregs.count, 8U); i++)
> 
> But this is the interrupt handler, and there's a pointer chase as well as
> the min() each time around the loop (I assume).  I doubt if the world will
> end, but it seems a bit lazy?

Hmm, the compiler isn't as smart as I though. The fix follows, thanks.

--

misc-phantom-improved-data-passing-fix

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

---
commit d1d9a974c008af6e7ad0347df7f9ca372da1bb4c
tree 9c9cda79e150a752f052831ea18f35f31c8f9a5f
parent 228d539e706cd42c2950aa26ac01590e413d8f51
author Jiri Slaby <jirislaby@gmail.com> Tue, 16 Oct 2007 16:46:28 +0200
committer Jiri Slaby <jirislaby@gmail.com> Tue, 16 Oct 2007 16:46:28 +0200

 drivers/misc/phantom.c |   37 ++++++++++++++++++++++++-------------
 1 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/drivers/misc/phantom.c b/drivers/misc/phantom.c
index 4a610cc..cd221fd 100644
--- a/drivers/misc/phantom.c
+++ b/drivers/misc/phantom.c
@@ -113,9 +113,11 @@ static long phantom_ioctl(struct file *file, unsigned int cmd,
 		pr_debug("phantom: writing %x to %u\n", r.value, r.reg);
 
 		/* preserve amp bit (don't allow to change it when in NOT_OH) */
-		if (r.reg == PHN_CONTROL && (dev->status & PHB_NOT_OH))
-			dev->ctl_reg = r.value = (r.value & ~PHN_CTL_AMP) |
-				(dev->ctl_reg & PHN_CTL_AMP);
+		if (r.reg == PHN_CONTROL && (dev->status & PHB_NOT_OH)) {
+			r.value &= ~PHN_CTL_AMP;
+			r.value |= dev->ctl_reg & PHN_CTL_AMP;
+			dev->ctl_reg = r.value;
+		}
 
 		iowrite32(r.value, dev->iaddr + r.reg);
 		ioread32(dev->iaddr); /* PCI posting */
@@ -133,7 +135,8 @@ static long phantom_ioctl(struct file *file, unsigned int cmd,
 		if (dev->status & PHB_NOT_OH)
 			memcpy(&dev->oregs, &rs, sizeof(rs));
 		else {
-			for (i = 0; i < min(rs.count, 8U); i++)
+			u32 m = min(rs.count, 8U);
+			for (i = 0; i < m; i++)
 				if (rs.mask & BIT(i))
 					iowrite32(rs.values[i], dev->oaddr + i);
 			ioread32(dev->iaddr); /* PCI posting */
@@ -152,13 +155,17 @@ static long phantom_ioctl(struct file *file, unsigned int cmd,
 		if (copy_to_user(argp, &r, sizeof(r)))
 			return -EFAULT;
 		break;
-	case PHN_GET_REGS:
+	case PHN_GET_REGS: {
+		u32 m;
+
 		if (copy_from_user(&rs, argp, sizeof(rs)))
 			return -EFAULT;
 
+		m = min(rs.count, 8U);
+
 		pr_debug("phantom: GRS %u regs %x\n", rs.count, rs.mask);
 		spin_lock_irqsave(&dev->regs_lock, flags);
-		for (i = 0; i < min(rs.count, 8U); i++)
+		for (i = 0; i < m; i++)
 			if (rs.mask & BIT(i))
 				rs.values[i] = ioread32(dev->iaddr + i);
 		spin_unlock_irqrestore(&dev->regs_lock, flags);
@@ -166,7 +173,7 @@ static long phantom_ioctl(struct file *file, unsigned int cmd,
 		if (copy_to_user(argp, &rs, sizeof(rs)))
 			return -EFAULT;
 		break;
-	case PHN_NOT_OH:
+	} case PHN_NOT_OH:
 		spin_lock_irqsave(&dev->regs_lock, flags);
 		if (dev->status & PHB_RUNNING) {
 			printk(KERN_ERR "phantom: you need to set NOT_OH "
@@ -265,13 +272,17 @@ static irqreturn_t phantom_isr(int irq, void *data)
 	iowrite32(0, dev->iaddr);
 	iowrite32(0xc0, dev->iaddr);
 
-	if (dev->status & PHB_NOT_OH)
-		for (i = 0; i < min(dev->oregs.count, 8U); i++)
-			if (dev->oregs.mask & BIT(i))
-				iowrite32(dev->oregs.values[i], dev->oaddr + i);
+	if (dev->status & PHB_NOT_OH) {
+		struct phm_regs *r = &dev->oregs;
+		u32 m = min(r->count, 8U);
 
-	dev->ctl_reg ^= PHN_CTL_AMP;
-	iowrite32(dev->ctl_reg, dev->iaddr + PHN_CONTROL);
+		for (i = 0; i < m; i++)
+			if (r->mask & BIT(i))
+				iowrite32(r->values[i], dev->oaddr + i);
+
+		dev->ctl_reg ^= PHN_CTL_AMP;
+		iowrite32(dev->ctl_reg, dev->iaddr + PHN_CONTROL);
+	}
 	spin_unlock(&dev->regs_lock);
 
 	ioread32(dev->iaddr); /* PCI posting */