Skip to contentSkip to conversation

i2c: designware: Code duplication removal and cleanups

PATCH Applied Jarkko Nikula 3 participants 17 messages Aug 31, 2015 – Oct 21, 2015 1 message not in the archive

Subsystems (3)
I2C subsystem host drivers Intel LPSS (Low Power SubSystem) drivers Synopsys DesignWare I2C driver
lore
Share & tools
lore

Jarkko Nikula

author
#0 lore raw

cover i2c: designware: Code duplication removal and cleanups

It bothered to me to see "static struct i2c_algorithm i2c_dw_algo {}" defined twice both in i2c-designware-pcidrv.c and i2c-designware-platdrv.c and so many exported i2c-designware-core functions.

It turned out some of them became unused or are local and there were also duplicated probe code that I moved to new common i2c_dw_probe().

Object sizes below before and after this set from CONFIG_X86_64=y build.

   text	   data	    bss	    dec	    hex	filename
   6439	   1096	      0	   7535	   1d6f	drivers/i2c/busses/i2c-designware-core.ko
   5123	   1588	     16	   6727	   1a47	drivers/i2c/busses/i2c-designware-pci.ko
   5274	   1096	     16	   6386	   18f2	drivers/i2c/busses/i2c-designware-platform.ko
  16836	   3780	     32	  20648	   50a8	(TOTALS)
   text	   data	    bss	    dec	    hex	filename
   7225	   1120	     16	   8361	   20a9	drivers/i2c/busses/i2c-designware-core.ko
   4281	   1524	      0	   5805	   16ad	drivers/i2c/busses/i2c-designware-pci.ko
   4337	   1072	      0	   5409	   1521	drivers/i2c/busses/i2c-designware-platform.ko
  15843	   3716	     16	  19575	   4c77	(TOTALS)

Jarkko Nikula (6):
i2c: designware: Remove interrupt clearing from i2c_dw_pci_probe()
i2c: designware: Disable interrupts before requesting PCI device
interrupt
i2c: designware: Remove unused functions
i2c: designware: Make dw_readl() and dw_writel() static
i2c: designware: Rename platform driver probe and PM functions
i2c: designware: Move common probe code into i2c_dw_probe()

 drivers/i2c/busses/i2c-designware-core.c    | 73 ++++++++++++++++++-----------
 drivers/i2c/busses/i2c-designware-core.h    | 10 +---
 drivers/i2c/busses/i2c-designware-pcidrv.c  | 31 ++----------
 drivers/i2c/busses/i2c-designware-platdrv.c | 52 ++++++--------------
 4 files changed, 63 insertions(+), 103 deletions(-)
Signature · 1 line
2.5.0

A message that is not in the archive (sent off-list or not yet received): 1441031493-18938-1-git-send-email-jarkko.nikula-VuQAYsv1563Yd54FQh9/[email protected]

author
#2 lore raw

PATCH 1/6 i2c: designware: Remove interrupt clearing from i2c_dw_pci_probe()

There is no need to clear interrupts in i2c_dw_pci_probe() since only place where interrupts are unmasked is i2c_dw_xfer_init() and there interrupts are always cleared after commit 2a2d95e9d6d2 ("i2c: designware: always clear interrupts before enabling them").

This allows to cleanup the code and replace i2c_dw_clear_int() in i2c_dw_xfer_init() by direct register read as there are no other callers.

Signed-off-by Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/[email protected]>
---
 drivers/i2c/busses/i2c-designware-core.c   | 8 +-------
 drivers/i2c/busses/i2c-designware-core.h   | 1 -
 drivers/i2c/busses/i2c-designware-pcidrv.c | 1 -
 3 files changed, 1 insertion(+), 9 deletions(-)
drivers/i2c/busses/i2c-designware-core.c +1 -7
index 7441cdc1b34a..dec0af7d0471 100644
@@ -438,7 +438,7 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
 	__i2c_dw_enable(dev, true);
 
 	/* Clear and enable interrupts */
-	i2c_dw_clear_int(dev);
+	dw_readl(dev, DW_IC_CLR_INTR);
 	dw_writel(dev, DW_IC_INTR_DEFAULT_MASK, DW_IC_INTR_MASK);
 }
 

@@ -839,12 +839,6 @@ void i2c_dw_disable(struct dw_i2c_dev *dev)
 }
 EXPORT_SYMBOL_GPL(i2c_dw_disable);
 
-void i2c_dw_clear_int(struct dw_i2c_dev *dev)
-{
-	dw_readl(dev, DW_IC_CLR_INTR);
-}
-EXPORT_SYMBOL_GPL(i2c_dw_clear_int);
-
 void i2c_dw_disable_int(struct dw_i2c_dev *dev)
 {
 	dw_writel(dev, 0, DW_IC_INTR_MASK);
drivers/i2c/busses/i2c-designware-core.h +0 -1
index 9630222abf32..f44aeeeb91c6 100644
@@ -122,7 +122,6 @@ extern irqreturn_t i2c_dw_isr(int this_irq, void *dev_id);
 extern void i2c_dw_enable(struct dw_i2c_dev *dev);
 extern u32 i2c_dw_is_enabled(struct dw_i2c_dev *dev);
 extern void i2c_dw_disable(struct dw_i2c_dev *dev);
-extern void i2c_dw_clear_int(struct dw_i2c_dev *dev);
 extern void i2c_dw_disable_int(struct dw_i2c_dev *dev);
 extern u32 i2c_dw_read_comp_param(struct dw_i2c_dev *dev);
drivers/i2c/busses/i2c-designware-pcidrv.c +0 -1
index df23e8c30e6f..e477dddcdae5 100644
@@ -268,7 +268,6 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 	}
 
 	i2c_dw_disable_int(dev);
