Re: [PATCH v2 10/16] gpio: add a reusable generic gpio_chip using regmap

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux SPI]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]     [Yosemite Campsites]

  Powered by Linux