Message-ID: <3b920b9e-07dc-7bda-4fe1-d15d07e708cc@sberdevices.ru>
Date:   Tue, 14 Mar 2023 15:12:29 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH v3 2/2] leds: add aw20xx driver
Content-Language: en-US
To:     Andy Shevchenko <andy.shevchenko@gmail.com>
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>
References: <20230314120252.48263-1-mmkurbanov@sberdevices.ru>
 <20230314120252.48263-3-mmkurbanov@sberdevices.ru>
From:   Martin Kurbanov <mmkurbanov@sberdevices.ru>
In-Reply-To: <20230314120252.48263-3-mmkurbanov@sberdevices.ru>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hello Andy. Thank you for review.
I have fixed most of your comments. Please take a look below.

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.

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


-- 
Best Regards,
Kurbanov Martin