Re: [PATCH v3 2/2] leds: Add support for Turris 1.x LEDs

[Andy Shevchenko]

On Mon, Sep 28, 2026 at 2:19 PM Josef Schlehofer
<pepe.schlehofer@xxxxxxxxx> wrote:
>
> From: Pali Rohár <pali@xxxxxxxxxx>
>
> Add a driver for the eight RGB LEDs on the front panel of the CZ.NIC
> Turris 1.x routers. They are driven by the CZ.NIC CPLD firmware, whose
> source is available at
> https://gitlab.nic.cz/turris/hw/turris_cpld/-/blob/master/CZ_NIC_Router_CPLD.v
>
> The LEDs use the multicolor LED class. Every LED except the WiFi LED can
> also be driven by the CPLD from hardware events, exposed as the private
> turris1x-cpld trigger. The five LAN LEDs share one set of colour
> registers, so the colour and brightness set last on any of them apply
> to all five.
>
> The controller device exposes the global brightness controlled by the
> button on the back of the router through `brightness`,
> `brightness_level`, and `brightness_levels/<N>`. The Turris Omnia
> driver already has `brightness`, so its ABI entry is extended to cover
> the Turris 1.x as well.

> Signed-off-by: Pali Rohár <pali@xxxxxxxxxx>

> Cc: Marek Behún <kabel@xxxxxxxxxx>
> Cc: Andy Shevchenko <andy@xxxxxxxxxx>

Please, move these (Cc list) to the block under the cutter '---' line,
so it won't pollute the commit message.

> Co-developed-by: Josef Schlehofer <pepe.schlehofer@xxxxxxxxx>
> Signed-off-by: Josef Schlehofer <pepe.schlehofer@xxxxxxxxx>
> ---

...

>  What:          /sys/class/leds/<led>/device/brightness
> -Date:          July 2020
> -KernelVersion: 5.9
> -Contact:       Marek Behún <kabel@xxxxxxxxxx>

> +Date:          July 2020 (Turris Omnia), September 2026 (Turris 1.x)
> +KernelVersion: 5.9 (Turris Omnia), 7.4 (Turris 1.x)

This is quite unusual. If you want to refer to the kernel version, do
it in the description. Also note the version (see below more on it).

> +Contact:       Marek Behún <kabel@xxxxxxxxxx>, linux-leds@xxxxxxxxxxxxxxx

Why? The mailing list is kinda default, no?

...

> +What:          /sys/class/leds/<led>/device/brightness_level
> +Date:          September 2026

Impossible. Use https://hansen.beer/~dave/phb/ to predict the dates of
the next release, this is a huge driver that most likely may not make
v7.4, so the v7.5 is a plausible candidate.

> +Contact:       Josef Schlehofer <pepe.schlehofer@xxxxxxxxx>
> +Description:   (RW) Index (0-7) of the global brightness level in use on the
> +               Turris 1.x routers. The button on the back side of the router
> +               steps through the levels. Writing to this file selects a level
> +               directly. The CPLD keeps the selection across driver unbind and

the driver

> +               reboot.
> +
> +               Format: %u
> +
> +What:          /sys/class/leds/<led>/device/brightness_levels/<N>
> +Date:          September 2026

As per above

> +Contact:       Josef Schlehofer <pepe.schlehofer@xxxxxxxxx>
> +Description:   (RW) Value of the global brightness level N (0-7) on the
> +               Turris 1.x routers, one file per level. These are the values
> +               the CPLD firmware steps through when the brightness button is
> +               pressed. Reading returns the value, writing accepts an integer
> +               between 0 and 255. The CPLD keeps the values across driver

the driver

> +               unbind and reboot, and restores the defaults (255 64 32 16 8 4
> +               2 0) at power-on and when the reset button is pressed.

...

> +#include <linux/bits.h>
> +#include <linux/container_of.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/kstrtox.h>
> +#include <linux/led-class-multicolor.h>
> +#include <linux/leds.h>
> +#include <linux/limits.h>
> +#include <linux/math.h>
> +#include <linux/minmax.h>

> +#include <linux/mod_devicetable.h>

Not anymore. Rely on what platform_device.h provides.

> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/spinlock.h>
> +#include <linux/sysfs.h>
> +#include <linux/types.h>

...

> +struct turris1x_leds {
> +       void __iomem *regs;
> +       /* Protects the CPLD LED registers and @reset */
> +       spinlock_t lock;
> +       /* Set by turris1x_leds_reset(), after which the LEDs are left alone */
> +       bool reset;
> +       struct turris1x_led led[TURRIS1X_LED_NUM];

Please, check the layout of all structures with `pahole`, it might
suggest a better one.

> +};

