Messages in this thread | | | From | Andy Shevchenko <> | Date | Tue, 18 May 2021 01:00:41 +0300 | Subject | Re: [PATCH v2 7/7] leds: Add support for RTL8231 LED scan matrix |
| |
On Mon, May 17, 2021 at 10:28 PM Sander Vanheule <sander@svanheule.net> wrote: > > Both single and bi-color scanning modes are supported. The driver will > verify that the addresses are valid for the current mode, before > registering the LEDs. LEDs can be turned on, off, or toggled at one of > six predefined rates from 40ms to 1280ms. > > Implements a platform device for use as child device with RTL8231 MFD,
as a child
> and uses the parent regmap to access the required registers.
...
> + When built as a module, this module will be named rtl8231_leds.
Again, what it's written here is not what is in Makefile.
> +obj-$(CONFIG_LEDS_RTL8231) += leds-rtl8231.o
...
> +/** > + * struct led_toggle_rate - description of an LED blinking mode > + * @interval: LED toggle rate in ms > + * @mode: Register field value used to active this mode
activate
> + * > + * For LED hardware accelerated blinking, with equal on and off delay. > + * Both delays are given by @interval, so the interval at which the LED blinks > + * (i.e. turn on and off once) is double this value. > + */
...
> +static unsigned int rtl8231_led_current_interval(struct rtl8231_led *pled) > +{ > + unsigned int mode;
> + unsigned int i = 0;
This...
> + if (regmap_field_read(pled->reg_field, &mode)) > + return 0; > + > + while (i < pled->modes->num_toggle_rates && mode != pled->modes->toggle_rates[i].mode) > + i++;
...and this will be better as a for-loop.
> + if (i < pled->modes->num_toggle_rates) > + return pled->modes->toggle_rates[i].interval;
> + else
Redundant.
> + return 0; > +}
...
> + unsigned int i = 0;
As per above.
...
> + interval = 500;
interval_ms
> + /* > + * If the current mode is blinking, choose the delay that (likely) changed. > + * Otherwise, choose the interval that would have the same total delay. > + */ > + interval = rtl8231_led_current_interval(pled);
> +
Redundant blank line.
> + if (interval > 0 && interval == *delay_off) > + interval = *delay_on; > + else if (interval > 0 && interval == *delay_on) > + interval = *delay_off; > + else > + interval = (*delay_on + *delay_off) / 2; > + }
...
> + u32 addr[2]; > + int err; > +
> + if (!fwnode_property_count_u32(fwnode, "reg"))
err = fwnode_property_count_u32(...); if (err < 0) return err; if (err == 0) return -ENODEV;
> + return -ENODEV; > + > + err = fwnode_property_read_u32_array(fwnode, "reg", addr, ARRAY_SIZE(addr));
If count returns 1? What's the point of counting if you always want two?
> + if (err) > + return err; > + > + *addr_port = addr[0]; > + *addr_led = addr[1]; > + > + return 0; > +}
...
> + pled = devm_kzalloc(dev, sizeof(*pled), GFP_KERNEL); > + if (IS_ERR(pled))
Wrong.
> + return PTR_ERR(pled);
...
> + err = rtl8231_led_read_address(fwnode, &port_index, &led_index); > +
Redundant blank line.
> + if (err) { > + dev_err(dev, "LED address invalid\n"); > + return err;
> + } else if (led_index >= RTL8231_NUM_LEDS || port_index >= port_counts[led_index]) {
Redundant 'else'
> + dev_err(dev, "LED address (%d.%d) invalid\n", port_index, led_index); > + return -ENODEV; > + }
...
> + map = dev_get_regmap(dev->parent, NULL); > + if (IS_ERR_OR_NULL(map)) {
Split it into two conditionals.
> + dev_err(dev, "failed to retrieve regmap\n"); > + if (!map) > + return -ENODEV; > + else > + return PTR_ERR(map); > + }
...
> + if (!device_property_match_string(dev, "realtek,led-scan-mode", "single-color")) {
It seems that device_property_match_string() and accompanying functions have wrong description of returned codes, i.e. it returns the index of the matched string. It's possible that some APIs are broken (but I believe that the former is the case).
That said, I think the proper comparison should be >= 0.
> + port_counts = rtl8231_led_port_counts_single; > + regmap_update_bits(map, RTL8231_REG_FUNC0, > + RTL8231_FUNC0_SCAN_MODE, RTL8231_FUNC0_SCAN_SINGLE); > + } else if (!device_property_match_string(dev, "realtek,led-scan-mode", "bi-color")) {
Ditto.
> + port_counts = rtl8231_led_port_counts_bicolor; > + regmap_update_bits(map, RTL8231_REG_FUNC0, > + RTL8231_FUNC0_SCAN_MODE, RTL8231_FUNC0_SCAN_BICOLOR); > + } else { > + dev_err(dev, "scan mode missing or invalid\n"); > + return -EINVAL; > + }
-- With Best Regards, Andy Shevchenko
| |