All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/6] i2c: mpc: Refactor to improve responsiveness
@ 2021-03-23  4:33 Chris Packham
  2021-03-23  4:33 ` [PATCH 1/6] dt-bindings: i2c-mpc: Document interrupt property as required Chris Packham
                   ` (6 more replies)
  0 siblings, 7 replies; 15+ messages in thread
From: Chris Packham @ 2021-03-23  4:33 UTC (permalink / raw)
  To: robh+dt, linux, wsa, jdelvare
  Cc: linux-i2c, devicetree, linux-kernel, Chris Packham

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.

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

-- 
2.30.2


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/6] dt-bindings: i2c-mpc: Document interrupt property as required
  2021-03-23  4:33 [PATCH 0/6] i2c: mpc: Refactor to improve responsiveness Chris Packham
@ 2021-03-23  4:33 ` Chris Packham
  2021-03-23  4:33 ` [PATCH 2/6] dt-bindings: i2c: convert i2c-mpc to json-schema Chris Packham
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 15+ messages in thread
From: Chris Packham @ 2021-03-23  4:33 UTC (permalink / raw)
  To: robh+dt, linux, wsa, jdelvare
  Cc: linux-i2c, devicetree, linux-kernel, Chris Packham

All of the in-tree device-trees that use the one of the compatible
strings from i2c-mpc.c supply an interrupts property. Make this property
mandatory to aid refactoring the driver.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 Documentation/devicetree/bindings/i2c/i2c-mpc.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mpc.txt b/Documentation/devicetree/bindings/i2c/i2c-mpc.txt
index 42a390526957..b15acb43d84d 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mpc.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mpc.txt
@@ -7,14 +7,14 @@ Required properties :
    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.
-
-Recommended properties :
-
  - 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.
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 2/6] dt-bindings: i2c: convert i2c-mpc to json-schema
  2021-03-23  4:33 [PATCH 0/6] i2c: mpc: Refactor to improve responsiveness Chris Packham
  2021-03-23  4:33 ` [PATCH 1/6] dt-bindings: i2c-mpc: Document interrupt property as required Chris Packham
@ 2021-03-23  4:33 ` Chris Packham
  2021-03-23 20:16   ` Rob Herring
  2021-03-23 21:15   ` Rob Herring
  2021-03-23  4:33 ` [PATCH 3/6] i2c: mpc: Make use of i2c_recover_bus() Chris Packham
                   ` (4 subsequent siblings)
  6 siblings, 2 replies; 15+ messages in thread
From: Chris Packham @ 2021-03-23  4:33 UTC (permalink / raw)
  To: robh+dt, linux, wsa, jdelvare
  Cc: linux-i2c, devicetree, linux-kernel, Chris Packham

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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 3/6] i2c: mpc: Make use of i2c_recover_bus()
  2021-03-23  4:33 [PATCH 0/6] i2c: mpc: Refactor to improve responsiveness Chris Packham
  2021-03-23  4:33 ` [PATCH 1/6] dt-bindings: i2c-mpc: Document interrupt property as required Chris Packham
  2021-03-23  4:33 ` [PATCH 2/6] dt-bindings: i2c: convert i2c-mpc to json-schema Chris Packham
@ 2021-03-23  4:33 ` Chris Packham
  2021-03-23  4:33 ` [PATCH 4/6] i2c: mpc: make interrupt mandatory and remove polling code Chris Packham
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 15+ messages in thread
From: Chris Packham @ 2021-03-23  4:33 UTC (permalink / raw)
  To: robh+dt, linux, wsa, jdelvare
  Cc: linux-i2c, devicetree, linux-kernel, Chris Packham

Move the existing calls of mpc_i2c_fixup() to a recovery function
registered via bus_recovery_info. This makes it more obvious that
recovery is supported and allows for a future where recovery is
triggered by the i2c core.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 drivers/i2c/busses/i2c-mpc.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index d94f05c8b8b7..6a0d55e9e8e3 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -586,7 +586,7 @@ static int mpc_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 			if ((status & (CSR_MCF | CSR_MBB | CSR_RXAK)) != 0) {
 				writeb(status & ~CSR_MAL,
 				       i2c->base + MPC_I2C_SR);
-				mpc_i2c_fixup(i2c);
+				i2c_recover_bus(&i2c->adap);
 			}
 			return -EIO;
 		}
@@ -622,7 +622,7 @@ static int mpc_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 			if ((status & (CSR_MCF | CSR_MBB | CSR_RXAK)) != 0) {
 				writeb(status & ~CSR_MAL,
 				       i2c->base + MPC_I2C_SR);
-				mpc_i2c_fixup(i2c);
+				i2c_recover_bus(&i2c->adap);
 			}
 			return -EIO;
 		}
@@ -637,6 +637,15 @@ static u32 mpc_functionality(struct i2c_adapter *adap)
 	  | I2C_FUNC_SMBUS_READ_BLOCK_DATA | I2C_FUNC_SMBUS_BLOCK_PROC_CALL;
 }
 
