Re: [PATCH v2 1/4] regulator: dt-bindings: Add fan53555 allowed modes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] The regulator driver lacks the `.of_map_mode` callback required to parse the newly added `regulator-initial-mode` device tree property.
- [High] The patch is missing the board and script modifications explicitly listed in the commit message, and includes raw `git status` output.
- [Medium] The schema fails to constrain the `regulator-allowed-modes` property despite the commit explicitly claiming to add allowed modes.

Pre-existing issues:
- [High] The driver's `fan53555_set_mode()` function accidentally writes to the voltage register (`vol_reg`) instead of the mode register (`mode_reg`) when switching to `REGULATOR_MODE_NORMAL`.
--

--- Patch [1]: [PATCH v2 1/4] regulator: dt-bindings: Add fan53555 allowed modes ---
commit 6198556f842d1d3f4521256414c33ff637d763cb
Author: Victor Krawiec <victor.krawiec@arturia.com>

regulator: dt-bindings: Add fan53555 allowed modes

This commit adds device tree binding definitions for the FAN53555 regulator's
operational modes (Auto-PFM and Forced PWM). It introduces a new header file
with mode constants and updates the YAML schema to allow specifying an initial
mode.

>     Documentation/devicetree/bindings/regulator/fcs,fan53555.yaml # new file:
>     include/dt-bindings/regulator/fcs,fan53555-regulator.h #
>     arch/arm64/boot/dts/rockchip/rk3399-drumfreak.dts #
>     arch/arm64/boot/simple-boot.img #       arch/arm64/boot/simple-kernel.img #
>     arch/arm64/configs/drumfreak_defconfig #
>     scripts/arturia_simple_boot_image.sh #

[Severity: High]
Are these files intentionally missing from the diff? The commit message lists
several board and script files, as well as driver implementation details, but
they are not included in the patch. It looks like accidental output from git
status was included in the commit message, and the patch submission might be
incomplete.

> diff --git a/Documentation/devicetree/bindings/regulator/fcs,fan53555.yaml b/Documentation/devicetree/bindings/regulator/fcs,fan53555.yaml
> index 69bae90fc4b28..9a18891f721e6 100644
> --- a/Documentation/devicetree/bindings/regulator/fcs,fan53555.yaml
> +++ b/Documentation/devicetree/bindings/regulator/fcs,fan53555.yaml
> @@ -48,6 +48,12 @@ properties:
>        VSEL0 register. When this pin is HIGH, VOUT is set by the VSEL1 register.
>      maxItems: 1
>  
> +  regulator-initial-mode:
> +    enum:
> +      [ 1, 2 ]

[Severity: Medium]
The commit message mentions adding allowed modes, but the schema only constrains
regulator-initial-mode. Should regulator-allowed-modes also be added to the
schema with constraints? 

Without it, dt_binding_check will permit any values for regulator-allowed-modes
since it falls back to the generic uint32-array schema.

> +    description:
> +      Defined in include/dt-bindings/regulator/fcs,fan53555-regulator.h

[ ... ]

While reviewing the proposed bindings, I noticed a couple of issues in the
corresponding driver implementation in drivers/regulator/fan53555.c.

drivers/regulator/fan53555.c:fan53555_regulator_register() {
        ...
        rdesc->ops = &fan53555_regulator_ops;
        rdesc->type = REGULATOR_VOLTAGE;
        ...
}

[Severity: High]
Does the driver need an .of_map_mode callback assigned to the regulator_desc?

Without this callback, the regulator core cannot parse regulator-initial-mode
during probe, and will silently ignore the property with a warning.

This is a pre-existing issue, but while reviewing the mode switching logic
that these bindings interact with:

drivers/regulator/fan53555.c:fan53555_set_mode() {
        ...
        case REGULATOR_MODE_NORMAL:
                regmap_update_bits(rdev->regmap, di->vol_reg, di->mode_mask, 0);
                break;
        ...
}

[Severity: High]
Does this mistakenly write to di->vol_reg instead of di->mode_reg?

On variants like the TCS4525 where di->mode_mask is BIT(7) or BIT(6), clearing
this bit in the voltage register clears VSEL_BUCK_EN. This would unexpectedly
turn off the regulator instead of changing its mode.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720100336.62004-1-victor.krawiec@arturia.com?part=1