| Message ID | 20201030114435.20169-1-kabel@kernel.org |
|---|---|
| Headers | show |
| Series | netdev trigger offloading and LEDs on Marvell PHYs | expand |
On Fri 2020-10-30 12:44:29, Marek Behún wrote: > The trigger_data struct is allocated with kzalloc, so we do not need to > explicitly set members to zero. > > Signed-off-by: Marek Behún <kabel@kernel.org> Acked-by: Pavel Machek <pavel@ucw.cz>
> +/* FIXME: Blinking rate is shared by all LEDs on a PHY. Should we check whether > + * another LED is currently blinking with incompatible rate? It would be cleaner > + * if we in this case failed to offload blinking this LED. > + * But consider this situation: > + * 1. user sets LED[1] to blink with period 500ms for some reason. This would > + * start blinking LED[1] with perion 670ms here period. > + * 2. user sets netdev trigger to LED[0] to blink on activity, default there > + * is 100ms period, which would translate here to 84ms. This is > + * incompatible with the already blinking LED, so we fail to offload to HW, > + * and netdev trigger does software offloading instead. > + * 3. user unsets blinking od LED[1], so now we theoretically can offload > + * netdev trigger to LED[0], but we don't know about it, and so it is left > + * in SW triggering until user writes the settings again > + * This could be solved by the netdev trigger periodically trying to offload to > + * HW if we reported that it is theoretically possible (by returning -EAGAIN > + * instead of -EOPNOTSUPP, for example). Do we want to do this? > + */ I believe we should check & fallback to software if there's already incompatible rate in use. No need to periodically re-try to activate the offload. Best regards, Pavel -- http://www.livejournal.com/~pavelmachek
Hi! > If the trigger with given configuration cannot be offloaded, the method > should return -EOPNOTSUPP, in which case the trigger must blink the LED > in SW. > > Signed-off-by: Marek Behún <kabel@kernel.org> > + > +If the second argument (enable) to the trigger_offload() method is false, any > +active HW offloading must be deactivated. Are errors permitted in the "disable" case? > +static inline void led_trigger_offload_stop(struct led_classdev *led_cdev) > +{ > + if (!led_cdev->trigger_offload) > + return; > + > + if (led_cdev->offloaded) > + led_cdev->trigger_offload(led_cdev, false); > +} Set offloaded to false? Let me check the rest to decide if this makes sense. Best regards, Pavel
On Fri 2020-10-30 12:44:33, Marek Behún wrote: > Add a new integer member phyindex to struct phy_device. This member is > unique for every phy_device. Atomic incrementation occurs in > phy_device_register. > > This can be used for example in LED sysfs API. The LED subsystem names > each LED in format `device:color:function`, but currently the PHY device > names are not suited for this, since in some situations a PHY device > name can look like this > d0032004.mdio-mii:01 > or even like this > !soc!internal-regs@d0000000!mdio@32004!switch0@10!mdio:08 > Clearly this cannot be used as the `device` part of a LED name. > > Signed-off-by: Marek Behún <kabel@kernel.org> Atomic should _not_ be neccessary for this. Just make sure access is serialised by some existing lock. Pavel > @@ -892,6 +893,7 @@ EXPORT_SYMBOL(get_phy_device); > */ > int phy_device_register(struct phy_device *phydev) > { > + static atomic_t phyindex; > int err; > > err = mdiobus_register_device(&phydev->mdio); > @@ -908,6 +910,7 @@ int phy_device_register(struct phy_device *phydev) > goto out; > } > > + phydev->phyindex = atomic_inc_return(&phyindex) - 1; > err = device_add(&phydev->mdio.dev); > if (err) { > phydev_err(phydev, "failed to add\n");
Hi! > Add support for controlling the LEDs connected to several families of > Marvell PHYs via Linux LED API. These families currently are: 88E1112, > 88E1116R, 88E1118, 88E1121R, 88E1149R, 88E1240, 88E1318S, 88E1340S, > 88E1510, 88E1545 and 88E1548P. > > This does not yet add support for compound LED modes. This could be > achieved via the LED multicolor framework. > > netdev trigger offloading is also implemented. > > Signed-off-by: Marek Behún <kabel@kernel.org> > + val = 0; > + if (!active_low) > + val |= BIT(0); > + if (tristate) > + val |= BIT(1); You are parsing these parameters in core... but they are not going to be common for all phys, right? Should you parse them in the driver? Should the parameters be same we have for gpio-leds? > +static const struct marvell_led_mode_info marvell_led_mode_info[] = { > + { LED_MODE(1, 0, 0), { 0x0, -1, 0x0, -1, -1, -1, }, COMMON }, > + { LED_MODE(1, 1, 1), { 0x1, 0x1, 0x1, 0x1, 0x1, 0x1, }, COMMON }, > + { LED_MODE(0, 1, 1), { 0x4, 0x4, 0x4, 0x4, 0x4, 0x4, }, COMMON }, > + { LED_MODE(1, 0, 1), { -1, 0x2, -1, 0x2, 0x2, 0x2, }, COMMON }, > + { LED_MODE(0, 1, 0), { 0x5, -1, 0x5, -1, 0x5, 0x5, }, COMMON }, > + { LED_MODE(0, 1, 0), { -1, -1, -1, 0x5, -1, -1, }, L3V5_TX }, > + { LED_MODE(0, 0, 1), { -1, -1, -1, -1, 0x0, 0x0, }, COMMON }, > + { LED_MODE(0, 0, 1), { -1, 0x0, -1, -1, -1, -1, }, L1V0_RX }, > +}; > + > +static int marvell_find_led_mode(struct phy_device *phydev, struct phy_led *led, > + struct led_netdev_data *trig) > +{ > + const struct marvell_leds_info *info = led->priv; > + const struct marvell_led_mode_info *mode; > + u32 key; > + int i; > + > + key = LED_MODE(trig->link, trig->tx, trig->rx); > + > + for (i = 0; i < ARRAY_SIZE(marvell_led_mode_info); ++i) { > + mode = &marvell_led_mode_info[i]; > + > + if (key != mode->key || mode->regval[led->addr] == -1 || > + !(info->flags & mode->flags)) > + continue; > + > + return mode->regval[led->addr]; > + } > + > + dev_dbg(led->cdev.dev, > + "cannot offload trigger configuration (%s, link=%i, tx=%i, rx=%i)\n", > + netdev_name(trig->net_dev), trig->link, trig->tx, trig->rx); > + > + return -1; > +} I'm wondering if it makes sense to offload changes on link state. Those should be fairly infrequent and you are not saving significant power there... and seems to complicate things. The "shared frequency blinking" looks quite crazy. Rest looks reasonably sane. Best regards, Pavel