[RFC,v2] media: i2c: add SCCB helpers

(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

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