diff mbox series

[v2] spi: dt-bindings: jcore,spi: convert spi-jcore to dtschema

Message ID 20240321180617.35390-1-five231003@gmail.com
State New
Headers show
Series [v2] spi: dt-bindings: jcore,spi: convert spi-jcore to dtschema | expand

Commit Message

Kousik Sanagavarapu March 21, 2024, 6:02 p.m. UTC
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

Comments

Krzysztof Kozlowski March 22, 2024, 6:03 a.m. UTC | #1
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
Kousik Sanagavarapu March 22, 2024, 6:33 a.m. UTC | #2
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)
Krzysztof Kozlowski March 22, 2024, 6:34 a.m. UTC | #3
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
Krzysztof Kozlowski March 22, 2024, 6:49 a.m. UTC | #4
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
Rob Herring (Arm) March 22, 2024, 3:05 p.m. UTC | #5
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
Kousik Sanagavarapu March 22, 2024, 4:48 p.m. UTC | #6
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 mbox series

Patch

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";
+    };