RE: [PATCH 1/6] staging: comedi: addi_apci_1564: clarify change-of-state interrupt support

2016-06-08 Thread Hartley Sweeten 
On Wednesday, June 08, 2016 2:34 AM, Ian Abbott wrote:
> On 06/06/16 20:51, Hartley Sweeten wrote:
>>  From that I assume that the other bits will always return 0.
>
> Since it doesn't explicitly sat the other bits return 0, and until 
> someone can confirm, I think it's better not to assume that.

OK. I'll update the patch and repost the series.

Thanks,
Hartley

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/6] staging: comedi: addi_apci_1564: clarify change-of-state interrupt support

2016-06-08 Thread Ian Abbott 

On 06/06/16 20:51, Hartley Sweeten wrote:

On Monday, June 06, 2016 12:33 PM, Ian Abbott wrote:

Okay, I think I must have got something inverted in my head at the time
I wrote my reply.

Follow-up question: when you do:

s->state = inl(dev->iobase + APCI1564_DI_INT_STATUS_REG);

should that be ANDed with something (possibly
APCI1564_DI_INT_MODE_MASK), or are the other bits (including the new
"event" bits) guaranteed to read zero?


I guess it could be ANDed with APCI1564_DI_INT_MODE_MASK for
completeness.

The datasheet from ADDI-DATA shows this information for the register:

Address + 0x10
   Write: Nicht benutzt (English translation: unused)
   Read: Status Register 4 to 19
 Interrupt Status 4 to 19
 0: No Interrupt
 1: Interrupt generated

 From that I assume that the other bits will always return 0.


Since it doesn't explicitly sat the other bits return 0, and until 
someone can confirm, I think it's better not to assume that.


--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 1/6] staging: comedi: addi_apci_1564: clarify change-of-state interrupt support

2016-06-06 Thread Hartley Sweeten 
On Monday, June 06, 2016 12:33 PM, Ian Abbott wrote:
> Okay, I think I must have got something inverted in my head at the time 
> I wrote my reply.
>
> Follow-up question: when you do:
>
>   s->state = inl(dev->iobase + APCI1564_DI_INT_STATUS_REG);
>
> should that be ANDed with something (possibly 
> APCI1564_DI_INT_MODE_MASK), or are the other bits (including the new 
> "event" bits) guaranteed to read zero?

I guess it could be ANDed with APCI1564_DI_INT_MODE_MASK for
completeness.

The datasheet from ADDI-DATA shows this information for the register:

Address + 0x10
  Write: Nicht benutzt  (English translation: unused)
  Read: Status Register 4 to 19
Interrupt Status 4 to 19
0: No Interrupt
1: Interrupt generated

>From that I assume that the other bits will always return 0.

Regards,
Hartley

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/6] staging: comedi: addi_apci_1564: clarify change-of-state interrupt support

2016-06-06 Thread Ian Abbott 

On 06/06/16 19:25, Hartley Sweeten wrote:

On Friday, June 03, 2016 2:58 AM, Ian Abbott wrote:

On 02/06/16 22:58, H Hartley Sweeten wrote:

This board supports change-of-state interrupts on digital inputs 4 to 19
not 0 to 15.

The current code "works" but it could set inappropriate bits in the mode1
and mode2 registers that setup which channels are enabled. It also doesn't
return the status of the upper 4 channels (19 to 16).

Fix the comment and mask the mode1/mode2 values so that only the interrupt
capable channels can be enabled.

Add the SDF_LSAMPL flag to the subdevice so that 32-bit samples are used
instead of 16-bit ones. This allows returning the upper 4 channels. Use
the remaining bits in the sample to return "event" flags to the user.

The timer and counter subdevices can also generate interrupts and are a bit
hacked. They don't currently follow the comedi API and they use send_sig()
to let the task that know that the interrupt occured. The "event" flags will
be used instead when these subdevices are fixed.

Signed-off-by: H Hartley Sweeten 
Cc: Ian Abbott 
Cc: Greg Kroah-Hartman 
---
   drivers/staging/comedi/drivers/addi_apci_1564.c | 37 
+
   1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/comedi/drivers/addi_apci_1564.c 
b/drivers/staging/comedi/drivers/addi_apci_1564.c
index f1ccfbd..37e18b3 100644





+#define APCI1564_DI_INT_MODE_MASK  0x0000 /* chans [19:4] */





+#define APCI1564_EVENT_MASK0xffff /* all but [19:4] */





+   s->state &= ~APCI1564_EVENT_MASK;
+

status = inl(dev->iobase + APCI1564_DI_IRQ_REG);
if (status & APCI1564_DI_IRQ_ENA) {
-   /* disable the interrupt */
+   s->state = inl(dev->iobase + APCI1564_DI_INT_STATUS_REG);
+   s->state |= APCI1564_EVENT_COS;





+   if (s->state & APCI1564_EVENT_MASK) {
+   comedi_buf_write_samples(s, >state, 1);
+   comedi_handle_events(dev, s);
+   }
+
return IRQ_HANDLED;
   }



