isl29501 and multiple calibration registers

isl29501 and multiple calibration registers

[Mathieu Othacehe]

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

Re: isl29501 and multiple calibration registers

[Jonathan Cameron]

On Fri, 25 May 2018 18:01:48 +0200
Mathieu Othacehe <m.othacehe@gmail.com> wrote:

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

First game is to try and match up with existing ABI defined for similar
parts.  If that fails, it's worth thinking about whether any of them are
likely to apply to a reasonably large number of parts in the near future.
There are always chip specific values though so somethings just need
a custom interface.   The key thing to remember is that they will effectively
not exist for generic userspace code.

Anyhow, you can either implement them as sysfs attributes, or take the
preferred route of iio_chan_spec_ext_info as that allows in kernel users
to access them rather than just userspace.

The challenge is really to work out how to fit these device specific
variables into the overall 'framework' of the ABI that is already defined.
The first step is often ABI definition document for
Documentation/ABI/testing/sysfs-bus-iio-isl29501 as it can be easier
to talk about an ABI doc than work it out from the code.

Thanks,

Jonathan
> 
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: isl29501 and multiple calibration registers

[Mathieu Othacehe]

[-- Attachment #1: Type: text/plain, Size: 254 bytes --]


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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-iio-isl29501-Add-documentation.patch --]
[-- Type: text/x-diff, Size: 8403 bytes --]

>From 6932a29f986a432ec591417b34e896351be6ce14 Mon Sep 17 00:00:00 2001
From: Mathieu Othacehe <m.othacehe@gmail.com>
Date: Mon, 28 May 2018 17:35:32 +0200
Subject: [PATCH] iio: isl29501: Add documentation.

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

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-isl29501 b/Documentation/ABI/testing/sysfs-bus-iio-isl29501
new file mode 100644
index 0000000..6f18f13
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-isl29501
@@ -0,0 +1,230 @@
+What:		/sys/bus/iio/devices/iio:deviceX/in_proximity0_raw
+KernelVersion:	4.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+                Get the distance from the obstacle in meters. The
+                distance is an integer between 0 and 65535.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_proximity0_noise_approximation
+KernelVersion:	4.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+                Get the measurement noise approximation in meters.
+		The approximation is an integer between 0 and 65535.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_magn0_raw
+KernelVersion:	4.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+                Get the magnitude of the received signal in nA.
+		The magnitude is an integer between 0 and 65535.
+
+What:		/sys/bus/iio/devices/iio:deviceX/integration_time
+KernelVersion:	4.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+                Control the length of each sample, which is equal to
+		the time during which the optical pulse is active.
+
+What:		/sys/bus/iio/devices/iio:deviceX/integration_time_available
+KernelVersion:	4.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+                Reading returns the list of possible integration times.
+
+What:		/sys/bus/iio/devices/iio:deviceX/sampling_frequency
+KernelVersion:	4.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+                Control the sampling frequency of the sensor.
+
+What:		/sys/bus/iio/devices/iio:deviceX/sampling_frequency_available
+KernelVersion:	4.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+                Reading returns the list of possible sampling frequencies.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_proximity0_calib_cs_i
+KernelVersion:	4.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+                Set the Crosstalk I 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.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_proximity0_calib_cs_i_exp
+KernelVersion:	4.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+                Set the Crosstalk I calibration exponent 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_calib_cs_q
+KernelVersion:	4.17
+Contact:	linux-iio@vger.kernel.org
+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.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_proximity0_calib_cs_q_exp
+KernelVersion:	4.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+                Set the Crosstalk Q calibration exponent 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_calib_cs_gain
+KernelVersion:	4.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+                Set the Crosstalk gain value when written to. The
+		value must be an integer between 0 and 65535
+		inclusive.
+
+		Get this value from hardware and show it when read
+		from.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_proximity0_calib_mag_ref
+KernelVersion:	4.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+                Set the magnitude reference when written to. The
+		value must be an integer between 0 and 65535
+		inclusive.
+
+		Get this value from hardware and show it when read
+		from.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_proximity0_calib_mag_exp
+KernelVersion:	4.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+                Set the magnitude reference exponent when written
+		to. The value must be an integer between 0 and 15
+		inclusive.
+
+		Get this value from hardware and show it when read
+		from.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_proximity0_phase_offset
+KernelVersion:	4.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+                Set the fixed distance offset calibration value when
+		written to. The value must be an integer
+		between 0 and 65535 inclusive.
+
+		Get this value from hardware and show it when read
+		from.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_proximity0_temp_ref
+KernelVersion:	4.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+                Set the temperature reference 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_exp
+KernelVersion:	4.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+                Set the phase correction exponent value when written
+		to. The value must be an integer between 0 and 15
+		inclusive.
+
+		Get this value from hardware and show it when read
+		from.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_proximity0_phase_temp_a
+KernelVersion:	4.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+                Set the phase temperature A 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_a
+KernelVersion:	4.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+                Set the phase ambient A 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_temp_b
+KernelVersion:	4.17
+Contact:	linux-iio@vger.kernel.org
+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:	linux-iio@vger.kernel.org
+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:	linux-iio@vger.kernel.org
+Description:
+                Set the current DAC scale 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_emitter_dac
+KernelVersion:	4.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+                Set the current DAC 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_emitter_offset
+KernelVersion:	4.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+                Set the emitter voltage measure offset 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.
\ No newline at end of file
-- 
2.7.4

