From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753180AbdEJOdI (ORCPT ); Wed, 10 May 2017 10:33:08 -0400 Received: from mail-pg0-f67.google.com ([74.125.83.67]:35907 "EHLO mail-pg0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752503AbdEJOdG (ORCPT ); Wed, 10 May 2017 10:33:06 -0400 Date: Wed, 10 May 2017 22:33:02 +0800 From: Eva Rachel Retuya To: Jonathan Cameron Cc: Rob Herring , "linux-iio@vger.kernel.org" , Hartmut Knaack , Lars-Peter Clausen , Peter Meerwald , Dmitry Torokhov , Michael Hennerich , Daniel Baluta , Alison Schofield , Florian Vaussard , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger Message-ID: <20170510143301.GE5456@Socrates-UM> Mail-Followup-To: Jonathan Cameron , Rob Herring , "linux-iio@vger.kernel.org" , Hartmut Knaack , Lars-Peter Clausen , Peter Meerwald , Dmitry Torokhov , Michael Hennerich , Daniel Baluta , Alison Schofield , Florian Vaussard , "linux-kernel@vger.kernel.org" References: <977684c1-8c87-80ae-fc13-576e30e88384@kernel.org> <7cba924d-333c-2bd0-8986-5513f833fe6a@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <7cba924d-333c-2bd0-8986-5513f833fe6a@kernel.org> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, May 02, 2017 at 04:59:12PM +0100, Jonathan Cameron wrote: > On 02/05/17 04:01, Rob Herring wrote: > > On Sun, Apr 30, 2017 at 7:32 PM, Jonathan Cameron wrote: > >> On 29/04/17 08:49, Eva Rachel Retuya wrote: > >>> The ADXL345 provides a DATA_READY interrupt function to signal > >>> availability of new data. This interrupt function is latched and can be > >>> cleared by reading the data registers. The polarity is set to active > >>> high by default. > >>> > >>> Support this functionality by setting it up as an IIO trigger. > >>> > >>> In addition, two output pins INT1 and INT2 are available for driving > >>> interrupts. Allow mapping to either pins by specifying the > >>> interrupt-names property in device tree. > >>> > >>> Signed-off-by: Eva Rachel Retuya > >> Coming together nicely, but a few more bits and pieces inline... > >> > >> One slight worry is that the irq names stuff is to restrictive > >> as we want to direct different interrupts to different pins if > >> both are supported! > > > > [...] > > > >>> @@ -199,6 +253,22 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap, > >>> dev_err(dev, "Failed to set data range: %d\n", ret); > >>> return ret; > >>> } > >>> + /* > >>> + * Any bits set to 0 send their respective interrupts to the INT1 pin, > >>> + * whereas bits set to 1 send their respective interrupts to the INT2 > >>> + * pin. Map all interrupts to the specified pin. > >> This is an interesting comment. The usual reason for dual interrupt > >> pins is precisely to not map all functions to the same one. That allows > >> for a saving in querying which interrupt it is by having just the data ready > >> on one pin and just the events on the other... > >> > >> Perhaps the current approach won't support that mode of operation? > >> Clearly we can't merge a binding that enforces them all being the same > >> and then change it later as it'll be incompatible. > >> > >> I'm not quite sure how one should do this sort of stuff in DT though. > >> > >> Rob? > > > > DT should just describe what is connected which I gather here could be > > either one or both IRQs. We generally distinguish the IRQs with the > > interrupt-names property and then retrieve it as below. > Picking this branch to continue on I'll grab Eva's replay as well. > > Eva said: > > I've thought about this before since to me that's the better approach > > than one or the other. I'm in a time crunch before hence I went with > > this way. The input driver does this as well and what I just did is to > > match what it does. If you could point me some drivers for reference, > > I'll gladly analyze those and present something better on the next > > revision. > > So taking both of these and having thought about it a bit more in my > current jet lagged state (I hate travelling - particularly with the > added amusement of a flat tyre on the plane). > > To my mind we need to describe what interrupts at there as Rob says. > It's all obvious if there is only one interrupt connected (often > the case I suspect as pins are in short supply on many SoCs). > > If we allow the binding to specify both pins (using names to do the > matching to which pin they are on the chip), then we could allow > the driver itself to optimize the usage according to what is enabled. > Note though that this can come later - for now we just need to allow > the specification of both interrupts if they are present. > > So lets talk about the ideal ;) > Probably makes sense to separate dataready and the events if possible. > Ideal would be to even allow individual events to have there own pins > as long as there are only two available. So we need a heuristic to > work out what interrupts to put where. It doesn't work well as a lookup > table (I tried it) > > #define ADXL345_OVERRUN = BIT(0) > #define ADXL345_WATERMARK = BIT(1) > #define ADXL345_FREEFALL = BIT(2) > #define ADXL345_INACTIVITY = BIT(3) > #define ADXL345_ACTIVITY = BIT(4) > #define ADXL345_DOUBLE_TAP = BIT(5) > #define ADXL345_SINGLE_TAP = BIT(6) > #define ADXL345_DATA_READY = BIT(7) > > So some function that takes the bitmap of what is enabled and > tries to divide it sensibly. > > int adxl345_int_heuristic(u8 input, u8 *output) > { > long bounce; > switch (hweight8(&input)) > { > case 0 ... 1: > *output = input; > break; > case 2: > *output = BIT(ffs(&input)); //this will put one on each interrupt. > break; > case 3 ... 7: //now it gets tricky. Perhaps always have dataready and watermark on own interrupt if set? > > if (input & (ADXL345_DATA_READY | ADXL345_WATERMARK)) > output = input & (ADXL345_DATA_READY | ADXL345_WATERMARK); > else // taps always on same one etc... > } > } > > Then your interrupt handler will need to look at the incoming and work out if it > needs to read the status register to know what it has. If it doesn't > need to then it doesn't do so. Be careful to only clear the right > interrupts though in that case as it is always possible both are set. > > Anyhow, right now all that needs to be there is the binding to allow two interrupts. > Absolutely fine if for now the driver only uses the first one. > > Jonathan Thank you for explaining it well. I'll refer to this while working with the issue. Eva > > > >>> + */ > >>> + of_irq = of_irq_get_byname(dev->of_node, "INT2"); > >>> + if (of_irq == irq) > >>> + regval = 0xFF; > >>> + else > >>> + regval = 0x00; > > -- > > 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 > > >