Re: [PATCH] mfd: qcom-pm8xxx: switch away from using chained IRQ handlers

[Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>]

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@xxxxxxxxxx>
>
> Regards,
> Bjorn
>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > ---
> >   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