Re: [PATCH net-next 4/5] net: ethernet: adi: Add a driver for the ADIN1140 MACPHY

[Andrew Lunn]

> +static int adin1140_get_phy_c45_mms(int devnum)
> +{
> +	switch (devnum) {
> +	case MDIO_MMD_PCS:
> +		return ADIN1140_PHY_C45_PCS_MMS2;
> +	case MDIO_MMD_PMAPMD:
> +		return ADIN1140_PHY_C45_PMA_PMD_MMS3;
> +	case MDIO_MMD_VEND2:
> +		return ADIN1140_PHY_C45_VS_PLCA_MMS4;
> +	default:
> +		return devnum;
> +	}
> +}
> +
> +static int adin1140_mdiobus_read_c45(struct mii_bus *bus, int addr,
> +				     int devnum, int regnum)
> +{
> +	struct oa_tc6 *tc6 = bus->priv;
> +	u32 regval;
> +	u32 mms;
> +	int ret;
> +
> +	mms = adin1140_get_phy_c45_mms(devnum);
> +	ret = oa_tc6_read_register(tc6, ADIN1140_MMS_REG(mms, regnum),
> +				   &regval);
> +	if (ret)
> +		return ret;
> +
> +	return regval;
> +}
> +
> +static int adin1140_mdiobus_write_c45(struct mii_bus *bus, int addr,
> +				      int devnum, int regnum, u16 val)
> +{
> +	struct oa_tc6 *tc6 = bus->priv;
> +	int ret;
> +
> +	ret = adin1140_get_phy_c45_mms(devnum);
> +	if (ret < 0)
> +		return ret;
> +
> +	return oa_tc6_write_register(tc6, ADIN1140_MMS_REG(ret, regnum), val);
> +}

At a quick look, these seem the same as oa_tc6_mdiobus_read_c45() and
oa_tc6_mdiobus_write_c45(). Please export them and use them.

> +static int adin1140_mdiobus_read(struct mii_bus *bus, int addr, int regnum)
> +{
> +	struct oa_tc6 *tc6 = bus->priv;
> +	u32 reg_val;
> +	int ret;
> +
> +	/* The ADIN1140's standard PHY C22 register map (OA TC6 0xFF00 -
> +	 * 0xFF1F), of which only 0xFF00 - 0xFF03 are implemented) cannot be
> +	 * accessed while frames are being received by the PHY. In case this
> +	 * happens the CONFIG0 and CONFIG2 register values will get corrupted,
> +	 * getting a random value. Both reads and writes cause the same
> +	 * behavior. This is a workaround that avoids MDIO accesses all
> +	 * together. Since this is a 10BASE-T1S PHY, only the loopback and
> +	 * reset (AN) bits in the control register (0x0) can be written.
> +	 * These functionalities have custom implementations in the PHY
> +	 * driver. Since the MAC and PHY are integrated in the same device, we
> +	 * can read the OA TC6 MACPHY ID register instead of the PHYID (0x2
> +	 * and 0x3) ones, as their value matches. C45 accesses do not cause
> +	 * this issue.
> +	 */
> +
> +	switch (regnum) {
> +	case MII_BMCR:
> +		return ADIN1140_PHY_CTRL_DEFAULT;
> +	case MII_BMSR:
> +		return ADIN1140_PHY_STATUS_DEFAULT;
> +	case MII_PHYSID1:
> +		ret = oa_tc6_read_register(tc6, ADIN1140_MACPHY_ID_REG,
> +					   &reg_val);
> +		if (ret)
> +			return ret;
> +
> +		return FIELD_GET(GENMASK(31, 16), reg_val);
> +	case MII_PHYSID2:
> +		ret = oa_tc6_read_register(tc6, ADIN1140_MACPHY_ID_REG,
> +					   &reg_val);
> +		if (ret)
> +			return ret;

Is it even worth reading this register? Why not hard code this as
well? Or do you expect a new version of the device which is less
FUBAR, and having a different PHY ID?

> +static int adin1140_mdiobus_write(struct mii_bus *bus, int addr, int regnum,
> +				  u16 val)
> +{
> +	return 0;

-EIO. Since writes are not support, you want to know if something
 actually does a write.

> +static int adin1140_mdio_register(struct adin1140_priv *priv)
> +{
> +	priv->mdiobus = mdiobus_alloc();
> +	if (!priv->mdiobus) {
> +		netdev_err(priv->netdev, "MDIO bus alloc failed\n");
> +		return -ENOMEM;
> +	}
> +
> +	priv->mdiobus->read = adin1140_mdiobus_read;
> +	priv->mdiobus->write = adin1140_mdiobus_write;
> +	priv->mdiobus->read_c45 = adin1140_mdiobus_read_c45;
> +	priv->mdiobus->write_c45 = adin1140_mdiobus_write_c45;

Name? id? 

      Andrew