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

-- 
http://www.livejournal.com/~pavelmachek