Re: [PATCH net-next 2/2] net: phy: air_en8811h: Add the Airoha EN8811H PHY driver

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Tue, Feb 06, 2024 at 08:47:51PM +0100, Eric Woudstra wrote:
> +static const unsigned long en8811h_led_trig = (BIT(TRIGGER_NETDEV_FULL_DUPLEX) |
> +					       BIT(TRIGGER_NETDEV_LINK)        |
> +					       BIT(TRIGGER_NETDEV_LINK_10)     |
> +					       BIT(TRIGGER_NETDEV_LINK_100)    |
> +					       BIT(TRIGGER_NETDEV_LINK_1000)   |
> +					       BIT(TRIGGER_NETDEV_LINK_2500)   |
> +					       BIT(TRIGGER_NETDEV_RX)          |
> +					       BIT(TRIGGER_NETDEV_TX));

Cosmetic: extra parens around bitwise OR is not necessary.

...
> +static int air_buckpbus_reg_write(struct phy_device *phydev,
> +				  u32 pbus_address, u32 pbus_data)
> +{
> +	int ret, saved_page;

	int saved_page;
	int ret = 0;

> +
> +	saved_page = phy_select_page(phydev, AIR_PHY_PAGE_EXTENDED_4);

	if (saved_page >= 0) {
> +
> +	ret = __air_buckpbus_reg_write(phydev, pbus_address, pbus_data);
> +	if (ret < 0)
> +		phydev_err(phydev, "%s 0x%08x failed: %d\n", __func__,
> +			   pbus_address, ret);

	}

> +
> +	return phy_restore_page(phydev, saved_page, ret);
> +;

Cosmetic: unnecessary ;

> +}
> +
> +static int __air_buckpbus_reg_read(struct phy_device *phydev,
> +				   u32 pbus_address, u32 *pbus_data)
> +{
> +	int pbus_data_low, pbus_data_high;
> +	int ret;
> +
> +	ret = __phy_write(phydev, AIR_PBUS_MODE, AIR_PBUS_MODE_ADDR_FIXED);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = __phy_write(phydev, AIR_PBUS_RD_ADDR_HIGH, HIWORD(pbus_address));
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = __phy_write(phydev, AIR_PBUS_RD_ADDR_LOW,  LOWORD(pbus_address));
> +	if (ret < 0)
> +		return ret;
> +
> +	pbus_data_high = __phy_read(phydev, AIR_PBUS_RD_DATA_HIGH);
> +	if (pbus_data_high < 0)
> +		return ret;
> +
> +	pbus_data_low = __phy_read(phydev, AIR_PBUS_RD_DATA_LOW);
> +	if (pbus_data_low < 0)
> +		return ret;
> +
> +	*pbus_data = (u16)pbus_data_low | ((u32)(u16)pbus_data_high << 16);

I don't think you need the u16 casts here.

> +	return 0;
> +}
> +
> +static int air_buckpbus_reg_read(struct phy_device *phydev,
> +				 u32 pbus_address, u32 *pbus_data)
> +{
> +	int ret, saved_page;
> +
> +	saved_page = phy_select_page(phydev, AIR_PHY_PAGE_EXTENDED_4);
> +
> +	ret = __air_buckpbus_reg_read(phydev, pbus_address, pbus_data);
> +	if (ret < 0)
> +		phydev_err(phydev, "%s 0x%08x failed: %d\n", __func__,
> +			   pbus_address, ret);

Same as air_buckpbus_reg_write().

...
> +static int air_write_buf(struct phy_device *phydev, u32 address,
> +			 const struct firmware *fw)
> +{
> +	int ret, saved_page;
> +
> +	saved_page = phy_select_page(phydev, AIR_PHY_PAGE_EXTENDED_4);
> +
> +	ret = __air_write_buf(phydev, address, fw);
> +	if (ret < 0)
> +		phydev_err(phydev, "%s 0x%08x failed: %d\n", __func__,
> +			   address, ret);

Same as air_buckpbus_reg_write().

...
> +
> +	cl45_data = phy_read_mmd(phydev, MDIO_MMD_VEND2, AIR_PHY_LED_BCR);
> +	if (cl45_data < 0)
> +		return cl45_data;
> +
> +	switch (mode) {
> +	case AIR_LED_MODE_DISABLE:
> +		cl45_data &= ~AIR_PHY_LED_BCR_EXT_CTRL;
> +		cl45_data &= ~AIR_PHY_LED_BCR_MODE_MASK;
> +		break;
> +	case AIR_LED_MODE_USER_DEFINE:
> +		cl45_data |= AIR_PHY_LED_BCR_EXT_CTRL;
> +		cl45_data |= AIR_PHY_LED_BCR_CLK_EN;
> +		break;
> +	default:
> +		phydev_err(phydev, "LED mode %d is not supported\n", mode);
> +		return -EINVAL;
> +	}
> +
> +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, AIR_PHY_LED_BCR, cl45_data);
> +	if (ret < 0)
> +		return ret;

Please always use phy_modify_mmd() rather than a
phy_read_mmd()...phy_write_mmd():

	cl45_data = 0;

	if (mode == AIR_LED_MODE_USER_DEFINE)
		cl45_data = AIR_PHY_LED_BCR_EXT_CTRL |
			    AIR_PHY_LED_BCR_CLK_EN;

	ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2, AIR_PHY_LED_BCR,
			     AIR_PHY_LED_BCR_EXT_CTRL |
			     AIR_PHY_LED_BCR_CLK_EN, cl45_data);
	if (ret < 0)
		return ret;

This ensures that the read/modify/write is done atomically on the bus.

...
> +	ret = air_buckpbus_reg_read(phydev, EN8811H_GPIO_OUTPUT, &pbus_value);
> +	if (ret < 0)
> +		return ret;
> +	pbus_value |= EN8811H_GPIO_OUTPUT_345;
> +	ret = air_buckpbus_reg_write(phydev, EN8811H_GPIO_OUTPUT, pbus_value);
> +	if (ret < 0)
> +		return ret;

Searching the driver for "air_buckpbus_reg_read", there are four
instances where you read-modify-write, and only one instance which is
just a read.

I wonder whether it would make more sense to structure the accessors as

	__air_buckpbus_set_address(phydev, addr)
	__air_buckpbus_read(phydev, *data)
	__air_buckpbus_write(phydev, data)

which would make implementing reg_read, reg_write and reg_modify()
easier, and the addition of reg_modify() means that (a) there are less
bus cycles (through having to set the address twice) and (b) ensures
that the read-modify-write can be done atomically on the bus. This
assumes that reading data doesn't auto-increment the address (which
you would need to check.)

...
> +static int en8811h_config_aneg(struct phy_device *phydev)
> +{
> +	bool changed = false;
> +	int err, val;
> +
> +	val = 0;
> +	if (linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
> +			      phydev->advertising))
> +		val |= MDIO_AN_10GBT_CTRL_ADV2_5G;

While you only support 2500base-T, is there any reason not to use
linkmode_adv_to_mii_10gbt_adv_t() ?

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!