[PATCH v1 0/4] mfd: intel_soc_pmic_bxtwc: Fix IRQ domain usage

[PATCH v1 0/4] mfd: intel_soc_pmic_bxtwc: Fix IRQ domain usage

[Andy Shevchenko]

It appears that the driver has been developed without proper thinking
of what the difference between HW IRQ and Linux IRQ (also known as vIRQ).
This misunderstanding led to the 0 being used as vIRQ which is no-no and
platform APIs unveal this after the commit a85a6c86c25b ("driver core:
platform: Clarify that IRQ 0 is invalid"). With this the Intel Broxton
Whiskey Cove PMIC driver has to be fixed all over the places.

In this series two immediate fixes as reported recently and we have
drivers for (patches 1 & 2) followed by the patch that prepares the
same fix for the devices that have no in-tree drivers yet. At the end
it is a fix for IRQ domain debugging (when enabled), but it requires
quite recent patches in IRQ domain and regmap IRQ subsystems, which is
reflected in the Depends-on tag (I can't use Fixes, as this doesn't
fix anything related to that).

The series touches a couple of subsystems as the respective drivers
has to be fixed simultaneously. The idea is to proceed the series via
MFD tree to where the main driver belongs.

I also hope that Zhang will be able to test it soon and provide his
Tested-by. I have tested this on Intel Joule SoM (Apollo Lake SoC).

Andy Shevchenko (4):
  mfd: intel_soc_pmic_bxtwc: Use IRQ domain for USB Type-C device
  mfd: intel_soc_pmic_bxtwc: Use IRQ domain for TMU device
  mfd: intel_soc_pmic_bxtwc: Use IRQ domain for PMIC devices
  mfd: intel_soc_pmic_bxtwc: Fix IRQ domain names duplication

 drivers/mfd/intel_soc_pmic_bxtwc.c     | 144 ++++++++++++++++---------
 drivers/platform/x86/intel/bxtwc_tmu.c |  22 ++--
 drivers/usb/typec/tcpm/wcove.c         |   4 -
 3 files changed, 97 insertions(+), 73 deletions(-)

-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 1/4] mfd: intel_soc_pmic_bxtwc: Use IRQ domain for USB Type-C device

[Andy Shevchenko]

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: 9c6235c86332 ("mfd: intel_soc_pmic_bxtwc: Add bxt_wcove_usbc device")
Fixes: d2061f9cc32d ("usb: typec: add driver for Intel Whiskey Cove PMIC USB Type-C PHY")
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 | 57 +++++++++++++++++++++---------
 drivers/usb/typec/tcpm/wcove.c     |  4 ---
 2 files changed, 40 insertions(+), 21 deletions(-)

diff --git a/drivers/mfd/intel_soc_pmic_bxtwc.c b/drivers/mfd/intel_soc_pmic_bxtwc.c
index ccd76800d8e4..d72995a9e820 100644
--- a/drivers/mfd/intel_soc_pmic_bxtwc.c
+++ b/drivers/mfd/intel_soc_pmic_bxtwc.c
@@ -240,16 +240,6 @@ static struct mfd_cell bxt_wc_dev[] = {
 		.num_resources = ARRAY_SIZE(thermal_resources),
 		.resources = thermal_resources,
 	},
-	{
-		.name = "bxt_wcove_usbc",
-		.num_resources = ARRAY_SIZE(usbc_resources),
-		.resources = usbc_resources,
-	},
-	{
-		.name = "bxt_wcove_ext_charger",
-		.num_resources = ARRAY_SIZE(charger_resources),
-		.resources = charger_resources,
-	},
 	{
 		.name = "bxt_wcove_bcu",
 		.num_resources = ARRAY_SIZE(bcu_resources),
@@ -271,6 +261,19 @@ static struct mfd_cell bxt_wc_dev[] = {
 	},
 };
 
