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@xxxxxxxxxx> 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@xxxxxxxxx>
> >> 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@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
