Re: [PATCH 2/2] regulator: pf0900: Modify volatile register range definition

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The `.n_yes_ranges` field in `pf0900_volatile_regs` is hardcoded to 1, causing most volatile registers to be ignored.
- [High] Interrupt status registers (_INT) are omitted from the volatile ranges, breaking IRQ handling.
--

commit 8da34e61f8f6ebeaacd20e2bb532c2786ad28d72
Author: Joy Zou <joy.zou@nxp.com>

regulator: pf0900: Modify volatile register range definition
    
The pf0900_range was incorrectly defined as a single continuous range
from PF0900_REG_DEV_ID to PF0900_REG_SYS_DIAG. This commit ensures only
volatile registers are read from the device on each access, while other
registers can be cached by regmap.

> diff --git a/drivers/regulator/pf0900-regulator.c b/drivers/regulator/pf0900-regulator.c
> index 5c44d2dbcab4d..bca76c58ed3bf 100644
> --- a/drivers/regulator/pf0900-regulator.c
> +++ b/drivers/regulator/pf0900-regulator.c
> @@ -284,13 +284,24 @@ struct pf0900_regulator_irq {
>  	unsigned int  event;
>  };
>  
> -static const struct regmap_range pf0900_range = {
> -	.range_min = PF0900_REG_DEV_ID,
> -	.range_max = PF0900_REG_SYS_DIAG,
> +static const struct regmap_range pf0900_range[] = {
> +	regmap_reg_range(PF0900_REG_SYSTEM_INT, PF0900_REG_SYSTEM_INT),
> +	regmap_reg_range(PF0900_REG_STATUS1_SNS, PF0900_REG_STATUS1_SNS),

[Severity: High]
Does this new configuration omit the interrupt status registers needed by
pf0900_irq_handler()?

Looking at pf0900_irq_handler(), it relies on regmap_read() to query the
hardware interrupt status:

pf0900_irq_handler() {
	...
	ret = regmap_read(pf0900->regmap, regu_irqs[i].reg, &val);
	...
}

Since these _INT registers are no longer listed in the volatile ranges,
will regmap reads return cached values instead of the actual hardware
state, causing the driver to silently ignore active interrupts?

[ ... ]

> +	regmap_reg_range(PF0900_REG_SECURE_WR1, PF0900_REG_SECURE_WR1),
> +	regmap_reg_range(PF0900_REG_WD_CNT1, PF0900_REG_WD_CNT2),
> +	regmap_reg_range(PF0900_REG_SYS_DIAG, PF0900_REG_SYS_DIAG),
>  };
>  
>  static const struct regmap_access_table pf0900_volatile_regs = {
> -	.yes_ranges = &pf0900_range,
> +	.yes_ranges = pf0900_range,
>  	.n_yes_ranges = 1,

[Severity: High]
Should this be updated to ARRAY_SIZE(pf0900_range)?

With n_yes_ranges hardcoded to 1, won't the regmap subsystem ignore all
but the very first entry (PF0900_REG_SYSTEM_INT) in the array? This
could cause reads to the remaining hardware status registers to return
stale cached values.

>  };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618-b4-regulator-opt-v1-0-c43b1f62aaf6@oss.nxp.com?part=2