mfd: da9052-core: Use regmap_irq_get_virq() and fix the probe

PATCH In review Fabio Estevam 4 participants 15 messages Sep 28, 2012 – Oct 3, 2012

Subsystems (2)

Multifunction devices (MFD) Dialog semiconductor drivers

lore

Share & tools

lore

Fabio Estevam

author

2012-09-28 01:57 UTC #0 lore raw

PATCH v2 1/1 mfd: da9052-core: Use regmap_irq_get_virq() and fix the probe

From: Fabio Estevam <[email protected]>

On a mx53qsb dt-kernel the da9052-core driver fails to probe as follows:

da9052 1-0048: DA9052 ADC IRQ failed ret=-22

The reason for the error was due to passing only the offset as the interrupt number in request_threaded_irq().

The recommended approach though is to use regmap_get_virq() to acquire the interrupt number.

Fix it and allow the driver to probe successfully.

Also provide a few more error logs and change the irq string to "adc-irq", so that it appears as a single word in 'cat /proc/interrupts'.

Signed-off-by Fabio Estevam <[email protected]>

---
Changes since v2:
- Use regmap_irq_get_virq() instead of relying on irq_base

Arnd/Mark,

I also plan to convert the other da9052 drivers to use regmap_irq_get_virq().

 drivers/mfd/da9052-core.c         |   31 ++++++++++++++++++-------------
 include/linux/mfd/da9052/da9052.h |    1 +
 2 files changed, 19 insertions(+), 13 deletions(-)

drivers/mfd/da9052-core.c +18 ~~-13~~

index a0a62b2..79f9674 100644
@@ -782,35 +782,40 @@ int __devinit da9052_device_init(struct da9052 *da9052, u8 chip_id)
 
 	da9052->chip_id = chip_id;
 
-	if (!pdata || !pdata->irq_base)
-		da9052->irq_base = -1;
-	else
-		da9052->irq_base = pdata->irq_base;
-
 	ret = regmap_add_irq_chip(da9052->regmap, da9052->chip_irq,
 				  IRQF_TRIGGER_LOW | IRQF_ONESHOT,
-				  da9052->irq_base, &da9052_regmap_irq_chip,
+				  -1, &da9052_regmap_irq_chip,
 				  &da9052->irq_data);
-	if (ret < 0)
+	if (ret < 0) {
+		dev_err(da9052->dev, "regmap_add_irq_chip failed: %d\n", ret);
 		goto regmap_err;
+	}
 
-	da9052->irq_base = regmap_irq_chip_get_base(da9052->irq_data);
+	da9052->irq = regmap_irq_get_virq(da9052->irq_data, DA9052_IRQ_ADC_EOM);
 
-	ret = request_threaded_irq(DA9052_IRQ_ADC_EOM, NULL, da9052_auxadc_irq,
+	if (da9052->irq < 0) {
+		ret = da9052->irq;
+		dev_err(da9052->dev, "regmap_irq_get_virq failed: %d\n", ret);
+		goto regmap_err;
+	}
+
+	ret = request_threaded_irq(da9052->irq, NULL, da9052_auxadc_irq,
 				   IRQF_TRIGGER_LOW | IRQF_ONESHOT,
-				   "adc irq", da9052);
+				   "adc-irq", da9052);
 	if (ret != 0)
 		dev_err(da9052->dev, "DA9052 ADC IRQ failed ret=%d\n", ret);
 
 	ret = mfd_add_devices(da9052->dev, -1, da9052_subdev_info,
 			      ARRAY_SIZE(da9052_subdev_info), NULL, 0, NULL);
-	if (ret)
+	if (ret) {
+		dev_err(da9052->dev, "mfd_add_devices failed: %d\n", ret);
 		goto err;
+	}
 
 	return 0;
 
 err:
-	free_irq(DA9052_IRQ_ADC_EOM, da9052);
+	free_irq(da9052->irq, da9052);
 	mfd_remove_devices(da9052->dev);
 regmap_err:
 	return ret;

