isl29501 and multiple calibration registers

Hello,

I'm currently working on a driver for the ISL29501 Time of Flight sensor[1] that I hope to publish soon. This sensor has many calibration and corrections registers (crosstalk_i, crosstalk_q, crosstalk_gain, phase_temp_correction ...).

I would like to make all those registers accessible from userspace but iio_chan_info_enum has no matching elements for most of those registers which are very specific to this sensor.

Any idea how could I proceed to make those registers accessible?

Thanks in advance,

Mathieu

[1]: https://www.intersil.com/content/dam/intersil/documents/isl2/isl29501.pdf
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

From 6932a29f986a432ec591417b34e896351be6ce14 Mon Sep 17 00:00:00 2001

Hi Jonathan,

Thanks for you answer, it seems fine to me to use IIO ABI when possible and extend it with iio_chan_spec_ext_info for exotic calibration registers.

Here is a proposal for sysfs-bus-iio-isl29501 document. Would it be ok?

Thanks,

Mathieu

The ISL29501 is a Time of Flight (ToF) based signal processing integrated circuit. The sensor enables long range optical distance sensing when combined with an external emitter and detector.

---
 Documentation/ABI/testing/sysfs-bus-iio-isl29501 | 230 +++++++++++++++++++++++
 1 file changed, 230 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-isl29501

Documentation/ABI/testing/sysfs-bus-iio-isl29501 new file +0 -0

new file mode 100644
index 0000000..6f18f13

Documentation/ABI/testing/sysfs-bus-iio-isl29501 new file +15 -0

+
+		Get this value from hardware and show it when read
+		from.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_proximity0_calib_cs_q
+KernelVersion:	4.17
+Contact:	[email protected]
+Description:
+                Set the Crosstalk Q calibration value when written
+		to. The value must be an integer between -32768 and
+		32767 inclusive.
+
+		Get this value from hardware and show it when read
+		from.
+

Documentation/ABI/testing/sysfs-bus-iio-isl29501 new file +4 -0

+
+		Get this value from hardware and show it when read
+		from.
+

Documentation/ABI/testing/sysfs-bus-iio-isl29501 new file +32 -0

+
+		Get this value from hardware and show it when read
+		from.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_proximity0_phase_temp_b
+KernelVersion:	4.17
+Contact:	[email protected]
+Description:
+                Set the phase temperature B correction coefficient
+		value when written to. The value must be an integer
+		between 0 and 255 inclusive.
+
+		Get this value from hardware and show it when read
+		from.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_proximity0_phase_amb_b
+KernelVersion:	4.17
+Contact:	[email protected]
+Description:
+                Set the phase ambient B correction coefficient value
+		when written to. The value must be an integer between
+		0 and 255 inclusive.
+
+		Get this value from hardware and show it when read
+		from.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_proximity0_driver_range
+KernelVersion:	4.17
+Contact:	[email protected]
+Description:
+                Set the current DAC scale when written to. The value
+		must be an integer between 0 and 255 inclusive.

Documentation/ABI/testing/sysfs-bus-iio-isl29501 new file +5 -0

+
+		Get this value from hardware and show it when read
+		from.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_proximity0_emitter_offset

Signature · 1 line

2.7.4

The exponent / (presumably) mantissa split in some of these is a pain, as we really don't want to have userspace have to figure out how those are related. They really need to be 'one' value. The fun bit is that sometimes the exponent is shared so we will need to find the best value for all the controlled parameters.

Please don't define elements that are already covered by standard ABI and are docuemnted in the sysfs-bus-iio files or similar.

This needs more information. From that I have no idea what the noise approximation is? Standard deviation of the noise perhaps? Some other statistical measure?

in_proximity0_amplitude and it needs to be in standard units for current, even if that requires conversion in the driver from the internal representation.

What is an "I" value? What is it's units? All this stuff should be documented here. I'm guessing we are talking quadrature signals, but that's not clear here.

This sounds like we have two different attributes for the same actual number. Can we combine them and deal with the exponent internally?

Again, sounds like we ought to be able to figure out how to split the exponent and mantissa. Afterall any userspace calibration code is going to have to do this anyway so it can't be that hard :)

These all need more explanation in the descriptions. What would userspace do with them? What effect do that have on the read signal?

Quoted 1 line

+What: /sys/bus/iio/devices/iio:deviceX/in_proximity0_calib_mag_ref

Hmm. What's this one? What is it a reference for?

I'm guessing this is the auto gain control, which has only basic documentation on the datasheet I'm looking at..

I think this is another one which can be combined with the 'value' to get the best possible representation of whatever this should be.

Quoted 1 line

+What: /sys/bus/iio/devices/iio:deviceX/in_proximity0_phase_offset

What do these numbers represent? Given the name I'd assume something to do with 1/65536 of a cycle but who knows...

If so we should have better units for this.

That is annoying. Shared exponent of various different values. Ideal is to have the driver figure out the 'best' option accuracy wise for whatever set of parameters we currently have.

Please explain clearly here what this is.

