On Wed, Aug 15, 2012 at 01:40:01PM +0100, Ian Abbott wrote:
> On 2012-08-15 11:48, Dan Carpenter wrote:
> >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:
> >>>>+#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
>
> >>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.
>
> The remaining #ifdefs are only used to fill in the list of supported
> boards, the static "layout" data referenced by the supported boards
> (the #ifdefs could be removed from the layout array with a moderate
> expansion of module size), and to optionally make it a PCI driver.
Hm... You're right. I guess that's not so bad then.
Except I really hate the DO_ISA macro. It's clearer (more readable)
to just use IS_ENABLED(). The cool thing about IS_ENABLED() is that
it looks like a function and it shows you which CONFIG option has
to be enabled. DO_ISA is a horrible name and a horrible macro.
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
