On Sun, Mar 09, 2025 at 12:46:55PM -0700, Aidan K. Wiggins via 9fans wrote:
> Hi Thierry,
>
> > Realtek uses the two masks for the macver: 0xFCF00000 to do what is
> > done (limitedly) with the macver on the current driver, and to set the
> > MACFG (MAC firmware conFiG?) type. The second mask 0x7C800000 is used
> > to refine with subconfiguration. I think we should do the same: the
> > general matching first; and handling the special cases, apart, after
> > without looping again.
>
> I think I understand what you're getting at here: instead of having an
> enum constant for every little card, we should be using the MACFG
> values as indicated by the result of ANDing with 0xFCF000000, as seen
> in re_check_mac_version() of Realtek's BSD source. I think there is
> merit in this approach, but it would require changing more of the code
> than I thought immediately necessary, which is partially why I just
> decided to follow suit in borrowing values from Linux (esp. with it's
> large userbase included). With looping in our vetmacv(), I was
> attempting to keep some level of guaranteed backwards compatibility,
> since I wasn't sure if a more specific (0xFCF) mask would tease out a
> new Macv value from an older card that was otherwise previously
> supported under the mask 0x7C800000.
>
The 8125 will add so much code that I think we can depart for some
things from the current state. Since the first test is about the PCI
device ID, before verifying the macver, the problems should be unlikely.
But my mind is not set for now about the style. What I want to find is
something both small and clear enough for anyone having to work with
the driver to understand rapidly what is done. The Linux team seems to
be working to some extend with the hardware producers, so I will, too,
keep also the Linux driver as a guide---but I give a kind of priority
to the Realtek provided one because they are supposed to know what
they do produce ;-)
One could also define flags and have a struct with the hardware
version and a definition of the corresponding flags, with the
initialization simply done if the flag if set.
> > What I'm unclear about at the moment is the Wifi support.
>
> Can you expand on this?
>
My card at least, has a RJ45 but provides wireless connection too. I have
to find if the distinction is the device ID, or the function (or a
revision), and if the Realtek driver has some parts dedicated to wireless
configuration.
So the question: are the wireless features mixed with this and should
be done "here", or can be postpone being supposedly dealt with by
another driver?
> > More generally, wouldn't it be more clear to use PCI_VENDOR_ID,
> > PCI_DEVICE_ID,
> > etc. from a header instead of the hardcoded values? (that may be hard
> > to grep to find if support is there or not---I missed at first that
> > there is a U.S. Robotics card with a Realtek chip in the list since
> > the vendor id is 0x16EC, to be compared with Realtek 0x10EC.)
>
> I agree with Ori's approach here, the device IDs are so specific that
> they really only need inclusion in their respective drivers. We do,
> however, have some vendor IDs in pci.h, so you could have:
>
> #define PCIdev(vid, did) ((vid)<<16|(did))
> enum{
> Rtl8169 = PCIdev(Vrealtek, 0x8169),
> Rtl8169c= PCIdev(0x16ec, 0x0116)
> ...
> }
>
> switch(PCIdev(p->vid, p->did)){
> ...
> }
Yep, I like it more this way!
>
> If it weren't for these non-realtek chips, I think we would just use
> switch(did){}.
>
The problem is that there is two "alien" cards: the US ROBOTICS, set
in the enum, and one hard coded in the switch (to fall back to the
Rtl8169 case):
case (0xC107<<16)|0x1259: /* Corega CG-LAPCIGT */
(These are the exceptions that we will have to verify we are not dropping
by inadvertence.)
--
Thierry Laronde <tlaronde +AT+ kergis +dot+ com>
http://www.kergis.com/
http://kertex.kergis.com/
Key fingerprint = 0FF7 E906 FBAF FE95 FD89 250D 52B1 AE95 6006 F40C
------------------------------------------
9fans: 9fans
Permalink:
https://9fans.topicbox.com/groups/9fans/T832e366730c74bfa-M7c40e88b71cc8ae8dd406a0a
Delivery options: https://9fans.topicbox.com/groups/9fans/subscription
