Re: [PATCH v7 2/3] leds: initial support for Turris Omnia LEDs

["Marek Behún" <marek.behun@nic.cz>]

On Fri, 24 Jul 2020 12:58:25 +0200
Pavel Machek <pavel@ucw.cz> 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@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:

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