Re: [PATCH v2 2/2] leds: Add support for Turris 1.x LEDs
From: Andy Shevchenko
Date: Tue Jul 05 2022 - 14:41:09 EST
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", ®);
> + 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