[PATCH 8/8] staging: comedi: amplc_pc236: Add attach_pci() hook

[Dan Carpenter]

On Wed, May 30, 2012 at 12:51:31PM +0100, Ian Abbott wrote:
> On 2012-05-29 15:45, Dan Carpenter wrote:
> >On Tue, May 29, 2012 at 02:49:35PM +0100, Ian Abbott wrote:
> >>Implement the attach_pci() hook as function pc236_attach_pci().  This is
> >>called bu comedi_pci_auto_config() in preference to the old attach()
> >>hook (implemented by pc236_attach()) and avoids searching for the probed
> >>PCI device.
> >>
> >>Factor out code common to pc236_find_pci() and pc236_attach_pci() into
> >>new function pc236_find_pci_board().  Factor out most code common to
> >>pc236_attach() and pc236_attach_pci() into new functions
> >>pc236_pci_common_attach() and pc236_common_attach().
> >>
> >>Also #if out member 'devid' from struct pc236_board unless PCI boards
> >>are supported as it is not used for ISA boards.
> >>
> >
> >I can't tell you how much I hate these ifdefs.  :(  They're every
> >where...  Who is it that cares about cares so much about 3 extra
> >unsigned shorts?
> >
> >The rules are that #ifdefs are only ok in .h files and not in .c
> >files.  We should be removing them and not adding them if we want
> >to move this code out of staging.
> 
> As an experiment, with the following patch (sorry for the
> line-wrapping which prevents it being applied) and with
> CONFIG_COMEDI_AMPLC_PC236_ISA defined and
> CONFIG_COMEDI_AMPLC_PC236_PCI not defined, the size of the
> amplc_pc236.ko increased in size from 5785 bytes to 6215 bytes and
> there was a compiler warning about pc236_attach_pci being defined
> but not used (this is what the .attach_pci hook in struct
> comedi_driver amplc_pc236_driver would get set to, but that is still
> conditionally compiled out).  If I set the .attach_pci hook to
> pc236_attach_pci regardless, it gets rid of the warning, but the
> size of the amplc_pc236.ko file increased to 6776 bytes as more code
> is reachable in theory although not in practice as the PCI probe
> code that calls this function indirectly is still conditionally
> compiled out.
> 

I was talking about the devid.  And yeah.  It shaves of 40 bytes to
put the #ifdef there.  I can only see three structs that are
affected so I thought it would be less.  But you you never answered
my question about who cares???

The reason this code is in staging is not because it uses a lot of
memory but because it's a mess.  Before we move it to the stock
kernel we will need to remove all the ifdefs out of the .c files.

That whole amplc_pc236.c file is riddled with nasty nasty nasty.

And no it's not the right idea to sed replace them if (IS_ENABLED()).

regards,
dan carpenter