Re: [PATCH] gpio/pinctrl: Add pin ranges before gpiochip

[Hans de Goede <hdegoede@xxxxxxxxxx>]

Hi,

On 30-10-2019 16:48, Hans de Goede wrote:
> Hi,
>
> On 30-10-2019 15:49, Linus Walleij wrote:
> > This fixes a semantic ordering issue as we need to add
> > pin ranges before adding gpiochips when gpiochips use
> > pin control as a backend: as it needs to talk to the
> > pin control backend during initialization, the backend
> > needs to be there already.
> >
> > Other drivers in the tree using pincontrol as backend do
> > not necessarily have this problem, as they might not need
> > to access the pincontrol portions during initialization.
> >
> > Cc: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > Cc: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> > Reported-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> > Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> > ---
> > Hans: would be great if you could test this. I can't
> > even compiletest right now because of a slow machine
> > as workhorse...
>
> This does not seem to work I've tested in both a Bay Trail and
> a Cherry Trail device and neither will boot the kernel after
> these changes I'm afraid.
>
> I think it might be best to pass in an array of ranges (*) to
> gpiochip_add_data and have it register the ranges before
> doing the irq-chip setup.

p.s.

For 5.4 we should probably revert
"gpio: merrifield: Pass irqchip when adding gpiochip"
and the fixes added on top of it, since AFAICT _AEI handling
will be broken on merrifield after this change too.

So I suggest that we revert the following commits (in revert order):

4c87540940cbc7ddbe9674087919c605fd5c2ef1 "gpio: merrifield: Move hardware initialization to callback"
6658f87f219427ee776c498e07c878eb5cad1be2 "gpio: merrifield: Restore use of irq_base"
8f86a5b4ad679e4836733b47414226074eee4e4d "gpio: merrifield: Pass irqchip when adding gpiochip"

That seems like the safest thing to do at this point in the cycle.

Regards,

Hans


