linux-kernel - Re: [PATCH v3 2/2] leds: add aw20xx driver

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Tue, 14 Mar 2023 18:22:25 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Martin Kurbanov <mmkurbanov@...rdevices.ru>
Cc:     Pavel Machek <pavel@....cz>, Lee Jones <lee@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        linux-leds@...r.kernel.org, linux-kernel@...r.kernel.org,
        kernel@...rdevices.ru, devicetree@...r.kernel.org
Subject: Re: [PATCH v3 2/2] leds: add aw20xx driver

On Tue, Mar 14, 2023 at 2:12 PM Martin Kurbanov
<mmkurbanov@...rdevices.ru> wrote:
>
> Hello Andy. Thank you for review.
> I have fixed most of your comments. Please take a look below.

Good, but you can postpone issuing a new version before letting me answer.

> On 2023-03-01 00:51, Andy Shevchenko wrote:
> >> +       /* The output current of each LED (see p.14 of datasheet for formula) */
> >> +       return (duty * global_imax_microamp) / 1000U;
> >
> > units.h ?
>
> These constants are needed to improve the accuracy of calculations.
> units.h doesn’t have any helpful definitions to use here.

Okay, let me look at v3 and I will comment there.

...

> >> +static int aw200xx_set_imax(const struct aw200xx *const chip,
> >> +                           u32 led_imax_microamp)
> >> +{
> >> +       struct imax_global {
> >> +               u32 regval;
> >> +               u32 microamp;
> >> +       } imaxs[] = {
> >> +               { 8,  3300 },
> >> +               { 9,  6700 },
> >> +               { 0,  10000 },
> >> +               { 11, 13300 },
> >> +               { 1,  20000 },
> >> +               { 13, 26700 },
> >> +               { 2,  30000 },
> >> +               { 3,  40000 },
> >> +               { 15, 53300 },
> >> +               { 4,  60000 },
> >> +               { 5,  80000 },
> >> +               { 6,  120000 },
> >> +               { 7,  160000 },
> >
> > This looks a bit random. Is there any pattern on how value is
> > connected to the register value?
>
> There is no ability to create any pattern here, because this table data
> doesn’t have any regularity.

There is a clear pattern.

You have two tables, i.e. with multiplier 10000 and second one with
multiplier 3333 (table in the datasheet seems bad from a math
perspective). And it's even shown correctly in the datasheet.

With this mix you missed 10.

The coefficient table is 1,2,3,4,6,8,12,16 for both tables.

Hence you need one table and two multipliers.

Please, rewrite accordingly.

-- 
With Best Regards,
Andy Shevchenko