| Message ID | 1528817686-7067-1-git-send-email-akinobu.mita@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Wed, Jun 13, 2018 at 12:34:46AM +0900, Akinobu Mita wrote: > (This is 2nd version of SCCB helpers patch. After 1st version was > submitted, I sent alternative patch titled "i2c: add I2C_M_FORCE_STOP". > But it wasn't accepted because it makes the I2C core code unreadable. > I couldn't find out a way to untangle it, so I returned to the original > approach.) > > This adds Serial Camera Control Bus (SCCB) helper functions (sccb_read_byte > and sccb_write_byte) that are intended to be used by some of Omnivision > sensor drivers. > > The ov772x driver is going to use these functions in order to make it work > with most i2c controllers. > > As the ov772x device doesn't support repeated starts, this driver currently > requires I2C_FUNC_PROTOCOL_MANGLING that is not supported by many i2c > controller drivers. > > With the sccb_read_byte() that issues two separated requests in order to > avoid repeated start, the driver doesn't require I2C_FUNC_PROTOCOL_MANGLING. > > Cc: Sebastian Reichel <sebastian.reichel@collabora.co.uk> > Cc: Wolfram Sang <wsa@the-dreams.de> > Cc: Jacopo Mondi <jacopo+renesas@jmondi.org> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Hans Verkuil <hans.verkuil@cisco.com> > Cc: Sakari Ailus <sakari.ailus@linux.intel.com> > Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> > --- Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> -- Sebastian > * v2 > - Convert all helpers into static inline functions, and remove C source > and Kconfig option. > - Acquire i2c adapter lock while issuing two requests for sccb_read_byte > > drivers/media/i2c/sccb.h | 74 ++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 74 insertions(+) > create mode 100644 drivers/media/i2c/sccb.h > > diff --git a/drivers/media/i2c/sccb.h b/drivers/media/i2c/sccb.h > new file mode 100644 > index 0000000..a531fdc > --- /dev/null > +++ b/drivers/media/i2c/sccb.h > @@ -0,0 +1,74 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Serial Camera Control Bus (SCCB) helper functions > + */ > + > +#ifndef __SCCB_H__ > +#define __SCCB_H__ > + > +#include <linux/i2c.h> > + > +/** > + * sccb_read_byte - Read data from SCCB slave device > + * @client: Handle to slave device > + * @addr: Register to be read from > + * > + * This executes the 2-phase write transmission cycle that is followed by a > + * 2-phase read transmission cycle, returning negative errno else a data byte > + * received from the device. > + */ > +static inline int sccb_read_byte(struct i2c_client *client, u8 addr) > +{ > + u8 val; > + struct i2c_msg msg[] = { > + { > + .addr = client->addr, > + .len = 1, > + .buf = &addr, > + }, > + { > + .addr = client->addr, > + .flags = I2C_M_RD, > + .len = 1, > + .buf = &val, > + }, > + }; > + int ret; > + int i; > + > + i2c_lock_adapter(client->adapter); > + > + /* Issue two separated requests in order to avoid repeated start */ > + for (i = 0; i < 2; i++) { > + ret = __i2c_transfer(client->adapter, &msg[i], 1); > + if (ret != 1) > + break; > + } > + > + i2c_unlock_adapter(client->adapter); > + > + return i == 2 ? val : ret; > +} > + > +/** > + * sccb_write_byte - Write data to SCCB slave device > + * @client: Handle to slave device > + * @addr: Register to write to > + * @data: Value to be written > + * > + * This executes the SCCB 3-phase write transmission cycle, returning negative > + * errno else zero on success. > + */ > +static inline int sccb_write_byte(struct i2c_client *client, u8 addr, u8 data) > +{ > + int ret; > + unsigned char msgbuf[] = { addr, data }; > + > + ret = i2c_master_send(client, msgbuf, 2); > + if (ret < 0) > + return ret; > + > + return 0; > +} > + > +#endif /* __SCCB_H__ */ > -- > 2.7.4 >
On 2018-06-12 17:34, Akinobu Mita wrote: > (This is 2nd version of SCCB helpers patch. After 1st version was > submitted, I sent alternative patch titled "i2c: add I2C_M_FORCE_STOP". > But it wasn't accepted because it makes the I2C core code unreadable. > I couldn't find out a way to untangle it, so I returned to the original > approach.) > > This adds Serial Camera Control Bus (SCCB) helper functions (sccb_read_byte > and sccb_write_byte) that are intended to be used by some of Omnivision > sensor drivers. > > The ov772x driver is going to use these functions in order to make it work > with most i2c controllers. > > As the ov772x device doesn't support repeated starts, this driver currently > requires I2C_FUNC_PROTOCOL_MANGLING that is not supported by many i2c > controller drivers. > > With the sccb_read_byte() that issues two separated requests in order to > avoid repeated start, the driver doesn't require I2C_FUNC_PROTOCOL_MANGLING. > > Cc: Sebastian Reichel <sebastian.reichel@collabora.co.uk> > Cc: Wolfram Sang <wsa@the-dreams.de> > Cc: Jacopo Mondi <jacopo+renesas@jmondi.org> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Hans Verkuil <hans.verkuil@cisco.com> > Cc: Sakari Ailus <sakari.ailus@linux.intel.com> > Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> > --- > * v2 > - Convert all helpers into static inline functions, and remove C source > and Kconfig option. > - Acquire i2c adapter lock while issuing two requests for sccb_read_byte > > drivers/media/i2c/sccb.h | 74 ++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 74 insertions(+) > create mode 100644 drivers/media/i2c/sccb.h > > diff --git a/drivers/media/i2c/sccb.h b/drivers/media/i2c/sccb.h > new file mode 100644 > index 0000000..a531fdc > --- /dev/null > +++ b/drivers/media/i2c/sccb.h > @@ -0,0 +1,74 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Serial Camera Control Bus (SCCB) helper functions > + */ > + > +#ifndef __SCCB_H__ > +#define __SCCB_H__ > + > +#include <linux/i2c.h> > + > +/** > + * sccb_read_byte - Read data from SCCB slave device > + * @client: Handle to slave device > + * @addr: Register to be read from > + * > + * This executes the 2-phase write transmission cycle that is followed by a > + * 2-phase read transmission cycle, returning negative errno else a data byte > + * received from the device. > + */ > +static inline int sccb_read_byte(struct i2c_client *client, u8 addr) > +{ > + u8 val; > + struct i2c_msg msg[] = { > + { > + .addr = client->addr, > + .len = 1, > + .buf = &addr, > + }, > + { > + .addr = client->addr, > + .flags = I2C_M_RD, > + .len = 1, > + .buf = &val, > + }, > + }; > + int ret; > + int i; > + > + i2c_lock_adapter(client->adapter); I'd say that i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT); is more appropriate. Maybe we should have a i2c_lock_segment helper? Cheers, Peter > + > + /* Issue two separated requests in order to avoid repeated start */ > + for (i = 0; i < 2; i++) { > + ret = __i2c_transfer(client->adapter, &msg[i], 1); > + if (ret != 1) > + break; > + } > + > + i2c_unlock_adapter(client->adapter); > + > + return i == 2 ? val : ret; > +} > + > +/** > + * sccb_write_byte - Write data to SCCB slave device > + * @client: Handle to slave device > + * @addr: Register to write to > + * @data: Value to be written > + * > + * This executes the SCCB 3-phase write transmission cycle, returning negative > + * errno else zero on success. > + */ > +static inline int sccb_write_byte(struct i2c_client *client, u8 addr, u8 data) > +{ > + int ret; > + unsigned char msgbuf[] = { addr, data }; > + > + ret = i2c_master_send(client, msgbuf, 2); > + if (ret < 0) > + return ret; > + > + return 0; > +} > + > +#endif /* __SCCB_H__ */ >
On 2018-06-12 19:31, Peter Rosin wrote: > On 2018-06-12 17:34, Akinobu Mita wrote: >> (This is 2nd version of SCCB helpers patch. After 1st version was >> submitted, I sent alternative patch titled "i2c: add I2C_M_FORCE_STOP". >> But it wasn't accepted because it makes the I2C core code unreadable. >> I couldn't find out a way to untangle it, so I returned to the original >> approach.) >> >> This adds Serial Camera Control Bus (SCCB) helper functions (sccb_read_byte >> and sccb_write_byte) that are intended to be used by some of Omnivision >> sensor drivers. >> >> The ov772x driver is going to use these functions in order to make it work >> with most i2c controllers. >> >> As the ov772x device doesn't support repeated starts, this driver currently >> requires I2C_FUNC_PROTOCOL_MANGLING that is not supported by many i2c >> controller drivers. >> >> With the sccb_read_byte() that issues two separated requests in order to >> avoid repeated start, the driver doesn't require I2C_FUNC_PROTOCOL_MANGLING. >> >> Cc: Sebastian Reichel <sebastian.reichel@collabora.co.uk> >> Cc: Wolfram Sang <wsa@the-dreams.de> >> Cc: Jacopo Mondi <jacopo+renesas@jmondi.org> >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> Cc: Hans Verkuil <hans.verkuil@cisco.com> >> Cc: Sakari Ailus <sakari.ailus@linux.intel.com> >> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com> >> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> >> --- >> * v2 >> - Convert all helpers into static inline functions, and remove C source >> and Kconfig option. >> - Acquire i2c adapter lock while issuing two requests for sccb_read_byte >> >> drivers/media/i2c/sccb.h | 74 ++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 74 insertions(+) >> create mode 100644 drivers/media/i2c/sccb.h >> >> diff --git a/drivers/media/i2c/sccb.h b/drivers/media/i2c/sccb.h >> new file mode 100644 >> index 0000000..a531fdc >> --- /dev/null >> +++ b/drivers/media/i2c/sccb.h >> @@ -0,0 +1,74 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +/* >> + * Serial Camera Control Bus (SCCB) helper functions >> + */ >> + >> +#ifndef __SCCB_H__ >> +#define __SCCB_H__ >> + >> +#include <linux/i2c.h> >> + >> +/** >> + * sccb_read_byte - Read data from SCCB slave device >> + * @client: Handle to slave device >> + * @addr: Register to be read from >> + * >> + * This executes the 2-phase write transmission cycle that is followed by a >> + * 2-phase read transmission cycle, returning negative errno else a data byte >> + * received from the device. >> + */ >> +static inline int sccb_read_byte(struct i2c_client *client, u8 addr) >> +{ >> + u8 val; >> + struct i2c_msg msg[] = { >> + { >> + .addr = client->addr, >> + .len = 1, >> + .buf = &addr, >> + }, >> + { >> + .addr = client->addr, >> + .flags = I2C_M_RD, >> + .len = 1, >> + .buf = &val, >> + }, >> + }; >> + int ret; >> + int i; >> + >> + i2c_lock_adapter(client->adapter); > > I'd say that > > i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT); > > is more appropriate. Maybe we should have a i2c_lock_segment helper? Hmmm, I looked into that and happened to scan the other callers of i2c_lock_adapter. And, AFAICT, almost all callers outside drivers/i2c/ would benefit from only locking the segment. The only exception I could find was in drivers/iio/temperature/mlx90614.c which seems to want to protect the adapter from trying to use the I2C bus while the device is in a strange state (I assume it is somehow causing glitches on the I2C bus as it wakes up). So, maybe the easier thing to do is change i2c_lock_adapter to only lock the segment, and then have the callers beneath drivers/i2c/ (plus the above mlx90614 driver) that really want to lock the root adapter instead of the segment adapter call a new function named i2c_lock_root (or something like that). Admittedly, that will be a few more trivial changes, but all but one will be under the I2C umbrella and thus require less interaction. Wolfram, what do you think? Cheers, Peter >> + >> + /* Issue two separated requests in order to avoid repeated start */ >> + for (i = 0; i < 2; i++) { >> + ret = __i2c_transfer(client->adapter, &msg[i], 1); >> + if (ret != 1) >> + break; >> + } >> + >> + i2c_unlock_adapter(client->adapter); >> + >> + return i == 2 ? val : ret; >> +} >> + >> +/** >> + * sccb_write_byte - Write data to SCCB slave device >> + * @client: Handle to slave device >> + * @addr: Register to write to >> + * @data: Value to be written >> + * >> + * This executes the SCCB 3-phase write transmission cycle, returning negative >> + * errno else zero on success. >> + */ >> +static inline int sccb_write_byte(struct i2c_client *client, u8 addr, u8 data) >> +{ >> + int ret; >> + unsigned char msgbuf[] = { addr, data }; >> + >> + ret = i2c_master_send(client, msgbuf, 2); >> + if (ret < 0) >> + return ret; >> + >> + return 0; >> +} >> + >> +#endif /* __SCCB_H__ */ >> >
On Wed, Jun 13, 2018 at 12:34:46AM +0900, Akinobu Mita wrote: > (This is 2nd version of SCCB helpers patch. After 1st version was > submitted, I sent alternative patch titled "i2c: add I2C_M_FORCE_STOP". > But it wasn't accepted because it makes the I2C core code unreadable. > I couldn't find out a way to untangle it, so I returned to the original > approach.) > > This adds Serial Camera Control Bus (SCCB) helper functions (sccb_read_byte > and sccb_write_byte) that are intended to be used by some of Omnivision > sensor drivers. > > The ov772x driver is going to use these functions in order to make it work > with most i2c controllers. > > As the ov772x device doesn't support repeated starts, this driver currently > requires I2C_FUNC_PROTOCOL_MANGLING that is not supported by many i2c > controller drivers. > > With the sccb_read_byte() that issues two separated requests in order to > avoid repeated start, the driver doesn't require I2C_FUNC_PROTOCOL_MANGLING. From a first glance, this looks like my preferred solution so far. Thanks for doing it! Let me sleep a bit over it for a thorough review... > --- /dev/null > +++ b/drivers/media/i2c/sccb.h I'd prefer this file to be in the i2c realm. Maybe 'include/linux/i2c-sccb.h" or something. I will come back to this.
> So, maybe the easier thing to do is change i2c_lock_adapter to only > lock the segment, and then have the callers beneath drivers/i2c/ > (plus the above mlx90614 driver) that really want to lock the root > adapter instead of the segment adapter call a new function named > i2c_lock_root (or something like that). Admittedly, that will be > a few more trivial changes, but all but one will be under the I2C > umbrella and thus require less interaction. > > Wolfram, what do you think? It sounds tempting, yet I am concerned about regressions. From that point of view, it is safer to introduce i2c_lock_segment() and convert the users which would benefit from that. How many drivers would be affected?
On 2018-06-14 17:41, Wolfram Sang wrote: > >> So, maybe the easier thing to do is change i2c_lock_adapter to only >> lock the segment, and then have the callers beneath drivers/i2c/ >> (plus the above mlx90614 driver) that really want to lock the root >> adapter instead of the segment adapter call a new function named >> i2c_lock_root (or something like that). Admittedly, that will be >> a few more trivial changes, but all but one will be under the I2C >> umbrella and thus require less interaction. >> >> Wolfram, what do you think? > > It sounds tempting, yet I am concerned about regressions. From that > point of view, it is safer to introduce i2c_lock_segment() and convert the > users which would benefit from that. How many drivers would be affected? Right, there is also the aspect that changing a function like this might surprise people. Maybe i2c_lock_adapter should be killed and all callers changed to one of i2c_lock_segment and i2c_lock_root? It's not that much churn... Current callers (I didn't hunt the very latest sources, nor -next) of i2c_lock_adapter are: drivers/i2c/i2c-core-slave.c Locks the adapter during (un)registration of the slave. Should keep locking the root adapter. drivers/i2c/busses/i2c-brcmstb.c Locks the adapter during suspend/resume. Should keep locking the root adapter. drivers/i2c/busses/i2c-davinci.c Locks the adapter if/when the CPU frequency changes. Should keep locking the root adapter. drivers/i2c/busses/i2c-gpio.c Locks the adapter while twiddling the lines from debugfs. Should keep locking the root adapter. drivers/i2c/busses/i2c-s3c2410.c Locks the adapter if/when the CPU frequency changes.Should keep locking the root adapter. drivers/i2c/busses/i2c-sprd.c Locks the adapter during suspend/resume. Should keep locking the root adapter. drivers/i2c/busses/i2c-tegra.c Locks the adapter during suspend/resume. Should keep locking the root adapter. drivers/i2c/muxes/i2c-mux-pca9541.c Locks the adapter during probe to make I2C transfers. Should only need to lock the segment. drivers/iio/temperature/mlx90614.c Mentioned previously up-thread, suspend/resume apparently does nasty things with the bus. Should probably keep locking the root adapter. drivers/char/tpm/tpm_i2c_infineon.c Does a bunch of __i2c_transfer calls inside the locked regions (and some sleeping). Should only need to lock the segment. drivers/input/touchscreen/rohm_bu21023.c Does a couple of __i2c_transfer calls inside the locked region. Should only need to lock the segment. drivers/media/dvb-frontends/af9013.c Does a one or two __i2c_transfer calls inside the locked regions. Should only need to lock the segment. drivers/media/dvb-frontends/drxk_hard.c This one is a bit hairy. It does all sorts of things inside the locked region. And it is a bit hard to say if the root adapter really needs to be locked. drivers/media/dvb-frontends/rtl2830.c Does regmap accesses inside the locked regions, with the regmap ops overridden to use __i2c_transfer. Should only need to lock the segment. drivers/media/dvb-frontends/tda1004x.c Does a bunch of __i2c_transfer calls inside the locked region. Should only need to lock the segment. drivers/media/tuners/tda18271-common.c Seems to opens a gate in conjunction with locking the adapter. So, I'm a bit uncertain what will happen on the other side of that gate if there is any stray thing happening on the I2C bus. drivers/mfd/88pm860x-i2c.c Does a bunch of adap->algo->master_xfer calls inside the locked regions. Should only need to lock the segment (and should probably be using __i2c_transfer). So, drxk_hard.c and tda18271-common.c are a bit uncertain. However, since these drivers do make calls to __i2c_transfer from inside their locked regions, both drivers will deadlock if the chips sit downstream from a mux-locked I2C mux. And if they don't, locking the segment and the adapter are equivalent. So, the risk of regressions are nil AFAICT. Famous last words... Cheers, Peter
> > It sounds tempting, yet I am concerned about regressions. From that > > point of view, it is safer to introduce i2c_lock_segment() and convert the > > users which would benefit from that. How many drivers would be affected? > > Right, there is also the aspect that changing a function like this > might surprise people. Maybe i2c_lock_adapter should be killed and > all callers changed to one of i2c_lock_segment and i2c_lock_root? Yes, I like this one. It makes the change very clear to people. > It's not that much churn... OK, convinced. Are you willing/able to take on this? We are close to rc1 which would be a very good timing because a) linux-next should be almost empty and b) we have nearly one cycle for linux-next and one cycle for the rc-phase to get as much testing as possible. But also, there is no actual need to rush... This means for the original SCCB patch, it is independent of our thoughts here. If SCCB gets in first, we will just convert it, too. Thanks Peter!
Hello, On Thursday, 14 June 2018 18:33:58 EEST Wolfram Sang wrote: > On Wed, Jun 13, 2018 at 12:34:46AM +0900, Akinobu Mita wrote: > > (This is 2nd version of SCCB helpers patch. After 1st version was > > submitted, I sent alternative patch titled "i2c: add I2C_M_FORCE_STOP". > > But it wasn't accepted because it makes the I2C core code unreadable. > > I couldn't find out a way to untangle it, so I returned to the original > > approach.) > > > > This adds Serial Camera Control Bus (SCCB) helper functions > > (sccb_read_byte and sccb_write_byte) that are intended to be used by some > > of Omnivision sensor drivers. > > > > The ov772x driver is going to use these functions in order to make it work > > with most i2c controllers. > > > > As the ov772x device doesn't support repeated starts, this driver > > currently requires I2C_FUNC_PROTOCOL_MANGLING that is not supported by > > many i2c controller drivers. > > > > With the sccb_read_byte() that issues two separated requests in order to > > avoid repeated start, the driver doesn't require > > I2C_FUNC_PROTOCOL_MANGLING. > > From a first glance, this looks like my preferred solution so far. > Thanks for doing it! Let me sleep a bit over it for a thorough review... > > > --- /dev/null > > +++ b/drivers/media/i2c/sccb.h > > I'd prefer this file to be in the i2c realm. Maybe > 'include/linux/i2c-sccb.h" or something. I will come back to this. And while at it, I think we also need a .c file, the functions (and especially sccb_read_byte()) should not be static inline.
> > I'd prefer this file to be in the i2c realm. Maybe > > 'include/linux/i2c-sccb.h" or something. I will come back to this. > > And while at it, I think we also need a .c file, the functions (and especially > sccb_read_byte()) should not be static inline. Before we discuss this, we should make sure that the read-function is complete and then we can decide further. I found some old notes based on our previous discussions about SCCB and refactoring its access. You mentioned there is HW supporting SCCB natively. Also, we found a device where an SCCB device is connected to an SMBUS controller (yuck!). So, given that, I'd think the read routine should look like this (in pseudo code): bool is_sccb_available(adap) { u32 needed_funcs = I2C_FUNC_SMBUS_BYTE | I2C_FUNC_SMBUS_WRITE_BYTE_DATA; /* * sccb_xfer not needed yet, since there is no driver support currently. * Just showing how it should be done if we ever need it. */ if (adap->algo->sccb_xfer) return true; if (i2c_get_functionality(adap) & needed_funcs == needed_funcs) return true; return false; } sccb_get_byte() { if (adap->algo->sccb_xfer) adap->algo->sccb_xfer(...) /* * Prereq: __i2c_smbus_xfer needs to be created! */ i2c_lock_adapter(SEGMENT); __i2c_smbus_xfer(..., SEND_BYTE, ...) __i2c_smbus_xfer(..., GET_BYTE, ...) i2c_unlock_adapter(SEGMENT) } sccb_write_byte() { return i2c_smbus_write_byte_data(...) } If I haven't overlooked something, this should make SCCB possible on I2C controllers and SMBus controllers. We honor the requirement of not having repeated start anywhere. As such, we might get even rid of the I2C_M_STOP flag (for in-kernel users, but we sadly export it to userspace). About the IGNORE_NAK flag, I think this still should work where supported as it is passed along indirectly with I2C_CLIENT_SCCB. However, this flag handling with SCCB is really a mess and needs an overhaul. This can be a second step, however. Most devices don't really need it, do they? Any further comments? Anything I missed? Kind regards, Wolfram
diff --git a/drivers/media/i2c/sccb.h b/drivers/media/i2c/sccb.h new file mode 100644 index 0000000..a531fdc --- /dev/null +++ b/drivers/media/i2c/sccb.h @@ -0,0 +1,74 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Serial Camera Control Bus (SCCB) helper functions + */ + +#ifndef __SCCB_H__ +#define __SCCB_H__ + +#include <linux/i2c.h> + +/** + * sccb_read_byte - Read data from SCCB slave device + * @client: Handle to slave device + * @addr: Register to be read from + * + * This executes the 2-phase write transmission cycle that is followed by a + * 2-phase read transmission cycle, returning negative errno else a data byte + * received from the device. + */ +static inline int sccb_read_byte(struct i2c_client *client, u8 addr) +{ + u8 val; + struct i2c_msg msg[] = { + { + .addr = client->addr, + .len = 1, + .buf = &addr, + }, + { + .addr = client->addr, + .flags = I2C_M_RD, + .len = 1, + .buf = &val, + }, + }; + int ret; + int i; + + i2c_lock_adapter(client->adapter); + + /* Issue two separated requests in order to avoid repeated start */ + for (i = 0; i < 2; i++) { + ret = __i2c_transfer(client->adapter, &msg[i], 1); + if (ret != 1) + break; + } + + i2c_unlock_adapter(client->adapter); + + return i == 2 ? val : ret; +} + +/** + * sccb_write_byte - Write data to SCCB slave device + * @client: Handle to slave device + * @addr: Register to write to + * @data: Value to be written + * + * This executes the SCCB 3-phase write transmission cycle, returning negative + * errno else zero on success. + */ +static inline int sccb_write_byte(struct i2c_client *client, u8 addr, u8 data) +{ + int ret; + unsigned char msgbuf[] = { addr, data }; + + ret = i2c_master_send(client, msgbuf, 2); + if (ret < 0) + return ret; + + return 0; +} + +#endif /* __SCCB_H__ */
(This is 2nd version of SCCB helpers patch. After 1st version was submitted, I sent alternative patch titled "i2c: add I2C_M_FORCE_STOP". But it wasn't accepted because it makes the I2C core code unreadable. I couldn't find out a way to untangle it, so I returned to the original approach.) This adds Serial Camera Control Bus (SCCB) helper functions (sccb_read_byte and sccb_write_byte) that are intended to be used by some of Omnivision sensor drivers. The ov772x driver is going to use these functions in order to make it work with most i2c controllers. As the ov772x device doesn't support repeated starts, this driver currently requires I2C_FUNC_PROTOCOL_MANGLING that is not supported by many i2c controller drivers. With the sccb_read_byte() that issues two separated requests in order to avoid repeated start, the driver doesn't require I2C_FUNC_PROTOCOL_MANGLING. Cc: Sebastian Reichel <sebastian.reichel@collabora.co.uk> Cc: Wolfram Sang <wsa@the-dreams.de> Cc: Jacopo Mondi <jacopo+renesas@jmondi.org> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Hans Verkuil <hans.verkuil@cisco.com> Cc: Sakari Ailus <sakari.ailus@linux.intel.com> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> --- * v2 - Convert all helpers into static inline functions, and remove C source and Kconfig option. - Acquire i2c adapter lock while issuing two requests for sccb_read_byte drivers/media/i2c/sccb.h | 74 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+) create mode 100644 drivers/media/i2c/sccb.h