Re: [PATCH v7 3/9] regulator: dt-bindings: Add MediaTek MT6392 PMIC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Incomplete schema constraints for `regulator-initial-mode` and `regulator-allowed-modes`.
- [Low] The Device Tree example includes an unrelated header and fails to demonstrate new binding properties.
--

commit 7a57eb124bc9a8ab53b73fb4508d2222b254ee23
Author: Luca Leonardo Scorcia <l.scorcia@gmail.com>

regulator: dt-bindings: Add MediaTek MT6392 PMIC

This commit adds device tree bindings for the regulators found in the
MediaTek MT6392 PMIC. It provides schemas for three buck converters and
several LDOs, including constraints for allowed mode settings.

> diff --git a/Documentation/devicetree/bindings/regulator/mediatek,mt6392-regulator.yaml b/Documentation/devicetree/bindings/regulator/mediatek,mt6392-regulator.yaml
> new file mode 100644
> index 0000000000000..197041df4ba1a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/mediatek,mt6392-regulator.yaml

[ ... ]

> +patternProperties:
> +  "^v(core|proc|sys)$":
> +    description: Buck regulators
> +    type: object
> +    $ref: regulator.yaml#
> +    properties:
> +      regulator-allowed-modes:
> +        description: |
> +          BUCK regulators can set regulator-initial-mode and regulator-allowed-modes to
> +          values specified in dt-bindings/regulator/mediatek,mt6392-regulator.h
> +        items:
> +          enum: [0, 1]

[Severity: Medium]
The description here mentions both regulator-initial-mode and
regulator-allowed-modes, but only regulator-allowed-modes is constrained.
Since regulator-initial-mode is not explicitly defined in these properties,
does it fall back to the unconstrained uint32 definition from regulator.yaml#?
Should regulator-initial-mode be added here to restrict it to [0, 1] as well?

> +    unevaluatedProperties: false

[ ... ]

> +  "^v(cama|dig18)$":
> +    description: LDOs with fixed output without mode setting
> +    type: object
> +    $ref: regulator.yaml#
> +    unevaluatedProperties: false

[Severity: Medium]
The description states these LDOs are "without mode setting", but the schema
does not explicitly forbid the mode properties. Because of the reference to
regulator.yaml#, doesn't this implicitly allow both regulator-initial-mode and
regulator-allowed-modes to pass validation?

> +
> +  "^v(aud22|camaf|camd|cn35|efuse|emc3v3|gp1|gp2|m|mc|mch)$":
> +    description: LDOs with adjustable output
> +    type: object
> +    $ref: regulator.yaml#
> +    properties:
> +      regulator-allowed-modes: false

[Severity: Medium]
This explicitly forbids regulator-allowed-modes. Should regulator-initial-mode
also be set to false here to fully prevent mode settings on these adjustable
LDOs?

> +    unevaluatedProperties: false
> +
> +required:
> +  - compatible
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>

[Severity: Low]
Is the arm-gic.h header needed here? There do not seem to be any interrupt
definitions in the example block.

Also, would it make sense to include the newly added
dt-bindings/regulator/mediatek,mt6392-regulator.h header instead, and add at
least one example usage of regulator-initial-mode or regulator-allowed-modes?
This would ensure the new properties and custom constraints are actively
exercised by the schema validator.

> +
> +    regulators {
> +      compatible = "mediatek,mt6392-regulator";

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615071836.362883-1-l.scorcia@gmail.com?part=3