[PATCH 1/2] staging: comedi: addi_apci_1564: driver no longer needs to include addi_common.h

[PATCH 1/2] staging: comedi: addi_apci_1564: driver no longer needs to include addi_common.h

[Chase Southwood]

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 <chase.southwood@gmail.com>
Cc: Ian Abbott <abbotti@mev.co.uk>
Cc: H Hartley Sweeten <hsweeten@visionengravers.com>
---
 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

[PATCH 2/2] staging: comedi: addi_apci_1564: use addi_watchdog module to init watchdog subdevice

[Chase Southwood]

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 <chase.southwood@gmail.com>
Cc: Ian Abbott <abbotti@mev.co.uk>
Cc: H Hartley Sweeten <hsweeten@visionengravers.com>
---
 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

RE: [PATCH 2/2] staging: comedi: addi_apci_1564: use addi_watchdog module to init watchdog subdevice

[Hartley Sweeten]

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 <chase.southwood@gmail.com>
> Cc: Ian Abbott <abbotti@mev.co.uk>
> Cc: H Hartley Sweeten <hsweeten@visionengravers.com>
> ---
>  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

Re: [PATCH 2/2] staging: comedi: addi_apci_1564: use addi_watchdog module to init watchdog subdevice

[Chase Southwood]

On Sun, Jul 13, 2014 at 2:17 PM, Hartley Sweeten
<HartleyS@visionengravers.com> 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 <chase.southwood@gmail.com>
>> Cc: Ian Abbott <abbotti@mev.co.uk>
>> Cc: H Hartley Sweeten <hsweeten@visionengravers.com>
>> ---
>>  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 <arnd@arndb.de>
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 <arnd@arndb.de>
Cc: H Hartley Sweeten <hsweeten@visionengravers.com>
Acked-by: Ian Abbott <abbotti@mev.co.uk>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

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

Re: [PATCH 1/2] staging: comedi: addi_apci_1564: driver no longer needs to include addi_common.h

[Ian Abbott]

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 <chase.southwood@gmail.com>
> Cc: Ian Abbott <abbotti@mev.co.uk>
> Cc: H Hartley Sweeten <hsweeten@visionengravers.com>
> ---
>   drivers/staging/comedi/drivers/addi_apci_1564.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

Re: [PATCH 2/2] staging: comedi: addi_apci_1564: use addi_watchdog module to init watchdog subdevice

[Ian Abbott]

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 <chase.southwood@gmail.com>
> Cc: Ian Abbott <abbotti@mev.co.uk>
> Cc: H Hartley Sweeten <hsweeten@visionengravers.com>
> ---
>   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 <abbotti@mev.co.uk>

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

Re: [PATCH 1/2] staging: comedi: addi_apci_1564: driver no longer needs to include addi_common.h

[Ian Abbott]

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 <chase.southwood@gmail.com>
>> Cc: Ian Abbott <abbotti@mev.co.uk>
>> Cc: H Hartley Sweeten <hsweeten@visionengravers.com>
>> ---
>>   drivers/staging/comedi/drivers/addi_apci_1564.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> Signed-off-by: Ian Abbott <abbotti@mev.co.uk>

I meant 'Reviewed-by', but whatever!

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

Re: [PATCH 2/2] staging: comedi: addi_apci_1564: use addi_watchdog module to init watchdog subdevice

[Chase Southwood]

On Mon, Jul 14, 2014 at 4:22 AM, Ian Abbott <abbotti@mev.co.uk> 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 <chase.southwood@gmail.com>
>> Cc: Ian Abbott <abbotti@mev.co.uk>
>> Cc: H Hartley Sweeten <hsweeten@visionengravers.com>
>> ---
>>   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 <abbotti@mev.co.uk>
>
> --
> -=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
> -=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

Re: [PATCH 2/2] staging: comedi: addi_apci_1564: use addi_watchdog module to init watchdog subdevice

[Ian Abbott]

On 2014-07-15 05:00, Chase Southwood wrote:
> On Mon, Jul 14, 2014 at 4:22 AM, Ian Abbott <abbotti@mev.co.uk> 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 <chase.southwood@gmail.com>
>>> Cc: Ian Abbott <abbotti@mev.co.uk>
>>> Cc: H Hartley Sweeten <hsweeten@visionengravers.com>
>>> ---
>>>    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: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

Re: [PATCH 2/2] staging: comedi: addi_apci_1564: use addi_watchdog module to init watchdog subdevice

[Chase Southwood]

On Tue, Jul 15, 2014 at 5:14 AM, Ian Abbott <abbotti@mev.co.uk> wrote:
> On 2014-07-15 05:00, Chase Southwood wrote:
>>
>> On Mon, Jul 14, 2014 at 4:22 AM, Ian Abbott <abbotti@mev.co.uk> 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 <chase.southwood@gmail.com>
>>>> Cc: Ian Abbott <abbotti@mev.co.uk>
>>>> Cc: H Hartley Sweeten <hsweeten@visionengravers.com>
>>>> ---
>>>>    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: <abbotti@mev.co.uk>        )=-
> -=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-