| Message ID | 20210323043331.21878-1-chris.packham@alliedtelesis.co.nz |
|---|---|
| Headers | show |
| Series | i2c: mpc: Refactor to improve responsiveness | expand |
On Tue, 23 Mar 2021 17:33:27 +1300, Chris Packham wrote: > Convert i2c-mpc to YAML. > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > --- > .../devicetree/bindings/i2c/i2c-mpc.txt | 62 ------------ > .../devicetree/bindings/i2c/i2c-mpc.yaml | 99 +++++++++++++++++++ > 2 files changed, 99 insertions(+), 62 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/i2c/i2c-mpc.txt > create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mpc.yaml > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: ./Documentation/devicetree/bindings/i2c/i2c-mpc.yaml:19:9: [warning] wrong indentation: expected 10 but found 8 (indentation) ./Documentation/devicetree/bindings/i2c/i2c-mpc.yaml:20:11: [warning] wrong indentation: expected 12 but found 10 (indentation) dtschema/dtc warnings/errors: See https://patchwork.ozlabs.org/patch/1457053 This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.
On Tue, Mar 23, 2021 at 08:22:00PM +0000, Chris Packham wrote: > Hi Rob, > > On 24/03/21 9:16 am, Rob Herring wrote: > > On Tue, 23 Mar 2021 17:33:27 +1300, Chris Packham wrote: > >> Convert i2c-mpc to YAML. > >> > >> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > >> --- > >> .../devicetree/bindings/i2c/i2c-mpc.txt | 62 ------------ > >> .../devicetree/bindings/i2c/i2c-mpc.yaml | 99 +++++++++++++++++++ > >> 2 files changed, 99 insertions(+), 62 deletions(-) > >> delete mode 100644 Documentation/devicetree/bindings/i2c/i2c-mpc.txt > >> create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mpc.yaml > >> > > My bot found errors running 'make dt_binding_check' on your patch: > > > > yamllint warnings/errors: > > ./Documentation/devicetree/bindings/i2c/i2c-mpc.yaml:19:9: [warning] wrong indentation: expected 10 but found 8 (indentation) > > ./Documentation/devicetree/bindings/i2c/i2c-mpc.yaml:20:11: [warning] wrong indentation: expected 12 but found 10 (indentation) > Hmm I did run 'make dt_binding_check' is yamllint run separately (or not > run if it's not installed?). No and yes... > > dtschema/dtc warnings/errors: > > > > See https://patchwork.ozlabs.org/patch/1457053 > > > > This check can fail if there are any dependencies. The base for a patch > > series is generally the most recent rc1. > > > > If you already ran 'make dt_binding_check' and didn't see the above > > error(s), then make sure 'yamllint' is installed and dt-schema is up to > > date: ^^^^^ > > > > pip3 install dtschema --upgrade > > > > Please check and re-submit. > Should be easy to fix the binding but I'll spend a bit of time trying to > get my tooling sorted.
On 24/03/21 10:15 am, Rob Herring wrote: > On Tue, Mar 23, 2021 at 05:33:27PM +1300, Chris Packham wrote: >> Convert i2c-mpc to YAML. >> >> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> >> --- >> .../devicetree/bindings/i2c/i2c-mpc.txt | 62 ------------ >> .../devicetree/bindings/i2c/i2c-mpc.yaml | 99 +++++++++++++++++++ >> 2 files changed, 99 insertions(+), 62 deletions(-) >> delete mode 100644 Documentation/devicetree/bindings/i2c/i2c-mpc.txt >> create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mpc.yaml >> >> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mpc.txt b/Documentation/devicetree/bindings/i2c/i2c-mpc.txt >> deleted file mode 100644 >> index b15acb43d84d..000000000000 >> --- a/Documentation/devicetree/bindings/i2c/i2c-mpc.txt >> +++ /dev/null >> @@ -1,62 +0,0 @@ >> -* I2C >> - >> -Required properties : >> - >> - - reg : Offset and length of the register set for the device >> - - compatible : should be "fsl,CHIP-i2c" where CHIP is the name of a >> - compatible processor, e.g. mpc8313, mpc8543, mpc8544, mpc5121, >> - mpc5200 or mpc5200b. For the mpc5121, an additional node >> - "fsl,mpc5121-i2c-ctrl" is required as shown in the example below. >> - - interrupts : <a b> where a is the interrupt number and b is a >> - field that represents an encoding of the sense and level >> - information for the interrupt. This should be encoded based on >> - the information in section 2) depending on the type of interrupt >> - controller you have. >> - >> -Recommended properties : >> - >> - - fsl,preserve-clocking : boolean; if defined, the clock settings >> - from the bootloader are preserved (not touched). >> - - clock-frequency : desired I2C bus clock frequency in Hz. >> - - fsl,timeout : I2C bus timeout in microseconds. >> - >> -Examples : >> - >> - /* MPC5121 based board */ >> - i2c@1740 { >> - #address-cells = <1>; >> - #size-cells = <0>; >> - compatible = "fsl,mpc5121-i2c", "fsl-i2c"; >> - reg = <0x1740 0x20>; >> - interrupts = <11 0x8>; >> - interrupt-parent = <&ipic>; >> - clock-frequency = <100000>; >> - }; >> - >> - i2ccontrol@1760 { >> - compatible = "fsl,mpc5121-i2c-ctrl"; >> - reg = <0x1760 0x8>; >> - }; >> - >> - /* MPC5200B based board */ >> - i2c@3d00 { >> - #address-cells = <1>; >> - #size-cells = <0>; >> - compatible = "fsl,mpc5200b-i2c","fsl,mpc5200-i2c","fsl-i2c"; >> - reg = <0x3d00 0x40>; >> - interrupts = <2 15 0>; >> - interrupt-parent = <&mpc5200_pic>; >> - fsl,preserve-clocking; >> - }; >> - >> - /* MPC8544 base board */ >> - i2c@3100 { >> - #address-cells = <1>; >> - #size-cells = <0>; >> - compatible = "fsl,mpc8544-i2c", "fsl-i2c"; >> - reg = <0x3100 0x100>; >> - interrupts = <43 2>; >> - interrupt-parent = <&mpic>; >> - clock-frequency = <400000>; >> - fsl,timeout = <10000>; >> - }; >> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mpc.yaml b/Documentation/devicetree/bindings/i2c/i2c-mpc.yaml >> new file mode 100644 >> index 000000000000..97cea8a817ea >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/i2c/i2c-mpc.yaml >> @@ -0,0 +1,99 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/i2c/i2c-mpc.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: I2C-Bus adapter for MPC824x/83xx/85xx/86xx/512x/52xx SoCs >> + >> +maintainers: >> + - Chris Packham <chris.packham@alliedtelesis.co.nz> >> + >> +allOf: >> + - $ref: /schemas/i2c/i2c-controller.yaml# >> + >> +properties: >> + compatible: >> + anyOf: >> + - items: >> + - enum: >> + - mpc5200-i2c >> + - fsl,mpc5200b-i2c >> + - fsl,mpc5200-i2c >> + - fsl,mpc5121-i2c >> + - fsl,mpc8313-i2c >> + - fsl,mpc8543-i2c >> + - fsl,mpc8544-i2c >> + >> + - const: fsl-i2c >> + >> + - contains: >> + const: fsl-i2c >> + minItems: 1 >> + maxItems: 4 > Can't we drop this and list out any other compatibles? I'm struggling a little bit with how to get the schema right to allow one or more of a set of compatible values. Basically I want to allow 'compatible = "fsl-i2c";' or 'compatible = "fsl,mpc8544-i2c", "fsl-i2c";' but disallow 'compatible = "foobar", "fsl-i2c";' >> + >> + reg: >> + maxItems: 1 >> + >> + interrupts: >> + maxItems: 1 >> + >> + fsl,preserve-clocking: >> + $ref: /schemas/types.yaml#/definitions/flag >> + description: | >> + if defined, the clock settings from the bootloader are >> + preserved (not touched) >> + >> + fsl,timeout: >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + description: | >> + I2C bus timeout in microseconds >> + >> +required: >> + - compatible >> + - reg >> + - interrupts >> + >> +unevaluatedProperties: false >> + >> +examples: >> + - | >> + /* MPC5121 based board */ >> + i2c@1740 { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + compatible = "fsl,mpc5121-i2c", "fsl-i2c"; >> + reg = <0x1740 0x20>; >> + interrupts = <11 0x8>; >> + interrupt-parent = <&ipic>; >> + clock-frequency = <100000>; >> + }; >> + >> + i2ccontrol@1760 { >> + compatible = "fsl,mpc5121-i2c-ctrl"; > Drop this or document it. I'm trying to get rid of undocumented (by > schemas) compatibles in examples. Will remove it >> + reg = <0x1760 0x8>; >> + }; >> + >> + /* MPC5200B based board */ >> + i2c@3d00 { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + compatible = "fsl,mpc5200b-i2c", "fsl,mpc5200-i2c", "fsl-i2c"; >> + reg = <0x3d00 0x40>; >> + interrupts = <2 15 0>; >> + interrupt-parent = <&mpc5200_pic>; >> + fsl,preserve-clocking; >> + }; >> + >> + /* MPC8544 base board */ >> + i2c@3100 { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + compatible = "fsl,mpc8544-i2c", "fsl-i2c"; >> + reg = <0x3100 0x100>; >> + interrupts = <43 2>; >> + interrupt-parent = <&mpic>; >> + clock-frequency = <400000>; >> + fsl,timeout = <10000>; >> + }; >> +... >> -- >> 2.30.2 >>
On 24/03/21 10:59 am, Chris Packham wrote: > > On 24/03/21 10:15 am, Rob Herring wrote: >> On Tue, Mar 23, 2021 at 05:33:27PM +1300, Chris Packham wrote: >>> Convert i2c-mpc to YAML. >>> >>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> >>> --- <snip> >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/i2c/i2c-mpc.yaml >>> @@ -0,0 +1,99 @@ >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/i2c/i2c-mpc.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: I2C-Bus adapter for MPC824x/83xx/85xx/86xx/512x/52xx SoCs >>> + >>> +maintainers: >>> + - Chris Packham <chris.packham@alliedtelesis.co.nz> >>> + >>> +allOf: >>> + - $ref: /schemas/i2c/i2c-controller.yaml# >>> + >>> +properties: >>> + compatible: >>> + anyOf: >>> + - items: >>> + - enum: >>> + - mpc5200-i2c >>> + - fsl,mpc5200b-i2c >>> + - fsl,mpc5200-i2c >>> + - fsl,mpc5121-i2c >>> + - fsl,mpc8313-i2c >>> + - fsl,mpc8543-i2c >>> + - fsl,mpc8544-i2c >>> + >>> + - const: fsl-i2c >>> + >>> + - contains: >>> + const: fsl-i2c >>> + minItems: 1 >>> + maxItems: 4 >> Can't we drop this and list out any other compatibles? > > I'm struggling a little bit with how to get the schema right to allow > one or more of a set of compatible values. > > Basically I want to allow 'compatible = "fsl-i2c";' or 'compatible = > "fsl,mpc8544-i2c", "fsl-i2c";' but disallow 'compatible = "foobar", > "fsl-i2c";' This is what I've ended up with properties: compatible: oneOf: - items: - enum: - mpc5200-i2c - fsl,mpc5200-i2c - fsl,mpc5121-i2c - fsl,mpc8313-i2c - fsl,mpc8543-i2c - fsl,mpc8544-i2c - fsl-i2c - const: fsl-i2c - items: - const: fsl,mpc5200b-i2c - const: fsl,mpc5200-i2c - const: fsl-i2c It passes `make dt_binding_check` and rejects things when I add other non-documented values to the compatible property. I did struggle with it so I'm not confident it's the best approach but it seems to work.
On 23/03/21 5:33 pm, Chris Packham wrote: > The "meat" of this series is in the last patch which is the change that > actually starts making use of the interrupts to drive a state machine. > The dt-bindings patches can probably go in at any time. The rest of the > series isn't dependent on them. > > I've tested it on a T2081 based system with a number of i2c and smbus > devices. Its the end of my work day so I figured I'd get this out now > but I'll do some more testing on a P2041 board and a few different i2c > devices tomorrow. I've done more testing on a T2081 and P2041 board. Both look good. I've had some feedback from Rob on the dt-bindings which I think I've got sorted now. I've got a couple of minor cosmetic changes to 6/6 but I'll hold fire on sending a v2 to give people a chance to look at the functional changes. > Chris Packham (6): > dt-bindings: i2c-mpc: Document interrupt property as required > dt-bindings: i2c: convert i2c-mpc to json-schema > i2c: mpc: Make use of i2c_recover_bus() > i2c: mpc: make interrupt mandatory and remove polling code > i2c: mpc: use device managed APIs > i2c: mpc: Interrupt driven transfer > > .../devicetree/bindings/i2c/i2c-mpc.txt | 62 --- > .../devicetree/bindings/i2c/i2c-mpc.yaml | 99 ++++ > drivers/i2c/busses/i2c-mpc.c | 513 ++++++++++-------- > 3 files changed, 373 insertions(+), 301 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/i2c/i2c-mpc.txt > create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mpc.yaml >
On Wed, Mar 24, 2021 at 03:36:13AM +0000, Chris Packham wrote: > > On 24/03/21 10:59 am, Chris Packham wrote: > > > > On 24/03/21 10:15 am, Rob Herring wrote: > >> On Tue, Mar 23, 2021 at 05:33:27PM +1300, Chris Packham wrote: > >>> Convert i2c-mpc to YAML. > >>> > >>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > >>> --- > <snip> > >>> --- /dev/null > >>> +++ b/Documentation/devicetree/bindings/i2c/i2c-mpc.yaml > >>> @@ -0,0 +1,99 @@ > >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > >>> +%YAML 1.2 > >>> +--- > >>> +$id: http://devicetree.org/schemas/i2c/i2c-mpc.yaml# > >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# > >>> + > >>> +title: I2C-Bus adapter for MPC824x/83xx/85xx/86xx/512x/52xx SoCs > >>> + > >>> +maintainers: > >>> + - Chris Packham <chris.packham@alliedtelesis.co.nz> > >>> + > >>> +allOf: > >>> + - $ref: /schemas/i2c/i2c-controller.yaml# > >>> + > >>> +properties: > >>> + compatible: > >>> + anyOf: > >>> + - items: > >>> + - enum: > >>> + - mpc5200-i2c > >>> + - fsl,mpc5200b-i2c > >>> + - fsl,mpc5200-i2c > >>> + - fsl,mpc5121-i2c > >>> + - fsl,mpc8313-i2c > >>> + - fsl,mpc8543-i2c > >>> + - fsl,mpc8544-i2c > >>> + > >>> + - const: fsl-i2c > >>> + > >>> + - contains: > >>> + const: fsl-i2c > >>> + minItems: 1 > >>> + maxItems: 4 > >> Can't we drop this and list out any other compatibles? > > > > I'm struggling a little bit with how to get the schema right to allow > > one or more of a set of compatible values. > > > > Basically I want to allow 'compatible = "fsl-i2c";' or 'compatible = > > "fsl,mpc8544-i2c", "fsl-i2c";' but disallow 'compatible = "foobar", > > "fsl-i2c";' > > This is what I've ended up with > > properties: > compatible: > oneOf: > - items: > - enum: > - mpc5200-i2c > - fsl,mpc5200-i2c > - fsl,mpc5121-i2c > - fsl,mpc8313-i2c > - fsl,mpc8543-i2c > - fsl,mpc8544-i2c > - fsl-i2c This one should be dropped. '"fsl-i2c", "fsl-i2c"' presumably isn't valid. There's a generic check for unique entries anyways, so it would still fail. > - const: fsl-i2c > - items: > - const: fsl,mpc5200b-i2c > - const: fsl,mpc5200-i2c > - const: fsl-i2c > > It passes `make dt_binding_check` and rejects things when I add other > non-documented values to the compatible property. I did struggle with it > so I'm not confident it's the best approach but it seems to work. Otherwise, looks right to me. Rob