Skip to contentSkip to conversation

i2c: npcm: Bug fixes timeout, spurious interrupts

PATCH Superseded Tyrone Ting 5 participants 31 messages Feb 20, 2022 – Mar 2, 2022

Subsystems (3)
Device tree bindings I2C subsystem host drivers Arm/nuvoton npcm architecture

v2 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Tyrone Ting

author
#0 lore raw

v2 cover i2c: npcm: Bug fixes timeout, spurious interrupts

From: Tyrone Ting <[email protected]>

This patchset includes the following fixes:

- Add dt-bindings description for NPCM845.
- Bug fix for timeout calculation.
- Better handling of spurious interrupts.
- Fix for event type in slave mode.
- Removal of own slave addresses [2:10].
- Support for next gen BMC (NPCM845).

The NPCM I2C driver is tested on NPCM750 and NPCM845 evaluation boards.

Addressed comments from:
- Jonathan Neuschäfer : https://lkml.org/lkml/2022/2/7/670
- Krzysztof Kozlowski : https://lkml.org/lkml/2022/2/7/760
- Rob Herring : https://lkml.org/lkml/2022/2/7/1166

                 https://lkml.org/lkml/2022/2/11/711
 - Krzysztof Kozlowski : https://lkml.org/lkml/2022/2/7/742
 - Jonathan Neuschäfer : https://lkml.org/lkml/2022/2/7/934
 - Jonathan Neuschäfer : https://lkml.org/lkml/2022/2/7/947
 - Jonathan Neuschäfer : https://lkml.org/lkml/2022/2/7/1057
 - Krzysztof Kozlowski : https://lkml.org/lkml/2022/2/7/1192
 - kernel test robot : https://lore.kernel.org/all/
                       [email protected]/

Changes since version 1:
- Add nuvoton,sys-mgr property in NPCM devicetree.
- Describe the commit message in imperative mood.
- Modify the description in i2c binding document to cover NPCM series.
- Add new property in i2c binding document.
- Create a new patch for client address calculation.
- Create a new patch for updating gcr property name.
- Create a new patch for removing unused clock node.
- Explain EOB in the commit description.
- Create a new patch for correcting NPCM register access width.
- Remove some comment since the corresponding logic no longer exists.
- Remove fixes tag while the patch adds an additional feature.
- Use devicetree data field to support NPCM845.

Tali Perry (7):
i2c: npcm: Fix client address calculation
i2c: npcm: Update gcr property name
i2c: npcm: Remove unused clock node
i2c: npcm: Fix timeout calculation
i2c: npcm: Add tx complete counter
i2c: npcm: Handle spurious interrupts
i2c: npcm: Remove own slave addresses 2:10

Tyrone Ting (4):
arm: dts: add new property for NPCM i2c module
dt-bindings: i2c: npcm: support NPCM845
i2c: npcm: Correct register access width
i2c: npcm: Support NPCM845

 .../bindings/i2c/nuvoton,npcm7xx-i2c.yaml     |  17 +-
 arch/arm/boot/dts/nuvoton-common-npcm7xx.dtsi |  16 ++
 drivers/i2c/busses/Kconfig                    |   8 +-
 drivers/i2c/busses/Makefile                   |   2 +-
 drivers/i2c/busses/i2c-npcm7xx.c              | 251 +++++++++++-------
 5 files changed, 193 insertions(+), 101 deletions(-)
Signature · 1 line
2.17.1
author
#1 lore raw

PATCH v2 1/11 arm: dts: add new property for NPCM i2c module

From: Tyrone Ting <[email protected]>

Add nuvoton,sys-mgr property for controlling NPCM gcr register.

Signed-off-by Tyrone Ting <[email protected]>
Signed-off-by Tali Perry <[email protected]>
---
 arch/arm/boot/dts/nuvoton-common-npcm7xx.dtsi | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
arch/arm/boot/dts/nuvoton-common-npcm7xx.dtsi +16 -0
index 3696980a3da1..0fee5fc67e02 100644
@@ -371,6 +371,7 @@
 				interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
 				pinctrl-names = "default";
 				pinctrl-0 = <&smb0_pins>;
+				nuvoton,sys-mgr = <&gcr>;
 				status = "disabled";
 			};
 

@@ -383,6 +384,7 @@
 				interrupts = <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>;
 				pinctrl-names = "default";
 				pinctrl-0 = <&smb1_pins>;
+				nuvoton,sys-mgr = <&gcr>;
 				status = "disabled";
 			};
 

@@ -395,6 +397,7 @@
 				interrupts = <GIC_SPI 66 IRQ_TYPE_LEVEL_HIGH>;
 				pinctrl-names = "default";
 				pinctrl-0 = <&smb2_pins>;
+				nuvoton,sys-mgr = <&gcr>;
 				status = "disabled";
 			};
 

@@ -407,6 +410,7 @@
 				interrupts = <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>;
 				pinctrl-names = "default";
 				pinctrl-0 = <&smb3_pins>;
+				nuvoton,sys-mgr = <&gcr>;
 				status = "disabled";
 			};
 

@@ -419,6 +423,7 @@
 				interrupts = <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>;
 				pinctrl-names = "default";
 				pinctrl-0 = <&smb4_pins>;
+				nuvoton,sys-mgr = <&gcr>;
 				status = "disabled";
 			};
 

@@ -431,6 +436,7 @@
 				interrupts = <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;
 				pinctrl-names = "default";
 				pinctrl-0 = <&smb5_pins>;
+				nuvoton,sys-mgr = <&gcr>;
 				status = "disabled";
 			};
 

@@ -443,6 +449,7 @@
 				interrupts = <GIC_SPI 70 IRQ_TYPE_LEVEL_HIGH>;
 				pinctrl-names = "default";
 				pinctrl-0 = <&smb6_pins>;
+				nuvoton,sys-mgr = <&gcr>;
 				status = "disabled";
 			};
 

@@ -455,6 +462,7 @@
 				interrupts = <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>;
 				pinctrl-names = "default";
 				pinctrl-0 = <&smb7_pins>;
+				nuvoton,sys-mgr = <&gcr>;
 				status = "disabled";
 			};
 

@@ -467,6 +475,7 @@
 				interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
 				pinctrl-names = "default";
 				pinctrl-0 = <&smb8_pins>;
+				nuvoton,sys-mgr = <&gcr>;
 				status = "disabled";
 			};
 

@@ -479,6 +488,7 @@
 				interrupts = <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>;
 				pinctrl-names = "default";
 				pinctrl-0 = <&smb9_pins>;
+				nuvoton,sys-mgr = <&gcr>;
 				status = "disabled";
 			};
 

@@ -491,6 +501,7 @@
 				interrupts = <GIC_SPI 74 IRQ_TYPE_LEVEL_HIGH>;
 				pinctrl-names = "default";
 				pinctrl-0 = <&smb10_pins>;
+				nuvoton,sys-mgr = <&gcr>;
 				status = "disabled";
 			};
 

@@ -503,6 +514,7 @@
 				interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>;
 				pinctrl-names = "default";
 				pinctrl-0 = <&smb11_pins>;
+				nuvoton,sys-mgr = <&gcr>;
 				status = "disabled";
 			};
 

@@ -515,6 +527,7 @@
 				interrupts = <GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH>;
 				pinctrl-names = "default";
 				pinctrl-0 = <&smb12_pins>;
+				nuvoton,sys-mgr = <&gcr>;
 				status = "disabled";
 			};
 

@@ -527,6 +540,7 @@
 				interrupts = <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>;
 				pinctrl-names = "default";
 				pinctrl-0 = <&smb13_pins>;
+				nuvoton,sys-mgr = <&gcr>;
 				status = "disabled";
 			};
 

@@ -539,6 +553,7 @@
 				interrupts = <GIC_SPI 78 IRQ_TYPE_LEVEL_HIGH>;
 				pinctrl-names = "default";
 				pinctrl-0 = <&smb14_pins>;
+				nuvoton,sys-mgr = <&gcr>;
 				status = "disabled";
 			};
 

@@ -551,6 +566,7 @@
 				interrupts = <GIC_SPI 79 IRQ_TYPE_LEVEL_HIGH>;
 				pinctrl-names = "default";
 				pinctrl-0 = <&smb15_pins>;
+				nuvoton,sys-mgr = <&gcr>;
 				status = "disabled";
 			};
 		};
Signature · 1 line
2.17.1
author
#2 lore raw

PATCH v2 2/11 dt-bindings: i2c: npcm: support NPCM845

In reply to Tyrone Ting

From: Tyrone Ting <[email protected]>

Add compatible and nuvoton,sys-mgr description for NPCM i2c module.

Signed-off-by Tyrone Ting <[email protected]>
Signed-off-by Tali Perry <[email protected]>
---
 .../bindings/i2c/nuvoton,npcm7xx-i2c.yaml       | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)
Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml +12 -5
index 128444942aec..809c51ac32fe 100644
@@ -7,17 +7,18 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
 title: nuvoton NPCM7XX I2C Controller Device Tree Bindings
 
 description: |