-	i2c_dw_clear_int(dev);
 	r = i2c_add_numbered_adapter(adap);
 	if (r) {
 		dev_err(&pdev->dev, "failure adding adapter\n");
Signature · 1 line
2.5.0
#3 lore raw

Re: [PATCH 1/6] i2c: designware: Remove interrupt clearing from i2c_dw_pci_probe()

Quoted 4 lines · Jarkko Nikula

There is no need to clear interrupts in i2c_dw_pci_probe() since only place where interrupts are unmasked is i2c_dw_xfer_init() and there interrupts are always cleared after commit 2a2d95e9d6d2 ("i2c: designware: always clear interrupts before enabling them").

Still, their status might be unclear when we init because firmware might have used it before. It should be easy to reintroduce this in patch 6, or?

#4 lore raw

Re: [PATCH 1/6] i2c: designware: Remove interrupt clearing from i2c_dw_pci_probe()

Quoted 10 lines · Wolfram Sang
Quoted 4 lines · Jarkko Nikula

There is no need to clear interrupts in i2c_dw_pci_probe() since only place where interrupts are unmasked is i2c_dw_xfer_init() and there interrupts are always cleared after commit 2a2d95e9d6d2 ("i2c: designware: always clear interrupts before enabling them").

Still, their status might be unclear when we init because firmware might have used it before. It should be easy to reintroduce this in patch 6, or?

Yes and then it would be also uniform across different enumeration methods if we ever need to add it. Now this clearing was done only for PCI devices. Although I don't think there is any need to add clearing to probe path as clearing happens beginning of transmit before enabling the interrupts.

Signature · 1 line
Jarkko
#5 lore raw

Re: [PATCH 1/6] i2c: designware: Remove interrupt clearing from i2c_dw_pci_probe()

Quoted 3 lines

Although I don't think there is any need to add clearing to probe path as clearing happens beginning of transmit before enabling the interrupts.

Well, in case the firmware left interrupts dangling, they would still be dangling unless someone starts some communication. Chances are small, but better safe than sorry? Could be an incremental patch on top of your V2 if you are up to it.

Thanks,

Wolfram

author
#6 lore raw

PATCH 2/6 i2c: designware: Disable interrupts before requesting PCI device interrupt

In reply to a message not in the archive

Device must not generate interrupts before registering the interrupt handler so move i2c_dw_disable_int() before requesting it.

There are no known issues with this. The code has been here since commit fe20ff5c7e9c ("i2c-designware: Add support for Designware core behind PCI devices.").

Signed-off-by Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/[email protected]>
---
 drivers/i2c/busses/i2c-designware-pcidrv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/i2c/busses/i2c-designware-pcidrv.c +1 -1
index e477dddcdae5..d9e8937f062f 100644
@@ -260,6 +260,7 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 
 	snprintf(adap->name, sizeof(adap->name), "i2c-designware-pci");
 
+	i2c_dw_disable_int(dev);
 	r = devm_request_irq(&pdev->dev, pdev->irq, i2c_dw_isr,
 			IRQF_SHARED | IRQF_COND_SUSPEND, adap->name, dev);
 	if (r) {

@@ -267,7 +268,6 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 		return r;
 	}
 
-	i2c_dw_disable_int(dev);
 	r = i2c_add_numbered_adapter(adap);
 	if (r) {
 		dev_err(&pdev->dev, "failure adding adapter\n");
Signature · 1 line
2.5.0
author
#7 lore raw

PATCH 3/6 i2c: designware: Remove unused functions

In reply to a message not in the archive

i2c_dw_is_enabled() became unused by the commit be58eda775c8 ("i2c: designware-pci: Cleanup driver power management") and i2c_dw_enable() by the commit 3a48d1c08fe0 ("i2c: prevent spurious interrupt on Designware controllers").

Signed-off-by Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/[email protected]>
---
 drivers/i2c/busses/i2c-designware-core.c | 13 -------------
 drivers/i2c/busses/i2c-designware-core.h |  2 --
 2 files changed, 15 deletions(-)
drivers/i2c/busses/i2c-designware-core.c +0 -13
index dec0af7d0471..c9e93a76a455 100644
@@ -815,19 +815,6 @@ tx_aborted:
 }
 EXPORT_SYMBOL_GPL(i2c_dw_isr);
 
-void i2c_dw_enable(struct dw_i2c_dev *dev)
-{
-       /* Enable the adapter */
-	__i2c_dw_enable(dev, true);
-}
-EXPORT_SYMBOL_GPL(i2c_dw_enable);
-
-u32 i2c_dw_is_enabled(struct dw_i2c_dev *dev)
-{
-	return dw_readl(dev, DW_IC_ENABLE);
-}
-EXPORT_SYMBOL_GPL(i2c_dw_is_enabled);
-
 void i2c_dw_disable(struct dw_i2c_dev *dev)
 {
 	/* Disable controller */
drivers/i2c/busses/i2c-designware-core.h +0 -2
index f44aeeeb91c6..10c79038834d 100644
@@ -119,8 +119,6 @@ extern int i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
 		int num);
 extern u32 i2c_dw_func(struct i2c_adapter *adap);
 extern irqreturn_t i2c_dw_isr(int this_irq, void *dev_id);
-extern void i2c_dw_enable(struct dw_i2c_dev *dev);
-extern u32 i2c_dw_is_enabled(struct dw_i2c_dev *dev);
 extern void i2c_dw_disable(struct dw_i2c_dev *dev);
 extern void i2c_dw_disable_int(struct dw_i2c_dev *dev);
 extern u32 i2c_dw_read_comp_param(struct dw_i2c_dev *dev);
Signature · 1 line
2.5.0
author
#8 lore raw

PATCH 4/6 i2c: designware: Make dw_readl() and dw_writel() static

In reply to a message not in the archive

dw_readl() and dw_writel() are not used outside of i2c-designware-core and they are not exported so make them static and remove their forward declaration.

Signed-off-by Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/[email protected]>
---
 drivers/i2c/busses/i2c-designware-core.c | 4 ++--
 drivers/i2c/busses/i2c-designware-core.h | 2 --
 2 files changed, 2 insertions(+), 4 deletions(-)
drivers/i2c/busses/i2c-designware-core.c +2 -2
index c9e93a76a455..52272a01c679 100644
@@ -165,7 +165,7 @@ static char *abort_sources[] = {
 		"lost arbitration",
 };
 
-u32 dw_readl(struct dw_i2c_dev *dev, int offset)
+static u32 dw_readl(struct dw_i2c_dev *dev, int offset)
 {
 	u32 value;
 

@@ -181,7 +181,7 @@ u32 dw_readl(struct dw_i2c_dev *dev, int offset)
 		return value;
 }
 
-void dw_writel(struct dw_i2c_dev *dev, u32 b, int offset)
+static void dw_writel(struct dw_i2c_dev *dev, u32 b, int offset)
 {
 	if (dev->accessor_flags & ACCESS_SWAP)
 		b = swab32(b);
drivers/i2c/busses/i2c-designware-core.h +0 -2
index 10c79038834d..0e73b8672402 100644
@@ -112,8 +112,6 @@ struct dw_i2c_dev {
 #define ACCESS_SWAP		0x00000001
 #define ACCESS_16BIT		0x00000002
 
-extern u32 dw_readl(struct dw_i2c_dev *dev, int offset);
-extern void dw_writel(struct dw_i2c_dev *dev, u32 b, int offset);
 extern int i2c_dw_init(struct dw_i2c_dev *dev);
 extern int i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
 		int num);
Signature · 1 line
2.5.0
author
#9 lore raw

PATCH 5/6 i2c: designware: Rename platform driver probe and PM functions

In reply to a message not in the archive

Make it easier to distinguish between i2c-designware-platdrv and i2c-designware-core functions and to be consistent with i2c-designware-pcidrv.

Signed-off-by Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/[email protected]>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)
drivers/i2c/busses/i2c-designware-platdrv.c +12 -12
index 3dd2de31a2f8..17167cd4812d 100644
@@ -133,7 +133,7 @@ static inline int dw_i2c_acpi_configure(struct platform_device *pdev)
 static inline void dw_i2c_acpi_unconfigure(struct platform_device *pdev) { }
 #endif
 
-static int dw_i2c_probe(struct platform_device *pdev)
+static int dw_i2c_plat_probe(struct platform_device *pdev)
 {
 	struct dw_i2c_dev *dev;
 	struct i2c_adapter *adap;

@@ -271,7 +271,7 @@ static int dw_i2c_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static int dw_i2c_remove(struct platform_device *pdev)
+static int dw_i2c_plat_remove(struct platform_device *pdev)
 {
 	struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
 

@@ -300,12 +300,12 @@ MODULE_DEVICE_TABLE(of, dw_i2c_of_match);
 #endif
 
 #ifdef CONFIG_PM_SLEEP
-static int dw_i2c_prepare(struct device *dev)
+static int dw_i2c_plat_prepare(struct device *dev)
 {
 	return pm_runtime_suspended(dev);
 }
 
-static void dw_i2c_complete(struct device *dev)
+static void dw_i2c_plat_complete(struct device *dev)
 {
 	if (dev->power.direct_complete)
 		pm_request_resume(dev);

@@ -316,7 +316,7 @@ static void dw_i2c_complete(struct device *dev)
 #endif
 
 #ifdef CONFIG_PM
-static int dw_i2c_suspend(struct device *dev)
+static int dw_i2c_plat_suspend(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);

@@ -327,7 +327,7 @@ static int dw_i2c_suspend(struct device *dev)
 	return 0;
 }
 
-static int dw_i2c_resume(struct device *dev)
+static int dw_i2c_plat_resume(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);

@@ -341,10 +341,10 @@ static int dw_i2c_resume(struct device *dev)
 }
 
 static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
-	.prepare = dw_i2c_prepare,
-	.complete = dw_i2c_complete,
-	SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_suspend, dw_i2c_resume)
-	SET_RUNTIME_PM_OPS(dw_i2c_suspend, dw_i2c_resume, NULL)
+	.prepare = dw_i2c_plat_prepare,
+	.complete = dw_i2c_plat_complete,
+	SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
+	SET_RUNTIME_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume, NULL)
 };
 
 #define DW_I2C_DEV_PMOPS (&dw_i2c_dev_pm_ops)

