On 5/1/19 12:43 AM, Pavel Machek wrote:
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.
I don't see how a constant with informative name can be harder
to read than bare number.
(You can try grep -ri '\<5\>' net/ :-) ]
I have something more informative:
grep -r "^#define [_A-Za-z0-9]*\(SIZE\|LEN\)\s*[0-9]$" net/
--
Best regards,
Jacek Anaszewski