+static int fsl_i2c_bus_recovery(struct i2c_adapter *adap)
+{
+	struct mpc_i2c *i2c = i2c_get_adapdata(adap);
+
+	mpc_i2c_fixup(i2c);
+
+	return 0;
+}
+
 static const struct i2c_algorithm mpc_algo = {
 	.master_xfer = mpc_xfer,
 	.functionality = mpc_functionality,
@@ -648,6 +657,10 @@ static struct i2c_adapter mpc_ops = {
 	.timeout = HZ,
 };
 
+static struct i2c_bus_recovery_info fsl_i2c_recovery_info = {
+	.recover_bus = fsl_i2c_bus_recovery,
+};
+
 static const struct of_device_id mpc_i2c_of_match[];
 static int fsl_i2c_probe(struct platform_device *op)
 {
@@ -740,6 +753,7 @@ static int fsl_i2c_probe(struct platform_device *op)
 	i2c_set_adapdata(&i2c->adap, i2c);
 	i2c->adap.dev.parent = &op->dev;
 	i2c->adap.dev.of_node = of_node_get(op->dev.of_node);
+	i2c->adap.bus_recovery_info = &fsl_i2c_recovery_info;
 
 	result = i2c_add_adapter(&i2c->adap);
 	if (result < 0)
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 4/6] i2c: mpc: make interrupt mandatory and remove polling code
  2021-03-23  4:33 [PATCH 0/6] i2c: mpc: Refactor to improve responsiveness Chris Packham
                   ` (2 preceding siblings ...)
  2021-03-23  4:33 ` [PATCH 3/6] i2c: mpc: Make use of i2c_recover_bus() Chris Packham
@ 2021-03-23  4:33 ` Chris Packham
  2021-03-23  4:33 ` [PATCH 5/6] i2c: mpc: use device managed APIs Chris Packham
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 15+ messages in thread
From: Chris Packham @ 2021-03-23  4:33 UTC (permalink / raw)
  To: robh+dt, linux, wsa, jdelvare
  Cc: linux-i2c, devicetree, linux-kernel, Chris Packham

All the in-tree dts files that use one of the compatible strings from
i2c-mpc.c provide an interrupt property. By making this mandatory we
can simplify the code.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 drivers/i2c/busses/i2c-mpc.c | 51 ++++++++++++++----------------------
 1 file changed, 19 insertions(+), 32 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index 6a0d55e9e8e3..5b746a898e8e 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -123,37 +123,21 @@ static void mpc_i2c_fixup(struct mpc_i2c *i2c)
 
 static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing)
 {
-	unsigned long orig_jiffies = jiffies;
 	u32 cmd_err;
-	int result = 0;
+	int result;
 
-	if (!i2c->irq) {
-		while (!(readb(i2c->base + MPC_I2C_SR) & CSR_MIF)) {
-			schedule();
-			if (time_after(jiffies, orig_jiffies + timeout)) {
-				dev_dbg(i2c->dev, "timeout\n");
-				writeccr(i2c, 0);
-				result = -ETIMEDOUT;
-				break;
-			}
-		}
-		cmd_err = readb(i2c->base + MPC_I2C_SR);
-		writeb(0, i2c->base + MPC_I2C_SR);
-	} else {
-		/* Interrupt mode */
-		result = wait_event_timeout(i2c->queue,
+	result = wait_event_timeout(i2c->queue,
 			(i2c->interrupt & CSR_MIF), timeout);
 
-		if (unlikely(!(i2c->interrupt & CSR_MIF))) {
-			dev_dbg(i2c->dev, "wait timeout\n");
-			writeccr(i2c, 0);
-			result = -ETIMEDOUT;
-		}
-
-		cmd_err = i2c->interrupt;
-		i2c->interrupt = 0;
+	if (unlikely(!(i2c->interrupt & CSR_MIF))) {
+		dev_dbg(i2c->dev, "wait timeout\n");
+		writeccr(i2c, 0);
+		result = -ETIMEDOUT;
 	}
 
+	cmd_err = i2c->interrupt;
+	i2c->interrupt = 0;
+
 	if (result < 0)
 		return result;
 
@@ -694,13 +678,16 @@ static int fsl_i2c_probe(struct platform_device *op)
 	}
 
 	i2c->irq = irq_of_parse_and_map(op->dev.of_node, 0);
-	if (i2c->irq) { /* no i2c->irq implies polling */
-		result = request_irq(i2c->irq, mpc_i2c_isr,
-				     IRQF_SHARED, "i2c-mpc", i2c);
-		if (result < 0) {
-			dev_err(i2c->dev, "failed to attach interrupt\n");
-			goto fail_request;
-		}
+	if (i2c->irq < 0) {
+		result = i2c->irq;
+		goto fail_map;
+	}
+
+	result = request_irq(i2c->irq, mpc_i2c_isr,
+			IRQF_SHARED, "i2c-mpc", i2c);
+	if (result < 0) {
+		dev_err(i2c->dev, "failed to attach interrupt\n");
+		goto fail_request;
 	}
 
 	/*
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 5/6] i2c: mpc: use device managed APIs
  2021-03-23  4:33 [PATCH 0/6] i2c: mpc: Refactor to improve responsiveness Chris Packham
                   ` (3 preceding siblings ...)
  2021-03-23  4:33 ` [PATCH 4/6] i2c: mpc: make interrupt mandatory and remove polling code Chris Packham
@ 2021-03-23  4:33 ` Chris Packham
  2021-03-23  4:33 ` [PATCH 6/6] i2c: mpc: Interrupt driven transfer Chris Packham
  2021-03-24  3:43 ` [PATCH 0/6] i2c: mpc: Refactor to improve responsiveness Chris Packham
  6 siblings, 0 replies; 15+ messages in thread
From: Chris Packham @ 2021-03-23  4:33 UTC (permalink / raw)
  To: robh+dt, linux, wsa, jdelvare
  Cc: linux-i2c, devicetree, linux-kernel, Chris Packham

Use device managed functions an clean up error handling.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 drivers/i2c/busses/i2c-mpc.c | 46 ++++++++++++++----------------------
 1 file changed, 18 insertions(+), 28 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index 5b746a898e8e..46cdb36e2f9b 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -654,7 +654,6 @@ static int fsl_i2c_probe(struct platform_device *op)
 	u32 clock = MPC_I2C_CLOCK_LEGACY;
 	int result = 0;
 	int plen;
-	struct resource res;
 	struct clk *clk;
 	int err;
 
@@ -662,7 +661,7 @@ static int fsl_i2c_probe(struct platform_device *op)
 	if (!match)
 		return -EINVAL;
 
-	i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
+	i2c = devm_kzalloc(&op->dev, sizeof(*i2c), GFP_KERNEL);
 	if (!i2c)
 		return -ENOMEM;
 
@@ -670,24 +669,21 @@ static int fsl_i2c_probe(struct platform_device *op)
 
 	init_waitqueue_head(&i2c->queue);
 
-	i2c->base = of_iomap(op->dev.of_node, 0);
-	if (!i2c->base) {
+	i2c->base = devm_platform_ioremap_resource(op, 0);
+	if (IS_ERR(i2c->base)) {
 		dev_err(i2c->dev, "failed to map controller\n");
-		result = -ENOMEM;
-		goto fail_map;
+		return PTR_ERR(i2c->base);
 	}
 
-	i2c->irq = irq_of_parse_and_map(op->dev.of_node, 0);
-	if (i2c->irq < 0) {
-		result = i2c->irq;
-		goto fail_map;
-	}
+	i2c->irq = platform_get_irq(op, 0);
+	if (i2c->irq < 0)
+		return i2c->irq;
 
-	result = request_irq(i2c->irq, mpc_i2c_isr,
+	result = devm_request_irq(&op->dev, i2c->irq, mpc_i2c_isr,
 			IRQF_SHARED, "i2c-mpc", i2c);
 	if (result < 0) {
 		dev_err(i2c->dev, "failed to attach interrupt\n");
-		goto fail_request;
+		return result;
 	}
 
 	/*
@@ -699,7 +695,7 @@ static int fsl_i2c_probe(struct platform_device *op)
 		err = clk_prepare_enable(clk);
 		if (err) {
 			dev_err(&op->dev, "failed to enable clock\n");
-			goto fail_request;
+			return err;
 		} else {
 			i2c->clk_per = clk;
 		}
@@ -731,32 +727,26 @@ static int fsl_i2c_probe(struct platform_device *op)
 	}
 	dev_info(i2c->dev, "timeout %u us\n", mpc_ops.timeout * 1000000 / HZ);
 
-	platform_set_drvdata(op, i2c);
-
 	i2c->adap = mpc_ops;
-	of_address_to_resource(op->dev.of_node, 0, &res);
 	scnprintf(i2c->adap.name, sizeof(i2c->adap.name),
-		  "MPC adapter at 0x%llx", (unsigned long long)res.start);
-	i2c_set_adapdata(&i2c->adap, i2c);
+		  "MPC adapter (%s)", of_node_full_name(op->dev.of_node));
 	i2c->adap.dev.parent = &op->dev;
+	i2c->adap.nr = op->id;
 	i2c->adap.dev.of_node = of_node_get(op->dev.of_node);
 	i2c->adap.bus_recovery_info = &fsl_i2c_recovery_info;
+	platform_set_drvdata(op, i2c);
+	i2c_set_adapdata(&i2c->adap, i2c);
 
-	result = i2c_add_adapter(&i2c->adap);
-	if (result < 0)
+	result = i2c_add_numbered_adapter(&i2c->adap);
+	if (result)
 		goto fail_add;
 
-	return result;
+	return 0;
 
  fail_add:
 	if (i2c->clk_per)
 		clk_disable_unprepare(i2c->clk_per);
-	free_irq(i2c->irq, i2c);
- fail_request:
-	irq_dispose_mapping(i2c->irq);
-	iounmap(i2c->base);
- fail_map:
-	kfree(i2c);
+
 	return result;
 };
 
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 6/6] i2c: mpc: Interrupt driven transfer
  2021-03-23  4:33 [PATCH 0/6] i2c: mpc: Refactor to improve responsiveness Chris Packham
                   ` (4 preceding siblings ...)
  2021-03-23  4:33 ` [PATCH 5/6] i2c: mpc: use device managed APIs Chris Packham
@ 2021-03-23  4:33 ` Chris Packham
  2021-03-24  3:43 ` [PATCH 0/6] i2c: mpc: Refactor to improve responsiveness Chris Packham
  6 siblings, 0 replies; 15+ messages in thread
From: Chris Packham @ 2021-03-23  4:33 UTC (permalink / raw)
  To: robh+dt, linux, wsa, jdelvare
  Cc: linux-i2c, devicetree, linux-kernel, Chris Packham

The fsl-i2c controller will generate an interrupt after every byte
transferred. Make use of this interrupt to drive a state machine which
allows the next part of a transfer to happen as soon as the interrupt is
received. This is particularly helpful with SMBUS devices like the LM81
which will timeout if we take too long between bytes in a transfer.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 drivers/i2c/busses/i2c-mpc.c | 430 +++++++++++++++++++----------------
 1 file changed, 237 insertions(+), 193 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index 46cdb36e2f9b..5ffde3428232 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -1,16 +1,11 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
- * (C) Copyright 2003-2004
- * Humboldt Solutions Ltd, adrian@humboldt.co.uk.
-
  * This is a combined i2c adapter and algorithm driver for the
  * MPC107/Tsi107 PowerPC northbridge and processors that include
  * the same I2C unit (8240, 8245, 85xx).
  *
- * Release 0.8
- *
- * This file is licensed under the terms of the GNU General Public
- * License version 2. This program is licensed "as is" without any
- * warranty of any kind, whether express or implied.
+ * Copyright (C) 2003-2004 Humboldt Solutions Ltd, adrian@humboldt.co.uk
+ * Copyright (C) 2021 Allied Telesis Labs
  */
 
 #include <linux/kernel.h>
@@ -58,11 +53,32 @@
 #define CSR_MIF  0x02
 #define CSR_RXAK 0x01
 
+enum mpc_i2c_action {
+	MPC_I2C_ACTION_INVALID = 0,
+	MPC_I2C_ACTION_START,
+	MPC_I2C_ACTION_RESTART,
+	MPC_I2C_ACTION_READ_BEGIN,
+	MPC_I2C_ACTION_READ_BYTE,
+	MPC_I2C_ACTION_WRITE,
+	MPC_I2C_ACTION_STOP,
+};
+
+static char *action_str[] = {
+	"invalid",
+	"start",
+	"restart",
+	"read begin",
+	"read",
+	"write",
+	"stop",
+};
+
 struct mpc_i2c {
 	struct device *dev;
 	void __iomem *base;
 	u32 interrupt;
-	wait_queue_head_t queue;
+	wait_queue_head_t waitq;
+	spinlock_t              lock;
 	struct i2c_adapter adap;
 	int irq;
 	u32 real_clk;
@@ -70,6 +86,16 @@ struct mpc_i2c {
 	u8 fdr, dfsrr;
 #endif
 	struct clk *clk_per;
+	u32 cntl_bits;
+	enum mpc_i2c_action action;
+	struct i2c_msg *msgs;
+	int num_msgs;
+	int curr_msg;
+	u32 byte_posn;
+	u32 block;
+	int rc;
+	int expect_rxack;
+
 };
 
 struct mpc_i2c_divider {
@@ -86,19 +112,6 @@ static inline void writeccr(struct mpc_i2c *i2c, u32 x)
 	writeb(x, i2c->base + MPC_I2C_CR);
 }
 
-static irqreturn_t mpc_i2c_isr(int irq, void *dev_id)
-{
-	struct mpc_i2c *i2c = dev_id;
-	if (readb(i2c->base + MPC_I2C_SR) & CSR_MIF) {
-		/* Read again to allow register to stabilise */
-		i2c->interrupt = readb(i2c->base + MPC_I2C_SR);
-		writeb(0, i2c->base + MPC_I2C_SR);
-		wake_up(&i2c->queue);
-		return IRQ_HANDLED;
-	}
-	return IRQ_NONE;
-}
-
 /* Sometimes 9th clock pulse isn't generated, and slave doesn't release
  * the bus, because it wants to send ACK.
  * Following sequence of enabling/disabling and sending start/stop generates
@@ -121,45 +134,6 @@ static void mpc_i2c_fixup(struct mpc_i2c *i2c)
 	}
 }
 
-static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing)
-{
-	u32 cmd_err;
-	int result;
-
-	result = wait_event_timeout(i2c->queue,
-			(i2c->interrupt & CSR_MIF), timeout);
-
-	if (unlikely(!(i2c->interrupt & CSR_MIF))) {
-		dev_dbg(i2c->dev, "wait timeout\n");
-		writeccr(i2c, 0);
-		result = -ETIMEDOUT;
-	}
-
-	cmd_err = i2c->interrupt;
-	i2c->interrupt = 0;
-
-	if (result < 0)
-		return result;
-
-	if (!(cmd_err & CSR_MCF)) {
-		dev_dbg(i2c->dev, "unfinished\n");
-		return -EIO;
-	}
-
-	if (cmd_err & CSR_MAL) {
-		dev_dbg(i2c->dev, "MAL\n");
-		return -EAGAIN;
-	}
-
-	if (writing && (cmd_err & CSR_RXAK)) {
-		dev_dbg(i2c->dev, "No RXAK\n");
-		/* generate stop */
-		writeccr(i2c, CCR_MEN);
-		return -ENXIO;
-	}
-	return 0;
-}
-
 #if defined(CONFIG_PPC_MPC52xx) || defined(CONFIG_PPC_MPC512x)
 static const struct mpc_i2c_divider mpc_i2c_dividers_52xx[] = {
 	{20, 0x20}, {22, 0x21}, {24, 0x22}, {26, 0x23},
@@ -434,168 +408,209 @@ static void mpc_i2c_setup_8xxx(struct device_node *node,
 }
 #endif /* CONFIG_FSL_SOC */
 
-static void mpc_i2c_start(struct mpc_i2c *i2c)
+static void mpc_i2c_finish(struct mpc_i2c *i2c, int rc)
 {
-	/* Clear arbitration */
-	writeb(0, i2c->base + MPC_I2C_SR);
-	/* Start with MEN */
-	writeccr(i2c, CCR_MEN);
+	i2c->rc = rc;
+	i2c->block = 0;
+	i2c->cntl_bits = CCR_MEN;
+	writeccr(i2c, i2c->cntl_bits);
+	wake_up(&i2c->waitq);
 }
 
-static void mpc_i2c_stop(struct mpc_i2c *i2c)
+static void mpc_i2c_do_action(struct mpc_i2c *i2c)
 {
-	writeccr(i2c, CCR_MEN);
-}
+	struct i2c_msg *msg = &i2c->msgs[i2c->curr_msg];
+	int dir = 0;
+	int recv_len = 0;
+	u8 byte;
+
+	dev_dbg(i2c->dev, "%s: action = %s\n", __func__,
+		action_str[i2c->action]);
+
+	i2c->cntl_bits &= ~(CCR_RSTA | CCR_MTX | CCR_TXAK);
+
+	if (msg->flags & I2C_M_RD)
+		dir = 1;
+	if (msg->flags & I2C_M_RECV_LEN)
+		recv_len = 1;
+
+	switch (i2c->action) {
+	case MPC_I2C_ACTION_RESTART:
+		i2c->cntl_bits |= CCR_RSTA;
+		fallthrough;
+
+	case MPC_I2C_ACTION_START:
+		i2c->cntl_bits |= CCR_MSTA | CCR_MTX;
+		writeccr(i2c, i2c->cntl_bits);
+		writeb((msg->addr << 1) | dir, i2c->base + MPC_I2C_DR);
+		i2c->expect_rxack = 1;
+		i2c->action = dir ? MPC_I2C_ACTION_READ_BEGIN : MPC_I2C_ACTION_WRITE;
+		break;
+
+	case MPC_I2C_ACTION_READ_BEGIN:
+		if (msg->len) {
+			if (msg->len == 1 && !(msg->flags & I2C_M_RECV_LEN))
+				i2c->cntl_bits |= CCR_TXAK;
+
+			writeccr(i2c, i2c->cntl_bits);
+			/* Dummy read */
+			readb(i2c->base + MPC_I2C_DR);
+		}
+		i2c->action = MPC_I2C_ACTION_READ_BYTE;
+		break;
 
-static int mpc_write(struct mpc_i2c *i2c, int target,
-		     const u8 *data, int length, int restart)
-{
-	int i, result;
-	unsigned timeout = i2c->adap.timeout;
-	u32 flags = restart ? CCR_RSTA : 0;
+	case MPC_I2C_ACTION_READ_BYTE:
+		if (i2c->byte_posn || !recv_len) {
+			/* Generate txack on next to last byte */
+			if (i2c->byte_posn == msg->len - 2)
+				i2c->cntl_bits |= CCR_TXAK;
+			/* Do not generate stop on last byte */
+			if (i2c->byte_posn == msg->len - 1)
+				i2c->cntl_bits |= CCR_MTX;
 
-	/* Start as master */
-	writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA | CCR_MTX | flags);
-	/* Write target byte */
-	writeb((target << 1), i2c->base + MPC_I2C_DR);
+			writeccr(i2c, i2c->cntl_bits);
+		}
 
-	result = i2c_wait(i2c, timeout, 1);
-	if (result < 0)
-		return result;
+		byte = readb(i2c->base + MPC_I2C_DR);
 
-	for (i = 0; i < length; i++) {
-		/* Write data byte */
-		writeb(data[i], i2c->base + MPC_I2C_DR);
+		if (i2c->byte_posn == 0 && recv_len) {
+			if (byte == 0 || byte > I2C_SMBUS_BLOCK_MAX) {
+				mpc_i2c_finish(i2c, -EPROTO);
+				return;
+			}
+			msg->len += byte;
+			/*
+			 * For block reads, generate txack here if data length
+			 * is 1 byte (total length is 2 bytes).
+			 */
+			if (msg->len == 2) {
+				i2c->cntl_bits |= CCR_TXAK;
+				writeccr(i2c, i2c->cntl_bits);
+			}
+		}
 
-		result = i2c_wait(i2c, timeout, 1);
-		if (result < 0)
-			return result;
+		dev_dbg(i2c->dev, "%s: %s %02x\n", __func__,
+			action_str[i2c->action], byte);
+		msg->buf[i2c->byte_posn++] = byte;
+		break;
+
+	case MPC_I2C_ACTION_WRITE:
+		dev_dbg(i2c->dev, "%s: %s %02x\n", __func__,
+			action_str[i2c->action], msg->buf[i2c->byte_posn]);
+		writeb(msg->buf[i2c->byte_posn++], i2c->base + MPC_I2C_DR);
+		i2c->expect_rxack = 1;
+		break;
+
+	case MPC_I2C_ACTION_STOP:
+		mpc_i2c_finish(i2c, 0);
+		break;
+
+	case MPC_I2C_ACTION_INVALID:
+	default:
+		BUG();
+		break;
 	}
 
-	return 0;
+	if (msg->len == i2c->byte_posn) {
+		i2c->curr_msg++;
+		i2c->byte_posn = 0;
+
+		if (i2c->curr_msg == i2c->num_msgs) {
+			i2c->action = MPC_I2C_ACTION_STOP;
+			/*
+			 * We don't get another interrupt on read so
+			 * finish the transfer now
+			 */
+			if (dir)
+				mpc_i2c_finish(i2c, 0);
+		} else {
+			i2c->action = MPC_I2C_ACTION_RESTART;
+		}
+	}
 }
 
-static int mpc_read(struct mpc_i2c *i2c, int target,
-		    u8 *data, int length, int restart, bool recv_len)
+static void mpc_i2c_do_intr(struct mpc_i2c *i2c, u8 status)
 {
-	unsigned timeout = i2c->adap.timeout;
-	int i, result;
-	u32 flags = restart ? CCR_RSTA : 0;
+	unsigned long flags;
 
-	/* Switch to read - restart */
-	writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA | CCR_MTX | flags);
-	/* Write target address byte - this time with the read flag set */
-	writeb((target << 1) | 1, i2c->base + MPC_I2C_DR);
+	spin_lock_irqsave(&i2c->lock, flags);
 
-	result = i2c_wait(i2c, timeout, 1);
-	if (result < 0)
-		return result;
+	if (!(status & CSR_MCF)) {
+		dev_dbg(i2c->dev, "unfinished\n");
+		mpc_i2c_finish(i2c, -EIO);
+		goto out;
+	}
 
-	if (length) {
-		if (length == 1 && !recv_len)
-			writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA | CCR_TXAK);
-		else
-			writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA);
-		/* Dummy read */
-		readb(i2c->base + MPC_I2C_DR);
+	if (status & CSR_MAL) {
+		dev_dbg(i2c->dev, "arbiritration lost\n");
+		mpc_i2c_finish(i2c, -EAGAIN);
+		goto out;
 	}
 
-	for (i = 0; i < length; i++) {
-		u8 byte;
+	if (i2c->expect_rxack && (status & CSR_RXAK)) {
+		dev_dbg(i2c->dev, "no RXAK\n");
+		mpc_i2c_finish(i2c, -ENXIO);
+		goto out;
+	}
+	i2c->expect_rxack = 0;
 
-		result = i2c_wait(i2c, timeout, 0);
-		if (result < 0)
-			return result;
+	mpc_i2c_do_action(i2c);
 
-		/*
-		 * For block reads, we have to know the total length (1st byte)
-		 * before we can determine if we are done.
-		 */
-		if (i || !recv_len) {
-			/* Generate txack on next to last byte */
-			if (i == length - 2)
-				writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA
-					 | CCR_TXAK);
-			/* Do not generate stop on last byte */
-			if (i == length - 1)
-				writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA
-					 | CCR_MTX);
-		}
+out:
+	spin_unlock_irqrestore(&i2c->lock, flags);
+}
 
