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

[Bjorn Andersson <bjorn.andersson@linaro.org>]

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
>