[PATCH v5 0/3] Add the clock stretching i2c property

[PATCH v5 0/3] Add the clock stretching i2c property

[Andi Shyti]

Hello,

fter a discussion between Krzysztof and Ryan[1], it has become
apparent that the i2c binding is lacking the definition of a
property that needs to be added at a more generic level. This
property is also used by the mpc i2c controller, which has been
updated in the second patch.

The DTS schema has been applied in commit c83dd2cb836e ("schemas:
i2c: Add the clock stretching property").

Thanks Krzysztof, Chris and Rob for the reviews.

Thank you,
Andi

[1] https://lore.kernel.org/all/c41ee6b5-ddb4-1253-de54-a295b3bab2cc@linaro.org/

Changelog
=========
v4 -> v5:
 - Add the clock stretching i2c property
 - Added r-b from Rob Herring in Patch 1 and from Krzysztof in
   Patch 3.

v3 -> v4:
 - Replaced "i2c-scl-clk-low-timeout-ms" with
   "i2c-scl-clk-low-timeout-us". Use microseconds instead of
   milliseconds.
 - Add tags from Chris.

v2 -> v3:
 - Chris recommended to use of_property_read_u32() instead of
   of_get_property(). Because there were two use of it I added
   the suggested cleanup in a separate patch.

v1 -> v2:
 - Removed the binding patch and send through a different channel
 - To ensure back compatibility, which was broken in v1, the
   legacy "fsl,timeout" has not been removed and marked as
   deprecated. In the driver the that property is checked anyway
   as a fallback in case the main i2c-scl-clk-low-timeout-ms is
   missing.

Andi Shyti (3):
  dt-bindings: i2c: mpc: Mark "fsl,timeout" as deprecated
  i2c: mpc: Use of_property_read_u32 instead of of_get_property
  i2c: mpc: Use i2c-scl-clk-low-timeout-us i2c property

 .../devicetree/bindings/i2c/i2c-mpc.yaml      |  3 +-
 drivers/i2c/busses/i2c-mpc.c                  | 35 ++++++++++++-------
 2 files changed, 25 insertions(+), 13 deletions(-)

-- 
2.39.2

[PATCH v5 1/3] dt-bindings: i2c: mpc: Mark "fsl,timeout" as deprecated

[Andi Shyti]

Now we have the i2c-scl-clk-low-timeout-us property defined in
the i2c schema.

Mark "fsl,timeout" as deprecated and update the example.

Signed-off-by: Andi Shyti <andi.shyti@kernel.org>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/i2c/i2c-mpc.yaml | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mpc.yaml b/Documentation/devicetree/bindings/i2c/i2c-mpc.yaml
index 018e1b9444248..70fb69b923c46 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mpc.yaml
+++ b/Documentation/devicetree/bindings/i2c/i2c-mpc.yaml
@@ -43,6 +43,7 @@ properties:
 
   fsl,timeout:
     $ref: /schemas/types.yaml#/definitions/uint32
+    deprecated: true
     description: |
       I2C bus timeout in microseconds
 
@@ -95,6 +96,6 @@ examples:
         interrupts = <43 2>;
         interrupt-parent = <&mpic>;
         clock-frequency = <400000>;
-        fsl,timeout = <10000>;
+        i2c-scl-clk-low-timeout-us = <10000>;
     };
 ...
-- 
2.39.2

Re: [PATCH v5 1/3] dt-bindings: i2c: mpc: Mark "fsl,timeout" as deprecated

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 347 bytes --]

On Sat, Mar 18, 2023 at 12:33:36AM +0100, Andi Shyti wrote:
> Now we have the i2c-scl-clk-low-timeout-us property defined in
> the i2c schema.
> 
> Mark "fsl,timeout" as deprecated and update the example.
> 
> Signed-off-by: Andi Shyti <andi.shyti@kernel.org>
> Reviewed-by: Rob Herring <robh@kernel.org>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v5 2/3] i2c: mpc: Use of_property_read_u32 instead of of_get_property

[Andi Shyti]

"of_property_read_u32()" is preferred to "of_get_property()" for
retreiving u32 from the device tree. Replace it.

