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

[PATCH v3 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 change has been sent as github pull reqest[2].

Thanks Krzysztof and Chris for the reviews.

Thank you,
Andi

[1] https://lore.kernel.org/all/c41ee6b5-ddb4-1253-de54-a295b3bab2cc@linaro.org/
[2] https://github.com/devicetree-org/dt-schema/pull/102

Changelog
=========
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.
 - Added Chris r-b in patch 3.

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-ms 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 v3 1/3] dt-bindings: i2c: mpc: Mark "fsl,timeout" as deprecated

[Andi Shyti]

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

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

Signed-off-by: Andi Shyti <andi.shyti@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 018e1b944424..bd2dd82471c1 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-ms = <10000>;
     };
 ...
-- 
2.39.2

[PATCH v3 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>
---
 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 81ac92bb4f6f..87e5c1725750 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

[PATCH v3 3/3] i2c: mpc: Use i2c-scl-clk-low-timeout-ms i2c property

[Andi Shyti]

"fsl,timeout" is marked as deprecated and replaced by the
"i2c-scl-clk-low-timeout-ms" 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>
---
 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 87e5c1725750..28f11e30ac50 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-ms" is not present.
+	 */
 	result = of_property_read_u32(op->dev.of_node,
-				      "fsl,timeout", &mpc_ops.timeout);
+				      "i2c-scl-clk-low-timeout-ms",
+				      &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 v3 2/3] i2c: mpc: Use of_property_read_u32 instead of of_get_property

[Chris Packham]

On 13/03/23 12:36, 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>

Reviewed-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

Gave the patches a spin on a P2041RDB so

Tested-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 81ac92bb4f6f..87e5c1725750 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"))

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

[Chris Packham]

On 13/03/23 12:36, Andi Shyti wrote:
> "fsl,timeout" is marked as deprecated and replaced by the
> "i2c-scl-clk-low-timeout-ms" 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>
Gave the patches a spin on a P2041RDB so

Tested-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

> ---
>   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 87e5c1725750..28f11e30ac50 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-ms" is not present.
> +	 */
>   	result = of_property_read_u32(op->dev.of_node,
> -				      "fsl,timeout", &mpc_ops.timeout);
> +				      "i2c-scl-clk-low-timeout-ms",
> +				      &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)

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

[Andi Shyti]

> On 13/03/23 12:36, Andi Shyti wrote:
> > "fsl,timeout" is marked as deprecated and replaced by the
> > "i2c-scl-clk-low-timeout-ms" 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>
> Gave the patches a spin on a P2041RDB so
> 
> Tested-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

Thanks, Chris!

Andi

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

[Rob Herring]

On Mon, 13 Mar 2023 00:36:11 +0100, Andi Shyti wrote:
> Now we have the i2c-scl-clk-low-timeout-ms property defined in
> the i2c schema.
> 
> Mark "fsl,timeout" as deprecated and update the example.
> 
> Signed-off-by: Andi Shyti <andi.shyti@kernel.org>
> ---
>  Documentation/devicetree/bindings/i2c/i2c-mpc.yaml | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i2c/i2c-mpc.example.dtb: i2c@3100: Unevaluated properties are not allowed ('i2c-scl-clk-low-timeout-ms' was unexpected)
	From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i2c/i2c-mpc.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230312233613.303408-2-andi.shyti@kernel.org

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

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 after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

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

[Krzysztof Kozlowski]

On 14/03/2023 15:10, Rob Herring wrote:
> 
> On Mon, 13 Mar 2023 00:36:11 +0100, Andi Shyti wrote:
>> Now we have the i2c-scl-clk-low-timeout-ms property defined in
>> the i2c schema.
>>
>> Mark "fsl,timeout" as deprecated and update the example.
>>
>> Signed-off-by: Andi Shyti <andi.shyti@kernel.org>
>> ---
>>  Documentation/devicetree/bindings/i2c/i2c-mpc.yaml | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
> 
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i2c/i2c-mpc.example.dtb: i2c@3100: Unevaluated properties are not allowed ('i2c-scl-clk-low-timeout-ms' was unexpected)

