linux-kernel - Re: [PATCH v2 2/2] iio: light: Add APDS9160 ALS & Proximity sensor driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20241211205302.2ba32a4a@jic23-huawei>
Date: Wed, 11 Dec 2024 20:53:02 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Mikael Gonella-Bolduc <mgonellabolduc@...onoff.com>
Cc: Mikael Gonella-Bolduc via B4 Relay
 <devnull+mgonellabolduc.dimonoff.com@...nel.org>, Lars-Peter Clausen
 <lars@...afoo.de>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
 <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Nathan Chancellor
 <nathan@...nel.org>, Nick Desaulniers <ndesaulniers@...gle.com>, Bill
 Wendling <morbo@...gle.com>, Justin Stitt <justinstitt@...gle.com>, Mikael
 Gonella-Bolduc <m.gonella.bolduc@...il.com>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 llvm@...ts.linux.dev, Hugo Villeneuve <hvilleneuve@...onoff.com>, Matti
 Vaittinen <mazziesaccount@...il.com>
Subject: Re: [PATCH v2 2/2] iio: light: Add APDS9160 ALS & Proximity sensor
 driver

On Mon, 9 Dec 2024 16:49:12 -0500
Mikael Gonella-Bolduc <mgonellabolduc@...onoff.com> wrote:

> On Sun, Dec 08, 2024 at 12:20:38PM +0000, Jonathan Cameron wrote:
> > On Fri, 06 Dec 2024 11:09:57 -0500
> > Mikael Gonella-Bolduc via B4 Relay <devnull+mgonellabolduc.dimonoff.com@...nel.org> wrote:
> >   
> > > From: Mikael Gonella-Bolduc <mgonellabolduc@...onoff.com>
> > > 
> > > APDS9160 is a combination of ALS and proximity sensors.
> > > 
> > > This patch add supports for:
> > >     - Intensity clear data and illuminance data
> > >     - Proximity data
> > >     - Gain control, rate control
> > >     - Event thresholds
> > > 
> > > Signed-off-by: Mikael Gonella-Bolduc <mgonellabolduc@...onoff.com>  
> > 
> > Hi Mikael,
> > 
> > As the bots noted, the maintainers entry has the wrong vendor prefix,
> > or the binding does.
> > 
> > Also the issue with missing include of linux/bitfield.h
> > 
> > Unused gain table is less obvious. Not sure what intent was on that one.
> > 
> > Given the discussion with Matti about how to do the gain control, please add
> > some description here of the outcome.  The control scheme is not particularly
> > obvious and is the key bit we should be reviewing.  It feels like you've
> > applied the feedback on v1 to the light channel but it is equally applicable
> > to proximity channels when they are just measures of reflected light intensity.
> > 
> > Various other minor things inline.
> > 
> > Thanks,
> > 
> > 
> > Jonathan  
> 
> Hi Jonathan,
> 
> I will fix the warnings the bots noted and other inline comments for v3, sorry about that.
> Regarding gain control for ALS, I kept the non-linear table provided in the datasheet.
> The user can adjust the integration time and the available scales will update
> depending on the value.
> For example, at 100ms, you have possible scales of 0.819, 0.269, 0.131, etc. (lux/count).
> The hardware gain and other relevant registers gets adjusted by the driver depending on selected scale.
> The attribute is kept as read-only as Matti suggested.
> 
> Now, for proximity, again I'm confused. Please bear with me a little.
> The only "scale" I see in the datasheet is that the proximity sensor is for a short distance of under 70mm.

That sounds like a design point for sensitivity of sensor vs light source brightness.

> There's nothing provided in the datasheet to convert the proximity ADC count to a distance or to anything meaningful like standard units.
> I don't feel like this is really precise and the intended use case is probably like mine where you can use this to detect
> if there's something covering the sensor or not.
> 
> I took a look at other light/proximity sensors, again, it's not clear for me how to handle this.
> It seems that some drivers just directly control the hardware gain register with the scale even if it's not really a scale.
Typical case is that it is a scale, just not of distance.  But rather controls an amplifier on the light sensor,
so same as for the ambient light sensor.

The ABI docs are a little vague on this Documentation/ABI/testing/sysfs-bus-iio
has
What:		/sys/.../iio:deviceX/in_proximity_raw
What:		/sys/.../iio:deviceX/in_proximity_input
What:		/sys/.../iio:deviceX/in_proximityY_raw
KernelVersion:	3.4
Contact:	linux-iio@...r.kernel.org
Description:
		Proximity measurement indicating that some
		object is near the sensor, usually by observing
		reflectivity of infrared or ultrasound emitted.

		Often these sensors are unit less and as such conversion
		to SI units is not possible. Higher proximity measurements
		indicate closer objects, and vice versa. Units after
		application of scale and offset are meters.

So it kind of says we can't relate them to real units, but then we provide
a unit. Hmm, not our finest and clearest documentation.

Probably best bet is to follow precedence as even if we haven't tightly defined
it that is what any userspace tuning these value will be using.

Given inverse square law and different characteristics of reflective surfaces
I think it is normally a case of crank the gain up until the signal is good.

In most cases these proximity sensors aren't much more than fancy switches
though can be used for approaching vs moving away detection.

Anyhow, I haven't checked all the precedence in existing drivers but from
memory scale is the standard choice.

Hardware gain as a writable control is just rarely used and only in devices where
it doesn't affect what we are measuring. In proximity that means time of flight
sensors, not ones based on reflected intensity.

Jonathan

> 
> What should I do?
> 
> Best regards,
> Mikael