Re: [PATCH 1/3] iio: light: Add driver for ap3216c

2019-02-20 Thread Sven Van Asbroeck
Again thanks for the feedback, Jonathan ! On Wed, Feb 20, 2019 at 7:32 AM Jonathan Cameron wrote: > > Looks to me like that happens (I haven't checked that thoroughly) via > kernfs_fops_write which takes a mutex - so we have a barrier. > Yes, if there's a mutex in the path somewhere (which

Re: [PATCH 1/3] iio: light: Add driver for ap3216c

2019-02-20 Thread Jonathan Cameron
On Mon, 18 Feb 2019 14:35:51 -0500 Sven Van Asbroeck wrote: > Hi Jonathan, > > Thanks again for your clear and extensive feedback ! > > On Mon, Feb 18, 2019 at 10:16 AM Jonathan Cameron > wrote: > > > > I suspect that would break lots of devices if it happened, but > > fair enough that

Re: [PATCH 1/3] iio: light: Add driver for ap3216c

2019-02-18 Thread Sven Van Asbroeck
Hi Jonathan, Thanks again for your clear and extensive feedback ! On Mon, Feb 18, 2019 at 10:16 AM Jonathan Cameron wrote: > > I suspect that would break lots of devices if it happened, but > fair enough that explicit might be good. One option would be > to document clearly in regmap the

Re: [PATCH 1/3] iio: light: Add driver for ap3216c

2019-02-18 Thread Sven Van Asbroeck
Hello Jonathan, First of all, thank you so much for taking the time for such a detailed reply ! On Mon, Feb 18, 2019 at 10:22 AM Jonathan Cameron wrote: > > > So, what I'm reading above is worrying. The interrupt is cleared > by the read of the data registers? I thought the datasheet allowed

Re: [PATCH 1/3] iio: light: Add driver for ap3216c

2019-02-18 Thread Jonathan Cameron
On Tue, 12 Feb 2019 22:25:39 -0500 Sven Van Asbroeck wrote: > Hi Bobby, > > On Tue, Feb 12, 2019 at 9:17 PM Robert Eshleman > wrote: > > > > First, thank you for the feedback. > > First of all, thank _you_ for doing the hard work on this driver ! > I very much respect what you've done

Re: [PATCH 1/3] iio: light: Add driver for ap3216c

2019-02-18 Thread Jonathan Cameron
On Tue, 12 Feb 2019 23:40:13 -0500 Sven Van Asbroeck wrote: > On Tue, Feb 12, 2019 at 3:47 PM Jonathan Cameron wrote: > > > > Hi Sven I think the issue, here is that you are putting guarantees on 'consistency' that IIO does not imply. In fact as I mention below, for many sensors it is not

Re: [PATCH 1/3] iio: light: Add driver for ap3216c

2019-02-13 Thread Robert Eshleman
Hey Peter, Thanks for the feedback. I have a question regarding INFO_SCALE and the calibration scale/bias below. Thanks for the feedback, it's much appreciated. On Mon, Feb 11, 2019 at 03:58:27PM +0100, Peter Meerwald-Stadler wrote: > On Sun, 10 Feb 2019, Robert Eshleman wrote: > > > This

Re: [PATCH 1/3] iio: light: Add driver for ap3216c

2019-02-13 Thread Robert Eshleman
On Tue, Feb 12, 2019 at 08:47:30PM +, Jonathan Cameron wrote: > On Mon, 11 Feb 2019 17:30:18 -0500 > Sven Van Asbroeck wrote: > > > On Mon, Feb 11, 2019 at 4:27 PM Jonathan Cameron wrote: > > > > > > Agreed. Or potentially just use regmap_bulk_read and rely on > > > the regmap internal

Re: [PATCH 1/3] iio: light: Add driver for ap3216c

2019-02-12 Thread Sven Van Asbroeck
On Tue, Feb 12, 2019 at 11:40 PM Sven Van Asbroeck wrote: > > Next, read ALS and PS _exclusively_ in the irq handler, guard it with > a mutex: > Wait a second, that wouldn't work, because we don't get an interrupt on every ALS/PS conversion, correct ? In that case, don't cache the als/ps value

Re: [PATCH 1/3] iio: light: Add driver for ap3216c

2019-02-12 Thread Sven Van Asbroeck
On Tue, Feb 12, 2019 at 3:47 PM Jonathan Cameron wrote: > > > Good question on whether it is guaranteed to read in increasing register > order (I didn't actually check the addresses are in increasing order > but assume they are or you would have pointed that out ;) > > That strikes me as

Re: [PATCH 1/3] iio: light: Add driver for ap3216c

2019-02-12 Thread Sven Van Asbroeck
Hi Bobby, On Tue, Feb 12, 2019 at 9:17 PM Robert Eshleman wrote: > > First, thank you for the feedback. First of all, thank _you_ for doing the hard work on this driver ! I very much respect what you've done here. > > I had initially went with a similar design, but there is > the case in which

Re: [PATCH 1/3] iio: light: Add driver for ap3216c

2019-02-12 Thread Robert Eshleman
On Mon, Feb 11, 2019 at 02:29:58PM -0500, Sven Van Asbroeck wrote: > On Sun, Feb 10, 2019 at 3:39 PM Robert Eshleman > wrote: > > > > This patch adds support for the ap3216c ambient light and proximity > > sensor. > > PS > > Why not use the chip in the mode where the interrupt is automatically

Re: [PATCH 1/3] iio: light: Add driver for ap3216c

2019-02-12 Thread Jonathan Cameron
On Mon, 11 Feb 2019 17:30:18 -0500 Sven Van Asbroeck wrote: > On Mon, Feb 11, 2019 at 4:27 PM Jonathan Cameron wrote: > > > > Agreed. Or potentially just use regmap_bulk_read and rely on > > the regmap internal locking to do it for you. > > Neat solution. But it may only work correctly iff

Re: [PATCH 1/3] iio: light: Add driver for ap3216c

2019-02-11 Thread Sven Van Asbroeck
On Mon, Feb 11, 2019 at 4:27 PM Jonathan Cameron wrote: > > Agreed. Or potentially just use regmap_bulk_read and rely on > the regmap internal locking to do it for you. Neat solution. But it may only work correctly iff regmap_bulk_read() reads the low address first. I'm not sure if this

Re: [PATCH 1/3] iio: light: Add driver for ap3216c

2019-02-11 Thread Jonathan Cameron
On Mon, 11 Feb 2019 14:09:12 -0500 Sven Van Asbroeck wrote: > Hi Robert, > > On Sun, Feb 10, 2019 at 3:39 PM Robert Eshleman > wrote: > > > > This patch adds support for the ap3216c ambient light and proximity > > sensor. > > Comments below. > Follow up inline... Mostly looks good to me,

Re: [PATCH 1/3] iio: light: Add driver for ap3216c

2019-02-11 Thread Sven Van Asbroeck
On Sun, Feb 10, 2019 at 3:39 PM Robert Eshleman wrote: > > This patch adds support for the ap3216c ambient light and proximity > sensor. PS Why not use the chip in the mode where the interrupt is automatically cleared by reading the data? This could work if you read the data in the interrupt

Re: [PATCH 1/3] iio: light: Add driver for ap3216c

2019-02-11 Thread Sven Van Asbroeck
Hi Robert, On Sun, Feb 10, 2019 at 3:39 PM Robert Eshleman wrote: > > This patch adds support for the ap3216c ambient light and proximity > sensor. Comments below. > > Supported features include: > > * Illuminance (lux) > * Proximity (raw) > * IR (raw) > * Rising/falling threshold events for

Re: [PATCH 1/3] iio: light: Add driver for ap3216c

2019-02-11 Thread Peter Meerwald-Stadler
On Sun, 10 Feb 2019, Robert Eshleman wrote: > This patch adds support for the ap3216c ambient light and proximity > sensor. comments below > Supported features include: > > * Illuminance (lux) > * Proximity (raw) > * IR (raw) > * Rising/falling threshold events for illuminance and proximity >

[PATCH 1/3] iio: light: Add driver for ap3216c

2019-02-10 Thread Robert Eshleman
This patch adds support for the ap3216c ambient light and proximity sensor. Supported features include: * Illuminance (lux) * Proximity (raw) * IR (raw) * Rising/falling threshold events for illuminance and proximity * Calibration scale for illuminance * Calibration bias for proximity