MIME-Version: 1.0
References: <20230314120252.48263-1-mmkurbanov@sberdevices.ru>
 <20230314120252.48263-3-mmkurbanov@sberdevices.ru> <3b920b9e-07dc-7bda-4fe1-d15d07e708cc@sberdevices.ru>
In-Reply-To: <3b920b9e-07dc-7bda-4fe1-d15d07e708cc@sberdevices.ru>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Tue, 14 Mar 2023 18:22:25 +0200
Message-ID: <CAHp75VcWuOEWZn2E8dG=Pb_KuEv06jYt_+nZSL-ceAQRPmgeGw@mail.gmail.com>
Subject: Re: [PATCH v3 2/2] leds: add aw20xx driver
To:     Martin Kurbanov <mmkurbanov@sberdevices.ru>
Cc:     Pavel Machek <pavel@ucw.cz>, Lee Jones <lee@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        linux-leds@vger.kernel.org, linux-kernel@vger.kernel.org,
        kernel@sberdevices.ru, devicetree@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Mar 14, 2023 at 2:12=E2=80=AFPM Martin Kurbanov
<mmkurbanov@sberdevices.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 f=
ormula) */
> >> +       return (duty * global_imax_microamp) / 1000U;
> >
> > units.h ?
>
> These constants are needed to improve the accuracy of calculations.
> units.h doesn=E2=80=99t 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[] =3D {
> >> +               { 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=E2=80=99t 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.

--=20
With Best Regards,
Andy Shevchenko