Re: [PATCH 4/6] mfd: max77759: modify irq configs
From: Amit Sunil Dhamne
Date: Tue Nov 25 2025 - 20:10:18 EST
Hi André,
On 11/23/25 10:21 PM, André Draszik wrote:
Hi Amit,
Thanks for your patches to enable the charger!
Ack!
From: Amit Sunil Dhamne <amitsd@xxxxxxxxxx>
Define specific bit-level masks for charger's registers and modify the
irq mask for charger irq_chip. Also, configure the max77759 interrupt
lines as active low to all interrupt registrations to ensure the
interrupt controllers are configured with the correct trigger type.
Signed-off-by: Amit Sunil Dhamne <amitsd@xxxxxxxxxx>
---
drivers/mfd/max77759.c | 24 +++++++++++++++++-------
include/linux/mfd/max77759.h | 9 +++++++++
2 files changed, 26 insertions(+), 7 deletions(-)
diff --git a/drivers/mfd/max77759.c b/drivers/mfd/max77759.c
index 6cf6306c4a3b..5fe22884f362 100644
--- a/drivers/mfd/max77759.c
+++ b/drivers/mfd/max77759.c
@@ -256,8 +256,17 @@ static const struct regmap_irq max77759_topsys_irqs[] = {
};
static const struct regmap_irq max77759_chgr_irqs[] = {
- REGMAP_IRQ_REG(MAX77759_CHARGER_INT_1, 0, GENMASK(7, 0)),
- REGMAP_IRQ_REG(MAX77759_CHARGER_INT_2, 1, GENMASK(7, 0)),
+ REGMAP_IRQ_REG(MAX77759_CHARGER_INT_1, 0,
+ MAX77759_CHGR_REG_CHG_INT_AICL |
+ MAX77759_CHGR_REG_CHG_INT_CHGIN |
+ MAX77759_CHGR_REG_CHG_INT_CHG |
+ MAX77759_CHGR_REG_CHG_INT_INLIM),
+ REGMAP_IRQ_REG(MAX77759_CHARGER_INT_2, 1,
+ MAX77759_CHGR_REG_CHG_INT2_BAT_OILO |
+ MAX77759_CHGR_REG_CHG_INT2_CHG_STA_CC |
+ MAX77759_CHGR_REG_CHG_INT2_CHG_STA_CV |
+ MAX77759_CHGR_REG_CHG_INT2_CHG_STA_TO |
+ MAX77759_CHGR_REG_CHG_INT2_CHG_STA_DONE),
};
static const struct regmap_irq_chip max77759_pmic_irq_chip = {
@@ -486,8 +495,8 @@ static int max77759_add_chained_irq_chip(struct device *dev,
"failed to get parent vIRQ(%d) for chip %s\n",
pirq, chip->name);
- ret = devm_regmap_add_irq_chip(dev, regmap, irq,
- IRQF_ONESHOT | IRQF_SHARED, 0, chip,
+ ret = devm_regmap_add_irq_chip(dev, regmap, irq, IRQF_ONESHOT |
+ IRQF_SHARED | IRQF_TRIGGER_LOW, 0, chip,
data);
Please correct me if I'm wrong, but I don't think this makes sense for a
chained IRQ in this case. What problem does this change fix?
While this patch was meant only for modifying the interrupt mask, I
added this for consistency because the main SoC -> PMIC line is active
low. However, I agree it is not strictly necessary here. I will drop it
in the next revision.
if (ret)
return dev_err_probe(dev, ret, "failed to add %s IRQ chip\n",
@@ -519,8 +528,9 @@ static int max77759_add_chained_maxq(struct i2c_client *client,
ret = devm_request_threaded_irq(&client->dev, apcmdres_irq,
NULL, apcmdres_irq_handler,
- IRQF_ONESHOT | IRQF_SHARED,
- dev_name(&client->dev), max77759);
+ IRQF_ONESHOT | IRQF_SHARED |
+ IRQF_TRIGGER_LOW, dev_name(&client->dev),
+ max77759);
dito.
Agreed, will drop.
if (ret)
return dev_err_probe(&client->dev, ret,
"MAX77759_MAXQ_INT_APCMDRESI failed\n");
@@ -633,7 +643,7 @@ static int max77759_probe(struct i2c_client *client)
return dev_err_probe(&client->dev, -EINVAL,
"invalid IRQ: %d\n", client->irq);
- irq_flags = IRQF_ONESHOT | IRQF_SHARED;
+ irq_flags = IRQF_ONESHOT | IRQF_SHARED | IRQF_TRIGGER_LOW;
I don't believe IRQF_TRIGGER_LOW should be added here, as this is board-specific.
The polarity is meant to be set via DT (and the only current user of this driver
does so).
You are correct. Since I am already retrieving the trigger type from DT
via irqd_get_trigger_type() below, I will drop this change.
irq_flags |= irqd_get_trigger_type(irq_data);
That's what gets us the config from DT.
ret = devm_regmap_add_irq_chip(&client->dev, max77759->regmap_top,
diff --git a/include/linux/mfd/max77759.h b/include/linux/mfd/max77759.h
index c6face34e385..0ef29a48deec 100644
--- a/include/linux/mfd/max77759.h
+++ b/include/linux/mfd/max77759.h
@@ -62,7 +62,16 @@
#define MAX77759_CHGR_REG_CHG_INT 0xb0
#define MAX77759_CHGR_REG_CHG_INT2 0xb1
#define MAX77759_CHGR_REG_CHG_INT_MASK 0xb2
+#define MAX77759_CHGR_REG_CHG_INT_AICL BIT(7)
+#define MAX77759_CHGR_REG_CHG_INT_CHGIN BIT(6)
+#define MAX77759_CHGR_REG_CHG_INT_CHG BIT(4)
+#define MAX77759_CHGR_REG_CHG_INT_INLIM BIT(2)
#define MAX77759_CHGR_REG_CHG_INT2_MASK 0xb3
+#define MAX77759_CHGR_REG_CHG_INT2_BAT_OILO BIT(4)
+#define MAX77759_CHGR_REG_CHG_INT2_CHG_STA_CC BIT(3)
+#define MAX77759_CHGR_REG_CHG_INT2_CHG_STA_CV BIT(2)
+#define MAX77759_CHGR_REG_CHG_INT2_CHG_STA_TO BIT(1)
+#define MAX77759_CHGR_REG_CHG_INT2_CHG_STA_DONE BIT(0)
Even if wireless out of scope, it'd still be nice to add macros for
the remaining bits to make this complete and avoid having to update
these again in case wireless support gets added in the future.
I would prefer to only define the macros I am currently using to keep
the review focused, unless you consider this a strict requirement.
Also, would be nice to keep existing style and indent the bits from
the registers (see existing bit definitions in this file a few lines
further up).
Finally, can you add the bits below the respective register (0xb0 / 0xb1)
please, to keep suffix meaningful, and to follow existing style for cases
like this (see MAX77759_MAXQ_REG_UIC_INT1).
I will fix the indentation and ordering in the next revision.
Regarding bit definitions: In [PATCH 5/6], the max77759_charger.c driver
defines bits for the register addresses defined in this file. Currently,
those macros are only used locally by the max77759 charger driver. Would
you prefer I move those definitions to this header file as well?
BR,
Amit
Cheers,
Andre'