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@xxxxxx>
> Reviewed-by: Dan Murphy <dmurphy@xxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: PGP signature
