[PATCHv8 0/5] I2C driver updates

[PATCHv8 0/5] I2C driver updates

[Shubhrajyoti D]

The series attempts to do the following

- The reset should not be done in the driver 
  have support for the same.
- Remove the sysc register access in the driver.


Incorporated review comments:

 - Fix the indentation of the code.
 - Restore in the error path is not needed as we are
   doing a init.
 - Combine the patch 1 and 2 as i2c-omap.h isn't a generic header
 - Making the changelogs more descriptive.
 - Add a default clockactivity field.
 - Renaming omap2_i2c_reset() to  omap_device_reset() and move it to omap_device.c
 - iterate over all of the hwmods in the omap_device and call omap_hwmod_reset() on
   each.


This is rebased on Kevin's tree  for_3.2/fixes/i2c branch.


Acknowledge Balaji ,Santosh ,Kevin , Paul and Jon for the review comments.



Shubhrajyoti D (5):
  OMAP3+: HWMOD: Add the default clockactivity for I2C
  OMAP: hwmod/device : add omap_device_reset to reset all the hwmods in
    the device
  OMAP: I2C: Reset support
  OMAP: I2C: Remove the reset in the init path
  OMAP: I2C: Remove the SYSC register definition

 arch/arm/mach-omap2/omap_hwmod_3xxx_data.c    |    7 +-
 arch/arm/mach-omap2/omap_hwmod_44xx_data.c    |    9 ++-
 arch/arm/plat-omap/i2c.c                      |    2 +
 arch/arm/plat-omap/include/plat/omap_device.h |    1 +
 arch/arm/plat-omap/omap_device.c              |   22 ++++++
 drivers/i2c/busses/i2c-omap.c                 |   88 +++++++------------------
 include/linux/i2c-omap.h                      |    1 +
 7 files changed, 58 insertions(+), 72 deletions(-)

[PATCHv8 1/5] OMAP3+: HWMOD: Add the default clockactivity for I2C

[Shubhrajyoti D]

For I2C clockactivity field is added for OMAP3 and OMAP4 that defines how the
interface (OCP) and functional (system) clocks behave when the I2C module is
idle.

The configuration of the clock activity bit field (per TRM) is as follows:
0x0: Both clocks can be cut off
0x1: Only OCP clock must be kept active; system clock
     can be cut off
0x3: Both clocks must be kept active
0x2: Only system clock must be kept active; OCP clock
     can be cut off

The patch makes 0x2(CLOCKACT_TEST_ICLK) the default for OMAP3 and OMAP4.

Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
Signed-off-by: Jon Hunter <jon-hunter@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod_3xxx_data.c |    7 ++++---
 arch/arm/mach-omap2/omap_hwmod_44xx_data.c |    9 +++++----
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
index 7f8915a..1b59118 100644
--- a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
@@ -1162,6 +1162,7 @@ static struct omap_hwmod_class_sysconfig i2c_sysc = {
 			   SYSC_HAS_ENAWAKEUP | SYSC_HAS_SOFTRESET |
 			   SYSC_HAS_AUTOIDLE | SYSS_HAS_RESET_STATUS),
 	.idlemodes	= (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART),
+	.clockact	= CLOCKACT_TEST_ICLK,
 	.sysc_fields    = &omap_hwmod_sysc_type1,
 };
 
