| Message ID | 20241003174252.1190628-4-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | mfd: intel_soc_pmic_bxtwc: Fix IRQ domain usage | expand |
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,
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>
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,
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> --- drivers/mfd/intel_soc_pmic_bxtwc.c | 31 ++++++++++++++------------ drivers/platform/x86/intel/bxtwc_tmu.c | 22 +++++------------- 2 files changed, 23 insertions(+), 30 deletions(-)