Re: [PATCH 1/2] dt-bindings: regulator: add SGMicro SGM3804

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The DT binding incorrectly models a dual-output regulator (positive and negative) as a single top-level regulator node, preventing independent configuration and referencing of the two outputs.
--

commit db24a4c2f2c96be5ccf7d7d19de93b27ab20e291
Author: Alexandre Hamamdjian <azkali.limited@gmail.com>

dt-bindings: regulator: add SGMicro SGM3804

This commit introduces the device tree binding for the SGMicro SGM3804,
an I2C-controlled charge-pump regulator providing positive and negative
outputs. It also adds the corresponding MAINTAINERS entry.

> diff --git a/Documentation/devicetree/bindings/regulator/sgmicro,sgm3804.yaml b/Documentation/devicetree/bindings/regulator/sgmicro,sgm3804.yaml
> new file mode 100644
> index 0000000000000..f1de17bd5395a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/sgmicro,sgm3804.yaml
> @@ -0,0 +1,60 @@

[ ... ]

> +description:
> +  The SGM3804 is an I2C-controlled positive and negative output charge-pump
> +  regulator typically used to provide the AVDD and AVEE supplies for display
> +  panels. Two reset GPIOs are used to sequence the positive and negative
> +  outputs independently.
> +
> +allOf:
> +  - $ref: regulator.yaml#

Does this binding correctly model the hardware's dual-output nature?

By applying regulator.yaml# directly to the parent I2C device node without
defining subnodes for the individual outputs, the entire device is treated
as a single regulator.

Since standard properties like regulator-name and regulator-min-microvolt
can only be specified once per node, how can consumers independently configure
or reference the negative rail (AVEE) via phandles?

Should this device be modeled using child subnodes for the outputs (for
example, using patternProperties for outp and outn)?

[ ... ]

> +  reset-gpios:
> +    minItems: 1
> +    maxItems: 2
> +    description:
> +      Reset/enable GPIOs for the positive (index 0) and, optionally, the
> +      negative (index 1) output rails.

If child subnodes are introduced for the positive and negative regulators,
would it be more standard to place an enable-gpios property within each
individual regulator subnode instead of using a shared reset-gpios array on
the parent node?

[ ... ]

> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        regulator@3e {
> +            compatible = "sgmicro,sgm3804";
> +            reg = <0x3e>;
> +            reset-gpios = <&tlmm 10 GPIO_ACTIVE_HIGH>,
> +                          <&tlmm 11 GPIO_ACTIVE_HIGH>;
> +            regulator-name = "panel-avdd";
> +            regulator-min-microvolt = <5000000>;
> +            regulator-max-microvolt = <5000000>;
> +        };
> +    };

The example demonstrates this limitation, as it only configures constraints
for panel-avdd.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260510-sgm3804-v1-0-e5e8799e0aa0@gmail.com?part=1