On Sunday 08 November 2009 19:47:53 Bartlomiej Zolnierkiewicz wrote:
> On Sunday 08 November 2009 19:40:27 Ivo van Doorn wrote:
> > On Sunday 08 November 2009, Bartlomiej Zolnierkiewicz wrote:
> > > On Sunday 08 November 2009 19:27:30 Ivo van Doorn wrote:
> > > > On Sunday 08 November 2009, Bartlomiej Zolnierkiewicz wrote:
> > > > > On Sunday 08 November 2009 19:08:23 Ivo van Doorn wrote:
> > > > > > On Sunday 08 November 2009, Bartlomiej Zolnierkiewicz wrote:
> > > > > > > On Sunday 08 November 2009 14:55:59 Ivo van Doorn wrote:
> > > > > > > > On Sunday 08 November 2009, Bartlomiej Zolnierkiewicz wrote:
> > > > > > > > > From: Bartlomiej Zolnierkiewicz <bzolnier@xxxxxxxxx>
> > > > > > > > > Subject: [PATCH] rt2800: add eFuse EEPROM support code to rt2800lib
> > > > > > > > >
> > > > > > > > > eFuse EEPROM is used also by USB chips (i.e. RT3070)
> > > > > > > > > so move the needed code from rt2800pci to rt2800lib.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@xxxxxxxxx>
> > > > > > > > > ---
> > > > > > > > > drivers/net/wireless/rt2x00/rt2800.h | 29 +++++++++++++++++++++
> > > > > > > > > drivers/net/wireless/rt2x00/rt2800lib.c | 43 ++++++++++++++++++++++++++++++++
> > > > > > > > > drivers/net/wireless/rt2x00/rt2800lib.h | 2 +
> > > > > > > > > drivers/net/wireless/rt2x00/rt2800pci.c | 38 ++--------------------------
> > > > > > > > > drivers/net/wireless/rt2x00/rt2800pci.h | 29 ---------------------
> > > > > > > > > 5 files changed, 77 insertions(+), 64 deletions(-)
> > > > > > > >
> > > > > > > >
> > > > > > > > > ===================================================================
> > > > > > > > > --- a/drivers/net/wireless/rt2x00/rt2800pci.c
> > > > > > > > > +++ b/drivers/net/wireless/rt2x00/rt2800pci.c
> > > > > > > > > @@ -147,44 +147,12 @@ static void rt2800pci_read_eeprom_pci(st
> > > > > > > > >
> > > > > > > > > static int rt2800pci_efuse_detect(struct rt2x00_dev *rt2x00dev)
> > > > > > > > > {
> > > > > > > > > - u32 reg;
> > > > > > > > > -
> > > > > > > > > - rt2800_register_read(rt2x00dev, EFUSE_CTRL, ®);
> > > > > > > > > -
> > > > > > > > > - return rt2x00_get_field32(reg, EFUSE_CTRL_PRESENT);
> > > > > > > > > + return rt2800_efuse_detect(rt2x00dev);
> > > > > > > > > }
> > > > > > > >
> > > > > > > > It would be better to fix all calls to rt2800pci_efuse_detect to use rt2800_efuse_detect
> > > > > > > > rather then adding a special wrapper function for it.
> > > > > > > >
> > > > > > > > > -static void rt2800pci_read_eeprom_efuse(struct rt2x00_dev *rt2x00dev)
> > > > > > > > > +static inline void rt2800pci_read_eeprom_efuse(struct rt2x00_dev *rt2x00dev)
> > > > > > > > > {
> > > > > > > > > - unsigned int i;
> > > > > > > > > -
> > > > > > > > > - for (i = 0; i < EEPROM_SIZE / sizeof(u16); i += 8)
> > > > > > > > > - rt2800pci_efuse_read(rt2x00dev, i);
> > > > > > > > > + rt2800_read_eeprom_efuse(rt2x00dev);
> > > > > > > > > }
> > > > > > > >
> > > > > > > > Same here.
> > > > > > >
> > > > > > > Could you please explain some more what do you mean by that?
> > > > > > > (Please note that we have an extra SOC handling in rt2800pci.c.)
> > > > > >
> > > > > > Your changes made the following 2 functions:
> > > > > >
> > > > > > int rt2800pci_efuse_detect(struct rt2x00_dev *rt2x00dev)
> > > > > > {
> > > > > > return rt2800_efuse_detect(rt2x00dev);
> > > > > > }
> > > > > >
> > > > > > void rt2800pci_read_eeprom_efuse(struct rt2x00_dev *rt2x00dev)
> > > > > > {
> > > > > > rt2800_read_eeprom_efuse(rt2x00dev);
> > > > > > }
> > > > > >
> > > > > > So why do we need rt2800pci_* versions in this case? They simply wrap
> > > > > > the rt2800 library function without providing anything extra...
> > > > >
> > > > > Please go read the original code..
> > > > >
> > > > > #ifdef CONFIG_RT2800PCI_PCI
> > > > > ...
> > > > > [ the code quoted in your mail ]
> > > > > ...
> > > > > #else
> > > > > static inline void rt2800pci_read_eeprom_pci(struct rt2x00_dev *rt2x00dev)
> > > > > {
> > > > > }
> > > > >
> > > > > static inline void rt2800pci_read_eeprom_efuse(struct rt2x00_dev *rt2x00dev)
> > > > > {
> > > > > }
> > > > > #endif /* CONFIG_RT2800PCI_PCI */
> > > > >
> > > > > [ #else is for CONFIG_RT2800PCI_WISOC ]
> > > >
> > > > True, but rt2800pci_read_eeprom_efuse() has no WISOC counterpart,
> > > > the fact that it is compiled into rt2x00lib without any restriction makes
> > > > the ifdef statements in the rt2800pci obsolete.
> > >
> > > Not really. Please note that driver's behavior WRT WISOC is preserved.
> >
> > The behavior, not the amount of code compiled into the binary.
> > The original code had the efuse code within the ifdef, during the move
> > to rt2800lib it was moved outside the ifdef.
> >
> > > > The purpose of the defines was to keep the EFUSE code out of the driver
> > > > on embedded systems. So either rt2800lib should do the same with ifdefs
> > > > in the rt2800lib.c and rt2800lib.h files, or we don't need the efuse specific
> > > > wrappers in rt2800pci.c.
> > >
> > > Said wrappers predate all of my rt2800 patches.
> >
> > Actually the code inside the function was larger, since you removed multiple
> > lines from the function (moved to rt2800lib). But as said above, the wrapper
> > had the code inside the ifdef, while the this patch has the code outside the
> > ifdef, and thus the wrapper has no need.
> >
> > > I think that they shouldn't have been added in the first place and I'll be
> > > happy to add patch removing them to rt2800 tree (since code savings seem to
> > > be really marginal and not worth the maintenance cost).
> >
> > Removing the ifdefs entirely would be fine.
>
> Like I said before -- this would mean driver's behavior change. Even though
> WISOC code is currently dead (RALINK_RT288X and RALINK_RT305X are never set)
> I prefer to not "overload" patches with logically different changes.
>
> If you feel strongly about it please fix it in rt2x00 code and rt2800 tree
> will deal with it, or alternatively please send me an incremental patch.
BTW the patch's impact is _320_ bytes increase of rt2800lib (on x86-64 so
it is probably much less on the affected embedded architectures):
text data bss dec hex filename
before:
16916 0 0 16916 4214 drivers/net/wireless/rt2x00/rt2800lib.o
after:
17281 0 0 17281 4381 drivers/net/wireless/rt2x00/rt2800lib.o
for the _completely_ dead code (because embedded WISOC support is never
enabled) that probably will be changed over anyway later during development.
IOW something like this is completely not worth to be worried about
and I would personally just skip it during review to not waste people's
time needlessly..
--
Bartlomiej Zolnierkiewicz
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
