| Message ID | 20211129105556.675235-2-phil@raspberrypi.com |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: bcm2835: Fix gpio hogs and pin reinitialisation | expand |
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 > >
On Wed, Dec 1, 2021 at 5:18 PM Phil Elwell <phil@raspberrypi.com> wrote: > 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. What I meant is why these calls are done in the probe and not in ->add_pin_ranges() callback? Shouldn't it fix the issue you have observed? ... > > 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. Which is wrong. Tags mustn't be multilines (i.e. split over a single line).
On 01/12/2021 15:38, Andy Shevchenko wrote: > On Wed, Dec 1, 2021 at 5:18 PM Phil Elwell <phil@raspberrypi.com> wrote: >> 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. > > What I meant is why these calls are done in the probe and not in > ->add_pin_ranges() callback? > Shouldn't it fix the issue you have observed? I'm no expert in the field, isn't it preferable to set the gpio-ranges pinctrl<->GPIO correspondence with a single line of DT rather than several lines of code? A quick grep shows over 700 instances of gpio-ranges in DT, at least some of which are reflexive, and only 7 drivers with add_pin_ranges methods. >>> 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. > > Which is wrong. Tags mustn't be multilines (i.e. split over a single line). checkpatch disagrees: scripts/checkpatch.pl 0001-ARM-dts-gpio-ranges-property-is-now-required.patch WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line) #10: [1] commit 2ab73c6d8323 ("gpio: Support GPIO controllers without pin-ranges") Phil
On Wed, Dec 1, 2021 at 6:08 PM Phil Elwell <phil@raspberrypi.com> wrote: > On 01/12/2021 15:38, Andy Shevchenko wrote: ... > >>> Fixes: 73345a18d464b ("pinctrl: bcm2835: Pass irqchip when adding > >>> gpiochip") > >>> Is it originally so strange indentation or is it only on my side? > >> > >> The "g" is below the "p" in the patch. > > > > Which is wrong. Tags mustn't be multilines (i.e. split over a single line). > > checkpatch disagrees: I don't care less about checkpatch false positives. You have a chance to fix it, btw.
On 01/12/2021 16:25, Andy Shevchenko wrote: > On Wed, Dec 1, 2021 at 6:08 PM Phil Elwell <phil@raspberrypi.com> wrote: >> On 01/12/2021 15:38, Andy Shevchenko wrote: > > ... > >>>>> Fixes: 73345a18d464b ("pinctrl: bcm2835: Pass irqchip when adding >>>>> gpiochip") > >>>>> Is it originally so strange indentation or is it only on my side? >>>> >>>> The "g" is below the "p" in the patch. >>> >>> Which is wrong. Tags mustn't be multilines (i.e. split over a single line). >> >> checkpatch disagrees: > > I don't care less about checkpatch false positives. You have a chance > to fix it, btw. I only split it because of checkpatch. What a great tool. I'll change for V2, which I can submit now if you'll ack it with just that change. Phil
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; }
...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. Fixes: 73345a18d464b ("pinctrl: bcm2835: Pass irqchip when adding gpiochip") Signed-off-by: Phil Elwell <phil@raspberrypi.com> --- drivers/pinctrl/bcm/pinctrl-bcm2835.c | 29 +++++++++++++++------------ 1 file changed, 16 insertions(+), 13 deletions(-)