Pull-request

Comments

The patch series does the following
- Warn fixes if CONFIG_PM_RUNTIME is not selected.
- In case of i2c remove register access was done without any
get_sync fix the same.
- Folds a patch from Tasslehoff to prevent any merge conflicts.
- Prevents the XDUF flag to be set if the underflow condition is not met.
- As per discussion in [1] .Adds a patch to rename the 1p153 errata and
use the unique id instead as the section number in the recent errata
docs has changed.
v9:
Fix the comments from Wolfram Sang
[1] http://www.spinics.net/lists/linux-i2c/msg07607.html
Tested on omap4sdp and omap3sdp.
The following changes since commit b821861b905a79f71746945237968c3382d99adc:
Merge tag 'ktest-for-v3.4-rc5' of git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-ktest (2012-05-01 19:43:34 -0700)
are available in the git repository at:
git://gitorious.org/linus-tree/linus-tree.git i2c_omap-fixes
Shubhrajyoti D (9):
I2C: OMAP: make omap_i2c_unidle/idle functions depend on CONFIG_PM_RUNTIME
I2C: OMAP: Fix the mismatch of pm_runtime enable and disable
I2C: OMAP: Fix the interrupt clearing in OMAP4
I2C: OMAP: Prevent the register access after pm_runtime_put in probe
I2C: OMAP: Don't check if wait_for_completion_timeout() returns less than zero
I2C: OMAP: Fix the crash in i2c remove
I2C: OMAP: Handle error check for pm runtime
I2C: OMAP: Do not set the XUDF(Transmit underflow) if the underflow is not reached
I2C: OMAP: Rename the 1p153 to the erratum id i462
Tasslehoff Kjappfot (1):
I2C: OMAP: prevent the overwrite of the errata flags
drivers/i2c/busses/i2c-omap.c | 127 ++++++++++++++++++++---------------------
1 files changed, 62 insertions(+), 65 deletions(-)

On Wed, May 02, 2012 at 08:02:05PM +0530, Shubhrajyoti D wrote:
> > The patch series does the following> > - Warn fixes if CONFIG_PM_RUNTIME is not selected.> - In case of i2c remove register access was done without any> get_sync fix the same.> - Folds a patch from Tasslehoff to prevent any merge conflicts.> - Prevents the XDUF flag to be set if the underflow condition is not met.> - As per discussion in [1] .Adds a patch to rename the 1p153 errata and> use the unique id instead as the section number in the recent errata> docs has changed.> > v9:> Fix the comments from Wolfram Sang
Patch 2 has my comment not addressed, so I stopped reviewing. It is
probably more helpful (and easier for me, too) if you do a changelog per
patch (and not of the whole series), then you can immediately see if
that specific changelog matches the current patch. 'git send-email
--annotate' might be helpful here.
Thanks,
Wolfram

Hi Wolfram,
Wolfram Sang <w.sang@pengutronix.de> writes:
> On Wed, May 02, 2012 at 08:02:11PM +0530, Shubhrajyoti D wrote:>> In omap_i2c_remove we are accessing the I2C_CON register without>> enabling the clocks. Fix the same by enabling the clocks and disabling>> it.
[...]
> I'd really like a comment from the PM experts if each and every driver> has to ensure that the clocks are enabled on remove like this?
Yes, this is correct.
In fact, this is the goal of runtime PM. The driver itself tells the PM
core (using runtime PM) when the device needs to be accessible and when
it doesn't.
Technically speaking, the it's up to the platform-specific runtime PM
implementation to decide whether or not the clocks are actually disable
or not (e.g. due to wakeup latency requirements, it might decide not to
cut clocks.)
Because of that, the changelog should be reworded to say something like
"ensure device is accessible" instead of "enable the clocks", because
the runtime PM implementation does more than just manage clocks.
Kevin
P.S. It's great to see you helping out maintaining i2c drivers. Thanks!
P.P.S. Before you merge this, I would strongly recommend we wait for a
few more Tested-bys, and a bit more description about how this
was tested. We've been having quite a few problems with
regressions introduced in OMAP drivers that have not been well
reviewed or tested.

