> +enum {
> + AIR_PHY_LED_DUR_BLINK_32M,
> + AIR_PHY_LED_DUR_BLINK_64M,
> + AIR_PHY_LED_DUR_BLINK_128M,
> + AIR_PHY_LED_DUR_BLINK_256M,
> + AIR_PHY_LED_DUR_BLINK_512M,
> + AIR_PHY_LED_DUR_BLINK_1024M,
DUR meaning duration? It has a blinks on for a little over a
kilometre? So a wave length of a little over 2 kilometres, or a
frequency of around 0.0005Hz :-)
> +static int __air_buckpbus_reg_write(struct phy_device *phydev,
> + u32 pbus_address, u32 pbus_data,
> + bool set_mode)
> +{
> + int ret;
> +
> + if (set_mode) {
> + ret = __phy_write(phydev, AIR_BPBUS_MODE,
> + AIR_BPBUS_MODE_ADDR_FIXED);
> + if (ret < 0)
> + return ret;
> + }
What does set_mode mean?
> +static int en8811h_load_firmware(struct phy_device *phydev)
> +{
> + struct device *dev = &phydev->mdio.dev;
> + const struct firmware *fw1, *fw2;
> + int ret;
> +
> + ret = request_firmware_direct(&fw1, EN8811H_MD32_DM, dev);
> + if (ret < 0)
> + return ret;
> +
> + ret = request_firmware_direct(&fw2, EN8811H_MD32_DSP, dev);
> + if (ret < 0)
> + goto en8811h_load_firmware_rel1;
> +
How big are these firmwares? This will map the entire contents into
memory. There is an alternative interface which allows you to get the
firmware in chunks. I the firmware is big, just getting 4K at a time
might be better, especially if this is an OpenWRT class device.
> +static int en8811h_restart_host(struct phy_device *phydev)
> +{
> + int ret;
> +
> + ret = air_buckpbus_reg_write(phydev, EN8811H_FW_CTRL_1,
> + EN8811H_FW_CTRL_1_START);
> + if (ret < 0)
> + return ret;
> +
> + return air_buckpbus_reg_write(phydev, EN8811H_FW_CTRL_1,
> + EN8811H_FW_CTRL_1_FINISH);
> +}
What is host in this context?
> +static int air_led_hw_control_set(struct phy_device *phydev, u8 index,
> + unsigned long rules)
> +{
> + struct en8811h_priv *priv = phydev->priv;
> + u16 on = 0, blink = 0;
> + int ret;
> +
> + if (index >= EN8811H_LED_COUNT)
> + return -EINVAL;
> +
> + priv->led[index].rules = rules;
> +
> + if (rules & (BIT(TRIGGER_NETDEV_LINK_10) | BIT(TRIGGER_NETDEV_LINK))) {
> + on |= AIR_PHY_LED_ON_LINK10;
> + if (rules & BIT(TRIGGER_NETDEV_RX))
> + blink |= AIR_PHY_LED_BLINK_10RX;
> + if (rules & BIT(TRIGGER_NETDEV_TX))
> + blink |= AIR_PHY_LED_BLINK_10TX;
> + }
> +
> + if (rules & (BIT(TRIGGER_NETDEV_LINK_100) | BIT(TRIGGER_NETDEV_LINK))) {
> + on |= AIR_PHY_LED_ON_LINK100;
> + if (rules & BIT(TRIGGER_NETDEV_RX))
> + blink |= AIR_PHY_LED_BLINK_100RX;
> + if (rules & BIT(TRIGGER_NETDEV_TX))
> + blink |= AIR_PHY_LED_BLINK_100TX;
> + }
> +
> + if (rules & (BIT(TRIGGER_NETDEV_LINK_1000) | BIT(TRIGGER_NETDEV_LINK))) {
> + on |= AIR_PHY_LED_ON_LINK1000;
> + if (rules & BIT(TRIGGER_NETDEV_RX))
> + blink |= AIR_PHY_LED_BLINK_1000RX;
> + if (rules & BIT(TRIGGER_NETDEV_TX))
> + blink |= AIR_PHY_LED_BLINK_1000TX;
> + }
> +
> + if (rules & (BIT(TRIGGER_NETDEV_LINK_2500) | BIT(TRIGGER_NETDEV_LINK))) {
> + on |= AIR_PHY_LED_ON_LINK2500;
> + if (rules & BIT(TRIGGER_NETDEV_RX))
> + blink |= AIR_PHY_LED_BLINK_2500RX;
> + if (rules & BIT(TRIGGER_NETDEV_TX))
> + blink |= AIR_PHY_LED_BLINK_2500TX;
> + }
> +
> + if (on == 0) {
> + if (rules & BIT(TRIGGER_NETDEV_RX)) {
> + blink |= AIR_PHY_LED_BLINK_10RX |
> + AIR_PHY_LED_BLINK_100RX |
> + AIR_PHY_LED_BLINK_1000RX |
> + AIR_PHY_LED_BLINK_2500RX;
> + }
> + if (rules & BIT(TRIGGER_NETDEV_TX)) {
> + blink |= AIR_PHY_LED_BLINK_10TX |
> + AIR_PHY_LED_BLINK_100TX |
> + AIR_PHY_LED_BLINK_1000TX |
> + AIR_PHY_LED_BLINK_2500TX;
> + }
> + }
Vendors do like making LED control unique. I've not seen any other MAC
or PHY where you can blink for activity at a given speed. You cannot
have 10 and 100 at the same time, so why are there different bits for
them?
I _think_ this can be simplified
+ if (rules & (BIT(TRIGGER_NETDEV_LINK_10) | BIT(TRIGGER_NETDEV_LINK))) {
+ on |= AIR_PHY_LED_ON_LINK10;
+ }
+
+ if (rules & (BIT(TRIGGER_NETDEV_LINK_100) | BIT(TRIGGER_NETDEV_LINK))) {
+ on |= AIR_PHY_LED_ON_LINK100;
+ }
+
+ if (rules & (BIT(TRIGGER_NETDEV_LINK_1000) | BIT(TRIGGER_NETDEV_LINK))) {
+ on |= AIR_PHY_LED_ON_LINK1000;
+ }
+
+ if (rules & (BIT(TRIGGER_NETDEV_LINK_2500) | BIT(TRIGGER_NETDEV_LINK))) {
+ on |= AIR_PHY_LED_ON_LINK2500;
+ }
+
+ if (rules & BIT(TRIGGER_NETDEV_RX)) {
+ blink |= AIR_PHY_LED_BLINK_10RX |
+ AIR_PHY_LED_BLINK_100RX |
+ AIR_PHY_LED_BLINK_1000RX |
+ AIR_PHY_LED_BLINK_2500RX;
+ }
+ if (rules & BIT(TRIGGER_NETDEV_TX)) {
+ blink |= AIR_PHY_LED_BLINK_10TX |
+ AIR_PHY_LED_BLINK_100TX |
+ AIR_PHY_LED_BLINK_1000TX |
+ AIR_PHY_LED_BLINK_2500TX;
Does this work?
Andrew
---
pw-bot: cr
