On Tue, Mar 14, 2023 at 2:12 PM Martin Kurbanov
<mmkurbanov@xxxxxxxxxxxxxx> 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
