[PATCH v2 3/3] staging: iio: light: isl29018: Use standard sysfs attributes for scale and integration time

[Daniel Baluta]

On Sat, Mar 14, 2015 at 9:58 PM, Roberta Dobrescu
<roberta.dobrescu at gmail.com> wrote:
> Thanks for the review, Daniel. I'll fix the issues you noticed.
> I just noticed a problem when reading lux.
>
>
>>>   static int isl29018_read_lux(struct isl29018_chip *chip, int *lux)
>>>   {
>>> -       int lux_data;
>>> -       unsigned int data_x_range, lux_unshifted;
>>> +       int lux_data, i;
>>> +       unsigned int data_x_range;
>>> +       unsigned int adc_bits;
>>>
>>>          lux_data = isl29018_read_sensor_input(chip,
>>> COMMMAND1_OPMODE_ALS_ONCE);
>>>
>>> @@ -168,10 +207,23 @@ static int isl29018_read_lux(struct isl29018_chip
>>> *chip, int *lux)
>>>           * ucalibscale ranges from 0-999999, so about 20 bits.  Split
>>>           * the /1,000,000 in two to reduce the risk of over/underflow.
>>>           */
>>> -       data_x_range = lux_data * chip->range;
>>> -       lux_unshifted = data_x_range * chip->calibscale;
>>> -       lux_unshifted += data_x_range / 1000 * chip->ucalibscale / 1000;
>>> -       *lux = lux_unshifted >> chip->adc_bit;
>>> +
>>> +       for (i = 0; i < ARRAY_SIZE(isl29018_int_times[chip->type]); ++i)
>>> +               if (chip->int_time.time ==
>>> isl29018_int_times[chip->type][i].time &&
>>> +                   chip->int_time.utime ==
>>> isl29018_int_times[chip->type][i].utime) {
>>> +                       adc_bits = isl29018_adc_bits[i];
>>> +                       break;
>>> +               }
>>> +
>>> +       if (i >= ARRAY_SIZE(isl29018_int_times[chip->type]))
>>> +               return -EINVAL;
>>> +
>>> +       data_x_range = lux_data * chip->scale.scale;
>>> +       data_x_range += lux_data / 1000 * chip->scale.uscale / 1000;
>>> +       data_x_range = data_x_range << (16 - adc_bits);
>
>
> I'm not sure about this part above. It seems to be a problem if we have
> 8 or 4 adc bits, data_x_range becomes 0. And even with 16 or 12 bits
> there are accuracy issues.
>
> One of the problems is with this line:
>         data_x_range += lux_data / 1000 * chip->scale.uscale / 1000;
>
> If lux_data is let's say 228, data_x_range is automatically 0 after this
> operation. I can change this lux_data * scale / 1000000 to solve this,
> but I have to see if this won't lead to overflow for 16 or 12 bits.
>
> Even if with this change, accuracy is far from being acceptable.
>
> I did a small test for 8 bits resolution and range 1000. For a measured
> lux_data of 228, data_x_range is 890 with the old formula (lux_data * range
>>> adc_bits). When calculating it with the new formula with scale
> this is only 768.
>
> I don't think this is ok at all. An option would be to have scale
> exposed to userspace, but when calculating illuminace to use the
> formula with range.
>
> What do you think about this?

I will think about it, and get back to you tomorrow.

We should try to understand the comment above:

>>> -       data_x_range = lux_data * chip->range;

It's also strange because they're sending to userspace an IIO_VAL_INT.

Daniel.