linux-kernel - Re: [PATCH v2 7/7] leds: Add support for RTL8231 LED scan matrix

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 23 May 2021 23:53:44 +0200
From:   Sander Vanheule <sander@...nheule.net>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Pavel Machek <pavel@....cz>, Rob Herring <robh+dt@...nel.org>,
        Lee Jones <lee.jones@...aro.org>,
        Mark Brown <broonie@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J . Wysocki" <rafael@...nel.org>,
        Michael Walle <michael@...le.cc>,
        Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Linux LED Subsystem <linux-leds@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Andrew Lunn <andrew@...n.ch>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 7/7] leds: Add support for RTL8231 LED scan matrix

Hi Andy,

Also here I've tried to address your remarks for v3, some extra details below.

On Tue, 2021-05-18 at 01:00 +0300, Andy Shevchenko wrote:
> On Mon, May 17, 2021 at 10:28 PM Sander Vanheule <sander@...nheule.net> wrote:
> > +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;
> > +}

Shrunk down to "for (...) if (...) return ...;" in v3.


> 
> > +               interval = 500;
> 
> interval_ms

Good suggestion, thanks. Don't need those comments in the code then.


> 
> > +       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 count returns 1, or more than 2, that's an error. But this check was missing
in v2, so I added it in v3.


> 
> > +       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.

Thanks, fixed.


Best,
Sander