[PATCH 0/1] regmap-irq: support chips with separate mask bits for irq edges

[PATCH 0/1] regmap-irq: support chips with separate mask bits for irq edges

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

I'm working on an MFD driver (and its accompanying drivers in various
subsystems) for a simple low-power PMIC which exposes a single GPIO
line. It has a couple of interrupts all bunched together in two
registers and all of them but the one for GPIO are controlled by
single mask bits. The GPIO interrupt is configured with two separate
bits - one for rising edge and one for falling edge interrupts.

Since the device is relatively simple I would really like to avoid
having to write the entire irq_chip boiler code if regmap_irq_chip
would be perfect in this case.

We already have the type mask fields in struct regmap_irq. This
patch proposes a simple change that reuses them. If the mask_base
and type_base offsets are the same, the enable callback will
use the bits written to the type buffer by the set_type callback
to only enable the requested edge interrupt.

Bartosz Golaszewski (1):
  regmap: irq: handle HW using separate mask bits for edges

 drivers/base/regmap/regmap-irq.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

-- 
2.19.1

[PATCH 1/1] regmap: irq: handle HW using separate mask bits for edges

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Some interrupt controllers use separate bits for controlling rising
and falling edge interrupts in the mask register.

Let's reuse the existing type fields in struct regmap_irq to make
regmap_irq_chip available to such HW.

If the type_base and mask_base offsets are the same - assume there
are separate bits for falling and rising edge interrupts and use
the value previously written to the type buffer by the set_type()
callback instead of the entire mask specified for this interrupt
so that we only enable the requested edge interrupts.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/base/regmap/regmap-irq.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index 429ca8ed7e51..109ae353c526 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -194,8 +194,24 @@ static void regmap_irq_enable(struct irq_data *data)
 	struct regmap_irq_chip_data *d = irq_data_get_irq_chip_data(data);
 	struct regmap *map = d->map;
 	const struct regmap_irq *irq_data = irq_to_regmap_irq(d, data->hwirq);
+	unsigned int mask;
 
-	d->mask_buf[irq_data->reg_offset / map->reg_stride] &= ~irq_data->mask;
+	/*
+	 * If the type_base and mask_base addresses are the same, then
+	 * the underlying hardware uses separate mask bits for rising and
+	 * falling edge interrupts, but we want to make them into a single
+	 * virtual interrupt with configurable edge.
+	 *
+	 * Instead of using the regular mask bits for this interrupt, use
+	 * the value previously written to the type buffer at the
+	 * corresponding offset in regmap_irq_set_type().
+	 */
+	if (unlikely(d->chip->type_base == d->chip->mask_base))
+		mask = d->type_buf[irq_data->reg_offset / map->reg_stride];
+	else
+		mask = irq_data->mask;
+
+	d->mask_buf[irq_data->reg_offset / map->reg_stride] &= ~mask;
 }
 
 static void regmap_irq_disable(struct irq_data *data)
-- 
2.19.1

Re: [PATCH 0/1] regmap-irq: support chips with separate mask bits for irq edges

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 648 bytes --]

On Tue, Dec 04, 2018 at 07:15:49PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> I'm working on an MFD driver (and its accompanying drivers in various
> subsystems) for a simple low-power PMIC which exposes a single GPIO
> line. It has a couple of interrupts all bunched together in two

Please don't send cover letters for single patches, if there is anything
that needs saying put it in the changelog of the patch or after the ---
if it's administrative stuff.  This reduces mail volume and ensures that 
any important information is recorded in the changelog rather than being
lost. 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/1] regmap: irq: handle HW using separate mask bits for edges

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1044 bytes --]

On Tue, Dec 04, 2018 at 07:15:50PM +0100, Bartosz Golaszewski wrote:

> Let's reuse the existing type fields in struct regmap_irq to make
> regmap_irq_chip available to such HW.

I'm not sure this is ideal, it makes the interface less clear for users
especially since there's nothing in the comments in the header that
users will look at which mentions the feature.

> If the type_base and mask_base offsets are the same - assume there
> are separate bits for falling and rising edge interrupts and use
> the value previously written to the type buffer by the set_type()
> callback instead of the entire mask specified for this interrupt
> so that we only enable the requested edge interrupts.

This feels like it's very strongly tied to a specific implementation of
the feature and TBH I'm somewhat unclear on what this ends up concretely
meaning.  It sounds like this hardware represents the two edges as
separate interrupts but you want to combine them into one but I can't
see exactly how the interrupt number gets mapped with your change.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/1] regmap: irq: handle HW using separate mask bits for edges

[Bartosz Golaszewski]

śr., 5 gru 2018 o 16:35 Mark Brown <broonie@kernel.org> napisał(a):
>
> On Tue, Dec 04, 2018 at 07:15:50PM +0100, Bartosz Golaszewski wrote:
>
> > Let's reuse the existing type fields in struct regmap_irq to make
> > regmap_irq_chip available to such HW.
>
> I'm not sure this is ideal, it makes the interface less clear for users
> especially since there's nothing in the comments in the header that
> users will look at which mentions the feature.
>
> > If the type_base and mask_base offsets are the same - assume there
> > are separate bits for falling and rising edge interrupts and use
> > the value previously written to the type buffer by the set_type()
> > callback instead of the entire mask specified for this interrupt
> > so that we only enable the requested edge interrupts.
>
> This feels like it's very strongly tied to a specific implementation of
> the feature and TBH I'm somewhat unclear on what this ends up concretely
> meaning.  It sounds like this hardware represents the two edges as
> separate interrupts but you want to combine them into one but I can't
> see exactly how the interrupt number gets mapped with your change.

Yes, it's two edges represented as separate interrupts. I will post a
patch with a different (hopefully clearer) approach together with an
example snippet from the code actually using it.

Bart