Re: [PATCH 2/3] staging: comedi: amplc_dio200: add helper macros to check bus type

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux