Re: [PATCH v2] dt-bindings: scsi: Convert hisilicon,hip05-sas-v1 to DT schema

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] dt-bindings: missing allOf conditionals for version-specific constraints
- [Low] dt-bindings: missing interrupt-parent causes dt_binding_check failure
--

commit 1af097509c0514a80fd91089aea8a399516e8223
Author: Rob Herring (Arm) <robh@kernel.org>

dt-bindings: scsi: Convert hisilicon,hip05-sas-v1 to DT schema

This commit converts the HiSilicon SAS controller binding from a plain text
document to a DT schema. The old text file is removed and replaced with a
new YAML schema covering both v1 and v2 hardware variants.

> diff --git a/Documentation/devicetree/bindings/scsi/hisilicon,hip05-sas-v1.yaml b/Documentation/devicetree/bindings/scsi/hisilicon,hip05-sas-v1.yaml
> new file mode 100644
> index 0000000000000..27d4cf5078e6e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/scsi/hisilicon,hip05-sas-v1.yaml
[ ... ]
> +  interrupts:
> +    description: >
> +      Interrupt specifiers for PHY, SATA, and completion queues; ordering and
> +      number depend on controller version.
[ ... ]
> +        - Completion queue interrupts - each completion queue has 1 interrupt
> +          source
> +    minItems: 58

[Severity: Medium]
Could this global minItems constraint inadvertently reject valid v2 hardware
configurations?

A typical v2 controller (8 phys, 32 queues) needs exactly 42 interrupts
(2 phy + 8 Sata + 32 CQ), which would inherently require fewer than 58
interrupts and fail schema validation.

> +    maxItems: 128
> +
> +  ctrl-reset-reg:
> +    description: Offset to controller reset register in SAS syscon region.
> +    $ref: /schemas/types.yaml#/definitions/uint32
[ ... ]
> +  ctrl-clock-ena-reg:
> +    description: Offset to controller clock enable register in SAS syscon region.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  hip06-sas-v2-quirk-amt:
> +    description:
> +      Indicates that the v2 controller has the "am-max-transmissions" limitation.
> +    type: boolean

[Severity: Medium]
Does this v2-specific quirk need an allOf conditional block with an if-then
restriction?

Without it, the quirk could silently pass schema validation on v1 hardware,
allowing it to be improperly set where it doesn't apply.

> +
> +  hisilicon,sas-syscon:
> +    description: Phandle of syscon used for SAS control.
> +    $ref: /schemas/types.yaml#/definitions/phandle
[ ... ]
> +examples:
> +  - |
> +    sas@c1000000 {
> +        compatible = "hisilicon,hip05-sas-v1";
> +        reg = <0xc1000000 0x10000>;
> +        dma-coherent;
> +        interrupts = <259 4>, <263 4>, <264 4>, /* phy0 */
> +                     <269 4>, <273 4>, <274 4>, /* phy1 */

[Severity: Low]
Does this example omit the interrupt-parent that was present in the original
text binding?

Without an overriding interrupt-parent, the default schema skeleton assumes a
GIC with 3 cells (#interrupt-cells = <3>). This causes dtc validation to fail
due to a size mismatch, as the 2-cell interrupts here do not align with the
default parent.

> +                     <279 4>, <283 4>, <284 4>, /* phy2 */
> +                     <289 4>, <293 4>, <294 4>, /* phy3 */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923191511.2205564-1-robh@kernel.org?part=1