Re: [PATCH 4/9] rt2x00: Remove rt2800 version constants.

[Ivo van Doorn <ivdoorn@gmail.com>]

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@gmail.com> wrote:
> >> On Fri, Apr 9, 2010 at 1:53 AM, Julian Calaby <julian.calaby@gmail.com> wrote:
> >>> On Fri, Apr 9, 2010 at 08:32, Ivo van Doorn <ivdoorn@gmail.com> 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@gmail.com>
> >>>>
> >>>> 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