On Wed, 30 Mar 2022 01:33:19 +0530
Shreeya Patel <shreeya.patel@xxxxxxxxxxxxx> wrote:
> On 27/03/22 20:00, Jonathan Cameron wrote:
>
> Hi Jonathan,
>
> Thanks for your detailed review. I am working on v2 with the modifications
> suggested by you.
>
> Just one comment inline.
>
...
> >> +static int ltrf216a_set_it_time(struct ltrf216a_data *data, int itime)
> >> +{
> >> + int i, ret, index = -1;
> >> + u8 reg;
> >> +
> >> + for (i = 0; i < ARRAY_SIZE(int_time_mapping); i++) {
> >> + if (int_time_mapping[i] == itime) {
> >> + index = i;
> >> + break;
> >> + }
> >> + }
> >> + /* Make sure integration time index is valid */
> >> + if (index < 0)
> >> + return -EINVAL;
> >> +
> >> + if (index == 0) {
> > Switch statement seems more appropriate than this stack of if else
> >
> >> + reg = 0x03;
> > reg isn't a great name as I assume this is the value, not the address
> > which was my first thought... Perhaps reg_val?
> >> + data->int_time_fac = 4;
> >> + } else if (index == 1) {
> >> + reg = 0x13;
> >> + data->int_time_fac = 2;
> >> + } else {
> >> + reg = (index << 4) | 0x02;
> > Unless I'm missing something index == 2 if we get here.
> > So why the calculation? I'd suggest defining the two fields and using
> > FIELD_PREP() to set up each part probably to one of a set of
> > #define LTRF216A_ALS_MEAS_RATE_
>
> I think the calculation here is to set the default value when the
> integration time = 1.
1 isn't a possible value in int_time_available.
I guess you mean 100ms in which case if this were a switch statement
switch (index) {
case 0: /* 400msec */
reg = 0x03;
data->int_time_fac = 4;
break;
case 1: /* 200msec */
reg = 0x13;
data->int_time_fac = 2;
break;
case 2: /* 100sec */
reg = 0x22;
data->int_time_fac = 1;
break;
}
btw from datasheet, 50ms and 25ms also seem possible, why not support them?
Note the switch might be better handled as a constant look up table of appropriate
structures.
> In this case, reg value will be 34 (0x22) which
> is the default value of ALS_MEAS_RATE register.
>
> I will still confirm it once from Zhigang before sending a v2.
>
> >> + data->int_time_fac = 1;
> >> + }
> >> +
>
Jonathan
