Re: [PATCH v4 13/13] video: backlight: mt6370: Add Mediatek MT6370 support

[ChiaEn Wu]

Andy Shevchenko <andy.shevchenko@xxxxxxxxx> 於 2022年7月13日 週三 晚上8:07寫道：
>
> On Wed, Jul 13, 2022 at 12:53 PM ChiaEn Wu <peterwu.pub@xxxxxxxxx> wrote:
> > Andy Shevchenko <andy.shevchenko@xxxxxxxxx> 於 2022年7月5日 週二 清晨5:14寫道：
> > > On Mon, Jul 4, 2022 at 7:43 AM ChiaEn Wu <peterwu.pub@xxxxxxxxx> wrote:
>
> Please, remove unneeded context when replying!
>
> ...
>
> > > > +               brightness_val[0] = (brightness - 1) & MT6370_BL_DIM2_MASK;
> > > > +               brightness_val[1] = (brightness - 1)
> > > > +                                   >> fls(MT6370_BL_DIM2_MASK);
> > >
> > > Bad indentation. One line?
> >
> > Well... if indent to one line, it will be over 80 characters(or called columns?)
> > From my understanding, it is not allowed, right??
>
> It's allowed to some extent.Use your common sense.
> Here it's obviously broken indentation.
>
> ...
>
> > > > +               prop_val = (ilog2(roundup_pow_of_two(prop_val)) + 1) >> 1;
> > >
> > > Isn't something closer to get_order() or fls()?
> >
> > I will revise it to "(get_order(prop_va * PAGE_SIZE) + 1) / 2" and
> > this change is meet your expectations??
>
> Nope. Try again. What about fls()?

I have tried two methods so far, as follows
-------------------------------------------------------------
/*
 * prop_val =  1      -->  1 steps --> b'00
 * prop_val =  2 ~  4 -->  4 steps --> b'01
 * prop_val =  5 ~ 16 --> 16 steps --> b'10
 * prop_val = 17 ~ 64 --> 64 steps --> b'11
*/

// 1. use fls() and ffs() combination
prop_val = ffs(prop_val) == fls(prop_val) ? fls(prop_val) >> 1 :
(fls(prop_val) + 1) >> 1;

// 2. use one line for-loop, but without fls()
for (i = --prop_val, prop_val = 0; i >> 2 * prop_val != 0; prop_val++);
-------------------------------------------------------------
Do these changes meet your expectations??

>
> ...
>
> > > > +       props->max_brightness = min_t(u32, brightness,
> > > > +                                     MT6370_BL_MAX_BRIGHTNESS);
> > >
> > > One line?
> >
> >  Ditto, it will be over 80 characters...
>
> As per above.
>
> --
> With Best Regards,
> Andy Shevchenko