| Message ID | 20220516064900.30517-3-ryan_chen@aspeedtech.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add ASPEED AST2600 I2C new controller driver | expand |
Hi Ryan, On Mon, 16 May 2022, at 16:18, ryan_chen wrote: > AST2600 support new register set for I2C controller, add bindings document > to support driver of i2c new register mode controller > > Signed-off-by: ryan_chen <ryan_chen@aspeedtech.com> > --- > .../bindings/i2c/aspeed,i2c-ast2600.ymal | 78 +++++++++++++++++++ > 1 file changed, 78 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.ymal > > diff --git > a/Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.ymal > b/Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.ymal > new file mode 100644 > index 000000000000..7c75f5bac24f > --- /dev/null > +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.ymal > @@ -0,0 +1,78 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/i2c/aspeed,i2c-ast2600.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: AST2600 I2C Controller on the AST26XX SoCs Device Tree Bindings > + > +maintainers: > + - Ryan Chen <ryan_chen@aspeedtech.com> > + > +allOf: > + - $ref: /schemas/i2c/i2c-controller.yaml# > + > +properties: > + compatible: > + enum: > + - aspeed,ast2600-i2c The original driver uses e.g. aspeed,ast2500-i2c-bus for the subordinate controllers. While the register layout changes, I'd prefer we try to use the existing compatibles rather than introducing a new set and causing some confusion. Further, what you're proposing here is effectively being used to select the driver implementation, which isn't the purpose of the devicetree. My preference would be to reuse the existing compatibles and instead select the driver implementation via Kconfig. Or, if we can figure out some way to do so, support both register interfaces in the one driver implementation and fall back to the old register interface where the new one isn't available (I don't think this is feasible though). > + > + reg: > + minItems: 1 > + items: > + - description: address offset and range of bus > + - description: address offset and range of bus buffer > + > + interrupts: > + maxItems: 1 > + > + clocks: > + maxItems: 1 > + description: > + root clock of bus, should reference the APB > + clock in the second cell > + > + resets: > + maxItems: 1 > + > + bus-frequency: > + minimum: 500 > + maximum: 2000000 > + default: 100000 > + description: frequency of the bus clock in Hz defaults to 100 kHz > when not > + specified > + > + multi-master: > + type: boolean > + description: > + states that there is another master active on this bus > + > +required: > + - reg > + - compatible > + - clocks > + - resets > + > +unevaluatedProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + #include <dt-bindings/clock/ast2600-clock.h> > + > + i2c_gr: i2c-global-regs@0 { > + compatible = "aspeed,ast2600-i2c-global", "syscon"; > + reg = <0x0 0x20>; > + resets = <&syscon ASPEED_RESET_I2C>; > + }; > + > + i2c0: i2c-bus@80 { > + #address-cells = <1>; > + #size-cells = <0>; > + #interrupt-cells = <1>; > + compatible = "aspeed,ast2600-i2c-bus"; This isn't quite right with respect to your binding description above :) Andrew
Hello Andrew, > -----Original Message----- > From: Andrew Jeffery <andrew@aj.id.au> > Sent: Friday, July 29, 2022 10:29 AM > To: Ryan Chen <ryan_chen@aspeedtech.com>; Joel Stanley <joel@jms.id.au>; > Philipp Zabel <p.zabel@pengutronix.de>; linux-arm-kernel@lists.infradead.org; > linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org; > openbmc@lists.ozlabs.org > Cc: BMC-SW <BMC-SW@aspeedtech.com> > Subject: Re: [PATCH v3 2/3] dt-bindings: i2c-ast2600: Add bindings for AST2600 > i2C driver > > Hi Ryan, > > On Mon, 16 May 2022, at 16:18, ryan_chen wrote: > > AST2600 support new register set for I2C controller, add bindings > > document to support driver of i2c new register mode controller > > > > Signed-off-by: ryan_chen <ryan_chen@aspeedtech.com> > > --- > > .../bindings/i2c/aspeed,i2c-ast2600.ymal | 78 > +++++++++++++++++++ > > 1 file changed, 78 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.ymal > > > > diff --git > > a/Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.ymal > > b/Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.ymal > > new file mode 100644 > > index 000000000000..7c75f5bac24f > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.ymal > > @@ -0,0 +1,78 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/i2c/aspeed,i2c-ast2600.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: AST2600 I2C Controller on the AST26XX SoCs Device Tree > > +Bindings > > + > > +maintainers: > > + - Ryan Chen <ryan_chen@aspeedtech.com> > > + > > +allOf: > > + - $ref: /schemas/i2c/i2c-controller.yaml# > > + > > +properties: > > + compatible: > > + enum: > > + - aspeed,ast2600-i2c > > The original driver uses e.g. aspeed,ast2500-i2c-bus for the subordinate > controllers. While the register layout changes, I'd prefer we try to use the > existing compatibles rather than introducing a new set and causing some > confusion. > > Further, what you're proposing here is effectively being used to select the > driver implementation, which isn't the purpose of the devicetree. > > My preference would be to reuse the existing compatibles and instead select > the driver implementation via Kconfig. Or, if we can figure out some way to do > so, support both register interfaces in the one driver implementation and fall > back to the old register interface where the new one isn't available (I don't > think this is feasible though). > Yes, that the reason go for another driver ast2600 to implement. Like others SOC driver implement different generation have diff driver in Kconfig and Makefile. Example : https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/Makefile#L82-L84 > > + > > + reg: > > + minItems: 1 > > + items: > > + - description: address offset and range of bus > > + - description: address offset and range of bus buffer > > + > > + interrupts: > > + maxItems: 1 > > + > > + clocks: > > + maxItems: 1 > > + description: > > + root clock of bus, should reference the APB > > + clock in the second cell > > + > > + resets: > > + maxItems: 1 > > + > > + bus-frequency: > > + minimum: 500 > > + maximum: 2000000 > > + default: 100000 > > + description: frequency of the bus clock in Hz defaults to 100 kHz > > when not > > + specified > > + > > + multi-master: > > + type: boolean > > + description: > > + states that there is another master active on this bus > > + > > +required: > > + - reg > > + - compatible > > + - clocks > > + - resets > > + > > +unevaluatedProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > + #include <dt-bindings/clock/ast2600-clock.h> > > + > > + i2c_gr: i2c-global-regs@0 { > > + compatible = "aspeed,ast2600-i2c-global", "syscon"; > > + reg = <0x0 0x20>; > > + resets = <&syscon ASPEED_RESET_I2C>; > > + }; > > + > > + i2c0: i2c-bus@80 { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + #interrupt-cells = <1>; > > + compatible = "aspeed,ast2600-i2c-bus"; > > This isn't quite right with respect to your binding description above :) Yes, the compatible need to be " aspeed,ast2600-i2c" is that your point ? If yes, I will start for v4. > Andrew
On Fri, 29 Jul 2022, at 12:33, Ryan Chen wrote: > Hello Andrew, > >> -----Original Message----- >> From: Andrew Jeffery <andrew@aj.id.au> >> Sent: Friday, July 29, 2022 10:29 AM >> To: Ryan Chen <ryan_chen@aspeedtech.com>; Joel Stanley <joel@jms.id.au>; >> Philipp Zabel <p.zabel@pengutronix.de>; linux-arm-kernel@lists.infradead.org; >> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org; >> openbmc@lists.ozlabs.org >> Cc: BMC-SW <BMC-SW@aspeedtech.com> >> Subject: Re: [PATCH v3 2/3] dt-bindings: i2c-ast2600: Add bindings for AST2600 >> i2C driver >> >> Hi Ryan, >> >> On Mon, 16 May 2022, at 16:18, ryan_chen wrote: >> > AST2600 support new register set for I2C controller, add bindings >> > document to support driver of i2c new register mode controller >> > >> > Signed-off-by: ryan_chen <ryan_chen@aspeedtech.com> >> > --- >> > .../bindings/i2c/aspeed,i2c-ast2600.ymal | 78 >> +++++++++++++++++++ >> > 1 file changed, 78 insertions(+) >> > create mode 100644 >> > Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.ymal >> > >> > diff --git >> > a/Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.ymal >> > b/Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.ymal >> > new file mode 100644 >> > index 000000000000..7c75f5bac24f >> > --- /dev/null >> > +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.ymal >> > @@ -0,0 +1,78 @@ >> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2 >> > +--- >> > +$id: http://devicetree.org/schemas/i2c/aspeed,i2c-ast2600.yaml# >> > +$schema: http://devicetree.org/meta-schemas/core.yaml# >> > + >> > +title: AST2600 I2C Controller on the AST26XX SoCs Device Tree >> > +Bindings >> > + >> > +maintainers: >> > + - Ryan Chen <ryan_chen@aspeedtech.com> >> > + >> > +allOf: >> > + - $ref: /schemas/i2c/i2c-controller.yaml# >> > + >> > +properties: >> > + compatible: >> > + enum: >> > + - aspeed,ast2600-i2c >> >> The original driver uses e.g. aspeed,ast2500-i2c-bus for the subordinate >> controllers. While the register layout changes, I'd prefer we try to use the >> existing compatibles rather than introducing a new set and causing some >> confusion. >> >> Further, what you're proposing here is effectively being used to select the >> driver implementation, which isn't the purpose of the devicetree. >> >> My preference would be to reuse the existing compatibles and instead select >> the driver implementation via Kconfig. Or, if we can figure out some way to do >> so, support both register interfaces in the one driver implementation and fall >> back to the old register interface where the new one isn't available (I don't >> think this is feasible though). >> > Yes, that the reason go for another driver ast2600 to implement. > Like others SOC driver implement different generation have diff driver > in Kconfig > and Makefile. > Example : > https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/Makefile#L82-L84 > > >> > + >> > + reg: >> > + minItems: 1 >> > + items: >> > + - description: address offset and range of bus >> > + - description: address offset and range of bus buffer >> > + >> > + interrupts: >> > + maxItems: 1 >> > + >> > + clocks: >> > + maxItems: 1 >> > + description: >> > + root clock of bus, should reference the APB >> > + clock in the second cell >> > + >> > + resets: >> > + maxItems: 1 >> > + >> > + bus-frequency: >> > + minimum: 500 >> > + maximum: 2000000 >> > + default: 100000 >> > + description: frequency of the bus clock in Hz defaults to 100 kHz >> > when not >> > + specified >> > + >> > + multi-master: >> > + type: boolean >> > + description: >> > + states that there is another master active on this bus >> > + >> > +required: >> > + - reg >> > + - compatible >> > + - clocks >> > + - resets >> > + >> > +unevaluatedProperties: false >> > + >> > +examples: >> > + - | >> > + #include <dt-bindings/interrupt-controller/arm-gic.h> >> > + #include <dt-bindings/clock/ast2600-clock.h> >> > + >> > + i2c_gr: i2c-global-regs@0 { >> > + compatible = "aspeed,ast2600-i2c-global", "syscon"; >> > + reg = <0x0 0x20>; >> > + resets = <&syscon ASPEED_RESET_I2C>; >> > + }; >> > + >> > + i2c0: i2c-bus@80 { >> > + #address-cells = <1>; >> > + #size-cells = <0>; >> > + #interrupt-cells = <1>; >> > + compatible = "aspeed,ast2600-i2c-bus"; >> >> This isn't quite right with respect to your binding description above :) > Yes, the compatible need to be " aspeed,ast2600-i2c" is that your point ? Yes, but only if we agree that we should have different compatibles for the different drivers. I'm not convinced about that yet. I think it's enough to have different Kconfig symbols, and select the old driver in aspeed_g4_defconfig, and the new driver in aspeed_g5_defconfig. Won't that gives us the right outcome without requiring a new set of compatibles? Andrew
Hello, > -----Original Message----- > From: Andrew Jeffery <andrew@aj.id.au> > Sent: Friday, July 29, 2022 11:13 AM > To: Ryan Chen <ryan_chen@aspeedtech.com>; Joel Stanley <joel@jms.id.au>; > Philipp Zabel <p.zabel@pengutronix.de>; linux-arm-kernel@lists.infradead.org; > linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org; > openbmc@lists.ozlabs.org > Cc: BMC-SW <BMC-SW@aspeedtech.com> > Subject: Re: [PATCH v3 2/3] dt-bindings: i2c-ast2600: Add bindings for AST2600 > i2C driver > > > > On Fri, 29 Jul 2022, at 12:33, Ryan Chen wrote: > > Hello Andrew, > > > >> -----Original Message----- > >> From: Andrew Jeffery <andrew@aj.id.au> > >> Sent: Friday, July 29, 2022 10:29 AM > >> To: Ryan Chen <ryan_chen@aspeedtech.com>; Joel Stanley > >> <joel@jms.id.au>; Philipp Zabel <p.zabel@pengutronix.de>; > >> linux-arm-kernel@lists.infradead.org; > >> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org; > >> openbmc@lists.ozlabs.org > >> Cc: BMC-SW <BMC-SW@aspeedtech.com> > >> Subject: Re: [PATCH v3 2/3] dt-bindings: i2c-ast2600: Add bindings > >> for AST2600 i2C driver > >> > >> Hi Ryan, > >> > >> On Mon, 16 May 2022, at 16:18, ryan_chen wrote: > >> > AST2600 support new register set for I2C controller, add bindings > >> > document to support driver of i2c new register mode controller > >> > > >> > Signed-off-by: ryan_chen <ryan_chen@aspeedtech.com> > >> > --- > >> > .../bindings/i2c/aspeed,i2c-ast2600.ymal | 78 > >> +++++++++++++++++++ > >> > 1 file changed, 78 insertions(+) > >> > create mode 100644 > >> > Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.ymal > >> > > >> > diff --git > >> > a/Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.ymal > >> > b/Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.ymal > >> > new file mode 100644 > >> > index 000000000000..7c75f5bac24f > >> > --- /dev/null > >> > +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.ymal > >> > @@ -0,0 +1,78 @@ > >> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML > >> > +1.2 > >> > +--- > >> > +$id: http://devicetree.org/schemas/i2c/aspeed,i2c-ast2600.yaml# > >> > +$schema: http://devicetree.org/meta-schemas/core.yaml# > >> > + > >> > +title: AST2600 I2C Controller on the AST26XX SoCs Device Tree > >> > +Bindings > >> > + > >> > +maintainers: > >> > + - Ryan Chen <ryan_chen@aspeedtech.com> > >> > + > >> > +allOf: > >> > + - $ref: /schemas/i2c/i2c-controller.yaml# > >> > + > >> > +properties: > >> > + compatible: > >> > + enum: > >> > + - aspeed,ast2600-i2c > >> > >> The original driver uses e.g. aspeed,ast2500-i2c-bus for the > >> subordinate controllers. While the register layout changes, I'd > >> prefer we try to use the existing compatibles rather than introducing > >> a new set and causing some confusion. > >> > >> Further, what you're proposing here is effectively being used to > >> select the driver implementation, which isn't the purpose of the devicetree. > >> > >> My preference would be to reuse the existing compatibles and instead > >> select the driver implementation via Kconfig. Or, if we can figure > >> out some way to do so, support both register interfaces in the one > >> driver implementation and fall back to the old register interface > >> where the new one isn't available (I don't think this is feasible though). > >> > > Yes, that the reason go for another driver ast2600 to implement. > > Like others SOC driver implement different generation have diff driver > > in Kconfig and Makefile. > > Example : > > https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/Makef > > ile#L82-L84 > > > > > >> > + > >> > + reg: > >> > + minItems: 1 > >> > + items: > >> > + - description: address offset and range of bus > >> > + - description: address offset and range of bus buffer > >> > + > >> > + interrupts: > >> > + maxItems: 1 > >> > + > >> > + clocks: > >> > + maxItems: 1 > >> > + description: > >> > + root clock of bus, should reference the APB > >> > + clock in the second cell > >> > + > >> > + resets: > >> > + maxItems: 1 > >> > + > >> > + bus-frequency: > >> > + minimum: 500 > >> > + maximum: 2000000 > >> > + default: 100000 > >> > + description: frequency of the bus clock in Hz defaults to 100 > >> > + kHz > >> > when not > >> > + specified > >> > + > >> > + multi-master: > >> > + type: boolean > >> > + description: > >> > + states that there is another master active on this bus > >> > + > >> > +required: > >> > + - reg > >> > + - compatible > >> > + - clocks > >> > + - resets > >> > + > >> > +unevaluatedProperties: false > >> > + > >> > +examples: > >> > + - | > >> > + #include <dt-bindings/interrupt-controller/arm-gic.h> > >> > + #include <dt-bindings/clock/ast2600-clock.h> > >> > + > >> > + i2c_gr: i2c-global-regs@0 { > >> > + compatible = "aspeed,ast2600-i2c-global", "syscon"; > >> > + reg = <0x0 0x20>; > >> > + resets = <&syscon ASPEED_RESET_I2C>; > >> > + }; > >> > + > >> > + i2c0: i2c-bus@80 { > >> > + #address-cells = <1>; > >> > + #size-cells = <0>; > >> > + #interrupt-cells = <1>; > >> > + compatible = "aspeed,ast2600-i2c-bus"; > >> > >> This isn't quite right with respect to your binding description above > >> :) > > Yes, the compatible need to be " aspeed,ast2600-i2c" is that your point ? > > Yes, but only if we agree that we should have different compatibles for the > different drivers. I'm not convinced about that yet. > > I think it's enough to have different Kconfig symbols, and select the old driver > in aspeed_g4_defconfig, and the new driver in aspeed_g5_defconfig. Won't > that gives us the right outcome without requiring a new set of compatibles? > The new driver in aspeed_g5_defconfig. And different compatible string claim will Load the new or legacy driver, it should ok like the different generation SOC. Have different design. Am I right? > Andrew
On Tue, 2 Aug 2022, at 18:34, Ryan Chen wrote: > Hello, > >> -----Original Message----- >> From: Andrew Jeffery <andrew@aj.id.au> >> Sent: Friday, July 29, 2022 11:13 AM >> To: Ryan Chen <ryan_chen@aspeedtech.com>; Joel Stanley <joel@jms.id.au>; >> Philipp Zabel <p.zabel@pengutronix.de>; linux-arm-kernel@lists.infradead.org; >> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org; >> openbmc@lists.ozlabs.org >> Cc: BMC-SW <BMC-SW@aspeedtech.com> >> Subject: Re: [PATCH v3 2/3] dt-bindings: i2c-ast2600: Add bindings for AST2600 >> i2C driver >> >> >> >> On Fri, 29 Jul 2022, at 12:33, Ryan Chen wrote: >> > Hello Andrew, >> > >> >> -----Original Message----- >> >> From: Andrew Jeffery <andrew@aj.id.au> >> >> Sent: Friday, July 29, 2022 10:29 AM >> >> To: Ryan Chen <ryan_chen@aspeedtech.com>; Joel Stanley >> >> <joel@jms.id.au>; Philipp Zabel <p.zabel@pengutronix.de>; >> >> linux-arm-kernel@lists.infradead.org; >> >> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org; >> >> openbmc@lists.ozlabs.org >> >> Cc: BMC-SW <BMC-SW@aspeedtech.com> >> >> Subject: Re: [PATCH v3 2/3] dt-bindings: i2c-ast2600: Add bindings >> >> for AST2600 i2C driver >> >> >> >> Hi Ryan, >> >> >> >> On Mon, 16 May 2022, at 16:18, ryan_chen wrote: >> >> > + i2c0: i2c-bus@80 { >> >> > + #address-cells = <1>; >> >> > + #size-cells = <0>; >> >> > + #interrupt-cells = <1>; >> >> > + compatible = "aspeed,ast2600-i2c-bus"; >> >> >> >> This isn't quite right with respect to your binding description above >> >> :) >> > Yes, the compatible need to be " aspeed,ast2600-i2c" is that your point ? >> >> Yes, but only if we agree that we should have different compatibles for the >> different drivers. I'm not convinced about that yet. >> >> I think it's enough to have different Kconfig symbols, and select the old driver >> in aspeed_g4_defconfig, and the new driver in aspeed_g5_defconfig. Won't >> that gives us the right outcome without requiring a new set of compatibles? >> > The new driver in aspeed_g5_defconfig. Right, behind a new Kconfig option. > And different compatible string > claim will > Load the new or legacy driver, I don't think we need this. It's enough to enable the new driver in the defconfig (and possibly disable the config option for the old driver). > it should ok like the different > generation SOC. Have > different design. > Am I right? We have SoC-specific compatibles already, so the new driver can just bind on the compatibles for the SoC revisions that have the new register interface. The old driver just binds to the device in the SoCs that have the old register interface. There's an overlap in support between the two drivers, but for people who care about which implementation they use they can choose to exclude that driver from their kernel config. None of this requires more compatibles be added. Does that help? Andrew
Hello, > -----Original Message----- > From: Andrew Jeffery <andrew@aj.id.au> > Sent: Tuesday, August 9, 2022 8:35 AM > To: Ryan Chen <ryan_chen@aspeedtech.com>; Joel Stanley <joel@jms.id.au>; > Philipp Zabel <p.zabel@pengutronix.de>; linux-arm-kernel@lists.infradead.org; > linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org; > openbmc@lists.ozlabs.org > Cc: BMC-SW <BMC-SW@aspeedtech.com> > Subject: Re: [PATCH v3 2/3] dt-bindings: i2c-ast2600: Add bindings for AST2600 > i2C driver > > > > On Tue, 2 Aug 2022, at 18:34, Ryan Chen wrote: > > Hello, > > > >> -----Original Message----- > >> From: Andrew Jeffery <andrew@aj.id.au> > >> Sent: Friday, July 29, 2022 11:13 AM > >> To: Ryan Chen <ryan_chen@aspeedtech.com>; Joel Stanley > >> <joel@jms.id.au>; Philipp Zabel <p.zabel@pengutronix.de>; > >> linux-arm-kernel@lists.infradead.org; > >> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org; > >> openbmc@lists.ozlabs.org > >> Cc: BMC-SW <BMC-SW@aspeedtech.com> > >> Subject: Re: [PATCH v3 2/3] dt-bindings: i2c-ast2600: Add bindings > >> for AST2600 i2C driver > >> > >> > >> > >> On Fri, 29 Jul 2022, at 12:33, Ryan Chen wrote: > >> > Hello Andrew, > >> > > >> >> -----Original Message----- > >> >> From: Andrew Jeffery <andrew@aj.id.au> > >> >> Sent: Friday, July 29, 2022 10:29 AM > >> >> To: Ryan Chen <ryan_chen@aspeedtech.com>; Joel Stanley > >> >> <joel@jms.id.au>; Philipp Zabel <p.zabel@pengutronix.de>; > >> >> linux-arm-kernel@lists.infradead.org; > >> >> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org; > >> >> openbmc@lists.ozlabs.org > >> >> Cc: BMC-SW <BMC-SW@aspeedtech.com> > >> >> Subject: Re: [PATCH v3 2/3] dt-bindings: i2c-ast2600: Add bindings > >> >> for AST2600 i2C driver > >> >> > >> >> Hi Ryan, > >> >> > >> >> On Mon, 16 May 2022, at 16:18, ryan_chen wrote: > >> >> > + i2c0: i2c-bus@80 { > >> >> > + #address-cells = <1>; > >> >> > + #size-cells = <0>; > >> >> > + #interrupt-cells = <1>; > >> >> > + compatible = "aspeed,ast2600-i2c-bus"; > >> >> > >> >> This isn't quite right with respect to your binding description > >> >> above > >> >> :) > >> > Yes, the compatible need to be " aspeed,ast2600-i2c" is that your point ? > >> > >> Yes, but only if we agree that we should have different compatibles > >> for the different drivers. I'm not convinced about that yet. > >> > >> I think it's enough to have different Kconfig symbols, and select the > >> old driver in aspeed_g4_defconfig, and the new driver in > >> aspeed_g5_defconfig. Won't that gives us the right outcome without > requiring a new set of compatibles? > >> > > The new driver in aspeed_g5_defconfig. > > Right, behind a new Kconfig option. > > > And different compatible string > > claim will > > Load the new or legacy driver, > > I don't think we need this. It's enough to enable the new driver in the defconfig > (and possibly disable the config option for the old driver). > > > it should ok like the different > > generation SOC. Have > > different design. > > Am I right? > > We have SoC-specific compatibles already, so the new driver can just bind on > the compatibles for the SoC revisions that have the new register interface. The > old driver just binds to the device in the SoCs that have the old register > interface. > > There's an overlap in support between the two drivers, but for people who care > about which implementation they use they can choose to exclude that driver > from their kernel config. > > None of this requires more compatibles be added. > > Does that help? The kernel device tree driver use compatibles string to separate the driver, like currently aspeed_g5_defconfig include the ast2500/ast2600. So use dtsi/dts to separate which driver to loaded. Use can use dts to choice which driver to loaded. So keep the dtsi/dts for user implement to loaded should be good, right? > > Andrew
diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.ymal b/Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.ymal new file mode 100644 index 000000000000..7c75f5bac24f --- /dev/null +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.ymal @@ -0,0 +1,78 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/i2c/aspeed,i2c-ast2600.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: AST2600 I2C Controller on the AST26XX SoCs Device Tree Bindings + +maintainers: + - Ryan Chen <ryan_chen@aspeedtech.com> + +allOf: + - $ref: /schemas/i2c/i2c-controller.yaml# + +properties: + compatible: + enum: + - aspeed,ast2600-i2c + + reg: + minItems: 1 + items: + - description: address offset and range of bus + - description: address offset and range of bus buffer + + interrupts: + maxItems: 1 + + clocks: + maxItems: 1 + description: + root clock of bus, should reference the APB + clock in the second cell + + resets: + maxItems: 1 + + bus-frequency: + minimum: 500 + maximum: 2000000 + default: 100000 + description: frequency of the bus clock in Hz defaults to 100 kHz when not + specified + + multi-master: + type: boolean + description: + states that there is another master active on this bus + +required: + - reg + - compatible + - clocks + - resets + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/clock/ast2600-clock.h> + + i2c_gr: i2c-global-regs@0 { + compatible = "aspeed,ast2600-i2c-global", "syscon"; + reg = <0x0 0x20>; + resets = <&syscon ASPEED_RESET_I2C>; + }; + + i2c0: i2c-bus@80 { + #address-cells = <1>; + #size-cells = <0>; + #interrupt-cells = <1>; + compatible = "aspeed,ast2600-i2c-bus"; + reg = <0x80 0x80>, <0xC00 0x20>; + clocks = <&syscon ASPEED_CLK_APB2>; + resets = <&syscon ASPEED_RESET_I2C>; + bus-frequency = <100000>; + };
AST2600 support new register set for I2C controller, add bindings document to support driver of i2c new register mode controller Signed-off-by: ryan_chen <ryan_chen@aspeedtech.com> --- .../bindings/i2c/aspeed,i2c-ast2600.ymal | 78 +++++++++++++++++++ 1 file changed, 78 insertions(+) create mode 100644 Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.ymal