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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Jul 5, 2022 at 6:11 PM Pali Rohár <pali@xxxxxxxxxx> wrote:
>
> This adds support for the RGB LEDs found on the front panel of the
> Turris 1.x routers. There are 8 RGB LEDs that are controlled by
> CZ.NIC CPLD firmware running on Lattice FPGA.
>
> CPLD firmware provides HW triggering mode for all LEDs except WiFi LED
> which is automatically enabled after power on reset. LAN LEDs share HW
> registers for RGB colors settings, so it is not possible to set different
> colors for individual LAN LEDs.
>
> CZ.NIC CPLD firmware is open source and available at:
> https://gitlab.nic.cz/turris/hw/turris_cpld/-/blob/master/CZ_NIC_Router_CPLD.v
>
> This driver uses the multicolor LED framework and HW led triggers.

Pardon me, but this driver seems like 3 years old by the APIs it's
using... I have to say this, because I was surprised a lot to see some
calls.

...

> +config LEDS_TURRIS_1X
> +       tristate "LED support for CZ.NIC's Turris 1.x"
> +       depends on LEDS_CLASS_MULTICOLOR

> +       depends on OF

Why?

If it's a functional (not compile time) dependency, make it

  depends on OF || COMPILE_TEST

> +       select LEDS_TRIGGERS
> +       help
> +         This option enables support for LEDs found on the front side of
> +         CZ.NIC's Turris 1.x routers.

...

> +#include <linux/i2c.h>
> +#include <linux/led-class-multicolor.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>

> +#include <linux/of.h>

Rather property.h. See below how.

...

> +/* LEDs 1-5 share common register for setting brightness */
> +#define TURRIS1X_LED_BRIGHTNESS_OFF(idx)       ({ const u8 _idx = (idx) & 7; \

Can you start with the GCC expression on a new line? I may give a much
shorter next line and increase readability.

> +                                                  (_idx == 0) ? 0 : \
> +                                                  (_idx <= 5) ? 1 : \
> +                                                  (_idx - 4); })
> +
> +#define TURRIS1X_LED_BRIGHTNESS_REG(idx, color)        TURRIS1X_LED_REG_OFF(0x13 + \
> +                                                 3 * TURRIS1X_LED_BRIGHTNESS_OFF(idx) + \
> +                                                 ((color) & 3))

Ditto.

...

> +static enum led_brightness turris1x_led_brightness_get(struct led_classdev *cdev)
> +{
> +       struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
> +       struct turris1x_leds *leds = dev_get_drvdata(cdev->dev->parent);
> +       struct turris1x_led *led = to_turris1x_led(mc_cdev);
> +
> +       if (!(readb(leds->regs + TURRIS1X_LED_SW_OVERRIDE_REG) & BIT(led->reg)))
> +               return 1;
> +       else if (!(readb(leds->regs + TURRIS1X_LED_SW_DISABLE_REG) & BIT(led->reg)))
> +               return 1;
> +       else

Redundant 'else' in both cases.

> +               return 0;
> +}

...

> +               /*
> +                * LEDs 1-5 (LAN) share common color settings in same sets
> +                * of HW registers and therefore it is not possible to set
> +                * different colors. So when chaning color of one LED then

chaining

> +                * reflect color change for all of them.
> +                */

> +               if (led->reg >= 1 && led->reg <= 5) {

Same is used in the macro above. Maybe you can provide a shortcut for
this instead of duplicating?

> +                       for (j = 0; j < ARRAY_SIZE(leds->leds); j++) {

> +                               if (leds->leds[j].reg < 1 ||
> +                                   leds->leds[j].reg > 5 ||

Ditto.

> +                                   leds->leds[j].reg == led->reg)
> +                                       continue;
> +                               for (i = 0; i < ARRAY_SIZE(led->subled_info); i++)
> +                                       leds->leds[j].mc_cdev.subled_info[i].intensity =
> +                                               mc_cdev->subled_info[i].intensity;
> +                       }
> +               }
> +       }

...

> +       ret = of_property_read_u32(np, "reg", &reg);
> +       if (ret || reg >= ARRAY_SIZE(leds->leds)) {
> +               dev_err(dev,
> +                       "Node %pOF: must contain 'reg' property with values between 0 and %u\n",
> +                       np, (unsigned int)ARRAY_SIZE(leds->leds) - 1);
> +               return -EINVAL;
> +       }
> +
> +       ret = of_property_read_u32(np, "color", &color);
> +       if (ret || color != LED_COLOR_ID_RGB) {
> +               dev_err(dev,
> +                       "Node %pOF: must contain 'color' property with value LED_COLOR_ID_RGB\n",
> +                       np);
> +               return -EINVAL;
> +       }
> +
> +       led = &leds->leds[reg];
> +
> +       if (led->registered) {
> +               dev_err(dev, "Node %pOF: duplicate 'reg' property %u\n",
> +                            np, reg);
> +               return -EINVAL;
> +       }

> +       init_data.fwnode = &np->fwnode;

Oh, no. We do not dereference fwnode, we have helpers for that.
Moreover, why not use fwnode to begin with?