Re: isl29501 and multiple calibration registers

[Jonathan Cameron]

On Mon, 28 May 2018 17:38:08 +0200
Mathieu Othacehe <m.othacehe@gmail.com> wrote:

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

Hi Mathieu,

It is always somewhat hard to work out how to fit new requirements into
the overall ABI of IIO.  In general a more detailed explanation is needed
(particularly as the datasheet is less than detailed on some of this).

Also don't repeat generic attribute documentation.

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.

Jonathan

> 
> 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
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-isl29501 b/Documentation/ABI/testing/sysfs-bus-iio-isl29501
> new file mode 100644
> index 0000000..6f18f13
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-isl29501
> @@ -0,0 +1,230 @@
> +What:		/sys/bus/iio/devices/iio:deviceX/in_proximity0_raw
> +KernelVersion:	4.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +                Get the distance from the obstacle in meters. The
> +                distance is an integer between 0 and 65535.

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

All we end up with is two different documents which can get out of sync
with each other causing confusion.


> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_proximity0_noise_approximation
> +KernelVersion:	4.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +                Get the measurement noise approximation in meters.
> +		The approximation is an integer between 0 and 65535.

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?

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_magn0_raw
> +KernelVersion:	4.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +                Get the magnitude of the received signal in nA.
> +		The magnitude is an integer between 0 and 65535.

No, that stomps over existing attributes and is very confusing.  If it is
the amplitude of the signal from which we are deriving proximity by time of
flight, then I think we have some precendence though I'm struggling to
find an example right now.

It needs to show that it is an attribute related to the proximity channel.

So, something like.

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.

If this is something we might want to read out via the buffered interfaces, you
probably need to treat it as an entirely different sensor channel from the
proximity one and we'll need to think about that a bit more.  There is some
related work going on to define mathematical functions applied to another signal
as part of the channel type (in the energy meter drivers).  It's stalled a bit,
but would be useful here as well.
 

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/integration_time
> +KernelVersion:	4.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +                Control the length of each sample, which is equal to
> +		the time during which the optical pulse is active.

This is a standard element, don't redefine.

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/integration_time_available
> +KernelVersion:	4.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +                Reading returns the list of possible integration times.
> +

Standard element of the ABI don't redefine.

> +What:		/sys/bus/iio/devices/iio:deviceX/sampling_frequency
> +KernelVersion:	4.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +                Control the sampling frequency of the sensor.
> +

standard.

> +What:		/sys/bus/iio/devices/iio:deviceX/sampling_frequency_available
> +KernelVersion:	4.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +                Reading returns the list of possible sampling frequencies.
> +

standard

> +What:		/sys/bus/iio/devices/iio:deviceX/in_proximity0_calib_cs_i
> +KernelVersion:	4.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +                Set the Crosstalk I calibration value when written
> +		to. The value must be an integer between -32768 and
> +		32767 inclusive.

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.

> +
> +		Get this value from hardware and show it when read
> +		from.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_proximity0_calib_cs_i_exp
> +KernelVersion:	4.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +                Set the Crosstalk I calibration exponent value when
> +		written to. The value must be an integer between
> +		0 and 255 inclusive.

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

