Hi Marek,
On 5/1/19 2:38 AM, Marek Behun wrote:
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
Ideally would be to add also namespacing prefix,
to make it clear that these are driver specific macros.
Hmm. Would these names be okay?
CMD_LED_COLOR_LENGTH
OMNIA_CMD_LED_COLOR_LEN
CMD_LED_COLOR_CMD
OMNIA_CMD_LED_COLOR
CMD_LED_COLOR_LED_ID
OMNIA_CMD_LED_COLOR_ID
CMD_LED_COLOR_RED
If LED color identifiers are not varying between
commands we can skip "CMD" part.
OMNIA_LED_COLOR_RED
CMD_LED_COLOR_GREEN
OMNIA_LED_COLOR_GREEN
CMD_LED_COLOR_BLUE
OMNIA_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.
Actually, thinking more of it - all in all this is legacy enum,
superseded by, yes, max_brightness property. So, I'd say 255 can stay.
--
Best regards,
Jacek Anaszewski