-		byte = readb(i2c->base + MPC_I2C_DR);
+static irqreturn_t mpc_i2c_isr(int irq, void *dev_id)
+{
+	struct mpc_i2c *i2c = dev_id;
+	u8 status = readb(i2c->base + MPC_I2C_SR);
 
-		/*
-		 * Adjust length if first received byte is length.
-		 * The length is 1 length byte plus actually data length
-		 */
-		if (i == 0 && recv_len) {
-			if (byte == 0 || byte > I2C_SMBUS_BLOCK_MAX)
-				return -EPROTO;
-			length += byte;
-			/*
-			 * For block reads, generate txack here if data length
-			 * is 1 byte (total length is 2 bytes).
-			 */
-			if (length == 2)
-				writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA
-					 | CCR_TXAK);
-		}
-		data[i] = byte;
+	if (status & CSR_MIF) {
+		writeb(0, i2c->base + MPC_I2C_SR);
+		mpc_i2c_do_intr(i2c, status);
+		return IRQ_HANDLED;
 	}
+	return IRQ_NONE;
+}
+
+static void mpc_i2c_wait_for_completion(struct mpc_i2c *i2c)
+{
+	long time_left;
 
-	return length;
+	time_left = wait_event_timeout(i2c->waitq, !i2c->block, i2c->adap.timeout);
+
+	if (!time_left)
+		i2c->rc = -ETIMEDOUT;
+	else if (time_left < 0)
+		i2c->rc = time_left;
 }
 
