Re: [PATCH v3 2/3] dt-bindings: i2c: Add Novatek NT726xx SoC I2C controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Low] DT Binding Anti-Pattern: Usage of custom `novatek,hwmods` string property to assign unique instance names.
- [Low] Schema Type Violation: The boolean property `novatek,stbc-controllable` incorrectly specifies an integer default value of `0`.
- [Low] DT Schema Validation Failure: I2C controller examples use invalid `#address-cells` and `#size-cells` values.
--

--- Patch [2]: [PATCH v3 2/3] dt-bindings: i2c: Add Novatek NT726xx SoC I2C controller ---
commit fc4c1ba789209e1e7e8f1fe88e1d6a2a1a7eec66
Author: Ben Huang <Ben_Huang@novatek.com.tw>

dt-bindings: i2c: Add Novatek NT726xx SoC I2C controller
    
Add device tree documentation for Novatek NT726xx SoC I2C controller.

> diff --git a/Documentation/devicetree/bindings/i2c/novatek,nt726xx-i2c.yaml b/Documentation/devicetree/bindings/i2c/novatek,nt726xx-i2c.yaml
> new file mode 100644
> index 0000000000000..866589c5ae51a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/novatek,nt726xx-i2c.yaml
[ ... ]
> +  novatek,hwmods:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    description: Name of each i2c pin, must be named with "i2cX". (X is
> +                 an integer starting from 0, must be unique)

[Severity: Low]
Does this violate standard Device Tree schema practices? 

Hardware nodes should typically not encode software logical indices. Instances
are usually mapped dynamically via the /aliases node (e.g., i2c0 = &i2c0;) or
derived from the reg base address rather than using a custom string property
like novatek,hwmods.

> +
> +  novatek,stbc-controllable:
> +    type: boolean
> +    description: Set if this i2c master, named as `stbc-i2c`, is
> +      controllable by stbc (Standby controller, actually is an 8051
> +      micro-processor) on Novatek NT726xx SoCs.
> +      For these `stbc-i2c`s, the driven clock is only 12 MHz and the
> +      authentication to stbc must be executed before controlling the
> +      registers.
> +
> +    default: 0

[Severity: Low]
Is this default value valid for a boolean property? 

Boolean properties in DT schema are evaluated by their presence or absence
and shouldn't accept an integer value. This will likely fail schema validation
during dt_binding_check. If a default is needed, it should be false.

[ ... ]
> +examples:
> +  - |
> +    i2c0: i2c@10000000 {
> +      compatible = "novatek,nt72600-i2c";
> +      reg = <0x0 0x10000000 0x0 0x100>;
> +      interrupts = <0 67 4>;
> +      clock-frequency = <400000>;
> +      novatek,hwmods = "i2c0";
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +    };
> +
> +    stbc-i2c1: i2c@20000000 {
> +      compatible = "novatek,nt72600-i2c";
> +      reg = <0x0 0x20000000 0x0 0x100>;
> +      interrupts = <0 77 4>;
> +      novatek,hwmods = "i2c1";
> +      novatek,stbc-controllable;
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +    };

[Severity: Low]
Do these cell counts pass core device tree schema validation for I2C controllers?

I2C controllers dictate the cell sizes for their child devices and normally
enforce #address-cells = <1> (for the I2C address) and #size-cells = <0> (since
child devices are not memory mapped). Both examples seem to be using <2>.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715073521.147610-1-Nina_Kuo@novatek.com.tw?part=2