Re: [PATCH leds/for-next v2 2/2] leds: turris-omnia: Add support for 256 brightness values

[Jacek Anaszewski <jacek.anaszewski@xxxxxxxxx>]

On 4/30/19 10:46 PM, Pavel Machek wrote:
> Hi!
>
> > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > > index 3747cbd0de2c..62d17c2f4878 100644
> > > --- a/drivers/leds/Kconfig
> > > +++ b/drivers/leds/Kconfig
> > > @@ -139,8 +139,8 @@ config LEDS_TURRIS_OMNIA
> > >   	  side of CZ.NIC's Turris Omnia router. There are 12 RGB LEDs on the
> > >   	  front panel.
> > >   	  This driver does not currently support setting LED colors, only
> > > -	  on/off state. Also HW triggering is disabled when the controller
> > > -	  is probed by this driver.
> > > +	  brightness. Also HW triggering is disabled when the controller is
> > > +	  probed by this driver.
> > >   
> >
> > I am not seeing where or how this is done in the driver on probe.
>
> I checked, and I believe it was ok.
>
> > >   config LEDS_LM3530
> > >   	tristate "LCD Backlight driver for LM3530"
> > > diff --git a/drivers/leds/leds-turris-omnia.c b/drivers/leds/leds-turris-omnia.c
> > > index dc9fac56b13a..0e805d94f298 100644
> > > --- a/drivers/leds/leds-turris-omnia.c
> > > +++ b/drivers/leds/leds-turris-omnia.c
> > > @@ -54,7 +54,7 @@ static int omnia_led_brightness_set_blocking(struct led_classdev *led,
> > >   	struct omnia_leds *leds = dev_get_drvdata(led->dev->parent);
> > >   	int idx = omnia_led_idx(leds, led);
> > >   	int ret;
> > > -	u8 state;
> > > +	u8 buf[5], state;
> >
> > Magic numbers
>
> Nothing wrong with magic "5" when you have 0, 1, 2, 3 and 4
> below. Constants are useful when they make code easier to read, not in
> this case.
>
> > >   	mutex_unlock(&leds->lock);
> > >   
> > >   	return ret;
> > > @@ -97,7 +105,7 @@ static int omnia_led_register(struct omnia_leds *leds,
> > >   	ret = fwnode_property_read_string(node, "label", &str);
> > >   	snprintf(led->name, sizeof(led->name), "omnia::%s", ret ? "" : str);
> > >   
> > > -	led->cdev.max_brightness = 1;
> > > +	led->cdev.max_brightness = 255;
> >
> > How about LED_FULL?
>
> Please don't.
>
> > > +	/* set all LEDs color to [255, 255, 255] */
> > > +	buf[0] = CMD_LED_COLOR;
> > > +	buf[1] = OMNIA_BOARD_LEDS;
> > > +	buf[2] = buf[3] = buf[4] = 255;
> > > +
> >
> > LED_FULL for this as above.
>
> I'd actually prefer 255 here; due to the way hardware is designed,
> it has brightness in byte. No need to put LED_FULL here, and force
> reader to check the headers to see what value LED_FULL has.

Let's be consistent. Please add constants for both 5 and 255.
The conventions are established for a reason.

--
Best regards,
Jacek Anaszewski