Re: [PATCH v1 1/3] dt-bindings: i2c: snps,dwc-i2c: Add StarFive JHB100 bindings

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The patch introduces a redundant device tree binding schema (`snps,dwc-i2c.yaml`) and a duplicate generic compatible string (`snps,dwc-i2c`) for the Synopsys DesignWare I2C controller.
- [Medium] Compatible strings are incorrectly used to encode the operational mode (master vs. slave) instead of describing the hardware IP variant.
- [Medium] FIFO depth properties (`dwc-i2c-tx-fifo-depth` and `dwc-i2c-rx-fifo-depth`) use an invalid and unregistered vendor prefix.
- [Medium] The custom property `starfive,mctp-i2c-ms` improperly uses the standard `-ms` suffix for a `phandle` reference and lacks an explicit type definition.
- [Medium] Missing constraints for the slave-only property `starfive,mctp-i2c-ms`.
--

commit 7e9b926c17cf1f417236a77e427787e7ba5331b1
Author: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com>

dt-bindings: i2c: snps,dwc-i2c: Add StarFive JHB100 bindings

Add device tree bindings for the Synopsys DesignWare Core (DWC) I2C
controller and its StarFive JHB100 implementation. The binding introduces
a new compatible string for the generic IP and two platform-specific
compatibles.

> diff --git a/Documentation/devicetree/bindings/i2c/snps,dwc-i2c.yaml b/Documentation/devicetree/bindings/i2c/snps,dwc-i2c.yaml
> new file mode 100644
> index 0000000000000..7227f24f7cbe0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/snps,dwc-i2c.yaml
> @@ -0,0 +1,120 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) 2024 StarFive Technology Co., Ltd.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/i2c/snps,dwc-i2c.yaml#

[Severity: High]
Does this introduce a redundant binding schema?

The Synopsys DesignWare I2C controller is already documented under
Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml with the
generic compatible string snps,designware-i2c.

Creating a parallel schema for the same hardware block might fracture the
hardware description and bypass existing constraints. Could the StarFive
JHB100 variants be added to the existing designware-i2c schema instead?

[ ... ]

> +properties:
> +  compatible:
> +    oneOf:
> +      - description: Generic Synopsys DWC I2C controller
> +        const: snps,dwc-i2c
> +      - description: StarFive JHB100 I2C master controller
> +        items:
> +          - const: starfive,jhb100-dwc-i2c-master
> +          - const: snps,dwc-i2c
> +      - description: StarFive JHB100 I2C slave controller
> +        items:
> +          - const: starfive,jhb100-dwc-i2c-slave
> +          - const: snps,dwc-i2c

[Severity: Medium]
Are these compatible strings encoding the software operational mode rather
than describing the hardware IP variant?

Since the hardware natively supports both modes, shouldn't the operating
mode be dynamically determined by the framework or described using standard
subsystem properties like an i2c-slave sub-node?

[ ... ]

> +  starfive,mctp-i2c-ms:
> +    description: |
> +      The property should contain reference to the master node associated with the slave.
> +      This value is only used in slave mode, especially for MCTP application.

[Severity: Medium]
Does the -ms suffix conflict with dt-schema evaluation? Properties ending
with -ms are strictly evaluated as a time unit in milliseconds, but the
description indicates this is a phandle reference to a master node.

Also, is there an explicit type definition missing here? Custom properties
intended as a phandle typically require an explicit
$ref: /schemas/types.yaml#/definitions/phandle.

[Severity: Medium]
Furthermore, since the description states this value is only used in slave
mode, should there be an allOf/if-then block added to enforce this
restriction based on the compatible string?

> +  dwc-i2c-tx-fifo-depth:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      The property describes the tx fifo depth.
> +    default: 8
> +
> +  dwc-i2c-rx-fifo-depth:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      The property describes the rx fifo depth.

[Severity: Medium]
Do these properties use an invalid vendor prefix?

Custom device tree properties must begin with a recognized vendor prefix
followed by a comma. Using dwc-i2c- without a comma violates core device
tree naming constraints. Should these use a registered prefix like snps,
or starfive, instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521034340.27837-1-lianfeng.ouyang@starfivetech.com?part=1