| Message ID | 20210921162433.1858296-1-dmitry.baryshkov@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mfd: qcom-pm8xxx: switch away from using chained IRQ handlers | expand |
On Tue 21 Sep 11:24 CDT 2021, Dmitry Baryshkov wrote: > PM8xxx PMIC family uses GPIO as parent IRQ. Using it together with the > irq_set_chained_handler_and_data() results in warnings from the GPIOLIB > as in this path the IRQ resources are not allocated (and thus the > corresponding GPIO is not marked as used for the IRQ. Use request_irq so > that the IRQ resources are proprely setup. > It's been a while since I booted a platform with ssbi PMIC (i.e. 8064 and older), but I don't remember ever seeing a warning. Can you please include in the commit message what warning this is and perhaps a reference to what in GPIOLIB changed to introduce this - in particular so that this fix can be found when people are searching the archive looking for similar issues. And I think it would be nice to Cc Linus Walleij. Change looks good though, so please update the commit message and add my: Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> Regards, Bjorn > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/mfd/qcom-pm8xxx.c | 39 ++++++++++++++++----------------------- > 1 file changed, 16 insertions(+), 23 deletions(-) > > diff --git a/drivers/mfd/qcom-pm8xxx.c b/drivers/mfd/qcom-pm8xxx.c > index ec18a04de355..2f2734ba5273 100644 > --- a/drivers/mfd/qcom-pm8xxx.c > +++ b/drivers/mfd/qcom-pm8xxx.c > @@ -65,7 +65,7 @@ > struct pm_irq_data { > int num_irqs; > struct irq_chip *irq_chip; > - void (*irq_handler)(struct irq_desc *desc); > + irq_handler_t irq_handler; > }; > > struct pm_irq_chip { > @@ -169,19 +169,16 @@ static int pm8xxx_irq_master_handler(struct pm_irq_chip *chip, int master) > return ret; > } > > -static void pm8xxx_irq_handler(struct irq_desc *desc) > +static irqreturn_t pm8xxx_irq_handler(int irq, void *data) > { > - struct pm_irq_chip *chip = irq_desc_get_handler_data(desc); > - struct irq_chip *irq_chip = irq_desc_get_chip(desc); > + struct pm_irq_chip *chip = data; > unsigned int root; > int i, ret, masters = 0; > > - chained_irq_enter(irq_chip, desc); > - > ret = regmap_read(chip->regmap, SSBI_REG_ADDR_IRQ_ROOT, &root); > if (ret) { > pr_err("Can't read root status ret=%d\n", ret); > - return; > + return IRQ_NONE; > } > > /* on pm8xxx series masters start from bit 1 of the root */ > @@ -192,7 +189,7 @@ static void pm8xxx_irq_handler(struct irq_desc *desc) > if (masters & (1 << i)) > pm8xxx_irq_master_handler(chip, i); > > - chained_irq_exit(irq_chip, desc); > + return IRQ_HANDLED; > } > > static void pm8821_irq_block_handler(struct pm_irq_chip *chip, > @@ -230,19 +227,17 @@ static inline void pm8821_irq_master_handler(struct pm_irq_chip *chip, > pm8821_irq_block_handler(chip, master, block); > } > > -static void pm8821_irq_handler(struct irq_desc *desc) > +static irqreturn_t pm8821_irq_handler(int irq, void *data) > { > - struct pm_irq_chip *chip = irq_desc_get_handler_data(desc); > - struct irq_chip *irq_chip = irq_desc_get_chip(desc); > + struct pm_irq_chip *chip = data; > unsigned int master; > int ret; > > - chained_irq_enter(irq_chip, desc); > ret = regmap_read(chip->regmap, > PM8821_SSBI_REG_ADDR_IRQ_MASTER0, &master); > if (ret) { > pr_err("Failed to read master 0 ret=%d\n", ret); > - goto done; > + return IRQ_NONE; > } > > /* bits 1 through 7 marks the first 7 blocks in master 0 */ > @@ -251,19 +246,18 @@ static void pm8821_irq_handler(struct irq_desc *desc) > > /* bit 0 marks if master 1 contains any bits */ > if (!(master & BIT(0))) > - goto done; > + return IRQ_NONE; > > ret = regmap_read(chip->regmap, > PM8821_SSBI_REG_ADDR_IRQ_MASTER1, &master); > if (ret) { > pr_err("Failed to read master 1 ret=%d\n", ret); > - goto done; > + return IRQ_NONE; > } > > pm8821_irq_master_handler(chip, 1, master); > > -done: > - chained_irq_exit(irq_chip, desc); > + return IRQ_HANDLED; > } > > static void pm8xxx_irq_mask_ack(struct irq_data *d) > @@ -574,14 +568,15 @@ static int pm8xxx_probe(struct platform_device *pdev) > if (!chip->irqdomain) > return -ENODEV; > > - irq_set_chained_handler_and_data(irq, data->irq_handler, chip); > + rc = devm_request_irq(&pdev->dev, irq, data->irq_handler, 0, dev_name(&pdev->dev), chip); > + if (rc) > + return rc; > + > irq_set_irq_wake(irq, 1); > > rc = of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev); > - if (rc) { > - irq_set_chained_handler_and_data(irq, NULL, NULL); > + if (rc) > irq_domain_remove(chip->irqdomain); > - } > > return rc; > } > @@ -594,11 +589,9 @@ static int pm8xxx_remove_child(struct device *dev, void *unused) > > static int pm8xxx_remove(struct platform_device *pdev) > { > - int irq = platform_get_irq(pdev, 0); > struct pm_irq_chip *chip = platform_get_drvdata(pdev); > > device_for_each_child(&pdev->dev, NULL, pm8xxx_remove_child); > - irq_set_chained_handler_and_data(irq, NULL, NULL); > irq_domain_remove(chip->irqdomain); > > return 0; > -- > 2.30.2 >
On 21/09/2021 22:49, Bjorn Andersson wrote: > On Tue 21 Sep 11:24 CDT 2021, Dmitry Baryshkov wrote: > >> PM8xxx PMIC family uses GPIO as parent IRQ. Using it together with the >> irq_set_chained_handler_and_data() results in warnings from the GPIOLIB >> as in this path the IRQ resources are not allocated (and thus the >> corresponding GPIO is not marked as used for the IRQ. Use request_irq so >> that the IRQ resources are proprely setup. >> > > It's been a while since I booted a platform with ssbi PMIC (i.e. 8064 > and older), but I don't remember ever seeing a warning. I've got Nexus 7 as an example of MDP4 hardware. The warning is more or less recent: from 2019 if I remember correctly. > > Can you please include in the commit message what warning this is and > perhaps a reference to what in GPIOLIB changed to introduce this - in > particular so that this fix can be found when people are searching the > archive looking for similar issues. Ack. > And I think it would be nice to Cc Linus Walleij. Ack. > > > Change looks good though, so please update the commit message and add > my: > > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > Regards, > Bjorn > >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> --- >> drivers/mfd/qcom-pm8xxx.c | 39 ++++++++++++++++----------------------- >> 1 file changed, 16 insertions(+), 23 deletions(-) >> >> diff --git a/drivers/mfd/qcom-pm8xxx.c b/drivers/mfd/qcom-pm8xxx.c >> index ec18a04de355..2f2734ba5273 100644 >> --- a/drivers/mfd/qcom-pm8xxx.c >> +++ b/drivers/mfd/qcom-pm8xxx.c >> @@ -65,7 +65,7 @@ >> struct pm_irq_data { >> int num_irqs; >> struct irq_chip *irq_chip; >> - void (*irq_handler)(struct irq_desc *desc); >> + irq_handler_t irq_handler; >> }; >> >> struct pm_irq_chip { >> @@ -169,19 +169,16 @@ static int pm8xxx_irq_master_handler(struct pm_irq_chip *chip, int master) >> return ret; >> } >> >> -static void pm8xxx_irq_handler(struct irq_desc *desc) >> +static irqreturn_t pm8xxx_irq_handler(int irq, void *data) >> { >> - struct pm_irq_chip *chip = irq_desc_get_handler_data(desc); >> - struct irq_chip *irq_chip = irq_desc_get_chip(desc); >> + struct pm_irq_chip *chip = data; >> unsigned int root; >> int i, ret, masters = 0; >> >> - chained_irq_enter(irq_chip, desc); >> - >> ret = regmap_read(chip->regmap, SSBI_REG_ADDR_IRQ_ROOT, &root); >> if (ret) { >> pr_err("Can't read root status ret=%d\n", ret); >> - return; >> + return IRQ_NONE; >> } >> >> /* on pm8xxx series masters start from bit 1 of the root */ >> @@ -192,7 +189,7 @@ static void pm8xxx_irq_handler(struct irq_desc *desc) >> if (masters & (1 << i)) >> pm8xxx_irq_master_handler(chip, i); >> >> - chained_irq_exit(irq_chip, desc); >> + return IRQ_HANDLED; >> } >> >> static void pm8821_irq_block_handler(struct pm_irq_chip *chip, >> @@ -230,19 +227,17 @@ static inline void pm8821_irq_master_handler(struct pm_irq_chip *chip, >> pm8821_irq_block_handler(chip, master, block); >> } >> >> -static void pm8821_irq_handler(struct irq_desc *desc) >> +static irqreturn_t pm8821_irq_handler(int irq, void *data) >> { >> - struct pm_irq_chip *chip = irq_desc_get_handler_data(desc); >> - struct irq_chip *irq_chip = irq_desc_get_chip(desc); >> + struct pm_irq_chip *chip = data; >> unsigned int master; >> int ret; >> >> - chained_irq_enter(irq_chip, desc); >> ret = regmap_read(chip->regmap, >> PM8821_SSBI_REG_ADDR_IRQ_MASTER0, &master); >> if (ret) { >> pr_err("Failed to read master 0 ret=%d\n", ret); >> - goto done; >> + return IRQ_NONE; >> } >> >> /* bits 1 through 7 marks the first 7 blocks in master 0 */ >> @@ -251,19 +246,18 @@ static void pm8821_irq_handler(struct irq_desc *desc) >> >> /* bit 0 marks if master 1 contains any bits */ >> if (!(master & BIT(0))) >> - goto done; >> + return IRQ_NONE; >> >> ret = regmap_read(chip->regmap, >> PM8821_SSBI_REG_ADDR_IRQ_MASTER1, &master); >> if (ret) { >> pr_err("Failed to read master 1 ret=%d\n", ret); >> - goto done; >> + return IRQ_NONE; >> } >> >> pm8821_irq_master_handler(chip, 1, master); >> >> -done: >> - chained_irq_exit(irq_chip, desc); >> + return IRQ_HANDLED; >> } >> >> static void pm8xxx_irq_mask_ack(struct irq_data *d) >> @@ -574,14 +568,15 @@ static int pm8xxx_probe(struct platform_device *pdev) >> if (!chip->irqdomain) >> return -ENODEV; >> >> - irq_set_chained_handler_and_data(irq, data->irq_handler, chip); >> + rc = devm_request_irq(&pdev->dev, irq, data->irq_handler, 0, dev_name(&pdev->dev), chip); >> + if (rc) >> + return rc; >> + >> irq_set_irq_wake(irq, 1); >> >> rc = of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev); >> - if (rc) { >> - irq_set_chained_handler_and_data(irq, NULL, NULL); >> + if (rc) >> irq_domain_remove(chip->irqdomain); >> - } >> >> return rc; >> } >> @@ -594,11 +589,9 @@ static int pm8xxx_remove_child(struct device *dev, void *unused) >> >> static int pm8xxx_remove(struct platform_device *pdev) >> { >> - int irq = platform_get_irq(pdev, 0); >> struct pm_irq_chip *chip = platform_get_drvdata(pdev); >> >> device_for_each_child(&pdev->dev, NULL, pm8xxx_remove_child); >> - irq_set_chained_handler_and_data(irq, NULL, NULL); >> irq_domain_remove(chip->irqdomain); >> >> return 0; >> -- >> 2.30.2 >> -- With best wishes Dmitry
On Tue, Sep 21, 2021 at 6:24 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > PM8xxx PMIC family uses GPIO as parent IRQ. Using it together with the > irq_set_chained_handler_and_data() results in warnings from the GPIOLIB > as in this path the IRQ resources are not allocated (and thus the > corresponding GPIO is not marked as used for the IRQ. Use request_irq so > that the IRQ resources are proprely setup. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> I sent this patch: https://lore.kernel.org/lkml/20210819154400.51932-1-linus.walleij@linaro.org/ David Heidelberg reported that it didn't work for him. David can you test Dmitry's patch instead and see if that works for you, I suppose I could have some bug in my patch :/ It would be nice with a Tested-by from David. FWIW the code looks good: Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
with this patch, it does boot without any problems or warnings regarding to IRQ. Tested-by: David Heidelberg <david@ixit.cz> Best regards David Heidelberg On Thu, Sep 23 2021 at 02:04:10 +0200, Linus Walleij <linus.walleij@linaro.org> wrote: > On Tue, Sep 21, 2021 at 6:24 PM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > >> PM8xxx PMIC family uses GPIO as parent IRQ. Using it together with >> the >> irq_set_chained_handler_and_data() results in warnings from the >> GPIOLIB >> as in this path the IRQ resources are not allocated (and thus the >> corresponding GPIO is not marked as used for the IRQ. Use >> request_irq so >> that the IRQ resources are proprely setup. >> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > I sent this patch: > https://lore.kernel.org/lkml/20210819154400.51932-1-linus.walleij@linaro.org/ > > David Heidelberg reported that it didn't work for him. > > David can you test Dmitry's patch instead and see if that works > for you, I suppose I could have some bug in my patch :/ > It would be nice with a Tested-by from David. > > FWIW the code looks good: > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > Yours, > Linus Walleij
On Thu, Sep 23, 2021 at 7:20 AM David Heidelberg <david@ixit.cz> wrote: > with this patch, it does boot without any problems or warnings > regarding to IRQ. Excellent, let's apply it! Yours, Linus Walleij
On Tue, 21 Sep 2021, Dmitry Baryshkov wrote: > PM8xxx PMIC family uses GPIO as parent IRQ. Using it together with the > irq_set_chained_handler_and_data() results in warnings from the GPIOLIB > as in this path the IRQ resources are not allocated (and thus the > corresponding GPIO is not marked as used for the IRQ. Use request_irq so > that the IRQ resources are proprely setup. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/mfd/qcom-pm8xxx.c | 39 ++++++++++++++++----------------------- > 1 file changed, 16 insertions(+), 23 deletions(-) Applied, thanks. -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
diff --git a/drivers/mfd/qcom-pm8xxx.c b/drivers/mfd/qcom-pm8xxx.c index ec18a04de355..2f2734ba5273 100644 --- a/drivers/mfd/qcom-pm8xxx.c +++ b/drivers/mfd/qcom-pm8xxx.c @@ -65,7 +65,7 @@ struct pm_irq_data { int num_irqs; struct irq_chip *irq_chip; - void (*irq_handler)(struct irq_desc *desc); + irq_handler_t irq_handler; }; struct pm_irq_chip { @@ -169,19 +169,16 @@ static int pm8xxx_irq_master_handler(struct pm_irq_chip *chip, int master) return ret; } -static void pm8xxx_irq_handler(struct irq_desc *desc) +static irqreturn_t pm8xxx_irq_handler(int irq, void *data) { - struct pm_irq_chip *chip = irq_desc_get_handler_data(desc); - struct irq_chip *irq_chip = irq_desc_get_chip(desc); + struct pm_irq_chip *chip = data; unsigned int root; int i, ret, masters = 0; - chained_irq_enter(irq_chip, desc); - ret = regmap_read(chip->regmap, SSBI_REG_ADDR_IRQ_ROOT, &root); if (ret) { pr_err("Can't read root status ret=%d\n", ret); - return; + return IRQ_NONE; } /* on pm8xxx series masters start from bit 1 of the root */ @@ -192,7 +189,7 @@ static void pm8xxx_irq_handler(struct irq_desc *desc) if (masters & (1 << i)) pm8xxx_irq_master_handler(chip, i); - chained_irq_exit(irq_chip, desc); + return IRQ_HANDLED; } static void pm8821_irq_block_handler(struct pm_irq_chip *chip, @@ -230,19 +227,17 @@ static inline void pm8821_irq_master_handler(struct pm_irq_chip *chip, pm8821_irq_block_handler(chip, master, block); } -static void pm8821_irq_handler(struct irq_desc *desc) +static irqreturn_t pm8821_irq_handler(int irq, void *data) { - struct pm_irq_chip *chip = irq_desc_get_handler_data(desc); - struct irq_chip *irq_chip = irq_desc_get_chip(desc); + struct pm_irq_chip *chip = data; unsigned int master; int ret; - chained_irq_enter(irq_chip, desc); ret = regmap_read(chip->regmap, PM8821_SSBI_REG_ADDR_IRQ_MASTER0, &master); if (ret) { pr_err("Failed to read master 0 ret=%d\n", ret); - goto done; + return IRQ_NONE; } /* bits 1 through 7 marks the first 7 blocks in master 0 */ @@ -251,19 +246,18 @@ static void pm8821_irq_handler(struct irq_desc *desc) /* bit 0 marks if master 1 contains any bits */ if (!(master & BIT(0))) - goto done; + return IRQ_NONE; ret = regmap_read(chip->regmap, PM8821_SSBI_REG_ADDR_IRQ_MASTER1, &master); if (ret) { pr_err("Failed to read master 1 ret=%d\n", ret); - goto done; + return IRQ_NONE; } pm8821_irq_master_handler(chip, 1, master); -done: - chained_irq_exit(irq_chip, desc); + return IRQ_HANDLED; } static void pm8xxx_irq_mask_ack(struct irq_data *d) @@ -574,14 +568,15 @@ static int pm8xxx_probe(struct platform_device *pdev) if (!chip->irqdomain) return -ENODEV; - irq_set_chained_handler_and_data(irq, data->irq_handler, chip); + rc = devm_request_irq(&pdev->dev, irq, data->irq_handler, 0, dev_name(&pdev->dev), chip); + if (rc) + return rc; + irq_set_irq_wake(irq, 1); rc = of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev); - if (rc) { - irq_set_chained_handler_and_data(irq, NULL, NULL); + if (rc) irq_domain_remove(chip->irqdomain); - } return rc; } @@ -594,11 +589,9 @@ static int pm8xxx_remove_child(struct device *dev, void *unused) static int pm8xxx_remove(struct platform_device *pdev) { - int irq = platform_get_irq(pdev, 0); struct pm_irq_chip *chip = platform_get_drvdata(pdev); device_for_each_child(&pdev->dev, NULL, pm8xxx_remove_child); - irq_set_chained_handler_and_data(irq, NULL, NULL); irq_domain_remove(chip->irqdomain); return 0;
PM8xxx PMIC family uses GPIO as parent IRQ. Using it together with the irq_set_chained_handler_and_data() results in warnings from the GPIOLIB as in this path the IRQ resources are not allocated (and thus the corresponding GPIO is not marked as used for the IRQ. Use request_irq so that the IRQ resources are proprely setup. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/mfd/qcom-pm8xxx.c | 39 ++++++++++++++++----------------------- 1 file changed, 16 insertions(+), 23 deletions(-) -- 2.30.2