Shubhrajyoti D <shubhrajyoti@ti.com> writes:
> The patch series does the following>> - Warn fixes if CONFIG_PM_RUNTIME is not selected.> - In case of i2c remove register access was done without any> get_sync fix the same.> - Folds a patch from Tasslehoff to prevent any merge conflicts.> - Prevents the XDUF flag to be set if the underflow condition is not met.> - As per discussion in [1] .Adds a patch to rename the 1p153 errata and> use the unique id instead as the section number in the recent errata> docs has changed.>> v9:> Fix the comments from Wolfram Sang>> [1] http://www.spinics.net/lists/linux-i2c/msg07607.html>> Tested on omap4sdp and omap3sdp.
Can you also describe how it was tested?
With the runtime PM changes, does it still hit full-chip retention in
idle and suspend after these changes?
I had a few minor comments on this version, otherwise feel free add
Reviewed-by: Kevin Hilman <khilman@ti.com>
That being said, before this is merged, I woudl like to see some more
non-author Tested-bys. We've been having lots of regressions of late
from OMAP drivers that are not being sufficiently tested before
merging. We need to ensure proper testing before merge.
Other testers should also report what platforms they tested on, and how
it was tested.
Thanks,
Kevin

Hi Kevin,
On Saturday 26 May 2012 03:43 AM, Kevin Hilman wrote:
> Shubhrajyoti D <shubhrajyoti@ti.com> writes:>>> The patch series does the following>>>> - Warn fixes if CONFIG_PM_RUNTIME is not selected.>> - In case of i2c remove register access was done without any>> get_sync fix the same.>> - Folds a patch from Tasslehoff to prevent any merge conflicts.>> - Prevents the XDUF flag to be set if the underflow condition is not met.>> - As per discussion in [1] .Adds a patch to rename the 1p153 errata and>> use the unique id instead as the section number in the recent errata>> docs has changed.>>>> v9:>> Fix the comments from Wolfram Sang>>>> [1] http://www.spinics.net/lists/linux-i2c/msg07607.html>>>> Tested on omap4sdp and omap3sdp.> Can you also describe how it was tested?
I did basic functionality tests using i2c-tools.
>> With the runtime PM changes, does it still hit full-chip retention in> idle and suspend after these changes?
Will check.
>> I had a few minor comments on this version,
Will fixup and resend.
> otherwise feel free add>> Reviewed-by: Kevin Hilman <khilman@ti.com>
Thanks for your review.
>> That being said, before this is merged, I woudl like to see some more> non-author Tested-bys. We've been having lots of regressions of late> from OMAP drivers that are not being sufficiently tested before> merging. We need to ensure proper testing before merge.>> Other testers should also report what platforms they tested on, and how> it was tested.>> Thanks,>> Kevin

On Saturday 26 May 2012 05:10 AM, Kevin Hilman wrote:
> Shubhrajyoti,>> Can you add one more patch to this series.
Yes will add it.
Thanks,
Shubhro
>> The patch below from Neil Brown has been circulating for awhile, and> I've been using it locally for awhile now too. It would help if it got> into this series and got some broader testing.>> Thanks,>> Kevin

On Saturday 26 May 2012 03:27 AM, Kevin Hilman wrote:
> Looks right.>> Can you be more specific in the changelog about when the errors/warning> happens?
By the way the count cribs Unbalanced pm_runtime_enable!
is what happens.
I found it by review, I did the testing with this patch.
Since there was an access without get_sync I was seeing
some other issues.
Did not test with the fix and without this patch.
> e.g. why pm_runtime_enable() is called again? Is this on> module unload/reload?>> Other than that>> Acked-by: Kevin Hilman <khilman@ti.com>
Thanks.