[PATCH v3 0/6] Add support for RZ/N1 SJA1000 CAN controller

[PATCH v3 0/6] Add support for RZ/N1 SJA1000 CAN controller

[Biju Das]

This patch series aims to add support for RZ/N1 SJA1000 CAN controller.

The SJA1000 CAN controller on RZ/N1 SoC has some differences compared
to others like it has no clock divider register (CDR) support and it has
no HW loopback (HW doesn't see tx messages on rx), so introduced a new
compatible 'renesas,rzn1-sja1000' to handle these differences.

v2->v3:
 * Added reg-io-width is a required property for technologic,sja1000 & renesas,rzn1-sja1000
 * Removed enum type from nxp,tx-output-config and updated the description
   for combination of TX0 and TX1.
 * Updated the example for technologic,sja1000
v1->v2:
 * Moved $ref: can-controller.yaml# to top along with if conditional to
   avoid multiple mapping issues with the if conditional in the subsequent
   patch.
 * Added an example for RZ/N1D SJA1000 usage.
 * Updated commit description for patch#2,#3 and #6
 * Removed the quirk macro SJA1000_NO_HW_LOOPBACK_QUIRK
 * Added prefix SJA1000_QUIRK_* for quirk macro.
 * Replaced of_device_get_match_data->device_get_match_data.
 * Added error handling on clk error path
 * Started using "devm_clk_get_optional_enabled" for clk get,prepare and enable.

Ref:
 [1] https://lore.kernel.org/linux-renesas-soc/20220701162320.102165-1-biju.das.jz@bp.renesas.com/T/#t

Biju Das (6):
  dt-bindings: can: sja1000: Convert to json-schema
  dt-bindings: can: nxp,sja1000: Document RZ/N1{D,S} support
  can: sja1000: Add Quirk for RZ/N1 SJA1000 CAN controller
  can: sja1000: Use device_get_match_data to get device data
  can: sja1000: Change the return type as void for SoC specific init
  can: sja1000: Add support for RZ/N1 SJA1000 CAN Controller

 .../bindings/net/can/nxp,sja1000.yaml         | 138 ++++++++++++++++++
 .../devicetree/bindings/net/can/sja1000.txt   |  58 --------
 drivers/net/can/sja1000/sja1000.c             |  13 +-
 drivers/net/can/sja1000/sja1000.h             |   3 +-
 drivers/net/can/sja1000/sja1000_platform.c    |  56 ++++---
 5 files changed, 186 insertions(+), 82 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
 delete mode 100644 Documentation/devicetree/bindings/net/can/sja1000.txt

-- 
2.25.1

[PATCH v3 1/6] dt-bindings: can: sja1000: Convert to json-schema

[Biju Das]

Convert the NXP SJA1000 CAN Controller Device Tree binding
documentation to json-schema.

Update the example to match reality.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v2->v3:
 * Added reg-io-width is a required property for technologic,sja1000
 * Removed enum type from nxp,tx-output-config and updated the description
   for combination of TX0 and TX1.
 * Updated the example
v1->v2:
 * Moved $ref: can-controller.yaml# to top along with if conditional to
   avoid multiple mapping issues with the if conditional in the subsequent
   patch.
---
 .../bindings/net/can/nxp,sja1000.yaml         | 103 ++++++++++++++++++
 .../devicetree/bindings/net/can/sja1000.txt   |  58 ----------
 2 files changed, 103 insertions(+), 58 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
 delete mode 100644 Documentation/devicetree/bindings/net/can/sja1000.txt

diff --git a/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml b/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
new file mode 100644
index 000000000000..d34060226e4e
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
@@ -0,0 +1,103 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/can/nxp,sja1000.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Memory mapped SJA1000 CAN controller from NXP (formerly Philips)
+
+maintainers:
+  - Wolfgang Grandegger <wg@grandegger.com>
+
+allOf:
+  - $ref: can-controller.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: technologic,sja1000
+    then:
+      required:
+        - reg-io-width
+
+properties:
+  compatible:
+    oneOf:
+      - description: NXP SJA1000 CAN Controller
+        const: nxp,sja1000
+      - description: Technologic Systems SJA1000 CAN Controller
+        const: technologic,sja1000
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  reg-io-width:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: I/O register width (in bytes) implemented by this device
+    default: 1
+    enum: [ 1, 2, 4 ]
+
+  nxp,external-clock-frequency:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    default: 16000000
+    description: |
+      Frequency of the external oscillator clock in Hz.
+      The internal clock frequency used by the SJA1000 is half of that value.
+
+  nxp,tx-output-mode:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [ 0, 1, 2, 3 ]
+    default: 1
+    description: |
+      operation mode of the TX output control logic. Valid values are:
+        <0x0> : bi-phase output mode
+        <0x1> : normal output mode (default)
+        <0x2> : test output mode
+        <0x3> : clock output mode
+
+  nxp,tx-output-config:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    default: 0x02
+    description: |
+      TX output pin configuration. Valid values are any one of the below
+      or combination of TX0 and TX1:
+        <0x01> : TX0 invert
+        <0x02> : TX0 pull-down (default)
+        <0x04> : TX0 pull-up
+        <0x06> : TX0 push-pull
+        <0x08> : TX1 invert
+        <0x10> : TX1 pull-down
+        <0x20> : TX1 pull-up
+        <0x30> : TX1 push-pull
+
+  nxp,clock-out-frequency:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: |
+      clock frequency in Hz on the CLKOUT pin.
+      If not specified or if the specified value is 0, the CLKOUT pin
+      will be disabled.
+
+  nxp,no-comparator-bypass:
+    type: boolean
+    description: Allows to disable the CAN input comparator.
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    can@1a000 {
+            compatible = "technologic,sja1000";
+            reg = <0x1a000 0x100>;
+            interrupts = <1>;
+            reg-io-width = <2>;
+            nxp,tx-output-config = <0x06>;
+            nxp,external-clock-frequency = <24000000>;
+    };
diff --git a/Documentation/devicetree/bindings/net/can/sja1000.txt b/Documentation/devicetree/bindings/net/can/sja1000.txt
deleted file mode 100644
index ac3160eca96a..000000000000
--- a/Documentation/devicetree/bindings/net/can/sja1000.txt
+++ /dev/null
@@ -1,58 +0,0 @@
-Memory mapped SJA1000 CAN controller from NXP (formerly Philips)
-
-Required properties:
-
-- compatible : should be one of "nxp,sja1000", "technologic,sja1000".
-
-- reg : should specify the chip select, address offset and size required
-	to map the registers of the SJA1000. The size is usually 0x80.
-
-- interrupts: property with a value describing the interrupt source
-	(number and sensitivity) required for the SJA1000.
-
-Optional properties:
-
-- reg-io-width : Specify the size (in bytes) of the IO accesses that
-	should be performed on the device.  Valid value is 1, 2 or 4.
-	This property is ignored for technologic version.
-	Default to 1 (8 bits).
-
-- nxp,external-clock-frequency : Frequency of the external oscillator
-	clock in Hz. Note that the internal clock frequency used by the
-	SJA1000 is half of that value. If not specified, a default value
-	of 16000000 (16 MHz) is used.
-
-- nxp,tx-output-mode : operation mode of the TX output control logic:
-	<0x0> : bi-phase output mode
-	<0x1> : normal output mode (default)
-	<0x2> : test output mode
-	<0x3> : clock output mode
-
-- nxp,tx-output-config : TX output pin configuration:
-	<0x01> : TX0 invert
-	<0x02> : TX0 pull-down (default)
-	<0x04> : TX0 pull-up
-	<0x06> : TX0 push-pull
-	<0x08> : TX1 invert
-	<0x10> : TX1 pull-down
-	<0x20> : TX1 pull-up
-	<0x30> : TX1 push-pull
-
-- nxp,clock-out-frequency : clock frequency in Hz on the CLKOUT pin.
-	If not specified or if the specified value is 0, the CLKOUT pin
-	will be disabled.
-
-- nxp,no-comparator-bypass : Allows to disable the CAN input comparator.
-
-For further information, please have a look to the SJA1000 data sheet.
-
-Examples:
-
-can@3,100 {
-	compatible = "nxp,sja1000";
-	reg = <3 0x100 0x80>;
-	interrupts = <2 0>;
-	interrupt-parent = <&mpic>;
-	nxp,external-clock-frequency = <16000000>;
-};
-
-- 
2.25.1

[PATCH v3 2/6] dt-bindings: can: nxp,sja1000: Document RZ/N1{D,S} support

[Biju Das]

Add CAN binding documentation for Renesas RZ/N1 SoC.

The SJA1000 CAN controller on RZ/N1 SoC has some differences compared
to others like it has no clock divider register (CDR) support and it has
no HW loopback (HW doesn't see tx messages on rx), so introduced a new
compatible 'renesas,rzn1-sja1000' to handle these differences.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v2->v3:
 * Added reg-io-width is required property for renesas,rzn1-sja1000.
v1->v2:
 * Updated commit description.
 * Added an example for RZ/N1D SJA1000 usage
---
 .../bindings/net/can/nxp,sja1000.yaml         | 35 +++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml b/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
index d34060226e4e..16786475eae3 100644
--- a/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
+++ b/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
@@ -19,6 +19,16 @@ allOf:
     then:
       required:
         - reg-io-width
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: renesas,rzn1-sja1000
+    then:
+      required:
+        - clocks
+        - clock-names
+        - reg-io-width
 
 properties:
   compatible:
@@ -27,6 +37,12 @@ properties:
         const: nxp,sja1000
       - description: Technologic Systems SJA1000 CAN Controller
         const: technologic,sja1000
+      - description: Renesas RZ/N1 SJA1000 CAN Controller
+        items:
+          - enum:
+              - renesas,r9a06g032-sja1000 # RZ/N1D
+              - renesas,r9a06g033-sja1000 # RZ/N1S
+          - const: renesas,rzn1-sja1000 # RZ/N1
 
   reg:
     maxItems: 1
@@ -34,6 +50,12 @@ properties:
   interrupts:
     maxItems: 1
 
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    const: can_clk
+
   reg-io-width:
     $ref: /schemas/types.yaml#/definitions/uint32
     description: I/O register width (in bytes) implemented by this device
@@ -101,3 +123,16 @@ examples:
             nxp,tx-output-config = <0x06>;
             nxp,external-clock-frequency = <24000000>;
     };
+
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/clock/r9a06g032-sysctrl.h>
+
+    can@52104000 {
+            compatible = "renesas,r9a06g032-sja1000","renesas,rzn1-sja1000";
+            reg = <0x52104000 0x800>;
+            reg-io-width = <4>;
+            interrupts = <GIC_SPI 95 IRQ_TYPE_LEVEL_HIGH>;
+            clocks = <&sysctrl R9A06G032_HCLK_CAN0>;
+            clock-names = "can_clk";
+    };
-- 
2.25.1

[PATCH v3 3/6] can: sja1000: Add Quirk for RZ/N1 SJA1000 CAN controller

[Biju Das]

As per Chapter 6.5.16 of the RZ/N1 Peripheral Manual, The SJA1000
CAN controller does not support Clock Divider Register compared to
the reference Philips SJA1000 device.

This patch adds a device quirk to handle this difference.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v2->v3:
 * No Change
v1->v2:
 * Updated commit description
 * Removed the quirk macro SJA1000_NO_HW_LOOPBACK_QUIRK
 * Added prefix SJA1000_QUIRK_* for quirk macro.
---
 drivers/net/can/sja1000/sja1000.c | 13 ++++++++-----
 drivers/net/can/sja1000/sja1000.h |  3 ++-
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
index 2e7638f98cf1..e9c55f5aa3c3 100644
--- a/drivers/net/can/sja1000/sja1000.c
+++ b/drivers/net/can/sja1000/sja1000.c
@@ -183,8 +183,9 @@ static void chipset_init(struct net_device *dev)
 {
 	struct sja1000_priv *priv = netdev_priv(dev);
 
-	/* set clock divider and output control register */
-	priv->write_reg(priv, SJA1000_CDR, priv->cdr | CDR_PELICAN);
+	if (!(priv->flags & SJA1000_QUIRK_NO_CDR_REG))
+		/* set clock divider and output control register */
+		priv->write_reg(priv, SJA1000_CDR, priv->cdr | CDR_PELICAN);
 
 	/* set acceptance filter (accept all) */
 	priv->write_reg(priv, SJA1000_ACCC0, 0x00);
@@ -208,9 +209,11 @@ static void sja1000_start(struct net_device *dev)
 	if (priv->can.state != CAN_STATE_STOPPED)
 		set_reset_mode(dev);
 
-	/* Initialize chip if uninitialized at this stage */
-	if (!(priv->read_reg(priv, SJA1000_CDR) & CDR_PELICAN))
-		chipset_init(dev);
+	if (!(priv->flags & SJA1000_QUIRK_NO_CDR_REG)) {
+		/* Initialize chip if uninitialized at this stage */
+		if (!(priv->read_reg(priv, SJA1000_CDR) & CDR_PELICAN))
+			chipset_init(dev);
+	}
 
 	/* Clear error counters and error code capture */
 	priv->write_reg(priv, SJA1000_TXERR, 0x0);
diff --git a/drivers/net/can/sja1000/sja1000.h b/drivers/net/can/sja1000/sja1000.h
index 9d46398f8154..7f736f1df547 100644
--- a/drivers/net/can/sja1000/sja1000.h
+++ b/drivers/net/can/sja1000/sja1000.h
@@ -145,7 +145,8 @@
 /*
  * Flags for sja1000priv.flags
  */
-#define SJA1000_CUSTOM_IRQ_HANDLER 0x1
+#define SJA1000_CUSTOM_IRQ_HANDLER	BIT(0)
+#define SJA1000_QUIRK_NO_CDR_REG	BIT(1)
 
 /*
  * SJA1000 private data structure
-- 
2.25.1

[PATCH v3 4/6] can: sja1000: Use device_get_match_data to get device data

[Biju Das]

This patch replaces of_match_device->device_get_match_data
to get pointer to device data.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v2->v3:
 * No change
v1->v2:
 * Replaced of_device_get_match_data->device_get_match_data.
---
 drivers/net/can/sja1000/sja1000_platform.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/net/can/sja1000/sja1000_platform.c b/drivers/net/can/sja1000/sja1000_platform.c
index f9ec7bd8dfac..0b78568f5286 100644
--- a/drivers/net/can/sja1000/sja1000_platform.c
+++ b/drivers/net/can/sja1000/sja1000_platform.c
@@ -210,7 +210,6 @@ static int sp_probe(struct platform_device *pdev)
 	struct resource *res_mem, *res_irq = NULL;
 	struct sja1000_platform_data *pdata;
 	struct device_node *of = pdev->dev.of_node;
-	const struct of_device_id *of_id;
 	const struct sja1000_of_data *of_data = NULL;
 	size_t priv_sz = 0;
 
@@ -243,11 +242,9 @@ static int sp_probe(struct platform_device *pdev)
 			return -ENODEV;
 	}
 
-	of_id = of_match_device(sp_of_table, &pdev->dev);
-	if (of_id && of_id->data) {
-		of_data = of_id->data;
+	of_data = device_get_match_data(&pdev->dev);
+	if (of_data)
 		priv_sz = of_data->priv_sz;
-	}
 
 	dev = alloc_sja1000dev(priv_sz);
 	if (!dev)
-- 
2.25.1

[PATCH v3 5/6] can: sja1000: Change the return type as void for SoC specific init

[Biju Das]

Change the return type as void for SoC specific init function as it
always return 0.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v2->v3:
 * No change.
v1->v2:
 * No change.
---
 drivers/net/can/sja1000/sja1000_platform.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/net/can/sja1000/sja1000_platform.c b/drivers/net/can/sja1000/sja1000_platform.c
index 0b78568f5286..81bc741905fd 100644
--- a/drivers/net/can/sja1000/sja1000_platform.c
+++ b/drivers/net/can/sja1000/sja1000_platform.c
@@ -31,7 +31,7 @@ MODULE_LICENSE("GPL v2");
 
 struct sja1000_of_data {
 	size_t  priv_sz;
-	int     (*init)(struct sja1000_priv *priv, struct device_node *of);
+	void    (*init)(struct sja1000_priv *priv, struct device_node *of);
 };
 
 struct technologic_priv {
@@ -94,15 +94,13 @@ static void sp_technologic_write_reg16(const struct sja1000_priv *priv,
 	spin_unlock_irqrestore(&tp->io_lock, flags);
 }
 
-static int sp_technologic_init(struct sja1000_priv *priv, struct device_node *of)
+static void sp_technologic_init(struct sja1000_priv *priv, struct device_node *of)
 {
 	struct technologic_priv *tp = priv->priv;
 
 	priv->read_reg = sp_technologic_read_reg16;
 	priv->write_reg = sp_technologic_write_reg16;
 	spin_lock_init(&tp->io_lock);
-
-	return 0;
 }
 
 static void sp_populate(struct sja1000_priv *priv,
@@ -266,11 +264,8 @@ static int sp_probe(struct platform_device *pdev)
 	if (of) {
 		sp_populate_of(priv, of);
 
-		if (of_data && of_data->init) {
-			err = of_data->init(priv, of);
-			if (err)
-				goto exit_free;
-		}
+		if (of_data && of_data->init)
+			of_data->init(priv, of);
 	} else {
 		sp_populate(priv, pdata, res_mem->flags);
 	}
-- 
2.25.1

[PATCH v3 6/6] can: sja1000: Add support for RZ/N1 SJA1000 CAN Controller

[Biju Das]

The SJA1000 CAN controller on RZ/N1 SoC has no clock divider register
(CDR) support compared to others.

This patch adds support for RZ/N1 SJA1000 CAN Controller.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v2->v3:
 * No change.
v1->v2:
 * Updated commit description as SJA1000_NO_HW_LOOPBACK_QUIRK is removed
 * Added error handling on clk error path
 * Started using "devm_clk_get_optional_enabled" for clk get,prepare and enable.
---
 drivers/net/can/sja1000/sja1000_platform.c | 38 +++++++++++++++++++---
 1 file changed, 33 insertions(+), 5 deletions(-)

diff --git a/drivers/net/can/sja1000/sja1000_platform.c b/drivers/net/can/sja1000/sja1000_platform.c
index 81bc741905fd..757fdb5da191 100644
--- a/drivers/net/can/sja1000/sja1000_platform.c
+++ b/drivers/net/can/sja1000/sja1000_platform.c
@@ -14,6 +14,7 @@
 #include <linux/irq.h>
 #include <linux/can/dev.h>
 #include <linux/can/platform/sja1000.h>
+#include <linux/clk.h>
 #include <linux/io.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
@@ -103,6 +104,11 @@ static void sp_technologic_init(struct sja1000_priv *priv, struct device_node *o
 	spin_lock_init(&tp->io_lock);
 }
 
+static void sp_rzn1_init(struct sja1000_priv *priv, struct device_node *of)
+{
+	priv->flags = SJA1000_QUIRK_NO_CDR_REG;
+}
+
 static void sp_populate(struct sja1000_priv *priv,
 			struct sja1000_platform_data *pdata,
 			unsigned long resource_mem_flags)
@@ -153,11 +159,13 @@ static void sp_populate_of(struct sja1000_priv *priv, struct device_node *of)
 		priv->write_reg = sp_write_reg8;
 	}
 
-	err = of_property_read_u32(of, "nxp,external-clock-frequency", &prop);
-	if (!err)
-		priv->can.clock.freq = prop / 2;
-	else
-		priv->can.clock.freq = SP_CAN_CLOCK; /* default */
+	if (!priv->can.clock.freq) {
+		err = of_property_read_u32(of, "nxp,external-clock-frequency", &prop);
+		if (!err)
+			priv->can.clock.freq = prop / 2;
+		else
+			priv->can.clock.freq = SP_CAN_CLOCK; /* default */
+	}
 
 	err = of_property_read_u32(of, "nxp,tx-output-mode", &prop);
 	if (!err)
@@ -192,8 +200,13 @@ static struct sja1000_of_data technologic_data = {
 	.init = sp_technologic_init,
 };
 
+static struct sja1000_of_data renesas_data = {
+	.init = sp_rzn1_init,
+};
+
 static const struct of_device_id sp_of_table[] = {
 	{ .compatible = "nxp,sja1000", .data = NULL, },
+	{ .compatible = "renesas,rzn1-sja1000", .data = &renesas_data, },
 	{ .compatible = "technologic,sja1000", .data = &technologic_data, },
 	{ /* sentinel */ },
 };
@@ -210,6 +223,7 @@ static int sp_probe(struct platform_device *pdev)
 	struct device_node *of = pdev->dev.of_node;
 	const struct sja1000_of_data *of_data = NULL;
 	size_t priv_sz = 0;
+	struct clk *clk;
 
 	pdata = dev_get_platdata(&pdev->dev);
 	if (!pdata && !of) {
@@ -234,6 +248,11 @@ static int sp_probe(struct platform_device *pdev)
 		irq = platform_get_irq(pdev, 0);
 		if (irq < 0)
 			return irq;
+
+		clk = devm_clk_get_optional_enabled(&pdev->dev, "can_clk");
+		if (IS_ERR(clk))
+			return dev_err_probe(&pdev->dev, PTR_ERR(clk),
+					     "CAN clk operation failed");
 	} else {
 		res_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 		if (!res_irq)
@@ -262,6 +281,15 @@ static int sp_probe(struct platform_device *pdev)
 	priv->reg_base = addr;
 
 	if (of) {
+		if (clk) {
+			priv->can.clock.freq  = clk_get_rate(clk) / 2;
+			if (!priv->can.clock.freq) {
+				err = -EINVAL;
+				dev_err(&pdev->dev, "Zero CAN clk rate");
+				goto exit_free;
+			}
+		}
+
 		sp_populate_of(priv, of);
 
 		if (of_data && of_data->init)
-- 
2.25.1

Re: [PATCH v3 1/6] dt-bindings: can: sja1000: Convert to json-schema

[Krzysztof Kozlowski]

On 04/07/2022 09:50, Biju Das wrote:
> Convert the NXP SJA1000 CAN Controller Device Tree binding
> documentation to json-schema.
> 
> Update the example to match reality.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v2->v3:
>  * Added reg-io-width is a required property for technologic,sja1000
>  * Removed enum type from nxp,tx-output-config and updated the description
>    for combination of TX0 and TX1.
>  * Updated the example
> v1->v2:
>  * Moved $ref: can-controller.yaml# to top along with if conditional to
>    avoid multiple mapping issues with the if conditional in the subsequent
>    patch.
> ---
>  .../bindings/net/can/nxp,sja1000.yaml         | 103 ++++++++++++++++++
>  .../devicetree/bindings/net/can/sja1000.txt   |  58 ----------
>  2 files changed, 103 insertions(+), 58 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
>  delete mode 100644 Documentation/devicetree/bindings/net/can/sja1000.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml b/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
> new file mode 100644
> index 000000000000..d34060226e4e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
> @@ -0,0 +1,103 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/can/nxp,sja1000.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Memory mapped SJA1000 CAN controller from NXP (formerly Philips)
> +
> +maintainers:
> +  - Wolfgang Grandegger <wg@grandegger.com>
> +
> +allOf:
> +  - $ref: can-controller.yaml#
> +  - if:

The advice of moving it up was not correct. The allOf containing ref and
if:then goes to place like in example-schema, so before
additional/unevaluatedProperties at the bottom.

Please do not introduce some inconsistent style.

> +      properties:
> +        compatible:
> +          contains:
> +            const: technologic,sja1000
> +    then:
> +      required:
> +        - reg-io-width
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - description: NXP SJA1000 CAN Controller
> +        const: nxp,sja1000
> +      - description: Technologic Systems SJA1000 CAN Controller
> +        const: technologic,sja1000

Entire entry should be just enum. Descriptions do not bring any
information - they copy compatible.

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  reg-io-width:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: I/O register width (in bytes) implemented by this device
> +    default: 1
> +    enum: [ 1, 2, 4 ]
> +
> +  nxp,external-clock-frequency:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    default: 16000000
> +    description: |
> +      Frequency of the external oscillator clock in Hz.
> +      The internal clock frequency used by the SJA1000 is half of that value.
> +
> +  nxp,tx-output-mode:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [ 0, 1, 2, 3 ]
> +    default: 1
> +    description: |
> +      operation mode of the TX output control logic. Valid values are:
> +        <0x0> : bi-phase output mode
> +        <0x1> : normal output mode (default)
> +        <0x2> : test output mode
> +        <0x3> : clock output mode

Use decimal values, just like in enum.

> +
> +  nxp,tx-output-config:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    default: 0x02
> +    description: |
> +      TX output pin configuration. Valid values are any one of the below
> +      or combination of TX0 and TX1:
> +        <0x01> : TX0 invert
> +        <0x02> : TX0 pull-down (default)
> +        <0x04> : TX0 pull-up
> +        <0x06> : TX0 push-pull
> +        <0x08> : TX1 invert
> +        <0x10> : TX1 pull-down
> +        <0x20> : TX1 pull-up
> +        <0x30> : TX1 push-pull
> +
> +  nxp,clock-out-frequency:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      clock frequency in Hz on the CLKOUT pin.
> +      If not specified or if the specified value is 0, the CLKOUT pin
> +      will be disabled.
> +
> +  nxp,no-comparator-bypass:
> +    type: boolean
> +    description: Allows to disable the CAN input comparator.
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    can@1a000 {
> +            compatible = "technologic,sja1000";

Unusual indentation. Use 4 spaces for the DTS example.

> +            reg = <0x1a000 0x100>;
> +            interrupts = <1>;
> +            reg-io-width = <2>;
> +            nxp,tx-output-config = <0x06>;
> +            nxp,external-clock-frequency = <24000000>;
> +    };
> diff --git a/Documentation/devicetree/bindings/net/can/sja1000.txt b/Documentation/devicetree/bindings/net/can/sja1000.txt
> deleted file mode 100644
> index ac3160eca96a..000000000000
> --- a/Documentation/devicetree/bindings/net/can/sja1000.txt
> +++ /dev/null
> @@ -1,58 +0,0 @@
> -Memory mapped SJA1000 CAN controller from NXP (formerly Philips)
> -
> -Required properties:
> -
> -- compatible : should be one of "nxp,sja1000", "technologic,sja1000".
> -
> -- reg : should specify the chip select, address offset and size required
> -	to map the registers of the SJA1000. The size is usually 0x80.
> -
> -- interrupts: property with a value describing the interrupt source
> -	(number and sensitivity) required for the SJA1000.
> -
> -Optional properties:
> -
> -- reg-io-width : Specify the size (in bytes) of the IO accesses that
> -	should be performed on the device.  Valid value is 1, 2 or 4.
> -	This property is ignored for technologic version.
> -	Default to 1 (8 bits).
> -
> -- nxp,external-clock-frequency : Frequency of the external oscillator
> -	clock in Hz. Note that the internal clock frequency used by the
> -	SJA1000 is half of that value. If not specified, a default value
> -	of 16000000 (16 MHz) is used.
> -
> -- nxp,tx-output-mode : operation mode of the TX output control logic:
> -	<0x0> : bi-phase output mode
> -	<0x1> : normal output mode (default)
> -	<0x2> : test output mode
> -	<0x3> : clock output mode
> -
> -- nxp,tx-output-config : TX output pin configuration:
> -	<0x01> : TX0 invert
> -	<0x02> : TX0 pull-down (default)
> -	<0x04> : TX0 pull-up
> -	<0x06> : TX0 push-pull
> -	<0x08> : TX1 invert
> -	<0x10> : TX1 pull-down
> -	<0x20> : TX1 pull-up
> -	<0x30> : TX1 push-pull
> -
> -- nxp,clock-out-frequency : clock frequency in Hz on the CLKOUT pin.
> -	If not specified or if the specified value is 0, the CLKOUT pin
> -	will be disabled.
> -
> -- nxp,no-comparator-bypass : Allows to disable the CAN input comparator.
> -
> -For further information, please have a look to the SJA1000 data sheet.
> -
> -Examples:
> -
> -can@3,100 {
> -	compatible = "nxp,sja1000";
> -	reg = <3 0x100 0x80>;
> -	interrupts = <2 0>;
> -	interrupt-parent = <&mpic>;
> -	nxp,external-clock-frequency = <16000000>;
> -};
> -


Best regards,
Krzysztof

Re: [PATCH v3 2/6] dt-bindings: can: nxp,sja1000: Document RZ/N1{D,S} support

[Krzysztof Kozlowski]

On 04/07/2022 09:50, Biju Das wrote:
> Add CAN binding documentation for Renesas RZ/N1 SoC.
> 
> The SJA1000 CAN controller on RZ/N1 SoC has some differences compared
> to others like it has no clock divider register (CDR) support and it has
> no HW loopback (HW doesn't see tx messages on rx), so introduced a new
> compatible 'renesas,rzn1-sja1000' to handle these differences.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v2->v3:
>  * Added reg-io-width is required property for renesas,rzn1-sja1000.
> v1->v2:
>  * Updated commit description.
>  * Added an example for RZ/N1D SJA1000 usage
> ---
>  .../bindings/net/can/nxp,sja1000.yaml         | 35 +++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml b/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
> index d34060226e4e..16786475eae3 100644
> --- a/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
> +++ b/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
> @@ -19,6 +19,16 @@ allOf:
>      then:
>        required:
>          - reg-io-width
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: renesas,rzn1-sja1000
> +    then:
> +      required:
> +        - clocks
> +        - clock-names
> +        - reg-io-width
>  
>  properties:
>    compatible:
> @@ -27,6 +37,12 @@ properties:
>          const: nxp,sja1000
>        - description: Technologic Systems SJA1000 CAN Controller
>          const: technologic,sja1000
> +      - description: Renesas RZ/N1 SJA1000 CAN Controller
> +        items:
> +          - enum:
> +              - renesas,r9a06g032-sja1000 # RZ/N1D
> +              - renesas,r9a06g033-sja1000 # RZ/N1S
> +          - const: renesas,rzn1-sja1000 # RZ/N1

This explains usage of oneOf, but still earlier entries should be just
an enum.

>  
>    reg:
>      maxItems: 1
> @@ -34,6 +50,12 @@ properties:
>    interrupts:
>      maxItems: 1
>  
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    const: can_clk

Skip entire clock-names. Does not bring any information, especially that
name is obvious.

> +
>    reg-io-width:
>      $ref: /schemas/types.yaml#/definitions/uint32
>      description: I/O register width (in bytes) implemented by this device
> @@ -101,3 +123,16 @@ examples:
>              nxp,tx-output-config = <0x06>;
>              nxp,external-clock-frequency = <24000000>;
>      };
> +
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/clock/r9a06g032-sysctrl.h>
> +
> +    can@52104000 {
> +            compatible = "renesas,r9a06g032-sja1000","renesas,rzn1-sja1000";

Missing space after ,

Wrong indentation.

> +            reg = <0x52104000 0x800>;
> +            reg-io-width = <4>;
> +            interrupts = <GIC_SPI 95 IRQ_TYPE_LEVEL_HIGH>;
> +            clocks = <&sysctrl R9A06G032_HCLK_CAN0>;
> +            clock-names = "can_clk";
> +    };


Best regards,
Krzysztof

RE: [PATCH v3 1/6] dt-bindings: can: sja1000: Convert to json-schema

[Biju Das]

Hi Krystof,

Thanks for the feedback.

> Subject: Re: [PATCH v3 1/6] dt-bindings: can: sja1000: Convert to json-
> schema
> 
> On 04/07/2022 09:50, Biju Das wrote:
> > Convert the NXP SJA1000 CAN Controller Device Tree binding
> > documentation to json-schema.
> >
> > Update the example to match reality.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > v2->v3:
> >  * Added reg-io-width is a required property for technologic,sja1000
> >  * Removed enum type from nxp,tx-output-config and updated the
> description
> >    for combination of TX0 and TX1.
> >  * Updated the example
> > v1->v2:
> >  * Moved $ref: can-controller.yaml# to top along with if conditional
> > to
> >    avoid multiple mapping issues with the if conditional in the
> subsequent
> >    patch.
> > ---
> >  .../bindings/net/can/nxp,sja1000.yaml         | 103 ++++++++++++++++++
> >  .../devicetree/bindings/net/can/sja1000.txt   |  58 ----------
> >  2 files changed, 103 insertions(+), 58 deletions(-)  create mode
> > 100644 Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
> >  delete mode 100644
> > Documentation/devicetree/bindings/net/can/sja1000.txt
> >
> > diff --git
> > a/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
> > b/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
> > new file mode 100644
> > index 000000000000..d34060226e4e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
> > @@ -0,0 +1,103 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id:
> > +
> > +title: Memory mapped SJA1000 CAN controller from NXP (formerly
> > +Philips)
> > +
> > +maintainers:
> > +  - Wolfgang Grandegger <wg@grandegger.com>
> > +
> > +allOf:
> > +  - $ref: can-controller.yaml#
> > +  - if:
> 
> The advice of moving it up was not correct. The allOf containing ref and
> if:then goes to place like in example-schema, so before
> additional/unevaluatedProperties at the bottom.
> 
> Please do not introduce some inconsistent style.

There are some examples like[1], where allOf is at the top.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml?h=next-20220704

Marc, please let us know, if you still prefer allOf at the top.

> 
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: technologic,sja1000
> > +    then:
> > +      required:
> > +        - reg-io-width
> > +
> > +properties:
> > +  compatible:
> > +    oneOf:
> > +      - description: NXP SJA1000 CAN Controller
> > +        const: nxp,sja1000
> > +      - description: Technologic Systems SJA1000 CAN Controller
> > +        const: technologic,sja1000
> 
> Entire entry should be just enum. Descriptions do not bring any
> information - they copy compatible.

OK, I will remove description and will use enum.

> 
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  reg-io-width:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: I/O register width (in bytes) implemented by this
> device
> > +    default: 1
> > +    enum: [ 1, 2, 4 ]
> > +
> > +  nxp,external-clock-frequency:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    default: 16000000
> > +    description: |
> > +      Frequency of the external oscillator clock in Hz.
> > +      The internal clock frequency used by the SJA1000 is half of that
> value.
> > +
> > +  nxp,tx-output-mode:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [ 0, 1, 2, 3 ]
> > +    default: 1
> > +    description: |
> > +      operation mode of the TX output control logic. Valid values are:
> > +        <0x0> : bi-phase output mode
> > +        <0x1> : normal output mode (default)
> > +        <0x2> : test output mode
> > +        <0x3> : clock output mode
> 
> Use decimal values, just like in enum.

OK.

> 
> > +
> > +  nxp,tx-output-config:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    default: 0x02
> > +    description: |
> > +      TX output pin configuration. Valid values are any one of the
> below
> > +      or combination of TX0 and TX1:
> > +        <0x01> : TX0 invert
> > +        <0x02> : TX0 pull-down (default)
> > +        <0x04> : TX0 pull-up
> > +        <0x06> : TX0 push-pull
> > +        <0x08> : TX1 invert
> > +        <0x10> : TX1 pull-down
> > +        <0x20> : TX1 pull-up
> > +        <0x30> : TX1 push-pull
> > +
> > +  nxp,clock-out-frequency:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: |
> > +      clock frequency in Hz on the CLKOUT pin.
> > +      If not specified or if the specified value is 0, the CLKOUT pin
> > +      will be disabled.
> > +
> > +  nxp,no-comparator-bypass:
> > +    type: boolean
> > +    description: Allows to disable the CAN input comparator.
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    can@1a000 {
> > +            compatible = "technologic,sja1000";
> 
> Unusual indentation. Use 4 spaces for the DTS example.

OK, Will do.

Cheers,
Biju

> 
> > +            reg = <0x1a000 0x100>;
> > +            interrupts = <1>;
> > +            reg-io-width = <2>;
> > +            nxp,tx-output-config = <0x06>;
> > +            nxp,external-clock-frequency = <24000000>;
> > +    };
> > diff --git a/Documentation/devicetree/bindings/net/can/sja1000.txt
> > b/Documentation/devicetree/bindings/net/can/sja1000.txt
> > deleted file mode 100644
> > index ac3160eca96a..000000000000
> > --- a/Documentation/devicetree/bindings/net/can/sja1000.txt
> > +++ /dev/null
> > @@ -1,58 +0,0 @@
> > -Memory mapped SJA1000 CAN controller from NXP (formerly Philips)
> > -
> > -Required properties:
> > -
> > -- compatible : should be one of "nxp,sja1000", "technologic,sja1000".
> > -
> > -- reg : should specify the chip select, address offset and size
> required
> > -	to map the registers of the SJA1000. The size is usually 0x80.
> > -
> > -- interrupts: property with a value describing the interrupt source
> > -	(number and sensitivity) required for the SJA1000.
> > -
> > -Optional properties:
> > -
> > -- reg-io-width : Specify the size (in bytes) of the IO accesses that
> > -	should be performed on the device.  Valid value is 1, 2 or 4.
> > -	This property is ignored for technologic version.
> > -	Default to 1 (8 bits).
> > -
> > -- nxp,external-clock-frequency : Frequency of the external oscillator
> > -	clock in Hz. Note that the internal clock frequency used by the
> > -	SJA1000 is half of that value. If not specified, a default value
> > -	of 16000000 (16 MHz) is used.
> > -
> > -- nxp,tx-output-mode : operation mode of the TX output control logic:
> > -	<0x0> : bi-phase output mode
> > -	<0x1> : normal output mode (default)
> > -	<0x2> : test output mode
> > -	<0x3> : clock output mode
> > -
> > -- nxp,tx-output-config : TX output pin configuration:
> > -	<0x01> : TX0 invert
> > -	<0x02> : TX0 pull-down (default)
> > -	<0x04> : TX0 pull-up
> > -	<0x06> : TX0 push-pull
> > -	<0x08> : TX1 invert
> > -	<0x10> : TX1 pull-down
> > -	<0x20> : TX1 pull-up
> > -	<0x30> : TX1 push-pull
> > -
> > -- nxp,clock-out-frequency : clock frequency in Hz on the CLKOUT pin.
> > -	If not specified or if the specified value is 0, the CLKOUT pin
> > -	will be disabled.
> > -
> > -- nxp,no-comparator-bypass : Allows to disable the CAN input
> comparator.
> > -
> > -For further information, please have a look to the SJA1000 data sheet.
> > -
> > -Examples:
> > -
> > -can@3,100 {
> > -	compatible = "nxp,sja1000";
> > -	reg = <3 0x100 0x80>;
> > -	interrupts = <2 0>;
> > -	interrupt-parent = <&mpic>;
> > -	nxp,external-clock-frequency = <16000000>;
> > -};
> > -

RE: [PATCH v3 2/6] dt-bindings: can: nxp,sja1000: Document RZ/N1{D,S} support

[Biju Das]

Hi Krzysztof,

Thanks for the feedback.

> Subject: Re: [PATCH v3 2/6] dt-bindings: can: nxp,sja1000: Document
> RZ/N1{D,S} support
> 
> On 04/07/2022 09:50, Biju Das wrote:
> > Add CAN binding documentation for Renesas RZ/N1 SoC.
> >
> > The SJA1000 CAN controller on RZ/N1 SoC has some differences compared
> > to others like it has no clock divider register (CDR) support and it
> > has no HW loopback (HW doesn't see tx messages on rx), so introduced a
> > new compatible 'renesas,rzn1-sja1000' to handle these differences.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > v2->v3:
> >  * Added reg-io-width is required property for renesas,rzn1-sja1000.
> > v1->v2:
> >  * Updated commit description.
> >  * Added an example for RZ/N1D SJA1000 usage
> > ---
> >  .../bindings/net/can/nxp,sja1000.yaml         | 35 +++++++++++++++++++
> >  1 file changed, 35 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
> > b/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
> > index d34060226e4e..16786475eae3 100644
> > --- a/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
> > +++ b/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
> > @@ -19,6 +19,16 @@ allOf:
> >      then:
> >        required:
> >          - reg-io-width
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: renesas,rzn1-sja1000
> > +    then:
> > +      required:
> > +        - clocks
> > +        - clock-names
> > +        - reg-io-width
> >
> >  properties:
> >    compatible:
> > @@ -27,6 +37,12 @@ properties:
> >          const: nxp,sja1000
> >        - description: Technologic Systems SJA1000 CAN Controller
> >          const: technologic,sja1000
> > +      - description: Renesas RZ/N1 SJA1000 CAN Controller
> > +        items:
> > +          - enum:
> > +              - renesas,r9a06g032-sja1000 # RZ/N1D
> > +              - renesas,r9a06g033-sja1000 # RZ/N1S
> > +          - const: renesas,rzn1-sja1000 # RZ/N1
> 
> This explains usage of oneOf, but still earlier entries should be just an
> enum.

OK.

> 
> >
> >    reg:
> >      maxItems: 1
> > @@ -34,6 +50,12 @@ properties:
> >    interrupts:
> >      maxItems: 1
> >
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  clock-names:
> > +    const: can_clk
> 
> Skip entire clock-names. Does not bring any information, especially that
> name is obvious.

Yes, true for single clock, clock-names doesn't make any value.
Will drop it.

> 
> > +
> >    reg-io-width:
> >      $ref: /schemas/types.yaml#/definitions/uint32
> >      description: I/O register width (in bytes) implemented by this
> > device @@ -101,3 +123,16 @@ examples:
> >              nxp,tx-output-config = <0x06>;
> >              nxp,external-clock-frequency = <24000000>;
> >      };
> > +
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    #include <dt-bindings/clock/r9a06g032-sysctrl.h>
> > +
> > +    can@52104000 {
> > +            compatible =
> > + "renesas,r9a06g032-sja1000","renesas,rzn1-sja1000";
> 
> Missing space after ,
OK.

> 
> Wrong indentation.

As you suggested in the previous patch, will make 4 spaces indentation for examples.

Cheers,
Biju

Re: [PATCH v3 1/6] dt-bindings: can: sja1000: Convert to json-schema

[Krzysztof Kozlowski]

On 04/07/2022 11:03, Biju Das wrote:
> Hi Krystof,
> 
> Thanks for the feedback.
> 
>> Subject: Re: [PATCH v3 1/6] dt-bindings: can: sja1000: Convert to json-
>> schema
>>
>> On 04/07/2022 09:50, Biju Das wrote:
>>> Convert the NXP SJA1000 CAN Controller Device Tree binding
>>> documentation to json-schema.
>>>
>>> Update the example to match reality.
>>>
>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
>>> ---
>>> v2->v3:
>>>  * Added reg-io-width is a required property for technologic,sja1000
>>>  * Removed enum type from nxp,tx-output-config and updated the
>> description
>>>    for combination of TX0 and TX1.
>>>  * Updated the example
>>> v1->v2:
>>>  * Moved $ref: can-controller.yaml# to top along with if conditional
>>> to
>>>    avoid multiple mapping issues with the if conditional in the
>> subsequent
>>>    patch.
>>> ---
>>>  .../bindings/net/can/nxp,sja1000.yaml         | 103 ++++++++++++++++++
>>>  .../devicetree/bindings/net/can/sja1000.txt   |  58 ----------
>>>  2 files changed, 103 insertions(+), 58 deletions(-)  create mode
>>> 100644 Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
>>>  delete mode 100644
>>> Documentation/devicetree/bindings/net/can/sja1000.txt
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
>>> b/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
>>> new file mode 100644
>>> index 000000000000..d34060226e4e
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
>>> @@ -0,0 +1,103 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
>>> +---
>>> +$id:
>>> +
>>> +title: Memory mapped SJA1000 CAN controller from NXP (formerly
>>> +Philips)
>>> +
>>> +maintainers:
>>> +  - Wolfgang Grandegger <wg@grandegger.com>
>>> +
>>> +allOf:
>>> +  - $ref: can-controller.yaml#
>>> +  - if:
>>
>> The advice of moving it up was not correct. The allOf containing ref and
>> if:then goes to place like in example-schema, so before
>> additional/unevaluatedProperties at the bottom.
>>
>> Please do not introduce some inconsistent style.
> 
> There are some examples like[1], where allOf is at the top.
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml?h=next-20220704

And they are wrong. There is always some incorrect code in the kernel,
but that's not argument to do it in incorrect way. The coding style is
here expressed in example-schema, so use this as an argument.

> 
> Marc, please let us know, if you still prefer allOf at the top.
> 
>>


Best regards,
Krzysztof

RE: [PATCH v3 1/6] dt-bindings: can: sja1000: Convert to json-schema

[Biju Das]

Hi Krzysztof,

Thanks for the feedback.

> Subject: Re: [PATCH v3 1/6] dt-bindings: can: sja1000: Convert to json-
> schema
> 
> On 04/07/2022 11:03, Biju Das wrote:
> > Hi Krystof,
> >
> > Thanks for the feedback.
> >
> >> Subject: Re: [PATCH v3 1/6] dt-bindings: can: sja1000: Convert to
> >> json- schema
> >>
> >> On 04/07/2022 09:50, Biju Das wrote:
> >>> Convert the NXP SJA1000 CAN Controller Device Tree binding
> >>> documentation to json-schema.
> >>>
> >>> Update the example to match reality.
> >>>
> >>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> >>> ---
> >>> v2->v3:
> >>>  * Added reg-io-width is a required property for technologic,sja1000
> >>>  * Removed enum type from nxp,tx-output-config and updated the
> >> description
> >>>    for combination of TX0 and TX1.
> >>>  * Updated the example
> >>> v1->v2:
> >>>  * Moved $ref: can-controller.yaml# to top along with if conditional
> >>> to
> >>>    avoid multiple mapping issues with the if conditional in the
> >> subsequent
> >>>    patch.
> >>> ---
> >>>  .../bindings/net/can/nxp,sja1000.yaml         | 103
> ++++++++++++++++++
> >>>  .../devicetree/bindings/net/can/sja1000.txt   |  58 ----------
> >>>  2 files changed, 103 insertions(+), 58 deletions(-)  create mode
> >>> 100644 Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
> >>>  delete mode 100644
> >>> Documentation/devicetree/bindings/net/can/sja1000.txt
> >>>
> >>> diff --git
> >>> a/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
> >>> b/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
> >>> new file mode 100644
> >>> index 000000000000..d34060226e4e
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
> >>> @@ -0,0 +1,103 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> >>> +---
> >>> +$id:
> >>> +
> >>> +title: Memory mapped SJA1000 CAN controller from NXP (formerly
> >>> +Philips)
> >>> +
> >>> +maintainers:
> >>> +  - Wolfgang Grandegger <wg@grandegger.com>
> >>> +
> >>> +allOf:
> >>> +  - $ref: can-controller.yaml#
> >>> +  - if:
> >>
> >> The advice of moving it up was not correct. The allOf containing ref
> >> and if:then goes to place like in example-schema, so before
> >> additional/unevaluatedProperties at the bottom.
> >>
> >> Please do not introduce some inconsistent style.
> >
> > There are some examples like[1], where allOf is at the top.
> > [1]
=Mw4Fhkri5BLK1Cqg8Wd1EKkbe0xDg%2Fnbl0JSd5j6Kmo%3D&amp;reser
> > ved=0
> 
> And they are wrong. There is always some incorrect code in the kernel,
> but that's not argument to do it in incorrect way. The coding style is
> here expressed in example-schema, so use this as an argument.

OK. Will stick to example-schema as coding style.

Cheers,
Biju

Re: [PATCH v3 3/6] can: sja1000: Add Quirk for RZ/N1 SJA1000 CAN controller

[Vincent MAILHOL]

Hi Biju,

I gave a quick look to your series. Nothing odd to me, I just have a
single nitpick.

On Mon. 4 juil. 2022 at 16:51, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> As per Chapter 6.5.16 of the RZ/N1 Peripheral Manual, The SJA1000
> CAN controller does not support Clock Divider Register compared to
> the reference Philips SJA1000 device.
>
> This patch adds a device quirk to handle this difference.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v2->v3:
>  * No Change
> v1->v2:
>  * Updated commit description
>  * Removed the quirk macro SJA1000_NO_HW_LOOPBACK_QUIRK
>  * Added prefix SJA1000_QUIRK_* for quirk macro.
> ---
>  drivers/net/can/sja1000/sja1000.c | 13 ++++++++-----
>  drivers/net/can/sja1000/sja1000.h |  3 ++-
>  2 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
> index 2e7638f98cf1..e9c55f5aa3c3 100644
> --- a/drivers/net/can/sja1000/sja1000.c
> +++ b/drivers/net/can/sja1000/sja1000.c
> @@ -183,8 +183,9 @@ static void chipset_init(struct net_device *dev)
>  {
>         struct sja1000_priv *priv = netdev_priv(dev);
>
> -       /* set clock divider and output control register */
> -       priv->write_reg(priv, SJA1000_CDR, priv->cdr | CDR_PELICAN);
> +       if (!(priv->flags & SJA1000_QUIRK_NO_CDR_REG))
> +               /* set clock divider and output control register */
> +               priv->write_reg(priv, SJA1000_CDR, priv->cdr | CDR_PELICAN);
>
>         /* set acceptance filter (accept all) */
>         priv->write_reg(priv, SJA1000_ACCC0, 0x00);
> @@ -208,9 +209,11 @@ static void sja1000_start(struct net_device *dev)
>         if (priv->can.state != CAN_STATE_STOPPED)
>                 set_reset_mode(dev);
>
> -       /* Initialize chip if uninitialized at this stage */
> -       if (!(priv->read_reg(priv, SJA1000_CDR) & CDR_PELICAN))
> -               chipset_init(dev);
> +       if (!(priv->flags & SJA1000_QUIRK_NO_CDR_REG)) {
> +               /* Initialize chip if uninitialized at this stage */
> +               if (!(priv->read_reg(priv, SJA1000_CDR) & CDR_PELICAN))

You can combine the two if in one:

|        /* Initialize chip if uninitialized at this stage */
|        if (!(priv->flags & SJA1000_QUIRK_NO_CDR_REG ||
|              priv->read_reg(priv, SJA1000_CDR) & CDR_PELICAN))
|                chipset_init(dev);

> +                       chipset_init(dev);
> +       }
>
>         /* Clear error counters and error code capture */
>         priv->write_reg(priv, SJA1000_TXERR, 0x0);
> diff --git a/drivers/net/can/sja1000/sja1000.h b/drivers/net/can/sja1000/sja1000.h
> index 9d46398f8154..7f736f1df547 100644
> --- a/drivers/net/can/sja1000/sja1000.h
> +++ b/drivers/net/can/sja1000/sja1000.h
> @@ -145,7 +145,8 @@
>  /*
>   * Flags for sja1000priv.flags
>   */
> -#define SJA1000_CUSTOM_IRQ_HANDLER 0x1
> +#define SJA1000_CUSTOM_IRQ_HANDLER     BIT(0)
> +#define SJA1000_QUIRK_NO_CDR_REG       BIT(1)
>
>  /*
>   * SJA1000 private data structure

RE: [PATCH v3 3/6] can: sja1000: Add Quirk for RZ/N1 SJA1000 CAN controller

[Biju Das]

Hi Vincent,

Thanks for the feedback.

> Subject: Re: [PATCH v3 3/6] can: sja1000: Add Quirk for RZ/N1 SJA1000 CAN
> controller
> 
> Hi Biju,
> 
> I gave a quick look to your series. Nothing odd to me, I just have a
> single nitpick.
> 
> On Mon. 4 juil. 2022 at 16:51, Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > As per Chapter 6.5.16 of the RZ/N1 Peripheral Manual, The SJA1000 CAN
> > controller does not support Clock Divider Register compared to the
> > reference Philips SJA1000 device.
> >
> > This patch adds a device quirk to handle this difference.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > v2->v3:
> >  * No Change
> > v1->v2:
> >  * Updated commit description
> >  * Removed the quirk macro SJA1000_NO_HW_LOOPBACK_QUIRK
> >  * Added prefix SJA1000_QUIRK_* for quirk macro.
> > ---
> >  drivers/net/can/sja1000/sja1000.c | 13 ++++++++-----
> > drivers/net/can/sja1000/sja1000.h |  3 ++-
> >  2 files changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/can/sja1000/sja1000.c
> > b/drivers/net/can/sja1000/sja1000.c
> > index 2e7638f98cf1..e9c55f5aa3c3 100644
> > --- a/drivers/net/can/sja1000/sja1000.c
> > +++ b/drivers/net/can/sja1000/sja1000.c
> > @@ -183,8 +183,9 @@ static void chipset_init(struct net_device *dev)
> > {
> >         struct sja1000_priv *priv = netdev_priv(dev);
> >
> > -       /* set clock divider and output control register */
> > -       priv->write_reg(priv, SJA1000_CDR, priv->cdr | CDR_PELICAN);
> > +       if (!(priv->flags & SJA1000_QUIRK_NO_CDR_REG))
> > +               /* set clock divider and output control register */
> > +               priv->write_reg(priv, SJA1000_CDR, priv->cdr |
> > + CDR_PELICAN);
> >
> >         /* set acceptance filter (accept all) */
> >         priv->write_reg(priv, SJA1000_ACCC0, 0x00); @@ -208,9 +209,11
> > @@ static void sja1000_start(struct net_device *dev)
> >         if (priv->can.state != CAN_STATE_STOPPED)
> >                 set_reset_mode(dev);
> >
> > -       /* Initialize chip if uninitialized at this stage */
> > -       if (!(priv->read_reg(priv, SJA1000_CDR) & CDR_PELICAN))
> > -               chipset_init(dev);
> > +       if (!(priv->flags & SJA1000_QUIRK_NO_CDR_REG)) {
> > +               /* Initialize chip if uninitialized at this stage */
> > +               if (!(priv->read_reg(priv, SJA1000_CDR) &
> > + CDR_PELICAN))
> 
> You can combine the two if in one:
> 
> |        /* Initialize chip if uninitialized at this stage */
> |        if (!(priv->flags & SJA1000_QUIRK_NO_CDR_REG ||
> |              priv->read_reg(priv, SJA1000_CDR) & CDR_PELICAN))
> |                chipset_init(dev);
> 
> > +                       chipset_init(dev);
> > +       }

OK, will fix this in next version.

Cheers,
Biju

> >
> >         /* Clear error counters and error code capture */
> >         priv->write_reg(priv, SJA1000_TXERR, 0x0); diff --git
> > a/drivers/net/can/sja1000/sja1000.h
> > b/drivers/net/can/sja1000/sja1000.h
> > index 9d46398f8154..7f736f1df547 100644
> > --- a/drivers/net/can/sja1000/sja1000.h
> > +++ b/drivers/net/can/sja1000/sja1000.h
> > @@ -145,7 +145,8 @@
> >  /*
> >   * Flags for sja1000priv.flags
> >   */
> > -#define SJA1000_CUSTOM_IRQ_HANDLER 0x1
> > +#define SJA1000_CUSTOM_IRQ_HANDLER     BIT(0)
> > +#define SJA1000_QUIRK_NO_CDR_REG       BIT(1)
> >
> >  /*
> >   * SJA1000 private data structure