RE: [PATCH 1/2] Staging: comedi: introduce outl_1564_* and inl_1564_* helper functions in hwdrv_apci1564.c
On Saturday, March 01, 2014 3:28 AM, Chase Southwood wrote: > Subject: [PATCH 1/2] Staging: comedi: introduce outl_1564_* and inl_1564_* > helper functions in hwdrv_apci1564.c > > This patch introduces a handful of outl and inl helper functions with the > ultimate goal of improving code readability and allowing several lines > which violate the character limit to be shortened in a sane way. > > Cc: Dan Carpenter > Signed-off-by: Chase Southwood > --- > This patchset serves as a replacement to my previous cleanup patchset for > hwdrv_apci1564.c > > Dan, > After spending a little bit more time with this and trying out different > ways of cleaning this up, I decided that making helper functions for all > of the most common register sets would look the best, but I haven't made > a helper for a few of the least common inl/outl calls because if I did, > the sheer number of helper functions would get quite ridiculous. > Let me know if you think my selections of what to make into helper > functions seems appropriate. Chase, Sorry for jumping in here late. I think if you just tidy up the register map defines it would help. Some of them are actually used incorrectly anyway. For instance, these two define the "base" offset for the digital input registers, which is also to digital input register, and the offset from the "base" to the interrupt mode1 register. #define APCI1564_DIGITAL_IP 0x04 #define APCI1564_DIGITAL_IP_INTERRUPT_MODE1 4 One use of the APCI1564_DIGITAL_IP_INTERRUPT_MODE1 define is in i_APCI1564_ConfigDigitalInput(): outl(data[2], devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP + APCI1564_DIGITAL_IP_INTERRUPT_MODE1); But in i_APCI1564_Reset () it is used as: outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP_INTERRUPT_MODE1); So in the first example the driver is writing to a register at offset 0x08 (0x04 + 4) but the second is writing to a register at offset 0x04 (4), which is really the digitial input register. I suggest you just fix the register map defines so they are the "real" offsets to each register and not a mix of real offsets and adders to those offsets. I would also rename them to help with the > 80 char lines. Something like this: /* * devpriv->i_IobaseAmcc Register map */ #define APCI1564_DI_REG 0x04 #define APCI1564_DI_INT_MODE1_REG 0x08 #define APCI1564_DI_INT_MODE2_REG 0x0c #define APCI1564_DI_INT_STATUS_REG 0x10 #define APCI1564_DI_IRQ_REG 0x14 #define APCI1564_DO_REG 0x18 #define APCI1564_DO_INT_CTRL_REG0x1c #define APCI1564_DO_INT_STATUS_REG 0x20 #define APCI1564_DO_IRQ_REG 0x24 #define APCI1564_WDOG_REG 0x28 #define APCI1564_WDOG_RELOAD_REG0x2c #define APCI1564_WDOG_TIMEBASE_REG 0x30 #define APCI1564_WDOG_CTRL_REG 0x34 #define APCI1564_WDOG_STATUS_REG0x38 #define APCI1564_WDOG_IRQ_REG 0x3c #define APCI1564_WDOG_WARN_TIMEVAL_REG 0x40 #define APCI1564_WDOG_WARN_TIMEBASE_REG 0x44 #define APCI1564_TIMER_REG 0x48 #define APCI1564_TIMER_RELOAD_REG 0x4c #define APCI1564_TIMER_TIMEBASE_REG 0x50 #define APCI1564_TIMER_CTRL_REG 0x54 #define APCI1564_TIMER_STATUS_REG 0x58 #define APCI1564_TIMER_IRQ_REG 0x5c #define APCI1564_TIMER_WARN_TIMEVAL_REG 0x60 #define APCI1564_TIMER_WARN_TIMEBASE_REG0x64 /* * devpriv->iobase Register map */ #define APCI1564_TCW_REG(x) (0x00 + ((x) * 20)) #define APCI1564_TCW_RELOAD_REG(x) (0x04 + ((x) * 20)) #define APCI1564_TCW_TIMEBASE_REG(x)(0x08 + ((x) * 20)) #define APCI1564_TCW_CTRL_REG(x)(0x0c + ((x) * 20)) #define APCI1564_TCW_STATUS_REG(x) (0x10 + ((x) * 20)) #define APCI1564_TCW_IRQ_REG(x) (0x14 + ((x) * 20)) #define APCI1564_TCW_WARN_TIMEVAL_REG(x)(0x18 + ((x) * 20)) #define APCI1564_TCW_WARN_TIMEBASE_REG(x) (0x1c + ((x) * 20)) You will probably want to split this into a couple patches to make reviewing easier. Maybe do it by subdevice: digital input, digital output, watchdog, timer, then the counters. Regards, Hartley -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/2] Staging: comedi: introduce outl_1564_* and inl_1564_* helper functions in hwdrv_apci1564.c
On Saturday, March 01, 2014 3:28 AM, Chase Southwood wrote: Subject: [PATCH 1/2] Staging: comedi: introduce outl_1564_* and inl_1564_* helper functions in hwdrv_apci1564.c This patch introduces a handful of outl and inl helper functions with the ultimate goal of improving code readability and allowing several lines which violate the character limit to be shortened in a sane way. Cc: Dan Carpenter dan.carpen...@oracle.com Signed-off-by: Chase Southwood chase.southw...@yahoo.com --- This patchset serves as a replacement to my previous cleanup patchset for hwdrv_apci1564.c Dan, After spending a little bit more time with this and trying out different ways of cleaning this up, I decided that making helper functions for all of the most common register sets would look the best, but I haven't made a helper for a few of the least common inl/outl calls because if I did, the sheer number of helper functions would get quite ridiculous. Let me know if you think my selections of what to make into helper functions seems appropriate. Chase, Sorry for jumping in here late. I think if you just tidy up the register map defines it would help. Some of them are actually used incorrectly anyway. For instance, these two define the base offset for the digital input registers, which is also to digital input register, and the offset from the base to the interrupt mode1 register. #define APCI1564_DIGITAL_IP 0x04 #define APCI1564_DIGITAL_IP_INTERRUPT_MODE1 4 One use of the APCI1564_DIGITAL_IP_INTERRUPT_MODE1 define is in i_APCI1564_ConfigDigitalInput(): outl(data[2], devpriv-i_IobaseAmcc + APCI1564_DIGITAL_IP + APCI1564_DIGITAL_IP_INTERRUPT_MODE1); But in i_APCI1564_Reset () it is used as: outl(0x0, devpriv-i_IobaseAmcc + APCI1564_DIGITAL_IP_INTERRUPT_MODE1); So in the first example the driver is writing to a register at offset 0x08 (0x04 + 4) but the second is writing to a register at offset 0x04 (4), which is really the digitial input register. I suggest you just fix the register map defines so they are the real offsets to each register and not a mix of real offsets and adders to those offsets. I would also rename them to help with the 80 char lines. Something like this: /* * devpriv-i_IobaseAmcc Register map */ #define APCI1564_DI_REG 0x04 #define APCI1564_DI_INT_MODE1_REG 0x08 #define APCI1564_DI_INT_MODE2_REG 0x0c #define APCI1564_DI_INT_STATUS_REG 0x10 #define APCI1564_DI_IRQ_REG 0x14 #define APCI1564_DO_REG 0x18 #define APCI1564_DO_INT_CTRL_REG0x1c #define APCI1564_DO_INT_STATUS_REG 0x20 #define APCI1564_DO_IRQ_REG 0x24 #define APCI1564_WDOG_REG 0x28 #define APCI1564_WDOG_RELOAD_REG0x2c #define APCI1564_WDOG_TIMEBASE_REG 0x30 #define APCI1564_WDOG_CTRL_REG 0x34 #define APCI1564_WDOG_STATUS_REG0x38 #define APCI1564_WDOG_IRQ_REG 0x3c #define APCI1564_WDOG_WARN_TIMEVAL_REG 0x40 #define APCI1564_WDOG_WARN_TIMEBASE_REG 0x44 #define APCI1564_TIMER_REG 0x48 #define APCI1564_TIMER_RELOAD_REG 0x4c #define APCI1564_TIMER_TIMEBASE_REG 0x50 #define APCI1564_TIMER_CTRL_REG 0x54 #define APCI1564_TIMER_STATUS_REG 0x58 #define APCI1564_TIMER_IRQ_REG 0x5c #define APCI1564_TIMER_WARN_TIMEVAL_REG 0x60 #define APCI1564_TIMER_WARN_TIMEBASE_REG0x64 /* * devpriv-iobase Register map */ #define APCI1564_TCW_REG(x) (0x00 + ((x) * 20)) #define APCI1564_TCW_RELOAD_REG(x) (0x04 + ((x) * 20)) #define APCI1564_TCW_TIMEBASE_REG(x)(0x08 + ((x) * 20)) #define APCI1564_TCW_CTRL_REG(x)(0x0c + ((x) * 20)) #define APCI1564_TCW_STATUS_REG(x) (0x10 + ((x) * 20)) #define APCI1564_TCW_IRQ_REG(x) (0x14 + ((x) * 20)) #define APCI1564_TCW_WARN_TIMEVAL_REG(x)(0x18 + ((x) * 20)) #define APCI1564_TCW_WARN_TIMEBASE_REG(x) (0x1c + ((x) * 20)) You will probably want to split this into a couple patches to make reviewing easier. Maybe do it by subdevice: digital input, digital output, watchdog, timer, then the counters. Regards, Hartley -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Staging: comedi: introduce outl_1564_* and inl_1564_* helper functions in hwdrv_apci1564.c
>On Saturday, March 1, 2014 6:18 PM, Chase Southwood > wrote: >Hi Dan, > [snip] > >I like this idea. Just to clarify though, basically all of the macros would >change to something like > >#define APCI1564_DIGITAL_IP 0x4 #define APCI1564_DIGITAL_IP_INTERRUPT_MODE1 >(0x4 + 0x4) >#define APCI1564_DIGITAL_IP_INTERRUPT_MODE2 (0x4 + 0x8) >#define >APCI1564_DIGITAL_IP_IRQ (0x4 + 0x10) #define APCI1564_DIGITAL_OP 0x18 >#define >APCI1564_DIGITAL_OP_RW 0x18 #define APCI1564_DIGITAL_OP_INTERRUPT >(0x18 + 0x4) #define >APCI1564_DIGITAL_OP_IRQ (0x18 + 0xc) >etc... [snip] Ah shoot, well that was a copy/paste trainwreck. Let's try that again, the macros would look something like: #define APCI1564_DIGITAL_IP 0x4 #define APCI1564_DIGITAL_IP_INTERRUPT_MODE1 (0x4 + 0x4) #define APCI1564_DIGITAL_IP_INTERRUPT_MODE2 (0x4 + 0x8) #define APCI1564_DIGITAL_IP_IRQ (0x4 + 0x10) #define APCI1564_DIGITAL_OP 0x18 #define APCI1564_DIGITAL_OP_RW 0x18 #define APCI1564_DIGITAL_OP_INTERRUPT (0x18 + 0x4) #define APCI1564_DIGITAL_OP_IRQ (0x18 + 0xc) There, that's better. Sorry about that. Thanks, Chase -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Staging: comedi: introduce outl_1564_* and inl_1564_* helper functions in hwdrv_apci1564.c
Hi Dan,
>On Saturday, March 1, 2014 6:46 AM, Dan Carpenter
>wrote:
>>On Sat, Mar 01, 2014 at 04:28:27AM -0600, Chase Southwood wrote:
>> This patch introduces a handful of outl and inl helper functions with the
>> ultimate goal of improving code readability and allowing several lines
>> which violate the character limit to be shortened in a sane way.
>>
>> Cc: Dan Carpenter
>> Signed-off-by: Chase Southwood
>> ---
>> This patchset serves as a replacement to my previous cleanup patchset for
>> hwdrv_apci1564.c
>>
>> Dan,
>> After spending a little bit more time with this and trying out different
>> ways of cleaning this up, I decided that making helper functions for all
>> of the most common register sets would look the best, but I haven't made
>> a helper for a few of the least common inl/outl calls because if I did,
>> the sheer number of helper functions would get quite ridiculous.
>> Let me know if you think my selections of what to make into helper
>> functions seems appropriate.
>>
>
>Yeah. You're right... It's kind of a lot of helper functions.
>
>I wonder if we could just do something like:
>
>static void outl_amcc(struct addi_private *devpriv, unsigned int cmd,
> unsigned int reg)
>{
> outl(cmd, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP + reg);
>}
>
>And then change APCI1564_DIGITAL_IP_INTERRUPT_MODE1 to be:
>
>#define APCI1564_DIGITAL_IP_INTERRUPT_MODE1 (0x4 + 0x4)
>
I like this idea. Just to clarify though, basically all of the macros would
change to something like
#define APCI1564_DIGITAL_IP 0x4 #define APCI1564_DIGITAL_IP_INTERRUPT_MODE1
(0x4 + 0x4) #define APCI1564_DIGITAL_IP_INTERRUPT_MODE2 (0x4 +
0x8) #define APCI1564_DIGITAL_IP_IRQ (0x4 + 0x10) #define APCI1564_DIGITAL_OP
0x18 #define APCI1564_DIGITAL_OP_RW 0x18 #define APCI1564_DIGITAL_OP_INTERRUPT
(0x18 + 0x4) #define APCI1564_DIGITAL_OP_IRQ (0x18 + 0xc)
etc... and then we just have the single helper function
(the corrected version from your follow up email) and then
the calls would be something to the effect of:
outl_amcc(devpriv, cmd, APCI1564_DIGITAL_IP_INTERRUPT_MODE1);
or whichever macro is appropriate? It definitely trims down
the length of the function calls by removing the dereference of
devpriv and the addition to get the proper register...I like that.
>
>The only problem with that would be i_APCI1564_Reset(). Is
>i_APCI1564_Reset() buggy? Ian or Hartley might know. Take a look at
>other comedi drivers as well to see what they do.
>
I agree. I'll look into the other addi-data drivers (the layout of each
appears pretty similar) or see if Ian or Hartley can shed more light on the
reset function, because I have a sneaking suspicion that a good few of the lines
in it already are buggy, and it seems like there's a chance that it's not
clearing
all of the registers that it should be, either. I could be wrong about that
though.
At any rate, I'll see what I can do.
Thanks,
Chase
>
>regards,
>
>dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Staging: comedi: introduce outl_1564_* and inl_1564_* helper functions in hwdrv_apci1564.c
On Sat, Mar 01, 2014 at 03:44:18PM +0300, Dan Carpenter wrote:
> On Sat, Mar 01, 2014 at 04:28:27AM -0600, Chase Southwood wrote:
> > This patch introduces a handful of outl and inl helper functions with the
> > ultimate goal of improving code readability and allowing several lines
> > which violate the character limit to be shortened in a sane way.
> >
> > Cc: Dan Carpenter
> > Signed-off-by: Chase Southwood
> > ---
> > This patchset serves as a replacement to my previous cleanup patchset for
> > hwdrv_apci1564.c
> >
> > Dan,
> > After spending a little bit more time with this and trying out different
> > ways of cleaning this up, I decided that making helper functions for all
> > of the most common register sets would look the best, but I haven't made
> > a helper for a few of the least common inl/outl calls because if I did,
> > the sheer number of helper functions would get quite ridiculous.
> > Let me know if you think my selections of what to make into helper
> > functions seems appropriate.
> >
>
> Yeah. You're right... It's kind of a lot of helper functions.
>
> I wonder if we could just do something like:
>
> static void outl_amcc(struct addi_private *devpriv, unsigned int cmd,
> unsigned int reg)
> {
> outl(cmd, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP + reg);
> }
>
Gar...
static void outl_amcc(struct addi_private *devpriv, unsigned int cmd,
unsigned int reg)
{
outl(cmd, devpriv->i_IobaseAmcc + reg);
}
regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Staging: comedi: introduce outl_1564_* and inl_1564_* helper functions in hwdrv_apci1564.c
On Sat, Mar 01, 2014 at 04:28:27AM -0600, Chase Southwood wrote:
> This patch introduces a handful of outl and inl helper functions with the
> ultimate goal of improving code readability and allowing several lines
> which violate the character limit to be shortened in a sane way.
>
> Cc: Dan Carpenter
> Signed-off-by: Chase Southwood
> ---
> This patchset serves as a replacement to my previous cleanup patchset for
> hwdrv_apci1564.c
>
> Dan,
> After spending a little bit more time with this and trying out different
> ways of cleaning this up, I decided that making helper functions for all
> of the most common register sets would look the best, but I haven't made
> a helper for a few of the least common inl/outl calls because if I did,
> the sheer number of helper functions would get quite ridiculous.
> Let me know if you think my selections of what to make into helper
> functions seems appropriate.
>
Yeah. You're right... It's kind of a lot of helper functions.
I wonder if we could just do something like:
static void outl_amcc(struct addi_private *devpriv, unsigned int cmd,
unsigned int reg)
{
outl(cmd, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP + reg);
}
And then change APCI1564_DIGITAL_IP_INTERRUPT_MODE1 to be:
#define APCI1564_DIGITAL_IP_INTERRUPT_MODE1 (0x4 + 0x4)
The only problem with that would be i_APCI1564_Reset(). Is
i_APCI1564_Reset() buggy? Ian or Hartley might know. Take a look at
other comedi drivers as well to see what they do.
regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Staging: comedi: introduce outl_1564_* and inl_1564_* helper functions in hwdrv_apci1564.c
On Sat, Mar 01, 2014 at 04:28:27AM -0600, Chase Southwood wrote:
This patch introduces a handful of outl and inl helper functions with the
ultimate goal of improving code readability and allowing several lines
which violate the character limit to be shortened in a sane way.
Cc: Dan Carpenter dan.carpen...@oracle.com
Signed-off-by: Chase Southwood chase.southw...@yahoo.com
---
This patchset serves as a replacement to my previous cleanup patchset for
hwdrv_apci1564.c
Dan,
After spending a little bit more time with this and trying out different
ways of cleaning this up, I decided that making helper functions for all
of the most common register sets would look the best, but I haven't made
a helper for a few of the least common inl/outl calls because if I did,
the sheer number of helper functions would get quite ridiculous.
Let me know if you think my selections of what to make into helper
functions seems appropriate.
Yeah. You're right... It's kind of a lot of helper functions.
I wonder if we could just do something like:
static void outl_amcc(struct addi_private *devpriv, unsigned int cmd,
unsigned int reg)
{
outl(cmd, devpriv-i_IobaseAmcc + APCI1564_DIGITAL_IP + reg);
}
And then change APCI1564_DIGITAL_IP_INTERRUPT_MODE1 to be:
#define APCI1564_DIGITAL_IP_INTERRUPT_MODE1 (0x4 + 0x4)
The only problem with that would be i_APCI1564_Reset(). Is
i_APCI1564_Reset() buggy? Ian or Hartley might know. Take a look at
other comedi drivers as well to see what they do.
regards,
dan carpenter
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Staging: comedi: introduce outl_1564_* and inl_1564_* helper functions in hwdrv_apci1564.c
On Sat, Mar 01, 2014 at 03:44:18PM +0300, Dan Carpenter wrote:
On Sat, Mar 01, 2014 at 04:28:27AM -0600, Chase Southwood wrote:
This patch introduces a handful of outl and inl helper functions with the
ultimate goal of improving code readability and allowing several lines
which violate the character limit to be shortened in a sane way.
Cc: Dan Carpenter dan.carpen...@oracle.com
Signed-off-by: Chase Southwood chase.southw...@yahoo.com
---
This patchset serves as a replacement to my previous cleanup patchset for
hwdrv_apci1564.c
Dan,
After spending a little bit more time with this and trying out different
ways of cleaning this up, I decided that making helper functions for all
of the most common register sets would look the best, but I haven't made
a helper for a few of the least common inl/outl calls because if I did,
the sheer number of helper functions would get quite ridiculous.
Let me know if you think my selections of what to make into helper
functions seems appropriate.
Yeah. You're right... It's kind of a lot of helper functions.
I wonder if we could just do something like:
static void outl_amcc(struct addi_private *devpriv, unsigned int cmd,
unsigned int reg)
{
outl(cmd, devpriv-i_IobaseAmcc + APCI1564_DIGITAL_IP + reg);
}
Gar...
static void outl_amcc(struct addi_private *devpriv, unsigned int cmd,
unsigned int reg)
{
outl(cmd, devpriv-i_IobaseAmcc + reg);
}
regards,
dan carpenter
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Staging: comedi: introduce outl_1564_* and inl_1564_* helper functions in hwdrv_apci1564.c
Hi Dan,
On Saturday, March 1, 2014 6:46 AM, Dan Carpenter dan.carpen...@oracle.com
wrote:
On Sat, Mar 01, 2014 at 04:28:27AM -0600, Chase Southwood wrote:
This patch introduces a handful of outl and inl helper functions with the
ultimate goal of improving code readability and allowing several lines
which violate the character limit to be shortened in a sane way.
Cc: Dan Carpenter dan.carpen...@oracle.com
Signed-off-by: Chase Southwood chase.southw...@yahoo.com
---
This patchset serves as a replacement to my previous cleanup patchset for
hwdrv_apci1564.c
Dan,
After spending a little bit more time with this and trying out different
ways of cleaning this up, I decided that making helper functions for all
of the most common register sets would look the best, but I haven't made
a helper for a few of the least common inl/outl calls because if I did,
the sheer number of helper functions would get quite ridiculous.
Let me know if you think my selections of what to make into helper
functions seems appropriate.
Yeah. You're right... It's kind of a lot of helper functions.
I wonder if we could just do something like:
static void outl_amcc(struct addi_private *devpriv, unsigned int cmd,
unsigned int reg)
{
outl(cmd, devpriv-i_IobaseAmcc + APCI1564_DIGITAL_IP + reg);
}
And then change APCI1564_DIGITAL_IP_INTERRUPT_MODE1 to be:
#define APCI1564_DIGITAL_IP_INTERRUPT_MODE1 (0x4 + 0x4)
I like this idea. Just to clarify though, basically all of the macros would
change to something like
#define APCI1564_DIGITAL_IP 0x4 #define APCI1564_DIGITAL_IP_INTERRUPT_MODE1
(0x4 + 0x4) #define APCI1564_DIGITAL_IP_INTERRUPT_MODE2 (0x4 +
0x8) #define APCI1564_DIGITAL_IP_IRQ (0x4 + 0x10) #define APCI1564_DIGITAL_OP
0x18 #define APCI1564_DIGITAL_OP_RW 0x18 #define APCI1564_DIGITAL_OP_INTERRUPT
(0x18 + 0x4) #define APCI1564_DIGITAL_OP_IRQ (0x18 + 0xc)
etc... and then we just have the single helper function
(the corrected version from your follow up email) and then
the calls would be something to the effect of:
outl_amcc(devpriv, cmd, APCI1564_DIGITAL_IP_INTERRUPT_MODE1);
or whichever macro is appropriate? It definitely trims down
the length of the function calls by removing the dereference of
devpriv and the addition to get the proper register...I like that.
The only problem with that would be i_APCI1564_Reset(). Is
i_APCI1564_Reset() buggy? Ian or Hartley might know. Take a look at
other comedi drivers as well to see what they do.
I agree. I'll look into the other addi-data drivers (the layout of each
appears pretty similar) or see if Ian or Hartley can shed more light on the
reset function, because I have a sneaking suspicion that a good few of the lines
in it already are buggy, and it seems like there's a chance that it's not
clearing
all of the registers that it should be, either. I could be wrong about that
though.
At any rate, I'll see what I can do.
Thanks,
Chase
regards,
dan carpenter
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Staging: comedi: introduce outl_1564_* and inl_1564_* helper functions in hwdrv_apci1564.c
On Saturday, March 1, 2014 6:18 PM, Chase Southwood chase.southw...@yahoo.com wrote: Hi Dan, [snip] I like this idea. Just to clarify though, basically all of the macros would change to something like #define APCI1564_DIGITAL_IP 0x4 #define APCI1564_DIGITAL_IP_INTERRUPT_MODE1 (0x4 + 0x4) #define APCI1564_DIGITAL_IP_INTERRUPT_MODE2 (0x4 + 0x8) #define APCI1564_DIGITAL_IP_IRQ (0x4 + 0x10) #define APCI1564_DIGITAL_OP 0x18 #define APCI1564_DIGITAL_OP_RW 0x18 #define APCI1564_DIGITAL_OP_INTERRUPT (0x18 + 0x4) #define APCI1564_DIGITAL_OP_IRQ (0x18 + 0xc) etc... [snip] Ah shoot, well that was a copy/paste trainwreck. Let's try that again, the macros would look something like: #define APCI1564_DIGITAL_IP 0x4 #define APCI1564_DIGITAL_IP_INTERRUPT_MODE1 (0x4 + 0x4) #define APCI1564_DIGITAL_IP_INTERRUPT_MODE2 (0x4 + 0x8) #define APCI1564_DIGITAL_IP_IRQ (0x4 + 0x10) #define APCI1564_DIGITAL_OP 0x18 #define APCI1564_DIGITAL_OP_RW 0x18 #define APCI1564_DIGITAL_OP_INTERRUPT (0x18 + 0x4) #define APCI1564_DIGITAL_OP_IRQ (0x18 + 0xc) There, that's better. Sorry about that. Thanks, Chase -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
