On Mon, Oct 25, 2021 at 6:58 AM Marek Behún kabel@kernel.org wrote:
NAK for this driver.
- display_banner() spams the output unnecessarily, the information should be printed with debug()
We will make the change as requested.
you are introducing custom mechanism for setting / getting PHY parameters, via custom specific env variables, for example in the set_phy_speed() and set_phy_link() functions, i.e.: sprintf(name1, "bnxt_eth%u_phy_speed", bp->cardnum); env_set(name1, name);
The whole point of several people in the past few years was to create generic mechanisms for such things. We have ethernet PHY DM class, you should use this. That way you won't need to introduce custom mechanisms to get the infromation, since there are mii/mdio commands.
These are chip internal settings stored internally in NVM. They are not modified via mii/mdio.
- print_mac() - the driver shouldn't even have this function, it should just set appropriate ethNaddr variable
The function was added for user convenience. We will remove it.
in bnxt_eth_probe() you are looking at the variable "ethaddr":
if (env_get("ethaddr")) secondary = 1;a driver should never look itself at this variable. Since your driver should be of UCLASS_ETH, the generic mechanism should use appropriate env variable by calling you .write_hwaddr method
We will try to modify it.
Basically you are going against all the points of the whole idea to have a generic API to set network driver parameters, and instead you are adding driver-specific custom mechanisms.
Please change that in next version.
Marek
Thank you very much for your review,
Roman