+static struct mfd_cell bxt_wc_chgr_dev[] = {
+	{
+		.name = "bxt_wcove_usbc",
+		.num_resources = ARRAY_SIZE(usbc_resources),
+		.resources = usbc_resources,
+	},
+	{
+		.name = "bxt_wcove_ext_charger",
+		.num_resources = ARRAY_SIZE(charger_resources),
+		.resources = charger_resources,
+	},
+};
+
 static int regmap_ipc_byte_reg_read(void *context, unsigned int reg,
 				    unsigned int *val)
 {
@@ -425,6 +428,26 @@ static int bxtwc_add_chained_irq_chip(struct intel_soc_pmic *pmic,
 					0, chip, data);
 }
 
+static int bxtwc_add_chained_devices(struct intel_soc_pmic *pmic,
+				     const struct mfd_cell *cells, int n_devs,
+				     struct regmap_irq_chip_data *pdata,
+				     int pirq, int irq_flags,
+				     const struct regmap_irq_chip *chip,
+				     struct regmap_irq_chip_data **data)
+{
+	struct device *dev = pmic->dev;
+	struct irq_domain *domain;
+	int ret;
+
+	ret = bxtwc_add_chained_irq_chip(pmic, pdata, pirq, irq_flags, chip, data);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to add %s IRQ chip\n", chip->name);
+
+	domain = regmap_irq_get_domain(*data);
+
+	return devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, cells, n_devs, NULL, 0, domain);
+}
+
 static int bxtwc_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -500,14 +523,14 @@ static int bxtwc_probe(struct platform_device *pdev)
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to add ADC IRQ chip\n");
 
-	/* Add chained IRQ handler for CHGR IRQs */
-	ret = bxtwc_add_chained_irq_chip(pmic, pmic->irq_chip_data,
-					 BXTWC_CHGR_LVL1_IRQ,
-					 IRQF_ONESHOT,
-					 &bxtwc_regmap_irq_chip_chgr,
-					 &pmic->irq_chip_data_chgr);
+	ret = bxtwc_add_chained_devices(pmic, bxt_wc_chgr_dev, ARRAY_SIZE(bxt_wc_chgr_dev),
+					pmic->irq_chip_data,
+					BXTWC_CHGR_LVL1_IRQ,
+					IRQF_ONESHOT,
+					&bxtwc_regmap_irq_chip_chgr,
+					&pmic->irq_chip_data_chgr);
 	if (ret)
-		return dev_err_probe(dev, ret, "Failed to add CHGR IRQ chip\n");
+		return ret;
 
 	/* Add chained IRQ handler for CRIT IRQs */
 	ret = bxtwc_add_chained_irq_chip(pmic, pmic->irq_chip_data,
diff --git a/drivers/usb/typec/tcpm/wcove.c b/drivers/usb/typec/tcpm/wcove.c
index cf719307b3f6..60b2766a69bf 100644
--- a/drivers/usb/typec/tcpm/wcove.c
+++ b/drivers/usb/typec/tcpm/wcove.c
@@ -621,10 +621,6 @@ static int wcove_typec_probe(struct platform_device *pdev)
 	if (irq < 0)
 		return irq;
 
-	irq = regmap_irq_get_virq(pmic->irq_chip_data_chgr, irq);
-	if (irq < 0)
-		return irq;
-
 	ret = guid_parse(WCOVE_DSM_UUID, &wcove->guid);
 	if (ret)
 		return ret;
-- 
2.43.0.rc1.1336.g36b5255a03ac

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

[Andy Shevchenko]

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(-)

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,
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 3/4] mfd: intel_soc_pmic_bxtwc: Use IRQ domain for PMIC devices

[Andy Shevchenko]

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: 57129044f504 ("mfd: intel_soc_pmic_bxtwc: Use chained IRQs for second level IRQ chips")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/mfd/intel_soc_pmic_bxtwc.c | 54 +++++++++++++++++-------------
 1 file changed, 30 insertions(+), 24 deletions(-)

