Hi Linus,
Am 2020-04-17 08:34, schrieb Michael Walle:
Hi Linus,
Am 2020-04-16 11:27, schrieb Linus Walleij:
On Thu, Apr 2, 2020 at 10:37 PM Michael Walle <michael@xxxxxxxx>
wrote:
There are quite a lot simple GPIO controller which are using regmap
to
access the hardware. This driver tries to be a base to unify existing
code into one place. This won't cover everything but it should be a
good
starting point.
It does not implement its own irq_chip because there is already a
generic one for regmap based devices. Instead, the irq_chip will be
instanciated in the parent driver and its irq domain will be
associate
to this driver.
For now it consists of the usual registers, like set (and an optional
clear) data register, an input register and direction registers.
Out-of-the-box, it supports consecutive register mappings and
mappings
where the registers have gaps between them with a linear mapping
between
GPIO offset and bit position. For weirder mappings the user can
register
its own .xlate().
Signed-off-by: Michael Walle <michael@xxxxxxxx>
Overall I really like this driver and I think we should merge is as
soon
as it is in reasonable shape and then improve on top so we can start
migrating drivers to it.
+static int gpio_regmap_to_irq(struct gpio_chip *chip, unsigned int
offset)
+{
+ struct gpio_regmap_data *data = gpiochip_get_data(chip);
+ struct gpio_regmap *gpio = data->gpio;
+
+ /* the user might have its own .to_irq callback */
+ if (gpio->to_irq)
+ return gpio->to_irq(gpio, offset);
+
+ return irq_create_mapping(gpio->irq_domain, offset);
I think that should at least be irq_find_mapping(), the mapping should
definately not be created by the .to_irq() callback since that is just
a convenience function.
what do you mean by conenience function? are there other ways? if you
use
irq_find_mapping() who will create the mappings? most gpio drivers use
a
similar function like gpio_regmap_to_irq().
+ if (gpio->irq_domain)
+ chip->to_irq = gpio_regmap_to_irq;
I don't know about this.
(...)
+ * @irq_domain: (Optional) IRQ domain if the
controller is
+ * interrupt-capable
(...)
+ struct irq_domain *irq_domain;
I don't think this is a good storage place for the irqdomain, we
already have
gpio_irq_chip inside gpio_chip and that has an irqdomain, we should
strive to reuse that infrastructure also for regmap GPIO I think, for
now
I would just leave .to_irq() out of this and let the driver deal with
any
irqs.
How would a driver attach the to_irq callback then? At the moment, the
gpio_regmap doesn't expose the gpio_chip. So either we have to do that
or
the config still have to have a .to_irq property.
Also, if I move the interrupt hanling completely out of the gpio-regmap,
the
driver would have to deal with "struct gpio_chip" which I would like to
avoid
if possible and keep it private to gpio-regmap.
Unfortunately, I don't have much experience how a good API for the
interrupt
handling and the gpio-regmap might look like. And there seems to be some
overlap between regmap-irq and the interrupt stuff in gpiolib. For
example,
both provide and set the irq_domain_ops. Thus handing the domain over to
gpio-regmap looked like a good idea to me. I get you point, that there
is
already a irqdomain in gpiolib and also a _to_irq() which is the same as
the current implementation in gpio-regmap. Maybe it makes sense to just
have a new function
int gpiolib_add_irqdomain(struct gpio_chip *gc, struct irq_domain
domain)
{
gc->irq.domain = domain;
gc->to_irq = gpiochip_to_irq;
}
which is called by gpio_regmap_register() if a config->irq_domain is
given.
-michael
