On Fri, 24 Jul 2020 12:58:25 +0200
Pavel Machek <pavel@xxxxxx> wrote:
> 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:
I think it was that way when it was first proposed, sometime last year,
but Dan complained about magic numbers, so I changed it to macros :D
> > +#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 think it shouldn't be an error when fewer than OMNIA_BOARD_LEDS are
in device tree...
> [I suppose I'll apply it anyway; these can be fixed post-merge].
As you wish. I can sent follow-up patch afterwards for to remove those
macros...
> Best regards,
> Pavel
