Re: [PATCH v3 2/7] dt-bindings: soc: Describe the BCM6846 Processor Monitor Bus

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [Low] dt-bindings: hwmon: brcm,bcm6846-pmb-pvtmon uses
  additionalProperties with $ref
- [Low] dt-bindings: soc: brcm,bcm6846-pmb ordered after dependent
  brcm,bcm6846-pmc
- [Low] dt-bindings: soc: brcm,bcm6846-pmb examples violate
  dt-check-style strict rules
- [Low] dt-bindings: soc: bcm: driver-model child nodes in
  brcm,bcm6846-pmb
- [Low] dt-bindings: soc: bcm6846-pmb: expand PMB as Power Management
  Bus
--

commit 6ce40c3812dd052e5e8e3945888002592aa196c2
Author: Linus Walleij <linusw@kernel.org>

dt-bindings: soc: Describe the BCM6846 Processor Monitor Bus

This commit adds Device Tree binding schemas and header constants for the
Broadcom BCM6846 Processor Monitor Bus (PMB) and its clock, hwmon, power,
and reset child controllers.

> diff --git a/Documentation/devicetree/bindings/hwmon/brcm,bcm6846-pmb-pvtmon.yaml b/Documentation/devicetree/bindings/hwmon/brcm,bcm6846-pmb-pvtmon.yaml
> new file mode 100644
> index 0000000000000..9bf52cf097a32
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/brcm,bcm6846-pmb-pvtmon.yaml
> @@ -0,0 +1,42 @@
[ ... ]
> +allOf:
> +  - $ref: /schemas/thermal/thermal-sensor.yaml#
> +
> +properties:
> +  compatible:
> +    const: brcm,bcm6846-pmb-pvtmon
> +
> +  clocks:
> +    maxItems: 1
> +    description: PVTMON acquisition clock
> +
> +  resets:
> +    maxItems: 1
> +    description: PVTMON analog-acquisition reset
> +
> +  '#thermal-sensor-cells':
> +    const: 0
> +
> +required:
> +  - compatible
> +  - clocks
> +  - resets
> +  - '#thermal-sensor-cells'
> +
> +additionalProperties: false

[Severity: Low]
Should this schema use unevaluatedProperties: false instead of
additionalProperties: false?  Because it references
/schemas/thermal/thermal-sensor.yaml# via allOf, additionalProperties: false
prevents properties defined only in the referenced schema from being
evaluated cleanly.

