On Saturday 10 April 2010, Gertjan van Wingerde wrote: > 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 Looks good, but could you make the chipset versioning naming similar to that what is used in rt2500pci and rt2500usb? > 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); I think we can shorten these names with: rt2x00_rt_rev rt2x00_rt_rev_lt rt2x00_rt_rev_gt > 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. Yes, this implementation would be fine. :) Thanks, Ivo -- 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
