| Message ID | 20230802160748.11208-6-kabel@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | leds: turris-omnia: updates | expand |
On Wed, 2 Aug 2023 18:07:47 +0200 Marek Behún <kabel@kernel.org> wrote: > +static int omnia_hwtrig_activate(struct led_classdev *cdev) > +{ > + struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev); > + struct omnia_leds *leds = dev_get_drvdata(cdev->dev->parent); > + struct omnia_led *led = to_omnia_led(mc_cdev); > + int err = 0; > + > + mutex_lock(&leds->lock); > + > + if (!led->on) { > + /* > + * If the LED is off (brightness was set to 0), the last > + * configured color was not necessarily sent to the MCU. > + * Recompute with max_brightness and send if needed. > + */ > + led_mc_calc_color_components(mc_cdev, cdev->max_brightness); > + > + if (omnia_led_channels_changed(led)) > + err = omnia_led_send_color_cmd(leds->client, led); > + } > + > + if (!err) { > + /* put the LED into MCU controlled mode */ > + err = omnia_cmd_write(leds->client, CMD_LED_MODE, > + CMD_LED_MODE_LED(led->reg)); > + if (!err) > + led->hwtrig = true; > + } > + > + mutex_unlock(&leds->lock); > + > + return err; > +} Pavel, Lee, here I wanted to ask: can the .activate() method of a LED trigger sleep? The .brightness_set() method of a LED cannot sleep, and therefore we have .brightness_set_blocking() method, which schedules the potentially sleeping call into a work. But there is no such mechanism for the LED trigger .activate() method. I looked at the LED core code, and it does not seem to me that .activate() sleeping would cause issues, besides keeping trigger list lock locked... Note that previously none of the LED triggers in drivers/leds/trigger sleeped in .activate(), but recently the ethernet PHY subsystem was wired into the netdev trigger, which may cause the .activate() method to do a PHY transfer, which AFAIK may sleep... Marek
On Wed, 02 Aug 2023, Marek Behún wrote: > On Wed, 2 Aug 2023 18:07:47 +0200 > Marek Behún <kabel@kernel.org> wrote: > > > +static int omnia_hwtrig_activate(struct led_classdev *cdev) > > +{ > > + struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev); > > + struct omnia_leds *leds = dev_get_drvdata(cdev->dev->parent); > > + struct omnia_led *led = to_omnia_led(mc_cdev); > > + int err = 0; > > + > > + mutex_lock(&leds->lock); > > + > > + if (!led->on) { > > + /* > > + * If the LED is off (brightness was set to 0), the last > > + * configured color was not necessarily sent to the MCU. > > + * Recompute with max_brightness and send if needed. > > + */ > > + led_mc_calc_color_components(mc_cdev, cdev->max_brightness); > > + > > + if (omnia_led_channels_changed(led)) > > + err = omnia_led_send_color_cmd(leds->client, led); > > + } > > + > > + if (!err) { > > + /* put the LED into MCU controlled mode */ > > + err = omnia_cmd_write(leds->client, CMD_LED_MODE, > > + CMD_LED_MODE_LED(led->reg)); > > + if (!err) > > + led->hwtrig = true; > > + } > > + > > + mutex_unlock(&leds->lock); > > + > > + return err; > > +} > > Pavel, Lee, here I wanted to ask: can the .activate() method of a LED > trigger sleep? The .brightness_set() method of a LED cannot sleep, and > therefore we have .brightness_set_blocking() method, which schedules > the potentially sleeping call into a work. But there is no such > mechanism for the LED trigger .activate() method. > > I looked at the LED core code, and it does not seem to me that > .activate() sleeping would cause issues, besides keeping trigger list > lock locked... > > Note that previously none of the LED triggers in drivers/leds/trigger > sleeped in .activate(), but recently the ethernet PHY subsystem was > wired into the netdev trigger, which may cause the .activate() method > to do a PHY transfer, which AFAIK may sleep... I suspect you know more than I do at this point. I could take a deep-dive into the code however a) I'm presently swamped with incoming reviews and b) I do not have any additional resources at my disposable than you do - it would consist of reading through and brain-grepping the code. Pavel or Jacek may have more knowledge at-hand though.
On Wed, 02 Aug 2023, Marek Behún wrote: > Add support for enabling MCU controlled mode of the Turris Omnia LEDs > via a LED private trigger called "omnia-mcu". Recall that private LED > triggers will only be listed in the sysfs trigger file for LEDs that > support them (currently there is no user of this mechanism). > > When in MCU controlled mode, the user can still set LED color, but the > blinking is done by MCU, which does different things for different LEDs: > - WAN LED is blinked according to the LED[0] pin of the WAN PHY > - LAN LEDs are blinked according to the LED[0] output of the > corresponding port of the LAN switch > - PCIe LEDs are blinked according to the logical OR of the MiniPCIe port > LED pins > > In the future I want to make the netdev trigger to transparently offload > the blinking to the HW if user sets compatible settings for the netdev > trigger (for LEDs associated with network devices). > There was some work on this already, and hopefully we will be able to > complete it sometime, but for now there are still multiple blockers for > this, and even if there weren't, we still would not be able to configure > HW controlled mode for the LEDs associated with MiniPCIe ports. > > In the meantime let's support HW controlled mode via the private LED > trigger mechanism. If, in the future, we manage to complete the netdev > trigger offloading, we can still keep this private trigger for backwards > compatibility, if needed. > > We also set "omnia-mcu" to cdev->default_trigger, so that the MCU keeps > control until the user first wants to take over it. If a different > default trigger is specified in device-tree via the > 'linux,default-trigger' property, LED class will overwrite > cdev->default_trigger, and so the DT property will be respected. > > Signed-off-by: Marek Behún <kabel@kernel.org> > --- > drivers/leds/Kconfig | 1 + > drivers/leds/leds-turris-omnia.c | 97 +++++++++++++++++++++++++++++--- > 2 files changed, 90 insertions(+), 8 deletions(-) > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > index 6046dfeca16f..ebb3b84d7a4f 100644 > --- a/drivers/leds/Kconfig > +++ b/drivers/leds/Kconfig > @@ -187,6 +187,7 @@ config LEDS_TURRIS_OMNIA > depends on I2C > depends on MACH_ARMADA_38X || COMPILE_TEST > depends on OF > + select LEDS_TRIGGERS > help > This option enables basic support for the LEDs found on the front > side of CZ.NIC's Turris Omnia router. There are 12 RGB LEDs on the > diff --git a/drivers/leds/leds-turris-omnia.c b/drivers/leds/leds-turris-omnia.c > index 636c6f802bcf..180b0cbeb92e 100644 > --- a/drivers/leds/leds-turris-omnia.c > +++ b/drivers/leds/leds-turris-omnia.c > @@ -31,7 +31,7 @@ struct omnia_led { > struct led_classdev_mc mc_cdev; > struct mc_subled subled_info[OMNIA_LED_NUM_CHANNELS]; > u8 cached_channels[OMNIA_LED_NUM_CHANNELS]; > - bool on; > + bool on, hwtrig; > int reg; > }; > > @@ -123,12 +123,14 @@ static int omnia_led_brightness_set_blocking(struct led_classdev *cdev, > > /* > * Only recalculate RGB brightnesses from intensities if brightness is > - * non-zero. Otherwise we won't be using them and we can save ourselves > - * some software divisions (Omnia's CPU does not implement the division > - * instruction). > + * non-zero (if it is zero and the LED is in HW blinking mode, we use > + * max_brightness as brightness). Otherwise we won't be using them and > + * we can save ourselves some software divisions (Omnia's CPU does not > + * implement the division instruction). > */ > - if (brightness) { > - led_mc_calc_color_components(mc_cdev, brightness); > + if (brightness || led->hwtrig) { > + led_mc_calc_color_components(mc_cdev, brightness ?: > + cdev->max_brightness); > > /* > * Send color command only if brightness is non-zero and the RGB > @@ -138,8 +140,11 @@ static int omnia_led_brightness_set_blocking(struct led_classdev *cdev, > err = omnia_led_send_color_cmd(leds->client, led); > } > > - /* send on/off state change only if (bool)brightness changed */ > - if (!err && !brightness != !led->on) { > + /* > + * Send on/off state change only if (bool)brightness changed and the LED > + * is not being blinked by HW. > + */ > + if (!err && !led->hwtrig && !brightness != !led->on) { > u8 state = CMD_LED_STATE_LED(led->reg); > > if (brightness) > @@ -155,6 +160,70 @@ static int omnia_led_brightness_set_blocking(struct led_classdev *cdev, > return err; > } > > +static struct led_hw_trigger_type omnia_hw_trigger_type; > + > +static int omnia_hwtrig_activate(struct led_classdev *cdev) > +{ > + struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev); > + struct omnia_leds *leds = dev_get_drvdata(cdev->dev->parent); > + struct omnia_led *led = to_omnia_led(mc_cdev); > + int err = 0; > + > + mutex_lock(&leds->lock); > + > + if (!led->on) { > + /* > + * If the LED is off (brightness was set to 0), the last > + * configured color was not necessarily sent to the MCU. > + * Recompute with max_brightness and send if needed. > + */ > + led_mc_calc_color_components(mc_cdev, cdev->max_brightness); > + > + if (omnia_led_channels_changed(led)) > + err = omnia_led_send_color_cmd(leds->client, led); > + } > + > + if (!err) { > + /* put the LED into MCU controlled mode */ Nit: You improved the comment above to be more grammatically correct by starting with an uppercase character. Please continue with this improvement for all comments there in. > + err = omnia_cmd_write(leds->client, CMD_LED_MODE, > + CMD_LED_MODE_LED(led->reg)); > + if (!err) > + led->hwtrig = true; > + } > + > + mutex_unlock(&leds->lock); > + > + return err; > +} > + > +static void omnia_hwtrig_deactivate(struct led_classdev *cdev) > +{ > + struct omnia_leds *leds = dev_get_drvdata(cdev->dev->parent); > + struct omnia_led *led = to_omnia_led(lcdev_to_mccdev(cdev)); > + int err; > + > + mutex_lock(&leds->lock); > + > + led->hwtrig = false; > + > + /* put the LED into software mode */ > + err = omnia_cmd_write(leds->client, CMD_LED_MODE, > + CMD_LED_MODE_LED(led->reg) | CMD_LED_MODE_USER); > + > + mutex_unlock(&leds->lock); > + > + if (err < 0) > + dev_err(cdev->dev, "Cannot put LED to software mode: %i\n", > + err); > +} > + > +static struct led_trigger omnia_hw_trigger = { > + .name = "omnia-mcu", > + .activate = omnia_hwtrig_activate, > + .deactivate = omnia_hwtrig_deactivate, > + .trigger_type = &omnia_hw_trigger_type, Not sure I understand this interface. Why not just set a bool instead of carrying around an empty struct? > +}; > + > static int omnia_led_register(struct i2c_client *client, struct omnia_led *led, > struct device_node *np) > { > @@ -198,6 +267,12 @@ static int omnia_led_register(struct i2c_client *client, struct omnia_led *led, > cdev = &led->mc_cdev.led_cdev; > cdev->max_brightness = 255; > cdev->brightness_set_blocking = omnia_led_brightness_set_blocking; > + cdev->trigger_type = &omnia_hw_trigger_type; > + /* > + * Use the omnia-mcu trigger as the default trigger. It may be rewritten > + * by LED class from the linux,default-trigger property. > + */ > + cdev->default_trigger = omnia_hw_trigger.name; > > /* put the LED into software mode */ > ret = omnia_cmd_write(client, CMD_LED_MODE, CMD_LED_MODE_LED(led->reg) | > @@ -310,6 +385,12 @@ static int omnia_leds_probe(struct i2c_client *client) > > mutex_init(&leds->lock); > > + ret = devm_led_trigger_register(dev, &omnia_hw_trigger); > + if (ret < 0) { > + dev_err(dev, "Cannot register private LED trigger: %d\n", ret); > + return ret; > + } > + > led = &leds->leds[0]; > for_each_available_child_of_node(np, child) { > ret = omnia_led_register(client, led, child); > -- > 2.41.0 >
Hi Marek, On 8/2/23 18:13, Marek Behún wrote: > On Wed, 2 Aug 2023 18:07:47 +0200 > Marek Behún <kabel@kernel.org> wrote: > >> +static int omnia_hwtrig_activate(struct led_classdev *cdev) >> +{ >> + struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev); >> + struct omnia_leds *leds = dev_get_drvdata(cdev->dev->parent); >> + struct omnia_led *led = to_omnia_led(mc_cdev); >> + int err = 0; >> + >> + mutex_lock(&leds->lock); >> + >> + if (!led->on) { >> + /* >> + * If the LED is off (brightness was set to 0), the last >> + * configured color was not necessarily sent to the MCU. >> + * Recompute with max_brightness and send if needed. >> + */ >> + led_mc_calc_color_components(mc_cdev, cdev->max_brightness); >> + >> + if (omnia_led_channels_changed(led)) >> + err = omnia_led_send_color_cmd(leds->client, led); >> + } >> + >> + if (!err) { >> + /* put the LED into MCU controlled mode */ >> + err = omnia_cmd_write(leds->client, CMD_LED_MODE, >> + CMD_LED_MODE_LED(led->reg)); >> + if (!err) >> + led->hwtrig = true; >> + } >> + >> + mutex_unlock(&leds->lock); >> + >> + return err; >> +} > > Pavel, Lee, here I wanted to ask: can the .activate() method of a LED > trigger sleep? The .brightness_set() method of a LED cannot sleep, and > therefore we have .brightness_set_blocking() method, which schedules > the potentially sleeping call into a work. But there is no such > mechanism for the LED trigger .activate() method. > > I looked at the LED core code, and it does not seem to me that > .activate() sleeping would cause issues, besides keeping trigger list > lock locked... > > Note that previously none of the LED triggers in drivers/leds/trigger > sleeped in .activate(), but recently the ethernet PHY subsystem was > wired into the netdev trigger, which may cause the .activate() method > to do a PHY transfer, which AFAIK may sleep... In general led_set_brightness() can't sleep because it is called from triggers in atomic contexts, which shouldn't be the case for activate(). .activate() is called from led_trigger_{set|remove}() which is called from led_classdev_{register|unregister}) and from sysfs trigger attr's led_trigger_write() handler, so no risk here. Triggers already call e.g. kzalloc() in .activate() which may sleep.
On Fri, 18 Aug 2023, Jacek Anaszewski wrote: > Hi Marek, > > On 8/2/23 18:13, Marek Behún wrote: > > On Wed, 2 Aug 2023 18:07:47 +0200 > > Marek Behún <kabel@kernel.org> wrote: > > > > > +static int omnia_hwtrig_activate(struct led_classdev *cdev) > > > +{ > > > + struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev); > > > + struct omnia_leds *leds = dev_get_drvdata(cdev->dev->parent); > > > + struct omnia_led *led = to_omnia_led(mc_cdev); > > > + int err = 0; > > > + > > > + mutex_lock(&leds->lock); > > > + > > > + if (!led->on) { > > > + /* > > > + * If the LED is off (brightness was set to 0), the last > > > + * configured color was not necessarily sent to the MCU. > > > + * Recompute with max_brightness and send if needed. > > > + */ > > > + led_mc_calc_color_components(mc_cdev, cdev->max_brightness); > > > + > > > + if (omnia_led_channels_changed(led)) > > > + err = omnia_led_send_color_cmd(leds->client, led); > > > + } > > > + > > > + if (!err) { > > > + /* put the LED into MCU controlled mode */ > > > + err = omnia_cmd_write(leds->client, CMD_LED_MODE, > > > + CMD_LED_MODE_LED(led->reg)); > > > + if (!err) > > > + led->hwtrig = true; > > > + } > > > + > > > + mutex_unlock(&leds->lock); > > > + > > > + return err; > > > +} > > > > Pavel, Lee, here I wanted to ask: can the .activate() method of a LED > > trigger sleep? The .brightness_set() method of a LED cannot sleep, and > > therefore we have .brightness_set_blocking() method, which schedules > > the potentially sleeping call into a work. But there is no such > > mechanism for the LED trigger .activate() method. > > > > I looked at the LED core code, and it does not seem to me that > > .activate() sleeping would cause issues, besides keeping trigger list > > lock locked... > > > > Note that previously none of the LED triggers in drivers/leds/trigger > > sleeped in .activate(), but recently the ethernet PHY subsystem was > > wired into the netdev trigger, which may cause the .activate() method > > to do a PHY transfer, which AFAIK may sleep... > > In general led_set_brightness() can't sleep because it is called from > triggers in atomic contexts, which shouldn't be the case for activate(). > > .activate() is called from led_trigger_{set|remove}() which is called from > led_classdev_{register|unregister}) and from sysfs trigger attr's > led_trigger_write() handler, so no risk here. > > Triggers already call e.g. kzalloc() in .activate() which may sleep. Thanks for stepping in Jacek. I really appreciate your help.
On Fri, 18 Aug 2023 10:09:21 +0100 Lee Jones <lee@kernel.org> wrote: > > + if (!err) { > > + /* put the LED into MCU controlled mode */ > > Nit: You improved the comment above to be more grammatically correct by > starting with an uppercase character. Please continue with this > improvement for all comments there in. OK. > > +static struct led_trigger omnia_hw_trigger = { > > + .name = "omnia-mcu", > > + .activate = omnia_hwtrig_activate, > > + .deactivate = omnia_hwtrig_deactivate, > > + .trigger_type = &omnia_hw_trigger_type, > > Not sure I understand this interface. > > Why not just set a bool instead of carrying around an empty struct? Let me explain: So, if a LED class device has the same trigger type as a LED trigger, the trigger will be available for that LED (it is listed in the sysfs trigger file and can be chosen). So we have a mechanism to "pair" a LED with a given trigger, to make it possible for the LED core to distinguish whether a given trigger is available for the LED. A boolean information would not be enough: if we used a bool, we would know that the trigger is private. But the LED core would not know for which LEDs the trigger should be avaiable. In pseudocode: list_triggers_for_led(led) { for (trigger in trigger_list) { if (!trigger.trigger_type || trigger.trigger_type == led.trigger_type) trigger is available for led else trigger is not available for led } } Is this explaination good enough? Marek
On Mon, 21 Aug 2023, Marek Behún wrote: > On Fri, 18 Aug 2023 10:09:21 +0100 > Lee Jones <lee@kernel.org> wrote: > > > > + if (!err) { > > > + /* put the LED into MCU controlled mode */ > > > > Nit: You improved the comment above to be more grammatically correct by > > starting with an uppercase character. Please continue with this > > improvement for all comments there in. > > OK. > > > > +static struct led_trigger omnia_hw_trigger = { > > > + .name = "omnia-mcu", > > > + .activate = omnia_hwtrig_activate, > > > + .deactivate = omnia_hwtrig_deactivate, > > > + .trigger_type = &omnia_hw_trigger_type, > > > > Not sure I understand this interface. > > > > Why not just set a bool instead of carrying around an empty struct? > > Let me explain: > > So, if a LED class device has the same trigger type as a LED trigger, > the trigger will be available for that LED (it is listed in the sysfs > trigger file and can be chosen). > > So we have a mechanism to "pair" a LED with a given trigger, to make it > possible for the LED core to distinguish whether a given trigger is > available for the LED. > > A boolean information would not be enough: if we used a bool, we would > know that the trigger is private. But the LED core would not know for > which LEDs the trigger should be avaiable. > > In pseudocode: > > list_triggers_for_led(led) { > for (trigger in trigger_list) { > if (!trigger.trigger_type || trigger.trigger_type == > led.trigger_type) > trigger is available for led > else > trigger is not available for led > } > } > > Is this explaination good enough? Yes, thank you.
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index 6046dfeca16f..ebb3b84d7a4f 100644 --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -187,6 +187,7 @@ config LEDS_TURRIS_OMNIA depends on I2C depends on MACH_ARMADA_38X || COMPILE_TEST depends on OF + select LEDS_TRIGGERS help This option enables basic support for the LEDs found on the front side of CZ.NIC's Turris Omnia router. There are 12 RGB LEDs on the diff --git a/drivers/leds/leds-turris-omnia.c b/drivers/leds/leds-turris-omnia.c index 636c6f802bcf..180b0cbeb92e 100644 --- a/drivers/leds/leds-turris-omnia.c +++ b/drivers/leds/leds-turris-omnia.c @@ -31,7 +31,7 @@ struct omnia_led { struct led_classdev_mc mc_cdev; struct mc_subled subled_info[OMNIA_LED_NUM_CHANNELS]; u8 cached_channels[OMNIA_LED_NUM_CHANNELS]; - bool on; + bool on, hwtrig; int reg; }; @@ -123,12 +123,14 @@ static int omnia_led_brightness_set_blocking(struct led_classdev *cdev, /* * Only recalculate RGB brightnesses from intensities if brightness is - * non-zero. Otherwise we won't be using them and we can save ourselves - * some software divisions (Omnia's CPU does not implement the division - * instruction). + * non-zero (if it is zero and the LED is in HW blinking mode, we use + * max_brightness as brightness). Otherwise we won't be using them and + * we can save ourselves some software divisions (Omnia's CPU does not + * implement the division instruction). */ - if (brightness) { - led_mc_calc_color_components(mc_cdev, brightness); + if (brightness || led->hwtrig) { + led_mc_calc_color_components(mc_cdev, brightness ?: + cdev->max_brightness); /* * Send color command only if brightness is non-zero and the RGB @@ -138,8 +140,11 @@ static int omnia_led_brightness_set_blocking(struct led_classdev *cdev, err = omnia_led_send_color_cmd(leds->client, led); } - /* send on/off state change only if (bool)brightness changed */ - if (!err && !brightness != !led->on) { + /* + * Send on/off state change only if (bool)brightness changed and the LED + * is not being blinked by HW. + */ + if (!err && !led->hwtrig && !brightness != !led->on) { u8 state = CMD_LED_STATE_LED(led->reg); if (brightness) @@ -155,6 +160,70 @@ static int omnia_led_brightness_set_blocking(struct led_classdev *cdev, return err; } +static struct led_hw_trigger_type omnia_hw_trigger_type; + +static int omnia_hwtrig_activate(struct led_classdev *cdev) +{ + struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev); + struct omnia_leds *leds = dev_get_drvdata(cdev->dev->parent); + struct omnia_led *led = to_omnia_led(mc_cdev); + int err = 0; + + mutex_lock(&leds->lock); + + if (!led->on) { + /* + * If the LED is off (brightness was set to 0), the last + * configured color was not necessarily sent to the MCU. + * Recompute with max_brightness and send if needed. + */ + led_mc_calc_color_components(mc_cdev, cdev->max_brightness); + + if (omnia_led_channels_changed(led)) + err = omnia_led_send_color_cmd(leds->client, led); + } + + if (!err) { + /* put the LED into MCU controlled mode */ + err = omnia_cmd_write(leds->client, CMD_LED_MODE, + CMD_LED_MODE_LED(led->reg)); + if (!err) + led->hwtrig = true; + } + + mutex_unlock(&leds->lock); + + return err; +} + +static void omnia_hwtrig_deactivate(struct led_classdev *cdev) +{ + struct omnia_leds *leds = dev_get_drvdata(cdev->dev->parent); + struct omnia_led *led = to_omnia_led(lcdev_to_mccdev(cdev)); + int err; + + mutex_lock(&leds->lock); + + led->hwtrig = false; + + /* put the LED into software mode */ + err = omnia_cmd_write(leds->client, CMD_LED_MODE, + CMD_LED_MODE_LED(led->reg) | CMD_LED_MODE_USER); + + mutex_unlock(&leds->lock); + + if (err < 0) + dev_err(cdev->dev, "Cannot put LED to software mode: %i\n", + err); +} + +static struct led_trigger omnia_hw_trigger = { + .name = "omnia-mcu", + .activate = omnia_hwtrig_activate, + .deactivate = omnia_hwtrig_deactivate, + .trigger_type = &omnia_hw_trigger_type, +}; + static int omnia_led_register(struct i2c_client *client, struct omnia_led *led, struct device_node *np) { @@ -198,6 +267,12 @@ static int omnia_led_register(struct i2c_client *client, struct omnia_led *led, cdev = &led->mc_cdev.led_cdev; cdev->max_brightness = 255; cdev->brightness_set_blocking = omnia_led_brightness_set_blocking; + cdev->trigger_type = &omnia_hw_trigger_type; + /* + * Use the omnia-mcu trigger as the default trigger. It may be rewritten + * by LED class from the linux,default-trigger property. + */ + cdev->default_trigger = omnia_hw_trigger.name; /* put the LED into software mode */ ret = omnia_cmd_write(client, CMD_LED_MODE, CMD_LED_MODE_LED(led->reg) | @@ -310,6 +385,12 @@ static int omnia_leds_probe(struct i2c_client *client) mutex_init(&leds->lock); + ret = devm_led_trigger_register(dev, &omnia_hw_trigger); + if (ret < 0) { + dev_err(dev, "Cannot register private LED trigger: %d\n", ret); + return ret; + } + led = &leds->leds[0]; for_each_available_child_of_node(np, child) { ret = omnia_led_register(client, led, child);
Add support for enabling MCU controlled mode of the Turris Omnia LEDs via a LED private trigger called "omnia-mcu". Recall that private LED triggers will only be listed in the sysfs trigger file for LEDs that support them (currently there is no user of this mechanism). When in MCU controlled mode, the user can still set LED color, but the blinking is done by MCU, which does different things for different LEDs: - WAN LED is blinked according to the LED[0] pin of the WAN PHY - LAN LEDs are blinked according to the LED[0] output of the corresponding port of the LAN switch - PCIe LEDs are blinked according to the logical OR of the MiniPCIe port LED pins In the future I want to make the netdev trigger to transparently offload the blinking to the HW if user sets compatible settings for the netdev trigger (for LEDs associated with network devices). There was some work on this already, and hopefully we will be able to complete it sometime, but for now there are still multiple blockers for this, and even if there weren't, we still would not be able to configure HW controlled mode for the LEDs associated with MiniPCIe ports. In the meantime let's support HW controlled mode via the private LED trigger mechanism. If, in the future, we manage to complete the netdev trigger offloading, we can still keep this private trigger for backwards compatibility, if needed. We also set "omnia-mcu" to cdev->default_trigger, so that the MCU keeps control until the user first wants to take over it. If a different default trigger is specified in device-tree via the 'linux,default-trigger' property, LED class will overwrite cdev->default_trigger, and so the DT property will be respected. Signed-off-by: Marek Behún <kabel@kernel.org> --- drivers/leds/Kconfig | 1 + drivers/leds/leds-turris-omnia.c | 97 +++++++++++++++++++++++++++++--- 2 files changed, 90 insertions(+), 8 deletions(-)