-static int mpc_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
+static int mpc_i2c_execute_msg(struct mpc_i2c *i2c)
 {
-	struct i2c_msg *pmsg;
-	int i;
-	int ret = 0;
-	unsigned long orig_jiffies = jiffies;
-	struct mpc_i2c *i2c = i2c_get_adapdata(adap);
+	unsigned long orig_jiffies;
+	unsigned long flags;
 
-	mpc_i2c_start(i2c);
+	spin_lock_irqsave(&i2c->lock, flags);
 
-	/* Allow bus up to 1s to become not busy */
-	while (readb(i2c->base + MPC_I2C_SR) & CSR_MBB) {
-		if (signal_pending(current)) {
-			dev_dbg(i2c->dev, "Interrupted\n");
-			writeccr(i2c, 0);
-			return -EINTR;
-		}
-		if (time_after(jiffies, orig_jiffies + HZ)) {
-			u8 status = readb(i2c->base + MPC_I2C_SR);
+	i2c->curr_msg = 0;
+	i2c->rc = 0;
+	i2c->byte_posn = 0;
+	i2c->block = 1;
+	i2c->action = MPC_I2C_ACTION_START;
 
-			dev_dbg(i2c->dev, "timeout\n");
-			if ((status & (CSR_MCF | CSR_MBB | CSR_RXAK)) != 0) {
-				writeb(status & ~CSR_MAL,
-				       i2c->base + MPC_I2C_SR);
-				i2c_recover_bus(&i2c->adap);
-			}
-			return -EIO;
-		}
-		schedule();
-	}
+	i2c->cntl_bits = CCR_MEN | CCR_MIEN;
+	writeb(0, i2c->base + MPC_I2C_SR);
+	writeccr(i2c, i2c->cntl_bits);
+
+	mpc_i2c_do_action(i2c);
+
+	spin_unlock_irqrestore(&i2c->lock, flags);
+
+	mpc_i2c_wait_for_completion(i2c);
+
+	if (i2c->rc == -EIO || i2c->rc == -EAGAIN || i2c->rc == -ETIMEDOUT)
+		i2c_recover_bus(&i2c->adap);
 
-	for (i = 0; ret >= 0 && i < num; i++) {
-		pmsg = &msgs[i];
-		dev_dbg(i2c->dev,
-			"Doing %s %d bytes to 0x%02x - %d of %d messages\n",
-			pmsg->flags & I2C_M_RD ? "read" : "write",
-			pmsg->len, pmsg->addr, i + 1, num);
-		if (pmsg->flags & I2C_M_RD) {
-			bool recv_len = pmsg->flags & I2C_M_RECV_LEN;
-
-			ret = mpc_read(i2c, pmsg->addr, pmsg->buf, pmsg->len, i,
-				       recv_len);
-			if (recv_len && ret > 0)
-				pmsg->len = ret;
-		} else {
-			ret =
-			    mpc_write(i2c, pmsg->addr, pmsg->buf, pmsg->len, i);
-		}
-	}
-	mpc_i2c_stop(i2c); /* Initiate STOP */
 	orig_jiffies = jiffies;
 	/* Wait until STOP is seen, allow up to 1 s */
 	while (readb(i2c->base + MPC_I2C_SR) & CSR_MBB) {
@@ -612,7 +627,35 @@ static int mpc_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 		}
 		cond_resched();
 	}
