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