@@ -1636,7 +1637,7 @@ static struct omap_hwmod_ocp_if *omap3xxx_i2c1_slaves[] = {
 
 static struct omap_hwmod omap3xxx_i2c1_hwmod = {
 	.name		= "i2c1",
-	.flags		= HWMOD_16BIT_REG,
+	.flags		= HWMOD_16BIT_REG | HWMOD_SET_DEFAULT_CLOCKACT,
 	.mpu_irqs	= omap2_i2c1_mpu_irqs,
 	.sdma_reqs	= omap2_i2c1_sdma_reqs,
 	.main_clk	= "i2c1_fck",
@@ -1670,7 +1671,7 @@ static struct omap_hwmod_ocp_if *omap3xxx_i2c2_slaves[] = {
 
 static struct omap_hwmod omap3xxx_i2c2_hwmod = {
 	.name		= "i2c2",
-	.flags		= HWMOD_16BIT_REG,
+	.flags		= HWMOD_16BIT_REG | HWMOD_SET_DEFAULT_CLOCKACT,
 	.mpu_irqs	= omap2_i2c2_mpu_irqs,
 	.sdma_reqs	= omap2_i2c2_sdma_reqs,
 	.main_clk	= "i2c2_fck",
@@ -1715,7 +1716,7 @@ static struct omap_hwmod_ocp_if *omap3xxx_i2c3_slaves[] = {
 
 static struct omap_hwmod omap3xxx_i2c3_hwmod = {
 	.name		= "i2c3",
-	.flags		= HWMOD_16BIT_REG,
+	.flags		= HWMOD_16BIT_REG | HWMOD_SET_DEFAULT_CLOCKACT,
 	.mpu_irqs	= i2c3_mpu_irqs,
 	.sdma_reqs	= i2c3_sdma_reqs,
 	.main_clk	= "i2c3_fck",
diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
index daaf165..c0d8a2e 100644
--- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
@@ -2246,6 +2246,7 @@ static struct omap_hwmod_class_sysconfig omap44xx_i2c_sysc = {
 			   SYSC_HAS_SOFTRESET | SYSS_HAS_RESET_STATUS),
 	.idlemodes	= (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART |
 			   SIDLE_SMART_WKUP),
+	.clockact	= CLOCKACT_TEST_ICLK,
 	.sysc_fields	= &omap_hwmod_sysc_type1,
 };
 
@@ -2300,7 +2301,7 @@ static struct omap_hwmod omap44xx_i2c1_hwmod = {
 	.name		= "i2c1",
 	.class		= &omap44xx_i2c_hwmod_class,
 	.clkdm_name	= "l4_per_clkdm",
-	.flags		= HWMOD_16BIT_REG,
+	.flags		= HWMOD_16BIT_REG | HWMOD_SET_DEFAULT_CLOCKACT,
 	.mpu_irqs	= omap44xx_i2c1_irqs,
 	.sdma_reqs	= omap44xx_i2c1_sdma_reqs,
 	.main_clk	= "i2c1_fck",
@@ -2356,7 +2357,7 @@ static struct omap_hwmod omap44xx_i2c2_hwmod = {
 	.name		= "i2c2",
 	.class		= &omap44xx_i2c_hwmod_class,
 	.clkdm_name	= "l4_per_clkdm",
-	.flags		= HWMOD_16BIT_REG,
+	.flags		= HWMOD_16BIT_REG | HWMOD_SET_DEFAULT_CLOCKACT,
 	.mpu_irqs	= omap44xx_i2c2_irqs,
 	.sdma_reqs	= omap44xx_i2c2_sdma_reqs,
 	.main_clk	= "i2c2_fck",
@@ -2412,7 +2413,7 @@ static struct omap_hwmod omap44xx_i2c3_hwmod = {
 	.name		= "i2c3",
 	.class		= &omap44xx_i2c_hwmod_class,
 	.clkdm_name	= "l4_per_clkdm",
-	.flags		= HWMOD_16BIT_REG,
+	.flags		= HWMOD_16BIT_REG | HWMOD_SET_DEFAULT_CLOCKACT,
 	.mpu_irqs	= omap44xx_i2c3_irqs,
 	.sdma_reqs	= omap44xx_i2c3_sdma_reqs,
 	.main_clk	= "i2c3_fck",
@@ -2468,7 +2469,7 @@ static struct omap_hwmod omap44xx_i2c4_hwmod = {
 	.name		= "i2c4",
 	.class		= &omap44xx_i2c_hwmod_class,
 	.clkdm_name	= "l4_per_clkdm",
-	.flags		= HWMOD_16BIT_REG,
+	.flags		= HWMOD_16BIT_REG | HWMOD_SET_DEFAULT_CLOCKACT,
 	.mpu_irqs	= omap44xx_i2c4_irqs,
 	.sdma_reqs	= omap44xx_i2c4_sdma_reqs,
 	.main_clk	= "i2c4_fck",
-- 
1.7.1

[PATCHv8 2/5] OMAP: hwmod/device : add omap_device_reset to reset all the hwmods in the device

[Shubhrajyoti D]

    Add a function omap_device_reset() to reset all the hwmods in the hwmod device.
    This is intended to be used by device drivers to reset all the hwmods in the
    device. This is needed to support some ips like i2c which  may have to do reset
    in error paths or in errata.

Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
---
 arch/arm/plat-omap/include/plat/omap_device.h |    1 +
 arch/arm/plat-omap/omap_device.c              |   22 ++++++++++++++++++++++
 2 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-omap/include/plat/omap_device.h b/arch/arm/plat-omap/include/plat/omap_device.h
index 51423d2..663ef55 100644
--- a/arch/arm/plat-omap/include/plat/omap_device.h
+++ b/arch/arm/plat-omap/include/plat/omap_device.h
@@ -116,6 +116,7 @@ int omap_device_enable_hwmods(struct omap_device *od);
 
 int omap_device_disable_clocks(struct omap_device *od);
 int omap_device_enable_clocks(struct omap_device *od);
+int omap_device_reset(struct device *dev);
 
 /*
  * Entries should be kept in latency order ascending
diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
index e8d9869..e80e8c4 100644
--- a/arch/arm/plat-omap/omap_device.c
+++ b/arch/arm/plat-omap/omap_device.c
@@ -1129,6 +1129,28 @@ int omap_device_enable_clocks(struct omap_device *od)
 	/* XXX pass along return value here? */
 	return 0;
 }
+/**
+ * omap_device_reset - reset the module.
+ * @dev: struct device*
+ *
+ * Reset all the hwmods associated with the device
+ */
+
+int omap_device_reset(struct device *dev)
+{
+	int r = 0;
+	int i;
+	struct platform_device *pdev = to_platform_device(dev);
+	struct omap_device *odev = to_omap_device(pdev);
+	struct omap_hwmod *oh;
+
+	for (i = 0; i < odev->hwmods_cnt; i++) {
+		oh = odev->hwmods[i];
+		r |= omap_hwmod_reset(oh);
+	}
+	return r;
+}
+
 
 struct device omap_device_parent = {
 	.init_name	= "omap",
-- 
1.7.1

[PATCHv8 3/5] OMAP: I2C: Reset support

[Shubhrajyoti D]

Under some error conditions the i2c driver may do a reset.
Adding a reset field and support in the device-specific code to aid
error-recovery.

Signed-off-by: Shubhrajyoti D <shubhrajyoti-l0cyMroinI0@public.gmane.org>
---
 arch/arm/plat-omap/i2c.c |    2 ++
 include/linux/i2c-omap.h |    1 +
 2 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-omap/i2c.c b/arch/arm/plat-omap/i2c.c
index db071bc..6cddde2 100644
--- a/arch/arm/plat-omap/i2c.c
+++ b/arch/arm/plat-omap/i2c.c
@@ -179,6 +179,8 @@ static inline int omap2_i2c_add_bus(int bus_id)
 	 */
 	if (cpu_is_omap34xx())
 		pdata->set_mpu_wkup_lat = omap_pm_set_max_mpu_wakeup_lat_compat;
+
+	pdata->device_reset = omap_device_reset;
 	pdev = omap_device_build(name, bus_id, oh, pdata,
 			sizeof(struct omap_i2c_bus_platform_data),
 			NULL, 0, 0);
diff --git a/include/linux/i2c-omap.h b/include/linux/i2c-omap.h
index 92a0dc7..fd38249 100644
--- a/include/linux/i2c-omap.h
+++ b/include/linux/i2c-omap.h
@@ -35,6 +35,7 @@ struct omap_i2c_bus_platform_data {
 	u32		rev;
 	u32		flags;
 	void		(*set_mpu_wkup_lat)(struct device *dev, long set);
+	int		(*device_reset) (struct device *dev);
 };
 
 #endif
-- 
1.7.1

[PATCHv8 4/5] OMAP: I2C: Remove the reset in the init path

[Shubhrajyoti D]

-  The reset in the driver at init is not needed anymore as the
   hwmod framework takes care of reseting it.
-  Reset is removed from omap_i2c_init, which was called
   not only during probe, but also after time out and error handling.
   device_reset were added in those places to effect the reset.
-  Earlier the hwmod SYSC settings were over-written in the driver.
   Removing the same and letting the hwmod take care of the settings.
-  Clean up the SYSS_RESETDONE_MASK macro as it is no longer needed.
-  Clean up the SYSCONFIG SYSC bit definition macros.
-  Fix the typos in wakeup.

Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
---
 drivers/i2c/busses/i2c-omap.c |   83 +++++++++++-----------------------------
 1 files changed, 23 insertions(+), 60 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 257c1a5..70136a2 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -155,19 +155,6 @@ enum {
 #define OMAP_I2C_SYSTEST_SDA_O		(1 << 0)	/* SDA line drive out */
 #endif
 
-/* OCP_SYSSTATUS bit definitions */
-#define SYSS_RESETDONE_MASK		(1 << 0)
-
-/* OCP_SYSCONFIG bit definitions */
-#define SYSC_CLOCKACTIVITY_MASK		(0x3 << 8)
-#define SYSC_SIDLEMODE_MASK		(0x3 << 3)
-#define SYSC_ENAWAKEUP_MASK		(1 << 2)
-#define SYSC_SOFTRESET_MASK		(1 << 1)
-#define SYSC_AUTOIDLE_MASK		(1 << 0)
-
-#define SYSC_IDLEMODE_SMART		0x2
-#define SYSC_CLOCKACTIVITY_FCLK		0x2
-
 /* Errata definitions */
 #define I2C_OMAP_ERRATA_I207		(1 << 0)
 #define I2C_OMAP3_1P153			(1 << 1)
@@ -182,6 +169,7 @@ struct omap_i2c_dev {
 	u32			latency;	/* maximum mpu wkup latency */
 	void			(*set_mpu_wkup_lat)(struct device *dev,
 						    long latency);
+	int			(*device_reset)(struct device *dev);
 	u32			speed;		/* Speed of bus in Khz */
 	u16			cmd_err;
 	u8			*buf;
@@ -312,65 +300,37 @@ static void omap_i2c_idle(struct omap_i2c_dev *dev)
 	}
 }
 
+static inline void omap_i2c_reset(struct omap_i2c_dev *dev)
+{
+	if (!dev->device_reset)
+		return;
+
+	if (dev->device_reset(dev->dev) < 0)
+		dev_err(dev->dev, "reset failed\n");
+}
+
 static int omap_i2c_init(struct omap_i2c_dev *dev)
 {
 	u16 psc = 0, scll = 0, sclh = 0, buf = 0;
 	u16 fsscll = 0, fssclh = 0, hsscll = 0, hssclh = 0;
 	unsigned long fclk_rate = 12000000;
-	unsigned long timeout;
 	unsigned long internal_clk = 0;
 	struct clk *fclk;
 	struct omap_i2c_bus_platform_data *pdata;
 
 	pdata = dev->dev->platform_data;
 
-	if (dev->rev >= OMAP_I2C_OMAP1_REV_2) {
-		/* Disable I2C controller before soft reset */
-		omap_i2c_write_reg(dev, OMAP_I2C_CON_REG,
-			omap_i2c_read_reg(dev, OMAP_I2C_CON_REG) &
-				~(OMAP_I2C_CON_EN));
-
-		omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG, SYSC_SOFTRESET_MASK);
-		/* For some reason we need to set the EN bit before the
-		 * reset done bit gets set. */
-		timeout = jiffies + OMAP_I2C_TIMEOUT;
-		omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
-		while (!(omap_i2c_read_reg(dev, OMAP_I2C_SYSS_REG) &
-			 SYSS_RESETDONE_MASK)) {
-			if (time_after(jiffies, timeout)) {
-				dev_warn(dev->dev, "timeout waiting "
-						"for controller reset\n");
-				return -ETIMEDOUT;
-			}
-			msleep(1);
-		}
-
-		/* SYSC register is cleared by the reset; rewrite it */
-		if (dev->rev == OMAP_I2C_REV_ON_2430) {
-
-			omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG,
-					   SYSC_AUTOIDLE_MASK);
-
-		} else if (dev->rev >= OMAP_I2C_REV_ON_3430) {
-			dev->syscstate = SYSC_AUTOIDLE_MASK;
-			dev->syscstate |= SYSC_ENAWAKEUP_MASK;
-			dev->syscstate |= (SYSC_IDLEMODE_SMART <<
-			      __ffs(SYSC_SIDLEMODE_MASK));
-			dev->syscstate |= (SYSC_CLOCKACTIVITY_FCLK <<
-			      __ffs(SYSC_CLOCKACTIVITY_MASK));
-
-			omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG,
-							dev->syscstate);
-			/*
-			 * Enabling all wakup sources to stop I2C freezing on
-			 * WFI instruction.
-			 * REVISIT: Some wkup sources might not be needed.
-			 */
-			dev->westate = OMAP_I2C_WE_ALL;
-			omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
-							dev->westate);
-		}
+	if (dev->rev >= OMAP_I2C_REV_ON_3430) {
+		/*
+		 * Enabling all wakeup sources to stop I2C freezing on
+		 * WFI instruction.
+		 * REVISIT: Some wakeup sources might not be needed.
+		 */
+		dev->westate = OMAP_I2C_WE_ALL;
+		omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
+						dev->westate);
 	}
+
 	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
 
 	if (pdata->flags & OMAP_I2C_FLAG_ALWAYS_ARMXOR_CLK) {
@@ -594,6 +554,7 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 		return r;
 	if (r == 0) {
 		dev_err(dev->dev, "controller timed out\n");
+		omap_i2c_reset(dev);
 		omap_i2c_init(dev);
 		return -ETIMEDOUT;
 	}
@@ -604,6 +565,7 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 	/* We have an error */
 	if (dev->cmd_err & (OMAP_I2C_STAT_AL | OMAP_I2C_STAT_ROVR |
 			    OMAP_I2C_STAT_XUDF)) {
+		omap_i2c_reset(dev);
 		omap_i2c_init(dev);
 		return -EIO;
 	}
@@ -1004,6 +966,7 @@ omap_i2c_probe(struct platform_device *pdev)
 	if (pdata != NULL) {
 		speed = pdata->clkrate;
 		dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat;
+		dev->device_reset = pdata->device_reset;
 	} else {
 		speed = 100;	/* Default speed */
 		dev->set_mpu_wkup_lat = NULL;
-- 
1.7.1

[PATCHv8 5/5] OMAP: I2C: Remove the SYSC register definition

[Shubhrajyoti D]

The SYSC register should not accessed in the driver removing the
define from the driver.
Also clean up the syscstate from the omap_i2c_dev struct.

Acked-by: Santosh Shilimkar <santosh.shilimkar-l0cyMroinI0@public.gmane.org>
Signed-off-by: Shubhrajyoti D <shubhrajyoti-l0cyMroinI0@public.gmane.org>
---
 drivers/i2c/busses/i2c-omap.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 70136a2..9dcd99b 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -63,7 +63,6 @@ enum {
 	OMAP_I2C_BUF_REG,
 	OMAP_I2C_CNT_REG,
 	OMAP_I2C_DATA_REG,
-	OMAP_I2C_SYSC_REG,
 	OMAP_I2C_CON_REG,
 	OMAP_I2C_OA_REG,
 	OMAP_I2C_SA_REG,
@@ -187,7 +186,6 @@ struct omap_i2c_dev {
 	u16			scllstate;
 	u16			sclhstate;
 	u16			bufstate;
-	u16			syscstate;
 	u16			westate;
 	u16			errata;
 };
@@ -202,7 +200,6 @@ static const u8 reg_map_ip_v1[] = {
 	[OMAP_I2C_BUF_REG] = 0x05,
 	[OMAP_I2C_CNT_REG] = 0x06,
 	[OMAP_I2C_DATA_REG] = 0x07,
-	[OMAP_I2C_SYSC_REG] = 0x08,
 	[OMAP_I2C_CON_REG] = 0x09,
 	[OMAP_I2C_OA_REG] = 0x0a,
 	[OMAP_I2C_SA_REG] = 0x0b,
@@ -223,7 +220,6 @@ static const u8 reg_map_ip_v2[] = {
 	[OMAP_I2C_BUF_REG] = 0x94,
 	[OMAP_I2C_CNT_REG] = 0x98,
 	[OMAP_I2C_DATA_REG] = 0x9c,
-	[OMAP_I2C_SYSC_REG] = 0x10,
 	[OMAP_I2C_CON_REG] = 0xa4,
 	[OMAP_I2C_OA_REG] = 0xa8,
 	[OMAP_I2C_SA_REG] = 0xac,
@@ -264,7 +260,6 @@ static void omap_i2c_unidle(struct omap_i2c_dev *dev)
 		omap_i2c_write_reg(dev, OMAP_I2C_SCLL_REG, dev->scllstate);
 		omap_i2c_write_reg(dev, OMAP_I2C_SCLH_REG, dev->sclhstate);
 		omap_i2c_write_reg(dev, OMAP_I2C_BUF_REG, dev->bufstate);
-		omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG, dev->syscstate);
 		omap_i2c_write_reg(dev, OMAP_I2C_WE_REG, dev->westate);
 		omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
 	}
-- 
1.7.1

Re: [PATCHv8 1/5] OMAP3+: HWMOD: Add the default clockactivity for I2C

[Paul Walmsley]

On Tue, 13 Dec 2011, Shubhrajyoti D wrote:

> For I2C clockactivity field is added for OMAP3 and OMAP4 that defines how the
> interface (OCP) and functional (system) clocks behave when the I2C module is
> idle.
> 
> The configuration of the clock activity bit field (per TRM) is as follows:
> 0x0: Both clocks can be cut off
> 0x1: Only OCP clock must be kept active; system clock
>      can be cut off
> 0x3: Both clocks must be kept active
> 0x2: Only system clock must be kept active; OCP clock
>      can be cut off
> 
> The patch makes 0x2(CLOCKACT_TEST_ICLK) the default for OMAP3 and OMAP4.
> 
> Signed-off-by: Shubhrajyoti D <shubhrajyoti-l0cyMroinI0@public.gmane.org>
> Signed-off-by: Jon Hunter <jon-hunter-l0cyMroinI0@public.gmane.org>

Thanks, queued for 3.3.


- Paul

Re: [PATCHv8 3/5] OMAP: I2C: Reset support

[Paul Walmsley]

On Tue, 13 Dec 2011, Shubhrajyoti D wrote:

> Under some error conditions the i2c driver may do a reset.
> Adding a reset field and support in the device-specific code to aid
> error-recovery.
> 
> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>

Thanks, queued for 3.3.


- Paul

Re: [PATCHv8 4/5] OMAP: I2C: Remove the reset in the init path

[Paul Walmsley]

Hi

On Tue, 13 Dec 2011, Shubhrajyoti D wrote:

> -  The reset in the driver at init is not needed anymore as the
>    hwmod framework takes care of reseting it.
> -  Reset is removed from omap_i2c_init, which was called
>    not only during probe, but also after time out and error handling.
>    device_reset were added in those places to effect the reset.
> -  Earlier the hwmod SYSC settings were over-written in the driver.
>    Removing the same and letting the hwmod take care of the settings.
> -  Clean up the SYSS_RESETDONE_MASK macro as it is no longer needed.
> -  Clean up the SYSCONFIG SYSC bit definition macros.
> -  Fix the typos in wakeup.
> 
> Signed-off-by: Shubhrajyoti D <shubhrajyoti-l0cyMroinI0@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-omap.c |   83 +++++++++++-----------------------------
>  1 files changed, 23 insertions(+), 60 deletions(-)

This patch either needs to be acked by Ben with a note that it's okay for 
us to merge through the OMAP tree, or needs to be merged by Ben during the 
3.4 merge window, after patches 1-3 have reached the mainline tree.


- Paul

Re: [PATCHv8 5/5] OMAP: I2C: Remove the SYSC register definition

[Paul Walmsley]

Hi 

On Tue, 13 Dec 2011, Shubhrajyoti D wrote:

> The SYSC register should not accessed in the driver removing the
> define from the driver.
> Also clean up the syscstate from the omap_i2c_dev struct.
> 
> Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> ---
>  drivers/i2c/busses/i2c-omap.c |    5 -----
>  1 files changed, 0 insertions(+), 5 deletions(-)

Same for this patch - it either needs to be acked by Ben with a note that 
it's okay for us to merge through the OMAP tree, or needs to be merged by 
Ben during the 3.4 merge window, after patches 1-3 have reached the 
mainline tree.


- Paul

Re: [PATCHv8 2/5] OMAP: hwmod/device : add omap_device_reset to reset all the hwmods in the device

[Paul Walmsley]

Hi

On Tue, 13 Dec 2011, Shubhrajyoti D wrote:

>     Add a function omap_device_reset() to reset all the hwmods in the hwmod device.
>     This is intended to be used by device drivers to reset all the hwmods in the
>     device. This is needed to support some ips like i2c which  may have to do reset
>     in error paths or in errata.
> 
> Signed-off-by: Shubhrajyoti D <shubhrajyoti-l0cyMroinI0@public.gmane.org>

Thanks, queued for 3.3.

- Paul

Re: [PATCHv8 3/5] OMAP: I2C: Reset support

[Cousson, Benoit]

Hi Shubhro,

On 12/13/2011 11:55 AM, Shubhrajyoti D wrote:
> Under some error conditions the i2c driver may do a reset.
> Adding a reset field and support in the device-specific code to aid
> error-recovery.
>
> Signed-off-by: Shubhrajyoti D<shubhrajyoti-l0cyMroinI0@public.gmane.org>
> ---
>   arch/arm/plat-omap/i2c.c |    2 ++
>   include/linux/i2c-omap.h |    1 +
>   2 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/plat-omap/i2c.c b/arch/arm/plat-omap/i2c.c
> index db071bc..6cddde2 100644
> --- a/arch/arm/plat-omap/i2c.c
> +++ b/arch/arm/plat-omap/i2c.c
> @@ -179,6 +179,8 @@ static inline int omap2_i2c_add_bus(int bus_id)
>   	 */
>   	if (cpu_is_omap34xx())
>   		pdata->set_mpu_wkup_lat = omap_pm_set_max_mpu_wakeup_lat_compat;
> +
> +	pdata->device_reset = omap_device_reset;

We should avoid introducing any new pdata function pointers since we are 
in the process of removing them for DT support.

Regards,
Benoit

Re: [PATCHv8 3/5] OMAP: I2C: Reset support

[Shubhrajyoti]

Hi Benoit,

On Friday 16 December 2011 02:21 PM, Cousson, Benoit wrote:
> Hi Shubhro,
>
> On 12/13/2011 11:55 AM, Shubhrajyoti D wrote:
>> Under some error conditions the i2c driver may do a reset.
>> Adding a reset field and support in the device-specific code to aid
>> error-recovery.
>>
>> Signed-off-by: Shubhrajyoti D<shubhrajyoti-l0cyMroinI0@public.gmane.org>
>> ---
>>   arch/arm/plat-omap/i2c.c |    2 ++
>>   include/linux/i2c-omap.h |    1 +
>>   2 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/plat-omap/i2c.c b/arch/arm/plat-omap/i2c.c
>> index db071bc..6cddde2 100644
>> --- a/arch/arm/plat-omap/i2c.c
>> +++ b/arch/arm/plat-omap/i2c.c
>> @@ -179,6 +179,8 @@ static inline int omap2_i2c_add_bus(int bus_id)
>>        */
>>       if (cpu_is_omap34xx())
>>           pdata->set_mpu_wkup_lat =
>> omap_pm_set_max_mpu_wakeup_lat_compat;
>> +
>> +    pdata->device_reset = omap_device_reset;
>
> We should avoid introducing any new pdata function pointers since we
> are in the process of removing them for DT support.
However if the driver has to do a reset due to some error what is the
recommended way?
Currently we used to access the SYSC directly I am removing the same and
introducing this  function pointer.

>
> Regards,
> Benoit

Re: [PATCHv8 4/5] OMAP: I2C: Remove the reset in the init path

[Shubhrajyoti]

Ben,

On Friday 16 December 2011 02:10 PM, Paul Walmsley wrote:
> Hi
>
> On Tue, 13 Dec 2011, Shubhrajyoti D wrote:
>
>> -  The reset in the driver at init is not needed anymore as the
>>    hwmod framework takes care of reseting it.
>> -  Reset is removed from omap_i2c_init, which was called
>>    not only during probe, but also after time out and error handling.
>>    device_reset were added in those places to effect the reset.
>> -  Earlier the hwmod SYSC settings were over-written in the driver.
>>    Removing the same and letting the hwmod take care of the settings.
>> -  Clean up the SYSS_RESETDONE_MASK macro as it is no longer needed.
>> -  Clean up the SYSCONFIG SYSC bit definition macros.
>> -  Fix the typos in wakeup.
>>
>> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
>> ---
>>  drivers/i2c/busses/i2c-omap.c |   83 +++++++++++-----------------------------
>>  1 files changed, 23 insertions(+), 60 deletions(-)
> This patch either needs to be acked by Ben with a note that it's okay for 
> us to merge through the OMAP tree, or needs to be merged by Ben during the 
> 3.4 merge window, after patches 1-3 have reached the mainline tree.
I agree.
Ben do you have any comments .
>
> - Paul

Re: [PATCHv8 3/5] OMAP: I2C: Reset support

[Tony Lindgren]

* Shubhrajyoti <shubhrajyoti-l0cyMroinI0@public.gmane.org> [111216 00:25]:
> Hi Benoit,
> 
> On Friday 16 December 2011 02:21 PM, Cousson, Benoit wrote:
> > Hi Shubhro,
> >
> > On 12/13/2011 11:55 AM, Shubhrajyoti D wrote:
> >> Under some error conditions the i2c driver may do a reset.
> >> Adding a reset field and support in the device-specific code to aid
> >> error-recovery.
> >>
> >> Signed-off-by: Shubhrajyoti D<shubhrajyoti-l0cyMroinI0@public.gmane.org>
> >> ---
> >>   arch/arm/plat-omap/i2c.c |    2 ++
> >>   include/linux/i2c-omap.h |    1 +
> >>   2 files changed, 3 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/arch/arm/plat-omap/i2c.c b/arch/arm/plat-omap/i2c.c
> >> index db071bc..6cddde2 100644
> >> --- a/arch/arm/plat-omap/i2c.c
> >> +++ b/arch/arm/plat-omap/i2c.c
> >> @@ -179,6 +179,8 @@ static inline int omap2_i2c_add_bus(int bus_id)
> >>        */
> >>       if (cpu_is_omap34xx())
> >>           pdata->set_mpu_wkup_lat =
> >> omap_pm_set_max_mpu_wakeup_lat_compat;
> >> +
> >> +    pdata->device_reset = omap_device_reset;
> >
> > We should avoid introducing any new pdata function pointers since we
> > are in the process of removing them for DT support.
> However if the driver has to do a reset due to some error what is the
> recommended way?
> Currently we used to access the SYSC directly I am removing the same and
> introducing this  function pointer.

Sounds like this sould be possible to handle with pm_runtime
calls from the driver which should end up making the right
hwmod calls.

Regards,

Tony

Re: [PATCHv8 4/5] OMAP: I2C: Remove the reset in the init path

[Datta, Shubhrajyoti]

On Tue, Jan 10, 2012 at 11:53 AM, Datta, Shubhrajyoti
<shubhrajyoti-l0cyMroinI0@public.gmane.org> wrote:
>
>
>
> On Fri, Dec 16, 2011 at 2:29 PM, Shubhrajyoti <shubhrajyoti-l0cyMroinI0@public.gmane.org> wrote:
>>
>> Ben,
>>
>> On Friday 16 December 2011 02:10 PM, Paul Walmsley wrote:
>> > Hi
>> >
>> > On Tue, 13 Dec 2011, Shubhrajyoti D wrote:
>> >
>> >> -  The reset in the driver at init is not needed anymore as the
>> >>    hwmod framework takes care of reseting it.
>> >> -  Reset is removed from omap_i2c_init, which was called
>> >>    not only during probe, but also after time out and error handling.
>> >>    device_reset were added in those places to effect the reset.
>> >> -  Earlier the hwmod SYSC settings were over-written in the driver.
>> >>    Removing the same and letting the hwmod take care of the settings.
>> >> -  Clean up the SYSS_RESETDONE_MASK macro as it is no longer needed.
>> >> -  Clean up the SYSCONFIG SYSC bit definition macros.
>> >> -  Fix the typos in wakeup.
>> >>
>> >> Signed-off-by: Shubhrajyoti D <shubhrajyoti-l0cyMroinI0@public.gmane.org>
>> >> ---
>> >>  drivers/i2c/busses/i2c-omap.c |   83 +++++++++++-----------------------------
>> >>  1 files changed, 23 insertions(+), 60 deletions(-)
>> > This patch either needs to be acked by Ben with a note that it's okay for
>> > us to merge through the OMAP tree, or needs to be merged by Ben during the
>> > 3.4 merge window, after patches 1-3 have reached the mainline tree.
>> I agree.
>> Ben do you have any comments .
>
>
If there are no further comments can this be merged also ?
Thanks.
>
>>
>> >
>> > - Paul
>>
>

Re: [PATCHv8 4/5] OMAP: I2C: Remove the reset in the init path

[Kevin Hilman]

"Datta, Shubhrajyoti" <shubhrajyoti@ti.com> writes:

> On Tue, Jan 10, 2012 at 11:53 AM, Datta, Shubhrajyoti
> <shubhrajyoti@ti.com> wrote:
>>
>>
>>
>> On Fri, Dec 16, 2011 at 2:29 PM, Shubhrajyoti <shubhrajyoti@ti.com> wrote:
>>>
>>> Ben,
>>>
>>> On Friday 16 December 2011 02:10 PM, Paul Walmsley wrote:
>>> > Hi
>>> >
>>> > On Tue, 13 Dec 2011, Shubhrajyoti D wrote:
>>> >
>>> >> -  The reset in the driver at init is not needed anymore as the
>>> >>    hwmod framework takes care of reseting it.
>>> >> -  Reset is removed from omap_i2c_init, which was called
>>> >>    not only during probe, but also after time out and error handling.
>>> >>    device_reset were added in those places to effect the reset.
>>> >> -  Earlier the hwmod SYSC settings were over-written in the driver.
>>> >>    Removing the same and letting the hwmod take care of the settings.
>>> >> -  Clean up the SYSS_RESETDONE_MASK macro as it is no longer needed.
>>> >> -  Clean up the SYSCONFIG SYSC bit definition macros.
>>> >> -  Fix the typos in wakeup.
>>> >>
>>> >> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
>>> >> ---
>>> >>  drivers/i2c/busses/i2c-omap.c |   83 +++++++++++-----------------------------
>>> >>  1 files changed, 23 insertions(+), 60 deletions(-)
>>> > This patch either needs to be acked by Ben with a note that it's okay for
>>> > us to merge through the OMAP tree, or needs to be merged by Ben during the
>>> > 3.4 merge window, after patches 1-3 have reached the mainline tree.
>>> I agree.
>>> Ben do you have any comments .
>>
>>
> If there are no further comments can this be merged also ?

As Benoit mentioned earlier, the addition of new pdata fields will cause
problems with the DT support already queued for v3.3.

This series should be reworked on top of the DT support which Ben has
now queued for v3.3 (my branch: for_3.3/i2c/misc)

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv8 4/5] OMAP: I2C: Remove the reset in the init path

[Shubhrajyoti]

On Tuesday 10 January 2012 08:56 PM, Kevin Hilman wrote:
> "Datta, Shubhrajyoti" <shubhrajyoti-l0cyMroinI0@public.gmane.org> writes:
>
>> On Tue, Jan 10, 2012 at 11:53 AM, Datta, Shubhrajyoti
>> <shubhrajyoti-l0cyMroinI0@public.gmane.org> wrote:
>>>
>>>
>>> On Fri, Dec 16, 2011 at 2:29 PM, Shubhrajyoti <shubhrajyoti-l0cyMroinI0@public.gmane.org> wrote:
>>>> Ben,
>>>>
>>>> On Friday 16 December 2011 02:10 PM, Paul Walmsley wrote:
>>>>> Hi
>>>>>
>>>>> On Tue, 13 Dec 2011, Shubhrajyoti D wrote:
>>>>>
>>>>>> -  The reset in the driver at init is not needed anymore as the
>>>>>>    hwmod framework takes care of reseting it.
>>>>>> -  Reset is removed from omap_i2c_init, which was called
>>>>>>    not only during probe, but also after time out and error handling.
>>>>>>    device_reset were added in those places to effect the reset.
>>>>>> -  Earlier the hwmod SYSC settings were over-written in the driver.
>>>>>>    Removing the same and letting the hwmod take care of the settings.
>>>>>> -  Clean up the SYSS_RESETDONE_MASK macro as it is no longer needed.
>>>>>> -  Clean up the SYSCONFIG SYSC bit definition macros.
>>>>>> -  Fix the typos in wakeup.
>>>>>>
>>>>>> Signed-off-by: Shubhrajyoti D <shubhrajyoti-l0cyMroinI0@public.gmane.org>
>>>>>> ---
>>>>>>  drivers/i2c/busses/i2c-omap.c |   83 +++++++++++-----------------------------
>>>>>>  1 files changed, 23 insertions(+), 60 deletions(-)
>>>>> This patch either needs to be acked by Ben with a note that it's okay for
>>>>> us to merge through the OMAP tree, or needs to be merged by Ben during the
>>>>> 3.4 merge window, after patches 1-3 have reached the mainline tree.
>>>> I agree.
>>>> Ben do you have any comments .
>>>
>> If there are no further comments can this be merged also ?
> As Benoit mentioned earlier, the addition of new pdata fields will cause
> problems with the DT support already queued for v3.3.
>
> This series should be reworked on top of the DT support which Ben has
> now queued for v3.3 (my branch: for_3.3/i2c/misc)

Yes will rework it.
> Kevin

Re: [PATCHv8 4/5] OMAP: I2C: Remove the reset in the init path

[Paul Walmsley]

On Wed, 11 Jan 2012, Shubhrajyoti wrote:

> On Tuesday 10 January 2012 08:56 PM, Kevin Hilman wrote:
> > "Datta, Shubhrajyoti" <shubhrajyoti@ti.com> writes:
> >
> >> On Tue, Jan 10, 2012 at 11:53 AM, Datta, Shubhrajyoti
> >> <shubhrajyoti@ti.com> wrote:
> >>>
> >>> On Fri, Dec 16, 2011 at 2:29 PM, Shubhrajyoti <shubhrajyoti@ti.com> wrote:
> >>>> On Friday 16 December 2011 02:10 PM, Paul Walmsley wrote:
> >>>>
> >>>>> This patch either needs to be acked by Ben with a note that it's okay for
> >>>>> us to merge through the OMAP tree, or needs to be merged by Ben during the
> >>>>> 3.4 merge window, after patches 1-3 have reached the mainline tree.
> >>>> I agree.
> >>>> Ben do you have any comments .
> >>>
> >> If there are no further comments can this be merged also ?
> > As Benoit mentioned earlier, the addition of new pdata fields will cause
> > problems with the DT support already queued for v3.3.
> >
> > This series should be reworked on top of the DT support which Ben has
> > now queued for v3.3 (my branch: for_3.3/i2c/misc)
> 
> Yes will rework it.

Depending on what you plan to do, you'll probably need to coordinate this 
with Tony also.  He's already pulled some of the patches from this series 
into his i2c branch.

The real question is how you plan to handle the device reset function, 
given that the driver should compile on a non-OMAP build (meaning that you 
can't call omap_device*() functions from the driver directly), nor should 
the driver touch the SYSCONFIG register directly.


- Paul

Re: [PATCHv8 4/5] OMAP: I2C: Remove the reset in the init path

[Shubhrajyoti]

On Wednesday 11 January 2012 06:59 PM, Paul Walmsley wrote:
> On Wed, 11 Jan 2012, Shubhrajyoti wrote:
>
>> On Tuesday 10 January 2012 08:56 PM, Kevin Hilman wrote:
>>> "Datta, Shubhrajyoti" <shubhrajyoti-l0cyMroinI0@public.gmane.org> writes:
>>>
>>>> On Tue, Jan 10, 2012 at 11:53 AM, Datta, Shubhrajyoti
>>>> <shubhrajyoti-l0cyMroinI0@public.gmane.org> wrote:
>>>>> On Fri, Dec 16, 2011 at 2:29 PM, Shubhrajyoti <shubhrajyoti-l0cyMroinI0@public.gmane.org> wrote:
>>>>>> On Friday 16 December 2011 02:10 PM, Paul Walmsley wrote:
>>>>>>
>>>>>>> This patch either needs to be acked by Ben with a note that it's okay for
>>>>>>> us to merge through the OMAP tree, or needs to be merged by Ben during the
>>>>>>> 3.4 merge window, after patches 1-3 have reached the mainline tree.
>>>>>> I agree.
>>>>>> Ben do you have any comments .
>>>> If there are no further comments can this be merged also ?
>>> As Benoit mentioned earlier, the addition of new pdata fields will cause
>>> problems with the DT support already queued for v3.3.
>>>
>>> This series should be reworked on top of the DT support which Ben has
>>> now queued for v3.3 (my branch: for_3.3/i2c/misc)
>> Yes will rework it.
> Depending on what you plan to do, you'll probably need to coordinate this 
> with Tony also.  He's already pulled some of the patches from this series 
> into his i2c branch.
>
> The real question is how you plan to handle the device reset function, 
> given that the driver should compile on a non-OMAP build (meaning that you 
> can't call omap_device*() functions from the driver directly), nor should 
> the driver touch the SYSCONFIG register directly.


Paul I thought through it myself not very clear.
- One way is to find some way for the dt to pass function pointer. Or maybe
a flag that  activates a static function / hwmod func.
 
Will give this a little more thought and get back. I have not any answer
right now.

>
> - Paul

Re: [PATCHv8 4/5] OMAP: I2C: Remove the reset in the init path

[Cousson, Benoit]

On 1/11/2012 2:29 PM, Paul Walmsley wrote:
> On Wed, 11 Jan 2012, Shubhrajyoti wrote:
>
>> On Tuesday 10 January 2012 08:56 PM, Kevin Hilman wrote:
>>> "Datta, Shubhrajyoti"<shubhrajyoti-l0cyMroinI0@public.gmane.org>  writes:
>>>
>>>> On Tue, Jan 10, 2012 at 11:53 AM, Datta, Shubhrajyoti
>>>> <shubhrajyoti-l0cyMroinI0@public.gmane.org>  wrote:
>>>>>
>>>>> On Fri, Dec 16, 2011 at 2:29 PM, Shubhrajyoti<shubhrajyoti-l0cyMroinI0@public.gmane.org>  wrote:
>>>>>> On Friday 16 December 2011 02:10 PM, Paul Walmsley wrote:
>>>>>>
>>>>>>> This patch either needs to be acked by Ben with a note that it's okay for
>>>>>>> us to merge through the OMAP tree, or needs to be merged by Ben during the
>>>>>>> 3.4 merge window, after patches 1-3 have reached the mainline tree.
>>>>>> I agree.
>>>>>> Ben do you have any comments .
>>>>>
>>>> If there are no further comments can this be merged also ?
>>> As Benoit mentioned earlier, the addition of new pdata fields will cause
>>> problems with the DT support already queued for v3.3.
>>>
>>> This series should be reworked on top of the DT support which Ben has
>>> now queued for v3.3 (my branch: for_3.3/i2c/misc)
>>
>> Yes will rework it.
>
> Depending on what you plan to do, you'll probably need to coordinate this
> with Tony also.  He's already pulled some of the patches from this series
> into his i2c branch.
>
> The real question is how you plan to handle the device reset function,
> given that the driver should compile on a non-OMAP build (meaning that you
> can't call omap_device*() functions from the driver directly), nor should
> the driver touch the SYSCONFIG register directly.

Something that puzzle me on that point is most architecture does not use 
plateform_device and thus does not have any pdata to hack some custom 
function pointers here an there.
It means that there is probably some better solution to handle that.

AFAIU, the way PPC is handling that is by splitting the driver in a 
generic part and in a platform specific part.

You can then add any OMAP specific hacks in the OMAP specific part while 
keeping most of generic code in a generic driver.

In this case, we just have to implement a small shim that will handle 
the OMAP specific code for the reset. The generic driver will handle the 
rest.

Maybe I'm missing some important details, but that does seems easily 
doable for the i2c case.

Regards,
Benoit

Re: [PATCHv8 4/5] OMAP: I2C: Remove the reset in the init path

[Paul Walmsley]

On Wed, 11 Jan 2012, Shubhrajyoti wrote:

> On Wednesday 11 January 2012 06:59 PM, Paul Walmsley wrote:
>
> > The real question is how you plan to handle the device reset function, 
> > given that the driver should compile on a non-OMAP build (meaning that you 
> > can't call omap_device*() functions from the driver directly), nor should 
> > the driver touch the SYSCONFIG register directly.
> 
> Paul I thought through it myself not very clear.
> - One way is to find some way for the dt to pass function pointer. Or maybe
> a flag that  activates a static function / hwmod func.
>  
> Will give this a little more thought and get back. I have not any answer
> right now.

Well, if it makes you feel any better, no one else seems to have thought 
through it very much either :-(

To me this is essentially implementing a bus-level (or really, 
integration-level) reset of the device.  This seems to be a function that 
many other buses might support.  So let's look at some of the other 
common Linux bus implementations.

PCI has pci_reset_function() in drivers/pci/pci.c, which a handful of 
PCI-specific drivers call. USB has usb_reset_device() in 
drivers/usb/core/hub.c, also called by a handful of devices.

So there's precedent for the use case.

Incidentally, this is the kind of thing that ideally should be supported 
via a common function pointer in struct bus_type or maybe struct device. 
So rather than the driver calling pci_reset_function() or 
usb_reset_device(), it seems preferable for the driver to do something 
like:

    if (dev->bus->device_reset)
        dev->bus->device_reset(dev);

It seems best to avoid binding a driver tightly to a particular bus 
whenever possible.

...

Now to consider something like this from an OMAP perspective.

In a rough approximation of an ideal world, much of the hwmod and 
omap_device code would live somewhere like drivers/omap_bus/.  Our devices 
would be omap_device based rather than platform_device or of_device based.  
We could export omap_device_reset() and then the I2C driver could call 
this directly.  We're at least a year away from this (if ever): we need 
PRM, CM, SCM drivers; we'd need to have a real omap_bus and omap_device 
that our drivers would use, unlike the faux omap_device we have now.  
Maybe the latter is never going to happen now due to the DT situation.
So any solution that would depend on these steps as prerequisites might 
easily be delayed for an infinite amount of time.

But let's say that a device_reset() function pointer existed inside the 
common struct bus_type in include/linux/device.h, as discussed above.  
>From an OMAP perspective, we could simply set that to point to 
omap_device_reset() in the OMAP-specific device creation code or DT 
notifier callback code.  PCI and USB code could set it to point to their 
own reset functions.  Something like the code fragment above would be 
recommended for all device drivers to do an integration device reset.

Anyway, just an idea.


- Paul

Re: [PATCHv8 4/5] OMAP: I2C: Remove the reset in the init path

[Paul Walmsley]

On Wed, 11 Jan 2012, Cousson, Benoit wrote:

> Something that puzzle me on that point is most architecture does not use
> plateform_device and thus does not have any pdata to hack some custom function
> pointers here an there.
> It means that there is probably some better solution to handle that.

PCI and USB have their own bus-level device reset functions that are 
exported from bus code that lives under drivers/.  In a similar fashion, 
we could export omap_device_reset() from our arch/arm/plat-omap code.  
But that would require that arch/arm/plat-omap and arch/arm/mach-omap2 
code to be compiled for any ARM build that includes i2c-omap.c.  This 
would be good to avoid, under the principle that drivers should be 
buildable for any architecture.  (This is also why we wish to remove 
omap_{read,write}*() and cpu_is_omap() from any drivers which contain 
those.)

The intention a few years ago was to eventually create a a real omap_bus 
and omap_device, with the bus/device integration code living under 
drivers.  In such a situation, we'd be able to use something like the 
PCI/USB approach cleanly, since there wouldn't be dependencies to arch/ 
code.

> AFAIU, the way PPC is handling that is by splitting the driver in a generic
> part and in a platform specific part.
>
> You can then add any OMAP specific hacks in the OMAP specific part while 
> keeping most of generic code in a generic driver.
> 
> In this case, we just have to implement a small shim that will handle the OMAP
> specific code for the reset. The generic driver will handle the rest.
> 
> Maybe I'm missing some important details, but that does seems easily doable
> for the i2c case.

Hmm.  How do those drivers call the platform-specific code in a way that 
allows those drivers to be built for non-PPC platforms?


- Paul

Re: [PATCHv8 4/5] OMAP: I2C: Remove the reset in the init path

[Cousson, Benoit]

On 1/11/2012 4:22 PM, Paul Walmsley wrote:
> On Wed, 11 Jan 2012, Cousson, Benoit wrote:
>
>> Something that puzzle me on that point is most architecture does not use
>> plateform_device and thus does not have any pdata to hack some custom function
>> pointers here an there.
>> It means that there is probably some better solution to handle that.
>
> PCI and USB have their own bus-level device reset functions that are
> exported from bus code that lives under drivers/.  In a similar fashion,
> we could export omap_device_reset() from our arch/arm/plat-omap code.
> But that would require that arch/arm/plat-omap and arch/arm/mach-omap2
> code to be compiled for any ARM build that includes i2c-omap.c.  This
> would be good to avoid, under the principle that drivers should be
> buildable for any architecture.  (This is also why we wish to remove
> omap_{read,write}*() and cpu_is_omap() from any drivers which contain
> those.)
>
> The intention a few years ago was to eventually create a a real omap_bus
> and omap_device, with the bus/device integration code living under
> drivers.  In such a situation, we'd be able to use something like the
> PCI/USB approach cleanly, since there wouldn't be dependencies to arch/
> code.

For reset, maybe the custom bus is a better approach, since it is 
something that is handled by the OMAP core infrastructure.
I was referring more to all the other function pointer we have today in 
driver like MMC, DSS that does not necessarily belong to a OMAP core stuff.

>> AFAIU, the way PPC is handling that is by splitting the driver in a generic
>> part and in a platform specific part.
>>
>> You can then add any OMAP specific hacks in the OMAP specific part while
>> keeping most of generic code in a generic driver.
>>
>> In this case, we just have to implement a small shim that will handle the OMAP
>> specific code for the reset. The generic driver will handle the rest.
>>
>> Maybe I'm missing some important details, but that does seems easily doable
>> for the i2c case.
>
> Hmm.  How do those drivers call the platform-specific code in a way that
> allows those drivers to be built for non-PPC platforms?

Well, the point is that a lot of that code should not be in the omap 
arch directory at the first place.
For example, the MMC is using extensively function pointers because of 
the control module dependency.
Having a control module driver will allow stacking the drivers without 
having to get any code in mach-omap directory.

Otherwise, there is as well a dedicated drivers/platform directory that 
can store some platform specific drivers. So far it looks like it is 
used for x86 board kind of drivers only.

Regards,
Benoit

Re: [PATCHv8 4/5] OMAP: I2C: Remove the reset in the init path

[Paul Walmsley]

On Thu, 12 Jan 2012, Cousson, Benoit wrote:

> On 1/11/2012 4:22 PM, Paul Walmsley wrote:
> > On Wed, 11 Jan 2012, Cousson, Benoit wrote:
> > 
> > > Something that puzzle me on that point is most architecture does not use
> > > plateform_device and thus does not have any pdata to hack some custom
> > > function
> > > pointers here an there.
> > > It means that there is probably some better solution to handle that.
> > 
> > PCI and USB have their own bus-level device reset functions that are
> > exported from bus code that lives under drivers/.  In a similar fashion,
> > we could export omap_device_reset() from our arch/arm/plat-omap code.
> > But that would require that arch/arm/plat-omap and arch/arm/mach-omap2
> > code to be compiled for any ARM build that includes i2c-omap.c.  This
> > would be good to avoid, under the principle that drivers should be
> > buildable for any architecture.  (This is also why we wish to remove
> > omap_{read,write}*() and cpu_is_omap() from any drivers which contain
> > those.)
> > 
> > The intention a few years ago was to eventually create a a real omap_bus
> > and omap_device, with the bus/device integration code living under
> > drivers.  In such a situation, we'd be able to use something like the
> > PCI/USB approach cleanly, since there wouldn't be dependencies to arch/
> > code.
> 
> For reset, maybe the custom bus is a better approach, since it is something
> that is handled by the OMAP core infrastructure.
> I was referring more to all the other function pointer we have today in driver
> like MMC, DSS that does not necessarily belong to a OMAP core stuff.

Yes, that's right.  In general, cross-device dependencies with no 
infrastructural consequences should be handled by EXPORT_SYMBOL() between 
driver code.


- Paul