Hi!

> This adds basic support for LEDs on the front side of CZ.NIC's Turris
> Omnia router.
> 
> There are 12 RGB LEDs. The controller supports HW triggering mode for
> the LEDs, but this driver does not support it yet, and sets all the LEDs
> defined in device-tree into SW mode upon probe.
> 
> This driver uses the multicolor LED framework.
> 
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> Reviewed-by: Dan Murphy <dmurphy@ti.com>
> ---
>  drivers/leds/Kconfig             |  11 ++
>  drivers/leds/Makefile            |   1 +
>  drivers/leds/leds-turris-omnia.c | 295 +++++++++++++++++++++++++++++++
>  3 files changed, 307 insertions(+)
>  create mode 100644 drivers/leds/leds-turris-omnia.c

Whoever told you to use defines here was evil:

> +#define OMNIA_CMD_LED_COLOR_LED		1
> +#define OMNIA_CMD_LED_COLOR_R		2
> +#define OMNIA_CMD_LED_COLOR_G		3
> +#define OMNIA_CMD_LED_COLOR_B		4
> +#define OMNIA_CMD_LED_COLOR_LEN		5

> +static int omnia_led_brightness_set_blocking(struct led_classdev *cdev,
> +					     enum led_brightness brightness)
> +{
> +	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
> +	struct omnia_leds *leds = dev_get_drvdata(cdev->dev->parent);
> +	struct omnia_led *led = to_omnia_led(mc_cdev);
> +	u8 buf[OMNIA_CMD_LED_COLOR_LEN], state;
> +	int ret;
> +
> +	mutex_lock(&leds->lock);
> +
> +	led_mc_calc_color_components(&led->mc_cdev, brightness);
> +
> +	buf[OMNIA_CMD] = CMD_LED_COLOR;
> +	buf[OMNIA_CMD_LED_COLOR_LED] = led->reg;
> +	buf[OMNIA_CMD_LED_COLOR_R] = mc_cdev->subled_info[0].brightness;
> +	buf[OMNIA_CMD_LED_COLOR_G] = mc_cdev->subled_info[1].brightness;
> +	buf[OMNIA_CMD_LED_COLOR_B] = mc_cdev->subled_info[2].brightness;

Aha, it is LED vs LEN, but please don't obscure code with macros like
that. It is important to see that buf[] is fully initialized, macros
hide that.

> +	state = CMD_LED_STATE_LED(led->reg);
> +	if (buf[OMNIA_CMD_LED_COLOR_R] || buf[OMNIA_CMD_LED_COLOR_G] || buf[OMNIA_CMD_LED_COLOR_B])
> +		state |= CMD_LED_STATE_ON;

I'd make this conditional on mc_cdev->subled_info[0].brightness[x],
not buf, but...


> +	int ret, count;
> +
> +	count = of_get_available_child_count(np);
> +	if (!count) {
> +		dev_err(dev, "LEDs are not defined in device tree!\n");
> +		return -ENODEV;
> +	} else if (count > OMNIA_BOARD_LEDS) {
> +		dev_err(dev, "Too many LEDs defined in device tree!\n");
> +		return -EINVAL;
> +	}

Should count be unsigned?

Or better, should we check it is _exactly_ OMNIA_BOARD_LEDS as we only
support this known hardware?

[I suppose I'll apply it anyway; these can be fixed post-merge].

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html