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

[Hans de Goede <hdegoede@redhat.com>]

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@linux.intel.com>
>> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
>> Reported-by: Hans de Goede <hdegoede@redhat.com>
>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>> ---
>> 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,
>>