> +
> +		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:	linux-iio@vger.kernel.org
> +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.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_proximity0_calib_cs_q_exp
> +KernelVersion:	4.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +                Set the Crosstalk Q calibration exponent 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.

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

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_proximity0_calib_cs_gain
> +KernelVersion:	4.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +                Set the Crosstalk gain value when written to. The
> +		value must be an integer between 0 and 65535
> +		inclusive.
> +
> +		Get this value from hardware and show it when read
> +		from.

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

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_proximity0_calib_mag_ref
> +KernelVersion:	4.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +                Set the magnitude reference when written to. The
> +		value must be an integer between 0 and 65535
> +		inclusive.
> +
> +		Get this value from hardware and show it when read
> +		from.

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

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_proximity0_calib_mag_exp
> +KernelVersion:	4.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +                Set the magnitude reference exponent when written
> +		to. The value must be an integer between 0 and 15
> +		inclusive.
> +
> +		Get this value from hardware and show it when read
> +		from.

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

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_proximity0_phase_offset
> +KernelVersion:	4.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +                Set the fixed distance offset calibration value when
> +		written to. The value must be an integer
> +		between 0 and 65535 inclusive.

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.

> +
> +		Get this value from hardware and show it when read
> +		from.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_proximity0_temp_ref
> +KernelVersion:	4.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +                Set the temperature reference value when written
> +		to. The value must be an integer between 0 and 255
> +		inclusive.

I'm going to assume this is a real temperature.  Please give it standard temperature
units.

> +
> +		Get this value from hardware and show it when read
> +		from.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_proximity0_phase_exp
> +KernelVersion:	4.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +                Set the phase correction exponent value when written
> +		to. The value must be an integer between 0 and 15
> +		inclusive.
> +
> +		Get this value from hardware and show it when read
> +		from.

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.


> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_proximity0_phase_temp_a
> +KernelVersion:	4.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +                Set the phase temperature A 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.
Please explain clearly here what this is.

ax^2 + bx + c etc

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_proximity0_phase_amb_a
> +KernelVersion:	4.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +                Set the phase ambient A correction coefficient value
> +		when written to. The value must be an integer between
> +		0 and 255 inclusive.

Make it clear we are talking about amibient light here.

> +
> +		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:	linux-iio@vger.kernel.org
> +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:	linux-iio@vger.kernel.org
> +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:	linux-iio@vger.kernel.org
> +Description:
> +                Set the current DAC scale 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_emitter_dac
> +KernelVersion:	4.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +                Set the current DAC value when written to. The value
> +		must be an integer between 0 and 255 inclusive.

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.


> +
> +		Get this value from hardware and show it when read
> +		from.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_proximity0_emitter_offset
> +KernelVersion:	4.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +                Set the emitter voltage measure offset 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.
> \ No newline at end of file

Add one to clean this up.

> -- 
> 2.7.4
> 

Re: isl29501 and multiple calibration registers

[Mathieu Othacehe]

[-- Attachment #1: Type: text/plain, Size: 3674 bytes --]


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.

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

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

Ok.

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

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

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

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

Done.

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

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

>> +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 renamed it into in_proximity0_calib_ampl_ref it is suppose to store
the in_proximity0_amplitude value read when calibrating crosstalk.

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

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

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.

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

> Please explain clearly here what this is.
>
> ax^2 + bx + c etc

Done.

> 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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-iio-isl29501-Add-documentation.patch --]
[-- Type: text/x-diff, Size: 5482 bytes --]

>From 576c5976cc4c664231001e858aea4f8758bd3978 Mon Sep 17 00:00:00 2001
From: Mathieu Othacehe <m.othacehe@gmail.com>
Date: Mon, 28 May 2018 17:35:32 +0200
Subject: [PATCH v2] iio: isl29501: Add documentation.

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 | 120 +++++++++++++++++++++++
 1 file changed, 120 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-isl29501

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.
+
+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.
+
+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.
+
+		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.
+
+		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.
+
+		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
+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
+		light conditions. It can be programmed to correct for
+		a second order error polynomial.
+
+		Phase data from in_proximity0_phase has to be
+		collected when temperature read from in_temp0_raw and
+		ambient light read from in_intensity0_raw are
+		modulated independently.
+
+		Then a least squares curve fit to a second order
+		polynomial has to be generated from the data. The
+		resultant curves have the form ax^2 + bx + c.
+
+		From those two curves, a and b coefficients shall be
+		stored in in_proximity0_calib_phase_temp_a and
+		in_proximity0_calib_phase_temp_b for temperature and
+		in in_proximity0_calib_phase_amb_a and
+		in_proximity0_calib_phase_amb_b for ambient light.
+
+		Those values must be integer between 0 and 8355840
+		inclusive.
+
+		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.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_proximity0_driver_range
+KernelVersion:	4.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+                Set the current DAC scale 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_emitter_offset
+KernelVersion:	4.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+                Set the emitter voltage measure offset 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.
-- 
2.7.4

