| 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: <aa7bdeb1-c8a9-4353-af56-869f16a083c2@google.com>
Date: Tue, 25 Nov 2025 17:10:01 -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
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),
>> };
>>
>> 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'
Powered by blists - more mailing lists