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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

Thanks,

									Pavel

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

Attachment: signature.asc
Description: Digital signature


[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux