Re: [PATCH v7 1/3] dt-bindings: i3c: Add support for Qualcomm I3C controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Low] dt-bindings: i3c: qcom,geni-se.yaml missing i3c child node pattern
- [Low] dt-bindings: i3c: qcom,geni-i3c.yaml requires optional pinctrl-1 state
- [Low] dt-bindings: i3c: qcom,geni-i3c.yaml omits power-domains and OPP properties
- [Low] dt-bindings: i3c: qcom,geni-i3c.yaml example uses QUP base for IBI_C reg
--

commit 2e47e899944b24dd4c6fc08e4da686257bf4137c
Author: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>

dt-bindings: i3c: Add support for Qualcomm I3C controller

This commit adds the device tree binding schema for the Qualcomm GENI-based
I3C controller and its associated In-Band Interrupt controller.

> diff --git a/Documentation/devicetree/bindings/i3c/qcom,geni-i3c.yaml b/Documentation/devicetree/bindings/i3c/qcom,geni-i3c.yaml
> new file mode 100644
> index 0000000000000..bab89aeee23f4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i3c/qcom,geni-i3c.yaml
> @@ -0,0 +1,115 @@

[ ... ]

> +  I3C controller nodes must be child nodes of a GENI-based Qualcomm Universal
> +  Peripheral(QUP) wrapper. Refer to the GENI QUP wrapper binding described in
> +  Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.yaml.

[Severity: Low]
Will device tree validation fail when an i3c child node is placed under a
GENI QUP wrapper node?

In Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.yaml,
patternProperties only permits spi, i2c, and serial child nodes while
setting additionalProperties: false:

patternProperties:
  "spi@[0-9a-f]+$":
    ...
    $ref: /schemas/spi/qcom,spi-geni-qcom.yaml#

  "i2c@[0-9a-f]+$":
    ...
    $ref: /schemas/i2c/qcom,i2c-geni-qcom.yaml#

  "serial@[0-9a-f]+$":
    ...
    $ref: /schemas/serial/qcom,serial-geni-qcom.yaml#
...
additionalProperties: false

Should Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.yaml also be
updated to allow i3c@[0-9a-f]+$ referencing /schemas/i3c/qcom,geni-i3c.yaml#
(and disallow it for qcom,geni-se-i2c-master-hub)?

> +
> +allOf:
> +  - $ref: i3c.yaml#
> +  - $ref: /schemas/soc/qcom/qcom,se-common-props.yaml#
> +
> +properties:

[ ... ]

> +  pinctrl-names:
> +    minItems: 1
> +    items:
> +      - const: default
> +      - const: sleep
> +
> +  dmas:
> +    maxItems: 2
> +
> +  dma-names:
> +    items:
> +      - const: tx
> +      - const: rx
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - interrupts
> +  - interconnects
> +  - interconnect-names
> +  - pinctrl-0
> +  - pinctrl-1
> +  - pinctrl-names

[Severity: Low]
Should pinctrl-1 (and pinctrl-0 / pinctrl-names) be dropped from the
required list, and pinctrl-0: true and pinctrl-1: true be declared in
properties?

Under properties, pinctrl-names specifies minItems: 1 so that the sleep
state is optional, which matches the sibling bindings in
Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml and
Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.yaml. However,
listing pinctrl-1 in required forces every node to provide a second pinctrl
state and causes dtbs_check failures for boards or SoC dtsi files that only
define the default state.

> +
> +unevaluatedProperties: false

[Severity: Low]
What happens during dtbs_check when a qcom,geni-i3c node specifies
power-domains, operating-points-v2, or required-opps?

Sibling GENI Serial Engine bindings such as
Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml define:

  operating-points-v2: true
...
  power-domains:
    maxItems: 1
...
  required-opps: true

Because neither i3c.yaml nor qcom,se-common-props.yaml defines these
properties and unevaluatedProperties is set to false here, won't QUPv3
Serial Engine nodes that use power-domains and OPP tables fail schema
validation?

> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/clock/qcom,sm8550-gcc.h>
> +    #include <dt-bindings/interconnect/qcom,kaanapali-rpmh.h>
> +
> +    i3c@884000 {
> +        compatible = "qcom,geni-i3c";
> +        reg = <0x00884000 0x4000>,
> +              <0x008c0000 0x10000>;

[Severity: Low]
Is 0x008c0000 the intended address for the IBI controller register space in
this example?

In arch/arm64/boot/dts/qcom/kaanapali.dtsi and
arch/arm64/boot/dts/qcom/sm8550.dtsi, 0x008c0000 is the register base of the
parent QUPv3 wrapper node (geniqup@8c0000) that contains SE1 at 0x00884000:

		qupv3_2: geniqup@8c0000 {
			compatible = "qcom,geni-se-qup";
			reg = <0x0 0x008c0000 0x0 0x2000>;

Also, this example mixes the SM8550 clock header with the Kaanapali
interconnect header. Could the example use consistent headers and a valid
IBI controller register address?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104122155.3789346-1-mukesh.savaliya@oss.qualcomm.com?part=1