Re: [PATCH] dt-bindings: mfd: as3722: Convert to DT schema

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The YAML schema artificially restricts the PMIC to a single pinmux state node by hardcoding the node name to `pinmux`.
- [Medium] The `interrupts` property is missing from the `required` list in the YAML binding despite being strictly required by the MFD driver.

Pre-existing issues:
- [High] The regulator driver incorrectly rejects `ams,ext-control = <3>`, preventing configuration of the ENABLE3 external control pin.
--

commit a75df0e5820c75f9d84e306c3842971fc178f5d3
Author: Shaunak Datar <shaunakkdatar@gmail.com>

dt-bindings: mfd: as3722: Convert to DT schema

Convert the AS3722 PMIC binding from the text format to YAML Schema to
enable DT Schema validation.

> diff --git a/Documentation/devicetree/bindings/mfd/ams,as3722.yaml b/Documentation/devicetree/bindings/mfd/ams,as3722.yaml
> new file mode 100644
> index 0000000000000..065537f50e063
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/ams,as3722.yaml
[ ... ]
> +  ams,system-power-controller:
> +    type: boolean
> +    description:
> +      Boolean, to enable the power off functionality through this device.
> +
> +  pinmux:
> +    type: object
> +    additionalProperties: false

[Severity: Medium]
Does this hardcoded node name artificially restrict the PMIC to a single pinmux
state node?

Device trees requiring multiple pinctrl states (e.g., default and sleep) will
fail schema validation if they define multiple state nodes. The schema uses a
fixed pinmux property name and additionalProperties: false on the root, which
rejects any other node names. Would it be better to use patternProperties
(e.g., "^pinmux(-[a-z0-9-]+)?$") to allow multiple state nodes?

> +    description:
> +      Pin control setting at boot. Each subnode represents some desired
[ ... ]
> +          ams,enable-tracking:
> +            type: boolean
> +            description:
> +              Enable tracking with SD1, only supported by LDO3.
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupt-controller
> +  - "#interrupt-cells"

[Severity: Medium]
Is the interrupts property missing from this required list?

The PMIC driver unconditionally fails to probe with -EINVAL at runtime if the
interrupt is absent (as3722_i2c_probe() calls irq_get_irq_data(i2c->irq)).
Device trees missing the interrupts property will silently pass schema
validation but fail at runtime. Adding interrupts to the required list would
accurately reflect the hardware and driver dependencies.

[ ... ]

While reviewing this binding conversion, I also noticed a pre-existing issue
in the regulator driver.

drivers/regulator/as3722-regulator.c:as3722_get_regulator_dt_data() {
    ...
		ret = of_property_read_u32(reg_node, "ams,ext-control", &prop);
		if (!ret) {
			if (prop < 3)
				reg_config->ext_control = prop;
			else
				dev_warn(&pdev->dev,
					"ext-control have invalid option: %u\n",
					prop);
		}
    ...
}

[Severity: High]
This is a pre-existing issue, but does this off-by-one check incorrectly reject
ams,ext-control = <3>?

A valid device tree specifying ams,ext-control = <3> is validated with
if (prop < 3), which rejects 3, warns the user, and defaults to 0 (no external
control), despite as3722_extreg_init() explicitly supporting
AS3722_EXT_CONTROL_ENABLE3 (3). Should this check be prop <= 3 so the ENABLE3
hardware pin can be correctly configured?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914174440.104115-1-shaunakkdatar@gmail.com?part=1