Re: [PATCH 1/2] pinctrl: bcm2835: Change init order for gpio hogs

[Phil Elwell <phil@raspberrypi.com>]

Hi Andy,

On 01/12/2021 12:06, Andy Shevchenko wrote:
> On Monday, November 29, 2021, Phil Elwell <phil@raspberrypi.com 
> <mailto:phil@raspberrypi.com>> wrote:
> 
>     ...and gpio-ranges
> 
>     pinctrl-bcm2835 is a combined pinctrl/gpio driver. Currently the gpio
>     side is registered first, but this breaks gpio hogs (which are
>     configured during gpiochip_add_data). Part of the hog initialisation
>     is a call to pinctrl_gpio_request, and since the pinctrl driver hasn't
>     yet been registered this results in an -EPROBE_DEFER from which it can
>     never recover.
> 
>     Change the initialisation sequence to register the pinctrl driver
>     first.
> 
>     This also solves a similar problem with the gpio-ranges property, which
>     is required in order for released pins to be returned to inputs.
> 
> 
> We have a callback in GPIO chip to register pin ranges, why driver does it 
> separately?

A few experiments (this is not my driver) appear to show that the call to 
pinctrl_add_gpio_range can be removed, but only once the gpio-ranges DT property 
has been added if we want to remain functionality throughout a bisect. That tidy 
up might be better done with a followup commit once the DT patch has also
been accepted, unless it's possible to guarantee the sequencing between
the pinctrl/gpio tree and the DT tree.

>     Fixes: 73345a18d464b ("pinctrl: bcm2835: Pass irqchip when adding
>                             gpiochip")
>     Signed-off-by: Phil Elwell <phil@raspberrypi.com <mailto:phil@raspberrypi.com>>
> 
> 
> 
> Is it originally so strange indentation or is it only on my side?

The "g" is below the "p" in the patch.

Thanks,

Phil

>     ---
>       drivers/pinctrl/bcm/pinctrl-bcm2835.c | 29 +++++++++++++++------------
>       1 file changed, 16 insertions(+), 13 deletions(-)
> 
>     diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
>     b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
>     index 2abcc6ce4eba..b607d10e4cbd 100644
>     --- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
>     +++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
>     @@ -1244,6 +1244,18 @@ static int bcm2835_pinctrl_probe(struct
>     platform_device *pdev)
>                      raw_spin_lock_init(&pc->irq_lock[i]);
>              }
> 
>     +       pc->pctl_desc = *pdata->pctl_desc;
>     +       pc->pctl_dev = devm_pinctrl_register(dev, &pc->pctl_desc, pc);
>     +       if (IS_ERR(pc->pctl_dev)) {
>     +               gpiochip_remove(&pc->gpio_chip);
>     +               return PTR_ERR(pc->pctl_dev);
>     +       }
>     +
>     +       pc->gpio_range = *pdata->gpio_range;
>     +       pc->gpio_range.base = pc->gpio_chip.base;
>     +       pc->gpio_range.gc = &pc->gpio_chip;
>     +       pinctrl_add_gpio_range(pc->pctl_dev, &pc->gpio_range);
>     +
>              girq = &pc->gpio_chip.irq;
>              girq->chip = &bcm2835_gpio_irq_chip;
>              girq->parent_handler = bcm2835_gpio_irq_handler;
>     @@ -1251,8 +1263,10 @@ static int bcm2835_pinctrl_probe(struct
>     platform_device *pdev)
>              girq->parents = devm_kcalloc(dev, BCM2835_NUM_IRQS,
>                                           sizeof(*girq->parents),
>                                           GFP_KERNEL);
>     -       if (!girq->parents)
>     +       if (!girq->parents) {
>     +               pinctrl_remove_gpio_range(pc->pctl_dev, &pc->gpio_range);
>                      return -ENOMEM;
>     +       }
> 
>              if (is_7211) {
>                      pc->wake_irq = devm_kcalloc(dev, BCM2835_NUM_IRQS,
>     @@ -1307,21 +1321,10 @@ static int bcm2835_pinctrl_probe(struct
>     platform_device *pdev)
>              err = gpiochip_add_data(&pc->gpio_chip, pc);
>              if (err) {
>                      dev_err(dev, "could not add GPIO chip\n");
>     +               pinctrl_remove_gpio_range(pc->pctl_dev, &pc->gpio_range);
>                      return err;
>              }
> 
>     -       pc->pctl_desc = *pdata->pctl_desc;
>     -       pc->pctl_dev = devm_pinctrl_register(dev, &pc->pctl_desc, pc);
>     -       if (IS_ERR(pc->pctl_dev)) {
>     -               gpiochip_remove(&pc->gpio_chip);
>     -               return PTR_ERR(pc->pctl_dev);
>     -       }
>     -
>     -       pc->gpio_range = *pdata->gpio_range;
>     -       pc->gpio_range.base = pc->gpio_chip.base;
>     -       pc->gpio_range.gc = &pc->gpio_chip;
>     -       pinctrl_add_gpio_range(pc->pctl_dev, &pc->gpio_range);
>     -
>              return 0;
>       }
> 
>     -- 
>     2.25.1
> 
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>