-	return (ret < 0) ? ret : num;
+
+	return i2c->rc;
+}
+
+static int mpc_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
+{
+	int rc, ret = num;
+	struct mpc_i2c *i2c = i2c_get_adapdata(adap);
+	int i;
+
+	dev_dbg(i2c->dev, "%s: num = %d\n", __func__, num);
+	for (i = 0; i < num; i++)
+		dev_dbg(i2c->dev, "  addr = %02x, flags = %02x, len = %d, %*ph\n",
+			msgs[i].addr, msgs[i].flags, msgs[i].len,
+			msgs[i].flags & I2C_M_RD ? 0 : msgs[i].len,
+			msgs[i].buf);
+
+	BUG_ON(i2c->msgs != NULL);
+	i2c->msgs = msgs;
+	i2c->num_msgs = num;
+
+	rc = mpc_i2c_execute_msg(i2c);
+	if (rc < 0)
+		ret = rc;
+
+	i2c->num_msgs = 0;
+	i2c->msgs = NULL;
+
+	return ret;
 }
 
 static u32 mpc_functionality(struct i2c_adapter *adap)
@@ -667,7 +710,8 @@ static int fsl_i2c_probe(struct platform_device *op)
 
 	i2c->dev = &op->dev; /* for debug and error output */
 
