On Tue, 23 Jul 2019 04:08:23 +0000 Joe Hershberger joe.hershberger@ni.com wrote: ...
-#define DEVADDR_PORT(p) (0x10 + (p)) +#define DEVADDR_PORT(p) (priv->port_reg_base + (p))
This is an obtuse macro. It should not reference a local variable internally. If you want to use priv, pass it as a parameter.
Okay.
struct mv88e61xx_phy_priv { struct mii_dev *mdio_bus; int smi_addr; int id;
int port_count;int port_reg_base;u8 global1;u8 global2;I think this could stand some commenting. global what? Why 2?
I'll add some comments here.
...
static int mv88e61xx_phy_wait(struct phy_device *phydev) {
struct mv88e61xx_phy_priv *priv = phydev->priv; int val; u32 timeout = 100; do {
val = mv88e61xx_reg_read(phydev, DEVADDR_GLOBAL_2,
val = mv88e61xx_reg_read(phydev, priv->global2,Probably just use phydev->priv->global2 instead of the local variable.
Done in v3.
...
static int mv88e61xx_port_read(struct phy_device *phydev, u8 port, u8 reg) {
struct mv88e61xx_phy_priv *priv = phydev->priv;Huh? When casually read, this seems useless.
Okay, I'll remove DEVADDR_PORT() macro.
...
static int mv88e61xx_port_write(struct phy_device *phydev, u8 port, u8 reg, u16 val) {
struct mv88e61xx_phy_priv *priv = phydev->priv;Huh? Ditto.
Will fix it.
...
+static void mv88e61xx_priv_reg_offs_pre_init(struct mv88e61xx_phy_priv *priv) +{
/** Initial 'port_reg_base' value must be in the port register,* map and the global_N register offsets must be correct,They are globalN, not global_N. What is the map that this referenced? Please reword.
Okay, done in v3.
* otherwise detection of switch ID won't work!That seems likely... but this looks very magic. Please document it a little better and if possible reference any public datasheets or reference manuals.
Will add more comments here. The datasheets or manuals are all under NDA, I can't add references here.
*/+#ifndef CONFIG_MV88E61XX_88E6020_FAMILY
priv->global1 = 0x1B;priv->global2 = 0x1C;priv->port_reg_base = 0x10;+#else
priv->global1 = 0x0F;priv->global2 = 0x07;priv->port_reg_base = 0x08;+#endif
I think it would be much cleaner if the contents of this function and the following switch case in the next function were implemented as driver data selections in the udevice_id structure and then used from there.
The problem is that this offset pre-initialisation is also required before the actual driver probing (i.e. calls of get_phy_id() from PHY framework).
...
@@ -1068,6 +1123,8 @@ int get_phy_id(struct mii_dev *bus, int smi_addr, int devad, u32 *phy_id) temp_phy.priv = &temp_priv; temp_mii.priv = &temp_phy;
mv88e61xx_priv_reg_offs_pre_init(&temp_priv);Ick.
will add a comment why it is required here.
-- Anatolij