Hi Jonathan and thanks for the review.
I'll send my comments but I'll be on holiday from tomorrow evening until
beginning of september so won't be able to send a V3 until later.
On 07/08/14 18:34, Jonathan Cameron wrote:
---
@@ -555,18 +557,24 @@ static irqreturn_t mma8452_interrupt(int irq, void *p)
{
struct iio_dev *indio_dev = p;
struct mma8452_data *data = iio_priv(indio_dev);
+ int ret = IRQ_NONE;
int src;
src = i2c_smbus_read_byte_data(data->client, MMA8452_INT_SRC);
if (src < 0)
return IRQ_NONE;
+ if (src & MMA8452_INT_DRDY) {
+ iio_trigger_poll_chained(indio_dev->trig, iio_get_time_ns());
+ ret = IRQ_HANDLED;
+ }
+
if (src & MMA8452_INT_TRANS) {
mma8452_transient_interrupt(indio_dev);
- return IRQ_HANDLED;
+ ret = IRQ_HANDLED;
}
- return IRQ_NONE;
Leave the returns as they were and just return from the new if statement...
It's neater and there is no reason to do a single point of exit if
there is no cleaning up to do.
The final return still has to be changed so that the function correctly
handles the case of both interrupt bits being set.
The idea wasn't so much as to create a single point of return as to keep
the two handler statements symmetrical and not have the second one a
special case just because it's the last.
The hardware supports other interrupts and there's a risk of someone
adding a third one later and forgetting to modify the second one.
But if you really insist I can do it that way.
+ return ret;
}
+static void mma8452_trigger_cleanup(struct iio_dev *indio_dev)
+{
+ if (indio_dev->trig) {
+ iio_trigger_unregister(indio_dev->trig);
Probably no reason not use devm_iio_trigger_alloc and avoid the frees.
Doesn't save much, but someone else will only propose a patch 10 minutes
after this goes in if you don't do it...
Ok will do for V3
if (client->irq) {
- int supported_interrupts = MMA8452_INT_TRANS;
+ int supported_interrupts = MMA8452_INT_DRDY | MMA8452_INT_TRANS;
+ int enabled_interrupts = MMA8452_INT_TRANS;
Note again, I'm very much against coming up with any events enabled....
It's not what people expect so all sorts of messy things could occur.
Yes, quite agree but actually they aren't enabled.
The bit allowing any interrupts generated by the transient detection
module to be routed to the interrupt pin is enabled.
The threshold is set to the maximum value (your remark on a previous patch).
BUT the bit that actually enables the transient detection (and hence the
events) is not set until userspace asks for it (by
mma8452_write_event_config())
The reason for setting the initial threshold is for the case where
userspace enables the event before writing the threshold value.
It seemed safer to default to a high threshold than a low one.
Regards,
Martin
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
