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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220402174903.035f1181@jic23-huawei>
Date:   Sat, 2 Apr 2022 17:49:03 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Shreeya Patel <shreeya.patel@...labora.com>
Cc:     lars@...afoo.de, robh+dt@...nel.org, Zhigang.Shi@...eon.com,
        krisman@...labora.com, linux-iio@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        kernel@...labora.com, alvaro.soliverez@...labora.com
Subject: Re: [PATCH 3/3] iio: light: Add support for ltrf216a sensor

On Wed, 30 Mar 2022 01:33:19 +0530
Shreeya Patel <shreeya.patel@...labora.com> 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