linux-kernel - Re: [PATCH 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: <20241201132054.0c063a11@jic23-huawei>
Date: Sun, 1 Dec 2024 13:20:54 +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 2/2] iio: light: Add APDS9160 ALS & Proximity sensor
 driver


> > > +};
> > > +MODULE_DEVICE_TABLE(of, apds9160_of_match);
> > > +
> > > +static struct i2c_driver apds9160_driver = {
> > > +	.driver	  = {
> > > +		.name	= APDS9160_DRIVER_NAME,
> > > +		.owner = THIS_MODULE,
> > > +		.of_match_table = apds9160_of_match,
> > > +	},
> > > +	.probe    = apds9160_probe,
> > > +	.remove	  = apds9160_remove,
> > > +	.id_table = apds9160_id,
> > > +};  
> 
> Hi Jonathan,
> 

Hi Mikael,

One quick process thing first.  Please crop replies. It is far too easy to miss comments
inline (I don't think there were any?) and reviewers aren't fond of scrolling through lots
of context that isn't relevant to a particular discussion.

> Thank you for the feedback. I'm currently in the process of integrating the comments from all the reviewers for a rev 2.
> However, there's still some things that are not clear for me that I'm not sure on how to handle properly.
> 
> First, regarding the integration time/gain/scale parameters. I took a look at the datasheet again as there is a table
> provided to get lux/count (scale?) for the ALS sensor depending on gain and integration time. 
> 
> It looks like the correlation in the table is almost linear but it's not as there is a loss of precision.
> For example, at 1x gain with integration time 100ms the lux/count is 0.819 but at 3x the table is stating 0.269 instead of exepected 0.273.
> 
> Is it still possible to use the gts helpers in that case?

Ah. Probably not if it goes non linear.  Matti? (+CC)

> 
> Second, regarding the use of the IIO_CHAN_INFO_HARDWAREGAIN channel info.
> I took a look at a couple of recent drivers, some use the IIO_CHAN_INFO_SCALE to ajust gain type registers.
> 
> In my use case, it feels like the scale is read-only as it is affected by the gain and integration time and both can be set independently
> with their respective available values. How should I handle this?
The general preference is for the scale to be the primary control. 
For a light sensor assuming the device doesn't support very long integration times, the
trade off is normally set the integration time as high as possible (as that gives lowest
noise) then tune the gain as necessary.

Another model is to let the integration time be controllable and then try and adjust
the gain to keep as close as possible to a requested scale.  Matti has spent more
brain power on this than anyone so I'll over to him for more precise suggestions!

> 
> Finally, you mention to use a dma safe buffer when calling regmap_bulk_read. 
> I took a look at other recent drivers and I don't see any differences on how they are handling this. 
> Could you provide an example of how to ensure the buffer allocated on the stack is dma safe?
Gah. Before going on, I'd failed to notice this was an I2C device and I2C transports
don't need DMA safe buffers (whether or not accessed through regmap). So that comment
was spurious.  Not sure why I thought it was an SPI device :(

Anyhow for future reference:
The only way to do a safe buffer on the stack is to allocate a huge buffer and then
find an appropriate aligned padding.  We simply don't do that.  So reality is you can't use a buffer
on the stack for transfers that require DMA safe buffers.  Either kzalloc the buffer
or look at the many examples where the driver has __aligned(IIO_DMA_MINALIGN) data in iio_priv()
accessed structure.

The regmap case is a little less than clear though.  Last time I checked the reality was
that regmap always bounce buffered anyway as part of it's handling for weird register
formats.  It doesn't need to though and when I asked the maintainer a few years back the
response was that we should continue to use dma safe buffers for bulk transfers if
the underlying transport (e.g. SPI) requires them.

Jonathan



> 
> Best regards,
> Mikael