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.
--
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
