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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
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
