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