Suggested-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Signed-off-by: Andi Shyti <andi.shyti@kernel.org>
Tested-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Reviewed-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 drivers/i2c/busses/i2c-mpc.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index bec0c5dc20d16..02baba2284e27 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -770,7 +770,6 @@ static const struct i2c_algorithm mpc_algo = {
 static struct i2c_adapter mpc_ops = {
 	.owner = THIS_MODULE,
 	.algo = &mpc_algo,
-	.timeout = HZ,
 };
 
 static struct i2c_bus_recovery_info fsl_i2c_recovery_info = {
@@ -781,11 +780,9 @@ static int fsl_i2c_probe(struct platform_device *op)
 {
 	const struct mpc_i2c_data *data;
 	struct mpc_i2c *i2c;
-	const u32 *prop;
-	u32 clock = MPC_I2C_CLOCK_LEGACY;
-	int result = 0;
-	int plen;
 	struct clk *clk;
+	int result;
+	u32 clock;
 	int err;
 
 	i2c = devm_kzalloc(&op->dev, sizeof(*i2c), GFP_KERNEL);
@@ -831,10 +828,10 @@ static int fsl_i2c_probe(struct platform_device *op)
 	if (of_property_read_bool(op->dev.of_node, "fsl,preserve-clocking")) {
 		clock = MPC_I2C_CLOCK_PRESERVE;
 	} else {
-		prop = of_get_property(op->dev.of_node, "clock-frequency",
-					&plen);
-		if (prop && plen == sizeof(u32))
-			clock = *prop;
+		result = of_property_read_u32(op->dev.of_node,
+					      "clock-frequency", &clock);
+		if (result)
+			clock = MPC_I2C_CLOCK_LEGACY;
 	}
 
 	data = device_get_match_data(&op->dev);
@@ -846,12 +843,16 @@ static int fsl_i2c_probe(struct platform_device *op)
 			mpc_i2c_setup_8xxx(op->dev.of_node, i2c, clock);
 	}
 
-	prop = of_get_property(op->dev.of_node, "fsl,timeout", &plen);
-	if (prop && plen == sizeof(u32)) {
-		mpc_ops.timeout = *prop * HZ / 1000000;
+	result = of_property_read_u32(op->dev.of_node,
+				      "fsl,timeout", &mpc_ops.timeout);
+	if (!result) {
+		mpc_ops.timeout *= HZ / 1000000;
 		if (mpc_ops.timeout < 5)
 			mpc_ops.timeout = 5;
+	} else {
+		mpc_ops.timeout = HZ;
 	}
+
 	dev_info(i2c->dev, "timeout %u us\n", mpc_ops.timeout * 1000000 / HZ);
 
 	if (of_property_read_bool(op->dev.of_node, "fsl,i2c-erratum-a004447"))
-- 
2.39.2

Re: [PATCH v5 2/3] i2c: mpc: Use of_property_read_u32 instead of of_get_property

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 576 bytes --]

On Sat, Mar 18, 2023 at 12:33:37AM +0100, Andi Shyti wrote:
> "of_property_read_u32()" is preferred to "of_get_property()" for
> retreiving u32 from the device tree. Replace it.
> 
> Suggested-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Signed-off-by: Andi Shyti <andi.shyti@kernel.org>
> Tested-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Reviewed-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

Fixed this checkpatch warning:

  WARNING: 'retreiving' may be misspelled - perhaps 'retrieving'?

and applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v5 3/3] i2c: mpc: Use i2c-scl-clk-low-timeout-us i2c property

[Andi Shyti]

"fsl,timeout" is marked as deprecated and replaced by the
"i2c-scl-clk-low-timeout-us" i2c property.

Use this latter and, in case it is missing, for back
compatibility, check whether we still have "fsl,timeout" defined.

Signed-off-by: Andi Shyti <andi.shyti@kernel.org>
Reviewed-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Tested-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/i2c/busses/i2c-mpc.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index 02baba2284e27..cfd074ee6d547 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -843,8 +843,18 @@ static int fsl_i2c_probe(struct platform_device *op)
 			mpc_i2c_setup_8xxx(op->dev.of_node, i2c, clock);
 	}
 
+	/*
+	 * "fsl,timeout" has been marked as deprecated and, to maintain
+	 * backward compatibility, we will only look for it if
+	 * "i2c-scl-clk-low-timeout-us" is not present.
+	 */
 	result = of_property_read_u32(op->dev.of_node,
-				      "fsl,timeout", &mpc_ops.timeout);
+				      "i2c-scl-clk-low-timeout-us",
+				      &mpc_ops.timeout);
+	if (result == -EINVAL)
+		result = of_property_read_u32(op->dev.of_node,
+					      "fsl,timeout", &mpc_ops.timeout);
+
 	if (!result) {
 		mpc_ops.timeout *= HZ / 1000000;
 		if (mpc_ops.timeout < 5)
-- 
2.39.2

Re: [PATCH v5 3/3] i2c: mpc: Use i2c-scl-clk-low-timeout-us i2c property

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 587 bytes --]

On Sat, Mar 18, 2023 at 12:33:38AM +0100, Andi Shyti wrote:
> "fsl,timeout" is marked as deprecated and replaced by the
> "i2c-scl-clk-low-timeout-us" i2c property.
> 
> Use this latter and, in case it is missing, for back
> compatibility, check whether we still have "fsl,timeout" defined.
> 
> Signed-off-by: Andi Shyti <andi.shyti@kernel.org>
> Reviewed-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Tested-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 0/3] Add the clock stretching i2c property

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 322 bytes --]


> The DTS schema has been applied in commit c83dd2cb836e ("schemas:
> i2c: Add the clock stretching property").

Everyone, A general remark: If bindings for the dtschema are added,
please include the linux-i2c mailing list. This binding is OK, but it
would be good to have the discussion also here for additional input.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 0/3] Add the clock stretching i2c property

[Andi Shyti]

Hi Wolfram,

On Wed, Mar 29, 2023 at 09:06:43PM +0200, Wolfram Sang wrote:
> 
> > The DTS schema has been applied in commit c83dd2cb836e ("schemas:
> > i2c: Add the clock stretching property").
> 
> Everyone, A general remark: If bindings for the dtschema are added,
> please include the linux-i2c mailing list. This binding is OK, but it
> would be good to have the discussion also here for additional input.

Thanks for letting me know. It's actually my mistake, as I didn't
check how to properly distribute the patch to all relevant
parties.

Regarding dtschema patches, they are typically submitted as
GitHub pull requests. How should I send it to the linux-i2c?
Should I also send it to the mailing list like any other patch?
(This might make the code review process somewhat confusing,
though).

Thanks,
Andi