| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <6f2f075b-3570-4424-9cc7-695fb8c3b988@google.com>
Date: Wed, 26 Nov 2025 20:15:38 -0800
From: Amit Sunil Dhamne <amitsd@...gle.com>
To: André Draszik <andre.draszik@...aro.org>,
Sebastian Reichel <sre@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Lee Jones <lee@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Badhri Jagan Sridharan <badhri@...gle.com>,
Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
Peter Griffin <peter.griffin@...aro.org>,
Tudor Ambarus <tudor.ambarus@...aro.org>,
Alim Akhtar <alim.akhtar@...sung.com>
Cc: linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
devicetree@...r.kernel.org, linux-usb@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-samsung-soc@...r.kernel.org,
RD Babiera <rdbabiera@...gle.com>, Kyle Tso <kyletso@...gle.com>
Subject: Re: [PATCH 4/6] mfd: max77759: modify irq configs
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@...gle.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@...gle.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'
Powered by blists - more mailing lists