From: Amit Sunil Dhamne <amitsd@google.com>
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@google.com>
---
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);
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);
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;
irq_flags |= irqd_get_trigger_type(irq_data);
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)
#define MAX77759_CHGR_REG_CHG_INT_OK 0xb4
#define MAX77759_CHGR_REG_CHG_DETAILS_00 0xb5
#define MAX77759_CHGR_REG_CHG_DETAILS_01 0xb6
--
2.52.0.rc2.455.g230fcf2819-googOn 23/11/2025 09:35, Amit Sunil Dhamne via B4 Relay wrote: > From: Amit Sunil Dhamne <amitsd@google.com> > > 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. This is just redundant explanation. You did not actually explain why you need to set the trigger. Best regards, Krzysztof
Hi Amit,
Thanks for your patches to enable the charger!
> From: Amit Sunil Dhamne <amitsd@google.com>
>
> 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@google.com>
> ---
> 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?
> 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.
> 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).
> 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.
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).
Cheers,
Andre'
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@google.com>
>>
>> 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@google.com>
>> ---
>> 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'
Hi Amit,
On Tue, 2025-11-25 at 17:10 -0800, Amit Sunil Dhamne wrote:
> 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@google.com>
> > >
> > > 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@google.com>
> > > ---
> > > 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),
> > > };
You should also add the remaining bits in each register here, so that the
regulator-irq can mask them when no user exists. It will only touch the
bits it knows about, so the state of the mask register is non-
deterministic with this change as-is (depends on how the bootloader
configured it).
[...]
> >
> >
> > > 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.
It makes sense to add them now, as per above.
> > 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?
Yes, would be nice to have them all in one place (in this file). That said,
CHG_INT, CHG_INT_MASK, and CHG_INT_OK all have the same layout and share
the same bits, so I personally would probably reuse the ones you added for
INT for all three of them - MASK (as you did already), and also for the OK
register. But up to you.
Cheers,
Andre'
On 11/25/25 10:44 PM, André Draszik wrote:
> Hi Amit,
>
> On Tue, 2025-11-25 at 17:10 -0800, Amit Sunil Dhamne wrote:
>> 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@google.com>
>>>>
>>>> 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@google.com>
>>>> ---
>>>> 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),
>>>> };
> You should also add the remaining bits in each register here, so that the
> regulator-irq can mask them when no user exists. It will only touch the
> bits it knows about, so the state of the mask register is non-
> deterministic with this change as-is (depends on how the bootloader
> configured it).
>
> [...]
I see what you're saying. I will remove this and keep it the way it was
before.
>
>>>
>>>> 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.
> It makes sense to add them now, as per above.
Okay. I will add them.
>
>
>
>>> 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?
> Yes, would be nice to have them all in one place (in this file). That said,
> CHG_INT, CHG_INT_MASK, and CHG_INT_OK all have the same layout and share
> the same bits, so I personally would probably reuse the ones you added for
> INT for all three of them - MASK (as you did already), and also for the OK
> register. But up to you.
Sounds good.
Thanks,
Amit
> Cheers,
> Andre'
On Wed, 2025-11-26 at 06:44 +0000, André Draszik wrote:
> Hi Amit,
>
> On Tue, 2025-11-25 at 17:10 -0800, Amit Sunil Dhamne wrote:
> > 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@google.com>
> > > >
> > > > 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@google.com>
> > > > ---
> > > > 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),
> > > > };
>
> You should also add the remaining bits in each register here, so that the
> regulator-irq can mask them when no user exists. It will only touch the
^^^^^^^^^^^^^
regmap-irq
A.
On 24/11/2025 07:21, André Draszik wrote: > >> 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). > If this is the main chip interrupt, then you are right and the code is obviously wrong. What's more, it is completely unexplained in the commit msg, because that vague statement cannot be taken as any reasonable explanation. Best regards, Krzysztof
On 11/23/25 11:39 PM, Krzysztof Kozlowski wrote: > On 24/11/2025 07:21, André Draszik wrote: >>> 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). >> > If this is the main chip interrupt, then you are right and the code is > obviously wrong. What's more, it is completely unexplained in the commit > msg, because that vague statement cannot be taken as any reasonable > explanation. You are right. As discussed in the thread with André, I will drop this particular change in the next rev. BR, Amit > > Best regards, > Krzysztof
© 2016 - 2025 Red Hat, Inc.