Re: [PATCH v2 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 issue(s) to consider:
- [Low] dt-bindings: soc: brcm,bcm6846-pmb ordered after dependent brcm,bcm6846-pmc
- [Low] dt-bindings: hwmon: brcm,bcm6846-pmb-pvtmon uses additionalProperties with $ref
- [Low] dt-bindings: soc: brcm,bcm6846-pmb example child nodes out of alphabetical order
- [Low] MAINTAINERS: missing S: status tag in BROADCOM BCMBCA PMC3 PMB DRIVER entry
- [Low] dt-bindings: reset: non-contiguous reset IDs in brcm,bcm6846-pmb.h
--

commit 44e16c21f8b2ce99a2b8f758565676392f84eec0
Author: Linus Walleij <linusw@kernel.org>

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

This commit adds Device Tree binding schemas and header files for the
Broadcom BCM6846 Processor Monitor Bus (PMB) and its child clock, reset,
and PVTMON hardware monitor controllers.

>     dt-bindings: soc: Describe the BCM6846 Processor Monitor Bus
>     
>     The third generation Power Management Controller (PMC3) accesses SoC
>     block control and reset registers through the Processor Monitor Bus
>     (PMB). Describe the BCM6846 PMC and PMB.
>     
>     Signed-off-by: Linus Walleij <linusw@kernel.org>

[Severity: Low]
Should this commit be ordered before commit 39aa42d4e944 ("dt-bindings:
mfd: Describe the BCM6846 PMC complex"), and should the commit message drop
"PMC and"?

In the parent commit 39aa42d4e944,
Documentation/devicetree/bindings/mfd/brcm,bcm6846-pmc.yaml references
/schemas/soc/bcm/brcm,bcm6846-pmb.yaml# via $ref and includes
dt-bindings/clock/brcm,bcm6846-pmb.h and
dt-bindings/reset/brcm,bcm6846-pmb.h in its example before they are added
here, which causes make dt_binding_check to fail at the parent commit
during bisection.

> 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]
Since this schema includes /schemas/thermal/thermal-sensor.yaml# via allOf,
should it use unevaluatedProperties: false instead of additionalProperties:
false so that properties from the referenced schema are evaluated properly?

> 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..a4a77fed94d05
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/bcm/brcm,bcm6846-pmb.yaml
> @@ -0,0 +1,80 @@
[ ... ]
> +        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]
Should the unaddressed hwmon child node be placed before reset-controller
in the example so that the child nodes are in alphabetical order and pass
scripts/dtc/dt-check-style --mode=strict without a child-name-order
warning?

> diff --git a/MAINTAINERS b/MAINTAINERS
> index eb4b3b631e82c..fa95f2dec63c5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5446,7 +5446,13 @@ N:	bcm[9]?6878
>  
>  BROADCOM BCMBCA PMC3 PMB DRIVER
>  M:	Linus Walleij <linusw@kernel.org>
> +F:	Documentation/devicetree/bindings/clock/brcm,bcm6846-pmb-clock.yaml
> +F:	Documentation/devicetree/bindings/hwmon/brcm,bcm6846-pmb-pvtmon.yaml
>  F:	Documentation/devicetree/bindings/mfd/brcm,bcm6846-pmc.yaml
> +F:	Documentation/devicetree/bindings/reset/brcm,bcm6846-pmb-reset.yaml
> +F:	Documentation/devicetree/bindings/soc/bcm/brcm,bcm6846-pmb.yaml
> +F:	include/dt-bindings/clock/brcm,bcm6846-pmb.h
> +F:	include/dt-bindings/reset/brcm,bcm6846-pmb.h

[Severity: Low]
Should the BROADCOM BCMBCA PMC3 PMB DRIVER entry in MAINTAINERS include an
S: status tag (such as S: Maintained) so that scripts/get_maintainer.pl
--self-test=sections does not warn about a section without status?

> diff --git a/include/dt-bindings/reset/brcm,bcm6846-pmb.h b/include/dt-bindings/reset/brcm,bcm6846-pmb.h
> new file mode 100644
> index 0000000000000..6b1a6baac1c60
> --- /dev/null
> +++ b/include/dt-bindings/reset/brcm,bcm6846-pmb.h
> @@ -0,0 +1,11 @@
> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> +#ifndef _DT_BINDINGS_RESET_BRCM_BCM6846_PMB_H
> +#define _DT_BINDINGS_RESET_BRCM_BCM6846_PMB_H
> +
> +/* XRDP packet engine and its three Runner cores. */
> +#define BCM6846_RESET_XRDP	0
> +
> +/* Process, voltage and temperature monitor analog acquisition reset. */
> +#define BCM6846_RESET_PVTMON	3
> +
> +#endif

[Severity: Low]
Since these reset constants represent abstract logical reset group IDs
rather than hardware register offsets or bit positions, should
BCM6846_RESET_PVTMON be defined as 1 instead of 3 so that the IDs in this
new binding header are contiguous starting from 0 and
bcm6846_pmb_reset_groups[] in drivers/reset/reset-bcmbca-pmb.c does not
carry two unused entries at indices 1 and 2?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261101-xg6846b-bcmbca-xrdp-reset-v2-0-455fe9eed5ee@kernel.org?part=2