Re: [PATCH 1/3] dt-bindings: soundwire: qcom: describe multi-master

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Missing `-cells` property definition for `qcom,multi-master-peer` causing dt-schema validation failures.
- [High] Undeclared `qcom,ports-ch-mask` property referenced in conditional block breaks `dt_binding_check`.
- [Medium] Single-master configuration block fails to explicitly forbid multi-master properties, silently allowing incomplete multi-master nodes.
- [Medium] Commit message explicitly lists two properties but the code adds three.
--

commit f97a4adf5082bcb22d04e3fb5cc90da2b743d260
Author: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>

dt-bindings: soundwire: qcom: describe multi-master

This commit adds properties to describe multi-master mode in v3.1.0+ Qualcomm
SoundWire IPs. It defines roles and peer relationships for logical buses using
multiple DATA lanes.

> v3.1.0+ Qualcomm SoundWire IPs can be paired to expose one logical
> bus with more than two DATA lanes. Add the two properties that
> describe such a pair:
> 
>   qcom,multi-master-mode  role in the pair ("primary" or "dependent")
>   qcom,multi-master-peer  <phandle, bus-port-offset> to the other IP

[Severity: Medium]
This isn't a bug, but there is a slight inconsistency in the commit message.
It mentions adding two properties, while the patch actually introduces three
properties, adding num-lanes as well. Should the commit message be updated to
reflect the third property?

> diff --git a/Documentation/devicetree/bindings/soundwire/qcom,soundwire.yaml b/Documentation/devicetree/bindings/soundwire/qcom,soundwire.yaml
> index 8e6973fa229c9..34ce8caec0a86 100644
> --- a/Documentation/devicetree/bindings/soundwire/qcom,soundwire.yaml
> +++ b/Documentation/devicetree/bindings/soundwire/qcom,soundwire.yaml
[ ... ]
> +  qcom,multi-master-peer:
> +    description:
> +      Peers in multi-master mode. Each entry is a
> +      <peer-phandle, bus-port-offset> pair. Peer N's DPn slots
> +      1, 2, 3, ... occupy bus ports (offset+1), (offset+2), ...
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    items:
> +      items:
> +        - description: peer phandle
> +        - description: bus port offset (last primary-local port)
> +    minItems: 1

[Severity: High]
Because qcom,multi-master-peer is defined as a phandle-array, doesn't the
referenced target node need a corresponding #qcom,multi-master-peer-cells
property for dt-schema to parse the arguments?

Since the schema uses unevaluatedProperties: false and does not define
#qcom,multi-master-peer-cells globally, it creates a situation where adding
the cells property fails schema validation, but omitting it causes dtbs_check
to fail. Can this be resolved by declaring the -cells property in this schema?

[ ... ]
>  allOf:
>    - $ref: soundwire-controller.yaml#
>  
> +  # Single-master (no qcom,multi-master-mode): needs port config.
> +  - if:
> +      not:
> +        required:
> +          - qcom,multi-master-mode
> +    then:
> +      required:
> +        - '#sound-dai-cells'
> +        - qcom,ports-offset1
> +        - qcom,ports-offset2

[Severity: Medium]
If a user writes an incomplete multi-master device tree node and forgets to
include the qcom,multi-master-mode property, will it silently pass validation
here as a single-master configuration?

Since qcom,multi-master-peer and num-lanes are globally declared,
unevaluatedProperties: false won't catch them. Would it be safer to explicitly
forbid the multi-master properties in this single-master block by setting
qcom,multi-master-peer: false and num-lanes: false?

[ ... ]
> +  # Multi-master dependent: contributes DATA lanes
> +  - if:
> +      properties:
> +        qcom,multi-master-mode:
> +          const: dependent
> +      required:
> +        - qcom,multi-master-mode
> +    then:
> +      properties:
> +        compatible:
> +          contains:
> +            const: qcom,soundwire-v3.1.0
> +        '#sound-dai-cells': false
> +        qcom,din-ports: false
> +        qcom,dout-ports: false
> +        qcom,ports-word-length: false
> +        qcom,ports-sinterval-low: false
> +        qcom,ports-sinterval: false
> +        qcom,ports-offset1: false
> +        qcom,ports-offset2: false
> +        qcom,ports-lane-control: false
> +        qcom,ports-block-pack-mode: false
> +        qcom,ports-hstart: false
> +        qcom,ports-hstop: false
> +        qcom,ports-block-group-count: false
> +        qcom,ports-ch-mask: false

[Severity: High]
Does qcom,ports-ch-mask need to be globally declared in the top-level
properties block?

The dtschema meta-schema requires all properties evaluated in
if/then/properties blocks to be declared globally, and qcom,ports-ch-mask does
not seem to be present in this file or inherited from soundwire-controller.yaml.
Won't this cause make dt_binding_check to fail with an undeclared property
error?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1789114385.git.srinivas.kandagatla@oss.qualcomm.com?part=1