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!
