Re: [PATCH v2 1/4] spi: dt-bindings: describe SA8255p

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unchecked `device_get_match_data()` return value causes a NULL pointer dereference if bound manually.
- [Medium] The DT binding example provides `dmas` but omits the strictly required `dma-names` property, and the schema lacks structural dependencies for it.
- [Medium] The `qcom,sa8255p-geni-spi` binding strictly requires a single `compatible` string, rejecting the valid `sa8797p` fallback array.
--

commit a89af5cc4b98b3af1b2a30423bbe32d7faf13e17
Author: Praveen Talari <praveen.talari@oss.qualcomm.com>

spi: dt-bindings: describe SA8255p

Add DT bindings for the QUP GENI SPI controller on sa8255p platform.
SA8255p platform abstracts resources such as clocks, interconnect and
GPIO pins configuration in Firmware. SCMI power and perf protocols are
utilized to request resource configurations.

> diff --git a/Documentation/devicetree/bindings/spi/qcom,sa8255p-geni-spi.yaml b/Documentation/devicetree/bindings/spi/qcom,sa8255p-geni-spi.yaml
> new file mode 100644
> index 0000000000000..8c6825b8ddb41
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/qcom,sa8255p-geni-spi.yaml
> @@ -0,0 +1,63 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/spi/qcom,sa8255p-geni-spi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: GENI based Qualcomm Universal Peripheral (QUP) Serial Peripheral Interface (SPI)
> +
> +maintainers:
> +  - Praveen Talari <praveen.talari@oss.qualcomm.com>
> +
> +properties:
> +  compatible:
> +    const: qcom,sa8255p-geni-spi

[Severity: Medium]
Will this strict constraint reject the qcom,sa8797p-geni-spi fallback array?

The parent QUP wrapper binding (qcom,sa8255p-geni-se-qup.yaml) explicitly
defines SPI child nodes matching "spi@[0-9a-f]+$" to have:
compatible: oneOf: - const: qcom,sa8255p-geni-spi - items: - const: qcom,sa8797p-geni-spi - const: qcom,sa8255p-geni-spi

When a device tree node for the sa8797p variant provides the multi-string
fallback array, dtbs_check will fail validation because the array does not
match this strict const constraint. Should this be updated to use a oneOf
block containing the fallback array?

[ ... ]

> +  dmas:
> +    maxItems: 2
> +
> +  dma-names:
> +    items:
> +      - const: tx
> +      - const: rx

[Severity: Medium]
Should there be a dependencies constraint here to enforce dma-names when
dmas is provided?

Without a constraint like dependencies: dmas: [ dma-names ], it is possible
to configure dmas without dma-names. Since the driver requires channels by
name, missing dma-names could cause issues that structural enforcement
would prevent.

[ ... ]

> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/dma/qcom-gpi.h>
> +
> +    spi@888000 {
> +        compatible = "qcom,sa8255p-geni-spi";
> +        reg = <0x888000 0x4000>;
> +        interrupts = <GIC_SPI 584 IRQ_TYPE_LEVEL_HIGH>;
> +        dmas = <&gpi_dma0 0 0 QCOM_GPI_SPI>,
> +               <&gpi_dma0 1 0 QCOM_GPI_SPI>;
> +        power-domains = <&scmi0_pd 0>, <&scmi0_dvfs 0>;
> +        power-domain-names = "power", "perf";
> +    };

[Severity: Medium]
Is the dma-names property missing from this example?

The spi-geni-qcom.c driver requires DMA channels by name and acquires them
using dma_request_chan(dev, "tx") and dma_request_chan(dev, "rx"). Because
the example configures dmas but omits the dma-names array entirely,
developers copying this as a reference might silently fail to initialize
DMA and fall back to PIO mode.


Additionally, regarding a later commit in this series ("spi: qcom-geni:
Enable SPI on SA8255p Qualcomm platforms"):

[Severity: High]
Does spi_geni_probe() safely handle a manual bind?

If the device is bound manually via sysfs (bind) or uses driver_override
without a matched OF/ACPI node, device_get_match_data() returns NULL.

The function sets:
mas->dev_data = device_get_match_data(&pdev->dev);

Followed immediately by a dereference:
ret = mas->dev_data->resources_init(&mas->se);

Will this result in a NULL pointer dereference panic under those conditions?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260530-enable-spi-on-sa8255p-v2-0-17574601bd63@oss.qualcomm.com?part=1