-  The NPCM750x includes sixteen I2C bus controllers. All Controllers support
-  both master and slave mode. Each controller can switch between master and slave
-  at run time (i.e. IPMB mode). Each controller has two 16 byte HW FIFO for TX and
-  RX.
+  I2C bus controllers of the NPCM series support both master and
+  slave mode. Each controller can switch between master and slave at run time
+  (i.e. IPMB mode). HW FIFO for TX and RX are supported.
 
 maintainers:
   - Tali Perry <[email protected]>
 
 properties:
   compatible:
-    const: nuvoton,npcm750-i2c
+     enum:
+      - nuvoton,npcm750-i2c
+      - nuvoton,npcm845-i2c
 
   reg:
     maxItems: 1

@@ -36,11 +37,16 @@ properties:
     default: 100000
     enum: [100000, 400000, 1000000]
 
+  nuvoton,sys-mgr:
+    $ref: "/schemas/types.yaml#/definitions/phandle"
+    description: The phandle of system manager register node.
+
 required:
   - compatible
   - reg
   - interrupts
   - clocks
+  - nuvoton,sys-mgr
 
 allOf:
   - $ref: /schemas/i2c/i2c-controller.yaml#

@@ -57,6 +63,7 @@ examples:
         clock-frequency = <100000>;
         interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
         compatible = "nuvoton,npcm750-i2c";
+        nuvoton,sys-mgr = <&gcr>;
     };
 
 ...
Signature · 1 line
2.17.1
maintainer
#3 lore raw

Re: [PATCH v2 02/11] dt-bindings: i2c: npcm: support NPCM845

Quoted 10 lines · Tyrone Ting

From: Tyrone Ting <[email protected]>

Add compatible and nuvoton,sys-mgr description for NPCM i2c module.

Signed-off-by Tyrone Ting <[email protected]>
Signed-off-by Tali Perry <[email protected]>
---
 .../bindings/i2c/nuvoton,npcm7xx-i2c.yaml       | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

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:
./Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml:19:6: [warning] wrong indentation: expected 4 but found 5 (indentation) ./Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml:20:7: [warning] wrong indentation: expected 7 but found 6 (indentation)

dtschema/dtc warnings/errors:

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1595125

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.

author
#4 lore raw

Re: [PATCH v2 02/11] dt-bindings: i2c: npcm: support NPCM845

Hi Rob:

Thank you for your comments and they will be addressed.

Rob Herring <[email protected]> 於 2022年2月21日 週一 上午10:36寫道：

Quoted 29 lines · Tyrone Ting
Quoted 10 lines

From: Tyrone Ting <[email protected]>

Add compatible and nuvoton,sys-mgr description for NPCM i2c module.

Signed-off-by Tyrone Ting <[email protected]>
Signed-off-by Tali Perry <[email protected]>
---
 .../bindings/i2c/nuvoton,npcm7xx-i2c.yaml       | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

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:
./Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml:19:6: [warning] wrong indentation: expected 4 but found 5 (indentation) ./Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml:20:7: [warning] wrong indentation: expected 7 but found 6 (indentation)

dtschema/dtc warnings/errors:

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1595125

This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1.

Quoted 8 lines

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.

Best regards,
Tyrone

maintainer
#5 lore raw

Re: [PATCH v2 02/11] dt-bindings: i2c: npcm: support NPCM845

In reply to Tyrone Ting
Quoted 42 lines · Tyrone Ting

From: Tyrone Ting <[email protected]>

Add compatible and nuvoton,sys-mgr description for NPCM i2c module.

Signed-off-by Tyrone Ting <[email protected]>
Signed-off-by Tali Perry <[email protected]>
---
 .../bindings/i2c/nuvoton,npcm7xx-i2c.yaml       | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)
Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml +6 -5
index 128444942aec..809c51ac32fe 100644
@@ -7,17 +7,18 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
 title: nuvoton NPCM7XX I2C Controller Device Tree Bindings
 
 description: |
-  The NPCM750x includes sixteen I2C bus controllers. All Controllers support
-  both master and slave mode. Each controller can switch between master and slave
-  at run time (i.e. IPMB mode). Each controller has two 16 byte HW FIFO for TX and
-  RX.
+  I2C bus controllers of the NPCM series support both master and
+  slave mode. Each controller can switch between master and slave at run time
+  (i.e. IPMB mode). HW FIFO for TX and RX are supported.
 
 maintainers:
   - Tali Perry <[email protected]>
 
 properties:
   compatible:
-    const: nuvoton,npcm750-i2c
+     enum:
+      - nuvoton,npcm750-i2c
+      - nuvoton,npcm845-i2c
 
   reg:
     maxItems: 1

@@ -36,11 +37,16 @@ properties:
     default: 100000
     enum: [100000, 400000, 1000000]

Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml properties: · line 49

Quoted 10 lines
Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml +5 -0
+  nuvoton,sys-mgr:
+    $ref: "/schemas/types.yaml#/definitions/phandle"
+    description: The phandle of system manager register node.
+
 required:
   - compatible
   - reg
   - interrupts
   - clocks
+  - nuvoton,sys-mgr

You can't make nuvoton,sys-mgr required for existing users. You can add an if/then schema for nuvoton,npcm845-i2c if you want to make it required in that case.

Rob

author
#6 lore raw

Re: [PATCH v2 02/11] dt-bindings: i2c: npcm: support NPCM845

Hi Rob:

Thank you for your comment and it'll be addressed.

Rob Herring <[email protected]> 於 2022年2月23日 週三 上午1:56寫道：

Quoted 59 lines · Tyrone Ting
Quoted 42 lines

From: Tyrone Ting <[email protected]>

Add compatible and nuvoton,sys-mgr description for NPCM i2c module.

Signed-off-by Tyrone Ting <[email protected]>
Signed-off-by Tali Perry <[email protected]>
---
 .../bindings/i2c/nuvoton,npcm7xx-i2c.yaml       | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)
Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml +6 -5
index 128444942aec..809c51ac32fe 100644
@@ -7,17 +7,18 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
 title: nuvoton NPCM7XX I2C Controller Device Tree Bindings
 
 description: |
-  The NPCM750x includes sixteen I2C bus controllers. All Controllers support
-  both master and slave mode. Each controller can switch between master and slave
-  at run time (i.e. IPMB mode). Each controller has two 16 byte HW FIFO for TX and
-  RX.
+  I2C bus controllers of the NPCM series support both master and
+  slave mode. Each controller can switch between master and slave at run time
+  (i.e. IPMB mode). HW FIFO for TX and RX are supported.
 
 maintainers:
   - Tali Perry <[email protected]>
 
 properties:
   compatible:
-    const: nuvoton,npcm750-i2c
+     enum:
+      - nuvoton,npcm750-i2c
+      - nuvoton,npcm845-i2c
 
   reg:
     maxItems: 1

@@ -36,11 +37,16 @@ properties:
     default: 100000
     enum: [100000, 400000, 1000000]

Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml properties: · line 49

Quoted 10 lines
Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml +5 -0
+  nuvoton,sys-mgr:
+    $ref: "/schemas/types.yaml#/definitions/phandle"
+    description: The phandle of system manager register node.
+
 required:
   - compatible
   - reg
   - interrupts
   - clocks
+  - nuvoton,sys-mgr

You can't make nuvoton,sys-mgr required for existing users. You can add an if/then schema for nuvoton,npcm845-i2c if you want to make it required in that case.

Quoted 1 line

Rob

Best regards,
Tyrone

author
#7 lore raw

PATCH v2 3/11 i2c: npcm: Fix client address calculation

In reply to Tyrone Ting

From: Tali Perry <[email protected]>

Fix i2c client address by left-shifting 1 bit before applying it to the data register.

Fixes 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver")
Signed-off-by Tali Perry <[email protected]>
Signed-off-by Tyrone Ting <[email protected]>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/i2c/busses/i2c-npcm7xx.c +1 -1
index 2ad166355ec9..4c225e1a058f 100644
@@ -2131,7 +2131,7 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 	}
 
 	npcm_i2c_init_params(bus);
-	bus->dest_addr = slave_addr;
+	bus->dest_addr = slave_addr << 1;
 	bus->msgs = msgs;
 	bus->msgs_num = num;
 	bus->cmd_err = 0;
Signature · 1 line
2.17.1
author
#8 lore raw

PATCH v2 4/11 i2c: npcm: Update gcr property name

In reply to Tyrone Ting

From: Tali Perry <[email protected]>

Use a generic name for NPCM system manager reigster.

Fixes 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver")
Signed-off-by Tali Perry <[email protected]>
Signed-off-by Tyrone Ting <[email protected]>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/i2c/busses/i2c-npcm7xx.c +2 -1
index 4c225e1a058f..a51db3f50274 100644
@@ -2236,6 +2236,7 @@ static int npcm_i2c_probe_bus(struct platform_device *pdev)
 	static struct regmap *clk_regmap;
 	int irq;
 	int ret;
+	struct device_node *np = pdev->dev.of_node;
 
 	bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
 	if (!bus)

@@ -2250,7 +2251,7 @@ static int npcm_i2c_probe_bus(struct platform_device *pdev)
 		return PTR_ERR(i2c_clk);
 	bus->apb_clk = clk_get_rate(i2c_clk);
 
