This driver no longer depends on anything in addi_common.h, save for a
few headers that it was including indirectly. Remove the include of
addi_common.h and add the includes of <linux/interrupt.h>
and <linux/sched.h> directly.
Signed-off-by: Chase Southwood <[email protected]>
Cc: Ian Abbott <[email protected]>
Cc: H Hartley Sweeten <[email protected]>
---
drivers/staging/comedi/drivers/addi_apci_1564.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/comedi/drivers/addi_apci_1564.c b/drivers/staging/comedi/drivers/addi_apci_1564.c
index 1e25342..16f3b69 100644
--- a/drivers/staging/comedi/drivers/addi_apci_1564.c
+++ b/drivers/staging/comedi/drivers/addi_apci_1564.c
@@ -1,13 +1,13 @@
#include <linux/module.h>
#include <linux/pci.h>
+#include <linux/interrupt.h>
+#include <linux/sched.h>
#include "../comedidev.h"
#include "comedi_fc.h"
#include "amcc_s5933.h"
#include "addi_watchdog.h"
-#include "addi-data/addi_common.h"
-
struct apci1564_private {
unsigned int amcc_iobase; /* base of AMCC I/O registers */
unsigned int mode1; /* riding-edge/high level channels */
--
2.0.1
Use the addi_watchdog module to provide support for the watchdog
subdevice.
Also, rearrange the subdevice init blocks so that the order makes sense.
Digital input/output subdevices and subdevices for DI/DO interrupt
support, followed by timer/counter/watchdog subdevices is the new order.
Signed-off-by: Chase Southwood <[email protected]>
Cc: Ian Abbott <[email protected]>
Cc: H Hartley Sweeten <[email protected]>
---
drivers/staging/comedi/drivers/addi_apci_1564.c | 34 +++++++++++++++----------
1 file changed, 20 insertions(+), 14 deletions(-)
diff --git a/drivers/staging/comedi/drivers/addi_apci_1564.c b/drivers/staging/comedi/drivers/addi_apci_1564.c
index 16f3b69..190b026 100644
--- a/drivers/staging/comedi/drivers/addi_apci_1564.c
+++ b/drivers/staging/comedi/drivers/addi_apci_1564.c
@@ -373,7 +373,7 @@ static int apci1564_auto_attach(struct comedi_device *dev,
dev->irq = pcidev->irq;
}
- ret = comedi_alloc_subdevices(dev, 4);
+ ret = comedi_alloc_subdevices(dev, 5);
if (ret)
return ret;
@@ -397,20 +397,8 @@ static int apci1564_auto_attach(struct comedi_device *dev,
s->insn_bits = apci1564_do_insn_bits;
s->insn_read = apci1564_do_read;
- /* Allocate and Initialise Timer Subdevice Structures */
- s = &dev->subdevices[2];
- s->type = COMEDI_SUBD_TIMER;
- s->subdev_flags = SDF_WRITEABLE;
- s->n_chan = 1;
- s->maxdata = 0;
- s->len_chanlist = 1;
- s->range_table = &range_digital;
- s->insn_write = apci1564_timer_write;
- s->insn_read = apci1564_timer_read;
- s->insn_config = apci1564_timer_config;
-
/* Change-Of-State (COS) interrupt subdevice */
- s = &dev->subdevices[3];
+ s = &dev->subdevices[2];
if (dev->irq) {
dev->read_subdev = s;
s->type = COMEDI_SUBD_DI;
@@ -428,6 +416,24 @@ static int apci1564_auto_attach(struct comedi_device *dev,
s->type = COMEDI_SUBD_UNUSED;
}
+ /* Allocate and Initialise Timer Subdevice Structures */
+ s = &dev->subdevices[3];
+ s->type = COMEDI_SUBD_TIMER;
+ s->subdev_flags = SDF_WRITEABLE;
+ s->n_chan = 1;
+ s->maxdata = 0;
+ s->len_chanlist = 1;
+ s->range_table = &range_digital;
+ s->insn_write = apci1564_timer_write;
+ s->insn_read = apci1564_timer_read;
+ s->insn_config = apci1564_timer_config;
+
+ /* Initialize the watchdog subdevice */
+ s = &dev->subdevices[4];
+ ret = addi_watchdog_init(s, devpriv->amcc_iobase + APCI1564_WDOG_REG);
+ if (ret)
+ return ret;
+
return 0;
}
--
2.0.1
On Saturday, July 12, 2014 3:44 PM, Chase Southwood wrote:
> Use the addi_watchdog module to provide support for the watchdog
> subdevice.
>
> Also, rearrange the subdevice init blocks so that the order makes sense.
> Digital input/output subdevices and subdevices for DI/DO interrupt
> support, followed by timer/counter/watchdog subdevices is the new order.
>
> Signed-off-by: Chase Southwood <[email protected]>
> Cc: Ian Abbott <[email protected]>
> Cc: H Hartley Sweeten <[email protected]>
> ---
> drivers/staging/comedi/drivers/addi_apci_1564.c | 34 +++++++++++++++----------
> 1 file changed, 20 insertions(+), 14 deletions(-)
Chase,
You should also add the dependency for the addi watchdog module
to the Kconfig.
Regards,
Hartley
On Sun, Jul 13, 2014 at 2:17 PM, Hartley Sweeten
<[email protected]> wrote:
> On Saturday, July 12, 2014 3:44 PM, Chase Southwood wrote:
>> Use the addi_watchdog module to provide support for the watchdog
>> subdevice.
>>
>> Also, rearrange the subdevice init blocks so that the order makes sense.
>> Digital input/output subdevices and subdevices for DI/DO interrupt
>> support, followed by timer/counter/watchdog subdevices is the new order.
>>
>> Signed-off-by: Chase Southwood <[email protected]>
>> Cc: Ian Abbott <[email protected]>
>> Cc: H Hartley Sweeten <[email protected]>
>> ---
>> drivers/staging/comedi/drivers/addi_apci_1564.c | 34 +++++++++++++++----------
>> 1 file changed, 20 insertions(+), 14 deletions(-)
>
> Chase,
>
> You should also add the dependency for the addi watchdog module
> to the Kconfig.
>
Hello, Hartley!
The select statement for COMEDI_ADDI_WATCHDOG was added to Kconfig for
the addi_apci_1564 driver in commit
8851362:
From: Arnd Bergmann <[email protected]>
Date: Tue, 3 Jun 2014 12:29:29 +0200
Subject: [PATCH] staging: comedi: addi_apci_1564: add addi_watchdog dependency
Signed-off-by: Arnd Bergmann <[email protected]>
Cc: H Hartley Sweeten <[email protected]>
Acked-by: Ian Abbott <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
This is because the driver has already been using
addi_watchdog_reset() and I had forgotten to add the select to the
Kconfig when I added that function call, so Arnd added it when a
randconfig build error turned up later. Sorry for the
confusion.
Thanks,
Chase
> Regards,
> Hartley
On 2014-07-12 23:42, Chase Southwood wrote:
> This driver no longer depends on anything in addi_common.h, save for a
> few headers that it was including indirectly. Remove the include of
> addi_common.h and add the includes of <linux/interrupt.h>
> and <linux/sched.h> directly.
>
> Signed-off-by: Chase Southwood <[email protected]>
> Cc: Ian Abbott <[email protected]>
> Cc: H Hartley Sweeten <[email protected]>
> ---
> drivers/staging/comedi/drivers/addi_apci_1564.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Signed-off-by: Ian Abbott <[email protected]>
--
-=( Ian Abbott @ MEV Ltd. E-mail: <[email protected]> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
On 2014-07-12 23:44, Chase Southwood wrote:
> Use the addi_watchdog module to provide support for the watchdog
> subdevice.
>
> Also, rearrange the subdevice init blocks so that the order makes sense.
> Digital input/output subdevices and subdevices for DI/DO interrupt
> support, followed by timer/counter/watchdog subdevices is the new order.
>
> Signed-off-by: Chase Southwood <[email protected]>
> Cc: Ian Abbott <[email protected]>
> Cc: H Hartley Sweeten <[email protected]>
> ---
> drivers/staging/comedi/drivers/addi_apci_1564.c | 34 +++++++++++++++----------
> 1 file changed, 20 insertions(+), 14 deletions(-)
I don't think the subdevice order matters that much, and I prefer to
keep them stable, but since this driver is in such a state of flux, it
doesn't really matter.
Reviewed-by: Ian Abbott <[email protected]>
--
-=( Ian Abbott @ MEV Ltd. E-mail: <[email protected]> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
On 2014-07-14 10:15, Ian Abbott wrote:
> On 2014-07-12 23:42, Chase Southwood wrote:
>> This driver no longer depends on anything in addi_common.h, save for a
>> few headers that it was including indirectly. Remove the include of
>> addi_common.h and add the includes of <linux/interrupt.h>
>> and <linux/sched.h> directly.
>>
>> Signed-off-by: Chase Southwood <[email protected]>
>> Cc: Ian Abbott <[email protected]>
>> Cc: H Hartley Sweeten <[email protected]>
>> ---
>> drivers/staging/comedi/drivers/addi_apci_1564.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> Signed-off-by: Ian Abbott <[email protected]>
I meant 'Reviewed-by', but whatever!
--
-=( Ian Abbott @ MEV Ltd. E-mail: <[email protected]> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
On Mon, Jul 14, 2014 at 4:22 AM, Ian Abbott <[email protected]> wrote:
> On 2014-07-12 23:44, Chase Southwood wrote:
>>
>> Use the addi_watchdog module to provide support for the watchdog
>> subdevice.
>>
>> Also, rearrange the subdevice init blocks so that the order makes sense.
>> Digital input/output subdevices and subdevices for DI/DO interrupt
>> support, followed by timer/counter/watchdog subdevices is the new order.
>>
>> Signed-off-by: Chase Southwood <[email protected]>
>> Cc: Ian Abbott <[email protected]>
>> Cc: H Hartley Sweeten <[email protected]>
>> ---
>> drivers/staging/comedi/drivers/addi_apci_1564.c | 34
>> +++++++++++++++----------
>> 1 file changed, 20 insertions(+), 14 deletions(-)
>
>
> I don't think the subdevice order matters that much, and I prefer to keep
> them stable, but since this driver is in such a state of flux, it doesn't
> really matter.
>
Hi Ian!
Quick question here about this. First off, duly noted that grouping
subdevices by function isn't necessary and I won't shuffle them around
like this in the future. Second, the reason I stuck the watchdog at
the end is because it causes an early return if addi_watchdog_init()
returns an error and it seemed appropriate at the end so it doesn't
prevent the initialization of any other subdevices if that call should
fail. Now I realize that it is very unlikely that that call fails,
but in any case should I put future subdevice inits above the watchdog
for the same reason (so they aren't at risk of not getting
initialized), or does that count for subdevice order not being stable
and you would prefer them all to go at the end?
Thanks,
Chase
> Reviewed-by: Ian Abbott <[email protected]>
>
> --
> -=( Ian Abbott @ MEV Ltd. E-mail: <[email protected]> )=-
> -=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
On 2014-07-15 05:00, Chase Southwood wrote:
> On Mon, Jul 14, 2014 at 4:22 AM, Ian Abbott <[email protected]> wrote:
>> On 2014-07-12 23:44, Chase Southwood wrote:
>>>
>>> Use the addi_watchdog module to provide support for the watchdog
>>> subdevice.
>>>
>>> Also, rearrange the subdevice init blocks so that the order makes sense.
>>> Digital input/output subdevices and subdevices for DI/DO interrupt
>>> support, followed by timer/counter/watchdog subdevices is the new order.
>>>
>>> Signed-off-by: Chase Southwood <[email protected]>
>>> Cc: Ian Abbott <[email protected]>
>>> Cc: H Hartley Sweeten <[email protected]>
>>> ---
>>> drivers/staging/comedi/drivers/addi_apci_1564.c | 34
>>> +++++++++++++++----------
>>> 1 file changed, 20 insertions(+), 14 deletions(-)
>>
>>
>> I don't think the subdevice order matters that much, and I prefer to keep
>> them stable, but since this driver is in such a state of flux, it doesn't
>> really matter.
>>
>
> Hi Ian!
> Quick question here about this. First off, duly noted that grouping
> subdevices by function isn't necessary and I won't shuffle them around
> like this in the future. Second, the reason I stuck the watchdog at
> the end is because it causes an early return if addi_watchdog_init()
> returns an error and it seemed appropriate at the end so it doesn't
> prevent the initialization of any other subdevices if that call should
> fail. Now I realize that it is very unlikely that that call fails,
> but in any case should I put future subdevice inits above the watchdog
> for the same reason (so they aren't at risk of not getting
> initialized), or does that count for subdevice order not being stable
> and you would prefer them all to go at the end?
Since you return an error from the auto_attach handler
apci1564_auto_attach() when addi_watchdog_init() fails, it makes little
difference what order the subdevices are initialized in. The error from
auto_attach handler causes the comedi core to call the detach handler
apci1564_detach() and tear everything down. Ultimately,
comedi_pci_auto_attach() will return an error back to the PCI probe
function apci1564_pci_probe(), which will propagate it to the PCI subsystem.
In general, if adding a new subdevice, either add it to the end or
replace an "unused" subdevice.
--
-=( Ian Abbott @ MEV Ltd. E-mail: <[email protected]> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
On Tue, Jul 15, 2014 at 5:14 AM, Ian Abbott <[email protected]> wrote:
> On 2014-07-15 05:00, Chase Southwood wrote:
>>
>> On Mon, Jul 14, 2014 at 4:22 AM, Ian Abbott <[email protected]> wrote:
>>>
>>> On 2014-07-12 23:44, Chase Southwood wrote:
>>>>
>>>>
>>>> Use the addi_watchdog module to provide support for the watchdog
>>>> subdevice.
>>>>
>>>> Also, rearrange the subdevice init blocks so that the order makes sense.
>>>> Digital input/output subdevices and subdevices for DI/DO interrupt
>>>> support, followed by timer/counter/watchdog subdevices is the new order.
>>>>
>>>> Signed-off-by: Chase Southwood <[email protected]>
>>>> Cc: Ian Abbott <[email protected]>
>>>> Cc: H Hartley Sweeten <[email protected]>
>>>> ---
>>>> drivers/staging/comedi/drivers/addi_apci_1564.c | 34
>>>> +++++++++++++++----------
>>>> 1 file changed, 20 insertions(+), 14 deletions(-)
>>>
>>>
>>>
>>> I don't think the subdevice order matters that much, and I prefer to keep
>>> them stable, but since this driver is in such a state of flux, it doesn't
>>> really matter.
>>>
>>
>> Hi Ian!
>> Quick question here about this. First off, duly noted that grouping
>> subdevices by function isn't necessary and I won't shuffle them around
>> like this in the future. Second, the reason I stuck the watchdog at
>> the end is because it causes an early return if addi_watchdog_init()
>> returns an error and it seemed appropriate at the end so it doesn't
>> prevent the initialization of any other subdevices if that call should
>> fail. Now I realize that it is very unlikely that that call fails,
>> but in any case should I put future subdevice inits above the watchdog
>> for the same reason (so they aren't at risk of not getting
>> initialized), or does that count for subdevice order not being stable
>> and you would prefer them all to go at the end?
>
>
> Since you return an error from the auto_attach handler
> apci1564_auto_attach() when addi_watchdog_init() fails, it makes little
> difference what order the subdevices are initialized in. The error from
> auto_attach handler causes the comedi core to call the detach handler
> apci1564_detach() and tear everything down. Ultimately,
> comedi_pci_auto_attach() will return an error back to the PCI probe function
> apci1564_pci_probe(), which will propagate it to the PCI subsystem.
>
> In general, if adding a new subdevice, either add it to the end or replace
> an "unused" subdevice.
>
Oh excellent. Perfect, that makes sense. Thanks for taking the time
to explain, I'll make sure everything goes at the end from now on.
Thanks,
Chase
>
> --
> -=( Ian Abbott @ MEV Ltd. E-mail: <[email protected]> )=-
> -=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-