> +       ret = devm_led_classdev_multicolor_register_ext(dev, &led->mc_cdev,
> +                                                       &init_data);
> +       if (ret) {
> +               dev_err(dev, "Cannot register LED %pOF: %i\n", np, ret);
> +               return ret;
> +       }
> +
> +       return 0;
> +}

...

> +static ssize_t brightness_show(struct device *dev, struct device_attribute *a,
> +                              char *buf)
> +{
> +       struct turris1x_leds *leds = dev_get_drvdata(dev);
> +       unsigned int brightness;
> +
> +       /*
> +        * Current brightness value is available in read-only register
> +        * TURRIS1X_LED_GET_GLOBAL_BRIGHTNESS_REG. Equivalent code is:
> +        * level = readb(leds->regs + TURRIS1X_LED_GLOBAL_LEVEL_REG) & 7;
> +        * brightness = readb(leds->regs + TURRIS1X_LED_GLOBAL_BRIGHTNESS_REG(level));
> +        */
> +       brightness = readb(leds->regs + TURRIS1X_LED_GET_GLOBAL_BRIGHTNESS_REG);
> +
> +       return sprintf(buf, "%u\n", brightness);

sysfs_emit()

> +}

...

> +       if (kstrtoul(buf, 10, &brightness))
> +               return -EINVAL;

Why shadow the error code from kstrtoul()? Note it might return
something different.

Do you really need unsigned long? Can't you use u8 and kstrtou8() respectively?

> +       if (brightness > 255)
> +               return -EINVAL;

Yeah, read above about u8.

...

> +       /*
> +        * Brightness can be set only to one of 8 predefined value levels
> +        * available in TURRIS1X_LED_GLOBAL_BRIGHTNESS_REG(level) registers.
> +        * Choose level which has nearest value to the specified brightness.

a level
the nearest

> +        */

...

> +               error = abs(value - (int)brightness);

Why casting?!

...

> +static ssize_t brightness_level_show(struct device *dev,
> +                                    struct device_attribute *a, char *buf)
> +{
> +       struct turris1x_leds *leds = dev_get_drvdata(dev);
> +       unsigned int level;
> +
> +       level = readb(leds->regs + TURRIS1X_LED_GLOBAL_LEVEL_REG) & 7;
> +
> +       return sprintf(buf, "%u\n", level);

sysfs_emit()

> +}

...

> +       if (kstrtoul(buf, 10, &level))
> +               return -EINVAL;

As per above.

...

> +static ssize_t brightness_values_show(struct device *dev,
> +                                     struct device_attribute *a, char *buf)
> +{
> +       struct turris1x_leds *leds = dev_get_drvdata(dev);
> +       unsigned int vals[8];
> +       int i;
> +
> +       for (i = 0; i < 8; i++)
> +               vals[i] = readb(leds->regs +
> +                               TURRIS1X_LED_GLOBAL_BRIGHTNESS_REG(i));
> +
> +       return sprintf(buf, "%u %u %u %u %u %u %u %u\n", vals[0], vals[1],
> +                      vals[2], vals[3], vals[4], vals[5], vals[6], vals[7]);

sysfs_emit()
Wouldn't it be better to have CSV instead? I think for such cases we
usually have this kind of format.

> +}

...

> +static struct attribute *turris1x_leds_controller_attrs[] = {
> +       &dev_attr_brightness.attr,
> +       &dev_attr_brightness_level.attr,
> +       &dev_attr_brightness_values.attr,

> +       NULL,

No comma for terminator.

> +};

...

> +static int turris1x_leds_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;

> +       struct device_node *np = dev_of_node(dev);
> +       struct device_node *child;

Why not use fwnode to begin with?

> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

> +       if (!res)
> +               return -ENODEV;

Besides dup code, which actually does not print a message...

> +       regs = devm_ioremap_resource(dev, res);

...we have devm_platform_ioremap_resource() to combine two above into one.

> +       if (IS_ERR(regs))
> +               return PTR_ERR(regs);

...

> +       ret = devm_led_trigger_register(dev, &turris1x_hw_trigger);
> +       if (ret) {
> +               dev_err(dev, "Cannot register private LED trigger: %d\n", ret);
> +               return ret;

return dev_err_probe(...);

> +       }

...

> +       for_each_available_child_of_node(np, child) {

device_for_each_child_node()

> +               ret = turris1x_led_register(dev, leds, child,
> +                                           val_sw_override, val_sw_disable);
> +               if (ret) {

> +                       of_node_put(child);

fwnode_handle_put()

> +                       return ret;
> +               }
> +       }

...

> +       ret = devm_device_add_groups(dev, turris1x_leds_controller_groups);
> +       if (ret) {
> +               dev_err(dev, "Could not add attribute group!\n");
> +               return ret;

return dev_err_probe(...);

> +       }
> +
> +       return 0;
> +}

...

> +       /*
> +        * LED registers are persisent across board resets.

persistent

> +        * So reset LEDs to default state before kernel reboots.
> +        */

...

> +                       writeb(0xff,

GENMASK() ?

> +                              leds->regs + TURRIS1X_LED_BRIGHTNESS_REG(i, j));
> +       }
> +}

...

> +static const struct of_device_id of_turris1x_leds_match[] = {
> +       { .compatible = "cznic,turris1x-leds" },
> +       {},

No comma for terminator.

> +};

-- 
With Best Regards,
Andy Shevchenko




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux