From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH v3 1/2] gpio: Add support for hierarchical IRQ domains Date: Mon, 3 Jun 2019 09:53:24 +0200 Message-ID: <20190603075324.GA27753@ulmo> References: <20190529145322.20630-1-thierry.reding@gmail.com> <20190529145322.20630-2-thierry.reding@gmail.com> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="+QahgC5+KEYLbs62" Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Linus Walleij Cc: Thomas Gleixner , Lina Iyer , Jon Hunter , Sowjanya Komatineni , Bitan Biswas , "open list:GPIO SUBSYSTEM" , linux-tegra@vger.kernel.org, "linux-kernel@vger.kernel.org" List-Id: linux-tegra@vger.kernel.org --+QahgC5+KEYLbs62 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sun, Jun 02, 2019 at 03:46:00PM +0200, Linus Walleij wrote: > On Wed, May 29, 2019 at 4:53 PM Thierry Reding = wrote: >=20 > > From: Thierry Reding > > > > Hierarchical IRQ domains can be used to stack different IRQ controllers > > on top of each other. One specific use-case where this can be useful is > > if a power management controller has top-level controls for wakeup > > interrupts. In such cases, the power management controller can be a > > parent to other interrupt controllers and program additional registers > > when an IRQ has its wake capability enabled or disabled. > > > > Signed-off-by: Thierry Reding > > --- > > Changes in v3: > > - use irq_create_fwspec_mapping() instead of irq_domain_alloc_irqs() > > - add missing kerneldoc for new parent_domain field > > - keep IRQ_DOMAIN dependency for clarity > > > > Changes in v2: > > - select IRQ_DOMAIN_HIERARCHY to avoid build failure > > - move more code into the gpiolib core >=20 > This is looking really good! >=20 > > config GPIOLIB_IRQCHIP > > select IRQ_DOMAIN > > + select IRQ_DOMAIN_HIERARCHY > > bool >=20 > Hm OK I guess. It would be ugly to ifdef all hierarchy > code in gpiolib. >=20 > > static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset) > > { > > + struct irq_domain *domain =3D chip->irq.domain; > > + > > if (!gpiochip_irqchip_irq_valid(chip, offset)) > > return -ENXIO; > > > > + if (irq_domain_is_hierarchy(domain)) { > > + struct irq_fwspec spec; > > + > > + spec.fwnode =3D domain->fwnode; > > + spec.param_count =3D 2; > > + spec.param[0] =3D offset; > > + spec.param[1] =3D IRQ_TYPE_NONE; > > + > > + return irq_create_fwspec_mapping(&spec); > > + } > > + > > return irq_create_mapping(chip->irq.domain, offset); >=20 > This is looking really good! >=20 > > + /* > > + * Allow GPIO chips to override the ->to_irq() if they really n= eed to. > > + * This should only be very rarely needed, the majority should = be fine > > + * with gpiochip_to_irq(). > > + */ > > + if (!gpiochip->to_irq) > > + gpiochip->to_irq =3D gpiochip_to_irq; >=20 > Please drop this. The default .to_irq() should be good for everyone. > Also patch 2/2 now contains a identical copy of the gpiolib > .to_irq() which I suspect you indended to drop, actually. It's not actually identical to the gpiolib implementation. There's still the conversion to the non-linear DT representation for GPIO specifiers =66rom the linear GPIO number space, which is not taken care of by the gpiolib variant. That's precisely the point why this patch makes it possible to let the driver override things. More generally, if we drop this we restrict access to the built-in hierarchical support to devices that use 2 cells as the specifier. Most drivers support that, but there are a few exceptions: - gpio-lpc32xx - gpio-mt7621 - gpio-pxa gpio-pxa seems like it's really just two-cell, but the other two are definitely different. As discussed before gpio-tegra186 is also different but could be tweaked into doing two-cell by generating the GPIO/IRQ map upfront. Thierry --+QahgC5+KEYLbs62 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAlz00fEACgkQ3SOs138+ s6ExUg/+Kgpk/o+5xU/+vPkJcf5qgh8ZH6Ieri+vtLVy6O9OIMHNXZB9Lq9DLVB7 Z21poqg0pUmpUttaCmGPkecyWqBH1vkgkZWLiLJ5BD40TiHFiOCHDC1MhrEBocsl k5RDqV/29pRq+itQq8o31RoxPvvYaKzgYd2ReFtOcptI901cF8m905TICVk480sZ WCe74YNqMTCQSg7ZMM2LkCVz+vZsCdbza1OtsL0rzIXpdAAvmGNtNMzYhZBn+l7b LmhG82DM9Zu53rQaVgderWXO/3XhQo7JQQTgTEXNP9bSpxeRU0gxnR7Wy1VtD+yr +mKRLLZ0oEUAhMg8RAxJPeFPPCZB+Xu7T2l9cL/BewHAH6V/1DompdJPokWLw8+Q v3CKY8962TcyrQo3tcDHnYUjmsiS69L+P5/Q5aydRZ8aGjKykrBfEF87D693uFm6 nqShCP6IHFxJnWVIYSEC9GfHuKW+Osae55lqReE5zJdtMnpmRcl5qSZIywi1VUYE BkxBL/gf6WjSqjdJxdHYje87Dhz0/OhKktGPLGRt04MfrOIAHaTUXjdIfJ28oylG sIavcbuO6d2mdBFAgGeQ/XpF1/8OPBSWHNyMwWw3nPt1vvjJaGMvfd1DrTBHJ52K MSGfRQ2/Ot14UGU6sYz3ZbZwZm9T1d5u5OZTTe7FN7sxib1/Kuk= =O3rF -----END PGP SIGNATURE----- --+QahgC5+KEYLbs62--