On 04/09/10 09:00, Julian Calaby wrote: > On Fri, Apr 9, 2010 at 16:54, Ivo Van Doorn <ivdoorn@xxxxxxxxx> wrote: >> On Fri, Apr 9, 2010 at 1:53 AM, Julian Calaby <julian.calaby@xxxxxxxxx> wrote: >>> On Fri, Apr 9, 2010 at 08:32, Ivo van Doorn <ivdoorn@xxxxxxxxx> wrote: >>>> On Thursday 08 April 2010, Gertjan van Wingerde wrote: >>>>> The rt2800 version constants are inconsistent, and the version number don't >>>>> mean a lot of things anyway. Use the literal values in the code instead of >>>>> some sort of fabricated version name macro. >>>>> >>>>> Signed-off-by: Gertjan van Wingerde <gwingerde@xxxxxxxxx> >>>> >>>> Perhaps a more elegant way of using and defining needs to be found. >>>> But at least the defined show what the purpose for the values is >>>> rather then having magical values spread around the code. >>> >>> Maybe something like: >>> >>> #define RTDEV_IS_RT2883_R1(dev) (rt2x00_rt(dev, RT2883) && \ >>> rt2x00_rev(dev) < 0x0300) >> >> I considered this as well, but we have many checks which either do >> rt2x00_rev() < 0xffff >> but also >> rt2x00_ref() == 0xffff > > I assume that there are certain ranges of revisions that correspond to > certain chips with certain ... features. So, you could have: > > #define RTDEV_IS_RT2883_R1(dev) (rt2x00_rt(dev, RT2883) && \ > rt2x00_rev(dev) < 0x0300) > > for the original chip, then > > #define RTDEV_IS_RT2883_R2(dev) (rt2x00_rt(dev, RT2883) && \ > rt2x00_rev(dev) >= 0x0300 && \ > rt2x00_rev(dev) < 0x1000) > > for the troubled second version and > > #define RTDEV_IS_RT2883_R3(dev) (rt2x00_rt(dev, RT2883) && \ > rt2x00_rev(dev) >= 0x1000) > > as a catch all for newer chips. OK. After spending the entire morning trying to understand the Ralink numbering scheme, I think I have come up with some sensible constant naming scheme and a set of helpers to clarify the code. As you can see below there is no constant numbering scheme for all chipsets, and the revision codes are chipset specific. First of all, we would define the following chipset revision constants: #define REV_RT2860C 0x0100 #define REV_RT2860D 0x0101 #define REV_RT2870D 0x0101 #define REV_RT2872E 0x0200 #define REV_RT3070E 0x0200 #define REV_RT3070F 0x0201 #define REV_RT3071E 0x0211 #define REV_RT3090E 0x0211 #define REV_RT3390E 0x0211 Next, we would create three helper functions: bool rt2x00_rt_rev(struct rt2x00_dev *rt2x00dev, u16 rt, u16 rev); bool rt2x00_less_than_rt_rev(struct rt2x00_dev *rt2x00dev, u16 rt, u16 rev); bool rt2x00_at_least_rt_rev(struct rt2x00_dev *rt2x00dev, u16 rt, u16 rev); This would cover all the usages that we currently have inside the code. Obviously more can be added once needed. Are you guys OK with this proposal? If yes, I'll update the series to change the version constants removal patch into a patch converting to the above scheme. --- Gertjan. -- 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
