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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] The commit message includes a restrictive corporate confidentiality boilerplate.
- [High] Redefinition of the standard `clock-frequency` property units to kHz instead of Hz.
- [High] Custom properties `nvt,hwmods` and `bus-enable` improperly reinvent standard Devicetree bindings and framework mechanisms.
- [Medium] Missing explicit type definitions for the custom properties `nvt,hwmods` and `bus-enable`.
- [Low] The custom property `bus-enable` is missing a vendor prefix.
- [Low] The compatible string uses an underscore instead of a hyphen.
--

commit b57b0e4f8d5415ed28a61406a5c742ff25ba61db
Author: SP_ISW1_AT@novatek.com.tw <SP_ISW1_AT@novatek.com.tw>

dt-bindings: i2c: add Novatek NT726xx SoC i2c controller

This commit adds device tree binding documentation for the Novatek NT726xx
SoC I2C master controller.

>     CONFIDENTIALITY NOTICE: This message, including attachments, contains information which may be confidential and privileged, and is intended only for use by the addressees designated above. Unless you are the intended recipient, any use, copying, disclosure, or distribution is prohibited. If you have received the message in error, please immediately delete the message and destroy all copies thereof and notify the sender by reply email. 本郵件及任何附件均屬機密，僅供其上指定地址之收件人使用。除非您是指定之收件人，否則請勿使用、複製、揭露或散布本郵件之任何部份。若您錯誤地收到此郵件，請立即回覆電子郵件通知寄件人，並請完全刪除且銷毀本郵件及其複本。

[Severity: High]
Is it possible to remove this confidentiality boilerplate from the commit
message?

Such legal disclaimers conflict with the open-source nature of the Linux
kernel, the GPL-2.0 license, and the Developer's Certificate of Origin.

> 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..0826ee9ec8315
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/novatek,nt726xx-i2c.yaml

[ ... ]

> +properties:
> +  compatible:
> +    const: novatek,nt726xx_i2c

[Severity: Low]
Does this compatible string follow the standard naming conventions?

Device tree guidelines typically suggest using hyphens rather than underscores
in compatible strings, which would make this novatek,nt726xx-i2c instead.

> +  nvt,hwmods:
> +    description: Name of each i2c pin, must be named with "i2cX" (X is
> +                 an integer starting from 0)
> +    minItems: 1

[Severity: High]
Is this custom property intended to encode the I2C adapter number?

Standard practice is to rely on the standard aliases node for this purpose,
like aliases { i2c0 = &i2c0; };, rather than bypassing it with a custom string.

[Severity: Medium]
Could we add an explicit type definition for nvt,hwmods?

Custom vendor properties require type definitions, such as referencing the
string type schema, because constraints like minItems are insufficient for
validation tools to deduce the type.

> +  bus-enable:
> +    description: enable this i2c pin function or not
> +    maxItems: 1

[Severity: High]
Does this property reinvent the standard status property?

Usually, device enablement is handled with status = "okay" or
status = "disabled" rather than creating a custom enable flag.

[Severity: Medium]
Since this is a custom property, shouldn't it include an explicit type
definition?

Additionally, using maxItems: 1 on what appears to be a scalar flag without an
array type definition will cause validation tools to fail.

[Severity: Low]
If this property is kept instead of using the standard status property, should
it include a vendor prefix like nvt,bus-enable to prevent future namespace
collisions with generic bindings?

[ ... ]

> +  clock-frequency:
> +    description: Operation clock frequency of i2c in kHz. Default is 100kHz.
> +    default: 100
> +    maxItems: 1

[Severity: High]
Is it safe to redefine the standard clock-frequency units to kHz here?

The generic i2c-controller schema and device tree specification define
clock-frequency in Hertz (Hz). If a standard device tree configures
clock-frequency = <100000> for 100 kHz, the driver might interpret it as
100,000 kHz, potentially leading to a misconfigured hardware clock divisor and
bus malfunction.

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