| Message ID | 1340029828-20751-1-git-send-email-shubhrajyoti@ti.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Jun 18, 2012 at 08:00:25PM +0530, Shubhrajyoti D wrote: > From: Felipe Balbi <balbi@ti.com> > > trivial patch, no functional changes Wrong. This patch does change some behaviour, are you aware of that? So, please check if the side-effect is affectong the code and adapt the commit message, if everything is okay. > > Signed-off-by: Felipe Balbi <balbi@ti.com> > Reviewed-by : Santosh Shilimkar <santosh.shilimkar@ti.com> > Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> > --- > drivers/i2c/busses/i2c-omap.c | 6 ++---- > 1 files changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > index e24eb1f..080193a 100644 > --- a/drivers/i2c/busses/i2c-omap.c > +++ b/drivers/i2c/busses/i2c-omap.c > @@ -844,8 +844,7 @@ complete: > OMAP_I2C_BUFSTAT_REG) > >> 8) & 0x3F; > } > - while (num_bytes) { > - num_bytes--; > + while (num_bytes--) { > w = omap_i2c_read_reg(dev, OMAP_I2C_DATA_REG); > if (dev->buf_len) { > *dev->buf++ = w; > @@ -887,8 +886,7 @@ complete: > OMAP_I2C_BUFSTAT_REG) > & 0x3F; > } > - while (num_bytes) { > - num_bytes--; > + while (num_bytes--) { > w = 0; > if (dev->buf_len) { > w = *dev->buf++; > -- > 1.7.5.4 >
On Mon, Jun 18, 2012 at 08:00:26PM +0530, Shubhrajyoti D wrote: > From: Felipe Balbi <balbi@ti.com> > > trivial patch, no functional changes. This patch seems to be correct, but it is not trivial. In fact, it is pretty easy to miss a "!" here which can cause subtle bugs. > > Signed-off-by: Felipe Balbi <balbi@ti.com> > Reviewed-by : Santosh Shilimkar <santosh.shilimkar@ti.com> > Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> > --- > drivers/i2c/busses/i2c-omap.c | 63 ++++++++++++++++++++--------------------- > 1 files changed, 31 insertions(+), 32 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > index 080193a..0e0ab8f 100644 > --- a/drivers/i2c/busses/i2c-omap.c > +++ b/drivers/i2c/busses/i2c-omap.c > @@ -845,22 +845,7 @@ complete: > >> 8) & 0x3F; > } > while (num_bytes--) { > - w = omap_i2c_read_reg(dev, OMAP_I2C_DATA_REG); > - if (dev->buf_len) { > - *dev->buf++ = w; > - dev->buf_len--; > - /* > - * Data reg in 2430, omap3 and > - * omap4 is 8 bit wide > - */ > - if (dev->flags & > - OMAP_I2C_FLAG_16BIT_DATA_REG) { > - if (dev->buf_len) { > - *dev->buf++ = w >> 8; > - dev->buf_len--; > - } > - } > - } else { > + if (!dev->buf_len) { > if (stat & OMAP_I2C_STAT_RRDY) > dev_err(dev->dev, > "RRDY IRQ while no data" > @@ -871,6 +856,21 @@ complete: > " requested\n"); > break; > } > + > + w = omap_i2c_read_reg(dev, OMAP_I2C_DATA_REG); > + *dev->buf++ = w; > + dev->buf_len--; > + /* > + * Data reg in 2430, omap3 and > + * omap4 is 8 bit wide > + */ > + if (dev->flags & > + OMAP_I2C_FLAG_16BIT_DATA_REG) { > + if (dev->buf_len) { > + *dev->buf++ = w >> 8; > + dev->buf_len--; > + } > + } > } > omap_i2c_ack_stat(dev, > stat & (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR)); > @@ -887,22 +887,7 @@ complete: > & 0x3F; > } > while (num_bytes--) { > - w = 0; > - if (dev->buf_len) { > - w = *dev->buf++; > - dev->buf_len--; > - /* > - * Data reg in 2430, omap3 and > - * omap4 is 8 bit wide > - */ > - if (dev->flags & > - OMAP_I2C_FLAG_16BIT_DATA_REG) { > - if (dev->buf_len) { > - w |= *dev->buf++ << 8; > - dev->buf_len--; > - } > - } > - } else { > + if (!dev->buf_len) { > if (stat & OMAP_I2C_STAT_XRDY) > dev_err(dev->dev, > "XRDY IRQ while no " > @@ -914,6 +899,20 @@ complete: > break; > } > > + w = *dev->buf++; > + dev->buf_len--; > + /* > + * Data reg in 2430, omap3 and > + * omap4 is 8 bit wide > + */ > + if (dev->flags & > + OMAP_I2C_FLAG_16BIT_DATA_REG) { > + if (dev->buf_len) { > + w |= *dev->buf++ << 8; > + dev->buf_len--; > + } > + } > + > if ((dev->errata & I2C_OMAP_ERRATA_I462) && > errata_omap3_i462(dev, &stat, &err)) > goto complete; > -- > 1.7.5.4 >
On Mon, Jun 18, 2012 at 08:00:28PM +0530, Shubhrajyoti D wrote: > From: Felipe Balbi <balbi@ti.com> > > stat & BIT(1) is the same as BIT(1), Not true. I'd guess you are missing some context in the patch description. > so let's > simplify things a bit by removing "stat &" from > all omap_i2c_ack_stat() calls. > > Signed-off-by: Felipe Balbi <balbi@ti.com> > Reviewed-by : Santosh Shilimkar <santosh.shilimkar@ti.com> > Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> > --- > drivers/i2c/busses/i2c-omap.c | 19 ++++++++++--------- > 1 files changed, 10 insertions(+), 9 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > index 6a79089..bac6305 100644 > --- a/drivers/i2c/busses/i2c-omap.c > +++ b/drivers/i2c/busses/i2c-omap.c > @@ -763,7 +763,7 @@ static int errata_omap3_i462(struct omap_i2c_dev *dev, u16 *stat, int *err) > > while (--timeout && !(*stat & OMAP_I2C_STAT_XUDF)) { > if (*stat & (OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) { > - omap_i2c_ack_stat(dev, *stat & (OMAP_I2C_STAT_XRDY | > + omap_i2c_ack_stat(dev, (OMAP_I2C_STAT_XRDY | > OMAP_I2C_STAT_XDR)); > return -ETIMEDOUT; > } > @@ -824,10 +824,11 @@ complete: > */ > if (stat & (OMAP_I2C_STAT_ARDY | OMAP_I2C_STAT_NACK | > OMAP_I2C_STAT_AL)) { > - omap_i2c_ack_stat(dev, stat & > - (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR | > - OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR | > - OMAP_I2C_STAT_ARDY)); > + omap_i2c_ack_stat(dev, (OMAP_I2C_STAT_RRDY | > + OMAP_I2C_STAT_RDR | > + OMAP_I2C_STAT_XRDY | > + OMAP_I2C_STAT_XDR | > + OMAP_I2C_STAT_ARDY)); > omap_i2c_complete_cmd(dev, err); > return IRQ_HANDLED; > } > @@ -874,8 +875,8 @@ complete: > } > } > } > - omap_i2c_ack_stat(dev, > - stat & (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR)); > + omap_i2c_ack_stat(dev, (OMAP_I2C_STAT_RRDY | > + OMAP_I2C_STAT_RDR)); > continue; > } > > @@ -922,8 +923,8 @@ complete: > > omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w); > } > - omap_i2c_ack_stat(dev, > - stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR)); > + omap_i2c_ack_stat(dev, (OMAP_I2C_STAT_XRDY | > + OMAP_I2C_STAT_XDR)); > continue; > } > > -- > 1.7.5.4 >
On Mon, Jun 18, 2012 at 08:00:15PM +0530, Shubhrajyoti D wrote: > > The patch series does the following > > - I2C register restore only if context if the context is lost > - Bus busy recovery mechanism. > - the reset is not done in init. > - Adds a patch to use devm_* functions > - Adds a pdata function pointer to do context save restore > - Split the omap_i2c_isr to increase readability > - Make the i2c use SET_RUNTIME_PM_OPS > - Use INIT_COMPLETION instead of init_completion > - Dropping a cleanup and taking few patchs Felipe's series as it may > not be needed. Patches look mostly okay for me without knowing the platform too much. For that reason, I'd be really appreciating Tested-by or even Reviewed-by tags! > This series mainly is the cleanups rebased on > i2c-embedded/for-next branch. Thanks. Regards, Wolfram
On Mon, Jun 18, 2012 at 9:00 PM, Wolfram Sang <w.sang@pengutronix.de> wrote: > On Mon, Jun 18, 2012 at 08:00:28PM +0530, Shubhrajyoti D wrote: >> From: Felipe Balbi <balbi@ti.com> >> >> stat & BIT(1) is the same as BIT(1), > > Not true. I'd guess you are missing some context in the patch > description. See the explanation , hope I understood your concern correctly. http://www.mail-archive.com/linux-omap@vger.kernel.org/msg70277.html > >> so let's >> simplify things a bit by removing "stat &" from >> all omap_i2c_ack_stat() calls. >> >> Signed-off-by: Felipe Balbi <balbi@ti.com> >> Reviewed-by : Santosh Shilimkar <santosh.shilimkar@ti.com> >> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> >> --- >> drivers/i2c/busses/i2c-omap.c | 19 ++++++++++--------- >> 1 files changed, 10 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c >> index 6a79089..bac6305 100644 >> --- a/drivers/i2c/busses/i2c-omap.c >> +++ b/drivers/i2c/busses/i2c-omap.c >> @@ -763,7 +763,7 @@ static int errata_omap3_i462(struct omap_i2c_dev *dev, u16 *stat, int *err) >> >> while (--timeout && !(*stat & OMAP_I2C_STAT_XUDF)) { >> if (*stat & (OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) { >> - omap_i2c_ack_stat(dev, *stat & (OMAP_I2C_STAT_XRDY | >> + omap_i2c_ack_stat(dev, (OMAP_I2C_STAT_XRDY | >> OMAP_I2C_STAT_XDR)); >> return -ETIMEDOUT; >> } >> @@ -824,10 +824,11 @@ complete: >> */ >> if (stat & (OMAP_I2C_STAT_ARDY | OMAP_I2C_STAT_NACK | >> OMAP_I2C_STAT_AL)) { >> - omap_i2c_ack_stat(dev, stat & >> - (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR | >> - OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR | >> - OMAP_I2C_STAT_ARDY)); >> + omap_i2c_ack_stat(dev, (OMAP_I2C_STAT_RRDY | >> + OMAP_I2C_STAT_RDR | >> + OMAP_I2C_STAT_XRDY | >> + OMAP_I2C_STAT_XDR | >> + OMAP_I2C_STAT_ARDY)); >> omap_i2c_complete_cmd(dev, err); >> return IRQ_HANDLED; >> } >> @@ -874,8 +875,8 @@ complete: >> } >> } >> } >> - omap_i2c_ack_stat(dev, >> - stat & (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR)); >> + omap_i2c_ack_stat(dev, (OMAP_I2C_STAT_RRDY | >> + OMAP_I2C_STAT_RDR)); >> continue; >> } >> >> @@ -922,8 +923,8 @@ complete: >> >> omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w); >> } >> - omap_i2c_ack_stat(dev, >> - stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR)); >> + omap_i2c_ack_stat(dev, (OMAP_I2C_STAT_XRDY | >> + OMAP_I2C_STAT_XDR)); >> continue; >> } >> >> -- >> 1.7.5.4 >> > > -- > Pengutronix e.K. | Wolfram Sang | > Industrial Linux Solutions | http://www.pengutronix.de/ |
On Tue, Jun 19, 2012 at 02:50:23PM +0530, Shubhrajyoti Datta wrote: > On Mon, Jun 18, 2012 at 9:00 PM, Wolfram Sang <w.sang@pengutronix.de> wrote: > > On Mon, Jun 18, 2012 at 08:00:28PM +0530, Shubhrajyoti D wrote: > >> From: Felipe Balbi <balbi@ti.com> > >> > >> stat & BIT(1) is the same as BIT(1), > > > > Not true. I'd guess you are missing some context in the patch > > description. > > > See the explanation , hope I understood your concern correctly. > > http://www.mail-archive.com/linux-omap@vger.kernel.org/msg70277.html The code may be right, but as Russell pointed out, the patch description is very weak. I could read the mail now to understand, but I prefer to wait for a better patch description. Because I need to ensure other people reading the git history later will understand easily without having to search the web. Regards, Wolfram
* Shubhrajyoti D <shubhrajyoti@ti.com> [120618 07:35]: > The reset in the driver at init is not needed anymore as the > following patch has removed the HWMOD_INIT_NO_RESET flag. > 6d3c55f [OMAP: hwmod: fix the i2c-reset timeout during bootup] > > This patch does the following > -removes the reset from the probe and implements a omap_i2c_reset > function to reset. > - Reset is removed from omap_i2c_init, which was called > not only during probe, but also after time out and error handling. > omap_i2c_reset is added in those places to effect the reset. See the comments regarding driver specific resets in hwmod code. The way to set this up is to have a shared inline function in i2c-omap.h that both the driver and hwmod code can use. Eventually hwmod code will do the reset only in late initcall if no driver is loaded for the device in question. Tony
* Shubhrajyoti D <shubhrajyoti@ti.com> [120618 07:35]: > From: Jon Hunter <jon-hunter@ti.com> > > The OMAP3530 is based upon the same silicon as the OMAP3430 and so the I2C > revision is the same for 3430 and 3530. However, the OMAP3630 device has the > same I2C revision as OMAP4. Correct the revision definition to reflect this. > > This patch is based on work done by Jon Hunter <jon-hunter@ti.com> > Changes from his patch > - Update OMAP_I2C_REV_ON_3430 also to reflect that it is same as 3530 ... > /* timeout waiting for the controller to respond */ > #define OMAP_I2C_TIMEOUT (msecs_to_jiffies(1000)) > @@ -298,7 +298,7 @@ static int omap_i2c_reset(struct omap_i2c_dev *dev) > omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG, > SYSC_AUTOIDLE_MASK); > > - } else if (dev->rev >= OMAP_I2C_REV_ON_3430) { > + } else if (dev->rev >= OMAP_I2C_REV_ON_3430_3530) { > dev->syscstate = SYSC_AUTOIDLE_MASK; > dev->syscstate |= SYSC_ENAWAKEUP_MASK; > dev->syscstate |= (SYSC_IDLEMODE_SMART << Having to patch all over the place for these revision defines leads into unmaintainable code as new SoCs get added. Please instead just check the revision once during init, then set up some feature bits like I2C_OMAP_NEEDS_XYZ_RESET that the runtime code can check. > @@ -1055,7 +1055,7 @@ omap_i2c_probe(struct platform_device *pdev) > if (dev->flags & OMAP_I2C_FLAG_APPLY_ERRATA_I207) > dev->errata |= I2C_OMAP_ERRATA_I207; > > - if (dev->rev <= OMAP_I2C_REV_ON_3430) > + if (dev->rev <= OMAP_I2C_REV_ON_3430_3530) > dev->errata |= I2C_OMAP_ERRATA_I462; > > if (!(dev->flags & OMAP_I2C_FLAG_NO_FIFO)) { > @@ -1073,7 +1073,7 @@ omap_i2c_probe(struct platform_device *pdev) > > dev->fifo_size = (dev->fifo_size / 2); > > - if (dev->rev >= OMAP_I2C_REV_ON_3530_4430) > + if (dev->rev >= OMAP_I2C_REV_ON_3630_4430) > dev->b_hw = 0; /* Disable hardware fixes */ > else > dev->b_hw = 1; /* Enable hardware fixes */ That way the code does not need to change when new SoC revisions get added. Tony
On Wednesday 20 June 2012 04:02 PM, Tony Lindgren wrote: > * Shubhrajyoti D <shubhrajyoti@ti.com> [120618 07:35]: >> From: Jon Hunter <jon-hunter@ti.com> >> >> The OMAP3530 is based upon the same silicon as the OMAP3430 and so the I2C >> revision is the same for 3430 and 3530. However, the OMAP3630 device has the >> same I2C revision as OMAP4. Correct the revision definition to reflect this. >> >> This patch is based on work done by Jon Hunter <jon-hunter@ti.com> >> Changes from his patch >> - Update OMAP_I2C_REV_ON_3430 also to reflect that it is same as 3530 > ... >> /* timeout waiting for the controller to respond */ >> #define OMAP_I2C_TIMEOUT (msecs_to_jiffies(1000)) >> @@ -298,7 +298,7 @@ static int omap_i2c_reset(struct omap_i2c_dev *dev) >> omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG, >> SYSC_AUTOIDLE_MASK); >> >> - } else if (dev->rev >= OMAP_I2C_REV_ON_3430) { >> + } else if (dev->rev >= OMAP_I2C_REV_ON_3430_3530) { >> dev->syscstate = SYSC_AUTOIDLE_MASK; >> dev->syscstate |= SYSC_ENAWAKEUP_MASK; >> dev->syscstate |= (SYSC_IDLEMODE_SMART << > Having to patch all over the place for these revision defines leads > into unmaintainable code as new SoCs get added. > > Please instead just check the revision once during init, then set > up some feature bits like I2C_OMAP_NEEDS_XYZ_RESET that the runtime > code can check. Even now at probe dev->rev is set by reading the rev register. The (macro)name used to identify is only changed. Also all the ips need reset the difference is that 2430 has less bits eg only autoidle and reset. So a flag needs reset may not be meaningful. > > >> @@ -1055,7 +1055,7 @@ omap_i2c_probe(struct platform_device *pdev) >> if (dev->flags & OMAP_I2C_FLAG_APPLY_ERRATA_I207) >> dev->errata |= I2C_OMAP_ERRATA_I207; >> >> - if (dev->rev <= OMAP_I2C_REV_ON_3430) >> + if (dev->rev <= OMAP_I2C_REV_ON_3430_3530) >> dev->errata |= I2C_OMAP_ERRATA_I462; >> >> if (!(dev->flags & OMAP_I2C_FLAG_NO_FIFO)) { >> @@ -1073,7 +1073,7 @@ omap_i2c_probe(struct platform_device *pdev) >> >> dev->fifo_size = (dev->fifo_size / 2); >> >> - if (dev->rev >= OMAP_I2C_REV_ON_3530_4430) >> + if (dev->rev >= OMAP_I2C_REV_ON_3630_4430) >> dev->b_hw = 0; /* Disable hardware fixes */ >> else >> dev->b_hw = 1; /* Enable hardware fixes */ > That way the code does not need to change when new SoC revisions > get added. I think it might still not need to change because of >= . I have just renamed OMAP_I2C_REV_ON_3530_4430 as 3530 rev is not the same as 4430 that the name implies. > > Tony
* Shubhrajyoti <shubhrajyoti@ti.com> [120620 06:06]: > On Wednesday 20 June 2012 04:02 PM, Tony Lindgren wrote: > > * Shubhrajyoti D <shubhrajyoti@ti.com> [120618 07:35]: > >> From: Jon Hunter <jon-hunter@ti.com> > >> > >> The OMAP3530 is based upon the same silicon as the OMAP3430 and so the I2C > >> revision is the same for 3430 and 3530. However, the OMAP3630 device has the > >> same I2C revision as OMAP4. Correct the revision definition to reflect this. > >> > >> This patch is based on work done by Jon Hunter <jon-hunter@ti.com> > >> Changes from his patch > >> - Update OMAP_I2C_REV_ON_3430 also to reflect that it is same as 3530 > > ... > >> /* timeout waiting for the controller to respond */ > >> #define OMAP_I2C_TIMEOUT (msecs_to_jiffies(1000)) > >> @@ -298,7 +298,7 @@ static int omap_i2c_reset(struct omap_i2c_dev *dev) > >> omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG, > >> SYSC_AUTOIDLE_MASK); > >> > >> - } else if (dev->rev >= OMAP_I2C_REV_ON_3430) { > >> + } else if (dev->rev >= OMAP_I2C_REV_ON_3430_3530) { > >> dev->syscstate = SYSC_AUTOIDLE_MASK; > >> dev->syscstate |= SYSC_ENAWAKEUP_MASK; > >> dev->syscstate |= (SYSC_IDLEMODE_SMART << > > Having to patch all over the place for these revision defines leads > > into unmaintainable code as new SoCs get added. > > > > Please instead just check the revision once during init, then set > > up some feature bits like I2C_OMAP_NEEDS_XYZ_RESET that the runtime > > code can check. > Even now at probe > > dev->rev is set by reading the rev register. > > The (macro)name used to identify is only changed. > > Also all the ips need reset the difference is that 2430 has less bits eg only autoidle and reset. > So a flag needs reset may not be meaningful. Hmm OK so this is a cosmetic/readability fix.. ..but your earlier patch now spreads the checking of revision to the new non-init function omap_i2c_reset. Why don't you just do this cosmetic/readability rename fix before your 03/13 patch? Then set the already existing OMAP_I2C_FLAG_RESET_REGS_POSTIDLE during init for some revisions and use that instead of the rev check in omap_i2c_reset? Tony
On Wed, Jun 20, 2012 at 7:44 PM, Tony Lindgren <tony@atomide.com> wrote: > * Shubhrajyoti <shubhrajyoti@ti.com> [120620 06:06]: >> On Wednesday 20 June 2012 04:02 PM, Tony Lindgren wrote: >> > * Shubhrajyoti D <shubhrajyoti@ti.com> [120618 07:35]: >> >> From: Jon Hunter <jon-hunter@ti.com> >> >> >> >> The OMAP3530 is based upon the same silicon as the OMAP3430 and so the I2C >> >> revision is the same for 3430 and 3530. However, the OMAP3630 device has the >> >> same I2C revision as OMAP4. Correct the revision definition to reflect this. >> >> >> >> This patch is based on work done by Jon Hunter <jon-hunter@ti.com> >> >> Changes from his patch >> >> - Update OMAP_I2C_REV_ON_3430 also to reflect that it is same as 3530 >> > ... >> >> /* timeout waiting for the controller to respond */ >> >> #define OMAP_I2C_TIMEOUT (msecs_to_jiffies(1000)) >> >> @@ -298,7 +298,7 @@ static int omap_i2c_reset(struct omap_i2c_dev *dev) >> >> omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG, >> >> SYSC_AUTOIDLE_MASK); >> >> >> >> - } else if (dev->rev >= OMAP_I2C_REV_ON_3430) { >> >> + } else if (dev->rev >= OMAP_I2C_REV_ON_3430_3530) { >> >> dev->syscstate = SYSC_AUTOIDLE_MASK; >> >> dev->syscstate |= SYSC_ENAWAKEUP_MASK; >> >> dev->syscstate |= (SYSC_IDLEMODE_SMART << >> > Having to patch all over the place for these revision defines leads >> > into unmaintainable code as new SoCs get added. >> > >> > Please instead just check the revision once during init, then set >> > up some feature bits like I2C_OMAP_NEEDS_XYZ_RESET that the runtime >> > code can check. >> Even now at probe >> >> dev->rev is set by reading the rev register. >> >> The (macro)name used to identify is only changed. >> >> Also all the ips need reset the difference is that 2430 has less bits eg only autoidle and reset. >> So a flag needs reset may not be meaningful. > > Hmm OK so this is a cosmetic/readability fix.. Yes > > ..but your earlier patch now spreads the checking of revision > to the new non-init function omap_i2c_reset. > > Why don't you just do this cosmetic/readability rename fix > before your 03/13 patch? OK thats doable.I can reorder the patch. > Then set the already existing > OMAP_I2C_FLAG_RESET_REGS_POSTIDLE during init for some > revisions and use that instead of the rev check in > omap_i2c_reset? OMAP_I2C_FLAG_RESET_REGS_POSTIDLE is a hwmod flag whose intention is to find know if it can lose context after idle. The rev check that we have is because post reset( triggered by driver) to know if we have to restore only autoidle or other bits like clockactivity sidle , enable wakeup . > > Tony > > -- > To unsubscribe from this list: send the line "unsubscribe linux-i2c" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 20 June 2012 03:59 PM, Tony Lindgren wrote: > See the comments regarding driver specific resets in hwmod code. you mean omap_hwmod.c > > The way to set this up is to have a shared inline function in > i2c-omap.h that both the driver and hwmod code can use. hwmod reset function uses oh (omap_hwmod). How could the driver also pass oh ? Could we keep a local copy in driver data? > Eventually hwmod code will do the reset only in late initcall > if no driver is loaded for the device in question.
* Shubhrajyoti <shubhrajyoti@ti.com> [120621 00:08]: > On Wednesday 20 June 2012 03:59 PM, Tony Lindgren wrote: > > See the comments regarding driver specific resets in hwmod code. > you mean omap_hwmod.c > > > > The way to set this up is to have a shared inline function in > > i2c-omap.h that both the driver and hwmod code can use. > hwmod reset function uses oh (omap_hwmod). > > How could the driver also pass oh ? > Could we keep a local copy in driver data? > > Eventually hwmod code will do the reset only in late initcall > > if no driver is loaded for the device in question. > There's no need for the driver to know anything about oh. The driver just needs to know the iobase. Tony
On Thursday 21 June 2012 12:50 PM, Tony Lindgren wrote: > * Shubhrajyoti <shubhrajyoti@ti.com> [120621 00:08]: >> On Wednesday 20 June 2012 03:59 PM, Tony Lindgren wrote: >>> See the comments regarding driver specific resets in hwmod code. >> you mean omap_hwmod.c >>> The way to set this up is to have a shared inline function in >>> i2c-omap.h that both the driver and hwmod code can use. >> hwmod reset function uses oh (omap_hwmod). >> >> How could the driver also pass oh ? >> Could we keep a local copy in driver data? >>> Eventually hwmod code will do the reset only in late initcall >>> if no driver is loaded for the device in question. > There's no need for the driver to know anything about oh. > The driver just needs to know the iobase. I will rework to make the hwmod and driver use the same function. In the meanwhile I will send a minimal/ remaining cleanups/ fixes so that it might get some time to bake in the next branch. > > Tony
* Shubhrajyoti <shubhrajyoti@ti.com> [120621 02:35]: > On Thursday 21 June 2012 12:50 PM, Tony Lindgren wrote: > > * Shubhrajyoti <shubhrajyoti@ti.com> [120621 00:08]: > >> On Wednesday 20 June 2012 03:59 PM, Tony Lindgren wrote: > >>> See the comments regarding driver specific resets in hwmod code. > >> you mean omap_hwmod.c > >>> The way to set this up is to have a shared inline function in > >>> i2c-omap.h that both the driver and hwmod code can use. > >> hwmod reset function uses oh (omap_hwmod). > >> > >> How could the driver also pass oh ? > >> Could we keep a local copy in driver data? > >>> Eventually hwmod code will do the reset only in late initcall > >>> if no driver is loaded for the device in question. > > There's no need for the driver to know anything about oh. > > The driver just needs to know the iobase. > I will rework to make the hwmod and driver use the same function. > In the meanwhile I will send a minimal/ remaining cleanups/ fixes so that > it might get some time to bake in the next branch. OK thanks! Tony
The patch series does the following - I2C register restore only if context if the context is lost - Bus busy recovery mechanism. - the reset is not done in init. - Adds a patch to use devm_* functions - Adds a pdata function pointer to do context save restore - Split the omap_i2c_isr to increase readability - Make the i2c use SET_RUNTIME_PM_OPS - Use INIT_COMPLETION instead of init_completion - Dropping a cleanup and taking few patchs Felipe's series as it may not be needed. This applies on Wolfram's i2c-embedded/for-next branch. Functional testing on omap4sdp and omap3sdp. Previous discurssions http://www.spinics.net/lists/linux-i2c/msg07748.html This series mainly is the cleanups rebased on i2c-embedded/for-next branch. The following changes since commit 0f009a914b40be8786fa67b1f4345cacc263b48c: i2c: tegra: make all resource allocation through devm_* (2012-06-13 16:01:38 +0200) are available in the git repository at: git://gitorious.org/linus-tree/linus-tree.git for_next/i2c_omap Felipe Balbi (4): I2C: OMAP: simplify num_bytes handling I2C: OMAP: decrease indentation level on data handling I2C: OMAP: add blank lines I2C: OMAP: simplify omap_i2c_ack_stat() Jon Hunter (1): I2C: OMAP: Correct I2C revision for OMAP3 Shubhrajyoti D (7): I2C: OMAP: I2C register restore only if context is lost I2C: OMAP: Remove the definition of SYSS_RESETDONE_MASK I2C: OMAP: Remove reset at init I2C: OMAP: Optimise the remove code I2C: OMAP: use devm_* functions I2C: OMAP: Use SET_RUNTIME_PM_OPS I2C: OMAP: Do not initialise the completion everytime Vikram Pandita (1): I2C: OMAP: Recover from Bus Busy condition arch/arm/plat-omap/i2c.c | 3 + drivers/i2c/busses/i2c-omap.c | 261 +++++++++++++++++++++++----------------- include/linux/i2c-omap.h | 1 + 3 files changed, 154 insertions(+), 111 deletions(-)