...
>
> >> +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
