Re: [PATCH v4 1/1] input: add driver for Bosch Sensortec's BMA150 accelerometer

[Jonathan Cameron <jic23@cam.ac.uk>]

On 07/22/11 14:24, Eric Andersson wrote:
>> Mostly looks fine.  Couple of nitpicks and one query about the interrupt
>> handling (more general than this driver).
>>
>> Can I confirm I've understood this correctly. When one receives any of the
>> interrupts, the driver just kicks out a reading?  Given there is a delay
>> before this read, imagine both high and low thresholds are set.  It's entirely
>> possible for both to occur before the thread gets there and the value that is
>> read to be somewhere in between... We'll get one interrupt and have no idea
>> what happened...
>>
>> Is there any way of working out what has occured and pushing that out?
> In our case we are enabling three conditional interrupts where each
> one of them have the chance of asserting the interrupt pin when
> their particular interrupt criterias are fulfilled. This means that
> one interrupt, for instance any motion, can assert the irq pin and
> then at the same time have a high- and/or low-g interrupt consecutively
> activated as well (but pin asserted by any motion already) where the
> pin will be kept asserted until all the criterias for each asserted
> interrupt are exited (i.e. not fullfilled anymore). So even if there
> is a delay from the pin assertion and the irq thread read-out it
> doesn't really matter if consecutive interrupts are activated in
> between. At least one of the criterias will be met and there will be a motion
> detection to read out. Of course, the interrupt types could be masked
> out from the status register (addr. 0x09 bits 0 & 1 for low-/high-g
> int), but I really don't see the purpose of this.
Perhaps tap type events?  High acceleration followed by not much. If unlucky
will register as stationary...  Lots of use cases out of input where you have
very little idea what is going on, but I guess what you have here might
work well for input.
> 
> A comment regarding latched vs. non-latched irqs - we are intentionally not
> using latched interrupts with the current setup since we currently don't
> see the need for it. A latched setup would typically be needed in a high
> inflow interrupt feature, such as new data interrupt, where the interrupt
> buffer can be flooded due to high event frequency.
> An alternative could be to activate the latched interrupt feature and
> explicitly clear the interrupts from the driver, but this would just fire
> off a new event directly until any of the interrupt criterias are fullfilled.
> If you have any preferences or requirements when it comes to this, please share
> your thoughts and we'll act upon this for the next version.
> 
>>> +	if (bma150->client->irq > 0) {
>>> +		ret = input_register_device(idev);
>>> +		if (ret < 0) {
>>> +			input_free_device(idev);
>>> +			return ret;
>>> +		}
>>> +		ret = request_threaded_irq(bma150->client->irq, NULL, bma150_irq_thread,
>>> +			IRQF_TRIGGER_RISING, BMA150_DRIVER, bma150);
>> Does this want to be oneshot? I don't think you have the latch enabled, so strobing
>> across a threshold could result in an interrupt storm.
> Sure, it could be worth having it sequential to the read-outs from the thread
> instead of buffering events while reading. I'll fix.
> 
> As for your nitpicks, I will fix them for the next version.
> 
> Thanks for reviewing!
>