Re: [PATCH 3/3] iio: light: Add support for ltrf216a sensor

[Jonathan Cameron]

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