On Wed, Aug 15, 2012 at 11:16:20AM +0100, Ian Abbott wrote:
> On 2012-08-15 08:14, Dan Carpenter wrote:
> >On Tue, Aug 14, 2012 at 04:31:28PM +0100, Ian Abbott wrote:
> >>Add helper macro IS_ISA_BOARD(board) to check if the driver supports ISA
> >>boards and this is an ISA board, and IS_PCI_BOARD(board) to check if the
> >>driver supports PCI boards and this is a PCI board.
> >>
> >>Signed-off-by: Ian Abbott <abbotti@xxxxxxxxx>
> >>---
> >> drivers/staging/comedi/drivers/amplc_dio200.c | 11 +++++++----
> >> 1 files changed, 7 insertions(+), 4 deletions(-)
> >>
> >>diff --git a/drivers/staging/comedi/drivers/amplc_dio200.c b/drivers/staging/comedi/drivers/amplc_dio200.c
> >>index 0905e40..3a7f592 100644
> >>--- a/drivers/staging/comedi/drivers/amplc_dio200.c
> >>+++ b/drivers/staging/comedi/drivers/amplc_dio200.c
> >>@@ -294,6 +294,9 @@ struct dio200_board {
> >> enum dio200_layout layout;
> >> };
> >>
> >>+#define IS_ISA_BOARD(board) (DO_ISA && (board)->bustype == isa_bustype)
> >>+#define IS_PCI_BOARD(board) (DO_PCI && (board)->bustype == pci_bustype)
> >
> >It would be better to make this an inline function.
> >
> >#if IS_ENABLED(CONFIG_COMEDI_AMPLC_DIO200_ISA)
> >
> >static inline bool is_isa_board(const struct dio200_board *board)
> >{
> > return board->bustype == isa_bustype;
> >}
> >
> >#else
> >
> >static inline bool is_isa_board(const struct dio200_board *board)
> >{
> > return 0;
> >}
> >
> >#endif
> >
> >What I'm trying to explain is that this would be a small step
> >towards moving the #if statements out of the .c file and getting
> >comedi out of staging. We need to do that for all the #if
> >statements.
>
> It looks like you're *adding* a #if in the above. I suppose you could have:
>
It's not. You just have *one* #ifdef which is written in the
correct way (in the header file) and *everything* that depends on
ISA goes inside that #if. All the other ifdefs are removed.
This is not something I made up. Read
Documentation/SubmittingPatches. The correct way to do ifdefs is
the #2 section. Put them in a header with static inlines as I
described.
> static inline bool is_isa_board(const struct dio200_board *board)
> {
> return DO_ISA && board->bustype == isa_bustype;
> }
>
> I prefer the macro as it seems "easier" for the compiler to
> recognize it as a constant in the case where DO_ISA expands to 0.
>
If we were just discussing this one function then, sure, we're not
going to complain about small style issues, but it's the whole file.
It's just riddled with #ifdefs. It can't be moved out of staging
until it is written according the same coding style as the rest of
the kernel.
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
