Re: [PATCH 5.10 29/76] pinctrl: bcm2835: Change init order for gpio hogs

Hi!

From: Phil Elwell phil@raspberrypi.com

[ Upstream commit 266423e60ea1b953fcc0cd97f3dad85857e434d1 ]

...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 does not get error handling right.

diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c index 1d21129f7751c..40ce18a0d0190 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);
  

• }

You do gpiochip_remove(), even when it was not added.

@@ -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;
• }

And yes, adding the removes here makes sense, but it looks like some are missing (in 5.10).

Something like this?

Best regards, Pavel diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c index 40ce18a0d019..d605548de7df 100644 --- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c +++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c @@ -1247,7 +1247,6 @@ static int bcm2835_pinctrl_probe(struct platform_device *pdev) 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); }

@@ -1264,16 +1263,18 @@ static int bcm2835_pinctrl_probe(struct platform_device *pdev) sizeof(*girq->parents), GFP_KERNEL); if (!girq->parents) { - pinctrl_remove_gpio_range(pc->pctl_dev, &pc->gpio_range); - return -ENOMEM; + err = -ENOMEM; + goto remove_gpio; }

if (is_7211) { pc->wake_irq = devm_kcalloc(dev, BCM2835_NUM_IRQS, sizeof(*pc->wake_irq), GFP_KERNEL); - if (!pc->wake_irq) - return -ENOMEM; + if (!pc->wake_irq) { + err = -ENOMEM; + goto remove_gpio; + } }

/* @@ -1297,8 +1298,10 @@ static int bcm2835_pinctrl_probe(struct platform_device *pdev)

len = strlen(dev_name(pc->dev)) + 16; name = devm_kzalloc(pc->dev, len, GFP_KERNEL); - if (!name) - return -ENOMEM; + if (!name) { + err = -ENOMEM; + goto remove_gpio; + }

snprintf(name, len, "%s:bank%d", dev_name(pc->dev), i);

@@ -1317,11 +1320,14 @@ 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; + goto remove_gpio; }

return 0; + +remove_gpio: + pinctrl_remove_gpio_range(pc->pctl_dev, &pc->gpio_range); + return err; }

static struct platform_driver bcm2835_pinctrl_driver = {