-	gcr_regmap = syscon_regmap_lookup_by_compatible("nuvoton,npcm750-gcr");
+	gcr_regmap = syscon_regmap_lookup_by_phandle(np, "nuvoton,sys-mgr");
 	if (IS_ERR(gcr_regmap))
 		return PTR_ERR(gcr_regmap);
 	regmap_write(gcr_regmap, NPCM_I2CSEGCTL, NPCM_I2CSEGCTL_INIT_VAL);
Signature · 1 line
2.17.1
#9 lore raw

Re: [PATCH v2 04/11] i2c: npcm: Update gcr property name

Quoted 3 lines · Tyrone Ting

From: Tali Perry <[email protected]>

Use a generic name for NPCM system manager reigster.

The subject is not accurate and you entirely skipped in commit msg the fact of an ABI break.

You do not update a property name but you change the way of getting GCR regmap.

Best regards,
Krzysztof

author
#10 lore raw

Re: [PATCH v2 04/11] i2c: npcm: Update gcr property name

Hi Krzysztof:

Thank you for your comments and please find my reply next to your comments.

Krzysztof Kozlowski <[email protected]> 於 2022年2月20日 週日 下午5:32寫道：

Quoted 12 lines · Tyrone Ting
Quoted 3 lines

From: Tali Perry <[email protected]>

Use a generic name for NPCM system manager reigster.

The subject is not accurate and you entirely skipped in commit msg the fact of an ABI break.

You do not update a property name but you change the way of getting GCR regmap.

I'll change the subject since the patch is to change the way to getting GCR regmap as you indicated.

About the ABI break, I responded in the discussion thread of [PATCH v2 00/11] i2c: npcm: Bug fixes timeout, spurious interrupts.

Quoted 2 lines

Best regards,
Krzysztof

Best regards,
Tyrone

author
#11 lore raw

PATCH v2 5/11 i2c: npcm: Remove unused clock node

In reply to Tyrone Ting

From: Tali Perry <[email protected]>

Remove unused npcm750-clk node.

Fixes 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver")
Signed-off-by Tali Perry <[email protected]>
Signed-off-by Tyrone Ting <[email protected]>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 5 -----
 1 file changed, 5 deletions(-)
drivers/i2c/busses/i2c-npcm7xx.c +0 -5
index a51db3f50274..9ccb9958945e 100644
@@ -2233,7 +2233,6 @@ static int npcm_i2c_probe_bus(struct platform_device *pdev)
 	struct i2c_adapter *adap;
 	struct clk *i2c_clk;
 	static struct regmap *gcr_regmap;
-	static struct regmap *clk_regmap;
 	int irq;
 	int ret;
 	struct device_node *np = pdev->dev.of_node;

@@ -2256,10 +2255,6 @@ static int npcm_i2c_probe_bus(struct platform_device *pdev)
 		return PTR_ERR(gcr_regmap);
 	regmap_write(gcr_regmap, NPCM_I2CSEGCTL, NPCM_I2CSEGCTL_INIT_VAL);
 
-	clk_regmap = syscon_regmap_lookup_by_compatible("nuvoton,npcm750-clk");
-	if (IS_ERR(clk_regmap))
-		return PTR_ERR(clk_regmap);
-
 	bus->reg = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(bus->reg))
 		return PTR_ERR(bus->reg);
Signature · 1 line
2.17.1
Reviewed-by on v2 5/11
#12 lore raw

Re: [PATCH v2 05/11] i2c: npcm: Remove unused clock node

Quoted 3 lines · Tyrone Ting

From: Tali Perry <[email protected]>

Remove unused npcm750-clk node.

You're not actually removing a node, for example in the sense of removing a devicetree node from a devicetree.

So, I think "Remove unused variable clk_regmap." would be a better description.

Quoted 21 lines
Fixes 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver")
Signed-off-by Tali Perry <[email protected]>
Signed-off-by Tyrone Ting <[email protected]>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 5 -----
 1 file changed, 5 deletions(-)
drivers/i2c/busses/i2c-npcm7xx.c +0 -1
index a51db3f50274..9ccb9958945e 100644
@@ -2233,7 +2233,6 @@ static int npcm_i2c_probe_bus(struct platform_device *pdev)
 	struct i2c_adapter *adap;
 	struct clk *i2c_clk;
 	static struct regmap *gcr_regmap;
-	static struct regmap *clk_regmap;
 	int irq;
 	int ret;
 	struct device_node *np = pdev->dev.of_node;

drivers/i2c/busses/i2c-npcm7xx.c static int npcm_i2c_probe_bus(struct platform_device *pdev) · line 2262

Quoted 7 lines
drivers/i2c/busses/i2c-npcm7xx.c +0 -4
@@ -2256,10 +2255,6 @@ static int npcm_i2c_probe_bus(struct platform_device *pdev)
 		return PTR_ERR(gcr_regmap);
 	regmap_write(gcr_regmap, NPCM_I2CSEGCTL, NPCM_I2CSEGCTL_INIT_VAL);
 
-	clk_regmap = syscon_regmap_lookup_by_compatible("nuvoton,npcm750-clk");
-	if (IS_ERR(clk_regmap))
-		return PTR_ERR(clk_regmap);
-

The change itself looks good to me,

Reviewed-by Jonathan Neuschäfer <[email protected]>
Quoted 6 lines
drivers/i2c/busses/i2c-npcm7xx.c +0 -0
 	bus->reg = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(bus->reg))
 		return PTR_ERR(bus->reg);
Signature · 1 line
2.17.1

Thanks,
Jonathan

author
#13 lore raw

Re: [PATCH v2 05/11] i2c: npcm: Remove unused clock node

Hi Jonathan:

Thank you for your comments and please find my reply next to your comments.

Jonathan Neuschäfer <[email protected]> 於 2022年2月21日 週一 下午7:49寫道：

Quoted 12 lines · Tyrone Ting
Quoted 3 lines

From: Tali Perry <[email protected]>

Remove unused npcm750-clk node.

You're not actually removing a node, for example in the sense of removing a devicetree node from a devicetree.

So, I think "Remove unused variable clk_regmap." would be a better description.

May I modify the description according to your input and attach "Reviewed-by: Jonathan Neuschäfer <[email protected]>" onto this commit in the next version of the patch set?

Quoted 41 lines
Quoted 21 lines
Fixes 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver")
Signed-off-by Tali Perry <[email protected]>
Signed-off-by Tyrone Ting <[email protected]>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 5 -----
 1 file changed, 5 deletions(-)
drivers/i2c/busses/i2c-npcm7xx.c +0 -1
index a51db3f50274..9ccb9958945e 100644
@@ -2233,7 +2233,6 @@ static int npcm_i2c_probe_bus(struct platform_device *pdev)
      struct i2c_adapter *adap;
      struct clk *i2c_clk;
      static struct regmap *gcr_regmap;
-     static struct regmap *clk_regmap;
      int irq;
      int ret;
      struct device_node *np = pdev->dev.of_node;

drivers/i2c/busses/i2c-npcm7xx.c static int npcm_i2c_probe_bus(struct platform_device *pdev) · line 2262

Quoted 7 lines
drivers/i2c/busses/i2c-npcm7xx.c +0 -4
@@ -2256,10 +2255,6 @@ static int npcm_i2c_probe_bus(struct platform_device *pdev)
              return PTR_ERR(gcr_regmap);
      regmap_write(gcr_regmap, NPCM_I2CSEGCTL, NPCM_I2CSEGCTL_INIT_VAL);
 
-     clk_regmap = syscon_regmap_lookup_by_compatible("nuvoton,npcm750-clk");
-     if (IS_ERR(clk_regmap))
-             return PTR_ERR(clk_regmap);
-

The change itself looks good to me,

Reviewed-by Jonathan Neuschäfer <[email protected]>
Quoted 6 lines
drivers/i2c/busses/i2c-npcm7xx.c +0 -1
      bus->reg = devm_platform_ioremap_resource(pdev, 0);
      if (IS_ERR(bus->reg))
              return PTR_ERR(bus->reg);
--

2.17.1

Quoted 2 lines

Thanks,
Jonathan

Best regards,
Tyrone

#14 lore raw

Re: [PATCH v2 05/11] i2c: npcm: Remove unused clock node

Quoted 14 lines · Tyrone Ting

Hi Jonathan:

Thank you for your comments and please find my reply next to your comments.

Jonathan Neuschäfer <[email protected]> 於 2022年2月21日 週一 下午7:49寫道：

Quoted 9 lines · Tyrone Ting
Quoted 3 lines

From: Tali Perry <[email protected]>

Remove unused npcm750-clk node.

You're not actually removing a node, for example in the sense of removing a devicetree node from a devicetree.

Quoted 7 lines
Quoted 3 lines

So, I think "Remove unused variable clk_regmap." would be a better description.

May I modify the description according to your input and attach "Reviewed-by: Jonathan Neuschäfer <[email protected]>" onto this commit in the next version of the patch set?

Yes!

Jonathan

author
#15 lore raw

Re: [PATCH v2 05/11] i2c: npcm: Remove unused clock node

Hi Jonathan:

Got it and thank you.

Jonathan Neuschäfer <[email protected]> 於 2022年2月22日 週二 下午11:59寫道：

Quoted 20 lines · Tyrone Ting
Quoted 18 lines

Hi Jonathan:

Thank you for your comments and please find my reply next to your comments.

Jonathan Neuschäfer <[email protected]> 於 2022年2月21日 週一 下午7:49寫道：

Quoted 12 lines · Tyrone Ting
Quoted 3 lines

From: Tali Perry <[email protected]>

Remove unused npcm750-clk node.

You're not actually removing a node, for example in the sense of removing a devicetree node from a devicetree.

So, I think "Remove unused variable clk_regmap." would be a better description.

Quoted 8 lines
Quoted 3 lines

May I modify the description according to your input and attach "Reviewed-by: Jonathan Neuschäfer <[email protected]>" onto this commit in the next version of the patch set?

Yes!

Jonathan

Best regards,
Tyrone

author
#16 lore raw

PATCH v2 6/11 i2c: npcm: Fix timeout calculation

In reply to Tyrone Ting

From: Tali Perry <[email protected]>

Use adap.timeout for timeout calculation instead of hard-coded value of 35ms.

Fixes 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver")
Signed-off-by Tali Perry <[email protected]>
Signed-off-by Tyrone Ting <[email protected]>
Reported-by kernel test robot <[email protected]>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/i2c/busses/i2c-npcm7xx.c +4 -4
index 9ccb9958945e..1e6e3f7f59a3 100644
@@ -2047,7 +2047,7 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 	u16 nwrite, nread;
 	u8 *write_data, *read_data;
 	u8 slave_addr;
-	int timeout;
+	unsigned long timeout;
 	int ret = 0;
 	bool read_block = false;
 	bool read_PEC = false;

@@ -2099,13 +2099,13 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 	 * 9: bits per transaction (including the ack/nack)
 	 */
 	timeout_usec = (2 * 9 * USEC_PER_SEC / bus->bus_freq) * (2 + nread + nwrite);
-	timeout = max(msecs_to_jiffies(35), usecs_to_jiffies(timeout_usec));
+	timeout = max_t(unsigned long, bus->adap.timeout, usecs_to_jiffies(timeout_usec));
 	if (nwrite >= 32 * 1024 || nread >= 32 * 1024) {
 		dev_err(bus->dev, "i2c%d buffer too big\n", bus->num);
 		return -EINVAL;
 	}
 
-	time_left = jiffies + msecs_to_jiffies(DEFAULT_STALL_COUNT) + 1;
+	time_left = jiffies + timeout + 1;
 	do {
 		/*
 		 * we must clear slave address immediately when the bus is not

@@ -2265,7 +2265,7 @@ static int npcm_i2c_probe_bus(struct platform_device *pdev)
 	adap = &bus->adap;
 	adap->owner = THIS_MODULE;
 	adap->retries = 3;
-	adap->timeout = HZ;
+	adap->timeout = msecs_to_jiffies(35);
 	adap->algo = &npcm_i2c_algo;
 	adap->quirks = &npcm_i2c_quirks;
 	adap->algo_data = bus;
Signature · 1 line
2.17.1
author
#17 lore raw

PATCH v2 7/11 i2c: npcm: Add tx complete counter

In reply to Tyrone Ting

From: Tali Perry <[email protected]>

tx_complete counter is used to indicate successful transaction count.
Similar counters for failed tx were previously added.

Fixes 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver")
Signed-off-by Tali Perry <[email protected]>
Signed-off-by Tyrone Ting <[email protected]>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/i2c/busses/i2c-npcm7xx.c +4 -0
index 1e6e3f7f59a3..ee4757eff4b3 100644
@@ -314,6 +314,7 @@ struct npcm_i2c {
 	u64 rec_fail_cnt;
 	u64 nack_cnt;
 	u64 timeout_cnt;
+	u64 tx_complete_cnt;
 };
 
 static inline void npcm_i2c_select_bank(struct npcm_i2c *bus,

@@ -684,6 +685,8 @@ static void npcm_i2c_callback(struct npcm_i2c *bus,
 	switch (op_status) {
 	case I2C_MASTER_DONE_IND:
 		bus->cmd_err = bus->msgs_num;
+		if (bus->tx_complete_cnt < ULLONG_MAX)
+			bus->tx_complete_cnt++;
 		fallthrough;
 	case I2C_BLOCK_BYTES_ERR_IND:
 		/* Master tx finished and all transmit bytes were sent */

@@ -2223,6 +2226,7 @@ static void npcm_i2c_init_debugfs(struct platform_device *pdev,
 	debugfs_create_u64("rec_succ_cnt", 0444, d, &bus->rec_succ_cnt);
 	debugfs_create_u64("rec_fail_cnt", 0444, d, &bus->rec_fail_cnt);
 	debugfs_create_u64("timeout_cnt", 0444, d, &bus->timeout_cnt);
+	debugfs_create_u64("tx_complete_cnt", 0444, d, &bus->tx_complete_cnt);
 
 	bus->debugfs = d;
 }
Signature · 1 line
2.17.1
author
#18 lore raw

PATCH v2 8/11 i2c: npcm: Correct register access width

In reply to Tyrone Ting

From: Tyrone Ting <[email protected]>

Use ioread8 instead of ioread32 to access the SMBnCTL3 register since the register is only 8-bit wide.

Fixes 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver")
Signed-off-by Tyrone Ting <[email protected]>
Signed-off-by Tali Perry <[email protected]>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/i2c/busses/i2c-npcm7xx.c +2 -2
index ee4757eff4b3..4715afcf9ac4 100644
@@ -360,14 +360,14 @@ static int npcm_i2c_get_SCL(struct i2c_adapter *_adap)
 {
 	struct npcm_i2c *bus = container_of(_adap, struct npcm_i2c, adap);
 
-	return !!(I2CCTL3_SCL_LVL & ioread32(bus->reg + NPCM_I2CCTL3));
+	return !!(I2CCTL3_SCL_LVL & ioread8(bus->reg + NPCM_I2CCTL3));
 }
 
 static int npcm_i2c_get_SDA(struct i2c_adapter *_adap)
 {
 	struct npcm_i2c *bus = container_of(_adap, struct npcm_i2c, adap);
 
-	return !!(I2CCTL3_SDA_LVL & ioread32(bus->reg + NPCM_I2CCTL3));
+	return !!(I2CCTL3_SDA_LVL & ioread8(bus->reg + NPCM_I2CCTL3));
 }
 
 static inline u16 npcm_i2c_get_index(struct npcm_i2c *bus)
Signature · 1 line
2.17.1
Reviewed-by on v2 8/11
#19 lore raw

Re: [PATCH v2 08/11] i2c: npcm: Correct register access width

Quoted 23 lines · Tyrone Ting

From: Tyrone Ting <[email protected]>

Use ioread8 instead of ioread32 to access the SMBnCTL3 register since the register is only 8-bit wide.

Fixes 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver")
Signed-off-by Tyrone Ting <[email protected]>
Signed-off-by Tali Perry <[email protected]>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/i2c/busses/i2c-npcm7xx.c +1 -1
index ee4757eff4b3..4715afcf9ac4 100644
@@ -360,14 +360,14 @@ static int npcm_i2c_get_SCL(struct i2c_adapter *_adap)
 {
 	struct npcm_i2c *bus = container_of(_adap, struct npcm_i2c, adap);
 
-	return !!(I2CCTL3_SCL_LVL & ioread32(bus->reg + NPCM_I2CCTL3));
+	return !!(I2CCTL3_SCL_LVL & ioread8(bus->reg + NPCM_I2CCTL3));
 }

drivers/i2c/busses/i2c-npcm7xx.c static int npcm_i2c_get_SCL(struct i2c_adapter *_adap) · line 373

Quoted 10 lines
drivers/i2c/busses/i2c-npcm7xx.c +1 -1
 
 static int npcm_i2c_get_SDA(struct i2c_adapter *_adap)
 {
 	struct npcm_i2c *bus = container_of(_adap, struct npcm_i2c, adap);
 
-	return !!(I2CCTL3_SDA_LVL & ioread32(bus->reg + NPCM_I2CCTL3));
+	return !!(I2CCTL3_SDA_LVL & ioread8(bus->reg + NPCM_I2CCTL3));
 }
 
 static inline u16 npcm_i2c_get_index(struct npcm_i2c *bus)
Reviewed-by Jonathan Neuschäfer <[email protected]>

Thanks,
Jonathan

author
#20 lore raw

PATCH v2 9/11 i2c: npcm: Handle spurious interrupts

In reply to Tyrone Ting

From: Tali Perry <[email protected]>

In order to better handle spurious interrupts:
1. Disable incoming interrupts in master only mode.
2. Clear end of busy (EOB) after every interrupt.
3. Return correct status during interrupt.

Fixes 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver")
Signed-off-by Tali Perry <[email protected]>
Signed-off-by Tyrone Ting <[email protected]>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 92 ++++++++++++++++++++++----------
 1 file changed, 63 insertions(+), 29 deletions(-)
drivers/i2c/busses/i2c-npcm7xx.c +63 -29
index 4715afcf9ac4..a265f6fdbf5c 100644
@@ -564,6 +564,15 @@ static inline void npcm_i2c_nack(struct npcm_i2c *bus)
 	iowrite8(val, bus->reg + NPCM_I2CCTL1);
 }
 