diff --git a/drivers/mfd/intel_soc_pmic_bxtwc.c b/drivers/mfd/intel_soc_pmic_bxtwc.c
index 628108dcf545..fefbeb4164fd 100644
--- a/drivers/mfd/intel_soc_pmic_bxtwc.c
+++ b/drivers/mfd/intel_soc_pmic_bxtwc.c
@@ -230,21 +230,11 @@ static const struct resource tmu_resources[] = {
 };
 
 static struct mfd_cell bxt_wc_dev[] = {
-	{
-		.name = "bxt_wcove_gpadc",
-		.num_resources = ARRAY_SIZE(adc_resources),
-		.resources = adc_resources,
-	},
 	{
 		.name = "bxt_wcove_thermal",
 		.num_resources = ARRAY_SIZE(thermal_resources),
 		.resources = thermal_resources,
 	},
-	{
-		.name = "bxt_wcove_bcu",
-		.num_resources = ARRAY_SIZE(bcu_resources),
-		.resources = bcu_resources,
-	},
 	{
 		.name = "bxt_wcove_gpio",
 		.num_resources = ARRAY_SIZE(gpio_resources),
@@ -263,6 +253,22 @@ static const struct mfd_cell bxt_wc_tmu_dev[] = {
 	},
 };
 
+static const struct mfd_cell bxt_wc_bcu_dev[] = {
+	{
+		.name = "bxt_wcove_bcu",
+		.num_resources = ARRAY_SIZE(bcu_resources),
+		.resources = bcu_resources,
+	},
+};
+
+static const struct mfd_cell bxt_wc_adc_dev[] = {
+	{
+		.name = "bxt_wcove_gpadc",
+		.num_resources = ARRAY_SIZE(adc_resources),
+		.resources = adc_resources,
+	},
+};
+
 static struct mfd_cell bxt_wc_chgr_dev[] = {
 	{
 		.name = "bxt_wcove_usbc",
@@ -508,23 +514,23 @@ static int bxtwc_probe(struct platform_device *pdev)
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to add PWRBTN 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,
-					 IRQF_ONESHOT,
-					 &bxtwc_regmap_irq_chip_bcu,
-					 &pmic->irq_chip_data_bcu);
+	ret = bxtwc_add_chained_devices(pmic, bxt_wc_bcu_dev, ARRAY_SIZE(bxt_wc_bcu_dev),
+					pmic->irq_chip_data,
+					BXTWC_BCU_LVL1_IRQ,
+					IRQF_ONESHOT,
+					&bxtwc_regmap_irq_chip_bcu,
+					&pmic->irq_chip_data_bcu);
 	if (ret)
-		return dev_err_probe(dev, ret, "Failed to add BUC IRQ chip\n");
+		return ret;
 
-	/* Add chained IRQ handler for ADC IRQs */
-	ret = bxtwc_add_chained_irq_chip(pmic, pmic->irq_chip_data,
-					 BXTWC_ADC_LVL1_IRQ,
-					 IRQF_ONESHOT,
-					 &bxtwc_regmap_irq_chip_adc,
-					 &pmic->irq_chip_data_adc);
+	ret = bxtwc_add_chained_devices(pmic, bxt_wc_adc_dev, ARRAY_SIZE(bxt_wc_adc_dev),
+					pmic->irq_chip_data,
+					BXTWC_ADC_LVL1_IRQ,
+					IRQF_ONESHOT,
+					&bxtwc_regmap_irq_chip_adc,
+					&pmic->irq_chip_data_adc);
 	if (ret)
-		return dev_err_probe(dev, ret, "Failed to add ADC IRQ chip\n");
+		return ret;
 
 	ret = bxtwc_add_chained_devices(pmic, bxt_wc_chgr_dev, ARRAY_SIZE(bxt_wc_chgr_dev),
 					pmic->irq_chip_data,
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 4/4] mfd: intel_soc_pmic_bxtwc: Fix IRQ domain names duplication

[Andy Shevchenko]

For all of the devices regmap IRQ may try to created the folder
with the same name which is impossible and fails with:

  debugfs: File '\_SB.IPC1.PMIC' in directory 'domains' already present!

Add domain_suffix to all of the IRQ chips driver registers to solve
the issue.

Fixes: 39d047c0b1c8 ("mfd: add Intel Broxton Whiskey Cove PMIC driver")
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")
Depends-on: dde286ee5770 ("regmap: Allow setting IRQ domain name suffix")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/mfd/intel_soc_pmic_bxtwc.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/mfd/intel_soc_pmic_bxtwc.c b/drivers/mfd/intel_soc_pmic_bxtwc.c
index fefbeb4164fd..b7204072e93e 100644
--- a/drivers/mfd/intel_soc_pmic_bxtwc.c
+++ b/drivers/mfd/intel_soc_pmic_bxtwc.c
@@ -148,6 +148,7 @@ static const struct regmap_irq_chip bxtwc_regmap_irq_chip = {
 
 static const struct regmap_irq_chip bxtwc_regmap_irq_chip_pwrbtn = {
 	.name = "bxtwc_irq_chip_pwrbtn",
+	.domain_suffix = "PWRBTN",
 	.status_base = BXTWC_PWRBTNIRQ,
 	.mask_base = BXTWC_MPWRBTNIRQ,
 	.irqs = bxtwc_regmap_irqs_pwrbtn,
@@ -157,6 +158,7 @@ static const struct regmap_irq_chip bxtwc_regmap_irq_chip_pwrbtn = {
 
 static const struct regmap_irq_chip bxtwc_regmap_irq_chip_tmu = {
 	.name = "bxtwc_irq_chip_tmu",
+	.domain_suffix = "TMU",
 	.status_base = BXTWC_TMUIRQ,
 	.mask_base = BXTWC_MTMUIRQ,
 	.irqs = bxtwc_regmap_irqs_tmu,
@@ -166,6 +168,7 @@ static const struct regmap_irq_chip bxtwc_regmap_irq_chip_tmu = {
 
 static const struct regmap_irq_chip bxtwc_regmap_irq_chip_bcu = {
 	.name = "bxtwc_irq_chip_bcu",
+	.domain_suffix = "BCU",
 	.status_base = BXTWC_BCUIRQ,
 	.mask_base = BXTWC_MBCUIRQ,
 	.irqs = bxtwc_regmap_irqs_bcu,
@@ -175,6 +178,7 @@ static const struct regmap_irq_chip bxtwc_regmap_irq_chip_bcu = {
 
 static const struct regmap_irq_chip bxtwc_regmap_irq_chip_adc = {
 	.name = "bxtwc_irq_chip_adc",
+	.domain_suffix = "ADC",
 	.status_base = BXTWC_ADCIRQ,
 	.mask_base = BXTWC_MADCIRQ,
 	.irqs = bxtwc_regmap_irqs_adc,
@@ -184,6 +188,7 @@ static const struct regmap_irq_chip bxtwc_regmap_irq_chip_adc = {
 
 static const struct regmap_irq_chip bxtwc_regmap_irq_chip_chgr = {
 	.name = "bxtwc_irq_chip_chgr",
+	.domain_suffix = "CHGR",
 	.status_base = BXTWC_CHGR0IRQ,
 	.mask_base = BXTWC_MCHGR0IRQ,
 	.irqs = bxtwc_regmap_irqs_chgr,
@@ -193,6 +198,7 @@ static const struct regmap_irq_chip bxtwc_regmap_irq_chip_chgr = {
 
 static const struct regmap_irq_chip bxtwc_regmap_irq_chip_crit = {
 	.name = "bxtwc_irq_chip_crit",
+	.domain_suffix = "CRIT",
 	.status_base = BXTWC_CRITIRQ,
 	.mask_base = BXTWC_MCRITIRQ,
 	.irqs = bxtwc_regmap_irqs_crit,
-- 
2.43.0.rc1.1336.g36b5255a03ac

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

[Hans de Goede]

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,

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

[张 宁]

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,