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

["张 宁" <zhangn1985@outlook.com>]

Hi, 

I have tested patch 1,2,3 on debian sid with kernel 6.11.1

for these 3 patches: 
Tested-by: Zhang Ning <zhangn1985@outlook.com>


________________________________________
发件人: Hans de Goede <hdegoede@redhat.com>
发送时间: 2024年10月4日 2:02
收件人: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; platform-driver-x86@vger.kernel.org <platform-driver-x86@vger.kernel.org>; linux-usb@vger.kernel.org <linux-usb@vger.kernel.org>
抄送: Andy Shevchenko <andy@kernel.org>; Lee Jones <lee@kernel.org>; Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>; Heikki Krogerus <heikki.krogerus@linux.intel.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Zhang Ning <zhangn1985@outlook.com>
主题: Re: [PATCH v1 2/4] mfd: intel_soc_pmic_bxtwc: Use IRQ domain for TMU device
 
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,