diff mbox

[v4,0/5] pinctrl/GPIO driver for Apple SoCs

Message ID 20211024101838.43107-1-joey.gouly@arm.com
State New
Headers show

Commit Message

Joey Gouly Oct. 24, 2021, 10:18 a.m. UTC 
Hi all,

Here is the v4 patchset for the Apple pinctrl/GPIO driver.

Changes since v3 [1]:
  - Applied Marc Zyngier's review/patch (with exception noted below)
  - Removed "apple,t8103-pinctrl" compatible from driver
  - Applied Acks/Review tags


With Marc's changes, the irq_chip was being shared between pinctrl
drivers and I was getting the following warning:

  drivers/gpio/gpiolib.c:1456:

    detected irqchip that is shared with multiple gpiochips: please fix
the driver.


So I applied the following diff to Marc's change, to not share the
irq_chips, while still being cleaner overall:

        pctl->gpio_chip.free = gpiochip_generic_free;
@@ -385,7 +388,7 @@ static int apple_gpio_gpio_register(struct
apple_gpio_pinctrl *pctl)
        if (girq->num_parents) {
                int i;

-               girq->chip = &apple_gpio_irqchip;
+               girq->chip = &pctl->irq_chip;
                girq->parent_handler = apple_gpio_gpio_irq_handler;

                girq->parents = kmalloc_array(girq->num_parents,

Marc said that hierarchical IRQ domains should solve this problem, but
I'll let him explain more on the list, maybe that should solved in a different
patch series.

There is a branch here with the driver:
  https://gitlab.arm.com/linux-arm/jg-open/-/tree/pinctrl_apple_v4

Thanks,
Joey

note: For those that have been testing this with PCIe etc, you will
probably want to also include the last commit in the following branch, as I
dropped the clock references in the code (due to the switch to power domains):
https://gitlab.arm.com/linux-arm/jg-open/-/commits/pinctrl_apple_v4_clock

[1]
https://lore.kernel.org/linux-gpio/20211016141839.45460-1-joey.gouly@arm.com/

Joey Gouly (4):
  dt-bindings: pinctrl: add #interrupt-cells to apple,pinctrl
  dt-bindings: pinctrl: Add apple,npins property to apple,pinctrl
  pinctrl: add pinctrl/GPIO driver for Apple SoCs
  MAINTAINERS: add pinctrl-apple-gpio to ARM/APPLE MACHINE

Marc Zyngier (1):
  gpio: Allow per-parent interrupt data

 .../bindings/pinctrl/apple,pinctrl.yaml       |  10 +
 MAINTAINERS                                   |   1 +
 drivers/gpio/gpiolib.c                        |   9 +-
 drivers/pinctrl/Kconfig                       |  16 +
 drivers/pinctrl/Makefile                      |   1 +
 drivers/pinctrl/pinctrl-apple-gpio.c          | 535 ++++++++++++++++++
 include/linux/gpio/driver.h                   |  19 +-
 7 files changed, 587 insertions(+), 4 deletions(-)
 create mode 100644 drivers/pinctrl/pinctrl-apple-gpio.c

Comments

Linus Walleij Oct. 24, 2021, 9:34 p.m. UTC | #1 
On Sun, Oct 24, 2021 at 12:19 PM Joey Gouly <joey.gouly@arm.com> wrote:

> Here is the v4 patchset for the Apple pinctrl/GPIO driver.

I'll wait for Marc Z:s comment on the irqchip instances, I'm
lost myself, I always solved it with per-instance irqchips.

Otherwise the patch series looks good to me, if Marcan
ACKs it too I am ready to merge them!

Yours,
Linus Walleij
Hector Martin Oct. 25, 2021, 9:12 a.m. UTC | #2 
On 24/10/2021 19.18, Joey Gouly wrote:
> Hi all,
> 
> Here is the v4 patchset for the Apple pinctrl/GPIO driver.
> 
> Changes since v3 [1]:
>    - Applied Marc Zyngier's review/patch (with exception noted below)
>    - Removed "apple,t8103-pinctrl" compatible from driver
>    - Applied Acks/Review tags

I mentioned two nits in a reply to #4, but nothing worth blocking 
merging over, so this is:

Acked-by: Hector Martin <marcan@marcan.st>

Linus: You can take everything but the MAINTAINERS patch, I'll push that 
one via SoC together with everything else for this window to avoid merge 
hell :)
Marc Zyngier Oct. 25, 2021, 10:28 a.m. UTC | #3 
On Sun, 24 Oct 2021 11:18:33 +0100,
Joey Gouly <joey.gouly@arm.com> wrote:
> 
> Hi all,
> 
> Here is the v4 patchset for the Apple pinctrl/GPIO driver.
> 
> Changes since v3 [1]:
>   - Applied Marc Zyngier's review/patch (with exception noted below)
>   - Removed "apple,t8103-pinctrl" compatible from driver
>   - Applied Acks/Review tags
> 
> 
> With Marc's changes, the irq_chip was being shared between pinctrl
> drivers and I was getting the following warning:
> 
>   drivers/gpio/gpiolib.c:1456:
> 
>     detected irqchip that is shared with multiple gpiochips: please fix
> the driver.
> 
> 
> So I applied the following diff to Marc's change, to not share the
> irq_chips, while still being cleaner overall:
> 
> diff --git a/drivers/pinctrl/pinctrl-apple-gpio.c
> b/drivers/pinctrl/pinctrl-apple-gpio.c
> index 732c347a2bbc..ce037a5c15c1 100644
> --- a/drivers/pinctrl/pinctrl-apple-gpio.c
> +++ b/drivers/pinctrl/pinctrl-apple-gpio.c
> @@ -35,6 +35,7 @@ struct apple_gpio_pinctrl {
> 
>         struct pinctrl_desc pinctrl_desc;
>         struct gpio_chip gpio_chip;
> +       struct irq_chip irq_chip;
>         u8 irqgrps[0];
>  };
> 
> @@ -369,6 +370,8 @@ static int apple_gpio_gpio_register(struct
> apple_gpio_pinctrl *pctl)
>                 return dev_err_probe(pctl->dev, -ENODEV,
>                                      "No gpio-controller property\n");
> 
> +       pctl->irq_chip = apple_gpio_irqchip;
> +
>         pctl->gpio_chip.label = dev_name(pctl->dev);
>         pctl->gpio_chip.request = gpiochip_generic_request;
>         pctl->gpio_chip.free = gpiochip_generic_free;
> @@ -385,7 +388,7 @@ static int apple_gpio_gpio_register(struct
> apple_gpio_pinctrl *pctl)
>         if (girq->num_parents) {
>                 int i;
> 
> -               girq->chip = &apple_gpio_irqchip;
> +               girq->chip = &pctl->irq_chip;
>                 girq->parent_handler = apple_gpio_gpio_irq_handler;
> 
>                 girq->parents = kmalloc_array(girq->num_parents,
> 
> Marc said that hierarchical IRQ domains should solve this problem, but
> I'll let him explain more on the list, maybe that should solved in a different
> patch series.

The issue I have with the gpiolib code is that it hijacks function
pointers from a structure that is not under its control, and that is
exactly what the hierarchical IRQ domains/irqchips were supposed to
prevent.

It isn't obvious to me why this cannot be fixed with a gpiolib domain
and irqchip stacked on top of the one exposed by the low-level driver,
providing the required hooks in a standard way. Yes, this is even more
indirection. It also isn't clear why gpiochip_set_irq_hooks() shouts:
if the hooks are already there, move on.

Ultimately, this sort of manipulation is what prevents the irq_chip
structure from being made 'const' everywhere (ok, there is another nit
because of the parent_device field, which I'm looking at getting rid
of). Keeping writable function pointers isn't great, overall.

Now, given that this is an issue that isn't directly related to the
driver at hand, it shouldn't be a blocker for merging it.

So for the driver itself:

Reviewed-by: Marc Zyngier <maz@kernel.org>

	M.
Linus Walleij Oct. 25, 2021, 11:26 p.m. UTC | #4 
On Mon, Oct 25, 2021 at 11:13 AM Hector Martin <marcan@marcan.st> wrote:
> On 24/10/2021 19.18, Joey Gouly wrote:
> > Hi all,
> >
> > Here is the v4 patchset for the Apple pinctrl/GPIO driver.
> >
> > Changes since v3 [1]:
> >    - Applied Marc Zyngier's review/patch (with exception noted below)
> >    - Removed "apple,t8103-pinctrl" compatible from driver
> >    - Applied Acks/Review tags
>
> I mentioned two nits in a reply to #4, but nothing worth blocking
> merging over, so this is:
>
> Acked-by: Hector Martin <marcan@marcan.st>
>
> Linus: You can take everything but the MAINTAINERS patch, I'll push that
> one via SoC together with everything else for this window to avoid merge
> hell :)

Excellent Joey can you respin the series with Hectors fixes on patch #4
and add in the ACKs and review tags also Marc Z:s, so I can queue
this for v5.16 ASAP.

Any remaining issues can certainly be ironed out in-tree.

Yours,
Linus Walleij
diff mbox

Patch

diff --git a/drivers/pinctrl/pinctrl-apple-gpio.c
b/drivers/pinctrl/pinctrl-apple-gpio.c
index 732c347a2bbc..ce037a5c15c1 100644
--- a/drivers/pinctrl/pinctrl-apple-gpio.c
+++ b/drivers/pinctrl/pinctrl-apple-gpio.c
@@ -35,6 +35,7 @@  struct apple_gpio_pinctrl {

        struct pinctrl_desc pinctrl_desc;
        struct gpio_chip gpio_chip;
+       struct irq_chip irq_chip;
        u8 irqgrps[0];
 };

@@ -369,6 +370,8 @@  static int apple_gpio_gpio_register(struct
apple_gpio_pinctrl *pctl)
                return dev_err_probe(pctl->dev, -ENODEV,
                                     "No gpio-controller property\n");

+       pctl->irq_chip = apple_gpio_irqchip;
+
        pctl->gpio_chip.label = dev_name(pctl->dev);
        pctl->gpio_chip.request = gpiochip_generic_request;