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

[Jijie Shao <shaojijie@huawei.com>]

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