@@ -356,8 +356,8 @@ static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
 MODULE_ALIAS("platform:i2c_designware");
 
 static struct platform_driver dw_i2c_driver = {
-	.probe = dw_i2c_probe,
-	.remove = dw_i2c_remove,
+	.probe = dw_i2c_plat_probe,
+	.remove = dw_i2c_plat_remove,
 	.driver		= {
 		.name	= "i2c_designware",
 		.of_match_table = of_match_ptr(dw_i2c_of_match),
Signature · 1 line
2.5.0
author
#10 lore raw

PATCH 6/6 i2c: designware: Move common probe code into i2c_dw_probe()

In reply to a message not in the archive

There is some code duplication in i2c-designware-platdrv and i2c-designware-pcidrv probe functions. What is even worse that duplication requires i2c_dw_xfer(), i2c_dw_func() and i2c_dw_isr() i2c-designware-core functions to be exported.

Therefore move common code into new i2c_dw_probe() and make functions above local to i2c-designware-core.

While merging the code patch does following functional changes:

- I2C Adapter name will be "i2c-designware". Previously adapter name was "Synopsys DesignWare I2C adapter" for platform and ACPI devices and "i2c-designware-pci" for PCI devices.
- Interrupt name too will be "i2c-designware". Previous it was platform device name, ACPI device name or "i2c-designware-pci".
- Error code from devm_request_irq() and i2c_add_numbered_adapter() will be printed in case of error.

Signed-off-by Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/[email protected]>
---
 drivers/i2c/busses/i2c-designware-core.c    | 48 +++++++++++++++++++++++++----
 drivers/i2c/busses/i2c-designware-core.h    |  5 +--
 drivers/i2c/busses/i2c-designware-pcidrv.c  | 30 ++----------------
 drivers/i2c/busses/i2c-designware-platdrv.c | 28 ++---------------
 4 files changed, 48 insertions(+), 63 deletions(-)
drivers/i2c/busses/i2c-designware-core.c +42 -6
index 52272a01c679..501c8ac0cf14 100644
@@ -618,7 +618,7 @@ static int i2c_dw_handle_tx_abort(struct dw_i2c_dev *dev)
 /*
  * Prepare controller for a transaction and call i2c_dw_xfer_msg
  */
-int
+static int
 i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 {
 	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);

@@ -702,14 +702,17 @@ done_nolock:
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(i2c_dw_xfer);
 
-u32 i2c_dw_func(struct i2c_adapter *adap)
+static u32 i2c_dw_func(struct i2c_adapter *adap)
 {
 	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
 	return dev->functionality;
 }
-EXPORT_SYMBOL_GPL(i2c_dw_func);
+
+static struct i2c_algorithm i2c_dw_algo = {
+	.master_xfer	= i2c_dw_xfer,
+	.functionality	= i2c_dw_func,
+};
 
 static u32 i2c_dw_read_clear_intrbits(struct dw_i2c_dev *dev)
 {

@@ -770,7 +773,7 @@ static u32 i2c_dw_read_clear_intrbits(struct dw_i2c_dev *dev)
  * Interrupt service routine. This gets called whenever an I2C interrupt
  * occurs.
  */
-irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
+static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
 {
 	struct dw_i2c_dev *dev = dev_id;
 	u32 stat, enabled;

@@ -813,7 +816,6 @@ tx_aborted:
 
 	return IRQ_HANDLED;
 }
-EXPORT_SYMBOL_GPL(i2c_dw_isr);
 
 void i2c_dw_disable(struct dw_i2c_dev *dev)
 {

@@ -838,5 +840,39 @@ u32 i2c_dw_read_comp_param(struct dw_i2c_dev *dev)
 }
 EXPORT_SYMBOL_GPL(i2c_dw_read_comp_param);
 
+int i2c_dw_probe(struct dw_i2c_dev *dev)
+{
+	struct i2c_adapter *adap = &dev->adapter;
+	int r;
+
+	init_completion(&dev->cmd_complete);
+	mutex_init(&dev->lock);
+
+	r = i2c_dw_init(dev);
+	if (r)
+		return r;
+
+	snprintf(adap->name, sizeof(adap->name), "i2c-designware");
+	adap->algo = &i2c_dw_algo;
+	adap->dev.parent = dev->dev;
+	i2c_set_adapdata(adap, dev);
+
+	i2c_dw_disable_int(dev);
+	r = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr, IRQF_SHARED,
+			     adap->name, dev);
+	if (r) {
+		dev_err(dev->dev, "failure requesting irq %i: %d\n",
+			dev->irq, r);
+		return r;
+	}
+
+	r = i2c_add_numbered_adapter(adap);
+	if (r)
+		dev_err(dev->dev, "failure adding adapter: %d\n", r);
+
+	return r;
+}
+EXPORT_SYMBOL_GPL(i2c_dw_probe);
+
 MODULE_DESCRIPTION("Synopsys DesignWare I2C bus adapter core");
 MODULE_LICENSE("GPL");
drivers/i2c/busses/i2c-designware-core.h +1 -4
index 0e73b8672402..1d50898e7b24 100644
@@ -113,13 +113,10 @@ struct dw_i2c_dev {
 #define ACCESS_16BIT		0x00000002
 
 extern int i2c_dw_init(struct dw_i2c_dev *dev);
-extern int i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
-		int num);
-extern u32 i2c_dw_func(struct i2c_adapter *adap);
-extern irqreturn_t i2c_dw_isr(int this_irq, void *dev_id);
 extern void i2c_dw_disable(struct dw_i2c_dev *dev);
 extern void i2c_dw_disable_int(struct dw_i2c_dev *dev);
 extern u32 i2c_dw_read_comp_param(struct dw_i2c_dev *dev);
+extern int i2c_dw_probe(struct dw_i2c_dev *dev);
 
 #if IS_ENABLED(CONFIG_I2C_DESIGNWARE_BAYTRAIL)
 extern int i2c_dw_eval_lock_support(struct dw_i2c_dev *dev);
drivers/i2c/busses/i2c-designware-pcidrv.c +3 -27
index d9e8937f062f..169be1e26241 100644
@@ -158,11 +158,6 @@ static struct dw_pci_controller dw_pci_controllers[] = {
 	},
 };
 
-static struct i2c_algorithm i2c_dw_algo = {
-	.master_xfer	= i2c_dw_xfer,
-	.functionality	= i2c_dw_func,
-};
-
 #ifdef CONFIG_PM
 static int i2c_dw_pci_suspend(struct device *dev)
 {

@@ -222,13 +217,12 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 	if (!dev)
 		return -ENOMEM;
 
-	init_completion(&dev->cmd_complete);
-	mutex_init(&dev->lock);
 	dev->clk = NULL;
 	dev->controller = controller;
 	dev->get_clk_rate_khz = i2c_dw_get_clk_rate_khz;
 	dev->base = pcim_iomap_table(pdev)[0];
 	dev->dev = &pdev->dev;
+	dev->irq = pdev->irq;
 	dev->functionality = controller->functionality |
 				DW_DEFAULT_FUNCTIONALITY;
 

@@ -246,33 +240,15 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 
 	dev->tx_fifo_depth = controller->tx_fifo_depth;
 	dev->rx_fifo_depth = controller->rx_fifo_depth;
-	r = i2c_dw_init(dev);
-	if (r)
-		return r;
 
 	adap = &dev->adapter;
-	i2c_set_adapdata(adap, dev);
 	adap->owner = THIS_MODULE;
 	adap->class = 0;
-	adap->algo = &i2c_dw_algo;
-	adap->dev.parent = &pdev->dev;
 	adap->nr = controller->bus_num;
 
-	snprintf(adap->name, sizeof(adap->name), "i2c-designware-pci");
-
-	i2c_dw_disable_int(dev);
-	r = devm_request_irq(&pdev->dev, pdev->irq, i2c_dw_isr,
-			IRQF_SHARED | IRQF_COND_SUSPEND, adap->name, dev);
-	if (r) {
-		dev_err(&pdev->dev, "failure requesting irq %i\n", dev->irq);
-		return r;
-	}
-
-	r = i2c_add_numbered_adapter(adap);
-	if (r) {
-		dev_err(&pdev->dev, "failure adding adapter\n");
+	r = i2c_dw_probe(dev);
+	if (r)
 		return r;
-	}
 
 	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
 	pm_runtime_use_autosuspend(&pdev->dev);
drivers/i2c/busses/i2c-designware-platdrv.c +2 -26
index 17167cd4812d..adcf4c3a81f0 100644
@@ -41,10 +41,6 @@
 #include <linux/platform_data/i2c-designware.h>
 #include "i2c-designware-core.h"
 
-static struct i2c_algorithm i2c_dw_algo = {
-	.master_xfer	= i2c_dw_xfer,
-	.functionality	= i2c_dw_func,
-};
 static u32 i2c_dw_get_clk_rate_khz(struct dw_i2c_dev *dev)
 {
 	return clk_get_rate(dev->clk)/1000;

@@ -155,8 +151,6 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 	if (IS_ERR(dev->base))
 		return PTR_ERR(dev->base);
 
-	init_completion(&dev->cmd_complete);
-	mutex_init(&dev->lock);
 	dev->dev = &pdev->dev;
 	dev->irq = irq;
 	platform_set_drvdata(pdev, dev);

@@ -231,33 +225,15 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 		dev->rx_fifo_depth = ((param1 >> 8)  & 0xff) + 1;
 		dev->adapter.nr = pdev->id;
 	}
-	r = i2c_dw_init(dev);
-	if (r)
-		return r;
-
-	i2c_dw_disable_int(dev);
-	r = devm_request_irq(&pdev->dev, dev->irq, i2c_dw_isr, IRQF_SHARED,
-			pdev->name, dev);
-	if (r) {
-		dev_err(&pdev->dev, "failure requesting irq %i\n", dev->irq);
-		return r;
-	}
 
 	adap = &dev->adapter;
-	i2c_set_adapdata(adap, dev);
 	adap->owner = THIS_MODULE;
 	adap->class = I2C_CLASS_DEPRECATED;
-	strlcpy(adap->name, "Synopsys DesignWare I2C adapter",
-			sizeof(adap->name));
-	adap->algo = &i2c_dw_algo;
-	adap->dev.parent = &pdev->dev;
 	adap->dev.of_node = pdev->dev.of_node;
 
-	r = i2c_add_numbered_adapter(adap);
-	if (r) {
-		dev_err(&pdev->dev, "failure adding adapter\n");
+	r = i2c_dw_probe(dev);
+	if (r)
 		return r;
-	}
 
 	if (dev->pm_runtime_disabled) {
 		pm_runtime_forbid(&pdev->dev);
Signature · 1 line
2.5.0
#11 lore raw

Re: [PATCH 6/6] i2c: designware: Move common probe code into i2c_dw_probe()

Quoted 4 lines

- I2C Adapter name will be "i2c-designware". Previously adapter name was "Synopsys DesignWare I2C adapter" for platform and ACPI devices and "i2c-designware-pci" for PCI devices.

I have a small tendency to make it "Synopsys DesignWare I2C adapter" for all cases. Not much, though.

Quoted 2 lines

- Interrupt name too will be "i2c-designware". Previous it was platform device name, ACPI device name or "i2c-designware-pci".

The device name makes them easier to distinguish in case you have multiple instantiations.

Rest of the series looks good to me, thanks!

#12 lore raw

Re: [PATCH 6/6] i2c: designware: Move common probe code into i2c_dw_probe()

Quoted 8 lines · Wolfram Sang
Quoted 4 lines

- I2C Adapter name will be "i2c-designware". Previously adapter name was "Synopsys DesignWare I2C adapter" for platform and ACPI devices and "i2c-designware-pci" for PCI devices.

I have a small tendency to make it "Synopsys DesignWare I2C adapter" for all cases. Not much, though.

Ok, will change.

Quoted 6 lines
Quoted 2 lines

- Interrupt name too will be "i2c-designware". Previous it was platform device name, ACPI device name or "i2c-designware-pci".

The device name makes them easier to distinguish in case you have multiple instantiations.

Good point. Bug reports definitely will benefit from it given that we have platforms where there are differences between i2c-designware instances. Same platform can have different instances enumerated from PCI or ACPI and those are registered as platform devices in drivers/mfd/intel-lpss.c.

Signature · 1 line
Jarkko
#13 lore raw

PATCH v2 6/6 i2c: designware: Move common probe code into i2c_dw_probe()

In reply to Wolfram Sang

There is some code duplication in i2c-designware-platdrv and i2c-designware-pcidrv probe functions. What is even worse that duplication requires i2c_dw_xfer(), i2c_dw_func() and i2c_dw_isr() i2c-designware-core functions to be exported.

Therefore move common code into new i2c_dw_probe() and make functions above local to i2c-designware-core.

While merging the code patch does following functional changes:

- I2C Adapter name will be "Synopsys DesignWare I2C adapter". Previously it was used for platform and ACPI devices but PCI device used "i2c-designware-pci".
- Using device name for interrupt name. Previous it was platform device name, ACPI device name or "i2c-designware-pci".
- Error code from devm_request_irq() and i2c_add_numbered_adapter() will be printed in case of error.

Signed-off-by Jarkko Nikula <[email protected]>

---
v2:
- Adapter name changed to "Synopsys DesignWare I2C adapter"
- Using device name for interrupt name

---
 drivers/i2c/busses/i2c-designware-core.c    | 49 +++++++++++++++++++++++++----
 drivers/i2c/busses/i2c-designware-core.h    |  5 +--
 drivers/i2c/busses/i2c-designware-pcidrv.c  | 30 ++----------------
 drivers/i2c/busses/i2c-designware-platdrv.c | 28 ++---------------
 4 files changed, 49 insertions(+), 63 deletions(-)
drivers/i2c/busses/i2c-designware-core.c +43 -6
index 52272a01c679..8c48b27ba059 100644
@@ -618,7 +618,7 @@ static int i2c_dw_handle_tx_abort(struct dw_i2c_dev *dev)
 /*
  * Prepare controller for a transaction and call i2c_dw_xfer_msg
  */
-int
+static int
 i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 {
 	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);

@@ -702,14 +702,17 @@ done_nolock:
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(i2c_dw_xfer);
 
-u32 i2c_dw_func(struct i2c_adapter *adap)
+static u32 i2c_dw_func(struct i2c_adapter *adap)
 {
 	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
 	return dev->functionality;
 }
-EXPORT_SYMBOL_GPL(i2c_dw_func);
+
+static struct i2c_algorithm i2c_dw_algo = {
+	.master_xfer	= i2c_dw_xfer,
+	.functionality	= i2c_dw_func,
+};
 
 static u32 i2c_dw_read_clear_intrbits(struct dw_i2c_dev *dev)
 {

@@ -770,7 +773,7 @@ static u32 i2c_dw_read_clear_intrbits(struct dw_i2c_dev *dev)
  * Interrupt service routine. This gets called whenever an I2C interrupt
  * occurs.
  */
-irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
+static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
 {
 	struct dw_i2c_dev *dev = dev_id;
 	u32 stat, enabled;

@@ -813,7 +816,6 @@ tx_aborted:
 
 	return IRQ_HANDLED;
 }
-EXPORT_SYMBOL_GPL(i2c_dw_isr);
 
 void i2c_dw_disable(struct dw_i2c_dev *dev)
 {

@@ -838,5 +840,40 @@ u32 i2c_dw_read_comp_param(struct dw_i2c_dev *dev)
 }
 EXPORT_SYMBOL_GPL(i2c_dw_read_comp_param);
 
+int i2c_dw_probe(struct dw_i2c_dev *dev)
+{
+	struct i2c_adapter *adap = &dev->adapter;
+	int r;
+
+	init_completion(&dev->cmd_complete);
+	mutex_init(&dev->lock);
+
+	r = i2c_dw_init(dev);
+	if (r)
+		return r;
+
+	snprintf(adap->name, sizeof(adap->name),
+		 "Synopsys DesignWare I2C adapter");
+	adap->algo = &i2c_dw_algo;
+	adap->dev.parent = dev->dev;
+	i2c_set_adapdata(adap, dev);
+
+	i2c_dw_disable_int(dev);
+	r = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr, IRQF_SHARED,
+			     dev_name(dev->dev), dev);
+	if (r) {
+		dev_err(dev->dev, "failure requesting irq %i: %d\n",
+			dev->irq, r);
+		return r;
+	}
+
+	r = i2c_add_numbered_adapter(adap);
+	if (r)
+		dev_err(dev->dev, "failure adding adapter: %d\n", r);
+
+	return r;
+}
+EXPORT_SYMBOL_GPL(i2c_dw_probe);
+
 MODULE_DESCRIPTION("Synopsys DesignWare I2C bus adapter core");
 MODULE_LICENSE("GPL");
drivers/i2c/busses/i2c-designware-core.h +1 -4
index 0e73b8672402..1d50898e7b24 100644
@@ -113,13 +113,10 @@ struct dw_i2c_dev {
 #define ACCESS_16BIT		0x00000002
 
 extern int i2c_dw_init(struct dw_i2c_dev *dev);
-extern int i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
-		int num);
-extern u32 i2c_dw_func(struct i2c_adapter *adap);
-extern irqreturn_t i2c_dw_isr(int this_irq, void *dev_id);
 extern void i2c_dw_disable(struct dw_i2c_dev *dev);
 extern void i2c_dw_disable_int(struct dw_i2c_dev *dev);
 extern u32 i2c_dw_read_comp_param(struct dw_i2c_dev *dev);
+extern int i2c_dw_probe(struct dw_i2c_dev *dev);
 
 #if IS_ENABLED(CONFIG_I2C_DESIGNWARE_BAYTRAIL)
 extern int i2c_dw_eval_lock_support(struct dw_i2c_dev *dev);
drivers/i2c/busses/i2c-designware-pcidrv.c +3 -27
index d9e8937f062f..169be1e26241 100644
@@ -158,11 +158,6 @@ static struct dw_pci_controller dw_pci_controllers[] = {
 	},
 };
 
-static struct i2c_algorithm i2c_dw_algo = {
-	.master_xfer	= i2c_dw_xfer,
-	.functionality	= i2c_dw_func,
-};
-
 #ifdef CONFIG_PM
 static int i2c_dw_pci_suspend(struct device *dev)
 {

@@ -222,13 +217,12 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 	if (!dev)
 		return -ENOMEM;
 
-	init_completion(&dev->cmd_complete);
-	mutex_init(&dev->lock);
 	dev->clk = NULL;
 	dev->controller = controller;
 	dev->get_clk_rate_khz = i2c_dw_get_clk_rate_khz;
 	dev->base = pcim_iomap_table(pdev)[0];
 	dev->dev = &pdev->dev;
+	dev->irq = pdev->irq;
 	dev->functionality = controller->functionality |
 				DW_DEFAULT_FUNCTIONALITY;
 

@@ -246,33 +240,15 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 
 	dev->tx_fifo_depth = controller->tx_fifo_depth;
 	dev->rx_fifo_depth = controller->rx_fifo_depth;
-	r = i2c_dw_init(dev);
-	if (r)
-		return r;
 
 	adap = &dev->adapter;
-	i2c_set_adapdata(adap, dev);
 	adap->owner = THIS_MODULE;
 	adap->class = 0;
-	adap->algo = &i2c_dw_algo;
-	adap->dev.parent = &pdev->dev;
 	adap->nr = controller->bus_num;
 
-	snprintf(adap->name, sizeof(adap->name), "i2c-designware-pci");
-
-	i2c_dw_disable_int(dev);
-	r = devm_request_irq(&pdev->dev, pdev->irq, i2c_dw_isr,
-			IRQF_SHARED | IRQF_COND_SUSPEND, adap->name, dev);
-	if (r) {
-		dev_err(&pdev->dev, "failure requesting irq %i\n", dev->irq);
-		return r;
-	}
-
-	r = i2c_add_numbered_adapter(adap);
-	if (r) {
-		dev_err(&pdev->dev, "failure adding adapter\n");
+	r = i2c_dw_probe(dev);
+	if (r)
 		return r;
-	}
 
 	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
 	pm_runtime_use_autosuspend(&pdev->dev);
drivers/i2c/busses/i2c-designware-platdrv.c +2 -26
index 17167cd4812d..adcf4c3a81f0 100644
@@ -41,10 +41,6 @@
 #include <linux/platform_data/i2c-designware.h>
 #include "i2c-designware-core.h"
 
-static struct i2c_algorithm i2c_dw_algo = {
-	.master_xfer	= i2c_dw_xfer,
-	.functionality	= i2c_dw_func,
-};
 static u32 i2c_dw_get_clk_rate_khz(struct dw_i2c_dev *dev)
 {
 	return clk_get_rate(dev->clk)/1000;

@@ -155,8 +151,6 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 	if (IS_ERR(dev->base))
 		return PTR_ERR(dev->base);
 
-	init_completion(&dev->cmd_complete);
-	mutex_init(&dev->lock);
 	dev->dev = &pdev->dev;
 	dev->irq = irq;
 	platform_set_drvdata(pdev, dev);

@@ -231,33 +225,15 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 		dev->rx_fifo_depth = ((param1 >> 8)  & 0xff) + 1;
 		dev->adapter.nr = pdev->id;
 	}
-	r = i2c_dw_init(dev);
-	if (r)
-		return r;
-
-	i2c_dw_disable_int(dev);
-	r = devm_request_irq(&pdev->dev, dev->irq, i2c_dw_isr, IRQF_SHARED,
-			pdev->name, dev);
-	if (r) {
-		dev_err(&pdev->dev, "failure requesting irq %i\n", dev->irq);
-		return r;
-	}
 
 	adap = &dev->adapter;
-	i2c_set_adapdata(adap, dev);
 	adap->owner = THIS_MODULE;
 	adap->class = I2C_CLASS_DEPRECATED;
-	strlcpy(adap->name, "Synopsys DesignWare I2C adapter",
-			sizeof(adap->name));
-	adap->algo = &i2c_dw_algo;
-	adap->dev.parent = &pdev->dev;
 	adap->dev.of_node = pdev->dev.of_node;
 
-	r = i2c_add_numbered_adapter(adap);
-	if (r) {
-		dev_err(&pdev->dev, "failure adding adapter\n");
+	r = i2c_dw_probe(dev);
+	if (r)
 		return r;
-	}
 
 	if (dev->pm_runtime_disabled) {
 		pm_runtime_forbid(&pdev->dev);
Signature · 1 line
2.6.1
#14 lore raw

Re: [PATCH v2 6/6] i2c: designware: Move common probe code into i2c_dw_probe()

Quoted 18 lines · Jarkko Nikula

There is some code duplication in i2c-designware-platdrv and i2c-designware-pcidrv probe functions. What is even worse that duplication requires i2c_dw_xfer(), i2c_dw_func() and i2c_dw_isr() i2c-designware-core functions to be exported.

Therefore move common code into new i2c_dw_probe() and make functions above local to i2c-designware-core.

While merging the code patch does following functional changes:

- I2C Adapter name will be "Synopsys DesignWare I2C adapter". Previously it was used for platform and ACPI devices but PCI device used "i2c-designware-pci".
- Using device name for interrupt name. Previous it was platform device name, ACPI device name or "i2c-designware-pci".
- Error code from devm_request_irq() and i2c_add_numbered_adapter() will be printed in case of error.

Quoted 1 line
Signed-off-by Jarkko Nikula <[email protected]>

There was a merge conflict with a bugfix from i2c/for-current. I think it is okay, but you may want to double check my i2c/for-next.

What about this irq-clearing-in-probe thingie on top of this series? :)

#15 lore raw

Re: [PATCH v2 6/6] i2c: designware: Move common probe code into i2c_dw_probe()

Hi

Quoted 3 lines · Wolfram Sang

There was a merge conflict with a bugfix from i2c/for-current. I think it is okay, but you may want to double check my i2c/for-next.

Looks like pm_runtime_disable() got dropped from your 36d48fb5766a ("i2c: designware-platdrv: enable RuntimePM before registering to the core") while handling the merge conflict. I'll send a fix.

Quoted 2 lines

What about this irq-clearing-in-probe thingie on top of this series? :)

I'll have a look at it. What's not entirely clear to me would it be no-op or not. HW is actually disabled after i2c_dw_init() which is called before requesting the interrupt but is not clear to me from the spec does HW clear interrupts while it goes idle.

So as a result I'd expect either a explicit interrupt clearing patch (to be more robust against potential unmasking changes) or a comment in __i2c_dw_enable() :-)

Signature · 1 line
Jarkko
Applied
#16 lore raw
In reply to Jarkko Nikula
Quoted 22 lines · Jarkko Nikula

It bothered to me to see "static struct i2c_algorithm i2c_dw_algo {}" defined twice both in i2c-designware-pcidrv.c and i2c-designware-platdrv.c and so many exported i2c-designware-core functions.

It turned out some of them became unused or are local and there were also duplicated probe code that I moved to new common i2c_dw_probe().

Object sizes below before and after this set from CONFIG_X86_64=y build.

   text	   data	    bss	    dec	    hex	filename
   6439	   1096	      0	   7535	   1d6f	drivers/i2c/busses/i2c-designware-core.ko
   5123	   1588	     16	   6727	   1a47	drivers/i2c/busses/i2c-designware-pci.ko
   5274	   1096	     16	   6386	   18f2	drivers/i2c/busses/i2c-designware-platform.ko
  16836	   3780	     32	  20648	   50a8	(TOTALS)
   text	   data	    bss	    dec	    hex	filename
   7225	   1120	     16	   8361	   20a9	drivers/i2c/busses/i2c-designware-core.ko
   4281	   1524	      0	   5805	   16ad	drivers/i2c/busses/i2c-designware-pci.ko
   4337	   1072	      0	   5409	   1521	drivers/i2c/busses/i2c-designware-platform.ko
  15843	   3716	     16	  19575	   4c77	(TOTALS)
Quoted 8 lines

Jarkko Nikula (6):
i2c: designware: Remove interrupt clearing from i2c_dw_pci_probe()
i2c: designware: Disable interrupts before requesting PCI device
interrupt
i2c: designware: Remove unused functions
i2c: designware: Make dw_readl() and dw_writel() static
i2c: designware: Rename platform driver probe and PM functions
i2c: designware: Move common probe code into i2c_dw_probe()

Applied to for-next (with V2 of patch 6), thanks!

Jarkko, would you be interested in maintaining the designware driver? For any non-trivial patch to this driver, I'd need assistance.

Thanks,

Wolfram

#17 lore raw

Hi

Quoted 3 lines · Wolfram Sang

Jarkko, would you be interested in maintaining the designware driver? For any non-trivial patch to this driver, I'd need assistance.

Yes I can. I think you could add also Mika and Andy as all of us are actively supporting it.

Signature · 1 line
Jarkko