Re: isl29501 and multiple calibration registers

[Jonathan Cameron]

On Tue, 05 Jun 2018 12:18:23 +0200
Mathieu Othacehe <m.othacehe@gmail.com> wrote:

> 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.
Good, lots there to look at.

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

Please send new versions as a fresh email thread with the
patch inline, not as an attachment.  For this version I've pasted the
patch inline below.

Sorry for the delay on this, busy week! Anyhow, this is probably still going
to take us a few more rounds to pin down well.

Some of this heads into the territory of frequency generators and wave
measurement so I've cc'd Lars who has had some involvement with those
devices.

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

The upshot is if we don't do this, the parameter is probably never going to
be used by userspace code.

> 
> > Please don't define elements that are already covered by standard ABI and are
> > docuemnted in the sysfs-bus-iio files or similar.  
> 
> Ok.
> 
> > 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.

Cool - let us know if you aren't getting anything as there may be someone
on the list with some additional contacts to dig out info!

> 
> > 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.
> 
> > 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.
> 
> > This sounds like we have two different attributes for the same actual number.
> > Can we combine them and deal with the exponent internally?  
> 
> Done.
> 
> > 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.
> 
> > 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.
> 
> >> +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 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
different elements.  So put amplitude in unabbreviated in this one.

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

So does this end up being applied as a simple offset? Can we use standard
ABI for it? _calibbias

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

Cool.

> 
> > Please explain clearly here what this is.
> >
> > ax^2 + bx + c etc  
> 
> Done.
> 
> > 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
From: Mathieu Othacehe <m.othacehe@gmail.com>
Date: Mon, 28 May 2018 17:35:32 +0200
Subject: [PATCH v2] iio: isl29501: Add documentation.

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 | 120 +++++++++++++++++++++++
 1 file changed, 120 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-isl29501

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
+		light conditions. It can be programmed to correct for
+		a second order error polynomial.
+
+		Phase data from in_proximity0_phase has to be
+		collected when temperature read from in_temp0_raw and
+		ambient light read from in_intensity0_raw are
+		modulated independently.
+
+		Then a least squares curve fit to a second order
+		polynomial has to be generated from the data. The
+		resultant curves have the form ax^2 + bx + c.
+
+		From those two curves, a and b coefficients shall be
+		stored in in_proximity0_calib_phase_temp_a and
+		in_proximity0_calib_phase_temp_b for temperature and
+		in in_proximity0_calib_phase_amb_a and
+		in_proximity0_calib_phase_amb_b for ambient light.
+
+		Those values must be integer between 0 and 8355840
+		inclusive.

I would imagine the are scaled? or the distance is going to be awfully
large!

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

+
+What:		/sys/bus/iio/devices/iio:deviceX/in_proximity0_driver_range
+KernelVersion:	4.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+                Set the current DAC scale 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.

I'm not sure what this one is...  Firstly range is used to mean
something else in IIO, this is simply amplitude of a square wave on
an output channel

out_altcurrent0_raw


+
+What:		/sys/bus/iio/devices/iio:deviceX/in_proximity0_emitter_offset
+KernelVersion:	4.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+                Set the emitter voltage measure offset when written
+		to. The value must be an integer between 0 and 255
+		inclusive.
I assume this is what goes in Emitter DAC?

If so it's a current offset and has well defined scale.

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

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

Re: isl29501 and multiple calibration registers

[Mathieu Othacehe]

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

Re: isl29501 and multiple calibration registers

[Mathieu Othacehe]

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

Mostly question of whether we should support floating point values from hardware (was Re: isl29501 and multiple calibration registers)

[Jonathan Cameron]

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

Re: Mostly question of whether we should support floating point values from hardware (was Re: isl29501 and multiple calibration registers)

[Mathieu Othacehe]

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

Re: Mostly question of whether we should support floating point values from hardware (was Re: isl29501 and multiple calibration registers)

[Jonathan Cameron]

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

Re: isl29501 and multiple calibration registers

[Jonathan Cameron]

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

Re: isl29501 and multiple calibration registers

[Mathieu Othacehe]

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)