From: Ryan Chen <ryan_chen@aspeedtech.com> To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Joel Stanley <joel@jms.id.au>, Andrew Jeffery <andrew@aj.id.au>, Philipp Zabel <p.zabel@pengutronix.de>, "openbmc@lists.ozlabs.org" <openbmc@lists.ozlabs.org>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, "linux-aspeed@lists.ozlabs.org" <linux-aspeed@lists.ozlabs.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org> Subject: RE: [PATCH v5 1/2] dt-bindings: i2c: Add support for ASPEED i2Cv2 Date: Tue, 21 Feb 2023 02:43:12 +0000 [thread overview] Message-ID: <TYZPR06MB527427ADCCD29DFD77FB0EE3F2A59@TYZPR06MB5274.apcprd06.prod.outlook.com> (raw) In-Reply-To: <676c7777-635c-cc1f-b919-d33e84a45442@linaro.org> Hello Krzysztof, > -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Monday, February 20, 2023 4:35 PM > To: Ryan Chen <ryan_chen@aspeedtech.com>; Rob Herring > <robh+dt@kernel.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Joel Stanley <joel@jms.id.au>; Andrew > Jeffery <andrew@aj.id.au>; Philipp Zabel <p.zabel@pengutronix.de>; > openbmc@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org; > linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v5 1/2] dt-bindings: i2c: Add support for ASPEED i2Cv2 > > On 20/02/2023 07:17, Ryan Chen wrote: > > AST2600 support new register set for I2Cv2 controller, add bindings > > document to support driver of i2cv2 new register mode controller. > > > > Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com> > > --- > > .../devicetree/bindings/i2c/aspeed,i2cv2.yaml | 83 > > +++++++++++++++++++ > > 1 file changed, 83 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/i2c/aspeed,i2cv2.yaml > > New compatible is okay, but as this is the same controller as old one, this > should go to old binding. > > There are several issues anyway here, but I won't reviewing it except few > obvious cases. > > > > > diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2cv2.yaml > > b/Documentation/devicetree/bindings/i2c/aspeed,i2cv2.yaml > > new file mode 100644 > > index 000000000000..913fb45d5fbe > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2cv2.yaml > > @@ -0,0 +1,83 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/i2c/aspeed,i2cv2.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: ASPEED I2Cv2 Controller on the AST26XX SoCs > > + > > +maintainers: > > + - Ryan Chen <ryan_chen@aspeedtech.com> > > + > > +allOf: > > + - $ref: /schemas/i2c/i2c-controller.yaml# > > + > > +properties: > > + compatible: > > + enum: > > + - aspeed,ast2600-i2cv2 > > + > > + reg: > > + minItems: 1 > > + items: > > + - description: address offset and range of register > > + - description: address offset and range of buffer register > > Why this is optional? Will modify minItems: 1 to 2 > > > + > > + interrupts: > > + maxItems: 1 > > + > > + clocks: > > + maxItems: 1 > > + description: > > + Reference clock for the I2C bus > > + > > + resets: > > + maxItems: 1 > > + > > + clock-frequency: > > + description: > > + Desired I2C bus clock frequency in Hz. default 100khz. > > + > > + multi-master: > > + type: boolean > > + description: > > + states that there is another master active on this bus > > Drop description and type. Just :true. > Since i2c.txt have multi-master will drop it. > > + > > + timeout: > > + type: boolean > > + description: Enable i2c bus timeout for master/slave (35ms) > > Why this is property for DT? It's for sure not bool, but proper type coming from > units. This is i2c controller feature for enable slave mode inactive timeout and also master mode sda/scl auto release timeout. So I will modify to aspeed,timeout: type: boolean description: I2C bus timeout enable for master/slave mode > > + > > + byte-mode: > > + type: boolean > > + description: Force i2c driver use byte mode transmit > > Drop, not a DT property. > > > + > > + buff-mode: > > + type: boolean > > + description: Force i2c driver use buffer mode transmit > > Drop, not a DT property. > The controller support 3 different for transfer. Byte mode: it means step by step to issue transfer. Example i2c read, each step will issue interrupt then enable next step. Sr (start read) | D | D | D | P Buffer mode: it means, the data can prepare into buffer register, then Trigger transfer. So Sr D D D P, only have only 1 interrupt handling. The DMA mode most like with buffer mode, The differ is data prepare in DRAM, than trigger transfer. So, should I modify to aspeed,byte: type: boolean description: Enable i2c controller transfer with byte mode aspeed,buff: type: boolean description: Enable i2c controller transfer with buff mode > > + > > + aspeed,gr: > > + $ref: /schemas/types.yaml#/definitions/phandle > > + description: The phandle of i2c global register node. > > + > > +required: > > + - compatible > > + - reg > > + - interrupts > > + - clocks > > + - resets > > + - aspeed,gr > > + > > +unevaluatedProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > + #include <dt-bindings/clock/ast2600-clock.h> > > + i2c: i2c-bus@80 { > > You did not test the bindings... This is i2c. > I do use command : make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/i2c/aspeed,i2cv2.yaml To test it. is it not correct method? I will modify "i2c: i2c-bus@80" -> "i2c0: i2c@80" > > Best regards, > Krzysztof
WARNING: multiple messages have this Message-ID (diff)
From: Ryan Chen <ryan_chen@aspeedtech.com> To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Joel Stanley <joel@jms.id.au>, Andrew Jeffery <andrew@aj.id.au>, Philipp Zabel <p.zabel@pengutronix.de>, "openbmc@lists.ozlabs.org" <openbmc@lists.ozlabs.org>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, "linux-aspeed@lists.ozlabs.org" <linux-aspeed@lists.ozlabs.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org> Subject: RE: [PATCH v5 1/2] dt-bindings: i2c: Add support for ASPEED i2Cv2 Date: Tue, 21 Feb 2023 02:43:12 +0000 [thread overview] Message-ID: <TYZPR06MB527427ADCCD29DFD77FB0EE3F2A59@TYZPR06MB5274.apcprd06.prod.outlook.com> (raw) In-Reply-To: <676c7777-635c-cc1f-b919-d33e84a45442@linaro.org> Hello Krzysztof, > -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Monday, February 20, 2023 4:35 PM > To: Ryan Chen <ryan_chen@aspeedtech.com>; Rob Herring > <robh+dt@kernel.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Joel Stanley <joel@jms.id.au>; Andrew > Jeffery <andrew@aj.id.au>; Philipp Zabel <p.zabel@pengutronix.de>; > openbmc@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org; > linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v5 1/2] dt-bindings: i2c: Add support for ASPEED i2Cv2 > > On 20/02/2023 07:17, Ryan Chen wrote: > > AST2600 support new register set for I2Cv2 controller, add bindings > > document to support driver of i2cv2 new register mode controller. > > > > Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com> > > --- > > .../devicetree/bindings/i2c/aspeed,i2cv2.yaml | 83 > > +++++++++++++++++++ > > 1 file changed, 83 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/i2c/aspeed,i2cv2.yaml > > New compatible is okay, but as this is the same controller as old one, this > should go to old binding. > > There are several issues anyway here, but I won't reviewing it except few > obvious cases. > > > > > diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2cv2.yaml > > b/Documentation/devicetree/bindings/i2c/aspeed,i2cv2.yaml > > new file mode 100644 > > index 000000000000..913fb45d5fbe > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2cv2.yaml > > @@ -0,0 +1,83 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/i2c/aspeed,i2cv2.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: ASPEED I2Cv2 Controller on the AST26XX SoCs > > + > > +maintainers: > > + - Ryan Chen <ryan_chen@aspeedtech.com> > > + > > +allOf: > > + - $ref: /schemas/i2c/i2c-controller.yaml# > > + > > +properties: > > + compatible: > > + enum: > > + - aspeed,ast2600-i2cv2 > > + > > + reg: > > + minItems: 1 > > + items: > > + - description: address offset and range of register > > + - description: address offset and range of buffer register > > Why this is optional? Will modify minItems: 1 to 2 > > > + > > + interrupts: > > + maxItems: 1 > > + > > + clocks: > > + maxItems: 1 > > + description: > > + Reference clock for the I2C bus > > + > > + resets: > > + maxItems: 1 > > + > > + clock-frequency: > > + description: > > + Desired I2C bus clock frequency in Hz. default 100khz. > > + > > + multi-master: > > + type: boolean > > + description: > > + states that there is another master active on this bus > > Drop description and type. Just :true. > Since i2c.txt have multi-master will drop it. > > + > > + timeout: > > + type: boolean > > + description: Enable i2c bus timeout for master/slave (35ms) > > Why this is property for DT? It's for sure not bool, but proper type coming from > units. This is i2c controller feature for enable slave mode inactive timeout and also master mode sda/scl auto release timeout. So I will modify to aspeed,timeout: type: boolean description: I2C bus timeout enable for master/slave mode > > + > > + byte-mode: > > + type: boolean > > + description: Force i2c driver use byte mode transmit > > Drop, not a DT property. > > > + > > + buff-mode: > > + type: boolean > > + description: Force i2c driver use buffer mode transmit > > Drop, not a DT property. > The controller support 3 different for transfer. Byte mode: it means step by step to issue transfer. Example i2c read, each step will issue interrupt then enable next step. Sr (start read) | D | D | D | P Buffer mode: it means, the data can prepare into buffer register, then Trigger transfer. So Sr D D D P, only have only 1 interrupt handling. The DMA mode most like with buffer mode, The differ is data prepare in DRAM, than trigger transfer. So, should I modify to aspeed,byte: type: boolean description: Enable i2c controller transfer with byte mode aspeed,buff: type: boolean description: Enable i2c controller transfer with buff mode > > + > > + aspeed,gr: > > + $ref: /schemas/types.yaml#/definitions/phandle > > + description: The phandle of i2c global register node. > > + > > +required: > > + - compatible > > + - reg > > + - interrupts > > + - clocks > > + - resets > > + - aspeed,gr > > + > > +unevaluatedProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > + #include <dt-bindings/clock/ast2600-clock.h> > > + i2c: i2c-bus@80 { > > You did not test the bindings... This is i2c. > I do use command : make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/i2c/aspeed,i2cv2.yaml To test it. is it not correct method? I will modify "i2c: i2c-bus@80" -> "i2c0: i2c@80" > > Best regards, > Krzysztof _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2023-02-21 2:43 UTC|newest] Thread overview: 68+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-02-20 6:17 [PATCH v5 0/2] Add ASPEED AST2600 I2Cv2 controller driver Ryan Chen 2023-02-20 6:17 ` Ryan Chen 2023-02-20 6:17 ` [PATCH v5 1/2] dt-bindings: i2c: Add support for ASPEED i2Cv2 Ryan Chen 2023-02-20 6:17 ` Ryan Chen 2023-02-20 8:28 ` Jeremy Kerr 2023-02-20 8:28 ` Jeremy Kerr 2023-02-20 9:50 ` Ryan Chen 2023-02-20 9:50 ` Ryan Chen 2023-02-20 10:43 ` Krzysztof Kozlowski 2023-02-20 10:43 ` Krzysztof Kozlowski 2023-02-20 11:24 ` Jeremy Kerr 2023-02-20 11:24 ` Jeremy Kerr 2023-02-21 3:32 ` Ryan Chen 2023-02-21 3:32 ` Ryan Chen 2023-02-21 9:43 ` Krzysztof Kozlowski 2023-02-21 9:43 ` Krzysztof Kozlowski 2023-02-22 1:14 ` Dhananjay Phadke 2023-02-22 1:14 ` Dhananjay Phadke 2023-02-22 1:30 ` Jeremy Kerr 2023-02-22 1:30 ` Jeremy Kerr 2023-02-20 8:35 ` Krzysztof Kozlowski 2023-02-20 8:35 ` Krzysztof Kozlowski 2023-02-21 2:43 ` Ryan Chen [this message] 2023-02-21 2:43 ` Ryan Chen 2023-02-21 9:40 ` Krzysztof Kozlowski 2023-02-21 9:40 ` Krzysztof Kozlowski 2023-02-21 10:42 ` Ryan Chen 2023-02-21 10:42 ` Ryan Chen 2023-02-21 11:04 ` Krzysztof Kozlowski 2023-02-21 11:04 ` Krzysztof Kozlowski 2023-02-22 2:59 ` Ryan Chen 2023-02-22 2:59 ` Ryan Chen 2023-02-22 8:25 ` Krzysztof Kozlowski 2023-02-22 8:25 ` Krzysztof Kozlowski 2023-02-22 10:31 ` Ryan Chen 2023-02-22 10:31 ` Ryan Chen 2023-02-22 10:35 ` Krzysztof Kozlowski 2023-02-22 10:35 ` Krzysztof Kozlowski 2023-02-22 10:47 ` Ryan Chen 2023-02-22 10:47 ` Ryan Chen 2023-02-23 9:28 ` Krzysztof Kozlowski 2023-02-23 9:28 ` Krzysztof Kozlowski 2023-02-23 10:25 ` Ryan Chen 2023-02-23 10:25 ` Ryan Chen 2023-02-20 6:17 ` [PATCH v5 2/2] i2c: aspeed: support ast2600 i2cv2 new register mode driver Ryan Chen 2023-02-20 6:17 ` Ryan Chen 2023-02-20 8:43 ` Krzysztof Kozlowski 2023-02-20 8:43 ` Krzysztof Kozlowski 2023-02-20 10:44 ` Krzysztof Kozlowski 2023-02-20 10:44 ` Krzysztof Kozlowski 2023-02-22 3:36 ` Ryan Chen 2023-02-22 3:36 ` Ryan Chen 2023-02-22 8:28 ` Krzysztof Kozlowski 2023-02-22 8:28 ` Krzysztof Kozlowski 2023-02-23 0:58 ` Ryan Chen 2023-02-23 0:58 ` Ryan Chen 2023-02-23 9:30 ` Krzysztof Kozlowski 2023-02-23 9:30 ` Krzysztof Kozlowski 2023-02-20 8:30 ` [PATCH v5 0/2] Add ASPEED AST2600 I2Cv2 controller driver Krzysztof Kozlowski 2023-02-20 8:30 ` Krzysztof Kozlowski 2023-02-20 9:56 ` Ryan Chen 2023-02-20 9:56 ` Ryan Chen 2023-02-20 10:35 ` Krzysztof Kozlowski 2023-02-20 10:35 ` Krzysztof Kozlowski 2023-02-21 1:12 ` Ryan Chen 2023-02-21 1:12 ` Ryan Chen 2023-02-21 9:38 ` Krzysztof Kozlowski 2023-02-21 9:38 ` Krzysztof Kozlowski
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=TYZPR06MB527427ADCCD29DFD77FB0EE3F2A59@TYZPR06MB5274.apcprd06.prod.outlook.com \ --to=ryan_chen@aspeedtech.com \ --cc=andrew@aj.id.au \ --cc=joel@jms.id.au \ --cc=krzysztof.kozlowski+dt@linaro.org \ --cc=krzysztof.kozlowski@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-aspeed@lists.ozlabs.org \ --cc=linux-kernel@vger.kernel.org \ --cc=openbmc@lists.ozlabs.org \ --cc=p.zabel@pengutronix.de \ --cc=robh+dt@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.