-	init_waitqueue_head(&i2c->queue);
+	init_waitqueue_head(&i2c->waitq);
+	spin_lock_init(&i2c->lock);
 
 	i2c->base = devm_platform_ioremap_resource(op, 0);
 	if (IS_ERR(i2c->base)) {
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/6] dt-bindings: i2c: convert i2c-mpc to json-schema
  2021-03-23  4:33 ` [PATCH 2/6] dt-bindings: i2c: convert i2c-mpc to json-schema Chris Packham
@ 2021-03-23 20:16   ` Rob Herring
  2021-03-23 20:22     ` Chris Packham
  2021-03-23 21:15   ` Rob Herring
  1 sibling, 1 reply; 15+ messages in thread
From: Rob Herring @ 2021-03-23 20:16 UTC (permalink / raw)
  To: Chris Packham
  Cc: linux-i2c, robh+dt, linux, wsa, jdelvare, linux-kernel, devicetree

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.


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/6] dt-bindings: i2c: convert i2c-mpc to json-schema
  2021-03-23 20:16   ` Rob Herring
@ 2021-03-23 20:22     ` Chris Packham
  2021-03-23 21:10       ` Rob Herring
  0 siblings, 1 reply; 15+ messages in thread
From: Chris Packham @ 2021-03-23 20:22 UTC (permalink / raw)
  To: Rob Herring
  Cc: linux-i2c, robh+dt, linux, wsa, jdelvare, linux-kernel, devicetree

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?).
> 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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/6] dt-bindings: i2c: convert i2c-mpc to json-schema
  2021-03-23 20:22     ` Chris Packham
