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

[Jonathan Cameron]

On Mon, 11 Feb 2019 17:30:18 -0500
Sven Van Asbroeck <thesven73@xxxxxxxxx> wrote:

> On Mon, Feb 11, 2019 at 4:27 PM Jonathan Cameron <jic23@xxxxxxxxxx> 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.

Jonathan