Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 2 Apr 2022 17:49:03 +0100
From:   Jonathan Cameron <jic23@kernel.org>
To:     Shreeya Patel <shreeya.patel@collabora.com>
Cc:     lars@metafoo.de, robh+dt@kernel.org, Zhigang.Shi@liteon.com,
        krisman@collabora.com, linux-iio@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        kernel@collabora.com, alvaro.soliverez@collabora.com
Subject: Re: [PATCH 3/3] iio: light: Add support for ltrf216a sensor
Message-ID: <20220402174903.035f1181@jic23-huawei>
In-Reply-To: <ffcdf821-38f2-f8d7-2bcd-8ea51cad1b96@collabora.com>
References: <20220325103014.6597-1-shreeya.patel@collabora.com>
        <20220325103014.6597-4-shreeya.patel@collabora.com>
        <20220327153049.10e525e9@jic23-huawei>
        <ffcdf821-38f2-f8d7-2bcd-8ea51cad1b96@collabora.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

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