@ 2021-03-23 21:10       ` Rob Herring
  0 siblings, 0 replies; 15+ messages in thread
From: Rob Herring @ 2021-03-23 21:10 UTC (permalink / raw)
  To: Chris Packham; +Cc: linux-i2c, linux, wsa, jdelvare, linux-kernel, devicetree

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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/6] dt-bindings: i2c: convert i2c-mpc to json-schema
  2021-03-23  4:33 ` [PATCH 2/6] dt-bindings: i2c: convert i2c-mpc to json-schema Chris Packham
  2021-03-23 20:16   ` Rob Herring
@ 2021-03-23 21:15   ` Rob Herring
  2021-03-23 21:59     ` Chris Packham
  1 sibling, 1 reply; 15+ messages in thread
From: Rob Herring @ 2021-03-23 21:15 UTC (permalink / raw)
  To: Chris Packham; +Cc: linux, wsa, jdelvare, linux-i2c, devicetree, linux-kernel

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?

> +
> +  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. 

> +        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
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/6] dt-bindings: i2c: convert i2c-mpc to json-schema
  2021-03-23 21:15   ` Rob Herring
@ 2021-03-23 21:59     ` Chris Packham
  2021-03-24  3:36       ` Chris Packham
  0 siblings, 1 reply; 15+ messages in thread
