Re: [PATCH v1 2/4] mfd: intel_soc_pmic_bxtwc: Use IRQ domain for TMU device

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 3-Oct-24 7:32 PM, Andy Shevchenko wrote:
> While design wise the idea of converting the driver to use
> the hierarchy of the IRQ chips is correct, the implementation
> has (inherited) flaws. This was unvelead when platform_get_irq()
> had started WARN() on IRQ 0 that is supposed to be a Linux
> IRQ number (also known as vIRQ).
> 
> Rework the driver to respect IRQ domain when creating each MFD
> device separately, as the domain is not the same for all of them.
> 
> Fixes: 957ae5098185 ("platform/x86: Add Whiskey Cove PMIC TMU support")
> Fixes: 57129044f504 ("mfd: intel_soc_pmic_bxtwc: Use chained IRQs for second level IRQ chips")
> Reported-by: Zhang Ning <zhangn1985@outlook.com>
> Closes: https://lore.kernel.org/r/TY2PR01MB3322FEDCDC048B7D3794F922CDBA2@TY2PR01MB3322.jpnprd01.prod.outlook.com
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks, patch looks good to me:

Acked-by: Hans de Goede <hdegoede@redhat.com>

Please feel free to merge this through the MFD tree as suggested in
the cover-letter.

Regards,

Hans


> ---
>  drivers/mfd/intel_soc_pmic_bxtwc.c     | 31 ++++++++++++++------------
>  drivers/platform/x86/intel/bxtwc_tmu.c | 22 +++++-------------
>  2 files changed, 23 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/mfd/intel_soc_pmic_bxtwc.c b/drivers/mfd/intel_soc_pmic_bxtwc.c
> index d72995a9e820..628108dcf545 100644
> --- a/drivers/mfd/intel_soc_pmic_bxtwc.c
> +++ b/drivers/mfd/intel_soc_pmic_bxtwc.c
> @@ -245,12 +245,6 @@ static struct mfd_cell bxt_wc_dev[] = {
>  		.num_resources = ARRAY_SIZE(bcu_resources),
>  		.resources = bcu_resources,
>  	},
> -	{
> -		.name = "bxt_wcove_tmu",
> -		.num_resources = ARRAY_SIZE(tmu_resources),
> -		.resources = tmu_resources,
> -	},
> -
>  	{
>  		.name = "bxt_wcove_gpio",
>  		.num_resources = ARRAY_SIZE(gpio_resources),
> @@ -261,6 +255,14 @@ static struct mfd_cell bxt_wc_dev[] = {
>  	},
>  };
>  
> +static const struct mfd_cell bxt_wc_tmu_dev[] = {
> +	{
> +		.name = "bxt_wcove_tmu",
> +		.num_resources = ARRAY_SIZE(tmu_resources),
> +		.resources = tmu_resources,
> +	},
> +};
> +
>  static struct mfd_cell bxt_wc_chgr_dev[] = {
>  	{
>  		.name = "bxt_wcove_usbc",
> @@ -489,6 +491,15 @@ static int bxtwc_probe(struct platform_device *pdev)
>  	if (ret)
>  		return dev_err_probe(dev, ret, "Failed to add IRQ chip\n");
>  
> +	ret = bxtwc_add_chained_devices(pmic, bxt_wc_tmu_dev, ARRAY_SIZE(bxt_wc_tmu_dev),
> +					pmic->irq_chip_data,
> +					BXTWC_TMU_LVL1_IRQ,
> +					IRQF_ONESHOT,
> +					&bxtwc_regmap_irq_chip_tmu,
> +					&pmic->irq_chip_data_tmu);
> +	if (ret)
> +		return ret;
> +
>  	ret = bxtwc_add_chained_irq_chip(pmic, pmic->irq_chip_data,
>  					 BXTWC_PWRBTN_LVL1_IRQ,
>  					 IRQF_ONESHOT,
> @@ -497,14 +508,6 @@ static int bxtwc_probe(struct platform_device *pdev)
>  	if (ret)
>  		return dev_err_probe(dev, ret, "Failed to add PWRBTN IRQ chip\n");
>  
> -	ret = bxtwc_add_chained_irq_chip(pmic, pmic->irq_chip_data,
> -					 BXTWC_TMU_LVL1_IRQ,
> -					 IRQF_ONESHOT,
> -					 &bxtwc_regmap_irq_chip_tmu,
> -					 &pmic->irq_chip_data_tmu);
> -	if (ret)
> -		return dev_err_probe(dev, ret, "Failed to add TMU IRQ chip\n");
> -
>  	/* Add chained IRQ handler for BCU IRQs */
>  	ret = bxtwc_add_chained_irq_chip(pmic, pmic->irq_chip_data,
>  					 BXTWC_BCU_LVL1_IRQ,
> diff --git a/drivers/platform/x86/intel/bxtwc_tmu.c b/drivers/platform/x86/intel/bxtwc_tmu.c
> index d0e2a3c293b0..9ac801b929b9 100644
> --- a/drivers/platform/x86/intel/bxtwc_tmu.c
> +++ b/drivers/platform/x86/intel/bxtwc_tmu.c
> @@ -48,9 +48,8 @@ static irqreturn_t bxt_wcove_tmu_irq_handler(int irq, void *data)
>  static int bxt_wcove_tmu_probe(struct platform_device *pdev)
>  {
>  	struct intel_soc_pmic *pmic = dev_get_drvdata(pdev->dev.parent);
> -	struct regmap_irq_chip_data *regmap_irq_chip;
>  	struct wcove_tmu *wctmu;
> -	int ret, virq, irq;
> +	int ret;
>  
>  	wctmu = devm_kzalloc(&pdev->dev, sizeof(*wctmu), GFP_KERNEL);
>  	if (!wctmu)
> @@ -59,27 +58,18 @@ static int bxt_wcove_tmu_probe(struct platform_device *pdev)
>  	wctmu->dev = &pdev->dev;
>  	wctmu->regmap = pmic->regmap;
>  
> -	irq = platform_get_irq(pdev, 0);
> -	if (irq < 0)
> -		return irq;
> +	wctmu->irq = platform_get_irq(pdev, 0);
> +	if (wctmu->irq < 0)
> +		return wctmu->irq;
>  
> -	regmap_irq_chip = pmic->irq_chip_data_tmu;
> -	virq = regmap_irq_get_virq(regmap_irq_chip, irq);
> -	if (virq < 0) {
> -		dev_err(&pdev->dev,
> -			"failed to get virtual interrupt=%d\n", irq);
> -		return virq;
> -	}
> -
> -	ret = devm_request_threaded_irq(&pdev->dev, virq,
> +	ret = devm_request_threaded_irq(&pdev->dev, wctmu->irq,
>  					NULL, bxt_wcove_tmu_irq_handler,
>  					IRQF_ONESHOT, "bxt_wcove_tmu", wctmu);
>  	if (ret) {
>  		dev_err(&pdev->dev, "request irq failed: %d,virq: %d\n",
> -							ret, virq);
> +			ret, wctmu->irq);
>  		return ret;
>  	}
> -	wctmu->irq = virq;
>  
>  	/* Unmask TMU second level Wake & System alarm */
>  	regmap_update_bits(wctmu->regmap, BXTWC_MTMUIRQ_REG,