| Message ID | 20240321180617.35390-1-five231003@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v2] spi: dt-bindings: jcore,spi: convert spi-jcore to dtschema | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 2 warnings, 53 lines checked |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On 21/03/2024 19:02, Kousik Sanagavarapu wrote: > Convert existing bindings of J-Core spi2 to dtschema. > > No new properties are added. > > Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com> > --- > Changes since v1: > - changed the subject line to conform. > - dropped desc for "clock" and "clock-names" properties. > - cleaned up stuff. You miss many other changes... Some unusal properties appeared. ... > +--- > + > +$id: http://devicetree.org/schemas/spi/jcore,spi.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: J-Core SPI controller > + > +description: > + The J-Core "spi2" device is a PIO-based SPI controller which used to > + perform byte-at-a-time transfers between the CPU and itself. > + > +maintainers: > + - Kousik Sanagavarapu <five231003@gmail.com> > + > +allOf: > + - $ref: spi-controller.yaml# > + > +properties: > + compatible: > + const: jcore,spi2 > + > + reg: > + maxItems: 1 > + > + clocks: > + maxItems: 1 > + > + clock-names: > + items: > + - const: ref_clk > + > + spi-max-frequency: > + $ref: /schemas/types.yaml#/definitions/uint32 No, drop. From which other SPI binding did you take it? I asked you to look at existing code. Best regards, Krzysztof
On Fri, 22 Mar 2024, 11:33 Krzysztof Kozlowski, <krzysztof.kozlowski@linaro.org> wrote: > > On 21/03/2024 19:02, Kousik Sanagavarapu wrote: >> >> + spi-max-frequency: >> + $ref: /schemas/types.yaml#/definitions/uint32 > > No, drop. From which other SPI binding did you take it? I asked you to > look at existing code. Without this, "make dt_binding_check" would break though, right at the position in the example where "spi-max-frequency" is used. That was also the reason why additionalProperties was set to true in the last iteration, but after reading the doc more carefully I realized that was wrong after you pointed it out. I followed along bindings/spi/nvidia,tegra114-spi.yaml. (Sorry for the HTML mail ping, I'm replying from mobile)
On 22/03/2024 07:23, Kousik Sanagavarapu wrote: > On Fri, 22 Mar 2024, 11:33 Krzysztof Kozlowski, < > krzysztof.kozlowski@linaro.org> wrote: > >> On 21/03/2024 19:02, Kousik Sanagavarapu wrote: >> >>> + spi-max-frequency: >>> + $ref: /schemas/types.yaml#/definitions/uint32 >> >> No, drop. From which other SPI binding did you take it? I asked you to >> look at existing code. >> > > Without this, "make dt_binding_check" would break though, right at the > position in the example where "spi-max-frequency" is used. That was > also the reason why additionalProperties was set to true in the last > iteration, but after reading the doc more carefully I realized that was > wrong after you pointed it out. > > I followed along bindings/spi/nvidia,tegra114-spi.yaml. OK, you are right, the property is used here in controller node, however Linux driver never parsed it. It was never used, so I propose to drop it from the binding and example. You can mention in commit msg that spi-max-frequency was not documented thus you drop it from the example. DTS should be fixed as well. I'll send a patch for it. Best regards, Krzysztof
On 22/03/2024 07:34, Krzysztof Kozlowski wrote: > On 22/03/2024 07:23, Kousik Sanagavarapu wrote: >> On Fri, 22 Mar 2024, 11:33 Krzysztof Kozlowski, < >> krzysztof.kozlowski@linaro.org> wrote: >> >>> On 21/03/2024 19:02, Kousik Sanagavarapu wrote: >>> >>>> + spi-max-frequency: >>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> >>> No, drop. From which other SPI binding did you take it? I asked you to >>> look at existing code. >>> >> >> Without this, "make dt_binding_check" would break though, right at the >> position in the example where "spi-max-frequency" is used. That was >> also the reason why additionalProperties was set to true in the last >> iteration, but after reading the doc more carefully I realized that was >> wrong after you pointed it out. >> >> I followed along bindings/spi/nvidia,tegra114-spi.yaml. > > OK, you are right, the property is used here in controller node, however > Linux driver never parsed it. It was never used, so I propose to drop it > from the binding and example. You can mention in commit msg that > spi-max-frequency was not documented thus you drop it from the example. > > DTS should be fixed as well. I'll send a patch for it. Cc Daniel, BTW, J2 core is rather odd platform to work on... Even cross compiling and building that DTB is tricky. If I failed, I have doubts that you tested the DTS with your binding. This applies to all GSoC or some Linux Mentorship programs: I suggest to choose for conversion bindings with more users and bigger possible impact. So first I would look at ARM64 and ARMv7 platforms. We still have around 1000 and 3500 unique warnings about undocumented compatibles for ARM64 defconfig and ARM multi_v7! That's the platforms you should choose. Not SuperH, ARC, or whatever with only one DTS which is difficult to build for regular developer. Best regards, Krzysztof
On Fri, Mar 22, 2024 at 07:49:57AM +0100, Krzysztof Kozlowski wrote: > On 22/03/2024 07:34, Krzysztof Kozlowski wrote: > > On 22/03/2024 07:23, Kousik Sanagavarapu wrote: > >> On Fri, 22 Mar 2024, 11:33 Krzysztof Kozlowski, < > >> krzysztof.kozlowski@linaro.org> wrote: > >> > >>> On 21/03/2024 19:02, Kousik Sanagavarapu wrote: > >>> > >>>> + spi-max-frequency: > >>>> + $ref: /schemas/types.yaml#/definitions/uint32 > >>> > >>> No, drop. From which other SPI binding did you take it? I asked you to > >>> look at existing code. > >>> > >> > >> Without this, "make dt_binding_check" would break though, right at the > >> position in the example where "spi-max-frequency" is used. That was > >> also the reason why additionalProperties was set to true in the last > >> iteration, but after reading the doc more carefully I realized that was > >> wrong after you pointed it out. > >> > >> I followed along bindings/spi/nvidia,tegra114-spi.yaml. > > > > OK, you are right, the property is used here in controller node, however > > Linux driver never parsed it. It was never used, so I propose to drop it > > from the binding and example. You can mention in commit msg that > > spi-max-frequency was not documented thus you drop it from the example. > > > > DTS should be fixed as well. I'll send a patch for it. > > Cc Daniel, > > BTW, J2 core is rather odd platform to work on... Even cross compiling > and building that DTB is tricky. If I failed, I have doubts that you > tested the DTS with your binding. > > This applies to all GSoC or some Linux Mentorship programs: I suggest to > choose for conversion bindings with more users and bigger possible > impact. So first I would look at ARM64 and ARMv7 platforms. We still > have around 1000 and 3500 unique warnings about undocumented compatibles > for ARM64 defconfig and ARM multi_v7! That's the platforms you should > choose. > > Not SuperH, ARC, or whatever with only one DTS which is difficult to > build for regular developer. To add to this, either ask DT maintainers what would be useful to work on or you can look here[1][2] to see what are the top occurring undocumented (by schema) compatibles. Rob [1] https://gitlab.com/robherring/linux-dt/-/jobs/6453674734 [2] https://gitlab.com/robherring/linux-dt/-/jobs/6453674732
On Fri, Mar 22, 2024 at 10:05:24AM -0500, Rob Herring wrote: > On Fri, Mar 22, 2024 at 07:49:57AM +0100, Krzysztof Kozlowski wrote: > > This applies to all GSoC or some Linux Mentorship programs: I suggest to > > choose for conversion bindings with more users and bigger possible > > impact. So first I would look at ARM64 and ARMv7 platforms. We still > > have around 1000 and 3500 unique warnings about undocumented compatibles > > for ARM64 defconfig and ARM multi_v7! That's the platforms you should > > choose. > > > > Not SuperH, ARC, or whatever with only one DTS which is difficult to > > build for regular developer. > > To add to this, either ask DT maintainers what would be useful to work > on or you can look here[1][2] to see what are the top occurring > undocumented (by schema) compatibles. > > Rob > > [1] https://gitlab.com/robherring/linux-dt/-/jobs/6453674734 > [2] https://gitlab.com/robherring/linux-dt/-/jobs/6453674732 Thank you for the valuable pieces of advice and links Krzysztof and Rob.
diff --git a/Documentation/devicetree/bindings/spi/jcore,spi.txt b/Documentation/devicetree/bindings/spi/jcore,spi.txt deleted file mode 100644 index 93936d16e139..000000000000 --- a/Documentation/devicetree/bindings/spi/jcore,spi.txt +++ /dev/null @@ -1,34 +0,0 @@ -J-Core SPI master - -Required properties: - -- compatible: Must be "jcore,spi2". - -- reg: Memory region for registers. - -- #address-cells: Must be 1. - -- #size-cells: Must be 0. - -Optional properties: - -- clocks: If a phandle named "ref_clk" is present, SPI clock speed - programming is relative to the frequency of the indicated clock. - Necessary only if the input clock rate is something other than a - fixed 50 MHz. - -- clock-names: Clock names, one for each phandle in clocks. - -See spi-bus.txt for additional properties not specific to this device. - -Example: - -spi@40 { - compatible = "jcore,spi2"; - #address-cells = <1>; - #size-cells = <0>; - reg = <0x40 0x8>; - spi-max-frequency = <25000000>; - clocks = <&bus_clk>; - clock-names = "ref_clk"; -} diff --git a/Documentation/devicetree/bindings/spi/jcore,spi.yaml b/Documentation/devicetree/bindings/spi/jcore,spi.yaml new file mode 100644 index 000000000000..b8ec3adaac8d --- /dev/null +++ b/Documentation/devicetree/bindings/spi/jcore,spi.yaml @@ -0,0 +1,53 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- + +$id: http://devicetree.org/schemas/spi/jcore,spi.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: J-Core SPI controller + +description: + The J-Core "spi2" device is a PIO-based SPI controller which used to + perform byte-at-a-time transfers between the CPU and itself. + +maintainers: + - Kousik Sanagavarapu <five231003@gmail.com> + +allOf: + - $ref: spi-controller.yaml# + +properties: + compatible: + const: jcore,spi2 + + reg: + maxItems: 1 + + clocks: + maxItems: 1 + + clock-names: + items: + - const: ref_clk + + spi-max-frequency: + $ref: /schemas/types.yaml#/definitions/uint32 + +required: + - compatible + - reg + +unevaluatedProperties: false + +examples: + - | + spi@40 { + compatible = "jcore,spi2"; + reg = <0x40 0x8>; + #address-cells = <1>; + #size-cells = <0>; + spi-max-frequency = <25000000>; + clocks = <&bus_clk>; + clock-names = "ref_clk"; + };
Convert existing bindings of J-Core spi2 to dtschema. No new properties are added. Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com> --- Changes since v1: - changed the subject line to conform. - dropped desc for "clock" and "clock-names" properties. - cleaned up stuff. Thanks for the review Krzysztof. .../devicetree/bindings/spi/jcore,spi.txt | 34 ------------ .../devicetree/bindings/spi/jcore,spi.yaml | 53 +++++++++++++++++++ 2 files changed, 53 insertions(+), 34 deletions(-) delete mode 100644 Documentation/devicetree/bindings/spi/jcore,spi.txt create mode 100644 Documentation/devicetree/bindings/spi/jcore,spi.yaml