Re: [PATCH v2 1/7] dt-bindings: mfd: x-powers,axp152: Add the AXP2101

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Low] dt-bindings: mfd: x-powers,axp152: x-powers,dcdc-freq: false at
  wrong level
- [Low] dt-bindings: mfd: x-powers,axp152: unwieldy per-compatible
  conditionals
--

commit 520d0c5b7e3c77d62056e2ccbc2df67bc5ef33a7
Author: Olivier Louvignes <olivier@mgcrea.io>

dt-bindings: mfd: x-powers,axp152: Add the AXP2101

This commit adds the x-powers,axp2101 compatible string to the X-Powers AXP
PMIC binding with optional interrupts and no DCDC frequency setting.

> diff --git a/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml b/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml
> index 45f015d63df16..024de5553d6b7 100644
> --- a/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml
> +++ b/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml
> @@ -69,6 +69,7 @@ allOf:
>              contains:
>                enum:
>                  - x-powers,axp15060
> +                - x-powers,axp2101
>                  - x-powers,axp305
>                  - x-powers,axp313a
>                  - x-powers,axp323
> @@ -85,6 +86,7 @@ allOf:
>                - x-powers,axp313a
>                - x-powers,axp323
>                - x-powers,axp15060
> +              - x-powers,axp2101
>                - x-powers,axp717
>  
>      then:

[Severity: Low]
Does adding x-powers,axp2101 to this allOf rule actually forbid
x-powers,dcdc-freq under the regulators child node?

x-powers,dcdc-freq is defined and parsed under properties -> regulators ->
properties, whereas the then clause of this block sets x-powers,dcdc-freq:
false at the top-level PMIC node:

Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml:allOf:
    then:
      properties:
        x-powers,dcdc-freq: false

Because the first allOf entry's else branch defines x-powers,dcdc-freq under
properties -> regulators -> properties with a 1800 to 4050 kHz range, schema
validation still permits x-powers,dcdc-freq inside the regulators node for
x-powers,axp2101. Nesting x-powers,dcdc-freq: false under properties ->
regulators -> properties in this then block would disallow it as intended.

> @@ -108,6 +110,7 @@ properties:
>            - x-powers,axp806
>            - x-powers,axp809
>            - x-powers,axp813
> +          - x-powers,axp2101
>            - x-powers,axp15060
>        - items:
>            - const: x-powers,axp228

[Severity: Low]
This isn't a bug, but x-powers,axp152.yaml already aggregates nearly twenty
PMIC variants with five per-compatible if-then blocks in allOf. Rather than
extending these conditional lists further (and inserting x-powers,axp2101 out
of numerical order after x-powers,axp813), would it be cleaner to split new
variants into a separate binding schema document?

Also, the commit message explains the optional interrupt line in terms of
Linux driver behavior ("the driver then registers only the regulators") and
gives no background on the AXP2101 hardware. Describing the PMIC and why the
interrupt line may be omitted in hardware or board wiring would make the
binding description clearer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261115171150.64431-1-olivier@mgcrea.io?part=1