@@ -818,7 +823,7 @@ regmap_err:
 
 void da9052_device_exit(struct da9052 *da9052)
 {
-	free_irq(DA9052_IRQ_ADC_EOM, da9052);
+	free_irq(da9052->irq, da9052);
 	regmap_del_irq_chip(da9052->chip_irq, da9052->irq_data);
 	mfd_remove_devices(da9052->dev);
 }

include/linux/mfd/da9052/da9052.h +1 -0

index 0507c4c..f0b259d 100644
@@ -99,6 +99,7 @@ struct da9052 {
 	u8 chip_id;
 
 	int chip_irq;
+	int irq;
 };
 
 /* ADC API */

Signature · 1 line

1.7.9.5

Arnd Bergmann

2012-09-28 06:42 UTC #1 lore raw

Quoted 3 lines · Fabio Estevam

---
Changes since v2:
- Use regmap_irq_get_virq() instead of relying on irq_base

This looks good.

Quoted 1 line

I also plan to convert the other da9052 drivers to use regmap_irq_get_virq().

Ok, I think they have to be submitted together though, because otherwise the other drivers are more broken now.

Arnd

Fabio Estevam

author

2012-09-28 10:45 UTC #2 lore raw

Hi Arnd,

Quoted 6 lines · Arnd Bergmann

This looks good.
Quoted 1 line
I also plan to convert the other da9052 drivers to use regmap_irq_get_virq().
Ok, I think they have to be submitted together though, because otherwise the other drivers are more broken now.

Yes, makes sense. Will convert the others and submit the patches altogether.

Do you think I should also remove the "pdata" references, ie, let the driver be dt-only?

I do not see any users for the da9052 drivers, except for mx53qsb, which I added two days ago.

Regards,

Fabio Estevam

Arnd Bergmann

2012-09-28 11:34 UTC #3 lore raw

Quoted 9 lines · Fabio Estevam

Quoted 6 lines · Arnd Bergmann
This looks good.
Quoted 1 line
I also plan to convert the other da9052 drivers to use regmap_irq_get_virq().
Ok, I think they have to be submitted together though, because otherwise the other drivers are more broken now.

Quoted 8 lines

Yes, makes sense. Will convert the others and submit the patches altogether.
Do you think I should also remove the "pdata" references, ie, let the driver be dt-only?
I do not see any users for the da9052 drivers, except for mx53qsb, which I added two days ago.

Definitely fine with me.

Arnd

Mark Brown

2012-09-28 10:52 UTC #4 lore raw

In reply to Arnd Bergmann

Quoted 6 lines · Arnd Bergmann

Quoted 1 line · Fabio Estevam
I also plan to convert the other da9052 drivers to use regmap_irq_get_virq().
Ok, I think they have to be submitted together though, because otherwise the other drivers are more broken now.

Given that the drivers don't currently load I'm not sure it's worth the effort of arranging to get them all merged through one tree, probably easier to get as many fixes as possible merged as fast as possible.

Arnd Bergmann

2012-09-28 11:35 UTC #5 lore raw

Quoted 11 lines · Mark Brown

Quoted 6 lines · Arnd Bergmann
Quoted 1 line · Fabio Estevam
I also plan to convert the other da9052 drivers to use regmap_irq_get_virq().
Ok, I think they have to be submitted together though, because otherwise the other drivers are more broken now.
Given that the drivers don't currently load I'm not sure it's worth the effort of arranging to get them all merged through one tree, probably easier to get as many fixes as possible merged as fast as possible.

Agreed. I didn't know the driver still had problems of this scale.

Arnd

Mark Brown

2012-09-28 10:49 UTC #6 lore raw

In reply to Fabio Estevam

drivers/mfd/da9052-core.c int __devinit da9052_device_init(struct da9052 *da9052, u8 chip_id) · line 802

Quoted 11 lines · Fabio Estevam

drivers/mfd/da9052-core.c +8 -2

-	da9052->irq_base = regmap_irq_chip_get_base(da9052->irq_data);
+	da9052->irq = regmap_irq_get_virq(da9052->irq_data, DA9052_IRQ_ADC_EOM);
 
