Messages in this thread | | | From | Andy Shevchenko <> | Date | Tue, 5 Jul 2022 20:40:26 +0200 | Subject | Re: [PATCH v2 2/2] leds: Add support for Turris 1.x LEDs |
| |
On Tue, Jul 5, 2022 at 6:11 PM Pali Rohár <pali@kernel.org> 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
| |