Re: [RFC PATCH V2 net-next 03/11] net: hibmcge: Add mdio and hardware configuration supported in this module

[Jijie Shao]

on 2024/8/16 10:14, Andrew Lunn wrote:
> > +int hbg_hw_adjust_link(struct hbg_priv *priv, u32 speed, u32 duplex)
> > +{
> > +	if (speed != HBG_PORT_MODE_SGMII_10M &&
> > +	    speed != HBG_PORT_MODE_SGMII_100M &&
> > +	    speed != HBG_PORT_MODE_SGMII_1000M)
> > +		return -EOPNOTSUPP;
> > +
> > +	if (duplex != DUPLEX_FULL && duplex != DUPLEX_HALF)
> > +		return -EOPNOTSUPP;
>
> Can this happen? We try to avoid defensive code, preferring to ensure
> it can never happen. So long as you have told phylib the limits of
> your hardware, it should enforce these.

Ok, I'll delete these.

> > @@ -26,11 +27,11 @@ static int hbg_init(struct hbg_priv *priv)
> >   		return dev_err_probe(dev, PTR_ERR(regmap), "failed to init regmap\n");
> >   
> >   	priv->regmap = regmap;
> > -	ret = hbg_hw_event_notify(priv, HBG_HW_EVENT_INIT);
> > +	ret = hbg_hw_init(priv);
> >   	if (ret)
> >   		return ret;
> >   
> > -	return hbg_hw_dev_specs_init(priv);
> > +	return hbg_mdio_init(priv);
> I've not read the previous patches, but that looks odd. Why is code
> you just added in previous patches getting replaced?

In previous patch, we did not introduce the hbg_hw_init().
In this patch, the hbg_hw_init() is introduced,
and the hbg_hw_event_notify() and hbg_hw_dev_specs_init() functions are moved to hbg_hw_init().


hbg_hw_init() can be moved to the previous patch.

> > +static int hbg_phy_connect(struct hbg_priv *priv)
> > +{
> > +	struct phy_device *phydev = priv->mac.phydev;
> > +	struct device *dev = &priv->pdev->dev;
> > +	struct hbg_mac *mac = &priv->mac;
> > +	int ret;
> > +
> > +	ret = phy_connect_direct(priv->netdev, mac->phydev, hbg_phy_adjust_link,
> > +				 PHY_INTERFACE_MODE_SGMII);
> > +	if (ret)
> > +		return dev_err_probe(dev, -ENOMEM, "failed to connect phy\n");
> Don't replace the error code. Doing so actually makes dev_err_probe()
> pointless because it is not going to see the EPROBE_DEFER.
Sorry, It is a mistake.
I'll fix it in the next version.

> > +int hbg_mdio_init(struct hbg_priv *priv)
> > +{
> > +	struct device *dev = &priv->pdev->dev;
> > +	struct hbg_mac *mac = &priv->mac;
> > +	struct phy_device *phydev;
> > +	struct mii_bus *mdio_bus;
> > +	int ret;
> > +
> > +	mac->phy_addr = priv->dev_specs.phy_addr;
> > +	mdio_bus = devm_mdiobus_alloc(dev);
> > +	if (!mdio_bus)
> > +		return dev_err_probe(dev, -ENOMEM, "failed to alloc MDIO bus\n");
> > +
> > +	mdio_bus->parent = dev;
> > +	mdio_bus->priv = priv;
> > +	mdio_bus->phy_mask = ~(1 << mac->phy_addr);
> > +	mdio_bus->name = "hibmcge mii bus";
> > +	mac->mdio_bus = mdio_bus;
> > +
> > +	mdio_bus->read = hbg_mdio_read22;
> > +	mdio_bus->write = hbg_mdio_write22;
> > +	snprintf(mdio_bus->id, MII_BUS_ID_SIZE, "%s-%s", "mii", dev_name(dev));
> > +
> > +	ret = devm_mdiobus_register(dev, mdio_bus);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "failed to register MDIO bus\n");
> > +
> > +	phydev = mdiobus_get_phy(mdio_bus, mac->phy_addr);
> > +	if (!phydev)
> > +		return dev_err_probe(dev, -EIO, "failed to get phy device\n");
> ENODEV is probably better, since the device does not exist.
>
> 	Andrew

Thanks a lot!

	Jijie Shao