...

> +/* Must be called with ddata->lock held */

This is good, but having a lockdep annotation is even better.

> +static void turris1x_led_set_colors(struct turris1x_leds *ddata, struct turris1x_led *led,
> +                                   enum led_brightness brightness)
> +{
> +       struct led_classdev_mc *mc_cdev = &led->mc_cdev;
> +       unsigned int i;
> +       u8 val;
> +
> +       led_mc_calc_color_components(mc_cdev, brightness);
> +
> +       /* The colour registers are 8 bits wide, do not let values wrap */
> +       for (i = 0; i < TURRIS1X_LED_NUM_COLORS; i++) {

for (unsigned int i...) {

> +               val = min_t(unsigned int, mc_cdev->subled_info[i].brightness, U8_MAX);

No min_t(), it should be an exceptional use, and it was especially
proven to have issues with < INT_MAX comparisons in some cases. I
think you mean to have clamp() here.

> +               turris1x_write(ddata, turris1x_color_reg(led->reg, i), val);
> +       }
> +}

...

> +static int turris1x_hwtrig_activate(struct led_classdev *cdev)
> +{
> +       struct turris1x_leds *ddata = dev_get_drvdata(cdev->dev->parent);
> +       struct turris1x_led *led = to_turris1x_led(cdev);
> +       unsigned long flags;
> +       u8 val;
> +
> +       spin_lock_irqsave(&ddata->lock, flags);

Why not guard()()?

> +       if (ddata->reset)
> +               goto unlock;
> +
> +       /*
> +        * If software turned the LED off, the last configured colour was not
> +        * necessarily written to the CPLD. Write it with max_brightness before
> +        * the hardware takes over.
> +        */
> +       if (!led->on)
> +               turris1x_led_set_colors(ddata, led, cdev->max_brightness);
> +
> +       /* Disable LED software control */
> +       val = turris1x_read(ddata, TURRIS1X_LED_SW_OVERRIDE_REG);
> +       turris1x_write(ddata, TURRIS1X_LED_SW_OVERRIDE_REG, val & ~BIT(led->reg));
> +
> +       led->hwtrig = true;
> +
> +unlock:
> +       spin_unlock_irqrestore(&ddata->lock, flags);
> +
> +       return 0;
> +}

...

> +       /*
> +        * Enable or disable the LED under software control. The CPLD ignores
> +        * this bit while the hardware trigger drives the LED, so leave it
> +        * alone.
> +        */
> +       if (!led->hwtrig) {
> +               val = turris1x_read(ddata, TURRIS1X_LED_SW_DISABLE_REG);
> +               if (brightness)
> +                       val &= ~BIT(led->reg);
> +               else
> +                       val |= BIT(led->reg);

If led->reg is unsigned long, you can use __asign_bit() here.

> +               turris1x_write(ddata, TURRIS1X_LED_SW_DISABLE_REG, val);
> +               led->on = !!brightness;
> +       }

...

> +static int turris1x_led_register(struct device *dev, struct turris1x_leds *ddata,
> +                                struct fwnode_handle *fwnode, u8 val_sw_override,
> +                                u8 val_sw_disable)
> +{
> +       static const unsigned int colors[TURRIS1X_LED_NUM_COLORS] = {
> +               LED_COLOR_ID_RED, LED_COLOR_ID_GREEN, LED_COLOR_ID_BLUE,
> +       };
> +       struct led_init_data init_data = {};
> +       struct led_classdev *cdev;
> +       struct turris1x_led *led;
> +       unsigned long flags;
> +       u8 val, dis;
> +       u32 reg, color;
> +       unsigned int i;
> +       int ret;
> +
> +       ret = fwnode_property_read_u32(fwnode, "reg", &reg);
> +       if (ret || reg >= TURRIS1X_LED_NUM)
> +               return dev_err_probe(dev, -EINVAL, "Invalid or missing 'reg' property\n");
> +
> +       ret = fwnode_property_read_u32(fwnode, "color", &color);
> +       if (ret || color != LED_COLOR_ID_RGB)
> +               return dev_err_probe(dev, -EINVAL, "Invalid or missing 'color' property\n");

Do not shadow the error code.

> +       led = &ddata->led[reg];
> +       if (led->registered)
> +               return dev_err_probe(dev, -EINVAL, "LED %u already registered\n", reg);

EBUSY / EEXIST ?

> +       led->reg = reg;
> +
> +       /* Set the initial colours to those currently in use */
> +       for (i = 0; i < TURRIS1X_LED_NUM_COLORS; i++) {

for (unsigned int i...) {

> +               led->subled_info[i].intensity = turris1x_read(ddata, turris1x_color_reg(reg, i));
> +               led->subled_info[i].color_index = colors[i];
> +               led->subled_info[i].channel = i;
> +       }
> +
> +       /*
> +        * LEDs 1-5 (LAN) share one set of colour registers, and the brightness
> +        * of an LED is applied by scaling its colour, so all of them show the
> +        * colour and brightness written last. Each LED still keeps its own
> +        * intensities and brightness, so multi_intensity and brightness report
> +        * what the LED was last given, which is not necessarily what it shows.
> +        */
> +       led->mc_cdev.subled_info = led->subled_info;
> +       led->mc_cdev.num_colors = TURRIS1X_LED_NUM_COLORS;
> +
> +       init_data.fwnode = fwnode;
> +
> +       cdev = &led->mc_cdev.led_cdev;
> +       cdev->max_brightness = 255;
> +       cdev->brightness_set = turris1x_led_brightness_set;
> +
> +       /* All LEDs except the WiFi LED can be driven by the hardware trigger */
> +       if (reg != TURRIS1X_LED_WIFI) {
> +               cdev->trigger_type = &turris1x_hw_trigger_type;
> +               cdev->hw_control_trigger = turris1x_hw_trigger.name;
> +       }
> +
> +       if (!(val_sw_override & BIT(reg)))
> +               cdev->default_trigger = turris1x_hw_trigger.name;
> +
> +       if (!(val_sw_override & BIT(reg)) || !(val_sw_disable & BIT(reg)))
> +               cdev->brightness = cdev->max_brightness;
> +
> +       led->on = !(val_sw_disable & BIT(reg));
> +
> +       ret = devm_led_classdev_multicolor_register_ext(dev, &led->mc_cdev, &init_data);
> +       if (ret)
> +               return dev_err_probe(dev, ret, "Cannot register LED %u\n", reg);
> +
> +       /*
> +        * A linux,default-trigger property replaces the hardware trigger, and
> +        * the CPLD then keeps driving the LED and ignores software control.
> +        * Take such an LED over in the state the LED core reports.
> +        */
> +       spin_lock_irqsave(&ddata->lock, flags);
> +       val = turris1x_read(ddata, TURRIS1X_LED_SW_OVERRIDE_REG);
> +       if (!led->hwtrig && !(val & BIT(reg))) {
> +               dis = turris1x_read(ddata, TURRIS1X_LED_SW_DISABLE_REG);
> +               if (cdev->brightness)
> +                       dis &= ~BIT(reg);
> +               else
> +                       dis |= BIT(reg);
> +               turris1x_write(ddata, TURRIS1X_LED_SW_DISABLE_REG, dis);
> +               led->on = !!cdev->brightness;
> +               turris1x_write(ddata, TURRIS1X_LED_SW_OVERRIDE_REG, val | BIT(reg));
> +       }
> +       spin_unlock_irqrestore(&ddata->lock, flags);
> +
> +       led->registered = true;
> +
> +       return 0;
> +}

...

> +       best_level = 0;
> +       best_error = INT_MAX;
> +       for (level = 0; level < TURRIS1X_LED_NUM_LEVELS; level++) {

Ditto.

> +               value = turris1x_read(ddata, TURRIS1X_LED_LEVEL_VALUE_REG + level);
> +               error = abs(value - brightness);
> +               if (error < best_error) {
> +                       best_error = error;
> +                       best_level = level;
> +               }
> +       }

...

> +/* _level is always a literal 0..7, it is pasted, stringified and stored */
> +#define TURRIS1X_LEVEL_ATTR(_level)                                            \
> +       static struct turris1x_level_attr turris1x_level_attr_##_level = {      \
> +               .attr = __ATTR(_level, 0644, brightness_level_value_show,       \
> +                              brightness_level_value_store),                   \

Don't we have __ATTR_RW() ?

> +               .level = _level,                                                \
> +       }

...

> +       device_for_each_child_node_scoped(dev, child) {
> +               ret = turris1x_led_register(dev, ddata, child, val_sw_override, val_sw_disable);
> +               if (ret)
> +                       return ret;
> +               count++;
> +       }

> +       if (!count)
> +               return dev_err_probe(dev, -ENODEV, "No LED devices found in device tree\n");

We have counting API, so this can be done ahead. Yes, it will iterate
over the list twice, but I don't think it's an issue.

-- 
With Best Regards,
Andy Shevchenko