RE: [PATCH net-next 2/5] net: ethernet: oa_tc6: Allow custom mii_bus

[Selvamani Rajagopal]

> -----Original Message-----
> From: Andrew Lunn <andrew@xxxxxxx>
> Sent: Saturday, May 2, 2026 8:51 PM
> To: ciprian.regus@xxxxxxxxxx
> Cc: Parthiban Veerasooran <parthiban.veerasooran@xxxxxxxxxxxxx>; Andrew Lunn
> <andrew+netdev@xxxxxxx>; David S. Miller <davem@xxxxxxxxxxxxx>; Eric Dumazet
> <edumazet@xxxxxxxxxx>; Jakub Kicinski <kuba@xxxxxxxxxx>; Paolo Abeni
> <pabeni@xxxxxxxxxx>; Simon Horman <horms@xxxxxxxxxx>; Jonathan Corbet
> <corbet@xxxxxxx>; Shuah Khan <skhan@xxxxxxxxxxxxxxxxxxx>; Heiner Kallweit
> <hkallweit1@xxxxxxxxx>; Russell King <linux@xxxxxxxxxxxxxxx>; Rob Herring
> <robh@xxxxxxxxxx>; Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>; Conor Dooley
> <conor+dt@xxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> doc@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH net-next 2/5] net: ethernet: oa_tc6: Allow custom mii_bus
> 
> 
> This Message Is From an External Sender
> This message came from outside your organization.
> 
> > @@ -538,32 +539,37 @@ static int oa_tc6_mdiobus_register(struct oa_tc6 *tc6)
> > {
> > int ret;
> >
> > - tc6->mdiobus = mdiobus_alloc();
> > if (!tc6->mdiobus) {
> > - netdev_err(tc6->netdev, "MDIO bus alloc failed\n");
> > - return -ENOMEM;
> > + tc6->mdiobus = mdiobus_alloc();
> > + if (!tc6->mdiobus) {
> > + netdev_err(tc6->netdev, "MDIO bus alloc failed\n");
> > + return -ENOMEM;
> > + }
> > +
> > + tc6->mdiobus->read = oa_tc6_mdiobus_read;
> > + tc6->mdiobus->write = oa_tc6_mdiobus_write;
> > + /* OPEN Alliance 10BASE-T1x compliance MAC-PHYs will have both C22 and
> > + * C45 registers space. If the PHY is discovered via C22 bus protocol it
> > + * assumes it uses C22 protocol and always uses C22 registers indirect
> > + * access to access C45 registers. This is because, we don't have a
> > + * clean separation between C22/C45 register space and C22/C45 MDIO bus
> > + * protocols. Resulting, PHY C45 registers direct access can't be used
> > + * which can save multiple SPI bus access. To support this feature, PHY
> > + * drivers can set .read_mmd/.write_mmd in the PHY driver to call
> > + * .read_c45/.write_c45. Ex: drivers/net/phy/microchip_t1s.c
> > + */
> > + tc6->mdiobus->read_c45 = oa_tc6_mdiobus_read_c45;
> > + tc6->mdiobus->write_c45 = oa_tc6_mdiobus_write_c45;
> > +
> > + tc6->own_mdiobus = true;
> > }
> >
> > tc6->mdiobus->priv = tc6;
> > - tc6->mdiobus->read = oa_tc6_mdiobus_read;
> > - tc6->mdiobus->write = oa_tc6_mdiobus_write;
> > - /* OPEN Alliance 10BASE-T1x compliance MAC-PHYs will have both C22 and
> > - * C45 registers space. If the PHY is discovered via C22 bus protocol it
> > - * assumes it uses C22 protocol and always uses C22 registers indirect
> > - * access to access C45 registers. This is because, we don't have a
> > - * clean separation between C22/C45 register space and C22/C45 MDIO bus
> > - * protocols. Resulting, PHY C45 registers direct access can't be used
> > - * which can save multiple SPI bus access. To support this feature, PHY
> > - * drivers can set .read_mmd/.write_mmd in the PHY driver to call
> > - * .read_c45/.write_c45. Ex: drivers/net/phy/microchip_t1s.c
> > - */
> > - tc6->mdiobus->read_c45 = oa_tc6_mdiobus_read_c45;
> > - tc6->mdiobus->write_c45 = oa_tc6_mdiobus_write_c45;
> > - tc6->mdiobus->name = "oa-tc6-mdiobus";
> > tc6->mdiobus->parent = tc6->dev;
> > + tc6->mdiobus->name = "oa-tc6-mdiobus";
> >
> > snprintf(tc6->mdiobus->id, ARRAY_SIZE(tc6->mdiobus->id), "%s",
> > - dev_name(&tc6->spi->dev));
> > + dev_name(&tc6->spi->dev));
> >
> > ret = mdiobus_register(tc6->mdiobus);
> > if (ret) {
> > @@ -577,19 +583,30 @@ static int oa_tc6_mdiobus_register(struct oa_tc6 *tc6)
> >
> > static void oa_tc6_mdiobus_unregister(struct oa_tc6 *tc6)
> > {
> > + if (!tc6->mdiobus)
> > + return;
> > +
> > mdiobus_unregister(tc6->mdiobus);
> > - mdiobus_free(tc6->mdiobus);
> > +
> > + if (tc6->own_mdiobus)
> > + mdiobus_free(tc6->mdiobus);
> > }
> >
> > static int oa_tc6_phy_init(struct oa_tc6 *tc6)
> > {
> > int ret;
> >
> > - ret = oa_tc6_check_phy_reg_direct_access_capability(tc6);
> > - if (ret) {
> > - netdev_err(tc6->netdev,
> > - "Direct PHY register access is not supported by the MAC-PHY\n");
> > - return ret;
> > + /* If the driver provided a mii_bus, it is also responsible for
> > + * implementing the bus access methods, so we don't have to worry
> > + * about checking the PHY access mode.
> > + */
> > + if (!tc6->mdiobus) {
> > + ret = oa_tc6_check_phy_reg_direct_access_capability(tc6);
> > + if (ret) {
> > + netdev_err(tc6->netdev,
> > + "Direct PHY register access is not supported by the MAC-PHY\n");
> > + return ret;
> > + }
> 
> This all seems pretty invasive and ugly. Please could you think what
> happens if instead of passing in an mdiobus, you pass a phydev. Is the
> change to the core simpler and cleaner?
> 
> Andrew

Kind of agree. Initially we were thinking about changing the existing code (Microchip's vendor code) to alloc mii_bus so that code would be same across multiple vendors. Either way, it would be invasive changes. So, we decide to go with minimal change to other vendor's code.

Trying to understand your suggestion. Are you suggesting to move entire mii_bus allocation/APIs implementation to vendor side and keep only phy dev usage in oa_tc6.c?

If my understanding is correct, I guess it would be cleaner. I can try that. Let me know.

Selva