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