+static inline void npcm_i2c_clear_master_status(struct npcm_i2c *bus)
+{
+	u8 val;
+
+	/* Clear NEGACK, STASTR and BER bits */
+	val = NPCM_I2CST_BER | NPCM_I2CST_NEGACK | NPCM_I2CST_STASTR;
+	iowrite8(val, bus->reg + NPCM_I2CST);
+}
+
 #if IS_ENABLED(CONFIG_I2C_SLAVE)
 static void npcm_i2c_slave_int_enable(struct npcm_i2c *bus, bool enable)
 {

@@ -643,8 +652,8 @@ static void npcm_i2c_reset(struct npcm_i2c *bus)
 	iowrite8(NPCM_I2CCST_BB, bus->reg + NPCM_I2CCST);
 	iowrite8(0xFF, bus->reg + NPCM_I2CST);
 
-	/* Clear EOB bit */
-	iowrite8(NPCM_I2CCST3_EO_BUSY, bus->reg + NPCM_I2CCST3);
+	/* Clear and disable EOB */
+	npcm_i2c_eob_int(bus, false);
 
 	/* Clear all fifo bits: */
 	iowrite8(NPCM_I2CFIF_CTS_CLR_FIFO, bus->reg + NPCM_I2CFIF_CTS);

@@ -656,6 +665,9 @@ static void npcm_i2c_reset(struct npcm_i2c *bus)
 	}
 #endif
 
+	/* clear status bits for spurious interrupts */
+	npcm_i2c_clear_master_status(bus);
+
 	bus->state = I2C_IDLE;
 }
 

@@ -818,15 +830,6 @@ static void npcm_i2c_read_fifo(struct npcm_i2c *bus, u8 bytes_in_fifo)
 	}
 }
 
-static inline void npcm_i2c_clear_master_status(struct npcm_i2c *bus)
-{
-	u8 val;
-
-	/* Clear NEGACK, STASTR and BER bits */
-	val = NPCM_I2CST_BER | NPCM_I2CST_NEGACK | NPCM_I2CST_STASTR;
-	iowrite8(val, bus->reg + NPCM_I2CST);
-}
-
 static void npcm_i2c_master_abort(struct npcm_i2c *bus)
 {
 	/* Only current master is allowed to issue a stop condition */

@@ -1234,7 +1237,16 @@ static irqreturn_t npcm_i2c_int_slave_handler(struct npcm_i2c *bus)
 		ret = IRQ_HANDLED;
 	} /* SDAST */
 
-	return ret;
+	/*
+	 * if irq is not one of the above, make sure EOB is disabled and all
+	 * status bits are cleared.
+	 */
+	if (ret == IRQ_NONE) {
+		npcm_i2c_eob_int(bus, false);
+		npcm_i2c_clear_master_status(bus);
+	}
+
+	return IRQ_HANDLED;
 }
 
 static int npcm_i2c_reg_slave(struct i2c_client *client)

@@ -1470,6 +1482,9 @@ static void npcm_i2c_irq_handle_nack(struct npcm_i2c *bus)
 		npcm_i2c_eob_int(bus, false);
 		npcm_i2c_master_stop(bus);
 
+		/* Clear SDA Status bit (by reading dummy byte) */
+		npcm_i2c_rd_byte(bus);
+
 		/*
 		 * The bus is released from stall only after the SW clears
 		 * NEGACK bit. Then a Stop condition is sent.

@@ -1477,6 +1492,8 @@ static void npcm_i2c_irq_handle_nack(struct npcm_i2c *bus)
 		npcm_i2c_clear_master_status(bus);
 		readx_poll_timeout_atomic(ioread8, bus->reg + NPCM_I2CCST, val,
 					  !(val & NPCM_I2CCST_BUSY), 10, 200);
+		/* verify no status bits are still set after bus is released */
+		npcm_i2c_clear_master_status(bus);
 	}
 	bus->state = I2C_IDLE;
 

@@ -1675,10 +1692,10 @@ static int npcm_i2c_recovery_tgclk(struct i2c_adapter *_adap)
 	int              iter = 27;
 
 	if ((npcm_i2c_get_SDA(_adap) == 1) && (npcm_i2c_get_SCL(_adap) == 1)) {
-		dev_dbg(bus->dev, "bus%d recovery skipped, bus not stuck",
-			bus->num);
+		dev_dbg(bus->dev, "bus%d-0x%x recovery skipped, bus not stuck",
+			bus->num, bus->dest_addr);
 		npcm_i2c_reset(bus);
-		return status;
+		return 0;
 	}
 
 	npcm_i2c_int_enable(bus, false);

@@ -1912,6 +1929,7 @@ static int npcm_i2c_init_module(struct npcm_i2c *bus, enum i2c_mode mode,
 	    bus_freq_hz < I2C_FREQ_MIN_HZ || bus_freq_hz > I2C_FREQ_MAX_HZ)
 		return -EINVAL;
 
+	npcm_i2c_int_enable(bus, false);
 	npcm_i2c_disable(bus);
 
 	/* Configure FIFO mode : */

@@ -1940,10 +1958,18 @@ static int npcm_i2c_init_module(struct npcm_i2c *bus, enum i2c_mode mode,
 	val = (val | NPCM_I2CCTL1_NMINTE) & ~NPCM_I2CCTL1_RWS;
 	iowrite8(val, bus->reg + NPCM_I2CCTL1);
 
-	npcm_i2c_int_enable(bus, true);
-
 	npcm_i2c_reset(bus);
 
+	/* check HW is OK: SDA and SCL should be high at this point. */
+	if ((npcm_i2c_get_SDA(&bus->adap) == 0) ||
+	    (npcm_i2c_get_SCL(&bus->adap) == 0)) {
+		dev_err(bus->dev, "I2C%d init fail: lines are low", bus->num);
+		dev_err(bus->dev, "SDA=%d SCL=%d", npcm_i2c_get_SDA(&bus->adap),
+			npcm_i2c_get_SCL(&bus->adap));
+		return -ENXIO;
+	}
+
+	npcm_i2c_int_enable(bus, true);
 	return 0;
 }
 

@@ -1991,10 +2017,14 @@ static irqreturn_t npcm_i2c_bus_irq(int irq, void *dev_id)
 #if IS_ENABLED(CONFIG_I2C_SLAVE)
 	if (bus->slave) {
 		bus->master_or_slave = I2C_SLAVE;
-		return npcm_i2c_int_slave_handler(bus);
+		if (npcm_i2c_int_slave_handler(bus))
+			return IRQ_HANDLED;
 	}
 #endif
-	return IRQ_NONE;
+	/* clear status bits for spurious interrupts */
+	npcm_i2c_clear_master_status(bus);
+
+	return IRQ_HANDLED;
 }
 
 static bool npcm_i2c_master_start_xmit(struct npcm_i2c *bus,

@@ -2051,7 +2081,6 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 	u8 *write_data, *read_data;
 	u8 slave_addr;
 	unsigned long timeout;
-	int ret = 0;
 	bool read_block = false;
 	bool read_PEC = false;
 	u8 bus_busy;

@@ -2141,12 +2170,12 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 	bus->read_block_use = read_block;
 
 	reinit_completion(&bus->cmd_complete);
-	if (!npcm_i2c_master_start_xmit(bus, slave_addr, nwrite, nread,
-					write_data, read_data, read_PEC,
-					read_block))
-		ret = -EBUSY;
 
-	if (ret != -EBUSY) {
+	npcm_i2c_int_enable(bus, true);
+
+	if (npcm_i2c_master_start_xmit(bus, slave_addr, nwrite, nread,
+				       write_data, read_data, read_PEC,
+				       read_block)) {
 		time_left = wait_for_completion_timeout(&bus->cmd_complete,
 							timeout);
 

@@ -2160,26 +2189,31 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 			}
 		}
 	}
-	ret = bus->cmd_err;
 
 	/* if there was BER, check if need to recover the bus: */
 	if (bus->cmd_err == -EAGAIN)
-		ret = i2c_recover_bus(adap);
+		bus->cmd_err = i2c_recover_bus(adap);
 
 	/*
 	 * After any type of error, check if LAST bit is still set,
 	 * due to a HW issue.
 	 * It cannot be cleared without resetting the module.
 	 */
-	if (bus->cmd_err &&
-	    (NPCM_I2CRXF_CTL_LAST_PEC & ioread8(bus->reg + NPCM_I2CRXF_CTL)))
+	else if (bus->cmd_err &&
+		 (NPCM_I2CRXF_CTL_LAST_PEC & ioread8(bus->reg + NPCM_I2CRXF_CTL)))
 		npcm_i2c_reset(bus);
 
