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

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

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

What should I do?

Best regards,
Mikael