[PATCH 4/6] mfd: max77759: modify irq configs

[André Draszik]

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 at 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 at 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'