On 19 October 2016 at 05:32, Alan Ott alan@softiron.co.uk wrote:
On 10/17/2016 05:45 PM, Daniil Egranov wrote:
The patch adds support for multi-controller configuration.
Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Daniil Egranov daniil.egranov@arm.com
I tested this, and it does work on my hardware here.
I guess my biggest criticism of this patch is, why change all the function prototypes to take mSoftc, mController, and mPciIo parameters? In the FreeBSD[0] driver, the msk_softc is simply held by the msk_if_softc (which is the main object for each network controller (interface)). The PCI I/O could be held there too. Each function that needs a msk_softc can simply get it from the msk_if_softc when it needs it.
The FreeBSD driver, for the most part, passes around msk_if_softc pointers, which makes sense, since from those pointers you can get to everything else.
Is there a reason that would not work here? It seems like a much cleaner interface an fewer variables to keep track of and it wouldn't have to change function interfaces to have redundant parameters.
For example, have a look at FreeBSD's msk_phy_readreg() at [1]. It takes a msk_if_softc pointer and gets the msk_softc from it when it needs it. Your version of msk_phy_readreg() takes an mController parameter and then does a lookup of the msk_softc from a list[2] (every time you want to read a register!). The calling functions _have_ the msk_if_softc already. Just make msk_phy_readreg() take a msk_if_softc pointer (like the BSD driver does), and there would be no lookup required.
Thanks Alan.
You guys know this code way better than I do, but I tend to agree with Alan that
a) it makes sense to follow the FreeBSD code more closely b) the patch in its current form mixes too many types of changes c) adding linked lists to look up values that could be obtained in a more straightforward way seems unwise.