From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from saturn.retrosnub.co.uk ([178.18.118.26]:38799 "EHLO saturn.retrosnub.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752351AbaHNRMa (ORCPT ); Thu, 14 Aug 2014 13:12:30 -0400 Message-ID: <53ECEDFD.7090409@kernel.org> Date: Thu, 14 Aug 2014 18:12:29 +0100 From: Jonathan Cameron MIME-Version: 1.0 To: Martin Fuzzey CC: linux-iio@vger.kernel.org, pmeerw@pmeerw.net Subject: Re: [PATCH V2 8/8] iio: mma8452: Add support for interrupt driven triggers. References: <20140729090128.4618.92936.stgit@localhost> <20140729090145.4618.72768.stgit@localhost> <53E3AA82.5040004@kernel.org> <53E3B57A.4030201@parkeon.com> In-Reply-To: <53E3B57A.4030201@parkeon.com> Content-Type: text/plain; charset=utf-8 Sender: linux-iio-owner@vger.kernel.org List-Id: linux-iio@vger.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