Re: [PATCH] I2C: OMAP: xfer: fix runtime PM get/put balance on error
Hi Kevin, Thanks for the patch , a doubt below On Wed, Jun 27, 2012 at 7:15 AM, Kevin Hilman wrote: > In omap_i2c_xfer(), ensure pm_runtime_put() is called, even on > failure. So the failure means that the usecount is incremented. However the device was not enabled. In that case could we consider void pm_runtime_put_noidle(struct device *dev); - decrement the device's usage counter Which will only decrement the counter and does not try to disable it. However I am not sure what happens if you try to disable an already disabled device. > > Without this, after a failed xfer, the runtime PM usecount will have > been incremented, but not decremented Agree. > causing the usecount to never > reach zero after a failure. This keeps the device always runtime PM > enabled which keeps the enclosing power domain active, and prevents > full-chip retention/off from happening during idle. > > Cc: Shubhrajyoti D > Signed-off-by: Kevin Hilman > --- > This patch applies to current i2c-embedded/for-next branch > > drivers/i2c/busses/i2c-omap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > index 9895fa7..b105733 100644 > --- a/drivers/i2c/busses/i2c-omap.c > +++ b/drivers/i2c/busses/i2c-omap.c > @@ -582,7 +582,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg > msgs[], int num) > > r = pm_runtime_get_sync(dev->dev); > if (IS_ERR_VALUE(r)) > - return r; > + goto out; > > r = omap_i2c_wait_for_bb(dev); > if (r < 0) > -- > 1.7.9.2 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2 V3] MXS: Set I2C timing registers for mxs-i2c
Dear Shawn Guo,
> On Wed, Jun 27, 2012 at 03:30:31AM +0200, Marek Vasut wrote:
> > Ok, I managed to replicate it just now. Scrap my previous email.
> >
> > Still, any idea what can cause this?
>
> I just spent some time on it. It looks like a document issue (dammit).
>
> The HW_I2C_TIMING2 EXAMPLE tells the value is 0x0015000d which is the
> one you looked at and used. But the register field figure tells it's
> 0x00300030, which seems the real case.
>
> The mxs_i2c_reset at probe changes the value to 0x0015000d which causes
> the first time playback nonfunctional, but the mxs_i2c_reset call in
> mxs_i2c_xfer_msg happens to reset the value back to 0x00300030, and
> then second time playback starts working.
>
> The changes below get everything work fine, both 100k and 400k. So
> with the changes in, you can add:
Ok, can you push the FSL guys to roll out updated document?
Do you consider it reasonable to add some calculations of these timing values
or
were they somehow fine-tuned with a scope/deep HW knowledge as the best working
values and we should stick to those?
> Tested-by: Shawn Guo
>
> Regards,
> Shawn
>
> --8<---
>
> diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
> index e38be56..c2467e9 100644
> --- a/drivers/i2c/busses/i2c-mxs.c
> +++ b/drivers/i2c/busses/i2c-mxs.c
> @@ -112,13 +112,13 @@ struct mxs_i2c_speed_config {
> const struct mxs_i2c_speed_config mxs_i2c_95kHz_config = {
> .timing0= 0x00780030,
> .timing1= 0x00800030,
> - .timing2= 0x0015000d,
> + .timing2= 0x00300030,
> };
>
> const struct mxs_i2c_speed_config mxs_i2c_400kHz_config = {
> .timing0= 0x000f0007,
> .timing1= 0x001f000f,
> - .timing2= 0x0015000d,
> + .timing2= 0x00300030,
> };
Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2 V3] MXS: Set I2C timing registers for mxs-i2c
On Wed, Jun 27, 2012 at 03:30:31AM +0200, Marek Vasut wrote:
> Ok, I managed to replicate it just now. Scrap my previous email.
>
> Still, any idea what can cause this?
>
I just spent some time on it. It looks like a document issue (dammit).
The HW_I2C_TIMING2 EXAMPLE tells the value is 0x0015000d which is the
one you looked at and used. But the register field figure tells it's
0x00300030, which seems the real case.
The mxs_i2c_reset at probe changes the value to 0x0015000d which causes
the first time playback nonfunctional, but the mxs_i2c_reset call in
mxs_i2c_xfer_msg happens to reset the value back to 0x00300030, and
then second time playback starts working.
The changes below get everything work fine, both 100k and 400k. So
with the changes in, you can add:
Tested-by: Shawn Guo
Regards,
Shawn
--8<---
diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
index e38be56..c2467e9 100644
--- a/drivers/i2c/busses/i2c-mxs.c
+++ b/drivers/i2c/busses/i2c-mxs.c
@@ -112,13 +112,13 @@ struct mxs_i2c_speed_config {
const struct mxs_i2c_speed_config mxs_i2c_95kHz_config = {
.timing0= 0x00780030,
.timing1= 0x00800030,
- .timing2= 0x0015000d,
+ .timing2= 0x00300030,
};
const struct mxs_i2c_speed_config mxs_i2c_400kHz_config = {
.timing0= 0x000f0007,
.timing1= 0x001f000f,
- .timing2= 0x0015000d,
+ .timing2= 0x00300030,
};
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] I2C: OMAP: xfer: fix runtime PM get/put balance on error
In omap_i2c_xfer(), ensure pm_runtime_put() is called, even on failure. Without this, after a failed xfer, the runtime PM usecount will have been incremented, but not decremented causing the usecount to never reach zero after a failure. This keeps the device always runtime PM enabled which keeps the enclosing power domain active, and prevents full-chip retention/off from happening during idle. Cc: Shubhrajyoti D Signed-off-by: Kevin Hilman --- This patch applies to current i2c-embedded/for-next branch drivers/i2c/busses/i2c-omap.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c index 9895fa7..b105733 100644 --- a/drivers/i2c/busses/i2c-omap.c +++ b/drivers/i2c/busses/i2c-omap.c @@ -582,7 +582,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) r = pm_runtime_get_sync(dev->dev); if (IS_ERR_VALUE(r)) - return r; + goto out; r = omap_i2c_wait_for_bb(dev); if (r < 0) -- 1.7.9.2 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/11] I2C: OMAP: Handle error check for pm runtime
Shubhrajyoti D writes: > If PM runtime get_sync fails return with the error > so that no further reads/writes goes through the interface. > This will avoid possible abort. Add a error message in case > of failure with the cause of the failure. > > Reviewed-by: Kevin Hilman > Signed-off-by: Shubhrajyoti D This patch introduced a regression where the runtime PM usecount will never reach zero and so CORE retention is prevented after any xfer failures... > --- > -v10 Use IS_ERR_VALUE > -v9 Fix the error handling > > drivers/i2c/busses/i2c-omap.c | 14 +++--- > 1 files changed, 11 insertions(+), 3 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > index 44e8cfa..c39b72f 100644 > --- a/drivers/i2c/busses/i2c-omap.c > +++ b/drivers/i2c/busses/i2c-omap.c > @@ -585,7 +585,9 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg > msgs[], int num) > int i; > int r; > > - pm_runtime_get_sync(dev->dev); > + r = pm_runtime_get_sync(dev->dev);< > + if (IS_ERR_VALUE(r)) > + return r; This return should be 'goto out' so the runtime PM usecount is decremented by the 'put'. Otherwise, after failure, the usecount remains non-zero, so the device is considered 'active' and keeps the containing power domain active. I found this on a 3730/OveroSTORM where the suspend/resume of MMC fails because I2C is already suspended. After the suspend though, the CORE powerdomain never again hits retention, and I tracked it down to this. I'll send a separate patch to fix this shortly. Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2 V3] MXS: Set I2C timing registers for mxs-i2c
Dear Shawn Guo, > > On Sat, Jun 09, 2012 at 01:45:50PM +0200, Marek Vasut wrote: > > > This patch configures the I2C bus timing registers according > > > to information passed via DT. Currently, 100kHz and 400kHz > > > modes are supported. > > > > > > Signed-off-by: Marek Vasut > > > > I gave it a test on imx28-evk board with audio playback. It seems > > the patch makes the first time playback non-functional, but the later > > playback is still working. > > Any hints what can be the source of this issue? I tested it with i2c > eeprom, saw no issues in there. I'll poke into it later. Ok, I managed to replicate it just now. Scrap my previous email. Still, any idea what can cause this? Best regards, Marek Vasut -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2 V3] MXS: Set I2C timing registers for mxs-i2c
Dear Shawn Guo, > On Sat, Jun 23, 2012 at 08:47:32PM +0200, Marek Vasut wrote: > > Dear Shawn Guo, > > > > > This patch configures the I2C bus timing registers according > > > to information passed via DT. Currently, 100kHz and 400kHz > > > modes are supported. > > > > [...] > > > > Is there any reason why this can not be merged other than the timing > > registers goo (which I believe shall stay as in the datasheet until we > > figure out if it's even reasonable to add some computation there). > > I assume this is a question for Wolfram. But I guess part of the reason > is there is still one comment from me staying unresolved. Shawn, I just re-tested the i2c with mpg123 playing "MPEG 1.0 layer III, VBR, 44100 Hz joint-stereo" and it worked on a first try ... can you retest please? Best regards, Marek Vasut -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/3] OMAP3 I2C/SCCB support
Hi Jean, On Tuesday 26 June 2012 18:56:16 Jean Delvare wrote: > On Tue, 26 Jun 2012 18:20:33 +0200, Laurent Pinchart wrote: > > It might be a good idea to implement i2c_smbus_*-like functions for 16-bit > > register addresses in the I2C core, they could be reused across drivers. > > Except that they would preferably not be called i2c_smbus_*, not being > part of the SMBus subset nor being commonly implemented by SMBus-only > controllers. Sure. We need another name. > Finding a proper name for these helpers function might be tough, as may be > figuring out which ones you really want to implement and which ones aren't > worth it. > > Also note that this is more or less what regmap is trying to do, so you > may want to check in that direction (see drivers/base/regmap) first. Thanks, I didn't know about that. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/3] OMAP3 I2C/SCCB support
Salut Laurent, On Tue, 26 Jun 2012 18:20:33 +0200, Laurent Pinchart wrote: > It might be a good idea to implement i2c_smbus_*-like functions for 16-bit > register addresses in the I2C core, they could be reused across drivers. Except that they would preferably not be called i2c_smbus_*, not being part of the SMBus subset nor being commonly implemented by SMBus-only controllers. Finding a proper name for these helpers function might be tough, as may be figuring out which ones you really want to implement and which ones aren't worth it. Also note that this is more or less what regmap is trying to do, so you may want to check in that direction (see drivers/base/regmap) first. -- Jean Delvare -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/3] OMAP3 I2C/SCCB support
Hi Gary,
On Tuesday 26 June 2012 10:25:58 Gary Thomas wrote:
> On 2012-06-26 10:20, Laurent Pinchart wrote:
> > On Tuesday 26 June 2012 16:49:35 jean-philippe francois wrote:
> >> 2012/6/26 Laurent Pinchart:
> >>> Hi everybody,
> >>>
> >>> While trying to use an OV7725 camera sensor with an OMAP3 system I ran
> >>> into a couple of issues related to the sensor communication protocol.
> >>> Instead of using the obiquitous I2C protocol, Omnivision invited the
> >>> SCCB communication bus, very similar to I2C but different enough not to
> >>> be compatible.
> >>
> >> I have been using omnivision sensor without being aware of this issue,
> >> and without any i2c reliability issue either.
> >>
> >> Here is typical code I use :
> >>
> >> /* This function is used to read value from register for i2c client */
> >> static int ov2710_read(struct i2c_client *client, unsigned short reg,
> >> unsigned char *val)
> >> {
> >>
> >> int err = 0;
> >> unsigned char outbuf[] = {(reg>>8)&0xff, reg&0xff};
> >
> > According to the SCCB specification (or at least the version I've found)
> > register addresses are 8-bit. The OV2710 might just be I2C-compatible.
> >
> >> unsigned char inbuf[1];
> >> struct i2c_msg msg[] = {
> >>
> >> { .addr = client->addr, .flags = 0, .buf = outbuf, .len = 2 },
> >> { .addr = client->addr, .flags = I2C_M_RD, .buf = inbuf, .len =
> >> 1 },
> >>
> >> };
> >>
> >> err = i2c_transfer(client->adapter, msg, 2);
> >> /* error handling and pass by ref handling */
> >>
> >>
> >> }
> >
> > The ov772x driver uses i2c_smbus_write_byte_data() and
> > i2c_smbus_read_byte_data(). The later calls
> >
> > i2c_smbus_xfer(client->adapter, client->addr, client->flags,
> >
> > I2C_SMBUS_READ, I2C_SMBUS_BYTE_DATA,&data);
> >
> > which calls i2c_smbus_xfer_emulated() for hosts that don't support SMBus
> > transfers natively, and that's pretty much equivalent to your above code
> > (except for the 8/16 bit register address).
> >
> > It might be a good idea to implement i2c_smbus_*-like functions for 16-bit
> > register addresses in the I2C core, they could be reused across drivers.
> >
> >> Is the point of this patch to avoid writing such functions again and
> >> again in every ov driver ?
> >
> > No, but that's a good idea as well :-)
> >
> >> What is solved in practice by this patch that is not solved by a regular
> >> i2c transfer ?
> >
> > The above code will not ignore acks/nacks and will not generate a stop
> > condition between the two messages. The SCCB specification states that
> > acks/nacks must be ignored and that a stop condition must be generated.
> > The OV7725 handles acks/nacks correctly, but chokes if no stop condition
> > is generated. The point of this patch set is to fix both problems
> > (although the acks/nacks issue might be theoretical only).
>
> How does it "choke"? I've had success talking to the OV8820 using the
> normal I2C driver as well.
With the patches applied:
[ 23.277893] omap3isp omap3isp: Revision 15.0 found
[ 23.283721] omap-iommu omap-iommu.0: isp: version 1.1
[ 25.244079] ov772x 2-0021: ov7725 Product ID 77:21 Manufacturer ID 7f:a2
Without the patches applied:
[ 23.505493] omap3isp omap3isp: Revision 15.0 found
[ 23.511138] omap-iommu omap-iommu.0: isp: version 1.1
[ 25.552246] omap_i2c omap_i2c.2: Arbitration lost
[ 26.583160] omap_i2c omap_i2c.2: timeout waiting for bus ready
[ 27.598785] omap_i2c omap_i2c.2: timeout waiting for bus ready
[ 28.614196] omap_i2c omap_i2c.2: timeout waiting for bus ready
[ 28.623260] isp_register_subdev_group: Unable to register subdev ov772x
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/3] OMAP3 I2C/SCCB support
On 2012-06-26 10:20, Laurent Pinchart wrote:
Hi Jean-Philippe,
On Tuesday 26 June 2012 16:49:35 jean-philippe francois wrote:
2012/6/26 Laurent Pinchart:
Hi everybody,
While trying to use an OV7725 camera sensor with an OMAP3 system I ran
into a couple of issues related to the sensor communication protocol.
Instead of using the obiquitous I2C protocol, Omnivision invited the SCCB
communication bus, very similar to I2C but different enough not to be
compatible.
I have been using omnivision sensor without being aware of this issue,
and without any i2c reliability issue either.
Here is typical code I use :
/* This function is used to read value from register for i2c client */
static int ov2710_read(struct i2c_client *client, unsigned short reg,
unsigned char *val)
{
int err = 0;
unsigned char outbuf[] = {(reg>>8)&0xff, reg&0xff};
According to the SCCB specification (or at least the version I've found)
register addresses are 8-bit. The OV2710 might just be I2C-compatible.
unsigned char inbuf[1];
struct i2c_msg msg[] = {
{ .addr = client->addr, .flags = 0, .buf = outbuf, .len = 2 },
{ .addr = client->addr, .flags = I2C_M_RD, .buf = inbuf, .len = 1 },
};
err = i2c_transfer(client->adapter, msg, 2);
/* error handling and pass by ref handling */
}
The ov772x driver uses i2c_smbus_write_byte_data() and
i2c_smbus_read_byte_data(). The later calls
i2c_smbus_xfer(client->adapter, client->addr, client->flags,
I2C_SMBUS_READ, I2C_SMBUS_BYTE_DATA,&data);
which calls i2c_smbus_xfer_emulated() for hosts that don't support SMBus
transfers natively, and that's pretty much equivalent to your above code
(except for the 8/16 bit register address).
It might be a good idea to implement i2c_smbus_*-like functions for 16-bit
register addresses in the I2C core, they could be reused across drivers.
Is the point of this patch to avoid writing such functions again and again
in every ov driver ?
No, but that's a good idea as well :-)
What is solved in practice by this patch that is not solved by a regular i2c
transfer ?
The above code will not ignore acks/nacks and will not generate a stop
condition between the two messages. The SCCB specification states that
acks/nacks must be ignored and that a stop condition must be generated. The
OV7725 handles acks/nacks correctly, but chokes if no stop condition is
generated. The point of this patch set is to fix both problems (although the
acks/nacks issue might be theoretical only).
How does it "choke"? I've had success talking to the OV8820 using the
normal I2C driver as well.
--
Gary Thomas | Consulting for the
MLB Associates |Embedded world
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: tegra: use clk_disable_unprepare in place of clk_disable
On 06/26/2012 12:27 AM, Laxman Dewangan wrote: > On Monday 25 June 2012 09:25 PM, Stephen Warren wrote: >> On 06/25/2012 03:46 AM, Laxman Dewangan wrote: >>> Stephen, >>> >>> On Wednesday 20 June 2012 09:57 PM, Stephen Warren wrote: On 06/20/2012 10:26 AM, Stephen Warren wrote: > On 06/20/2012 06:56 AM, Laxman Dewangan wrote: >> Use clk_disable_unprepare() inplace of clk_disable(). >> This was missed as part of moving clock enable/disable to >> prepare/unprepare for using the common clock framework. ... > I see no reason not to take the second patch in the series through the > I2C tree though. Uggh. Ignore that paragraph - the other patch was sent separately not as a series. >>> so are you taking care of this patch or do I need to send the patch >>> based on your tree in place of linux-next? >> Yes, this patch should be applied through the Tegra tree, since it will >> be a dependency of the common clock framework switchover there, which I >> hope to take place this kernel cycle. >> >> I did just attempt to apply this patch to the for-3.6/common-clk branch, >> but it doesn't apply:-( Could you please rebase and resend. Thanks. > > Looked at your common_clk branch and the related code is not there. > The clk_disable() in the particular case is introduced by change > i2c: tegra: make all resource allocation through devm_* > which is not in your branch. > > Then later Prashant post the change as > i2c: tegra: Add clk_prepare/clk_unprepare > and it does not accounted for the above patch. > > So none of your local tree will have this issue. OK. In that case, it's best if this patch goes through the I2C tree since that's where the code is that it's modifying. This might not be optimal for runtime git bisection depending on the order Linus ends up merging things, but it's probably as good as we can do without inter-twining the I2C and Tegra trees a lot. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3 v2] i2c: i801: enable irq for byte_by_byte transactions
Hi Daniel,
On Wed, 20 Jun 2012 15:34:49 +0200, Jean Delvare wrote:
> I'll get at least the I2C block read tested on ICH5.
Some more notes about this patch, now that have done some testing...
> > (...)
> > + } else if (priv->count < priv->len - 1) {
>
> Is this just paranoia or do you believe it could actually happen? I
> admit I don't see how. If it is paranoia then the same should be done
> for the read part.
This is good paranoia and I recommend doing the same for block reads,
where it is even more important. An array overrun on block write means
you'll be sending random memory bytes to your I2C device, which is
already bad. But an array overrun on block read means you'll _trash_
random memory bytes, with tragic consequences. The bug right below did
exactly this to me: the block read would never stop, and after a few
seconds only my machine would die with a different error each time,
depending on what memory got trashed.
I ended up testing for both count < len and len <= I2C_SMBUS_BLOCK_MAX,
for both reads and writes. Probably that's overkill and either should
be sufficient, but I wanted to play it really safe at first.
>
> > + outb(priv->data[++priv->count], SMBBLKDAT(priv));
> > + outb_p(priv->cmd | I801_START, SMBHSTCNT(priv));
>
> I don't get the I801_START here and above. We are in the middle of the block
> transaction. The polling-based code only sets I801_START at the
> beginning, not for every byte, so why would it be different here?
I confirm this is a serious bug. The patch broke I2C block read on my
ICH5 machine completely, until I removed these two I801_START.
After fixing this, testing is conclusive on my ICH5 machine (where the
SMBus interrupt is shared.) BTW, the debug message complaining when
pcists.ints isn't set should be dropped, in my case the interrupt is
shared with the sound chip and DVB-T card, and this caused a massive
flood of this message while I was debugging. The message isn't terribly
useful anyway IMHO, there's nothing wrong with that case.
Next step for me is testing on my ICH7-M laptop.
--
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/3] OMAP3 I2C/SCCB support
Hi Jean-Philippe,
On Tuesday 26 June 2012 16:49:35 jean-philippe francois wrote:
> 2012/6/26 Laurent Pinchart :
> > Hi everybody,
> >
> > While trying to use an OV7725 camera sensor with an OMAP3 system I ran
> > into a couple of issues related to the sensor communication protocol.
> > Instead of using the obiquitous I2C protocol, Omnivision invited the SCCB
> > communication bus, very similar to I2C but different enough not to be
> > compatible.
> I have been using omnivision sensor without being aware of this issue,
> and without any i2c reliability issue either.
>
> Here is typical code I use :
>
> /* This function is used to read value from register for i2c client */
> static int ov2710_read(struct i2c_client *client, unsigned short reg,
> unsigned char *val)
> {
> int err = 0;
> unsigned char outbuf[] = {(reg>>8)&0xff, reg&0xff};
According to the SCCB specification (or at least the version I've found)
register addresses are 8-bit. The OV2710 might just be I2C-compatible.
> unsigned char inbuf[1];
> struct i2c_msg msg[] = {
> { .addr = client->addr, .flags = 0, .buf = outbuf, .len = 2 },
> { .addr = client->addr, .flags = I2C_M_RD, .buf = inbuf, .len = 1 },
> };
> err = i2c_transfer(client->adapter, msg, 2);
> /* error handling and pass by ref handling */
>
> }
The ov772x driver uses i2c_smbus_write_byte_data() and
i2c_smbus_read_byte_data(). The later calls
i2c_smbus_xfer(client->adapter, client->addr, client->flags,
I2C_SMBUS_READ, I2C_SMBUS_BYTE_DATA, &data);
which calls i2c_smbus_xfer_emulated() for hosts that don't support SMBus
transfers natively, and that's pretty much equivalent to your above code
(except for the 8/16 bit register address).
It might be a good idea to implement i2c_smbus_*-like functions for 16-bit
register addresses in the I2C core, they could be reused across drivers.
> Is the point of this patch to avoid writing such functions again and again
> in every ov driver ?
No, but that's a good idea as well :-)
> What is solved in practice by this patch that is not solved by a regular i2c
> transfer ?
The above code will not ignore acks/nacks and will not generate a stop
condition between the two messages. The SCCB specification states that
acks/nacks must be ignored and that a stop condition must be generated. The
OV7725 handles acks/nacks correctly, but chokes if no stop condition is
generated. The point of this patch set is to fix both problems (although the
acks/nacks issue might be theoretical only).
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/3] OMAP3 I2C/SCCB support
2012/6/26 Laurent Pinchart :
> Hi everybody,
>
> While trying to use an OV7725 camera sensor with an OMAP3 system I ran into a
> couple of issues related to the sensor communication protocol. Instead of
> using the obiquitous I2C protocol, Omnivision invited the SCCB communication
> bus, very similar to I2C but different enough not to be compatible.
>
I have been using omnivision sensor without being aware of this issue,
and without any i2c reliability issue either.
Here is typical code I use :
/* This function is used to read value from register for i2c client */
static int ov2710_read(struct i2c_client *client, unsigned short reg,
unsigned char *val)
{
int err = 0;
unsigned char outbuf[] = {(reg>>8)&0xff, reg&0xff};
unsigned char inbuf[1];
struct i2c_msg msg[] = {
{ .addr = client->addr, .flags = 0, .buf = outbuf, .len = 2 },
{ .addr = client->addr, .flags = I2C_M_RD, .buf = inbuf, .len = 1 },
};
err = i2c_transfer(client->adapter, msg, 2);
/* error handling and pass by ref handling */
}
Is the point of this patch to avoid writing such functions again and
again in every ov driver ?
What is solved in practice by this patch that is not solved by a
regular i2c transfer ?
Regards,
Jean-Philippe François
> SCCB documentation is available at
> http://www.ovt.com/download_document.php?type=document&DID=63. SCCB exists in
> two flavors, 3-wire and 2-wire. The 3-wire version is too different from I2C
> to be of interest here.
>
> SCCB is very close to SMBus. It supports two transactions, an 8-bit register
> read and an 8-bit register write. The write transaction transmits a 3-byte
> message (addr/w, reg, data). The read transaction transmits 2 2-byte messages
> (addr/w, reg, followed by addr/r, data).
>
> The two majors differences between I2C and SCCB are in the acknowledgment bit
> and in the stop bit.
>
> SCCB devices generate no ack bit and don't take the ack bit generated by the
> master into account. The ack bit is thus an unspecified bit, present in the
> transfer but whose value must be ignored. However, in practice, the SCCB
> components I've seen implement the ack bit in write transactions.
>
> The stop bit is handled as in I2C, except that a stop bit needs to be
> generated between the two messages that make the read transaction (as opposed
> to SMBus, where no stop bit must be present between the two messages).
>
> I need SCCB support in the I2C core and in the OMAP3 I2C driver and have two
> options. The OMAP3 I2C controller support SCCB natively. The interface exposed
> by the hardware is at the transaction level (smbus_xfer), while the i2c-omap
> driver exposes a message level interface (master_xfer). To use the native SCCB
> support, we would thus have to either allow i2c_smbus_xfer() to choose between
> smbus_xfer and i2c_smbus_xfer_emulated based on the client type (I2C or SCCB),
> or export i2c_smbus_xfer_emulated() to use it as a fallback in the i2c-omap
> driver for I2C clients.
>
> Another option is not to use the hardware SCCB support, but to emulate it
> through I2C. In that case the i2c-omap driver must generate a stop bit after
> the first message of a read transaction, and ignore the ack bit. Two flags
> would then be necessary, one in the message to force the stop bit, and one in
> the i2c_client to mark the device as using SCCB.
>
> Implementing support for the second option is required for I2C masters that
> have no hardware SCCB support. As the first option won't bring much benefits,
> I've decided to skip it for now. The following three patches thus implement
> emulated SCCB support in the I2C core, with a small change in the i2c-omap
> driver to support the new I2C_M_STOP flag.
>
> Laurent Pinchart (3):
> i2c: Add SCCB support
> i2c: Fall back to emulated SMBus if the operation isn't supported
> natively
> i2c: omap: Add support for I2C_M_STOP message flag
>
> drivers/i2c/busses/i2c-omap.c | 2 ++
> drivers/i2c/i2c-core.c | 22 +++---
> include/linux/i2c.h | 2 ++
> 3 files changed, 23 insertions(+), 3 deletions(-)
>
> --
> Regards,
>
> Laurent Pinchart
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] i2c_dw: deadlock happening when system is trying to suspend
From: liu chuansheng Subject: [PATCH] i2c_dw: deadlock happening when system is trying to suspend In i2c_dw code, there is a race condition that causes pm suspend thread blocking there always. The scenerio is as below: PM thread: suspend --> pm_suspend --> enter_state --> dpm_suspend_start(will call i2c_dw_pci_suspend(), and the dw_i2c_dev->lock is hold) ... suspend_enter --> dpm_suspend_noirq --> suspend_device_irqs --> synchronize_irq() synchronize_irq will wait for any pending irq is handled, and the correpsonding irq thread is finished. In this case, there is a i2c device interrupt is pending, the irq thread do the below things: IRQ thread: i2c_smbus_read_byte_data --> i2c_smbus_xfer --> i2c_transfer --> i2c_dw_xfer --> down() The irq thread blocked at down dw_i2c_dev->lock, because in PM thread, it has been hold after calling i2c_dw_pci_suspend(), but PM thread is waiting for IRQ thread, then deadlock happened. The solution is moving the down() action after pm_runtime_get_sync(). Signed-off-by: liu chuansheng --- drivers/i2c/busses/i2c-designware-core.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c index 1e48bec..748ecb1 100644 --- a/drivers/i2c/busses/i2c-designware-core.c +++ b/drivers/i2c/busses/i2c-designware-core.c @@ -512,8 +512,8 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num); - mutex_lock(&dev->lock); pm_runtime_get_sync(dev->dev); + mutex_lock(&dev->lock); INIT_COMPLETION(dev->cmd_complete); dev->msgs = msgs; -- 1.7.0.4 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC/PATCH 2/3] i2c: Fall back to emulated SMBus if the operation isn't supported natively
Adapter drivers might support only a subset of the SMBus operations
natively. Those drivers currently have to manually emulate unsupported
operations using I2C.
Make the i2c_smbus_xfer() function fall back to
i2c_smbus_xfer_emulated() when the adapter's .smbus_xfer() operation
returns -EOPNOTSUPP, like it already does when the .smbus_xfer()
operation isn't available at all.
Signed-off-by: Laurent Pinchart
---
drivers/i2c/i2c-core.c |9 +++--
1 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 8cfa660..16e750e 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -2113,8 +2113,8 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
unsigned short flags,
union i2c_smbus_data *data)
{
unsigned long orig_jiffies;
+ s32 res = -EOPNOTSUPP;
int try;
- s32 res;
flags &= I2C_M_TEN | I2C_CLIENT_PEC | I2C_CLIENT_SCCB;
@@ -2134,7 +2134,12 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16
addr, unsigned short flags,
break;
}
i2c_unlock_adapter(adapter);
- } else
+ }
+
+ /* Fall back to i2c_smbus_xfer_emulated of the adapter doesn't implement
+* native support for the SMBus operation.
+*/
+ if (res == -EOPNOTSUPP && adapter->algo->master_xfer)
res = i2c_smbus_xfer_emulated(adapter, addr, flags, read_write,
command, protocol, data);
--
1.7.3.4
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC/PATCH 1/3] i2c: Add SCCB support
SCCB is a serial communication bus developed by Omnivision. Its 2-wire
mode is very similar to SMBus byte data transactions, but requires the
controller to ignore the ACK bit and to insert a stop condition after
each message.
Add a device SCCB flag and a message stop flag to be passed to
controller drivers.
Signed-off-by: Laurent Pinchart
---
drivers/i2c/i2c-core.c | 13 -
include/linux/i2c.h|2 ++
2 files changed, 14 insertions(+), 1 deletions(-)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index feb7dc3..8cfa660 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1939,6 +1939,12 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter
*adapter, u16 addr,
u8 partial_pec = 0;
int status;
+ if (unlikely(flags & I2C_CLIENT_SCCB) && size != I2C_SMBUS_BYTE_DATA) {
+ dev_err(&adapter->dev,
+ "SCCB devices only support I2C_SMBUS_BYTE_DATA\n");
+ return -EINVAL;
+ }
+
msgbuf0[0] = command;
switch (size) {
case I2C_SMBUS_QUICK:
@@ -1956,6 +1962,11 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter
*adapter, u16 addr,
}
break;
case I2C_SMBUS_BYTE_DATA:
+ if (unlikely(flags & I2C_CLIENT_SCCB)) {
+ msg[0].flags |= I2C_M_IGNORE_NAK | I2C_M_STOP;
+ msg[1].flags |= I2C_M_IGNORE_NAK | I2C_M_STOP;
+ }
+
if (read_write == I2C_SMBUS_READ)
msg[1].len = 1;
else {
@@ -2105,7 +2116,7 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
unsigned short flags,
int try;
s32 res;
- flags &= I2C_M_TEN | I2C_CLIENT_PEC;
+ flags &= I2C_M_TEN | I2C_CLIENT_PEC | I2C_CLIENT_SCCB;
if (adapter->algo->smbus_xfer) {
i2c_lock_adapter(adapter);
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 195d8b3..bd42914 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -420,6 +420,7 @@ void i2c_lock_adapter(struct i2c_adapter *);
void i2c_unlock_adapter(struct i2c_adapter *);
/*flags for the client struct: */
+#define I2C_CLIENT_SCCB0x02/* Use Omnivision SCCB protocol
*/
#define I2C_CLIENT_PEC 0x04/* Use Packet Error Checking */
#define I2C_CLIENT_TEN 0x10/* we have a ten bit chip address */
/* Must equal I2C_M_TEN below */
@@ -540,6 +541,7 @@ struct i2c_msg {
__u16 flags;
#define I2C_M_TEN 0x0010 /* this is a ten bit chip address */
#define I2C_M_RD 0x0001 /* read data, from slave to master */
+#define I2C_M_STOP 0x8000 /* if I2C_FUNC_PROTOCOL_MANGLING */
#define I2C_M_NOSTART 0x4000 /* if I2C_FUNC_PROTOCOL_MANGLING */
#define I2C_M_REV_DIR_ADDR 0x2000 /* if I2C_FUNC_PROTOCOL_MANGLING */
#define I2C_M_IGNORE_NAK 0x1000 /* if I2C_FUNC_PROTOCOL_MANGLING */
--
1.7.3.4
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC/PATCH 3/3] i2c: omap: Add support for I2C_M_STOP message flag
Generate a stop condition after each message marked with I2C_M_STOP. Signed-off-by: Laurent Pinchart --- drivers/i2c/busses/i2c-omap.c |2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c index 801df60..cf1bda0 100644 --- a/drivers/i2c/busses/i2c-omap.c +++ b/drivers/i2c/busses/i2c-omap.c @@ -545,6 +545,8 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap, if (dev->speed > 400) w |= OMAP_I2C_CON_OPMODE_HS; + if (msg->flags & I2C_M_STOP) + stop = 1; if (msg->flags & I2C_M_TEN) w |= OMAP_I2C_CON_XA; if (!(msg->flags & I2C_M_RD)) -- 1.7.3.4 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC/PATCH 0/3] OMAP3 I2C/SCCB support
Hi everybody, While trying to use an OV7725 camera sensor with an OMAP3 system I ran into a couple of issues related to the sensor communication protocol. Instead of using the obiquitous I2C protocol, Omnivision invited the SCCB communication bus, very similar to I2C but different enough not to be compatible. SCCB documentation is available at http://www.ovt.com/download_document.php?type=document&DID=63. SCCB exists in two flavors, 3-wire and 2-wire. The 3-wire version is too different from I2C to be of interest here. SCCB is very close to SMBus. It supports two transactions, an 8-bit register read and an 8-bit register write. The write transaction transmits a 3-byte message (addr/w, reg, data). The read transaction transmits 2 2-byte messages (addr/w, reg, followed by addr/r, data). The two majors differences between I2C and SCCB are in the acknowledgment bit and in the stop bit. SCCB devices generate no ack bit and don't take the ack bit generated by the master into account. The ack bit is thus an unspecified bit, present in the transfer but whose value must be ignored. However, in practice, the SCCB components I've seen implement the ack bit in write transactions. The stop bit is handled as in I2C, except that a stop bit needs to be generated between the two messages that make the read transaction (as opposed to SMBus, where no stop bit must be present between the two messages). I need SCCB support in the I2C core and in the OMAP3 I2C driver and have two options. The OMAP3 I2C controller support SCCB natively. The interface exposed by the hardware is at the transaction level (smbus_xfer), while the i2c-omap driver exposes a message level interface (master_xfer). To use the native SCCB support, we would thus have to either allow i2c_smbus_xfer() to choose between smbus_xfer and i2c_smbus_xfer_emulated based on the client type (I2C or SCCB), or export i2c_smbus_xfer_emulated() to use it as a fallback in the i2c-omap driver for I2C clients. Another option is not to use the hardware SCCB support, but to emulate it through I2C. In that case the i2c-omap driver must generate a stop bit after the first message of a read transaction, and ignore the ack bit. Two flags would then be necessary, one in the message to force the stop bit, and one in the i2c_client to mark the device as using SCCB. Implementing support for the second option is required for I2C masters that have no hardware SCCB support. As the first option won't bring much benefits, I've decided to skip it for now. The following three patches thus implement emulated SCCB support in the I2C core, with a small change in the i2c-omap driver to support the new I2C_M_STOP flag. Laurent Pinchart (3): i2c: Add SCCB support i2c: Fall back to emulated SMBus if the operation isn't supported natively i2c: omap: Add support for I2C_M_STOP message flag drivers/i2c/busses/i2c-omap.c |2 ++ drivers/i2c/i2c-core.c| 22 +++--- include/linux/i2c.h |2 ++ 3 files changed, 23 insertions(+), 3 deletions(-) -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv10 1/7] I2C: OMAP: I2C register restore only if context is lost
On Tuesday 26 June 2012 11:38 AM, Shubhrajyoti wrote: > On Monday 25 June 2012 06:00 PM, Felipe Balbi wrote: >>> Cc: Kevin Hilman Signed-off-by: Shubhrajyoti D >> how will this ever work with DT ? > What you say makes sense however that is what currently > most of the omap drivers do. > > Will check on this. >> I say we get rid of the OMAP-specific >> API and build this "context lost" status directly on dev_pm_info and >> have something like pm_runtime_lost_context() or something with pm QoS >> tell you if a device has lost its context. >> >> Also, your commit log doesn't really state any problems you might have >> reached before, Didnt see any issues while reviewing found that the restore was done always. >> or any improvements wrt latency coming out of suspend >> and so on. I am only restoring only 4-5 registers. >> >> IMHO, drivers need a generic way to differentiate if they're resuming >> from OFF or RET, otherwise we will end up with a bunch of OMAP-specific >> hackery on all drivers > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv8 03/13] I2C: OMAP: Remove reset at init
* Shubhrajyoti [120621 02:35]: > On Thursday 21 June 2012 12:50 PM, Tony Lindgren wrote: > > * Shubhrajyoti [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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
