Re: [PATCH v2] iio: light: Add support for ROHM RPR0521 sensor

jic23
2015-06-11T14:18:49Z

Daniel Baluta writes:

> On Sun, Jun 7, 2015 at 12:08 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>>
>>
>> On 06/03/2015 09:56 AM, Daniel Baluta wrote:
>>>
>>> On Thu, May 28, 2015 at 5:17 PM, Daniel Baluta <daniel.baluta@intel.com>
>>> wrote:
>>>>
>>>> <snip>
>>>>
>>>>>>>> +static const struct iio_chan_spec rpr0521_channels[] = {
>>>>>>>> + {
>>>>>>>> + .type = IIO_INTENSITY,
>>>>>>>> + .modified = 1,
>>>>>>>> + .address = RPR0521_CHAN_ALS_DATA0,
>>>>>>>> + .channel2 = IIO_MOD_LIGHT_BOTH,
>>>>>>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>>>>>>> + BIT(IIO_CHAN_INFO_CALIBSCALE),
>>>>>>>
>>>>>>>
>>>>>>> why CALIBSCALE and not SCALE?
>>>>>>
>>>>>>
>>>>>> Because this is used to set/get gain, which is used by the hardware
>>>>>> to do proper scaling.
>>>>>>
>>>>>> AFAIK this should be calibscale.
>>>>>
>>>>>
>>>>> in sysfs-bus-iiof on CALIBSCALE: Hardware applied calibration scale
>>>>> factor
>>>>> (assumed to fix production inaccuracies).
>>>>>
>>>>> this doesn't seem applicable here, it is a gain factor controlling
>>>>> measurement resolution
>>>>
>>>>
>>>> Ok, I see now and it makes sense :).
>>>>
>>>> # echo 1 > in_intensity_ir_calibscale
>>>> # cat in_intensity_ir_raw
>>>> 79
>>>> # echo 64 > in_intensity_ir_calibscale
>>>> # cat in_intensity_ir_raw
>>>> 5084
>>>>
>>>> The user should get the same value regardless of the gain :), and in the
>>>> above example for x64 gain it should have a 1/64 scale.
>>>>
>>>>
>>>> <snip>
>>>>
>>>>>> Or we can consider that the chan->type is always valid?
>>>>>
>>>>>
>>>>> I'd think so; you also assume that chan->address is valid
>>>>>
>>>>> I suggest to use chan->address to point to a table containing the
>>>>> address and the mask
>>>>
>>>>
>>>> <snip>
>>>>
>>>>>> Which sensors? It means they do not agree with the ABI:
>>>>>>
>>>>>>
>>>>>> http://lxr.free-electrons.com/source/Documentation/ABI/testing/sysfs-bus-iio#L1131
>>>>>
>>>>>
>>>>> that 'clarification' was added recently,
>>>>> 614e8842ddf5502f0e781f91695bfbc1e1e1d9b6 (with 3.18)
>>>>> "Proximity measurement .. by observing reflectivity"
>>>>>
>>>>> high proximity <-> high reflectivity -- this is the reality of what most
>>>>> sensors output (including yours)
>>>>>
>>>>> proximity and distance are opposite concepts;
>>>>> high proximity <-> low distance, and vice versa
>>>>>
>>>>> the distance part doesn't make sense in the ABI description
>>>>
>>>>
>>>> At least sx9500 uses this convention and userspace applications rely on
>>>> this.
>>>
>>>
>>> OK, so wee need to agree on this part and to add a proper descriptor to
>>> the ABI.
>>>
>>> Jonathan, what do you say?
>>>
>> I agree that we need to agree one way or the other. Proximity being higher
>> as you get closer seems slightly more logical to me
>> (I wish now that I'd argued in favour of just doing distance, but such
>> is hindsight). Still I'm happy with whatever consensus forms.
>
> + Matt.
>
> Ok, now I see where the ambiguity comes from. The ABI for proximity also
> covers AS3935 Franklin Lightning Sensor IC, where sensor's output provides
> an estimation on the distance to the head of a storm :).
>
> But, I don't think this is the type of proximity most of people think of :).
>
> I am not sure how to modify the ABI without breaking the AS3935, but I will
> think of a solution and send a RFC as soon as possible.
>
> thanks,
> Daniel.
Hmm. We could define new ABI and deprecate the old one I suppose. Or
take the view that ABI breakage fixes are aloud as long as we have both
options in the tree currently and hence are defining one of them to be
wrong.