> *) Terminated with a range from 0 to 0
>
> > ---
> >   drivers/gpio/gpio-merrifield.c             | 18 +++++++++++-------
> >   drivers/pinctrl/intel/pinctrl-baytrail.c   | 14 +++++++++-----
> >   drivers/pinctrl/intel/pinctrl-cherryview.c | 16 ++++++++++------
> >   drivers/pinctrl/intel/pinctrl-intel.c      | 16 ++++++++++------
> >   drivers/pinctrl/pinctrl-amd.c              | 14 +++++++++-----
> >   5 files changed, 49 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/gpio/gpio-merrifield.c b/drivers/gpio/gpio-merrifield.c
> > index 2f1e9da81c1e..195e253cb561 100644
> > --- a/drivers/gpio/gpio-merrifield.c
> > +++ b/drivers/gpio/gpio-merrifield.c
> > @@ -463,13 +463,10 @@ static int mrfld_gpio_probe(struct pci_dev *pdev, const struct pci_device_id *id
> >       girq->default_type = IRQ_TYPE_NONE;
> >       girq->handler = handle_bad_irq;
> > -    pci_set_drvdata(pdev, priv);
> > -    retval = devm_gpiochip_add_data(&pdev->dev, &priv->chip, priv);
> > -    if (retval) {
> > -        dev_err(&pdev->dev, "gpiochip_add error %d\n", retval);
> > -        return retval;
> > -    }
> > -
> > +    /*
> > +     * Needs to happen first since the gpiochip is using pin
> > +     * control as back-end.
> > +     */
> >       pinctrl_dev_name = mrfld_gpio_get_pinctrl_dev_name(priv);
> >       for (i = 0; i < ARRAY_SIZE(mrfld_gpio_ranges); i++) {
> >           range = &mrfld_gpio_ranges[i];
> > @@ -484,6 +481,13 @@ static int mrfld_gpio_probe(struct pci_dev *pdev, const struct pci_device_id *id
> >           }
> >       }
> > +    pci_set_drvdata(pdev, priv);
> > +    retval = devm_gpiochip_add_data(&pdev->dev, &priv->chip, priv);
> > +    if (retval) {
> > +        dev_err(&pdev->dev, "gpiochip_add error %d\n", retval);
> > +        return retval;
> > +    }
> > +
> >       return 0;
> >   }
> > diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c
> > index beb26550c25f..b308567c5153 100644
> > --- a/drivers/pinctrl/intel/pinctrl-baytrail.c
> > +++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
> > @@ -1549,16 +1549,20 @@ static int byt_gpio_probe(struct byt_gpio *vg)
> >           girq->handler = handle_bad_irq;
> >       }
> > -    ret = devm_gpiochip_add_data(&vg->pdev->dev, gc, vg);
> > +    /*
> > +     * Needs to happen first since the gpiochip is using pin
> > +     * control as back-end.
> > +     */
> > +    ret = gpiochip_add_pin_range(gc, dev_name(&vg->pdev->dev),
> > +                     0, 0, vg->soc_data->npins);
> >       if (ret) {
> > -        dev_err(&vg->pdev->dev, "failed adding byt-gpio chip\n");
> > +        dev_err(&vg->pdev->dev, "failed to add GPIO pin range\n");
> >           return ret;
> >       }
> > -    ret = gpiochip_add_pin_range(&vg->chip, dev_name(&vg->pdev->dev),
> > -                     0, 0, vg->soc_data->npins);
> > +    ret = devm_gpiochip_add_data(&vg->pdev->dev, gc, vg);
> >       if (ret) {
> > -        dev_err(&vg->pdev->dev, "failed to add GPIO pin range\n");
> > +        dev_err(&vg->pdev->dev, "failed adding byt-gpio chip\n");
> >           return ret;
> >       }
> > diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
> > index dff2a81250b6..13144e192c1f 100644
> > --- a/drivers/pinctrl/intel/pinctrl-cherryview.c
> > +++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
> > @@ -1572,12 +1572,10 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
> >       if (need_valid_mask)
> >           chip->irq.init_valid_mask = chv_init_irq_valid_mask;
> > -    ret = devm_gpiochip_add_data(pctrl->dev, chip, pctrl);
> > -    if (ret) {
> > -        dev_err(pctrl->dev, "Failed to register gpiochip\n");
> > -        return ret;
> > -    }
> > -
> > +    /*
> > +     * Needs to happen first since the gpiochip is using pin
> > +     * control as back-end.
> > +     */
> >       for (i = 0; i < community->ngpio_ranges; i++) {
> >           range = &community->gpio_ranges[i];
> >           ret = gpiochip_add_pin_range(chip, dev_name(pctrl->dev),
> > @@ -1589,6 +1587,12 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
> >           }
> >       }
> > +    ret = devm_gpiochip_add_data(pctrl->dev, chip, pctrl);
> > +    if (ret) {
> > +        dev_err(pctrl->dev, "Failed to register gpiochip\n");
> > +        return ret;
> > +    }
> > +
> >       /*
> >        * The same set of machines in chv_no_valid_mask[] have incorrectly
> >        * configured GPIOs that generate spurious interrupts so we use
> > diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> > index b54b27228ad9..78afcf13c444 100644
> > --- a/drivers/pinctrl/intel/pinctrl-intel.c
> > +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> > @@ -1225,12 +1225,10 @@ static int intel_gpio_probe(struct intel_pinctrl *pctrl, int irq)
> >       pctrl->irqchip.irq_set_wake = intel_gpio_irq_wake;
> >       pctrl->irqchip.flags = IRQCHIP_MASK_ON_SUSPEND;
> > -    ret = devm_gpiochip_add_data(pctrl->dev, &pctrl->chip, pctrl);
> > -    if (ret) {
> > -        dev_err(pctrl->dev, "failed to register gpiochip\n");
> > -        return ret;
> > -    }
> > -
> > +    /*
> > +     * Needs to happen first since the gpiochip is using pin
> > +     * control as back-end.
> > +     */
> >       for (i = 0; i < pctrl->ncommunities; i++) {
> >           struct intel_community *community = &pctrl->communities[i];
> > @@ -1241,6 +1239,12 @@ static int intel_gpio_probe(struct intel_pinctrl *pctrl, int irq)
> >           }
> >       }
> > +    ret = devm_gpiochip_add_data(pctrl->dev, &pctrl->chip, pctrl);
> > +    if (ret) {
> > +        dev_err(pctrl->dev, "failed to register gpiochip\n");
> > +        return ret;
> > +    }
> > +
> >       /*
> >        * We need to request the interrupt here (instead of providing chip
> >        * to the irq directly) because on some platforms several GPIO
> > diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
> > index 2c61141519f8..3637059083ff 100644
> > --- a/drivers/pinctrl/pinctrl-amd.c
> > +++ b/drivers/pinctrl/pinctrl-amd.c
> > @@ -912,17 +912,21 @@ static int amd_gpio_probe(struct platform_device *pdev)
> >           return PTR_ERR(gpio_dev->pctrl);
> >       }
> > -    ret = gpiochip_add_data(&gpio_dev->gc, gpio_dev);
> > -    if (ret)
> > -        return ret;
> > -
> > +    /*
> > +     * Needs to happen first since the gpiochip is using pin
> > +     * control as back-end.
> > +     */
> >       ret = gpiochip_add_pin_range(&gpio_dev->gc, dev_name(&pdev->dev),
> >                   0, 0, gpio_dev->gc.ngpio);
> >       if (ret) {
> >           dev_err(&pdev->dev, "Failed to add pin range\n");
> > -        goto out2;
> > +        return ret;
> >       }
> > +    ret = gpiochip_add_data(&gpio_dev->gc, gpio_dev);
> > +    if (ret)
> > +        return ret;
> > +
> >       ret = gpiochip_irqchip_add(&gpio_dev->gc,
> >                   &amd_gpio_irqchip,
> >                   0,