> diff --git a/Documentation/devicetree/bindings/mfd/brcm,bcm6846-pmc.yaml b/Documentation/devicetree/bindings/mfd/brcm,bcm6846-pmc.yaml
> index d4c927cabb3f4..77fbf92926ab3 100644
> --- a/Documentation/devicetree/bindings/mfd/brcm,bcm6846-pmc.yaml
> +++ b/Documentation/devicetree/bindings/mfd/brcm,bcm6846-pmc.yaml
> @@ -60,6 +60,11 @@ examples:
>              compatible = "brcm,bcm6846-pmb";
>              reg = <0x20100 0x200>;
>  
> +            pmb_power: power-controller {
> +                compatible = "brcm,bcm6846-pmb-power";
> +                #power-domain-cells = <1>;
> +            };
> +
>              pmb_clk: clock-controller {
>                  compatible = "brcm,bcm6846-pmb-clock";
>                  clocks = <&osc50m>;

[Severity: Low]
Should this commit be ordered before commit "dt-bindings: mfd: Describe the
BCM6846 PMC complex"?

That earlier commit introduces brcm,bcm6846-pmc.yaml referencing
/schemas/soc/bcm/brcm,bcm6846-pmb.yaml# and including
dt-bindings/clock/brcm,bcm6846-pmb.h and
dt-bindings/reset/brcm,bcm6846-pmb.h before this commit adds them, which
breaks make dt_binding_check bisectability at the parent commit.  In
addition, the commit message here still states that it describes the PMC and
only mentions the power controller child, omitting the clock, reset, and
PVTMON schemas and headers introduced in this patch.

> diff --git a/Documentation/devicetree/bindings/soc/bcm/brcm,bcm6846-pmb.yaml b/Documentation/devicetree/bindings/soc/bcm/brcm,bcm6846-pmb.yaml
> new file mode 100644
> index 0000000000000..134a8a676dbd0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/bcm/brcm,bcm6846-pmb.yaml
> @@ -0,0 +1,90 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/soc/bcm/brcm,bcm6846-pmb.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Broadcom BCM6846 Processor Monitor Bus
> +
> +maintainers:
> +  - Linus Walleij <linusw@kernel.org>
> +
> +description:
> +  The Processor Monitor Bus (PMB) connects the third generation Power
> +  Management Controller (PMC3) to the control registers of miscellaneous
> +  independent hardware blocks.

[Severity: Low]
As reported by William Zhang on v2 [1], PMB stands for Power Management Bus
rather than Processor Monitor Bus.  The commit subject, commit message, and
all five new binding schemas in this patch still expand PMB as Processor
Monitor Bus.

[ ... ]

> +  power-controller:
> +    type: object
> +    $ref: /schemas/power/brcm,bcm6846-pmb-power.yaml#
> +
> +  clock-controller:
> +    type: object
> +    $ref: /schemas/clock/brcm,bcm6846-pmb-clock.yaml#
> +
> +  reset-controller:
> +    type: object
> +    $ref: /schemas/reset/brcm,bcm6846-pmb-reset.yaml#
> +
> +  hwmon:
> +    type: object
> +    $ref: /schemas/hwmon/brcm,bcm6846-pmb-pvtmon.yaml#

[Severity: Low]
As reported by Philipp Zabel on RFC [2], should #power-domain-cells,
#clock-cells, #reset-cells, #thermal-sensor-cells, and the reference clock
live directly on the brcm,bcm6846-pmb node instead of using four addressless
child nodes?

Neither power-controller nor reset-controller has any DT resources of its
own, and the power, clock, reset, and hwmon drivers all access shared 32-bit
BPCM zone control and PVTMON_CONFIG registers over the parent PMB keyhole
interface.  Folding the provider cells into the brcm,bcm6846-pmb node avoids
encoding the Linux MFD sub-driver split into the Device Tree ABI while still
allowing the MFD driver to instantiate child devices internally.

[ ... ]

> +        pmb_power: power-controller {
> +            compatible = "brcm,bcm6846-pmb-power";
> +            #power-domain-cells = <1>;
> +        };
> +
> +        pmb_clk: clock-controller {
> +            compatible = "brcm,bcm6846-pmb-clock";
> +            clocks = <&osc50m>;
> +            clock-names = "ref";
> +            #clock-cells = <1>;
> +        };
> +
> +        pmb_reset: reset-controller {
> +            compatible = "brcm,bcm6846-pmb-reset";
> +            #reset-cells = <1>;
> +        };
> +
> +        hwmon {
> +            compatible = "brcm,bcm6846-pmb-pvtmon";
> +            clocks = <&pmb_clk BCM6846_CLK_PVTMON>;
> +            resets = <&pmb_reset BCM6846_RESET_PVTMON>;
> +            #thermal-sensor-cells = <0>;
> +        };
> +    };

[Severity: Low]
Does this example (and the matching example in brcm,bcm6846-pmc.yaml) trigger
unused-labels and child-name-order warnings under scripts/dtc/dt-check-style
--mode=strict?  The pmb_power label is never referenced in the example, and
the unaddressed child nodes under the PMB node are not in alphabetical order
(clock-controller, hwmon, power-controller, reset-controller).

[1] https://lore.kernel.org/r/CAHi4H7GOMM9K6XG15vPyFMLOza3QQ7QPxUkCi85jA_e9D6YbKg@mail.gmail.com
[2] https://lore.kernel.org/r/f1c24a603964302ec1462b1cea790a2a7079fb3d.camel@pengutronix.de

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-xg6846b-bcmbca-xrdp-reset-v3-0-d6adeb71afc9@kernel.org?part=2