Messages in this thread | ![/](/images/icornerl.gif) | | Date | Fri, 7 Dec 2018 13:14:18 +0000 | From | Mark Brown <> | Subject | Re: [RFC] regmap-irq: add "main register" and level-irq support |
| |
On Fri, Dec 07, 2018 at 09:58:29AM +0200, Matti Vaittinen wrote: > On Wed, Dec 05, 2018 at 05:27:01PM +0000, Mark Brown wrote:
> > If they're full subdevices you'd be pointing at the relevant platform > > device anyway.
> I am not sure if I have explained me well enough =) When we look at > regmap_add_irq_chip we see that it creates the IRQ domains. And we > further see: > > d->domain = irq_domain_add_linear(map->dev->of_node, > chip->num_irqs, > ®map_domain_ops, d);
> where map->dev->of_node points the node added to regmap. And as we > really want to use the i2c to access registers, we should have done the > regmap using:
> devm_regmap_init_i2c(i2c, ®map);
> where the i2c is the struct i2c_client *. The dev in i2c_client is the > i2c device - which has of_node pointing at the "main i2c node" - not the > sub block nodes. Hence all irq chips created by regmap_add_irq_chip for > this physical i2c slave device will point to the same DT node.
Hrm, right. You'd need to have a proxy regmap for the child devices for that. Not ideal.
> bool mask_writeonly:1; > + bool no_of:1; > > and in the regmap_add_irq_chip do:
> node = (chip->no_of) ? NULL : map->dev->of_node; > d->domain = irq_domain_add_linear(node, chip->num_irqs, > ®map_domain_ops, d);
> Then we could have chip->no_of set for the 'main irq chip' and for those > chips we don't wan't to expose via DT. In my case I would leave no_of > unset only for the irq-chip which I created for the GPIO? Is this a > silly idea?
That's worth a shot, yes - I'd need to see it fully fleshed out I think but it looks sensible (no ternery operator please).
> > The mapping isn't guaranteed to be a 1:1 mapping - one way this hardware > > gets structured is that the central interrupt controller is saying which > > IP block is flagging an interrupt rather than which register has an > > interrupt set in it. You can then have either more than one detailed > > status register for that IP
> Correct. But I guess the IP blocks often have limited set of registers for the > "sub interrupts". For such cases my idea would work, right. My RFC > handled case where there is many 'sub registers' to read for one 'main > bit.
Your idea definitely works for the current case, yes - I'm just thinking about future edge and extension cases.
> > or several smaller IPs reporting through a > > single detailed status register.
> I think that if we have more than two layers of irqs (main and sub) - > then we should do cascaded controllers.
Yeah, and my first thought here is that we should just be using cascaded controllers all the time (but like I say I'm not 100% certain on that).
> > Right, it's about working out which subregister to read - the point is > > you can do this by adding a link in either direction, I'm suggesting > > doing it from the individual interrupt to the main register since we > > already have individual data structures for those and it feels less > > likely to run into hard to represent corner cases.
> I see your point now. But as I said, I am not sure we should add the > overhead of 'main irq bit description' for simple cases just to cover > the corner cases. Yet I can try seeing what I can come up with if you > think this is the way to go.
If you could take a look that'd be great.
> > > * @main_status: Optional base main status register address. Some chips have > > > * "main status register(s)" which indicate actual irq registers > > > * with unhandled interrupts. Provide main status register > > > * address here to avoid reading irq registers with no > > > * active interrupts. Mapping from main status value to > > > * interrupt register can be provided in sub_reg_offsets.
> > > Perhaps this would clarify that the status_base, mask_base, ack_base, > > > etc. should still be used normally?
> > Probably.
> I am not sure this means I was able to convince you or if this is the > polite way to tell me not to bother =) I am Finnish guy who does not > understand small talk or polite hints ;)
I mean that probably if you add clarifications like you suggest that might do it. It's one of these things where you can't 100% tell how things will look until you see the actual thing. [unhandled content-type:application/pgp-signature] | ![\](/images/icornerr.gif) |