On Tue 2019-04-30 22:52:36, Jacek Anaszewski wrote: > 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. I don't believe such convention exists for kernel, and it certainly makes code harder to read in case of "5"... That code is currently simple and easy to read, lets keep it that way. (You can try grep -ri '\<5\>' net/ :-) ] 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
