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

[Robert Eshleman <bobbyeshleman@gmail.com>]

On Tue, Feb 12, 2019 at 08:47:30PM +0000, Jonathan Cameron wrote:
> On Mon, 11 Feb 2019 17:30:18 -0500
> Sven Van Asbroeck <thesven73@gmail.com> wrote:
> 
> > On Mon, Feb 11, 2019 at 4:27 PM Jonathan Cameron <jic23@kernel.org> 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 function has that guarantee. If
> > somebody changes
> > the read order, the driver will break. But I think I'm being overly
> > paranoid here :)
> 
> 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 behaviour that should probably be documented as long
> as it is true currently.
> 
> > 
> > > So yes, it's more than possible that userspace won't get the same number
> > > of events as samples taken over the limit, but I don't know why we care.
> > > We can about missing a threshold being passed entirely, not about knowing
> > > how many samples we were above it for.  
> > 
> > I suspect that we run a small risk of losing an event, like so:
> > 
> > PS (12.5 ms)
> > --> interrupt -> iio event  
> More interesting if this one never happened, so we got a one off proximity
> event missed.
> 
> > ALS (100 ms)
> > --> interrupt -> iio event  
> > PS (12.5 ms)
> > --> interrupt ========= no iio event generated  
> > ALS (100 ms)
> > --> interrupt -> iio event  
> > 
> > To see why, imagine that the scheduler decides to move away from the
> > threaded interrupt
> > handler right before ap3216c_clear_int(). Say 20ms, which I know is a
> > loooong time,
> > but bear with me, the point is that it _could_ happen as we're not a RTOS.
> > 
> > static irqreturn_t ap3216c_event_handler(int irq, void *p)
> > {
> >         /* imagine ALS interrupt came in, INT_STATUS is 0b01 */
> >         regmap_read(data->regmap, AP3216C_INT_STATUS, &status);
> >         if (status & mask1) iio_push_event(PROX);
> >         if (status & mask2) iio_push_event(LIGHT);
> > 
> >         /* imagine schedule happens here */
> >         msleep(20);
> >         /* while we were not running, PS interrupt came in
> >            INT_STATUS is now 0b11
> >            yet no new interrupt is generated, as we are ONESHOT
> >         */
> >         ap3216c_clear_int(data);
> >         /* clears both bits, interrupt line goes low.
> >             knowledge that the PS interrupt came in is now lost */
> > }
> > 
> > Not sure if that's acceptable driver behaviour. In real life it
> > probably wouldn't matter much,
> > except for occasional added latency maybe ?
> Good point, I'd missed that a single clear was clearing both bits
> rather than just the one we thought had fired.
> 
> If we clear just the right one, (which I think we can do from
> the datasheet
> "1: Software clear after writing 1 into address 0x01 each bit#"
> However the code isn't writing a 3 in that clear, so I'm not
> sure if the datasheet is correct or not...
> 
> and it is a level interrupt (which I think it is?) then we would
> be safe against this miss.
> 
> If either we can only globally clear or it's not a level interrupt
> there isn't much we can do to avoid a miss, it's just a bad hardware
> design.

This totally makes sense, obviously something I had missed.  
I think you are right, if each INT bit is cleared individually,
then that second event won't be lost.

I'll take a closer look at the ideas put forth by you and Sven here on
using mutex on some of the other cases (such as the write_event_config
vs event_handler race condition) and put those into v2.

Thanks again for all the constructive feedback.

-Bobby