Re: [PATCH v2 1/3] dt-bindings: i2c: Add StarFive JHB100 I2C

[Krzysztof Kozlowski <krzk@kernel.org>]

On 27/05/2026 10:50, lianfeng.ouyang wrote:
> From: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com>
> 
> Add device tree bindings for the Starfive I2C controller
> and its implementation
> 
> The binding defines two platform-specific compatibles for the StarFive
> JHB100 implementation:
> - "starfive,jhb100-i2c-master"
> - "starfive,jhb100-i2c-slave"

You might get the same questions as v1, till you solve them. Why do you
add device role to the compatible?

Do not explain  WHAT you did here. Explain the background and why you
did that way.

Also, use undeprecated naming, not master/slave.

> 
> The controller supports standard I2C and SMBus protocols, programmable
> FIFO depths, and optional SMBus Alert routing. The binding documents
> the necessary clocks, resets, and timing properties.
> 
> Signed-off-by: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com>
> ---
>  .../bindings/i2c/starfive,jhb100-i2c.yaml     | 128 ++++++++++++++++++
>  1 file changed, 128 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/starfive,jhb100-i2c.yaml
> 
> diff --git a/Documentation/devicetree/bindings/i2c/starfive,jhb100-i2c.yaml b/Documentation/devicetree/bindings/i2c/starfive,jhb100-i2c.yaml
> new file mode 100644
> index 000000000000..c8631348121c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/starfive,jhb100-i2c.yaml
> @@ -0,0 +1,128 @@
> +# 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/starfive,jhb100-i2c.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: StarFive jhb100 I2C Controller
> +
> +maintainers:
> +  - Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com>
> +
> +allOf:
> +  - $ref: /schemas/i2c/i2c-controller.yaml#
> +
> +properties:
> +  compatible:
> +    description: |
> +      Must be one of:
> +      - "starfive,jhb100-i2c-master" for master mode controller
> +      - "starfive,jhb100-i2c-slave" for slave mode controller

Drop description, redundant. Schema tells that.

> +    enum:
> +      - starfive,jhb100-i2c-master
> +      - starfive,jhb100-i2c-slave
> +
> +  reg:
> +    maxItems: 1
> +    description: StarFive I2C controller memory mapped registers

Drop description, redundant.

> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    minItems: 2
> +    maxItems: 2

Drop both, redundant.

> +    items:
> +      - description: I2C controller reference clock source
> +      - description: APB interface clock source
> +
> +  clock-names:
> +    minItems: 2
> +    maxItems: 2

Same. From where did you take such syntax...

> +    items:
> +      - const: ref
> +      - const: pclk
> +
> +  resets:
> +    maxItems: 1
> +    description: Phandle to the reset controller for the I2C controller

Drop description. Or say something useful.

> +
> +  clock-frequency:
> +    description: Desired I2C bus clock frequency in Hz
> +    enum: [100000, 400000, 1000000, 3400000]

Aren't you duplicating constraints from dtschema?

> +    default: 400000
> +
> +  i2c-sda-hold-time-ns:

So you added a generic property - where is it documented? Generic
properties must be in common schema or dtschema.

And please prove that none of the generic properties are suitable.

> +    $ref: /schemas/types.yaml#/definitions/uint32

I don't think you tested it. And this concludes my review. I finished
here. Please do not send untested bindings.


Best regards,
Krzysztof