| Message ID | 20180610142909.278028c5@archlinux (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jonathan, I'll send a v3 as a separate patch, here are some answers to your comments. > I'm not totally clear what this is. From a really quick look at the > docs, I think we are looking at in phase and quadrature elements > of the amplitude. As the amplitude is basically a light intensity > measurement That's my understanding too, I'll rephrase. > > in_intensity0_q_raw > in_intensity0_i_raw > in_intensity0_scale if it is possible to relate this anything real > I think in reality it does have a unit, we just have no visibility > of what that is. Ok. > > We also have a phase measurement, but naturally only one of htem > in_phase0_raw > in_phase0_scale Does that mean adding a new iio_chan_type "IIO_PHASE"? > Hmm. Thinking more on this, we could treat this as a separate channel. > Given it's light intensity it would be an intensity channel. > It's in some sense the combination of the split quadrature elements > above > in_intensity0_raw > in_intensity0_scale. Note for these that the scale is assumed by > most userspace code to be fixed, so you'll need to roll the exponent > part into the _raw value not the _scale.# Seems fine! > in_proximity0_calib_cs_i -> in_intensity0_i_calibbias > in_proximity0_calib_cs_q -> in_intensity0_q_calibbias > in_proximity0_calib_cs_gain =-> in_intensity0_calibscale (I think the cs will effect intensity?) Ok. > + The gain read from in_proximity0_hardwaregain shall > + be written into in_proximity0_calib_cs_gain when > + calibrating crosstalk. > > I'm not following this one, hardwaregain is usually a write > parameter. So should be under control of the userspace. The sensor has an "Automatic Gain Control" (AGC) which sets the analog signal levels at an optimum level by controlling programmable gain amplifiers according to the datasheet. The AGC value in an output of the sensor at it is supposed to be saved when calibrating crosstalk in "Crosstalk Gain" registers. Would it be ok to use hardwaregain as a read-only register for this purpose? > > + > + As the crosstalk is dependant on the emitter current, > + the amplitude read from in_proximity0_amplitude shall > + be written into in_proximity0_calib_ampl_ref when > + calibrating crosstalk. > > This last one is trickier from the description. Do we know how it > is applied by the hardware? Is it a simple offset? > > Looking at the document an1724.pdf this doesn't seem to be something > that it necessarily makes any sense to expose to userspace. It is > a calibration that has no external inputs - just a sequence of > internal actions. Hence I would just do this on power up. No I don't know how it is applied. However, it can't be done on power up as it requires the emitter to be blocked from reaching the photodiode. I guess the principle here is to measure the amplitude of the received signal while the emitter is blocked so that it can be substracted to the further measures. Would it be ok to use in_intensity0_calibbias for it? > +What: /sys/bus/iio/devices/iio:deviceX/in_proximity0_calib_distance_ref > +What: /sys/bus/iio/devices/iio:deviceX/in_proximity0_calib_temp_ref > > Hmm. So these last two are C in the equation. Seems so (the sensor is computing the C from those two). > + Finally, the c constant is set by the sensor in > + function of in_proximity0_calib_temp_ref and > + in_proximity0_calib_distance_ref values. > + > + To fill in_proximity0_calib_distance_ref, a distance > + measurement at a known distance has to be made. The > + result of the substraction between the known distance > + and the measured value shall be stored in > + in_proximity0_calib_distance_ref. The sensor > + temperature at the time of this measurement read shall > + be stored in in_proximity0_calib_temp_ref. > + > + Get those values from hardware and show them when read > + from. > > I'm slightly less bothered by getting these perfect as they are very > chip specific and generic userspace code is unlikely to play with them. > We may want to revisit these in the future... > > If we are using phase and intensity channels as suggested above, > these will want adjusting to take that into account. Those calib fields would become: in_proximity0_calib_temp_ref -> in_temp0_calibbias in_proximity0_calib_distance_ref -> in_proximity0_calibbias It would also require a new channel "in_intensity1_raw" for the ambient light output. Is it ok? > Treat the emitter as an output current channel and all this becomes > simpler. Ok. Thanks for your patience, Mathieu -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi,
I have another concern about I and Q representation. The formula given
by Renesas to put them under decimal form is:
(MSB << 8 + LSB) << EXP/100000
Given that EXP is an unsigned 8 bit integer, the maximum value of I and
Q is ~2^255 which can only be represented as a double.
Hence, it is not possible to represent them under in_intensity0_i/q_raw
even using a scale.
Same thing for I and Q calibbias (crosstalk values), the user can not
input such a big number.
How could we proceed about it, would it be ok to represent those values
as EXP << 16 + MSB << 8 + LSB both for raw and calibbias values?
Thanks,
Mathieu
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 11 Jun 2018 16:57:31 +0200 Mathieu Othacehe <m.othacehe@gmail.com> wrote: > Hi Jonathan, > > I'll send a v3 as a separate patch, here are some answers to your > comments. > > > I'm not totally clear what this is. From a really quick look at the > > docs, I think we are looking at in phase and quadrature elements > > of the amplitude. As the amplitude is basically a light intensity > > measurement > > That's my understanding too, I'll rephrase. > > > > > in_intensity0_q_raw > > in_intensity0_i_raw > > in_intensity0_scale if it is possible to relate this anything real > > I think in reality it does have a unit, we just have no visibility > > of what that is. > > Ok. > > > > > We also have a phase measurement, but naturally only one of htem > > in_phase0_raw > > in_phase0_scale > > Does that mean adding a new iio_chan_type "IIO_PHASE"? Yes, it rather looks like we need it. > > > Hmm. Thinking more on this, we could treat this as a separate channel. > > Given it's light intensity it would be an intensity channel. > > It's in some sense the combination of the split quadrature elements > > above > > in_intensity0_raw > > in_intensity0_scale. Note for these that the scale is assumed by > > most userspace code to be fixed, so you'll need to roll the exponent > > part into the _raw value not the _scale.# > > Seems fine! > > > in_proximity0_calib_cs_i -> in_intensity0_i_calibbias > > in_proximity0_calib_cs_q -> in_intensity0_q_calibbias > > in_proximity0_calib_cs_gain =-> in_intensity0_calibscale (I think the cs will effect intensity?) > > Ok. > > > + The gain read from in_proximity0_hardwaregain shall > > + be written into in_proximity0_calib_cs_gain when > > + calibrating crosstalk. > > > > I'm not following this one, hardwaregain is usually a write > > parameter. So should be under control of the userspace. > > The sensor has an "Automatic Gain Control" (AGC) which sets the analog > signal levels at an optimum level by controlling programmable gain > amplifiers according to the datasheet. > > The AGC value in an output of the sensor at it is supposed to be > saved when calibrating crosstalk in "Crosstalk Gain" registers. > > Would it be ok to use hardwaregain as a read-only register for this > purpose? I'm not really keen on doing that (as hardware gain has a well defined different meaning). This is a rather opaque device specific value. > > > > > + > > + As the crosstalk is dependant on the emitter current, > > + the amplitude read from in_proximity0_amplitude shall > > + be written into in_proximity0_calib_ampl_ref when > > + calibrating crosstalk. > > > > This last one is trickier from the description. Do we know how it > > is applied by the hardware? Is it a simple offset? > > > > Looking at the document an1724.pdf this doesn't seem to be something > > that it necessarily makes any sense to expose to userspace. It is > > a calibration that has no external inputs - just a sequence of > > internal actions. Hence I would just do this on power up. > > No I don't know how it is applied. However, it can't be done on power up > as it requires the emitter to be blocked from reaching the photodiode. This is interesting as it's specifically documented as requiring no external actions. Oh well, another clear datasheet. > > I guess the principle here is to measure the amplitude of the received > signal while the emitter is blocked so that it can be substracted to the > further measures. Would it be ok to use in_intensity0_calibbias for it? For the control parameter yes if it meets the ABI description, for the value during calibration no. Calibbias is a control parameter not an output. > > > +What: /sys/bus/iio/devices/iio:deviceX/in_proximity0_calib_distance_ref > > +What: /sys/bus/iio/devices/iio:deviceX/in_proximity0_calib_temp_ref > > > > Hmm. So these last two are C in the equation. > > Seems so (the sensor is computing the C from those two). > > > + Finally, the c constant is set by the sensor in > > + function of in_proximity0_calib_temp_ref and > > + in_proximity0_calib_distance_ref values. > > + > > + To fill in_proximity0_calib_distance_ref, a distance > > + measurement at a known distance has to be made. The > > + result of the substraction between the known distance > > + and the measured value shall be stored in > > + in_proximity0_calib_distance_ref. The sensor > > + temperature at the time of this measurement read shall > > + be stored in in_proximity0_calib_temp_ref. > > + > > + Get those values from hardware and show them when read > > + from. > > > > I'm slightly less bothered by getting these perfect as they are very > > chip specific and generic userspace code is unlikely to play with them. > > We may want to revisit these in the future... > > > > If we are using phase and intensity channels as suggested above, > > these will want adjusting to take that into account. > > > Those calib fields would become: > > in_proximity0_calib_temp_ref -> in_temp0_calibbias > in_proximity0_calib_distance_ref -> in_proximity0_calibbias These are kind of fine, as they are the linear offsets. Doesn't really match well with the quadratic term though. > > It would also require a new channel "in_intensity1_raw" for the ambient > light output. Is it ok? That one is fine. > > > Treat the emitter as an output current channel and all this becomes > > simpler. > > Ok. > > Thanks for your patience, You are welcome, this is a fiddly device! Sane hardware would store all these in on chip flash, but I guess it's a cost thing to not do so. > > Mathieu Jonathan -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 15 Jun 2018 14:34:08 +0200 Mathieu Othacehe <m.othacehe@gmail.com> wrote: > Hi, > > I have another concern about I and Q representation. The formula given > by Renesas to put them under decimal form is: > > (MSB << 8 + LSB) << EXP/100000 > > Given that EXP is an unsigned 8 bit integer, the maximum value of I and > Q is ~2^255 which can only be represented as a double. > > Hence, it is not possible to represent them under in_intensity0_i/q_raw > even using a scale. > > Same thing for I and Q calibbias (crosstalk values), the user can not > input such a big number. > > How could we proceed about it, would it be ok to represent those values > as EXP << 16 + MSB << 8 + LSB both for raw and calibbias values? No (unless I'm missunderstanding). That will be totally impossible to interpret in userspace. Hmm. This is a pain and frankly silly as there is no way the device is even vaguely accurate over that sort of range. Anyhow, so what can we do as these have to be combined into one value to have a consistent interface? Can we map this sanely to an ieee 64 bit floating point standard? A bit fiddly given the totally odd way it comes from the device. So the 100000 bit we can push off to a _scale attribute leaving us with just (16 bit value) << (8 bit value). Now standard floating point would need 1.value << (8 bit value) so we'll need to search for the first set bit and adjust the exponent as appropriate - hence the annoying need to put this in a double. We'd still need an in kernel print function for a double but we can lift that form an appropriately licensed c library - keep it in the driver for now. https://en.wikipedia.org/wiki/Double-precision_floating-point_format. So in rough form... fls('mantissa') will get us the most significant set bit. Shift the mantissa so that falls off the top and add that shift to the exponent. Poke in the right places in a standard double. Now this is where it gets even uglier. IIO assumes 2 32 bit parts so we'll need to mash it into those in some reasonable (ish) fashion. The core IIO then needs to pretty print it. Hmm. This looks annoyingly like we may need to do some core rework to make val and val2 64 bit relatively soon but I'd rather we didn't stall this driver on that. I'd like to hear other peoples inputs on this before we go to far. Sensible to support floating point or not? Jonathan > > Thanks, > > Mathieu -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jonathan, > I'm not really keen on doing that (as hardware gain has a well defined > different meaning). This is a rather opaque device specific value. Ok, I'll keep it as an extended field then. > This is interesting as it's specifically documented as requiring no external > actions. Oh well, another clear datasheet. :) > You are welcome, this is a fiddly device! Sane hardware would store > all these in on chip flash, but I guess it's a cost thing to not do so. I sent a patch with the updated documentation and the driver itself. Note that I left the read/write of float values as TODO (returning -EINVAL when asked for). This way I hope we can keep reviewing this driver, will trying to find the best way to deal with those annoying floating numbers. Thanks, Mathieu (I cc'd Pierre-Moana who is working with me on this driver) -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jonathan, > https://en.wikipedia.org/wiki/Double-precision_floating-point_format. > > So in rough form... > > fls('mantissa') will get us the most significant set bit. > Shift the mantissa so that falls off the top and add that shift > to the exponent. > > Poke in the right places in a standard double. Yes, understood, this way we can format Intersil float representation (m*2^e) to IEEE754 (1.m*2^e). > Now this is where it gets even uglier. IIO assumes 2 32 bit > parts so we'll need to mash it into those in some reasonable > (ish) fashion. The core IIO then needs to pretty print it. An option would be to add IIO_VAL_DOUBLE format value that would print ieee754 double in a similar way as '%a' option of glibc' printf ([-]0xh.hhhhp). A "iio_double_to_int" function would also be needed to parse a user inputed number under '%a' representation into two 'int' passed to write_raw. > Hmm. This looks annoyingly like we may need to do some core > rework to make val and val2 64 bit relatively soon but I'd > rather we didn't stall this driver on that. Yes the splitting would be really ugly as the mantissa on 52 bit will have to be splitted in two parts, something like: val: sign | exponent | mantissa (20 most valuable bits) val2: mantissa (32 last bits) Or something as dirty as that ... All of this seems really hacky but would allow user to input big floating values (that won't fit into val.val2 format with val and val2 on 32 or even 64 bit format). What do you think ? Thanks, Mathieu -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 27 Jun 2018 15:43:38 +0200 Mathieu Othacehe <m.othacehe@gmail.com> wrote: > Hi Jonathan, > > > https://en.wikipedia.org/wiki/Double-precision_floating-point_format. > > > > So in rough form... > > > > fls('mantissa') will get us the most significant set bit. > > Shift the mantissa so that falls off the top and add that shift > > to the exponent. > > > > Poke in the right places in a standard double. > > Yes, understood, this way we can format Intersil float representation > (m*2^e) to IEEE754 (1.m*2^e). > > > Now this is where it gets even uglier. IIO assumes 2 32 bit > > parts so we'll need to mash it into those in some reasonable > > (ish) fashion. The core IIO then needs to pretty print it. > > An option would be to add IIO_VAL_DOUBLE format value that would print > ieee754 double in a similar way as '%a' option of glibc' printf > ([-]0xh.hhhhp). It's not exactly human readable if we print it like that so not ideal. Hmm. It would be nice for reading it back into a program though. Perhaps put out a patch doing this as an RFC and we'll see what response we get! > > A "iio_double_to_int" function would also be needed to parse a user > inputed number under '%a' representation into two 'int' passed to > write_raw. yes - nasty but we'll need it. > > > Hmm. This looks annoyingly like we may need to do some core > > rework to make val and val2 64 bit relatively soon but I'd > > rather we didn't stall this driver on that. > > Yes the splitting would be really ugly as the mantissa on 52 bit will > have to be splitted in two parts, something like: > > val: sign | exponent | mantissa (20 most valuable bits) > val2: mantissa (32 last bits) > > Or something as dirty as that ... > > All of this seems really hacky but would allow user to input big > floating values (that won't fit into val.val2 format with val and val2 > on 32 or even 64 bit format). > > What do you think ? We can make it less hacky by adding a new callback function similar to we did for the quaternion case - read_raw_multi that at least takes 64 bit val and val2 so we can shove a double in one of them (without using floating point maths in kernel as that's a pain) Jonathan > > Thanks, > > Mathieu -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/ABI/testing/sysfs-bus-iio-isl29501 b/Documentation/ABI/testing/sysfs-bus-iio-isl29501 new file mode 100644 index 0000000..7d6ade8 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-bus-iio-isl29501 @@ -0,0 +1,120 @@ +What: /sys/bus/iio/devices/iio:deviceX/in_proximity0_phase_i +What: /sys/bus/iio/devices/iio:deviceX/in_proximity0_phase_q +KernelVersion: 4.17 +Contact: linux-iio@vger.kernel.org +Description: + The sensor uses analog quadrature signal processing + techniques to estimate the phase of the received + signal. The received signal is demodulated into + "in-phase" (I) and "quadrature" (Q) components. + + Get I and Q values as unit less integer between -32768 + and 32767 inclusive when read from. I'm not totally clear what this is. From a really quick look at the docs, I think we are looking at in phase and quadrature elements of the amplitude. As the amplitude is basically a light intensity measurement in_intensity0_q_raw in_intensity0_i_raw in_intensity0_scale if it is possible to relate this anything real I think in reality it does have a unit, we just have no visibility of what that is. We also have a phase measurement, but naturally only one of htem in_phase0_raw in_phase0_scale + +What: /sys/bus/iio/devices/iio:deviceX/in_proximity0_amplitude +KernelVersion: 4.17 +Contact: linux-iio@vger.kernel.org +Description: + Get the amplitude of the received signal in A between + 0 and 2.148A when read from. Hmm. Thinking more on this, we could treat this as a separate channel. Given it's light intensity it would be an intensity channel. It's in some sense the combination of the split quadrature elements above in_intensity0_raw in_intensity0_scale. Note for these that the scale is assumed by most userspace code to be fixed, so you'll need to roll the exponent part into the _raw value not the _scale.# + +What: /sys/bus/iio/devices/iio:deviceX/in_proximity0_calib_cs_i +What: /sys/bus/iio/devices/iio:deviceX/in_proximity0_calib_cs_q +What: /sys/bus/iio/devices/iio:deviceX/in_proximity0_calib_cs_gain +What: /sys/bus/iio/devices/iio:deviceX/in_proximity0_calib_ampl_ref +KernelVersion: 4.17 +Contact: linux-iio@vger.kernel.org +Description: + Crosstalk calibration compensates for electrical + crosstalk observed by the photodiode. To measure + crosstalk, the emitter light must be blocked from + reaching the photodiode. + + The "in-phase" component signal value read from + in_proximity0_phase_i shall be written into + in_proximity0_calib_cs_i when calibrating crosstalk. So reading this I'm thinking these are actually just applied as offsets? Now I assume the device applies them internally which in terms of IIO makes them calibbias attributes applied to the relevant channels. in_proximity0_calib_cs_i -> in_intensity0_i_calibbias in_proximity0_calib_cs_q -> in_intensity0_q_calibbias in_proximity0_calib_cs_gain =-> in_intensity0_calibscale (I think the cs will effect intensity?) + + In a similar way, the "quadrature" component signal + value read from in_proximity0_phase_q shall be written + into in_proximity0_calib_cs_q when calibrating + crosstalk. + + The gain read from in_proximity0_hardwaregain shall + be written into in_proximity0_calib_cs_gain when + calibrating crosstalk. I'm not following this one, hardwaregain is usually a write parameter. So should be under control of the userspace. + + As the crosstalk is dependant on the emitter current, + the amplitude read from in_proximity0_amplitude shall + be written into in_proximity0_calib_ampl_ref when + calibrating crosstalk. This last one is trickier from the description. Do we know how it is applied by the hardware? Is it a simple offset? Looking at the document an1724.pdf this doesn't seem to be something that it necessarily makes any sense to expose to userspace. It is a calibration that has no external inputs - just a sequence of internal actions. Hence I would just do this on power up. + + Get those values from hardware and show them when read + from. + +What: /sys/bus/iio/devices/iio:deviceX/in_proximity0_calib_phase_temp_a +What: /sys/bus/iio/devices/iio:deviceX/in_proximity0_calib_phase_temp_b +What: /sys/bus/iio/devices/iio:deviceX/in_proximity0_calib_phase_amb_a +What: /sys/bus/iio/devices/iio:deviceX/in_proximity0_calib_phase_amb_b +What: /sys/bus/iio/devices/iio:deviceX/in_proximity0_calib_distance_ref +What: /sys/bus/iio/devices/iio:deviceX/in_proximity0_calib_temp_ref Hmm. So these last two are C in the equation. +KernelVersion: 4.17 +Contact: linux-iio@vger.kernel.org +Description: + The sensor is able to perform correction of distance + measurements due to changing temperature and ambient