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

[Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com>]

On Wed, Dec 11, 2024 at 08:53:02PM +0000, Jonathan Cameron wrote:
> On Mon, 9 Dec 2024 16:49:12 -0500
> Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.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@kernel.org> wrote:
> > >   
> > > > From: Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.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@dimonoff.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@vger.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
> 

Hi Jonathan,

Thank you for the clarifications.
Please see v3.

Best regards,
Mikael