Hi Akinobu,
Thank you for the patch.
On Monday, 9 July 2018 18:41:13 EEST Akinobu Mita wrote:
> This adds Serial Camera Control Bus (SCCB) helpers (sccb_is_available,
> 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 helpers.
>
> It was previously only worked with the i2c controller drivers that support
> I2C_FUNC_PROTOCOL_MANGLING, because the ov772x device doesn't support
> repeated starts. After commit 0b964d183cbf ("media: ov772x: allow i2c
> controllers without I2C_FUNC_PROTOCOL_MANGLING"), reading ov772x register
> is replaced with issuing two separated i2c messages in order to avoid
> repeated start. Using SCCB helpers hides the implementation detail.
>
> Cc: Peter Rosin <p...@axentia.se>
> Cc: Sebastian Reichel <sebastian.reic...@collabora.co.uk>
> Cc: Wolfram Sang <w...@the-dreams.de>
> Cc: Jacopo Mondi <jacopo+rene...@jmondi.org>
> Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> Cc: Hans Verkuil <hans.verk...@cisco.com>
> Cc: Sakari Ailus <sakari.ai...@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mche...@s-opensource.com>
> Signed-off-by: Akinobu Mita <akinobu.m...@gmail.com>
> ---
> include/linux/i2c-sccb.h | 77 +++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 77 insertions(+)
> create mode 100644 include/linux/i2c-sccb.h
>
> diff --git a/include/linux/i2c-sccb.h b/include/linux/i2c-sccb.h
> new file mode 100644
> index 0000000..61dece9
> --- /dev/null
> +++ b/include/linux/i2c-sccb.h
> @@ -0,0 +1,77 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Serial Camera Control Bus (SCCB) helper functions
> + */
> +
> +#ifndef _LINUX_I2C_SCCB_H
> +#define _LINUX_I2C_SCCB_H
> +
> +#include <linux/i2c.h>
> +
> +/**
> + * sccb_is_available - Check if the adapter supports SCCB protocol
> + * @adap: I2C adapter
> + *
> + * Return true if the I2C adapter is capable of using SCCB helper
> functions, + * false otherwise.
> + */
> +static inline bool sccb_is_available(struct i2c_adapter *adap)
> +{
> + u32 needed_funcs = I2C_FUNC_SMBUS_BYTE | I2C_FUNC_SMBUS_WRITE_BYTE_DATA;
> +
> +#if 0
> + /*
> + * 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;
> +#endif
> +
> + return (i2c_get_functionality(adap) & needed_funcs) == needed_funcs;
> +}
> +
> +/**
> + * 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)
> +{
> + int ret;
> + union i2c_smbus_data data;
> +
> + i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +
> + ret = __i2c_smbus_xfer(client->adapter, client->addr, client->flags,
> + I2C_SMBUS_WRITE, addr, I2C_SMBUS_BYTE, NULL);
> + if (ret < 0)
> + goto out;
> +
> + ret = __i2c_smbus_xfer(client->adapter, client->addr, client->flags,
> + I2C_SMBUS_READ, 0, I2C_SMBUS_BYTE, &data);
> +out:
> + i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +
> + return ret < 0 ? ret : data.byte;
> +}
I think I mentioned in a previous review of this patch that the function is
too big to be a static inline. It should instead be moved to a .c file.
> +/**
> + * 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)
> +{
> + return i2c_smbus_write_byte_data(client, addr, data);
> +}
> +
> +#endif /* _LINUX_I2C_SCCB_H */
--
Regards,
Laurent Pinchart