I'm struggling to see how that works.  It looks like once
APCI1564_EVENT_COS has been set in s->state, it never gets cleared (or
at least it only gets cleared momentarily), and after that, the `if
(s->state & APCI1564_EVENT_MASK)` test will always be true, and so it
will write a sample to the buffer on every interrupt.


Ian,

1) When an interrupt occurs the last "events" are masked off
 (leaving the last state of the digital inputs).
2) If the digital inputs caused the interrupt, the state of the
 digital inputs is read and the event bit APCI1564_EVENT_COS
 is set.


Okay, I think I must have got something inverted in my head at the time 
I wrote my reply.


Follow-up question: when you do:

s->state = inl(dev->iobase + APCI1564_DI_INT_STATUS_REG);

should that be ANDed with something (possibly 
APCI1564_DI_INT_MODE_MASK), or are the other bits (including the new 
"event" bits) guaranteed to read zero?



--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 1/6] staging: comedi: addi_apci_1564: clarify change-of-state interrupt support

2016-06-06 Thread Hartley Sweeten 
On Friday, June 03, 2016 2:58 AM, Ian Abbott wrote:
> On 02/06/16 22:58, H Hartley Sweeten wrote:
>> This board supports change-of-state interrupts on digital inputs 4 to 19
>> not 0 to 15.
>>
>> The current code "works" but it could set inappropriate bits in the mode1
>> and mode2 registers that setup which channels are enabled. It also doesn't
>> return the status of the upper 4 channels (19 to 16).
>>
>> Fix the comment and mask the mode1/mode2 values so that only the interrupt
>> capable channels can be enabled.
>>
>> Add the SDF_LSAMPL flag to the subdevice so that 32-bit samples are used
>> instead of 16-bit ones. This allows returning the upper 4 channels. Use
>> the remaining bits in the sample to return "event" flags to the user.
>>
>> The timer and counter subdevices can also generate interrupts and are a bit
>> hacked. They don't currently follow the comedi API and they use send_sig()
>> to let the task that know that the interrupt occured. The "event" flags will
>> be used instead when these subdevices are fixed.
>>
>> Signed-off-by: H Hartley Sweeten 
>> Cc: Ian Abbott 
>> Cc: Greg Kroah-Hartman 
>> ---
>>   drivers/staging/comedi/drivers/addi_apci_1564.c | 37 
>> +
>>   1 file changed, 26 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/staging/comedi/drivers/addi_apci_1564.c 
>> b/drivers/staging/comedi/drivers/addi_apci_1564.c
>> index f1ccfbd..37e18b3 100644



>> +#define APCI1564_DI_INT_MODE_MASK   0x0000 /* chans [19:4] */



>> +#define APCI1564_EVENT_MASK 0xffff /* all but [19:4] */



>> +s->state &= ~APCI1564_EVENT_MASK;
>> +
>   status = inl(dev->iobase + APCI1564_DI_IRQ_REG);
>   if (status & APCI1564_DI_IRQ_ENA) {
> - /* disable the interrupt */
> + s->state = inl(dev->iobase + APCI1564_DI_INT_STATUS_REG);
> + s->state |= APCI1564_EVENT_COS;



> + if (s->state & APCI1564_EVENT_MASK) {
> + comedi_buf_write_samples(s, >state, 1);
> + comedi_handle_events(dev, s);
> + }
> +
>   return IRQ_HANDLED;
>   }

> I'm struggling to see how that works.  It looks like once 
> APCI1564_EVENT_COS has been set in s->state, it never gets cleared (or 
> at least it only gets cleared momentarily), and after that, the `if 
> (s->state & APCI1564_EVENT_MASK)` test will always be true, and so it 
> will write a sample to the buffer on every interrupt.

Ian,

1) When an interrupt occurs the last "events" are masked off
(leaving the last state of the digital inputs).
2) If the digital inputs caused the interrupt, the state of the
digital inputs is read and the event bit APCI1564_EVENT_COS
is set.
3) Similarly, the other patches add support for the timer and
   counter subdevices and set the APCI1564_EVENT_TIMER or
APCI1564_EVENT_COUNTER(chan) bits as appropriate.
4) At the end of the interrupt if any of the event bits are set
the sample will be written to the buffer and the event handled.

This seemed like the best way to handle the timer and counter
interrupts without adding unnecessary async command support.

Regards,
Hartley

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/6] staging: comedi: addi_apci_1564: clarify change-of-state interrupt support

2016-06-02 Thread H Hartley Sweeten 
This board supports change-of-state interrupts on digital inputs 4 to 19
not 0 to 15.

The current code "works" but it could set inappropriate bits in the mode1
and mode2 registers that setup which channels are enabled. It also doesn't
return the status of the upper 4 channels (19 to 16).

