[RFC/PATCH 0/3] OMAP3 I2C/SCCB support

2012-06-26 Thread 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.

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


Re: [RFC/PATCH 0/3] OMAP3 I2C/SCCB support

2012-06-26 Thread jean-philippe francois
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-omap" 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-06-26 Thread Laurent Pinchart
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-omap" 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-06-26 Thread Gary Thomas

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-omap" 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-06-26 Thread Laurent Pinchart
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-omap" 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-06-26 Thread Jean Delvare
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-omap" 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-06-26 Thread Laurent Pinchart
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-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html