+	/* after any xfer, successful or not, stall and EOB must be disabled */
+	npcm_i2c_stall_after_start(bus, false);
+	npcm_i2c_eob_int(bus, false);
+
 #if IS_ENABLED(CONFIG_I2C_SLAVE)
 	/* reenable slave if it was enabled */
 	if (bus->slave)
 		iowrite8((bus->slave->addr & 0x7F) | NPCM_I2CADDR_SAEN,
 			 bus->reg + NPCM_I2CADDR1);
+#else
+	npcm_i2c_int_enable(bus, false);
 #endif
 	return bus->cmd_err;
 }
Signature · 1 line
2.17.1
author
#21 lore raw

PATCH v2 10/11 i2c: npcm: Remove own slave addresses 2:10

In reply to Tyrone Ting

From: Tali Perry <[email protected]>

NPCM can support up to 10 own slave addresses. In practice, only one address is actually being used. In order to access addresses 2 and above, need to switch register banks. The switch needs spinlock.
To avoid using spinlock for this useless feature removed support of SA >= 2.

Also fix returned slave event enum.

Remove some comment since the bank selection is not required. The bank selection is not required since the supported slave addresses are reduced.

Fixes 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver")
Signed-off-by Tali Perry <[email protected]>
Signed-off-by Tyrone Ting <[email protected]>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 46 ++++++++++++++++----------------
 1 file changed, 23 insertions(+), 23 deletions(-)