That's expected as it depends on:
https://github.com/devicetree-org/dt-schema/pull/102

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 13/03/2023 00:36, Andi Shyti wrote:
> Now we have the i2c-scl-clk-low-timeout-ms property defined in
> the i2c schema.
> 
> Mark "fsl,timeout" as deprecated and update the example.
> 
> Signed-off-by: Andi Shyti <andi.shyti@kernel.org>
> ---
>  Documentation/devicetree/bindings/i2c/i2c-mpc.yaml | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 14/03/2023 15:21, Krzysztof Kozlowski wrote:
> On 13/03/2023 00:36, Andi Shyti wrote:
>> Now we have the i2c-scl-clk-low-timeout-ms property defined in
>> the i2c schema.
>>
>> Mark "fsl,timeout" as deprecated and update the example.
>>
>> Signed-off-by: Andi Shyti <andi.shyti@kernel.org>
>> ---
>>  Documentation/devicetree/bindings/i2c/i2c-mpc.yaml | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
> 
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

...

and unreviewed. Are you sure these are using same units? Old code used us.

Your example:
	i2c-scl-clk-low-timeout-ms = <10000>;

10s timeout?

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 13/03/2023 00:36, Andi Shyti wrote:
> "fsl,timeout" is marked as deprecated and replaced by the
> "i2c-scl-clk-low-timeout-ms" 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>
> ---
>  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 87e5c1725750..28f11e30ac50 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-ms" is not present.
> +	 */
>  	result = of_property_read_u32(op->dev.of_node,
> -				      "fsl,timeout", &mpc_ops.timeout);
> +				      "i2c-scl-clk-low-timeout-ms",
> +				      &mpc_ops.timeout);
> +	if (result == -EINVAL)
> +		result = of_property_read_u32(op->dev.of_node,
> +					      "fsl,timeout", &mpc_ops.timeout);

Wasn't old property in us and new one is in ms?

Best regards,
Krzysztof

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

[Andi Shyti]

On Tue, Mar 14, 2023 at 03:22:19PM +0100, Krzysztof Kozlowski wrote:
> On 14/03/2023 15:21, Krzysztof Kozlowski wrote:
> > On 13/03/2023 00:36, Andi Shyti wrote:
> >> Now we have the i2c-scl-clk-low-timeout-ms property defined in
> >> the i2c schema.
> >>
> >> Mark "fsl,timeout" as deprecated and update the example.
> >>
> >> Signed-off-by: Andi Shyti <andi.shyti@kernel.org>
> >> ---
> >>  Documentation/devicetree/bindings/i2c/i2c-mpc.yaml | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> > 
> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
> ...
> 
> and unreviewed. Are you sure these are using same units? Old code used us.
> 
> Your example:
> 	i2c-scl-clk-low-timeout-ms = <10000>;
> 
> 10s timeout?

ops!

I will keep it ms, if anyone doesn't have anything against. The
clock stretching goes in order of milliseconds and in any case
doesn't need to be precisely that, as long as it's longer.

I don't see any need for it to be us... if it 12500us, let it be
13ms.

I will update the example.

Can I keep your r-b with:

	i2c-scl-clk-low-timeout-ms = <10>;

?

Thanks for noticing it, it would have been a wacky mistake,
Andi

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

[Andi Shyti]

Hi,

On Tue, Mar 14, 2023 at 03:22:52PM +0100, Krzysztof Kozlowski wrote:
> On 13/03/2023 00:36, Andi Shyti wrote:
> > "fsl,timeout" is marked as deprecated and replaced by the
> > "i2c-scl-clk-low-timeout-ms" 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>
> > ---
> >  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 87e5c1725750..28f11e30ac50 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-ms" is not present.
> > +	 */
> >  	result = of_property_read_u32(op->dev.of_node,
> > -				      "fsl,timeout", &mpc_ops.timeout);
> > +				      "i2c-scl-clk-low-timeout-ms",
> > +				      &mpc_ops.timeout);
> > +	if (result == -EINVAL)
> > +		result = of_property_read_u32(op->dev.of_node,
> > +					      "fsl,timeout", &mpc_ops.timeout);
> 
> Wasn't old property in us and new one is in ms?

