Re: [RFC] regmap-irq: add "main register" and level-irq support

[Matti Vaittinen]

On Fri, Dec 07, 2018 at 01:14:18PM +0000, Mark Brown wrote:
> 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:
> 
> > 		d->domain = irq_domain_add_linear(map->dev->of_node,
> > 						  chip->num_irqs,
> > 						  &regmap_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, &regmap);
> 
> > 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,
> > 						  &regmap_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).

Do you think this would be benefical even if we add the 'main irq
support'? If so, I can generate a patch out of this. I think this would
really suffice for my current need - but this stops wokrking as soon as
more than one sub-irq-chip want's to expose interrupts via DT.

> > > 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.

I could send an example on how the driver utilizing the original RFC
interface would look like. I am starting to think it was not *that* bad
after all...

> > > 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.

I did some experiment. I will post this as another RFC - but I am really
not terribly happy about it. It's complex (well, in my opinion) and I am
not sure the driver interface is much easier. But you can see it
yourself.


-- 
Matti Vaittinen
ROHM Semiconductors

~~~ "I don't think so," said Rene Descartes.  Just then, he vanished ~~~