Fix the comment and mask the mode1/mode2 values so that only the interrupt
capable channels can be enabled.

Add the SDF_LSAMPL flag to the subdevice so that 32-bit samples are used
instead of 16-bit ones. This allows returning the upper 4 channels. Use
the remaining bits in the sample to return "event" flags to the user.

The timer and counter subdevices can also generate interrupts and are a bit
hacked. They don't currently follow the comedi API and they use send_sig()
to let the task that know that the interrupt occured. The "event" flags will
be used instead when these subdevices are fixed.

Signed-off-by: H Hartley Sweeten 
Cc: Ian Abbott 
Cc: Greg Kroah-Hartman 
---
 drivers/staging/comedi/drivers/addi_apci_1564.c | 37 +
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/comedi/drivers/addi_apci_1564.c 
b/drivers/staging/comedi/drivers/addi_apci_1564.c
index f1ccfbd..37e18b3 100644
--- a/drivers/staging/comedi/drivers/addi_apci_1564.c
+++ b/drivers/staging/comedi/drivers/addi_apci_1564.c
@@ -77,6 +77,7 @@
 #define APCI1564_DI_REG0x00
 #define APCI1564_DI_INT_MODE1_REG  0x04
 #define APCI1564_DI_INT_MODE2_REG  0x08
+#define APCI1564_DI_INT_MODE_MASK  0x0000 /* chans [19:4] */
 #define APCI1564_DI_INT_STATUS_REG 0x0c
 #define APCI1564_DI_IRQ_REG0x10
 #define APCI1564_DI_IRQ_ENABIT(2)
@@ -111,6 +112,13 @@
  */
 #define APCI1564_COUNTER(x)((x) * 0x20)
 
+/*
+ * The dev->read_subdev is used to return the interrupt events along with
+ * the state of the interrupt capable inputs.
+ */
+#define APCI1564_EVENT_COS BIT(31)
+#define APCI1564_EVENT_MASK0xffff /* all but [19:4] */
+
 struct apci1564_private {
unsigned long eeprom;   /* base address of EEPROM register */
unsigned long timer;/* base address of 12-bit timer */
@@ -165,18 +173,16 @@ static irqreturn_t apci1564_interrupt(int irq, void *d)
unsigned int ctrl;
unsigned int chan;
 
+   s->state &= ~APCI1564_EVENT_MASK;
+
status = inl(dev->iobase + APCI1564_DI_IRQ_REG);
if (status & APCI1564_DI_IRQ_ENA) {
-   /* disable the interrupt */
+   s->state = inl(dev->iobase + APCI1564_DI_INT_STATUS_REG);
+   s->state |= APCI1564_EVENT_COS;
+
+   /* clear the interrupt */
outl(status & ~APCI1564_DI_IRQ_ENA,
 dev->iobase + APCI1564_DI_IRQ_REG);
-
-   s->state = inl(dev->iobase + APCI1564_DI_INT_STATUS_REG) &
-  0x;
-   comedi_buf_write_samples(s, >state, 1);
-   comedi_handle_events(dev, s);
-
-   /* enable the interrupt */
outl(status, dev->iobase + APCI1564_DI_IRQ_REG);
}
 
@@ -214,6 +220,11 @@ static irqreturn_t apci1564_interrupt(int irq, void *d)
}
}
 
+   if (s->state & APCI1564_EVENT_MASK) {
+   comedi_buf_write_samples(s, >state, 1);
+   comedi_handle_events(dev, s);
+   }
+
return IRQ_HANDLED;
 }
 
@@ -255,7 +266,7 @@ static int apci1564_diag_insn_bits(struct comedi_device 
*dev,
 /*
  * Change-Of-State (COS) interrupt configuration
  *
- * Channels 0 to 15 are interruptible. These channels can be configured
+ * Channels 4 to 19 are interruptible. These channels can be configured
  * to generate interrupts based on AND/OR logic for the desired channels.
  *
  * OR logic
@@ -343,6 +354,10 @@ static int apci1564_cos_insn_config(struct comedi_device 
*dev,
default:
return -EINVAL;
}
+
+   /* ensure the mode bits are in-range for channels [19:4] */
+   devpriv->mode1 &= APCI1564_DI_INT_MODE_MASK;
+   devpriv->mode2 &= APCI1564_DI_INT_MODE_MASK;
break;
default:
return -EINVAL;
@@ -409,7 +424,7 @@ static int apci1564_cos_cmd(struct comedi_device *dev,
 {
struct apci1564_private *devpriv = dev->private;
 
-   if (!devpriv->ctrl) {
+   if (!devpriv->ctrl && !(devpriv->mode1 || devpriv->mode2)) {
dev_warn(dev->class_dev,
 "Interrupts disabled due to mode configuration!\n");
return -EINVAL;
@@ -501,7 +516,7 @@ static int apci1564_auto_attach(struct comedi_device *dev,
if (dev->irq) {
dev->read_subdev = s;
s->type =