Thanks, Krzysztof! Good catch!

Chris, you are the only user of this property, as of now. Is it
OK if we keep it ms? I will send a proper patch to do the
conversion.

To me it doesn't make much sense to have the timeout defined in
us as that's of the same order of the raising and falling time
of the clock. Any opinion?

Andi

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

[Chris Packham]

Hi Andi,

On 15/03/23 04:16, Andi Shyti wrote:
> Hi,
>
> On Tue, Mar 14, 2023 at 03:22:52PM +0100, Krzysztof Kozlowski wrote:
>> On 13/03/2023 00:36, Andi Shyti wrote:
>>> "fsl,timeout" is marked as deprecated and replaced by the
>>> "i2c-scl-clk-low-timeout-ms" 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>
>>> ---
>>>   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 87e5c1725750..28f11e30ac50 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-ms" is not present.
>>> +	 */
>>>   	result = of_property_read_u32(op->dev.of_node,
>>> -				      "fsl,timeout", &mpc_ops.timeout);
>>> +				      "i2c-scl-clk-low-timeout-ms",
>>> +				      &mpc_ops.timeout);
>>> +	if (result == -EINVAL)
>>> +		result = of_property_read_u32(op->dev.of_node,
>>> +					      "fsl,timeout", &mpc_ops.timeout);
>> Wasn't old property in us and new one is in ms?
> Thanks, Krzysztof! Good catch!
>
> Chris, you are the only user of this property, as of now. Is it
> OK if we keep it ms? I will send a proper patch to do the
> conversion.
>
> To me it doesn't make much sense to have the timeout defined in
> us as that's of the same order of the raising and falling time
> of the clock. Any opinion?
I think it'd be easier to stick to us as then the same code can be used 
to probe both the old property and the new one. However I won't object 
if you adjust for the us to ms conversion between handling the new 
property vs the old one.
>
> Andi

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

[Andi Shyti]

Hi Chris,

> >>> "fsl,timeout" is marked as deprecated and replaced by the
> >>> "i2c-scl-clk-low-timeout-ms" 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>
> >>> ---
> >>>   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 87e5c1725750..28f11e30ac50 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-ms" is not present.
> >>> +	 */
> >>>   	result = of_property_read_u32(op->dev.of_node,
> >>> -				      "fsl,timeout", &mpc_ops.timeout);
> >>> +				      "i2c-scl-clk-low-timeout-ms",
> >>> +				      &mpc_ops.timeout);
> >>> +	if (result == -EINVAL)
> >>> +		result = of_property_read_u32(op->dev.of_node,
> >>> +					      "fsl,timeout", &mpc_ops.timeout);
> >> Wasn't old property in us and new one is in ms?
> > Thanks, Krzysztof! Good catch!
> >
> > Chris, you are the only user of this property, as of now. Is it
> > OK if we keep it ms? I will send a proper patch to do the
> > conversion.
> >
> > To me it doesn't make much sense to have the timeout defined in
> > us as that's of the same order of the raising and falling time
> > of the clock. Any opinion?

> I think it'd be easier to stick to us as then the same code can be used 
> to probe both the old property and the new one. However I won't object 
> if you adjust for the us to ms conversion between handling the new 
> property vs the old one.

yeah... indeed it became quite ugly.

OK, I will just rename it from "i2c-scl-clk-low-timeout-ms" to
"i2c-scl-clk-low-timeout-us", respin the schema pull request and
leave everything as it is.

Thanks for your input, Chris!

Andi