Hi Andrew, Andrew Lunn <and...@lunn.ch> writes:
> The mv88e6390 has two MDIO busses. The internal MDIO bus is used for > the internal PHYs. The external MDIO can be used for external PHYs. > The external MDIO bus will be instantiated if there is an > "mdio-external" node in the device tree. Thanks for pushing the 88E6390 support. Some comments below. > +static int mv88e6xxx_read_phy(struct mv88e6xxx_chip *chip, int addr, int reg, > +static int mv88e6xxx_write_phy(struct mv88e6xxx_chip *chip, int addr, int > reg, > static int mv88e6xxx_phy_read(struct mv88e6xxx_chip *chip, int phy, > static int mv88e6xxx_phy_write(struct mv88e6xxx_chip *chip, int phy, Adding mv88e6xxx_read/write_phy() in addition to existing mv88e6xxx_phy_read/write() feels really confusing and hard to maintain. Can that be done the other way around maybe? > +static int mv88e6xxx_external_mdio_register(struct mv88e6xxx_chip *chip, > + struct device_node *np) > +static void mv88e6xxx_external_mdio_unregister(struct mv88e6xxx_chip *chip) > static int mv88e6xxx_mdio_register(struct mv88e6xxx_chip *chip, > struct device_node *np) We already have mv88e6xxx_mdio_register/unregister(). Isn't it possible to tweak them to take a struct mv88e6xxx_mdio_bus instance and use them twice for both internal and external MDIO busses? > + if (mv88e6xxx_has(chip, MV88E6XXX_FLAG_G2_EXTERNAL_MDIO)) { > + err = mv88e6xxx_external_mdio_register(chip, np); > + if (err) > + goto out_mdio; > + } We are trying to get rid of the flags and family checks... Please don't add new ones. If the external MDIO bus is a new feature of switches like 88E6390, isn't it better to add new external_phy_read/write ops and register the bus if they are provided? if (chip->info->ops->external_phy_read) { struct mv88e6xxx_mdio_bus *external_mdio_bus; ... err = mv88e6xxx_mdio_register(external_mdio_bus); if (err) ... } Thanks, Vivien