-	ret = request_threaded_irq(DA9052_IRQ_ADC_EOM, NULL, da9052_auxadc_irq,
+	if (da9052->irq < 0) {
+		ret = da9052->irq;
+		dev_err(da9052->dev, "regmap_irq_get_virq failed: %d\n", ret);
+		goto regmap_err;
+	}
+
+	ret = request_threaded_irq(da9052->irq, NULL, da9052_auxadc_irq,

This will fix the problem but the usage of da9052->irq here is very odd, normally that'd be the primary IRQ for the device but this is actually just the interrupt for the ADC.

Otherwise

Reviwed-by Mark Brown <[email protected]>

Fabio Estevam

2012-09-28 14:24 UTC #7 lore raw

Hi Mark,

Quoted 18 lines · Mark Brown

Review
Quoted 11 lines · Fabio Estevam
(quoted diff) +8 -2
-	da9052->irq_base = regmap_irq_chip_get_base(da9052->irq_data);
+	da9052->irq = regmap_irq_get_virq(da9052->irq_data, DA9052_IRQ_ADC_EOM);
 
-	ret = request_threaded_irq(DA9052_IRQ_ADC_EOM, NULL, da9052_auxadc_irq,
+	if (da9052->irq < 0) {
+		ret = da9052->irq;
+		dev_err(da9052->dev, "regmap_irq_get_virq failed: %d\n", ret);
+		goto regmap_err;
+	}
+
+	ret = request_threaded_irq(da9052->irq, NULL, da9052_auxadc_irq,
This will fix the problem but the usage of da9052->irq here is very odd, normally that'd be the primary IRQ for the device but this is actually just the interrupt for the ADC.

Quoted 4 lines

Otherwise
Reviwed-by Mark Brown <[email protected]>

Would you prefer the version below? If so, I can send it as v3:

drivers/mfd/da9052-core.c +19 ~~-13~~

index a0a62b2..4f74bf6 100644
@@ -782,35 +782,41 @@ int __devinit da9052_device_init(struct da9052 *da9052, u8 chip_id)
 
 	da9052->chip_id = chip_id;
 
-	if (!pdata || !pdata->irq_base)
-		da9052->irq_base = -1;
-	else
-		da9052->irq_base = pdata->irq_base;
-
 	ret = regmap_add_irq_chip(da9052->regmap, da9052->chip_irq,
 				  IRQF_TRIGGER_LOW | IRQF_ONESHOT,
-				  da9052->irq_base, &da9052_regmap_irq_chip,
+				  -1, &da9052_regmap_irq_chip,
 				  &da9052->irq_data);
-	if (ret < 0)
+	if (ret < 0) {
+		dev_err(da9052->dev, "regmap_add_irq_chip failed: %d\n", ret);
 		goto regmap_err;
+	}
 
-	da9052->irq_base = regmap_irq_chip_get_base(da9052->irq_data);
+	da9052->irq = regmap_irq_get_virq(da9052->irq_data, 0);
 
-	ret = request_threaded_irq(DA9052_IRQ_ADC_EOM, NULL, da9052_auxadc_irq,
+	if (da9052->irq < 0) {
+		ret = da9052->irq;
+		dev_err(da9052->dev, "regmap_irq_get_virq failed: %d\n", ret);
+		goto regmap_err;
+	}
+
+	ret = request_threaded_irq(da9052->irq + DA9052_IRQ_ADC_EOM, NULL, 
+				   da9052_auxadc_irq,
 				   IRQF_TRIGGER_LOW | IRQF_ONESHOT,
-				   "adc irq", da9052);
+				   "adc-irq", da9052);
 	if (ret != 0)
 		dev_err(da9052->dev, "DA9052 ADC IRQ failed ret=%d\n", ret);
 
 	ret = mfd_add_devices(da9052->dev, -1, da9052_subdev_info,
 			      ARRAY_SIZE(da9052_subdev_info), NULL, 0, NULL);
-	if (ret)
+	if (ret) {
+		dev_err(da9052->dev, "mfd_add_devices failed: %d\n", ret);
 		goto err;
+	}
 
 	return 0;
 
 err:
-	free_irq(DA9052_IRQ_ADC_EOM, da9052);
+	free_irq(da9052->irq + DA9052_IRQ_ADC_EOM, da9052);
 	mfd_remove_devices(da9052->dev);
 regmap_err:
 	return ret;

@@ -818,7 +824,7 @@ regmap_err:
 
 void da9052_device_exit(struct da9052 *da9052)
 {
-	free_irq(DA9052_IRQ_ADC_EOM, da9052);
+	free_irq(da9052->irq + DA9052_IRQ_ADC_EOM, da9052);
 	regmap_del_irq_chip(da9052->chip_irq, da9052->irq_data);
 	mfd_remove_devices(da9052->dev);
 }

include/linux/mfd/da9052/da9052.h +1 -0

index 0507c4c..f0b259d 100644
@@ -99,6 +99,7 @@ struct da9052 {
 	u8 chip_id;
 
 	int chip_irq;
+	int irq;
 };
 
 /* ADC API */

Mark Brown

2012-09-28 14:29 UTC #8 lore raw

drivers/mfd/da9052-core.c int __devinit da9052_device_init(struct da9052 *da9052, u8 chip_id) · line 794

Quoted 4 lines · Fabio Estevam

Would you prefer the version below? If so, I can send it as v3:
drivers/mfd/da9052-core.c +1 -1
-	da9052->irq_base = regmap_irq_chip_get_base(da9052->irq_data);
+	da9052->irq = regmap_irq_get_virq(da9052->irq_data, 0);

No, this doesn't support linear domains, if you're going to do this it'd be better to just leave it as irq_base though as discussed that's not ideal. When I've done these conversions I've just done the virq conversion at every use.

Fabio Estevam

author

2012-09-28 15:37 UTC #9 lore raw

Quoted 4 lines · Mark Brown

No, this doesn't support linear domains, if you're going to do this it'd be better to just leave it as irq_base though as discussed that's not ideal. When I've done these conversions I've just done the virq conversion at every use.

Ok, so if I understand you correctly you suggest that Samuel applies the v1 of the patch:
https://lkml.org/lkml/2012/9/26/551

Please confirm.

Regards,

Fabio Estevam

Mark Brown

2012-09-28 15:51 UTC #10 lore raw

Quoted 7 lines · Fabio Estevam

On Fri, Sep 28, 2012 at 11:29 AM, Mark Brown
Quoted 4 lines
No, this doesn't support linear domains, if you're going to do this it'd be better to just leave it as irq_base though as discussed that's not ideal. When I've done these conversions I've just done the virq conversion at every use.

Quoted 5 lines

Ok, so if I understand you correctly you suggest that Samuel applies the v1 of the patch:
https://lkml.org/lkml/2012/9/26/551
Please confirm.

Well, I'd really rather fix it to work with linear domains while we're at it. Right now it's mostly working by accident rather than design (and even for legacy domains it's doing the wrong thing since it doesn't pass the irq_base into mfd_add_devices). This is especially true if we're looking at making the device DT only in which case there's really no reason to use anything other than a linear domain.

But yes, of the changes posted thus far this is the best one.

Fabio Estevam

author

2012-09-28 20:34 UTC #11 lore raw

Quoted 2 lines · Mark Brown

Well, I'd really rather fix it to work with linear domains while we're at it. Right now it's mostly working by accident rather than design

Ok, does the conversion patch below look fine, please?

---
 drivers/mfd/da9052-core.c         |   92 ++++++++++++++++++++++++++++++-------
 include/linux/mfd/da9052/da9052.h |    3 +-
 2 files changed, 77 insertions(+), 18 deletions(-)

drivers/mfd/da9052-core.c +49 -1

index a0a62b2..c95e7d1 100644
@@ -16,6 +16,7 @@
 #include <linux/input.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
+#include <linux/irqdomain.h>
 #include <linux/mfd/core.h>
 #include <linux/slab.h>
 #include <linux/module.h>

@@ -364,6 +365,53 @@ static const int32_t tbat_lookup[255] = {
 	-11823, -11903, -11982
 };
 
+static void da9052_irq_enable(struct irq_data *data)
+{
+}
+
+static void da9052_irq_disable(struct irq_data *data)
+{
+}
+
+static struct irq_chip da9052_irq_chip = {
+	.name			= "da9052",
+	.irq_disable		= da9052_irq_disable,
+	.irq_enable		= da9052_irq_enable,
+};
+
+static int da9052_irq_map(struct irq_domain *h, unsigned int virq,
+			      irq_hw_number_t hw)
+{
+	struct regmap_irq_chip_data *data = h->host_data;
+
+	irq_set_chip_data(virq, data);
+	irq_set_chip_and_handler(virq, &da9052_irq_chip, handle_edge_irq);
+	irq_set_nested_thread(virq, 1);
+
+	/*
+	 * ARM needs us to explicitly flag the IRQ as valid
+	 * and will set them noprobe when we do so.
+	 */
+#ifdef CONFIG_ARM
+	set_irq_flags(virq, IRQF_VALID);
+#else
+	irq_set_noprobe(virq);
+#endif
+
+	return 0;
+}
+
+static struct irq_domain_ops da9052_domain_ops = {
+	.map	= da9052_irq_map,
+	.xlate	= irq_domain_xlate_twocell,
+};
+
+static int da9052_map_irq(struct da9052 *da9052, int irq)
+{
+	return regmap_irq_get_virq(da9052->irq_chip, irq);
+
+}
+
 static const u8 chan_mux[DA9052_ADC_VBBAT + 1] = {
 	[DA9052_ADC_VDDOUT]	= DA9052_ADC_MAN_MUXSEL_VDDOUT,
 	[DA9052_ADC_ICH]	= DA9052_ADC_MAN_MUXSEL_ICH,

@@ -772,7 +820,7 @@ EXPORT_SYMBOL_GPL(da9052_regmap_config);
 int __devinit da9052_device_init(struct da9052 *da9052, u8 chip_id)
 {
 	struct da9052_pdata *pdata = da9052->dev->platform_data;
-	int ret;
+	int ret, i;
 
 	mutex_init(&da9052->auxadc_lock);
 	init_completion(&da9052->done);

@@ -782,35 +830,44 @@ int __devinit da9052_device_init(struct da9052

*da9052, u8 chip_id)

da9052->chip_id = chip_id;

- if (!pdata || !pdata->irq_base)
- da9052->irq_base = -1;
- else
- da9052->irq_base = pdata->irq_base;
+ /* Allocate a virtual IRQ domain to distribute to the regmap domains */
+ da9052->virq = irq_domain_add_linear(NULL, ARRAY_SIZE(da9052_irqs),
+ &da9052_domain_ops, da9052);
+ if (!da9052->virq) {
+ ret = -EINVAL;
+ goto regmap_err;
+ }

- ret = regmap_add_irq_chip(da9052->regmap, da9052->chip_irq,
+ ret = regmap_add_irq_chip(da9052->regmap,
+ irq_create_mapping(da9052->virq, 1),
IRQF_TRIGGER_LOW | IRQF_ONESHOT,
- da9052->irq_base, &da9052_regmap_irq_chip,
- &da9052->irq_data);
- if (ret < 0)
+ -1, &da9052_regmap_irq_chip,
+ &da9052->irq_chip);
+ if (ret < 0) {
+ dev_err(da9052->dev, "regmap_add_irq_chip failed: %d\n", ret);
goto regmap_err;
+ }

- da9052->irq_base = regmap_irq_chip_get_base(da9052->irq_data);
-
- ret = request_threaded_irq(DA9052_IRQ_ADC_EOM, NULL, da9052_auxadc_irq,
+ i = da9052_map_irq(da9052, DA9052_IRQ_ADC_EOM);
+ ret = request_threaded_irq(i, NULL, da9052_auxadc_irq,
IRQF_TRIGGER_LOW | IRQF_ONESHOT,
- "adc irq", da9052);
+ "adc-irq", da9052);
if (ret != 0)
dev_err(da9052->dev, "DA9052 ADC IRQ failed ret=%d\n", ret);

ret = mfd_add_devices(da9052->dev, -1, da9052_subdev_info,
ARRAY_SIZE(da9052_subdev_info), NULL, 0, NULL);
- if (ret)
+ if (ret) {
+ dev_err(da9052->dev, "mfd_add_devices failed: %d\n", ret);
goto err;
+ }

return 0;

err:
- free_irq(DA9052_IRQ_ADC_EOM, da9052);
+ free_irq(da9052_map_irq(da9052, DA9052_IRQ_ADC_EOM), da9052);
+ regmap_del_irq_chip(irq_create_mapping(da9052->virq, 0),
+ da9052->irq_chip);
mfd_remove_devices(da9052->dev);
regmap_err:
return ret;
@@ -818,8 +875,9 @@ regmap_err:

void da9052_device_exit(struct da9052 *da9052)
{
- free_irq(DA9052_IRQ_ADC_EOM, da9052);
- regmap_del_irq_chip(da9052->chip_irq, da9052->irq_data);
+ free_irq(da9052_map_irq(da9052, DA9052_IRQ_ADC_EOM), da9052);
+ regmap_del_irq_chip(irq_create_mapping(da9052->virq, 0),
+ da9052->irq_chip);
mfd_remove_devices(da9052->dev);
}

(quoted diff) +0 -0

b/include/linux/mfd/da9052/da9052.h
index 0507c4c..24e450d 100644

include/linux/mfd/da9052/da9052.h +2 -1

@@ -95,10 +95,11 @@ struct da9052 {
 	struct completion done;
 
 	int irq_base;
-	struct regmap_irq_chip_data *irq_data;
+	struct regmap_irq_chip_data *irq_chip;
 	u8 chip_id;
 
 	int chip_irq;
+	struct irq_domain *virq;	
 };
 
 /* ADC API */

Signature · 1 line

1.7.9.5

Mark Brown

2012-10-01 10:22 UTC #12 lore raw

drivers/mfd/da9052-core.c static const int32_t tbat_lookup[255] = { · line 380

Quoted 5 lines · Fabio Estevam

drivers/mfd/da9052-core.c +5 -0

+static struct irq_chip da9052_irq_chip = {
+	.name			= "da9052",
+	.irq_disable		= da9052_irq_disable,
+	.irq_enable		= da9052_irq_enable,
+};

I don't understand what this irq_chip or the custom domain you're adding are for?

Fabio Estevam

author

2012-10-03 16:04 UTC #13 lore raw

Quoted 10 lines · Mark Brown

drivers/mfd/da9052-core.c static const int32_t tbat_lookup[255] = { · line 380
Quoted 5 lines · Fabio Estevam
drivers/mfd/da9052-core.c +5 -0
+static struct irq_chip da9052_irq_chip = {
+     .name                   = "da9052",
+     .irq_disable            = da9052_irq_disable,
+     .irq_enable             = da9052_irq_enable,
+};
I don't understand what this irq_chip or the custom domain you're adding are for?

I declared da9052_irq_chip because I do the following:

irq_set_chip_and_handler(virq, &da9052_irq_chip, handle_level_irq);

I am following the arizona-irq approach.

If this is not suitable, can you please point me to some driver that does not define irq_chip and can still do setup "irq_set_chip_and_handler()"?

Thanks,

Fabio Estevam

Mark Brown

2012-10-03 18:08 UTC #14 lore raw

Quoted 7 lines · Fabio Estevam

On Mon, Oct 1, 2012 at 7:22 AM, Mark Brown
Quoted 2 lines
I don't understand what this irq_chip or the custom domain you're adding are for?
I declared da9052_irq_chip because I do the following:

Quoted 7 lines

irq_set_chip_and_handler(virq, &da9052_irq_chip, handle_level_irq);
I am following the arizona-irq approach.
If this is not suitable, can you please point me to some driver that does not define irq_chip and can still do setup "irq_set_chip_and_handler()"?

Any other regmap-irq user. The reason the Arizona driver is doing this is that it has two interrupt controllers in it, one for the main chip and one for the always on domain, which is why it registers two regmap-irq domains. Notice that there's only two subinterrupts in the primary domain.