Re: [RFC PATCH v2] regmap: regmap-irq/gpio-max77620: add level-irq support
On Mon, Dec 17, 2018 at 06:07:22PM +, Mark Brown wrote: > On Mon, Dec 17, 2018 at 10:42:48AM +0200, Matti Vaittinen wrote: > > On Thu, Dec 13, 2018 at 06:20:26PM +, Mark Brown wrote: > > > > I can't remember and can't find any record of any discussion of it which > > > is odd, might've been on IRC or something. Let's just remove it and see > > > what breaks, since we generally provide the type along with the request > > > for the interrupt I'm not sure how often the default actually gets used. > > > Possibly safer as a second patch though in case there is a good reason > > > that I missed so we can easily revert it. > > > So how do you see this - should the regmap_add_irq_chip read the current > > type setting information from HW and populate the cached type values > > based on the current HW configuration? (I think that would be corect > > thing to do). > > Yes. I'll go with this then. I'll try sending the patch removing the default edge configuration still today. > > > > It > > > does look safe to me but it's possible I missed something. Equally it > > > only seems to be some quite old Tegra systems using the max77620 so > > > perhaps mainline usage of affected devices is limited anyway... > > > Right. This makes me wonder if there is some other preferred approach on > > this... How other drivers are doing the type configurations? Why they > > are not using regmap-irq? Am I missing something? But what comes to > > changing the regmap-irq type-setting this is definitely a good news =) > > I suspect a lot of devices lack configurability or have never actually > done anything where configurability would matter - probably the biggest > use of regmap-irq is interrupts internal to a chip where there's no real > need for that, and even where there are GPIOs I'd be surprised if many > of them were actually used as interrupts rather than dumb outputs or > something given that most embedded systems have an abundance of GPIOs > directly on the SoC which are much better. Thanks for the explanation =) This makes sense. -- Matti Vaittinen ROHM Semiconductors ~~~ "I don't think so," said Rene Descartes. Just then, he vanished ~~~
Re: [RFC PATCH v2] regmap: regmap-irq/gpio-max77620: add level-irq support
On Mon, Dec 17, 2018 at 10:42:48AM +0200, Matti Vaittinen wrote: > On Thu, Dec 13, 2018 at 06:20:26PM +, Mark Brown wrote: > > I can't remember and can't find any record of any discussion of it which > > is odd, might've been on IRC or something. Let's just remove it and see > > what breaks, since we generally provide the type along with the request > > for the interrupt I'm not sure how often the default actually gets used. > > Possibly safer as a second patch though in case there is a good reason > > that I missed so we can easily revert it. > So how do you see this - should the regmap_add_irq_chip read the current > type setting information from HW and populate the cached type values > based on the current HW configuration? (I think that would be corect > thing to do). Yes. > > It > > does look safe to me but it's possible I missed something. Equally it > > only seems to be some quite old Tegra systems using the max77620 so > > perhaps mainline usage of affected devices is limited anyway... > Right. This makes me wonder if there is some other preferred approach on > this... How other drivers are doing the type configurations? Why they > are not using regmap-irq? Am I missing something? But what comes to > changing the regmap-irq type-setting this is definitely a good news =) I suspect a lot of devices lack configurability or have never actually done anything where configurability would matter - probably the biggest use of regmap-irq is interrupts internal to a chip where there's no real need for that, and even where there are GPIOs I'd be surprised if many of them were actually used as interrupts rather than dumb outputs or something given that most embedded systems have an abundance of GPIOs directly on the SoC which are much better. signature.asc Description: PGP signature
Re: [RFC PATCH v2] regmap: regmap-irq/gpio-max77620: add level-irq support
Hello Mark, On Thu, Dec 13, 2018 at 06:20:26PM +, Mark Brown wrote: > On Tue, Dec 11, 2018 at 04:05:55PM +0200, Matti Vaittinen wrote: > > > One specific question hit me while doing this. Why does the regmap-irq > > core do default trigger type configuration? I did leave this in the > > patch - but to me it is strange. For me it would be unexpected that the > > HW default trigger level is changed by common code. I would understand > > if change was done by some board specific code, or code specific to a > > chip - but 'core' doing this seems wrong to me. Should it be removed? > > I can't remember and can't find any record of any discussion of it which > is odd, might've been on IRC or something. Let's just remove it and see > what breaks, since we generally provide the type along with the request > for the interrupt I'm not sure how often the default actually gets used. > Possibly safer as a second patch though in case there is a good reason > that I missed so we can easily revert it. So how do you see this - should the regmap_add_irq_chip read the current type setting information from HW and populate the cached type values based on the current HW configuration? (I think that would be corect thing to do). > Unfortunately this also collides with a change I applied earlier on from > Bartosz which supports chips that use masks instead of a separate type > register to handle types so it'll need respinning, sorry about that. No problem - I'll fetch the latest changes from regulator tree and see how to fit this in. It may be this will be done after the holidays though - I'm not sure how my schedules are during the next few weeks... Besides I have also this "main IRQ status" -thing ongoing. > It > does look safe to me but it's possible I missed something. Equally it > only seems to be some quite old Tegra systems using the max77620 so > perhaps mainline usage of affected devices is limited anyway... Right. This makes me wonder if there is some other preferred approach on this... How other drivers are doing the type configurations? Why they are not using regmap-irq? Am I missing something? But what comes to changing the regmap-irq type-setting this is definitely a good news =) And... Thanks for all the feedback and support Mark! Br, Matti Vaittinen -- Matti Vaittinen ROHM Semiconductors ~~~ "I don't think so," said Rene Descartes. Just then, he vanished ~~~
Re: [RFC PATCH v2] regmap: regmap-irq/gpio-max77620: add level-irq support
On Tue, Dec 11, 2018 at 04:05:55PM +0200, Matti Vaittinen wrote: > One specific question hit me while doing this. Why does the regmap-irq > core do default trigger type configuration? I did leave this in the > patch - but to me it is strange. For me it would be unexpected that the > HW default trigger level is changed by common code. I would understand > if change was done by some board specific code, or code specific to a > chip - but 'core' doing this seems wrong to me. Should it be removed? I can't remember and can't find any record of any discussion of it which is odd, might've been on IRC or something. Let's just remove it and see what breaks, since we generally provide the type along with the request for the interrupt I'm not sure how often the default actually gets used. Possibly safer as a second patch though in case there is a good reason that I missed so we can easily revert it. Unfortunately this also collides with a change I applied earlier on from Bartosz which supports chips that use masks instead of a separate type register to handle types so it'll need respinning, sorry about that. It does look safe to me but it's possible I missed something. Equally it only seems to be some quite old Tegra systems using the max77620 so perhaps mainline usage of affected devices is limited anyway... signature.asc Description: PGP signature
[RFC PATCH v2] regmap: regmap-irq/gpio-max77620: add level-irq support
Add level active IRQ support to regmap-irq irqchip. Change breaks
existing regmap-irq type setting. Convert the existing drivers which
use regmap-irq with trigger type setting (gpio-max77620) to work
with this new approach. So we do not magically support level-active
IRQs on gpio-max77620 - but add support to the regmap-irq for chips
which support them =)
We do not support distinguishing situation where HW supports rising
and falling edge detection but not both. Separating this would require
inventing yet another flags for IRQ types. We use the existing
functionality which does logical OR for riding and falling edge values
if both edges are requested to be detected. This may not work on all
HWs.
Signed-off-by: Matti Vaittinen
---
One specific question hit me while doing this. Why does the regmap-irq
core do default trigger type configuration? I did leave this in the
patch - but to me it is strange. For me it would be unexpected that the
HW default trigger level is changed by common code. I would understand
if change was done by some board specific code, or code specific to a
chip - but 'core' doing this seems wrong to me. Should it be removed?
And I still don't have max77620 so I have only done _wery_ limited
testing. I would _really_ appreciate if someone had time to review this
thoroughly - and even happier if someone had possibility to try this out
with the max77620.
drivers/base/regmap/regmap-irq.c | 61 +++--
drivers/gpio/gpio-max77620.c | 96 ++--
include/linux/regmap.h | 29
3 files changed, 122 insertions(+), 64 deletions(-)
diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index 429ca8ed7e51..eacbb807d1c6 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -162,12 +162,8 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
continue;
reg = d->chip->type_base +
(i * map->reg_stride * d->type_reg_stride);
- if (d->chip->type_invert)
- ret = regmap_irq_update_bits(d, reg,
- d->type_buf_def[i], ~d->type_buf[i]);
- else
- ret = regmap_irq_update_bits(d, reg,
- d->type_buf_def[i], d->type_buf[i]);
+ ret = regmap_irq_update_bits(d, reg, d->type_buf_def[i],
+d->type_buf[i]);
if (ret != 0)
dev_err(d->map->dev, "Failed to sync type in %x\n",
reg);
@@ -212,27 +208,42 @@ static int regmap_irq_set_type(struct irq_data *data,
unsigned int type)
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);
- int reg = irq_data->type_reg_offset / map->reg_stride;
+ int reg;
+ const struct regmap_irq_type *t = &irq_data->type;
- if (!(irq_data->type_rising_mask | irq_data->type_falling_mask))
- return 0;
+ if ((t->types_supported & type) != type)
+ return -ENOTSUPP;
+
+ reg = t->type_reg_offset / map->reg_stride;
- d->type_buf[reg] &= ~(irq_data->type_falling_mask |
- irq_data->type_rising_mask);
+ if (t->type_reg_mask)
+ d->type_buf[reg] &= ~t->type_reg_mask;
+ else
+ d->type_buf[reg] &= ~(t->type_falling_val |
+ t->type_rising_val |
+ t->type_level_low_val |
+ t->type_level_high_val);
switch (type) {
case IRQ_TYPE_EDGE_FALLING:
- d->type_buf[reg] |= irq_data->type_falling_mask;
+ d->type_buf[reg] |= t->type_falling_val;
break;
case IRQ_TYPE_EDGE_RISING:
- d->type_buf[reg] |= irq_data->type_rising_mask;
+ d->type_buf[reg] |= t->type_rising_val;
break;
case IRQ_TYPE_EDGE_BOTH:
- d->type_buf[reg] |= (irq_data->type_falling_mask |
- irq_data->type_rising_mask);
+ d->type_buf[reg] |= (t->type_falling_val |
+ t->type_rising_val);
+ break;
+
+ case IRQ_TYPE_LEVEL_HIGH:
+ d->type_buf[reg] |= t->type_level_high_val;
break;
+ case IRQ_TYPE_LEVEL_LOW:
+ d->type_buf[reg] |= t->type_level_low_val;
+ break;
default:
return -EINVAL;
}
@@ -602,9 +613,15 @@ int regmap_add_irq_chip(struct regmap *map, int irq, int
irq_flags,
if (chip->num_type_reg) {
for (i = 0; i < chip->num_irqs; i++) {
-
