Hi Sander, I'll hold off on sending v7 until I sort out the mess I've made with the dt-bindings (spun off into a different series [1]) [1] - https://lore.kernel.org/lkml/20250209234751.460404-1-chris.packham@xxxxxxxxxxxxxxxxxxx/ On 10/02/2025 03:19, Sander Vanheule wrote: > Hi Chris, > > On Tue, 2025-02-04 at 16:02 +1300, Chris Packham wrote: >> Add a driver for the MDIO controller on the RTL9300 family of Ethernet >> switches with integrated SoC. There are 4 physical SMI interfaces on the >> RTL9300 however access is done using the switch ports. The driver takes >> the MDIO bus hierarchy from the DTS and uses this to configure the >> switch ports so they are associated with the correct PHY. This mapping >> is also used when dealing with software requests from phylib. >> >> Signed-off-by: Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx> >> --- >> >> Notes: >> Changes in v6: >> - Parse port->phy mapping from devicetree removing the need for the >> realtek,port property > Good to see you found a way to do this! > > >> +/* >> + * MDIO controller for RTL9300 switches with integrated SoC. >> + * >> + * The MDIO communication is abstracted by the switch. At the software level >> + * communication uses the switch port to address the PHY. We work out the >> + * mapping based on the MDIO bus described in device tree and the realtek,port >> + * property. >> + */ > Needs an update again ;-) Yep will do. >> +static int rtl9300_mdio_phy_to_port(struct mii_bus *bus, int phy_id) >> +{ >> + struct rtl9300_mdio_chan *chan = bus->priv; >> + struct rtl9300_mdio_priv *priv = chan->priv; >> + int i; >> + >> + for (i = find_first_bit(priv->valid_ports, MAX_PORTS); >> + i < MAX_PORTS; >> + i = find_next_bit(priv->valid_ports, MAX_PORTS, i + 1)) > You could use the for_each_set_bit(i, priv->valid_ports, MAX_PORTS) loop macro. I figured there must be a wrapper for this idiom but I couldn't find it for looking. >> +static int rtl9300_mdio_read_c22(struct mii_bus *bus, int phy_id, int regnum) >> +{ > [...] >> + >> + err = regmap_write(regmap, SMI_ACCESS_PHY_CTRL_2, port << 16); > Another candidate for FIELD_PREP() Yep. There's a couple more too. > >> + if (err) >> + return err; >> + >> + val = FIELD_PREP(GENMASK(24, 20), regnum) | >> + FIELD_PREP(GENMASK(19, 15), 0x1f) | >> + FIELD_PREP(GENMASK(14, 3), 0xfff) | > You could use #define-s for the GENMASK() field masks too, similar to PHY_CTRL_*. That > would make what you're setting a bit clearer, compared to these literal values. Sure will do. > Nit: You're also setting all-one values, so GENMASK(19, 15) and GENMASK(14, 3) by > themselves are sufficient. E.g. PHY_CTRL_NO_PAGE_PARK and PHY_CTRL_NO_PAGE_SELECT. This part I'm not planning on doing. Right now I am just setting them to all-ones but the same code may end up needing to grow page logic (Realtek's SDK has code that adds page read/write functions to phylib). >> +static int rtl9300_mdiobus_probe(struct platform_device *pdev) >> +{ > [...] >> + >> + device_for_each_child_node(dev, child) { >> + err = rtl9300_mdiobus_probe_one(dev, priv, child); > In your next patch you use 'status = "disabled"' for the base dtsi. You may want to use > fwnode_for_each_available_child_node() in that case, so unused busses are not probed. Hmm, the existing code is only registering two mdio buses. Although I can't see why it's not attempting to to register the other two. Ah OK. It's because device_for_each_child_node() wraps device_get_next_child_node() which calls fwnode_get_next_child_node() which calls of_fwnode_get_next_child_node() which does the available check with of_get_next_available_child(). So I don't think there's any change to be made here.