ax^2 + bx + c etc

Hmm. We have done this in two ways in the past, either as a control signal of the proximity or a separate output signal. I'm not immediately sure which makes sense here. Probably the current output channel option like in the si1145 driver.

From 576c5976cc4c664231001e858aea4f8758bd3978 Mon Sep 17 00:00:00 2001

Hi Jonathan,

Thanks for you review. I was able to found more informations about the chip at this address:
https://www.intersil.com/en/products/optoelectronics/proximity-sensors/light-to-digital-sensors/ISL29501.html#document.

Please find a v2 attached and my answer to your comments below.

Quoted 6 lines

The exponent / (presumably) mantissa split in some of these is a pain, as we really don't want to have userspace have to figure out how those are related. They really need to be 'one' value. The fun bit is that sometimes the exponent is shared so we will need to find the best value for all the controlled parameters.

Yes it will certainly be painful to implement.

Quoted 3 lines

Please don't define elements that are already covered by standard ABI and are docuemnted in the sysfs-bus-iio files or similar.

Ok.

Quoted 3 lines

This needs more information. From that I have no idea what the noise approximation is? Standard deviation of the noise perhaps? Some other statistical measure?

This is a point where I couldn't found any information. I'll ask Renesas for help about this one.

Quoted 3 lines

in_proximity0_amplitude and it needs to be in standard units for current, even if that requires conversion in the driver from the internal representation.

This one is only useful for calibration so in_proximity0_amplitude seems fine.

Quoted 3 lines

What is an "I" value? What is it's units? All this stuff should be documented here. I'm guessing we are talking quadrature signals, but that's not clear here.

Yes it is the in-phase part of the signal, I detailed this part in v2.

Quoted 2 lines

This sounds like we have two different attributes for the same actual number. Can we combine them and deal with the exponent internally?

Done.

Quoted 3 lines

Again, sounds like we ought to be able to figure out how to split the exponent and mantissa. Afterall any userspace calibration code is going to have to do this anyway so it can't be that hard :)

Done.

Quoted 2 lines

These all need more explanation in the descriptions. What would userspace do with them? What effect do that have on the read signal?

Detailed in v2.

Quoted 3 lines

Quoted 1 line

+What: /sys/bus/iio/devices/iio:deviceX/in_proximity0_calib_mag_ref

Hmm. What's this one? What is it a reference for?

Quoted 2 lines

I'm guessing this is the auto gain control, which has only basic documentation on the datasheet I'm looking at..

I renamed it into in_proximity0_calib_ampl_ref it is suppose to store the in_proximity0_amplitude value read when calibrating crosstalk.

From a consistency point of view, I wouldn't use different abbreviations for

Quoted 3 lines

I think this is another one which can be combined with the 'value' to get the best possible representation of whatever this should be.

Done.

Quoted 4 lines

Quoted 1 line

+What: /sys/bus/iio/devices/iio:deviceX/in_proximity0_phase_offset

What do these numbers represent? Given the name I'd assume something to do with 1/65536 of a cycle but who knows...

Quoted 1 line

If so we should have better units for this.

The naming of this field in the datasheet is really misleading. It is in fact the distance calibration. It stores the difference between a known distance and corresponding measured distance. I renamed it in_proximity0_calib_distance_ref in v2.

Quoted 4 lines

That is annoying. Shared exponent of various different values. Ideal is to have the driver figure out the 'best' option accuracy wise for whatever set of parameters we currently have.

Ok, I'll try to implement it in the driver.

Quoted 4 lines

Please explain clearly here what this is.

ax^2 + bx + c etc

Done.

Quoted 3 lines

Hmm. We have done this in two ways in the past, either as a control signal of the proximity or a separate output signal. I'm not immediately sure which makes sense here. Probably the current output channel option like in the si1145 driver.

Ok.

Thanks,

Mathieu

From 576c5976cc4c664231001e858aea4f8758bd3978 Mon Sep 17 00:00:00 2001

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

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

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?)

(quoted diff) +3 -0

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

(quoted diff) +5 -0

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

(quoted diff) +2 -0

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

(quoted diff) +14 -0

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

Treat the emitter as an output current channel and all this becomes simpler.

Hi Jonathan,

I'll send a v3 as a separate patch, here are some answers to your comments.

Quoted 4 lines

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.

Quoted 6 lines

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.

Quoted 4 lines

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"?

Quoted 9 lines

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!

Quoted 3 lines

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.

Quoted 6 lines

(quoted diff) +3 -0

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

Quoted 11 lines

(quoted diff) +5 -0

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

Quoted 4 lines

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?

Quoted 5 lines

(quoted diff) +2 -0

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

Quoted 19 lines

(quoted diff) +14 -0

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

Quoted 2 lines

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?

Quoted 3 lines

Treat the emitter as an output current channel and all this becomes simpler.

Ok.

Thanks for your patience,

Mathieu

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

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.

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.

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.

Hi Jonathan,

Quoted 9 lines

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

Quoted 3 lines

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.

Quoted 4 lines

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