LinuxLists.cc - [PATCH v5 0/3] Add the clock stretching i2c property

2023-03-17 23:34:03

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

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/[email protected]/

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

2023-03-17 23:34:06

by Andi Shyti

[permalink] [raw]

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

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 <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
---
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

2023-03-17 23:34:13

by Andi Shyti

[permalink] [raw]

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

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

Suggested-by: Chris Packham <[email protected]>
Signed-off-by: Andi Shyti <[email protected]>
Tested-by: Chris Packham <[email protected]>
Reviewed-by: Chris Packham <[email protected]>
---
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

2023-03-17 23:34:37

by 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