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

[Lianfeng Ouyang]

Hi, Krzysztof
Thanks for the review.

> -----邮件原件-----
> 发件人: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
> 发送时间: 2026年5月27日 21:05
> 收件人: Lianfeng Ouyang <lianfeng.ouyang@xxxxxxxxxxxxxxxx>; Andi Shyti
> <andi.shyti@xxxxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>; Krzysztof Kozlowski
> <krzk+dt@xxxxxxxxxx>; Conor Dooley <conor+dt@xxxxxxxxxx>; Mika
> Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>; Andy Shevchenko
> <andriy.shevchenko@xxxxxxxxxxxxxxx>
> 抄送: linux-i2c@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx
> 主题: Re: [PATCH v2 1/3] dt-bindings: i2c: Add StarFive JHB100 I2C
> 
> On 27/05/2026 10:50, lianfeng.ouyang wrote:
> > From: Lianfeng Ouyang <lianfeng.ouyang@xxxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxx>
> > ---
> >  .../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@xxxxxxxxxxxxxxxx>
> > +
> > +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

Sorry, I didn't know I needed to run dt-binding_check before, but now I have 
set up the environment. Starting from the next version, I will run dt-binding_check 
and modify it according to your reply. Then, I will remove the description of 
the required system properties and the common i2c properties

Best Regards,
Lianfeng Ouyang