Received-SPF: pass (google.com: domain of linux-kernel+bounces-14776-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Tue, 2 Jan 2024 19:32:48 +0000
From: Jonathan Cameron <jic23@kernel.org>
To: Javier Carrasco <javier.carrasco.cruz@gmail.com>
Cc: Christian Eggers <ceggers@arri.de>, Lars-Peter Clausen
 <lars@metafoo.de>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski
 <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>,
 linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
 linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] io: light: as73211: add support for as7331
Message-ID: <20240102193248.25a6f62d@jic23-huawei>
In-Reply-To: <03f469ca-c5f4-4255-90f4-6715a1455e0d@gmail.com>
References: <20231220-as7331-v1-0-745b73c27703@gmail.com>
	<20231220-as7331-v1-2-745b73c27703@gmail.com>
	<20231226161414.47d5171e@jic23-huawei>
	<03f469ca-c5f4-4255-90f4-6715a1455e0d@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit


...

> 
> >> +static int as73211_intensity_scale(struct as73211_data *data, int chan, int *val, int *val2)
> >> +{
> >> +	unsigned int scale;
> >> +
> >> +	switch (chan) {
> >> +	case IIO_MOD_X:
> >> +		scale = AS73211_SCALE_X;
> >> +		break;
> >> +	case IIO_MOD_Y:
> >> +		scale = AS73211_SCALE_Y;
> >> +		break;
> >> +	case IIO_MOD_Z:
> >> +		scale = AS73211_SCALE_Z;
> >> +		break;
> >> +	default:
> >> +		return -EINVAL;
> >> +	}
> >> +	scale /= as73211_gain(data);
> >> +	scale /= as73211_integration_time_1024cyc(data);
> >> +	*val = scale;
> >> +
> >> +	return IIO_VAL_INT;  
> > 
> > Obviously it's really a question about the original code but why not
> > use IIO_VAL_FRACTIONAL here as well as below? Superficially looks
> > like it should work in a similar fashion.
> > 
> > If not, perhaps a comment here somewhere?
> >   
> You are right, the use of IIO_VAL_INT comes from the original
> implementation. I did not modify that because the expected precision
> (according to the datasheet is 3 decimal places) is guaranteed with the
> use of nW/m^2 instead of nW/cm^2 (the units used in the datasheet).
> 
> I think the best approach would have been using IIO_VAL_FRACTIONAL and
> the units provided in the datasheet, but changing units now could cause
> problems to current users. We could still use IIO_VAL_FRACTIONAL unless
> that might affect current users in any way. Otherwise I will add a
> comment as suggested.

It's possible we'd get slightly better precision from IIO_VAL_FRACTIONAL
as the string formatter does 64 bit maths and will print far too many
decimal places (matters for high precision ADCs where the rounding
errors are otherwise a problem).

I'd be surprised if anyone noticed as this is read only anyway.
So as far as I'm concerned switch to IIO_VAL_FRACTIONAL but keeping
the same units for this would be a good change. Perhaps doesn't belong
in this patch however.

> 
> >> @@ -355,30 +444,12 @@ static int as73211_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec cons
> >>  			*val2 = AS73211_SCALE_TEMP_MICRO;
> >>  			return IIO_VAL_INT_PLUS_MICRO;
> >>  
> >> -		case IIO_INTENSITY: {
> >> -			unsigned int scale;
> >> -
> >> -			switch (chan->channel2) {
> >> -			case IIO_MOD_X:
> >> -				scale = AS73211_SCALE_X;
> >> -				break;
> >> -			case IIO_MOD_Y:
> >> -				scale = AS73211_SCALE_Y;
> >> -				break;
> >> -			case IIO_MOD_Z:
> >> -				scale = AS73211_SCALE_Z;
> >> -				break;
> >> -			default:
> >> -				return -EINVAL;
> >> -			}
> >> -			scale /= as73211_gain(data);
> >> -			scale /= as73211_integration_time_1024cyc(data);
> >> -			*val = scale;
> >> -			return IIO_VAL_INT;
> >> +		case IIO_INTENSITY:
> >> +			return data->spec_dev->intensity_scale(data, chan->channel2, val, val2);  
> > Where it doesn't hurt readability, I'd prefer we stayed as close to 80 chars or below
> > as reasonably possible.  So here wrap so val, val2); is on the next line.
> >   
> In order to meet the 80-char rule, three lines will be required
> (wrapping val, val2 is not enough; chan->channel2 must have its own
> line). It looks a bit weird, but I have nothing against it.
> 
> On the other hand, the original code did not always follow the 80-char
> rule (up to 99 chars per line are used), so using two lines with a first
> one of 84 chars could be an option.

Up to you. I'd be fine with 84 chars.

Jonathan