Re: [PATCH 1/3] dt-bindings: Add InvenSense ICM-42370-p accelerometer

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Medium] The binding intentionally omits support for SPI and I3C protocols, breaking schema validation for valid hardware configurations.
- [Medium] The `interrupts` property is restricted to a single item, contradicting the device's 2 programmable interrupts.
- [Medium] The schema is missing the `mount-matrix` property, which is essential for 3-axis accelerometers.
- [Low] The binding includes a fragile link to a staging (UAT) website instead of a production URL.
- [Low] The compatible string drops the 'P' suffix from the part number, risking future naming collisions.
--

--- Patch [1]: [PATCH 1/3] dt-bindings: Add InvenSense ICM-42370-p accelerometer ---
commit 12d3935884516d76927755afe69d0ed970ee7417
Author: Kanak Shilledar <[email protected]>

dt-bindings: Add InvenSense ICM-42370-p accelerometer

Adds devicetree documentation for the ICM-42370-P 3-axis accelerometer.
Support is currently only described for I2C, leaving out SPI and I3C and
reset for future work.

> diff --git a/Documentation/devicetree/bindings/iio/accel/invensense,icm42370.yaml b/Documentation/devicetree/bindings/iio/accel/invensense,icm42370.yaml
> new file mode 100644
> index 0000000000000..561e798b911ba
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/accel/invensense,icm42370.yaml
[ ... ]
> +description: |
> +  3-axis accelerometer MotionTracking device.
> +
> +  It supports I3C, I2C and SPI serial communication, has a 2.25kB FIFO
> +  and 2 programmable interrupts with low-power wake-on-motion support.
> +
> +  It also has programmable filters and an embedded temperature sensor.
> +
> +  https://uat.invensense.com/en-us/products/3-axis/icm-42370-p

[Severity: Low]
This isn't a bug, but this documentation link points to a staging (UAT)
website instead of a production URL. Could this be updated to the production
URL?

> +
> +properties:
> +  compatible:
> +    const: invensense,icm42370

[Severity: Low]
This isn't a bug, but the compatible string drops the 'P' suffix from the
part number. Does this risk future naming collisions if a non-P variant
exists or is released? Could the compatible string be updated to strictly
match the part number invensense,icm42370p?

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1

[Severity: Medium]
Will this limitation of maxItems to 1 conflict with valid hardware designs
that route both of the device's interrupt lines to the host? Should the
schema allow up to 2 items for this property and define interrupt-names?

> +
> +  drive-open-drain:
> +    type: boolean
> +
> +  vdd-supply:
> +    description: Regulator operating range between 1.71V to 3.6V.
> +
> +  vddio-supply:
> +    description: Regulator operating range between 1.71V to 3.6V.
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +unevaluatedProperties: false

[Severity: Medium]
By omitting support for SPI and I3C protocols and setting
unevaluatedProperties to false without including SPI schema references, won't
this cause schema validation errors for valid hardware configurations? Standard
properties like spi-max-frequency or spi-cpol will be incorrectly rejected.
Should the binding describe all hardware capabilities rather than just the
tested protocols?

[Severity: Medium]
Does the omission of the mount-matrix property, combined with
unevaluatedProperties being set to false, mean that device trees describing
the sensor's physical orientation on the PCB will also fail validation? Could
mount-matrix be explicitly declared as true?

> +
> +examples:
> +  - |
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1