Hi Dan, On Tue, 30 Apr 2019 14:46:09 -0500 Dan Murphy <dmurphy@xxxxxx> wrote: > I am not seeing where or how this is done in the driver on probe. In function omnia_led_register https://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git/tree/drivers/leds/leds-turris-omnia.c?h=for-next#n107 > > + u8 buf[5], state; ... > > + buf[0] = CMD_LED_COLOR; > > + buf[1] = idx; > > + buf[2] = buf[3] = buf[4] = brightness; > > Magic numbers Hmm. Would these names be okay? CMD_LED_COLOR_LENGTH CMD_LED_COLOR_CMD CMD_LED_COLOR_LED_ID CMD_LED_COLOR_RED CMD_LED_COLOR_GREEN CMD_LED_COLOR_BLUE If constants are used they have to indicate which command they apply to. Or maybe a struct cmd_led_color { u8 cmd; u8 id; u8 red, green, blue; } could be defined, but I think that this is used very sporadically in the kernel. > What happens if the LEDs are in HW triggered mode already? > Should this not be checked especially if the driver is removed and re-installed the uP has > this configured as HW mode. Unless you reset the uP as well. In current version of this driver on driver probe all LEDs are put into SW mode. On driver remove they are put to HW mode. It is not possible to read from the microcontroller in which mode they are. > > + led->cdev.max_brightness = 255; > > How about LED_FULL? I thought about this but was not sure, for the same reason Pavel mentioned: I wanted to indicate that this microcontroller supports 8bit per channel. LED_FULL is a kernel specific macro and although it will never change and is equal, it makes less (fewer?) sense to me to use it. But I will change it in the next version per your request. Marek
