Re: [PATCH 03/16] arm: dts: twl4030: add twl4030-regen child

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The YAML binding schema fails to allow 'startup-delay-us', causing a conflict between driver probe requirements and dtbs_check validation.
- [Medium] Architectural flaw in Device Tree design: conflating a PMIC digital control signal with an external voltage regulator.
--

--- Patch [3]: [PATCH 03/16] arm: dts: twl4030: add twl4030-regen child ---
commit a39086d961abe1def74820ce0f67a5656f16f99d
Author: Grond <grond66@riseup.net>

arm: dts: twl4030: add twl4030-regen child
    
This is needed on some boards (for example, the Pandora), where REGEN is
used to drive external regulators.

> diff --git a/arch/arm/boot/dts/ti/omap/twl4030.dtsi b/arch/arm/boot/dts/ti/omap/twl4030.dtsi
> index 07b9ca942e78d..e6f80b8f0a731 100644
> --- a/arch/arm/boot/dts/ti/omap/twl4030.dtsi
> +++ b/arch/arm/boot/dts/ti/omap/twl4030.dtsi
> @@ -112,6 +112,20 @@ vsim: regulator-vsim {
>  		regulator-max-microvolt = <3000000>;
>  	};
>  
> +	regen: regulator-regen {
> +		compatible = "ti,twl4030-regen";
> +		/*
> +		 * Since REGEN is technically just a signal that actuates an
> +		 * external regulator, we do not know what voltage it will

[Severity: Medium]
Is there an architectural mismatch in how this external regulator is modeled?

The comment acknowledges that REGEN is just a control signal, but the driver
forces the PMIC's digital signal node to define the external component's
voltage parameters:

drivers/regulator/twl-regulator.c:twlreg_probe() {
        if (c->min_uV == 0 || c->max_uV == 0) {
                dev_err(&pdev->dev, "minimum or maximum regulator voltage zero in DT\n");
                return -EINVAL;
        }
}

Would it be more aligned with Device Tree principles to model the PMIC's
REGEN pin as an enable-gpio or a voltage-less switch, and define a separate
regulator-fixed node for the external regulator that uses REGEN to enable it?

> +		 * eventually have, or what it's on/off timing should be.
> +		 * Therefore, disable it here; boards that use it can override
> +		 * the status to "okay" once they have set the
> +		 * regulator-{min,max}-microvolt and startup-delay-us
> +		 * attributes.
> +		 */

[Severity: High]
Will this cause a conflict between driver probe requirements and the
dtbs_check validation?

Looking at twlreg_probe() in drivers/regulator/twl-regulator.c, the driver
explicitly requires the startup-delay-us property:

drivers/regulator/twl-regulator.c:twlreg_probe() {
        ret = of_property_read_u32(pdev->dev.of_node, "startup-delay-us",
                                   &info->desc.enable_time);
        if (ret) {
                dev_err(&pdev->dev, "no valid startup delay set in DT: %d\n", ret);
                return ret;
        }
}

However, the ti,twl.yaml binding schema introduces ti,twl4030-regen but fails
to add startup-delay-us to the allowed properties, leading to
unevaluatedProperties: false rejecting it during validation.

If board developers provide the property to satisfy the driver, they will
fail schema validation, but if they omit it to pass validation, the driver
will fail to probe at runtime. Can the schema be updated to allow this, or
is there a different expected behavior?

> +		status = "disabled";
> +	};

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1783749722.git.hns@goldelico.com?part=3