drivers/i2c/busses/i2c-npcm7xx.c +23 -23
index a265f6fdbf5c..2cbf9c679aed 100644
@@ -124,6 +124,8 @@ enum i2c_addr {
  * use this array to get the address or each register.
  */
 #define I2C_NUM_OWN_ADDR 10
+#define I2C_NUM_OWN_ADDR_SUPPORTED 2
+
 static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = {
 	NPCM_I2CADDR1, NPCM_I2CADDR2, NPCM_I2CADDR3, NPCM_I2CADDR4,
 	NPCM_I2CADDR5, NPCM_I2CADDR6, NPCM_I2CADDR7, NPCM_I2CADDR8,

@@ -392,14 +394,10 @@ static void npcm_i2c_disable(struct npcm_i2c *bus)
 #if IS_ENABLED(CONFIG_I2C_SLAVE)
 	int i;
 
-	/* select bank 0 for I2C addresses */
-	npcm_i2c_select_bank(bus, I2C_BANK_0);
-
 	/* Slave addresses removal */
-	for (i = I2C_SLAVE_ADDR1; i < I2C_NUM_OWN_ADDR; i++)
+	for (i = I2C_SLAVE_ADDR1; i < I2C_NUM_OWN_ADDR_SUPPORTED; i++)
 		iowrite8(0, bus->reg + npcm_i2caddr[i]);
 
-	npcm_i2c_select_bank(bus, I2C_BANK_1);
 #endif
 	/* Disable module */
 	i2cctl2 = ioread8(bus->reg + NPCM_I2CCTL2);

@@ -604,8 +602,7 @@ static int npcm_i2c_slave_enable(struct npcm_i2c *bus, enum i2c_addr addr_type,
 			i2cctl1 &= ~NPCM_I2CCTL1_GCMEN;
 		iowrite8(i2cctl1, bus->reg + NPCM_I2CCTL1);
 		return 0;
-	}
-	if (addr_type == I2C_ARP_ADDR) {
+	} else if (addr_type == I2C_ARP_ADDR) {
 		i2cctl3 = ioread8(bus->reg + NPCM_I2CCTL3);
 		if (enable)
 			i2cctl3 |= I2CCTL3_ARPMEN;

@@ -614,16 +611,18 @@ static int npcm_i2c_slave_enable(struct npcm_i2c *bus, enum i2c_addr addr_type,
 		iowrite8(i2cctl3, bus->reg + NPCM_I2CCTL3);
 		return 0;
 	}
+	if (addr_type > I2C_SLAVE_ADDR2 && addr_type <= I2C_SLAVE_ADDR10) {
+		dev_err(bus->dev,
+			"try to enable more then 2 SA not supported\n");
+	}
 	if (addr_type >= I2C_ARP_ADDR)
 		return -EFAULT;
 	/* select bank 0 for address 3 to 10 */
-	if (addr_type > I2C_SLAVE_ADDR2)
-		npcm_i2c_select_bank(bus, I2C_BANK_0);
+
 	/* Set and enable the address */
 	iowrite8(sa_reg, bus->reg + npcm_i2caddr[addr_type]);
 	npcm_i2c_slave_int_enable(bus, enable);
-	if (addr_type > I2C_SLAVE_ADDR2)
-		npcm_i2c_select_bank(bus, I2C_BANK_1);
+
 	return 0;
 }
 #endif

@@ -846,15 +845,13 @@ static u8 npcm_i2c_get_slave_addr(struct npcm_i2c *bus, enum i2c_addr addr_type)
 {
 	u8 slave_add;
 
-	/* select bank 0 for address 3 to 10 */
-	if (addr_type > I2C_SLAVE_ADDR2)
-		npcm_i2c_select_bank(bus, I2C_BANK_0);
+	if (addr_type > I2C_SLAVE_ADDR2 && addr_type <= I2C_SLAVE_ADDR10) {
+		dev_err(bus->dev,
+			"get slave: try to use more then 2 slave addresses not supported\n");
+	}
 
 	slave_add = ioread8(bus->reg + npcm_i2caddr[(int)addr_type]);
 
-	if (addr_type > I2C_SLAVE_ADDR2)
-		npcm_i2c_select_bank(bus, I2C_BANK_1);
-
 	return slave_add;
 }
 

@@ -864,12 +861,12 @@ static int npcm_i2c_remove_slave_addr(struct npcm_i2c *bus, u8 slave_add)
 
 	/* Set the enable bit */
 	slave_add |= 0x80;
-	npcm_i2c_select_bank(bus, I2C_BANK_0);
-	for (i = I2C_SLAVE_ADDR1; i < I2C_NUM_OWN_ADDR; i++) {
+
+	for (i = I2C_SLAVE_ADDR1; i < I2C_NUM_OWN_ADDR_SUPPORTED; i++) {
 		if (ioread8(bus->reg + npcm_i2caddr[i]) == slave_add)
 			iowrite8(0, bus->reg + npcm_i2caddr[i]);
 	}
-	npcm_i2c_select_bank(bus, I2C_BANK_1);
+
 	return 0;
 }
 

@@ -924,11 +921,15 @@ static int npcm_i2c_slave_get_wr_buf(struct npcm_i2c *bus)
 	for (i = 0; i < I2C_HW_FIFO_SIZE; i++) {
 		if (bus->slv_wr_size >= I2C_HW_FIFO_SIZE)
 			break;
-		i2c_slave_event(bus->slave, I2C_SLAVE_READ_REQUESTED, &value);
+		if (bus->state == I2C_SLAVE_MATCH) {
+			i2c_slave_event(bus->slave, I2C_SLAVE_READ_REQUESTED, &value);
+			bus->state = I2C_OPER_STARTED;
+		} else {
+			i2c_slave_event(bus->slave, I2C_SLAVE_READ_PROCESSED, &value);
+		}
 		ind = (bus->slv_wr_ind + bus->slv_wr_size) % I2C_HW_FIFO_SIZE;
 		bus->slv_wr_buf[ind] = value;
 		bus->slv_wr_size++;
-		i2c_slave_event(bus->slave, I2C_SLAVE_READ_PROCESSED, &value);
 	}
 	return I2C_HW_FIFO_SIZE - ret;
 }

@@ -976,7 +977,6 @@ static void npcm_i2c_slave_xmit(struct npcm_i2c *bus, u16 nwrite,
 	if (nwrite == 0)
 		return;
 
-	bus->state = I2C_OPER_STARTED;
 	bus->operation = I2C_WRITE_OPER;
 
 	/* get the next buffer */
Signature · 1 line
2.17.1
author
#22 lore raw

PATCH v2 11/11 i2c: npcm: Support NPCM845

In reply to Tyrone Ting

From: Tyrone Ting <[email protected]>

Add NPCM8XX I2C support.
The NPCM8XX uses a similar i2c module as NPCM7XX. The internal HW FIFO is larger in NPCM8XX.

Signed-off-by Tyrone Ting <[email protected]>
Signed-off-by Tali Perry <[email protected]>
---
 drivers/i2c/busses/Kconfig       |  8 +--
 drivers/i2c/busses/Makefile      |  2 +-
 drivers/i2c/busses/i2c-npcm7xx.c | 87 ++++++++++++++++++++++----------
 3 files changed, 66 insertions(+), 31 deletions(-)
drivers/i2c/busses/Kconfig +4 -4
index 42da31c1ab70..ab9ee2de5e00 100644
@@ -817,13 +817,13 @@ config I2C_NOMADIK
 	  I2C interface from ST-Ericsson's Nomadik and Ux500 architectures,
 	  as well as the STA2X11 PCIe I/O HUB.
 
-config I2C_NPCM7XX
+config I2C_NPCM
 	tristate "Nuvoton I2C Controller"
-	depends on ARCH_NPCM7XX || COMPILE_TEST
+	depends on ARCH_NPCM || COMPILE_TEST
 	help
 	  If you say yes to this option, support will be included for the
-	  Nuvoton I2C controller, which is available on the NPCM7xx BMC
-	  controller.
+	  Nuvoton I2C controller, which is available on the NPCM BMC
+	  controllers.
 	  Driver can also support slave mode (select I2C_SLAVE).
 
 config I2C_OCORES
drivers/i2c/busses/Makefile +1 -1
index 1d00dce77098..01fdf74a5565 100644
@@ -80,7 +80,7 @@ obj-$(CONFIG_I2C_MT7621)	+= i2c-mt7621.o
 obj-$(CONFIG_I2C_MV64XXX)	+= i2c-mv64xxx.o
 obj-$(CONFIG_I2C_MXS)		+= i2c-mxs.o
 obj-$(CONFIG_I2C_NOMADIK)	+= i2c-nomadik.o
-obj-$(CONFIG_I2C_NPCM7XX)	+= i2c-npcm7xx.o
+obj-$(CONFIG_I2C_NPCM)		+= i2c-npcm7xx.o
 obj-$(CONFIG_I2C_OCORES)	+= i2c-ocores.o
 obj-$(CONFIG_I2C_OMAP)		+= i2c-omap.o
 obj-$(CONFIG_I2C_OWL)		+= i2c-owl.o
drivers/i2c/busses/i2c-npcm7xx.c +61 -26

196 lines not on this page · raw message

Signature · 1 line
2.17.1
#23 lore raw

Re: [PATCH v2 11/11] i2c: npcm: Support NPCM845

Quoted 81 lines · Tyrone Ting

From: Tyrone Ting <[email protected]>

Add NPCM8XX I2C support.
The NPCM8XX uses a similar i2c module as NPCM7XX. The internal HW FIFO is larger in NPCM8XX.

Signed-off-by Tyrone Ting <[email protected]>
Signed-off-by Tali Perry <[email protected]>
---
 drivers/i2c/busses/Kconfig       |  8 +--
 drivers/i2c/busses/Makefile      |  2 +-
 drivers/i2c/busses/i2c-npcm7xx.c | 87 ++++++++++++++++++++++----------
 3 files changed, 66 insertions(+), 31 deletions(-)
drivers/i2c/busses/Kconfig +4 -4
index 42da31c1ab70..ab9ee2de5e00 100644
@@ -817,13 +817,13 @@ config I2C_NOMADIK
 	  I2C interface from ST-Ericsson's Nomadik and Ux500 architectures,
 	  as well as the STA2X11 PCIe I/O HUB.
 
-config I2C_NPCM7XX
+config I2C_NPCM
 	tristate "Nuvoton I2C Controller"
-	depends on ARCH_NPCM7XX || COMPILE_TEST
+	depends on ARCH_NPCM || COMPILE_TEST
 	help
 	  If you say yes to this option, support will be included for the
-	  Nuvoton I2C controller, which is available on the NPCM7xx BMC
-	  controller.
+	  Nuvoton I2C controller, which is available on the NPCM BMC
+	  controllers.
 	  Driver can also support slave mode (select I2C_SLAVE).
 
 config I2C_OCORES
drivers/i2c/busses/Makefile +1 -1
index 1d00dce77098..01fdf74a5565 100644
@@ -80,7 +80,7 @@ obj-$(CONFIG_I2C_MT7621)	+= i2c-mt7621.o
 obj-$(CONFIG_I2C_MV64XXX)	+= i2c-mv64xxx.o
 obj-$(CONFIG_I2C_MXS)		+= i2c-mxs.o
 obj-$(CONFIG_I2C_NOMADIK)	+= i2c-nomadik.o
-obj-$(CONFIG_I2C_NPCM7XX)	+= i2c-npcm7xx.o
+obj-$(CONFIG_I2C_NPCM)		+= i2c-npcm7xx.o
 obj-$(CONFIG_I2C_OCORES)	+= i2c-ocores.o
 obj-$(CONFIG_I2C_OMAP)		+= i2c-omap.o
 obj-$(CONFIG_I2C_OWL)		+= i2c-owl.o
drivers/i2c/busses/i2c-npcm7xx.c +3 -3
index 2cbf9c679aed..b281e0424e3e 100644
@@ -17,6 +17,7 @@
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 

@@ -91,7 +92,7 @@ enum i2c_addr {
 
 /* init register and default value required to enable module */
 #define NPCM_I2CSEGCTL			0xE4
-#define NPCM_I2CSEGCTL_INIT_VAL		0x0333F000
+#define NPCM_I2CSEGCTL_INIT_VAL		bus->data->segctl_init_val
 
 /* Common regs */
 #define NPCM_I2CSDA			0x00

@@ -228,8 +229,7 @@ static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = {
 #define NPCM_I2CFIF_CTS_CLR_FIFO	BIT(6)
 #define NPCM_I2CFIF_CTS_SLVRSTR		BIT(7)
 
-/* NPCM_I2CTXF_CTL reg fields */
-#define NPCM_I2CTXF_CTL_TX_THR		GENMASK(4, 0)
+/* NPCM_I2CTXF_CTL reg field */
 #define NPCM_I2CTXF_CTL_THR_TXIE	BIT(6)
 
 /* NPCM_I2CT_OUT reg fields */

drivers/i2c/busses/i2c-npcm7xx.c static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = { · line 241

Quoted 5 lines
drivers/i2c/busses/i2c-npcm7xx.c +1 -1
@@ -238,22 +238,22 @@ static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = {
 #define NPCM_I2CT_OUT_T_OUTST		BIT(7)
 
 /* NPCM_I2CTXF_STS reg fields */
-#define NPCM_I2CTXF_STS_TX_BYTES	GENMASK(4, 0)
+#define NPCM_I2CTXF_STS_TX_BYTES	bus->data->txf_sts_tx_bytes

It's not a clean code to use defines for complex types. It's not a constant anymore, so just use bus->data->txf_sts_tx_bytes directly.

The same in other places.

drivers/i2c/busses/i2c-npcm7xx.c static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = { · line 243

Quoted 2 lines
drivers/i2c/busses/i2c-npcm7xx.c +0 -0
 #define NPCM_I2CTXF_STS_TX_THST		BIT(6)

Best regards,
Krzysztof

author
#24 lore raw

Re: [PATCH v2 11/11] i2c: npcm: Support NPCM845

Hi Krzysztof:

Thank you for your comments and they'll be addressed.

Krzysztof Kozlowski <[email protected]> 於 2022年2月20日 週日 下午5:36寫道：

Quoted 92 lines · Tyrone Ting
Quoted 81 lines

From: Tyrone Ting <[email protected]>

Add NPCM8XX I2C support.
The NPCM8XX uses a similar i2c module as NPCM7XX. The internal HW FIFO is larger in NPCM8XX.

Signed-off-by Tyrone Ting <[email protected]>
Signed-off-by Tali Perry <[email protected]>
---
 drivers/i2c/busses/Kconfig       |  8 +--
 drivers/i2c/busses/Makefile      |  2 +-
 drivers/i2c/busses/i2c-npcm7xx.c | 87 ++++++++++++++++++++++----------
 3 files changed, 66 insertions(+), 31 deletions(-)
drivers/i2c/busses/Kconfig +4 -4
index 42da31c1ab70..ab9ee2de5e00 100644
@@ -817,13 +817,13 @@ config I2C_NOMADIK
        I2C interface from ST-Ericsson's Nomadik and Ux500 architectures,
        as well as the STA2X11 PCIe I/O HUB.
 
-config I2C_NPCM7XX
+config I2C_NPCM
      tristate "Nuvoton I2C Controller"
-     depends on ARCH_NPCM7XX || COMPILE_TEST
+     depends on ARCH_NPCM || COMPILE_TEST
      help
        If you say yes to this option, support will be included for the
-       Nuvoton I2C controller, which is available on the NPCM7xx BMC
-       controller.
+       Nuvoton I2C controller, which is available on the NPCM BMC
+       controllers.
        Driver can also support slave mode (select I2C_SLAVE).
 
 config I2C_OCORES
drivers/i2c/busses/Makefile +1 -1
index 1d00dce77098..01fdf74a5565 100644
@@ -80,7 +80,7 @@ obj-$(CONFIG_I2C_MT7621)    += i2c-mt7621.o
 obj-$(CONFIG_I2C_MV64XXX)    += i2c-mv64xxx.o
 obj-$(CONFIG_I2C_MXS)                += i2c-mxs.o
 obj-$(CONFIG_I2C_NOMADIK)    += i2c-nomadik.o
-obj-$(CONFIG_I2C_NPCM7XX)    += i2c-npcm7xx.o
+obj-$(CONFIG_I2C_NPCM)               += i2c-npcm7xx.o
 obj-$(CONFIG_I2C_OCORES)     += i2c-ocores.o
 obj-$(CONFIG_I2C_OMAP)               += i2c-omap.o
 obj-$(CONFIG_I2C_OWL)                += i2c-owl.o
drivers/i2c/busses/i2c-npcm7xx.c +3 -3
index 2cbf9c679aed..b281e0424e3e 100644
@@ -17,6 +17,7 @@
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 

@@ -91,7 +92,7 @@ enum i2c_addr {
 
 /* init register and default value required to enable module */
 #define NPCM_I2CSEGCTL                       0xE4
-#define NPCM_I2CSEGCTL_INIT_VAL              0x0333F000
+#define NPCM_I2CSEGCTL_INIT_VAL              bus->data->segctl_init_val
 
 /* Common regs */
 #define NPCM_I2CSDA                  0x00

@@ -228,8 +229,7 @@ static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = {
 #define NPCM_I2CFIF_CTS_CLR_FIFO     BIT(6)
 #define NPCM_I2CFIF_CTS_SLVRSTR              BIT(7)
 
-/* NPCM_I2CTXF_CTL reg fields */
-#define NPCM_I2CTXF_CTL_TX_THR               GENMASK(4, 0)
+/* NPCM_I2CTXF_CTL reg field */
 #define NPCM_I2CTXF_CTL_THR_TXIE     BIT(6)
 
 /* NPCM_I2CT_OUT reg fields */

drivers/i2c/busses/i2c-npcm7xx.c static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = { · line 241

Quoted 5 lines
drivers/i2c/busses/i2c-npcm7xx.c +1 -1
@@ -238,22 +238,22 @@ static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = {
 #define NPCM_I2CT_OUT_T_OUTST                BIT(7)
 
 /* NPCM_I2CTXF_STS reg fields */
-#define NPCM_I2CTXF_STS_TX_BYTES     GENMASK(4, 0)
+#define NPCM_I2CTXF_STS_TX_BYTES     bus->data->txf_sts_tx_bytes

It's not a clean code to use defines for complex types. It's not a constant anymore, so just use bus->data->txf_sts_tx_bytes directly.

Quoted 7 lines

The same in other places.

drivers/i2c/busses/i2c-npcm7xx.c static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = { · line 243

Quoted 2 lines
drivers/i2c/busses/i2c-npcm7xx.c +0 -0
 #define NPCM_I2CTXF_STS_TX_THST              BIT(6)

Best regards,
Krzysztof

Best regards,
Tyrone

#25 lore raw
In reply to Tyrone Ting
Quoted 11 lines · Tyrone Ting

From: Tyrone Ting <[email protected]>

This patchset includes the following fixes:

- Add dt-bindings description for NPCM845.
- Bug fix for timeout calculation.
- Better handling of spurious interrupts.
- Fix for event type in slave mode.
- Removal of own slave addresses [2:10].
- Support for next gen BMC (NPCM845).

Quoted 5 lines

The NPCM I2C driver is tested on NPCM750 and NPCM845 evaluation boards.

Addressed comments from:
- Jonathan Neuschäfer : https://lkml.org/lkml/2022/2/7/670
- Krzysztof Kozlowski : https://lkml.org/lkml/2022/2/7/760

How did you address the ABI change comment? I still see you break the ABI with the introduction of a new, required property.

Best regards,
Krzysztof

author
#26 lore raw

Hi Krzysztof:

Thank you for your comments and please find my reply next to your comments.

Krzysztof Kozlowski <[email protected]> 於 2022年2月20日 週日 下午5:30寫道：

Quoted 13 lines · Tyrone Ting
Quoted 11 lines

From: Tyrone Ting <[email protected]>

This patchset includes the following fixes:

- Add dt-bindings description for NPCM845.
- Bug fix for timeout calculation.
- Better handling of spurious interrupts.
- Fix for event type in slave mode.
- Removal of own slave addresses [2:10].
- Support for next gen BMC (NPCM845).

Quoted 9 lines
Quoted 5 lines

The NPCM I2C driver is tested on NPCM750 and NPCM845 evaluation boards.

Addressed comments from:
- Jonathan Neuschäfer : https://lkml.org/lkml/2022/2/7/670
- Krzysztof Kozlowski : https://lkml.org/lkml/2022/2/7/760

How did you address the ABI change comment? I still see you break the ABI with the introduction of a new, required property.

I add the new, required property "nuvoton,sys-mgr" in the file nuvoton-common-npcm7xx.dtsi.
The file nuvoton-common-npcm7xx.dtsi is required by the existing upstream NPCM devicetree files.
It is also updated and committed in this patch set [PATCH v2 01/11]
arm: dts: add new property for NPCM i2c module.
Please let me know if I misunderstand the meaning of "breaking the ABI". Thank you again.

Quoted 3 lines

Best regards,
Krzysztof

Best regards,
Tyrone

#27 lore raw
Quoted 29 lines · Tyrone Ting

Hi Krzysztof:

Thank you for your comments and please find my reply next to your comments.

Krzysztof Kozlowski <[email protected]> 於 2022年2月20日 週日 下午5:30寫道：

Quoted 13 lines · Tyrone Ting
Quoted 11 lines

From: Tyrone Ting <[email protected]>

This patchset includes the following fixes:

- Add dt-bindings description for NPCM845.
- Bug fix for timeout calculation.
- Better handling of spurious interrupts.
- Fix for event type in slave mode.
- Removal of own slave addresses [2:10].
- Support for next gen BMC (NPCM845).

Quoted 9 lines
Quoted 5 lines

The NPCM I2C driver is tested on NPCM750 and NPCM845 evaluation boards.

Addressed comments from:
- Jonathan Neuschäfer : https://lkml.org/lkml/2022/2/7/670
- Krzysztof Kozlowski : https://lkml.org/lkml/2022/2/7/760

How did you address the ABI change comment? I still see you break the ABI with the introduction of a new, required property.

Quoted 8 lines

I add the new, required property "nuvoton,sys-mgr" in the file nuvoton-common-npcm7xx.dtsi.
The file nuvoton-common-npcm7xx.dtsi is required by the existing upstream NPCM devicetree files.
It is also updated and committed in this patch set [PATCH v2 01/11]
arm: dts: add new property for NPCM i2c module.
Please let me know if I misunderstand the meaning of "breaking the ABI". Thank you again.

Breaking the ABI means that old DTS stop working with new kernel. Your change breaks old (and out-of-tree) DTS.

What is more, your change is not bisectable because DTS goes via separate branch or tree than driver change.

You need to keep old code as fallback, if getting nuvoton,sys-mgr fails.

Best regards,
Krzysztof

author
#28 lore raw

Hi Krzysztof:

Got it and thank you for your comments.

I'll keep old code as fallback, if getting nuvoton,sys-mgr fails as you point out.

Krzysztof Kozlowski <[email protected]> 於 2022年2月21日 週一 下午4:32寫道：

Quoted 43 lines · Tyrone Ting
Quoted 29 lines

Hi Krzysztof:

Thank you for your comments and please find my reply next to your comments.

Krzysztof Kozlowski <[email protected]> 於 2022年2月20日 週日 下午5:30寫道：

Quoted 13 lines · Tyrone Ting
Quoted 11 lines

From: Tyrone Ting <[email protected]>

This patchset includes the following fixes:

- Add dt-bindings description for NPCM845.
- Bug fix for timeout calculation.
- Better handling of spurious interrupts.
- Fix for event type in slave mode.
- Removal of own slave addresses [2:10].
- Support for next gen BMC (NPCM845).

Quoted 9 lines
Quoted 5 lines

The NPCM I2C driver is tested on NPCM750 and NPCM845 evaluation boards.

Addressed comments from:
- Jonathan Neuschäfer : https://lkml.org/lkml/2022/2/7/670
- Krzysztof Kozlowski : https://lkml.org/lkml/2022/2/7/760

How did you address the ABI change comment? I still see you break the ABI with the introduction of a new, required property.

Quoted 8 lines

I add the new, required property "nuvoton,sys-mgr" in the file nuvoton-common-npcm7xx.dtsi.
The file nuvoton-common-npcm7xx.dtsi is required by the existing upstream NPCM devicetree files.
It is also updated and committed in this patch set [PATCH v2 01/11]
arm: dts: add new property for NPCM i2c module.
Please let me know if I misunderstand the meaning of "breaking the ABI". Thank you again.

Breaking the ABI means that old DTS stop working with new kernel. Your change breaks old (and out-of-tree) DTS.

Quoted 7 lines

What is more, your change is not bisectable because DTS goes via separate branch or tree than driver change.

You need to keep old code as fallback, if getting nuvoton,sys-mgr fails.

Best regards,
Krzysztof

Best regards,
Tyrone

#29 lore raw
Quoted 3 lines

I'll keep old code as fallback, if getting nuvoton,sys-mgr fails as you point out.

Yeah, fallback is much needed. And if you implement it, then you can also split the series into two. One for the DTS changes and one for the I2C changes. That would make upstreaming a lot easier.

author
#30 lore raw

Hi Wolfram:

Thank you for your comment and it'll be addressed.

Wolfram Sang <[email protected]> 於 2022年3月2日 週三 上午3:45寫道：

Quoted 9 lines
Quoted 4 lines

I'll keep old code as fallback, if getting nuvoton,sys-mgr fails as you point out.

Yeah, fallback is much needed. And if you implement it, then you can also split the series into two. One for the DTS changes and one for the I2C changes. That would make upstreaming a lot easier.

Regards,
Tyrone