diff mbox

isl29501 and multiple calibration registers

Message ID 20180610142909.278028c5@archlinux
State New, archived
Headers show

Commit Message

Jonathan Cameron June 10, 2018, 1:29 p.m. UTC
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

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

Comments

Mathieu Othacehe June 11, 2018, 2:57 p.m. UTC | #1
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
Mathieu Othacehe June 15, 2018, 12:34 p.m. UTC | #2
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
Jonathan Cameron June 16, 2018, 5:13 p.m. UTC | #3
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
Jonathan Cameron June 16, 2018, 5:46 p.m. UTC | #4
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
Mathieu Othacehe June 19, 2018, 10:24 a.m. UTC | #5
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
Mathieu Othacehe June 27, 2018, 1:43 p.m. UTC | #6
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
Jonathan Cameron June 30, 2018, 5:55 p.m. UTC | #7
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 mbox

Patch

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