Re: [PATCH v6 04/11] mfd: sec: add support for S2MU005 PMIC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Incorrect register access in sec_pmic_dump_rev() due to missing PMIC ID handling.
- [High] Missing interrupt acknowledgment for S2MU005 due to ack_base evaluating to 0.
- [Medium] Mismatch between commit message description and actual implementation of IRQ chips.
--

commit ffec6859a51e25188ffb07502f3e1c81cbef3517
Author: Kaustabh Chakraborty <kauschluss@disroot.org>

mfd: sec: add support for S2MU005 PMIC

This commit adds initial driver support for the Samsung S2MU005 PMIC,
mapping its charger, MUIC, and LED sub-devices and initializing its
interrupts.

> S2MU005's interrupt registers can be properly divided into three regmap
> IRQ chips, one each for the charger, flash LEDs, and the MUIC.
> 
> Add initial support for S2MU005 in the PMIC driver, along with it's three
> interrupt chips.

Is this description accurate regarding the three IRQ chips?

Looking at the implementation in sec-irq.c, there is only a single monolithic
regmap_irq_chip defined for the S2MU005 (s2mu005_irq_chip), which uses the
get_irq_reg callback to multiplex the disjoint register ranges.

Also, this isn't a bug, but there is a typo in the second paragraph ("it's"
instead of "its").

> diff --git a/drivers/mfd/sec-common.c b/drivers/mfd/sec-common.c
> index bd8b5f9686892..ad1c7de83a620 100644
> --- a/drivers/mfd/sec-common.c
> +++ b/drivers/mfd/sec-common.c

[ ... ]

> @@ -250,6 +262,10 @@ int sec_pmic_probe(struct device *dev, int device_type, unsigned int irq,
>  		sec_devs = s2mpu05_devs;
>  		num_sec_devs = ARRAY_SIZE(s2mpu05_devs);
>  		break;
> +	case S2MU005:
> +		sec_devs = s2mu005_devs;
> +		num_sec_devs = ARRAY_SIZE(s2mu005_devs);
> +		break;
>  	default:
>  		return dev_err_probe(sec_pmic->dev, -EINVAL,

Could this cause unexpected behavior when reading the chip revision?

Later in sec_pmic_probe(), sec_pmic_dump_rev() is called, which
unconditionally reads S2MPS11_REG_ID (0x00) for unhandled device types.
For S2MU005, the register at 0x00 is actually S2MU005_REG_CHGR_INT1.

Does reading this interrupt register outside of an IRQ context clear pending
hardware interrupts and cause the driver to lose initial state events?

I note that this regression was fixed later in the patch series by commit
f26ef88e98392 ("mfd: sec: resolve PMIC revision in S2MU005").

> diff --git a/drivers/mfd/sec-irq.c b/drivers/mfd/sec-irq.c
> index 133188391f7c2..91a2922463fb6 100644
> --- a/drivers/mfd/sec-irq.c
> +++ b/drivers/mfd/sec-irq.c

[ ... ]

> @@ -337,6 +397,17 @@ static const struct regmap_irq_chip s2mpu05_irq_chip = {
>  	.ack_base = S2MPU05_REG_INT1,
>  };
>  
> +static const struct regmap_irq_chip s2mu005_irq_chip = {
> +	.name = "s2mu005",
> +	.irqs = s2mu005_irqs,
> +	.num_irqs = ARRAY_SIZE(s2mu005_irqs),
> +	.num_regs = 4,
> +	.status_base = S2MU005_REG_CHGR_INT1,
> +	.mask_base = S2MU005_REG_CHGR_INT1M,
> +	.ack_base = S2MU005_REG_CHGR_INT1,
> +	.get_irq_reg = s2mu005_irq_get_reg,
> +};

Could this configuration bypass the interrupt acknowledgment phase?

Because S2MU005_REG_CHGR_INT1 is the first element in enum s2mu005_reg, it
evaluates to 0.

Looking at the regmap_irq framework, it executes:

    if (chip->ack_base || chip->use_ack)

Since ack_base is 0 and use_ack is omitted (defaulting to false), will the
framework fail to acknowledge level-triggered PMIC interrupts, leaving the
hardware asserting the IRQ line constantly and potentially resulting in an
unhandled interrupt storm?

Would explicitly adding .use_ack = true to the irq_chip configuration safely
resolve this?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515-s2mu005-pmic-v6-0-1979106992d4@disroot.org?part=4