From: Eva Rachel Retuya <eraretuya@gmail.com>
To: Jonathan Cameron <jic23@kernel.org>
Cc: Rob Herring <robh+dt@kernel.org>,
"linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>,
Hartmut Knaack <knaack.h@gmx.de>,
Lars-Peter Clausen <lars@metafoo.de>,
Peter Meerwald <pmeerw@pmeerw.net>,
Dmitry Torokhov <dmitry.torokhov@gmail.com>,
Michael Hennerich <michael.hennerich@analog.com>,
Daniel Baluta <daniel.baluta@gmail.com>,
Alison Schofield <amsfield22@gmail.com>,
Florian Vaussard <florian.vaussard@heig-vd.ch>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger
Date: Wed, 10 May 2017 22:33:02 +0800 [thread overview]
Message-ID: <20170510143301.GE5456@Socrates-UM> (raw)
In-Reply-To: <7cba924d-333c-2bd0-8986-5513f833fe6a@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 <jic23@kernel.org> 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 <eraretuya@gmail.com>
> >> 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
> >
>
next prev parent reply other threads:[~2017-05-10 14:33 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-04-29 7:48 [PATCH v2 0/4] iio: accel: adxl345: Add support for buffered readings Eva Rachel Retuya
2017-04-29 7:48 ` [PATCH v2 1/4] dt-bindings: iio: accel: adxl345: Add optional interrupt-names support Eva Rachel Retuya
2017-04-29 7:48 ` [PATCH v2 2/4] iio: accel: adxl345_core: Introduce set_mode and data_ready functions Eva Rachel Retuya
2017-05-01 0:22 ` Jonathan Cameron
2017-05-01 19:42 ` Andy Shevchenko
2017-05-01 19:48 ` Jonathan Cameron
2017-05-01 20:07 ` Andy Shevchenko
2017-05-01 20:18 ` Jonathan Cameron
2017-05-02 11:39 ` Eva Rachel Retuya
2017-05-02 16:32 ` Jonathan Cameron
2017-05-10 13:07 ` Eva Rachel Retuya
2017-05-01 11:21 ` Andy Shevchenko
2017-05-02 11:46 ` Eva Rachel Retuya
2017-04-29 7:49 ` [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger Eva Rachel Retuya
2017-05-01 0:32 ` Jonathan Cameron
2017-05-02 3:01 ` Rob Herring
2017-05-02 15:59 ` Jonathan Cameron
2017-05-10 14:33 ` Eva Rachel Retuya [this message]
2017-05-02 11:59 ` Eva Rachel Retuya
2017-05-01 11:31 ` Andy Shevchenko
2017-05-02 12:15 ` Eva Rachel Retuya
2017-05-02 21:05 ` Andy Shevchenko
2017-05-10 13:24 ` Eva Rachel Retuya
2017-05-14 15:15 ` Jonathan Cameron
2017-05-14 16:08 ` Dmitry Torokhov
2017-05-05 18:26 ` Jonathan Cameron
2017-05-10 13:31 ` Eva Rachel Retuya
2017-04-29 7:49 ` [PATCH v2 4/4] iio: accel: adxl345: Add support for triggered buffer Eva Rachel Retuya
2017-05-01 0:42 ` Jonathan Cameron
2017-05-02 12:23 ` Eva Rachel Retuya
2017-05-02 16:08 ` Jonathan Cameron
2017-05-01 11:24 ` Andy Shevchenko
2017-05-02 12:24 ` Eva Rachel Retuya
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170510143301.GE5456@Socrates-UM \
--to=eraretuya@gmail.com \
--cc=amsfield22@gmail.com \
--cc=daniel.baluta@gmail.com \
--cc=dmitry.torokhov@gmail.com \
--cc=florian.vaussard@heig-vd.ch \
--cc=jic23@kernel.org \
--cc=knaack.h@gmx.de \
--cc=lars@metafoo.de \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=michael.hennerich@analog.com \
--cc=pmeerw@pmeerw.net \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).