From: Chris Packham @ 2021-03-23 21:59 UTC (permalink / raw)
  To: Rob Herring; +Cc: linux, wsa, jdelvare, linux-i2c, devicetree, linux-kernel


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
>>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/6] dt-bindings: i2c: convert i2c-mpc to json-schema
  2021-03-23 21:59     ` Chris Packham
@ 2021-03-24  3:36       ` Chris Packham
  2021-03-27 17:18         ` Rob Herring
  0 siblings, 1 reply; 15+ messages in thread
From: Chris Packham @ 2021-03-24  3:36 UTC (permalink / raw)
  To: Rob Herring; +Cc: linux, wsa, jdelvare, linux-i2c, devicetree, linux-kernel


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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/6] i2c: mpc: Refactor to improve responsiveness
  2021-03-23  4:33 [PATCH 0/6] i2c: mpc: Refactor to improve responsiveness Chris Packham
                   ` (5 preceding siblings ...)
  2021-03-23  4:33 ` [PATCH 6/6] i2c: mpc: Interrupt driven transfer Chris Packham
@ 2021-03-24  3:43 ` Chris Packham
  6 siblings, 0 replies; 15+ messages in thread
From: Chris Packham @ 2021-03-24  3:43 UTC (permalink / raw)
  To: robh+dt, linux, wsa, jdelvare; +Cc: linux-i2c, devicetree, linux-kernel


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
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/6] dt-bindings: i2c: convert i2c-mpc to json-schema
  2021-03-24  3:36       ` Chris Packham
@ 2021-03-27 17:18         ` Rob Herring
  0 siblings, 0 replies; 15+ messages in thread
From: Rob Herring @ 2021-03-27 17:18 UTC (permalink / raw)
  To: Chris Packham; +Cc: linux, wsa, jdelvare, linux-i2c, devicetree, linux-kernel

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2021-03-27 17:19 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-23  4:33 [PATCH 0/6] i2c: mpc: Refactor to improve responsiveness Chris Packham
2021-03-23  4:33 ` [PATCH 1/6] dt-bindings: i2c-mpc: Document interrupt property as required Chris Packham
2021-03-23  4:33 ` [PATCH 2/6] dt-bindings: i2c: convert i2c-mpc to json-schema Chris Packham
2021-03-23 20:16   ` Rob Herring
2021-03-23 20:22     ` Chris Packham
2021-03-23 21:10       ` Rob Herring
2021-03-23 21:15   ` Rob Herring
2021-03-23 21:59     ` Chris Packham
2021-03-24  3:36       ` Chris Packham
2021-03-27 17:18         ` Rob Herring
2021-03-23  4:33 ` [PATCH 3/6] i2c: mpc: Make use of i2c_recover_bus() Chris Packham
2021-03-23  4:33 ` [PATCH 4/6] i2c: mpc: make interrupt mandatory and remove polling code Chris Packham
2021-03-23  4:33 ` [PATCH 5/6] i2c: mpc: use device managed APIs Chris Packham
2021-03-23  4:33 ` [PATCH 6/6] i2c: mpc: Interrupt driven transfer Chris Packham
2021-03-24  3:43 ` [PATCH 0/6] i2c: mpc: Refactor to improve responsiveness Chris Packham

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.