Re: [PATCH V2 8/8] iio: mma8452: Add support for interrupt driven triggers.

[Jonathan Cameron <jic23@kernel.org>]

On 07/08/14 18:20, Martin Fuzzey wrote:
> 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.
> 
Hope you had a good time!
> 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.
Leave it be, I'd missed the possiblity of both.
>>> +    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())
Fair enough. Thanks for the explanation.
> 
> 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.
Hmm.. More fool them if they don't check ;)

J
> 
> Regards,
> 
> Martin
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html