[PATCH -next v4 0/3] introduce SCCB regmap

[PATCH -next v4 0/3] introduce SCCB regmap

[Akinobu Mita]

This patchset introduces Serial Camera Control Bus (SCCB) support for
regmap API and convert ov772x and ov9650 drivers to use it.

This patchset was previously submitted as "introduce SCCB helpers"
that provides three functions (sccb_is_available, sccb_read_byte, and
sccb_write_byte).  This time, the helpers are replaced by regmap API,
but internal code is not much changed from the previous version.

* v4
- Introduce SCCB regmap instead of helper functions,
  suggested by Sebastian Reichel
- Change ov772x driver to use regmap instead of helper functions
- Add register access conversion for ov9650 driver

* v3
- Rewrite the helpers based on the code provided by Wolfram
- Convert ov772x driver to use SCCB helpers

 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

Akinobu Mita (3):
  regmap: add SCCB support
  media: ov772x: use SCCB regmap
  media: ov9650: use SCCB regmap

 drivers/base/regmap/Kconfig       |   4 +
 drivers/base/regmap/Makefile      |   1 +
 drivers/base/regmap/regmap-sccb.c | 128 +++++++++++++++++++++++++
 drivers/media/i2c/Kconfig         |   2 +
 drivers/media/i2c/ov772x.c        | 192 ++++++++++++++++----------------------
 drivers/media/i2c/ov9650.c        | 157 +++++++++++++++----------------
 include/linux/regmap.h            |  35 +++++++
 7 files changed, 326 insertions(+), 193 deletions(-)
 create mode 100644 drivers/base/regmap/regmap-sccb.c

Cc: Mark Brown <broonie@kernel.org>
Cc: Peter Rosin <peda@axentia.se>
Cc: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
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>
-- 
2.7.4

[PATCH -next v4 1/3] regmap: add SCCB support

[Akinobu Mita]

This adds Serial Camera Control Bus (SCCB) support for regmap API that
is intended to be used by some of Omnivision sensor drivers.

The ov772x and ov9650 drivers are going to use this SCCB regmap API.

The ov772x driver 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 this SCCB regmap hides the implementation detail.

The ov9650 driver also issues two separated i2c messages to read the
registers as the device doesn't support repeated start.  So it can
make use of this SCCB regmap.

Cc: Mark Brown <broonie@kernel.org>
Cc: Peter Rosin <peda@axentia.se>
Cc: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
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>
---
 drivers/base/regmap/Kconfig       |   4 ++
 drivers/base/regmap/Makefile      |   1 +
 drivers/base/regmap/regmap-sccb.c | 128 ++++++++++++++++++++++++++++++++++++++
 include/linux/regmap.h            |  35 +++++++++++
 4 files changed, 168 insertions(+)
 create mode 100644 drivers/base/regmap/regmap-sccb.c

diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig
index aff34c0..6ad5ef4 100644
--- a/drivers/base/regmap/Kconfig
+++ b/drivers/base/regmap/Kconfig
@@ -45,3 +45,7 @@ config REGMAP_IRQ
 config REGMAP_SOUNDWIRE
 	tristate
 	depends on SOUNDWIRE_BUS
+
+config REGMAP_SCCB
+	tristate
+	depends on I2C
diff --git a/drivers/base/regmap/Makefile b/drivers/base/regmap/Makefile
index 5ed0023..f5b4e88 100644
--- a/drivers/base/regmap/Makefile
+++ b/drivers/base/regmap/Makefile
@@ -15,3 +15,4 @@ obj-$(CONFIG_REGMAP_MMIO) += regmap-mmio.o
 obj-$(CONFIG_REGMAP_IRQ) += regmap-irq.o
 obj-$(CONFIG_REGMAP_W1) += regmap-w1.o
 obj-$(CONFIG_REGMAP_SOUNDWIRE) += regmap-sdw.o
+obj-$(CONFIG_REGMAP_SCCB) += regmap-sccb.o
diff --git a/drivers/base/regmap/regmap-sccb.c b/drivers/base/regmap/regmap-sccb.c
new file mode 100644
index 0000000..b6eb876
--- /dev/null
+++ b/drivers/base/regmap/regmap-sccb.c
@@ -0,0 +1,128 @@
+// SPDX-License-Identifier: GPL-2.0
+// Register map access API - SCCB support
+
+#include <linux/regmap.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+
+#include "internal.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 bool sccb_is_available(struct i2c_adapter *adap)
+{
+	u32 needed_funcs = I2C_FUNC_SMBUS_BYTE | I2C_FUNC_SMBUS_WRITE_BYTE_DATA;
+
+	/*
+	 * If we ever want support for hardware doing SCCB natively, we will
+	 * introduce a sccb_xfer() callback to struct i2c_algorithm and check
+	 * for it here.
+	 */
+
+	return (i2c_get_functionality(adap) & needed_funcs) == needed_funcs;
+}
+
+/**
+ * regmap_sccb_read - Read data from SCCB slave device
+ * @context: Device that will be interacted wit
+ * @reg: Register to be read from
+ * @val: Pointer to store read value
+ *
+ * This executes the 2-phase write transmission cycle that is followed by a
+ * 2-phase read transmission cycle, returning negative errno else zero on
+ * success.
+ */
+static int regmap_sccb_read(void *context, unsigned int reg, unsigned int *val)
+{
+	struct device *dev = context;
+	struct i2c_client *i2c = to_i2c_client(dev);
+	int ret;
+	union i2c_smbus_data data;
+
+	i2c_lock_bus(i2c->adapter, I2C_LOCK_SEGMENT);
+
+	ret = __i2c_smbus_xfer(i2c->adapter, i2c->addr, i2c->flags,
+			       I2C_SMBUS_WRITE, reg, I2C_SMBUS_BYTE, NULL);
+	if (ret < 0)
+		goto out;
+
+	ret = __i2c_smbus_xfer(i2c->adapter, i2c->addr, i2c->flags,
+			       I2C_SMBUS_READ, 0, I2C_SMBUS_BYTE, &data);
+	if (ret < 0)
+		goto out;
+
+	*val = data.byte;
+out:
+	i2c_unlock_bus(i2c->adapter, I2C_LOCK_SEGMENT);
+
+	return ret;
+}
+
+/**
+ * regmap_sccb_write - Write data to SCCB slave device
+ * @context: Device that will be interacted wit
+ * @reg: Register to write to
+ * @val: Value to be written
+ *
+ * This executes the SCCB 3-phase write transmission cycle, returning negative
+ * errno else zero on success.
+ */
+static int regmap_sccb_write(void *context, unsigned int reg, unsigned int val)
+{
+	struct device *dev = context;
+	struct i2c_client *i2c = to_i2c_client(dev);
+
+	return i2c_smbus_write_byte_data(i2c, reg, val);
+}
+
+static struct regmap_bus regmap_sccb_bus = {
+	.reg_write = regmap_sccb_write,
+	.reg_read = regmap_sccb_read,
+};
+
+static const struct regmap_bus *regmap_get_sccb_bus(struct i2c_client *i2c,
+					const struct regmap_config *config)
+{
+	if (config->val_bits == 8 && config->reg_bits == 8 &&
+			sccb_is_available(i2c->adapter))
+		return &regmap_sccb_bus;
+
+	return ERR_PTR(-ENOTSUPP);
+}
+
+struct regmap *__regmap_init_sccb(struct i2c_client *i2c,
+				  const struct regmap_config *config,
+				  struct lock_class_key *lock_key,
+				  const char *lock_name)
+{
+	const struct regmap_bus *bus = regmap_get_sccb_bus(i2c, config);
+
+	if (IS_ERR(bus))
+		return ERR_CAST(bus);
+
+	return __regmap_init(&i2c->dev, bus, &i2c->dev, config,
+			     lock_key, lock_name);
+}
+EXPORT_SYMBOL_GPL(__regmap_init_sccb);
+
+struct regmap *__devm_regmap_init_sccb(struct i2c_client *i2c,
+				       const struct regmap_config *config,
+				       struct lock_class_key *lock_key,
+				       const char *lock_name)
+{
+	const struct regmap_bus *bus = regmap_get_sccb_bus(i2c, config);
+
+	if (IS_ERR(bus))
+		return ERR_CAST(bus);
+
+	return __devm_regmap_init(&i2c->dev, bus, &i2c->dev, config,
+				  lock_key, lock_name);
+}
+EXPORT_SYMBOL_GPL(__devm_regmap_init_sccb);
+
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 4f38068..52bf358 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -514,6 +514,10 @@ struct regmap *__regmap_init_i2c(struct i2c_client *i2c,
 				 const struct regmap_config *config,
 				 struct lock_class_key *lock_key,
 				 const char *lock_name);
+struct regmap *__regmap_init_sccb(struct i2c_client *i2c,
+				  const struct regmap_config *config,
+				  struct lock_class_key *lock_key,
+				  const char *lock_name);
 struct regmap *__regmap_init_slimbus(struct slim_device *slimbus,
 				 const struct regmap_config *config,
 				 struct lock_class_key *lock_key,
@@ -558,6 +562,10 @@ struct regmap *__devm_regmap_init_i2c(struct i2c_client *i2c,
 				      const struct regmap_config *config,
 				      struct lock_class_key *lock_key,
 				      const char *lock_name);
+struct regmap *__devm_regmap_init_sccb(struct i2c_client *i2c,
+				       const struct regmap_config *config,
+				       struct lock_class_key *lock_key,
+				       const char *lock_name);
 struct regmap *__devm_regmap_init_spi(struct spi_device *dev,
 				      const struct regmap_config *config,
 				      struct lock_class_key *lock_key,
@@ -646,6 +654,19 @@ int regmap_attach_dev(struct device *dev, struct regmap *map,
 				i2c, config)
 
 /**
+ * regmap_init_sccb() - Initialise register map
+ *
+ * @i2c: Device that will be interacted with
+ * @config: Configuration for register map
+ *
+ * The return value will be an ERR_PTR() on error or a valid pointer to
+ * a struct regmap.
+ */
+#define regmap_init_sccb(i2c, config)					\
+	__regmap_lockdep_wrapper(__regmap_init_sccb, #config,		\
+				i2c, config)
+
+/**
  * regmap_init_slimbus() - Initialise register map
  *
  * @slimbus: Device that will be interacted with
@@ -798,6 +819,20 @@ bool regmap_ac97_default_volatile(struct device *dev, unsigned int reg);
 				i2c, config)
 
 /**
+ * devm_regmap_init_sccb() - Initialise managed register map
+ *
+ * @i2c: Device that will be interacted with
+ * @config: Configuration for register map
+ *
+ * The return value will be an ERR_PTR() on error or a valid pointer
+ * to a struct regmap.  The regmap will be automatically freed by the
+ * device management code.
+ */
+#define devm_regmap_init_sccb(i2c, config)				\
+	__regmap_lockdep_wrapper(__devm_regmap_init_sccb, #config,	\
+				i2c, config)
+
+/**
  * devm_regmap_init_spi() - Initialise register map
  *
  * @dev: Device that will be interacted with
-- 
2.7.4

[PATCH -next v4 2/3] media: ov772x: use SCCB regmap

[Akinobu Mita]

Convert ov772x register access to use SCCB regmap.

Cc: Mark Brown <broonie@kernel.org>
Cc: Peter Rosin <peda@axentia.se>
Cc: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
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>
---
 drivers/media/i2c/Kconfig  |   1 +
 drivers/media/i2c/ov772x.c | 192 +++++++++++++++++++--------------------------
 2 files changed, 82 insertions(+), 111 deletions(-)

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 341452f..b923a51 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -715,6 +715,7 @@ config VIDEO_OV772X
 	tristate "OmniVision OV772x sensor support"
 	depends on I2C && VIDEO_V4L2
 	depends on MEDIA_CAMERA_SUPPORT
+	select REGMAP_SCCB
 	---help---
 	  This is a Video4Linux2 sensor-level driver for the OmniVision
 	  OV772x camera.
diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 7158c31..0d3ed23 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -21,6 +21,7 @@
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/regmap.h>
 #include <linux/slab.h>
 #include <linux/v4l2-mediabus.h>
 #include <linux/videodev2.h>
@@ -414,6 +415,7 @@ struct ov772x_priv {
 	struct v4l2_subdev                subdev;
 	struct v4l2_ctrl_handler	  hdl;
 	struct clk			 *clk;
+	struct regmap			 *regmap;
 	struct ov772x_camera_info        *info;
 	struct gpio_desc		 *pwdn_gpio;
 	struct gpio_desc		 *rstb_gpio;
@@ -549,51 +551,18 @@ static struct ov772x_priv *to_ov772x(struct v4l2_subdev *sd)
 	return container_of(sd, struct ov772x_priv, subdev);
 }
 
-static int ov772x_read(struct i2c_client *client, u8 addr)
-{
-	int ret;
-	u8 val;
-
-	ret = i2c_master_send(client, &addr, 1);
-	if (ret < 0)
-		return ret;
-	ret = i2c_master_recv(client, &val, 1);
-	if (ret < 0)
-		return ret;
-
-	return val;
-}
-
-static inline int ov772x_write(struct i2c_client *client, u8 addr, u8 value)
-{
-	return i2c_smbus_write_byte_data(client, addr, value);
-}
-
-static int ov772x_mask_set(struct i2c_client *client, u8  command, u8  mask,
-			   u8  set)
-{
-	s32 val = ov772x_read(client, command);
-
-	if (val < 0)
-		return val;
-
-	val &= ~mask;
-	val |= set & mask;
-
-	return ov772x_write(client, command, val);
-}
-
-static int ov772x_reset(struct i2c_client *client)
+static int ov772x_reset(struct ov772x_priv *priv)
 {
 	int ret;
 
-	ret = ov772x_write(client, COM7, SCCB_RESET);
+	ret = regmap_write(priv->regmap, COM7, SCCB_RESET);
 	if (ret < 0)
 		return ret;
 
 	usleep_range(1000, 5000);
 
-	return ov772x_mask_set(client, COM2, SOFT_SLEEP_MODE, SOFT_SLEEP_MODE);
+	return regmap_update_bits(priv->regmap, COM2, SOFT_SLEEP_MODE,
+				  SOFT_SLEEP_MODE);
 }
 
 /*
@@ -611,8 +580,8 @@ static int ov772x_s_stream(struct v4l2_subdev *sd, int enable)
 	if (priv->streaming == enable)
 		goto done;
 
-	ret = ov772x_mask_set(client, COM2, SOFT_SLEEP_MODE,
-			      enable ? 0 : SOFT_SLEEP_MODE);
+	ret = regmap_update_bits(priv->regmap, COM2, SOFT_SLEEP_MODE,
+				 enable ? 0 : SOFT_SLEEP_MODE);
 	if (ret)
 		goto done;
 
@@ -657,7 +626,6 @@ static int ov772x_set_frame_rate(struct ov772x_priv *priv,
 				 const struct ov772x_color_format *cfmt,
 				 const struct ov772x_win_size *win)
 {
-	struct i2c_client *client = v4l2_get_subdevdata(&priv->subdev);
 	unsigned long fin = clk_get_rate(priv->clk);
 	unsigned int best_diff;
 	unsigned int fsize;
@@ -723,11 +691,11 @@ static int ov772x_set_frame_rate(struct ov772x_priv *priv,
 		}
 	}
 
-	ret = ov772x_write(client, COM4, com4 | COM4_RESERVED);
+	ret = regmap_write(priv->regmap, COM4, com4 | COM4_RESERVED);
 	if (ret < 0)
 		return ret;
 
-	ret = ov772x_write(client, CLKRC, clkrc | CLKRC_RESERVED);
+	ret = regmap_write(priv->regmap, CLKRC, clkrc | CLKRC_RESERVED);
 	if (ret < 0)
 		return ret;
 
@@ -788,8 +756,7 @@ static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct ov772x_priv *priv = container_of(ctrl->handler,
 						struct ov772x_priv, hdl);
-	struct v4l2_subdev *sd = &priv->subdev;
-	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct regmap *regmap = priv->regmap;
 	int ret = 0;
 	u8 val;
 
@@ -808,27 +775,27 @@ static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
 		val = ctrl->val ? VFLIP_IMG : 0x00;
 		if (priv->info && (priv->info->flags & OV772X_FLAG_VFLIP))
 			val ^= VFLIP_IMG;
-		return ov772x_mask_set(client, COM3, VFLIP_IMG, val);
+		return regmap_update_bits(regmap, COM3, VFLIP_IMG, val);
 	case V4L2_CID_HFLIP:
 		val = ctrl->val ? HFLIP_IMG : 0x00;
 		if (priv->info && (priv->info->flags & OV772X_FLAG_HFLIP))
 			val ^= HFLIP_IMG;
-		return ov772x_mask_set(client, COM3, HFLIP_IMG, val);
+		return regmap_update_bits(regmap, COM3, HFLIP_IMG, val);
 	case V4L2_CID_BAND_STOP_FILTER:
 		if (!ctrl->val) {
 			/* Switch the filter off, it is on now */
-			ret = ov772x_mask_set(client, BDBASE, 0xff, 0xff);
+			ret = regmap_update_bits(regmap, BDBASE, 0xff, 0xff);
 			if (!ret)
-				ret = ov772x_mask_set(client, COM8,
-						      BNDF_ON_OFF, 0);
+				ret = regmap_update_bits(regmap, COM8,
+							 BNDF_ON_OFF, 0);
 		} else {
 			/* Switch the filter on, set AEC low limit */
 			val = 256 - ctrl->val;
-			ret = ov772x_mask_set(client, COM8,
-					      BNDF_ON_OFF, BNDF_ON_OFF);
+			ret = regmap_update_bits(regmap, COM8,
+						 BNDF_ON_OFF, BNDF_ON_OFF);
 			if (!ret)
-				ret = ov772x_mask_set(client, BDBASE,
-						      0xff, val);
+				ret = regmap_update_bits(regmap, BDBASE,
+							 0xff, val);
 		}
 
 		return ret;
@@ -841,18 +808,19 @@ static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
 static int ov772x_g_register(struct v4l2_subdev *sd,
 			     struct v4l2_dbg_register *reg)
 {
-	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct ov772x_priv *priv = to_ov772x(sd);
 	int ret;
+	unsigned int val;
 
 	reg->size = 1;
 	if (reg->reg > 0xff)
 		return -EINVAL;
 
-	ret = ov772x_read(client, reg->reg);
+	ret = regmap_read(priv->regmap, reg->reg, &val);
 	if (ret < 0)
 		return ret;
 
-	reg->val = (__u64)ret;
+	reg->val = (__u64)val;
 
 	return 0;
 }
@@ -860,13 +828,13 @@ static int ov772x_g_register(struct v4l2_subdev *sd,
 static int ov772x_s_register(struct v4l2_subdev *sd,
 			     const struct v4l2_dbg_register *reg)
 {
-	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct ov772x_priv *priv = to_ov772x(sd);
 
 	if (reg->reg > 0xff ||
 	    reg->val > 0xff)
 		return -EINVAL;
 
-	return ov772x_write(client, reg->reg, reg->val);
+	return regmap_write(priv->regmap, reg->reg, reg->val);
 }
 #endif
 
@@ -1004,7 +972,7 @@ static void ov772x_select_params(const struct v4l2_mbus_framefmt *mf,
 
 static int ov772x_edgectrl(struct ov772x_priv *priv)
 {
-	struct i2c_client *client = v4l2_get_subdevdata(&priv->subdev);
+	struct regmap *regmap = priv->regmap;
 	int ret;
 
 	if (!priv->info)
@@ -1018,19 +986,19 @@ static int ov772x_edgectrl(struct ov772x_priv *priv)
 		 * Remove it when manual mode.
 		 */
 
-		ret = ov772x_mask_set(client, DSPAUTO, EDGE_ACTRL, 0x00);
+		ret = regmap_update_bits(regmap, DSPAUTO, EDGE_ACTRL, 0x00);
 		if (ret < 0)
 			return ret;
 
-		ret = ov772x_mask_set(client,
-				      EDGE_TRSHLD, OV772X_EDGE_THRESHOLD_MASK,
-				      priv->info->edgectrl.threshold);
+		ret = regmap_update_bits(regmap, EDGE_TRSHLD,
+					 OV772X_EDGE_THRESHOLD_MASK,
+					 priv->info->edgectrl.threshold);
 		if (ret < 0)
 			return ret;
 
-		ret = ov772x_mask_set(client,
-				      EDGE_STRNGT, OV772X_EDGE_STRENGTH_MASK,
-				      priv->info->edgectrl.strength);
+		ret = regmap_update_bits(regmap, EDGE_STRNGT,
+					 OV772X_EDGE_STRENGTH_MASK,
+					 priv->info->edgectrl.strength);
 		if (ret < 0)
 			return ret;
 
@@ -1040,15 +1008,15 @@ static int ov772x_edgectrl(struct ov772x_priv *priv)
 		 *
 		 * Set upper and lower limit.
 		 */
-		ret = ov772x_mask_set(client,
-				      EDGE_UPPER, OV772X_EDGE_UPPER_MASK,
-				      priv->info->edgectrl.upper);
+		ret = regmap_update_bits(regmap, EDGE_UPPER,
+					 OV772X_EDGE_UPPER_MASK,
+					 priv->info->edgectrl.upper);
 		if (ret < 0)
 			return ret;
 
-		ret = ov772x_mask_set(client,
-				      EDGE_LOWER, OV772X_EDGE_LOWER_MASK,
-				      priv->info->edgectrl.lower);
+		ret = regmap_update_bits(regmap, EDGE_LOWER,
+					 OV772X_EDGE_LOWER_MASK,
+					 priv->info->edgectrl.lower);
 		if (ret < 0)
 			return ret;
 	}
@@ -1060,12 +1028,11 @@ static int ov772x_set_params(struct ov772x_priv *priv,
 			     const struct ov772x_color_format *cfmt,
 			     const struct ov772x_win_size *win)
 {
-	struct i2c_client *client = v4l2_get_subdevdata(&priv->subdev);
 	int ret;
 	u8  val;
 
 	/* Reset hardware. */
-	ov772x_reset(client);
+	ov772x_reset(priv);
 
 	/* Edge Ctrl. */
 	ret = ov772x_edgectrl(priv);
@@ -1073,32 +1040,32 @@ static int ov772x_set_params(struct ov772x_priv *priv,
 		return ret;
 
 	/* Format and window size. */
-	ret = ov772x_write(client, HSTART, win->rect.left >> 2);
+	ret = regmap_write(priv->regmap, HSTART, win->rect.left >> 2);
 	if (ret < 0)
 		goto ov772x_set_fmt_error;
-	ret = ov772x_write(client, HSIZE, win->rect.width >> 2);
+	ret = regmap_write(priv->regmap, HSIZE, win->rect.width >> 2);
 	if (ret < 0)
 		goto ov772x_set_fmt_error;
-	ret = ov772x_write(client, VSTART, win->rect.top >> 1);
+	ret = regmap_write(priv->regmap, VSTART, win->rect.top >> 1);
 	if (ret < 0)
 		goto ov772x_set_fmt_error;
-	ret = ov772x_write(client, VSIZE, win->rect.height >> 1);
+	ret = regmap_write(priv->regmap, VSIZE, win->rect.height >> 1);
 	if (ret < 0)
 		goto ov772x_set_fmt_error;
-	ret = ov772x_write(client, HOUTSIZE, win->rect.width >> 2);
+	ret = regmap_write(priv->regmap, HOUTSIZE, win->rect.width >> 2);
 	if (ret < 0)
 		goto ov772x_set_fmt_error;
-	ret = ov772x_write(client, VOUTSIZE, win->rect.height >> 1);
+	ret = regmap_write(priv->regmap, VOUTSIZE, win->rect.height >> 1);
 	if (ret < 0)
 		goto ov772x_set_fmt_error;
-	ret = ov772x_write(client, HREF,
+	ret = regmap_write(priv->regmap, HREF,
 			   ((win->rect.top & 1) << HREF_VSTART_SHIFT) |
 			   ((win->rect.left & 3) << HREF_HSTART_SHIFT) |
 			   ((win->rect.height & 1) << HREF_VSIZE_SHIFT) |
 			   ((win->rect.width & 3) << HREF_HSIZE_SHIFT));
 	if (ret < 0)
 		goto ov772x_set_fmt_error;
-	ret = ov772x_write(client, EXHCH,
+	ret = regmap_write(priv->regmap, EXHCH,
 			   ((win->rect.height & 1) << EXHCH_VSIZE_SHIFT) |
 			   ((win->rect.width & 3) << EXHCH_HSIZE_SHIFT));
 	if (ret < 0)
@@ -1107,15 +1074,14 @@ static int ov772x_set_params(struct ov772x_priv *priv,
 	/* Set DSP_CTRL3. */
 	val = cfmt->dsp3;
 	if (val) {
-		ret = ov772x_mask_set(client,
-				      DSP_CTRL3, UV_MASK, val);
+		ret = regmap_update_bits(priv->regmap, DSP_CTRL3, UV_MASK, val);
 		if (ret < 0)
 			goto ov772x_set_fmt_error;
 	}
 
 	/* DSP_CTRL4: AEC reference point and DSP output format. */
 	if (cfmt->dsp4) {
-		ret = ov772x_write(client, DSP_CTRL4, cfmt->dsp4);
+		ret = regmap_write(priv->regmap, DSP_CTRL4, cfmt->dsp4);
 		if (ret < 0)
 			goto ov772x_set_fmt_error;
 	}
@@ -1131,13 +1097,12 @@ static int ov772x_set_params(struct ov772x_priv *priv,
 	if (priv->hflip_ctrl->val)
 		val ^= HFLIP_IMG;
 
-	ret = ov772x_mask_set(client,
-			      COM3, SWAP_MASK | IMG_MASK, val);
+	ret = regmap_update_bits(priv->regmap, COM3, SWAP_MASK | IMG_MASK, val);
 	if (ret < 0)
 		goto ov772x_set_fmt_error;
 
 	/* COM7: Sensor resolution and output format control. */
-	ret = ov772x_write(client, COM7, win->com7_bit | cfmt->com7);
+	ret = regmap_write(priv->regmap, COM7, win->com7_bit | cfmt->com7);
 	if (ret < 0)
 		goto ov772x_set_fmt_error;
 
@@ -1150,10 +1115,11 @@ static int ov772x_set_params(struct ov772x_priv *priv,
 	if (priv->band_filter_ctrl->val) {
 		unsigned short band_filter = priv->band_filter_ctrl->val;
 
-		ret = ov772x_mask_set(client, COM8, BNDF_ON_OFF, BNDF_ON_OFF);
+		ret = regmap_update_bits(priv->regmap, COM8,
+					 BNDF_ON_OFF, BNDF_ON_OFF);
 		if (!ret)
-			ret = ov772x_mask_set(client, BDBASE,
-					      0xff, 256 - band_filter);
+			ret = regmap_update_bits(priv->regmap, BDBASE,
+						 0xff, 256 - band_filter);
 		if (ret < 0)
 			goto ov772x_set_fmt_error;
 	}
@@ -1162,7 +1128,7 @@ static int ov772x_set_params(struct ov772x_priv *priv,
 
 ov772x_set_fmt_error:
 
-	ov772x_reset(client);
+	ov772x_reset(priv);
 
 	return ret;
 }
@@ -1276,12 +1242,12 @@ static int ov772x_video_probe(struct ov772x_priv *priv)
 		return ret;
 
 	/* Check and show product ID and manufacturer ID. */
-	pid = ov772x_read(client, PID);
-	if (pid < 0)
-		return pid;
-	ver = ov772x_read(client, VER);
-	if (ver < 0)
-		return ver;
+	ret = regmap_read(priv->regmap, PID, &pid);
+	if (ret < 0)
+		return ret;
+	ret = regmap_read(priv->regmap, VER, &ver);
+	if (ret < 0)
+		return ret;
 
 	switch (VERSION(pid, ver)) {
 	case OV7720:
@@ -1297,12 +1263,12 @@ static int ov772x_video_probe(struct ov772x_priv *priv)
 		goto done;
 	}
 
-	midh = ov772x_read(client, MIDH);
-	if (midh < 0)
-		return midh;
-	midl = ov772x_read(client, MIDL);
-	if (midl < 0)
-		return midl;
+	ret = regmap_read(priv->regmap, MIDH, &midh);
+	if (ret < 0)
+		return ret;
+	ret = regmap_read(priv->regmap, MIDL, &midl);
+	if (ret < 0)
+		return ret;
 
 	dev_info(&client->dev,
 		 "%s Product ID %0x:%0x Manufacturer ID %x:%x\n",
@@ -1386,8 +1352,12 @@ static int ov772x_probe(struct i2c_client *client,
 			const struct i2c_device_id *did)
 {
 	struct ov772x_priv	*priv;
-	struct i2c_adapter	*adapter = client->adapter;
 	int			ret;
+	static const struct regmap_config ov772x_regmap_config = {
+		.reg_bits = 8,
+		.val_bits = 8,
+		.max_register = DSPAUTO,
+	};
 
 	if (!client->dev.of_node && !client->dev.platform_data) {
 		dev_err(&client->dev,
@@ -1395,16 +1365,16 @@ static int ov772x_probe(struct i2c_client *client,
 		return -EINVAL;
 	}
 
-	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
-		dev_err(&adapter->dev,
-			"I2C-Adapter doesn't support SMBUS_BYTE_DATA\n");
-		return -EIO;
-	}
-
 	priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
+	priv->regmap = devm_regmap_init_sccb(client, &ov772x_regmap_config);
+	if (IS_ERR(priv->regmap)) {
+		dev_err(&client->dev, "Failed to allocate register map\n");
+		return PTR_ERR(priv->regmap);
+	}
+
 	priv->info = client->dev.platform_data;
 	mutex_init(&priv->lock);
 
-- 
2.7.4

[PATCH -next v4 3/3] media: ov9650: use SCCB regmap

[Akinobu Mita]

Convert ov965x register access to use SCCB regmap.

Cc: Mark Brown <broonie@kernel.org>
Cc: Peter Rosin <peda@axentia.se>
Cc: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
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>
---
 drivers/media/i2c/Kconfig  |   1 +
 drivers/media/i2c/ov9650.c | 157 ++++++++++++++++++++++-----------------------
 2 files changed, 76 insertions(+), 82 deletions(-)

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index b923a51..7028824 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -755,6 +755,7 @@ config VIDEO_OV7740
 config VIDEO_OV9650
 	tristate "OmniVision OV9650/OV9652 sensor support"
 	depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
+	select REGMAP_SCCB
 	---help---
 	  This is a V4L2 sensor-level driver for the Omnivision
 	  OV9650 and OV9652 camera sensors.
diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c
index 5bea31c..16f625f 100644
--- a/drivers/media/i2c/ov9650.c
+++ b/drivers/media/i2c/ov9650.c
@@ -20,6 +20,7 @@
 #include <linux/media.h>
 #include <linux/module.h>
 #include <linux/ratelimit.h>
+#include <linux/regmap.h>
 #include <linux/slab.h>
 #include <linux/string.h>
 #include <linux/videodev2.h>
@@ -259,7 +260,7 @@ struct ov965x {
 	/* Protects the struct fields below */
 	struct mutex lock;
 
-	struct i2c_client *client;
+	struct regmap *regmap;
 
 	/* Exposure row interval in us */
 	unsigned int exp_row_interval;
@@ -424,51 +425,40 @@ static inline struct ov965x *to_ov965x(struct v4l2_subdev *sd)
 	return container_of(sd, struct ov965x, sd);
 }
 
-static int ov965x_read(struct i2c_client *client, u8 addr, u8 *val)
+static int ov965x_read(struct ov965x *ov965x, u8 addr, u8 *val)
 {
-	u8 buf = addr;
-	struct i2c_msg msg = {
-		.addr = client->addr,
-		.flags = 0,
-		.len = 1,
-		.buf = &buf
-	};
 	int ret;
+	unsigned int buf;
 
-	ret = i2c_transfer(client->adapter, &msg, 1);
-	if (ret == 1) {
-		msg.flags = I2C_M_RD;
-		ret = i2c_transfer(client->adapter, &msg, 1);
-
-		if (ret == 1)
-			*val = buf;
-	}
+	ret = regmap_read(ov965x->regmap, addr, &buf);
+	if (!ret)
+		*val = buf;
 
-	v4l2_dbg(2, debug, client, "%s: 0x%02x @ 0x%02x. (%d)\n",
+	v4l2_dbg(2, debug, &ov965x->sd, "%s: 0x%02x @ 0x%02x. (%d)\n",
 		 __func__, *val, addr, ret);
 
-	return ret == 1 ? 0 : ret;
+	return ret;
 }
 
-static int ov965x_write(struct i2c_client *client, u8 addr, u8 val)
+static int ov965x_write(struct ov965x *ov965x, u8 addr, u8 val)
 {
-	u8 buf[2] = { addr, val };
+	int ret;
 
-	int ret = i2c_master_send(client, buf, 2);
+	ret = regmap_write(ov965x->regmap, addr, val);
 
-	v4l2_dbg(2, debug, client, "%s: 0x%02x @ 0x%02X (%d)\n",
+	v4l2_dbg(2, debug, &ov965x->sd, "%s: 0x%02x @ 0x%02X (%d)\n",
 		 __func__, val, addr, ret);
 
-	return ret == 2 ? 0 : ret;
+	return ret;
 }
 
-static int ov965x_write_array(struct i2c_client *client,
+static int ov965x_write_array(struct ov965x *ov965x,
 			      const struct i2c_rv *regs)
 {
 	int i, ret = 0;
 
 	for (i = 0; ret == 0 && regs[i].addr != REG_NULL; i++)
-		ret = ov965x_write(client, regs[i].addr, regs[i].value);
+		ret = ov965x_write(ov965x, regs[i].addr, regs[i].value);
 
 	return ret;
 }
@@ -486,7 +476,7 @@ static int ov965x_set_default_gamma_curve(struct ov965x *ov965x)
 	unsigned int i;
 
 	for (i = 0; i < ARRAY_SIZE(gamma_curve); i++) {
-		int ret = ov965x_write(ov965x->client, addr, gamma_curve[i]);
+		int ret = ov965x_write(ov965x, addr, gamma_curve[i]);
 
 		if (ret < 0)
 			return ret;
@@ -506,7 +496,7 @@ static int ov965x_set_color_matrix(struct ov965x *ov965x)
 	unsigned int i;
 
 	for (i = 0; i < ARRAY_SIZE(mtx); i++) {
-		int ret = ov965x_write(ov965x->client, addr, mtx[i]);
+		int ret = ov965x_write(ov965x, addr, mtx[i]);
 
 		if (ret < 0)
 			return ret;
@@ -542,16 +532,15 @@ static int __ov965x_set_power(struct ov965x *ov965x, int on)
 static int ov965x_s_power(struct v4l2_subdev *sd, int on)
 {
 	struct ov965x *ov965x = to_ov965x(sd);
-	struct i2c_client *client = ov965x->client;
 	int ret = 0;
 
-	v4l2_dbg(1, debug, client, "%s: on: %d\n", __func__, on);
+	v4l2_dbg(1, debug, sd, "%s: on: %d\n", __func__, on);
 
 	mutex_lock(&ov965x->lock);
 	if (ov965x->power == !on) {
 		ret = __ov965x_set_power(ov965x, on);
 		if (!ret && on) {
-			ret = ov965x_write_array(client,
+			ret = ov965x_write_array(ov965x,
 						 ov965x_init_regs);
 			ov965x->apply_frame_fmt = 1;
 			ov965x->ctrls.update = 1;
@@ -609,13 +598,13 @@ static int ov965x_set_banding_filter(struct ov965x *ov965x, int value)
 	int ret;
 	u8 reg;
 
-	ret = ov965x_read(ov965x->client, REG_COM8, &reg);
+	ret = ov965x_read(ov965x, REG_COM8, &reg);
 	if (!ret) {
 		if (value == V4L2_CID_POWER_LINE_FREQUENCY_DISABLED)
 			reg &= ~COM8_BFILT;
 		else
 			reg |= COM8_BFILT;
-		ret = ov965x_write(ov965x->client, REG_COM8, reg);
+		ret = ov965x_write(ov965x, REG_COM8, reg);
 	}
 	if (value == V4L2_CID_POWER_LINE_FREQUENCY_DISABLED)
 		return 0;
@@ -631,7 +620,7 @@ static int ov965x_set_banding_filter(struct ov965x *ov965x, int value)
 	       ov965x->fiv->interval.numerator;
 	mbd = ((mbd / (light_freq * 2)) + 500) / 1000UL;
 
-	return ov965x_write(ov965x->client, REG_MBD, mbd);
+	return ov965x_write(ov965x, REG_MBD, mbd);
 }
 
 static int ov965x_set_white_balance(struct ov965x *ov965x, int awb)
@@ -639,17 +628,17 @@ static int ov965x_set_white_balance(struct ov965x *ov965x, int awb)
 	int ret;
 	u8 reg;
 
-	ret = ov965x_read(ov965x->client, REG_COM8, &reg);
+	ret = ov965x_read(ov965x, REG_COM8, &reg);
 	if (!ret) {
 		reg = awb ? reg | REG_COM8 : reg & ~REG_COM8;
-		ret = ov965x_write(ov965x->client, REG_COM8, reg);
+		ret = ov965x_write(ov965x, REG_COM8, reg);
 	}
 	if (!ret && !awb) {
-		ret = ov965x_write(ov965x->client, REG_BLUE,
+		ret = ov965x_write(ov965x, REG_BLUE,
 				   ov965x->ctrls.blue_balance->val);
 		if (ret < 0)
 			return ret;
-		ret = ov965x_write(ov965x->client, REG_RED,
+		ret = ov965x_write(ov965x, REG_RED,
 				   ov965x->ctrls.red_balance->val);
 	}
 	return ret;
@@ -677,14 +666,13 @@ static int ov965x_set_brightness(struct ov965x *ov965x, int val)
 		return -EINVAL;
 
 	for (i = 0; i < NUM_BR_REGS && !ret; i++)
-		ret = ov965x_write(ov965x->client, regs[0][i],
+		ret = ov965x_write(ov965x, regs[0][i],
 				   regs[val][i]);
 	return ret;
 }
 
 static int ov965x_set_gain(struct ov965x *ov965x, int auto_gain)
 {
-	struct i2c_client *client = ov965x->client;
 	struct ov965x_ctrls *ctrls = &ov965x->ctrls;
 	int ret = 0;
 	u8 reg;
@@ -693,14 +681,14 @@ static int ov965x_set_gain(struct ov965x *ov965x, int auto_gain)
 	 * gain value in REG_VREF, REG_GAIN is not overwritten.
 	 */
 	if (ctrls->auto_gain->is_new) {
-		ret = ov965x_read(client, REG_COM8, &reg);
+		ret = ov965x_read(ov965x, REG_COM8, &reg);
 		if (ret < 0)
 			return ret;
 		if (ctrls->auto_gain->val)
 			reg |= COM8_AGC;
 		else
 			reg &= ~COM8_AGC;
-		ret = ov965x_write(client, REG_COM8, reg);
+		ret = ov965x_write(ov965x, REG_COM8, reg);
 		if (ret < 0)
 			return ret;
 	}
@@ -719,15 +707,15 @@ static int ov965x_set_gain(struct ov965x *ov965x, int auto_gain)
 		rgain = (gain - ((1 << m) * 16)) / (1 << m);
 		rgain |= (((1 << m) - 1) << 4);
 
-		ret = ov965x_write(client, REG_GAIN, rgain & 0xff);
+		ret = ov965x_write(ov965x, REG_GAIN, rgain & 0xff);
 		if (ret < 0)
 			return ret;
-		ret = ov965x_read(client, REG_VREF, &reg);
+		ret = ov965x_read(ov965x, REG_VREF, &reg);
 		if (ret < 0)
 			return ret;
 		reg &= ~VREF_GAIN_MASK;
 		reg |= (((rgain >> 8) & 0x3) << 6);
-		ret = ov965x_write(client, REG_VREF, reg);
+		ret = ov965x_write(ov965x, REG_VREF, reg);
 		if (ret < 0)
 			return ret;
 		/* Return updated control's value to userspace */
@@ -742,10 +730,10 @@ static int ov965x_set_sharpness(struct ov965x *ov965x, unsigned int value)
 	u8 com14, edge;
 	int ret;
 
-	ret = ov965x_read(ov965x->client, REG_COM14, &com14);
+	ret = ov965x_read(ov965x, REG_COM14, &com14);
 	if (ret < 0)
 		return ret;
-	ret = ov965x_read(ov965x->client, REG_EDGE, &edge);
+	ret = ov965x_read(ov965x, REG_EDGE, &edge);
 	if (ret < 0)
 		return ret;
 	com14 = value ? com14 | COM14_EDGE_EN : com14 & ~COM14_EDGE_EN;
@@ -756,33 +744,32 @@ static int ov965x_set_sharpness(struct ov965x *ov965x, unsigned int value)
 	} else {
 		com14 &= ~COM14_EEF_X2;
 	}
-	ret = ov965x_write(ov965x->client, REG_COM14, com14);
+	ret = ov965x_write(ov965x, REG_COM14, com14);
 	if (ret < 0)
 		return ret;
 
 	edge &= ~EDGE_FACTOR_MASK;
 	edge |= ((u8)value & 0x0f);
 
-	return ov965x_write(ov965x->client, REG_EDGE, edge);
+	return ov965x_write(ov965x, REG_EDGE, edge);
 }
 
 static int ov965x_set_exposure(struct ov965x *ov965x, int exp)
 {
-	struct i2c_client *client = ov965x->client;
 	struct ov965x_ctrls *ctrls = &ov965x->ctrls;
 	bool auto_exposure = (exp == V4L2_EXPOSURE_AUTO);
 	int ret;
 	u8 reg;
 
 	if (ctrls->auto_exp->is_new) {
-		ret = ov965x_read(client, REG_COM8, &reg);
+		ret = ov965x_read(ov965x, REG_COM8, &reg);
 		if (ret < 0)
 			return ret;
 		if (auto_exposure)
 			reg |= (COM8_AEC | COM8_AGC);
 		else
 			reg &= ~(COM8_AEC | COM8_AGC);
-		ret = ov965x_write(client, REG_COM8, reg);
+		ret = ov965x_write(ov965x, REG_COM8, reg);
 		if (ret < 0)
 			return ret;
 	}
@@ -794,12 +781,12 @@ static int ov965x_set_exposure(struct ov965x *ov965x, int exp)
 		 * Manual exposure value
 		 * [b15:b0] - AECHM (b15:b10), AECH (b9:b2), COM1 (b1:b0)
 		 */
-		ret = ov965x_write(client, REG_COM1, exposure & 0x3);
+		ret = ov965x_write(ov965x, REG_COM1, exposure & 0x3);
 		if (!ret)
-			ret = ov965x_write(client, REG_AECH,
+			ret = ov965x_write(ov965x, REG_AECH,
 					   (exposure >> 2) & 0xff);
 		if (!ret)
-			ret = ov965x_write(client, REG_AECHM,
+			ret = ov965x_write(ov965x, REG_AECHM,
 					   (exposure >> 10) & 0x3f);
 		/* Update the value to minimize rounding errors */
 		ctrls->exposure->val = ((exposure * ov965x->exp_row_interval)
@@ -822,7 +809,7 @@ static int ov965x_set_flip(struct ov965x *ov965x)
 	if (ov965x->ctrls.vflip->val)
 		mvfp |= MVFP_FLIP;
 
-	return ov965x_write(ov965x->client, REG_MVFP, mvfp);
+	return ov965x_write(ov965x, REG_MVFP, mvfp);
 }
 
 #define NUM_SAT_LEVELS	5
@@ -846,7 +833,7 @@ static int ov965x_set_saturation(struct ov965x *ov965x, int val)
 		return -EINVAL;
 
 	for (i = 0; i < NUM_SAT_REGS && !ret; i++)
-		ret = ov965x_write(ov965x->client, addr + i, regs[val][i]);
+		ret = ov965x_write(ov965x, addr + i, regs[val][i]);
 
 	return ret;
 }
@@ -856,16 +843,15 @@ static int ov965x_set_test_pattern(struct ov965x *ov965x, int value)
 	int ret;
 	u8 reg;
 
-	ret = ov965x_read(ov965x->client, REG_COM23, &reg);
+	ret = ov965x_read(ov965x, REG_COM23, &reg);
 	if (ret < 0)
 		return ret;
 	reg = value ? reg | COM23_TEST_MODE : reg & ~COM23_TEST_MODE;
-	return ov965x_write(ov965x->client, REG_COM23, reg);
+	return ov965x_write(ov965x, REG_COM23, reg);
 }
 
 static int __g_volatile_ctrl(struct ov965x *ov965x, struct v4l2_ctrl *ctrl)
 {
-	struct i2c_client *client = ov965x->client;
 	unsigned int exposure, gain, m;
 	u8 reg0, reg1, reg2;
 	int ret;
@@ -877,10 +863,10 @@ static int __g_volatile_ctrl(struct ov965x *ov965x, struct v4l2_ctrl *ctrl)
 	case V4L2_CID_AUTOGAIN:
 		if (!ctrl->val)
 			return 0;
-		ret = ov965x_read(client, REG_GAIN, &reg0);
+		ret = ov965x_read(ov965x, REG_GAIN, &reg0);
 		if (ret < 0)
 			return ret;
-		ret = ov965x_read(client, REG_VREF, &reg1);
+		ret = ov965x_read(ov965x, REG_VREF, &reg1);
 		if (ret < 0)
 			return ret;
 		gain = ((reg1 >> 6) << 8) | reg0;
@@ -891,13 +877,13 @@ static int __g_volatile_ctrl(struct ov965x *ov965x, struct v4l2_ctrl *ctrl)
 	case V4L2_CID_EXPOSURE_AUTO:
 		if (ctrl->val == V4L2_EXPOSURE_MANUAL)
 			return 0;
-		ret = ov965x_read(client, REG_COM1, &reg0);
+		ret = ov965x_read(ov965x, REG_COM1, &reg0);
 		if (ret < 0)
 			return ret;
-		ret = ov965x_read(client, REG_AECH, &reg1);
+		ret = ov965x_read(ov965x, REG_AECH, &reg1);
 		if (ret < 0)
 			return ret;
-		ret = ov965x_read(client, REG_AECHM, &reg2);
+		ret = ov965x_read(ov965x, REG_AECHM, &reg2);
 		if (ret < 0)
 			return ret;
 		exposure = ((reg2 & 0x3f) << 10) | (reg1 << 2) |
@@ -1279,32 +1265,31 @@ static int ov965x_set_frame_size(struct ov965x *ov965x)
 	int i, ret = 0;
 
 	for (i = 0; ret == 0 && i < NUM_FMT_REGS; i++)
-		ret = ov965x_write(ov965x->client, frame_size_reg_addr[i],
+		ret = ov965x_write(ov965x, frame_size_reg_addr[i],
 				   ov965x->frame_size->regs[i]);
 	return ret;
 }
 
 static int __ov965x_set_params(struct ov965x *ov965x)
 {
-	struct i2c_client *client = ov965x->client;
 	struct ov965x_ctrls *ctrls = &ov965x->ctrls;
 	int ret = 0;
 	u8 reg;
 
 	if (ov965x->apply_frame_fmt) {
 		reg = DEF_CLKRC + ov965x->fiv->clkrc_div;
-		ret = ov965x_write(client, REG_CLKRC, reg);
+		ret = ov965x_write(ov965x, REG_CLKRC, reg);
 		if (ret < 0)
 			return ret;
 		ret = ov965x_set_frame_size(ov965x);
 		if (ret < 0)
 			return ret;
-		ret = ov965x_read(client, REG_TSLB, &reg);
+		ret = ov965x_read(ov965x, REG_TSLB, &reg);
 		if (ret < 0)
 			return ret;
 		reg &= ~TSLB_YUYV_MASK;
 		reg |= ov965x->tslb_reg;
-		ret = ov965x_write(client, REG_TSLB, reg);
+		ret = ov965x_write(ov965x, REG_TSLB, reg);
 		if (ret < 0)
 			return ret;
 	}
@@ -1318,10 +1303,10 @@ static int __ov965x_set_params(struct ov965x *ov965x)
 	 * Select manual banding filter, the filter will
 	 * be enabled further if required.
 	 */
-	ret = ov965x_read(client, REG_COM11, &reg);
+	ret = ov965x_read(ov965x, REG_COM11, &reg);
 	if (!ret)
 		reg |= COM11_BANDING;
-	ret = ov965x_write(client, REG_COM11, reg);
+	ret = ov965x_write(ov965x, REG_COM11, reg);
 	if (ret < 0)
 		return ret;
 	/*
@@ -1333,12 +1318,11 @@ static int __ov965x_set_params(struct ov965x *ov965x)
 
 static int ov965x_s_stream(struct v4l2_subdev *sd, int on)
 {
-	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	struct ov965x *ov965x = to_ov965x(sd);
 	struct ov965x_ctrls *ctrls = &ov965x->ctrls;
 	int ret = 0;
 
-	v4l2_dbg(1, debug, client, "%s: on: %d\n", __func__, on);
+	v4l2_dbg(1, debug, sd, "%s: on: %d\n", __func__, on);
 
 	mutex_lock(&ov965x->lock);
 	if (ov965x->streaming == !on) {
@@ -1358,7 +1342,7 @@ static int ov965x_s_stream(struct v4l2_subdev *sd, int on)
 				ctrls->update = 0;
 		}
 		if (!ret)
-			ret = ov965x_write(client, REG_COM2,
+			ret = ov965x_write(ov965x, REG_COM2,
 					   on ? 0x01 : 0x11);
 	}
 	if (!ret)
@@ -1421,6 +1405,7 @@ static int ov965x_configure_gpios_pdata(struct ov965x *ov965x,
 {
 	int ret, i;
 	int gpios[NUM_GPIOS];
+	struct device *dev = regmap_get_device(ov965x->regmap);
 
 	gpios[GPIO_PWDN] = pdata->gpio_pwdn;
 	gpios[GPIO_RST]  = pdata->gpio_reset;
@@ -1430,7 +1415,7 @@ static int ov965x_configure_gpios_pdata(struct ov965x *ov965x,
 
 		if (!gpio_is_valid(gpio))
 			continue;
-		ret = devm_gpio_request_one(&ov965x->client->dev, gpio,
+		ret = devm_gpio_request_one(dev, gpio,
 					    GPIOF_OUT_INIT_HIGH, "OV965X");
 		if (ret < 0)
 			return ret;
@@ -1446,7 +1431,7 @@ static int ov965x_configure_gpios_pdata(struct ov965x *ov965x,
 
 static int ov965x_configure_gpios(struct ov965x *ov965x)
 {
-	struct device *dev = &ov965x->client->dev;
+	struct device *dev = regmap_get_device(ov965x->regmap);
 
 	ov965x->gpios[GPIO_PWDN] = devm_gpiod_get_optional(dev, "powerdown",
 							GPIOD_OUT_HIGH);
@@ -1467,7 +1452,6 @@ static int ov965x_configure_gpios(struct ov965x *ov965x)
 
 static int ov965x_detect_sensor(struct v4l2_subdev *sd)
 {
-	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	struct ov965x *ov965x = to_ov965x(sd);
 	u8 pid, ver;
 	int ret;
@@ -1480,9 +1464,9 @@ static int ov965x_detect_sensor(struct v4l2_subdev *sd)
 	msleep(25);
 
 	/* Check sensor revision */
-	ret = ov965x_read(client, REG_PID, &pid);
+	ret = ov965x_read(ov965x, REG_PID, &pid);
 	if (!ret)
-		ret = ov965x_read(client, REG_VER, &ver);
+		ret = ov965x_read(ov965x, REG_VER, &ver);
 
 	__ov965x_set_power(ov965x, 0);
 
@@ -1509,12 +1493,21 @@ static int ov965x_probe(struct i2c_client *client,
 	struct v4l2_subdev *sd;
 	struct ov965x *ov965x;
 	int ret;
+	static const struct regmap_config ov965x_regmap_config = {
+		.reg_bits = 8,
+		.val_bits = 8,
+		.max_register = 0xab,
+	};
 
 	ov965x = devm_kzalloc(&client->dev, sizeof(*ov965x), GFP_KERNEL);
 	if (!ov965x)
 		return -ENOMEM;
 
-	ov965x->client = client;
+	ov965x->regmap = devm_regmap_init_sccb(client, &ov965x_regmap_config);
+	if (IS_ERR(ov965x->regmap)) {
+		dev_err(&client->dev, "Failed to allocate register map\n");
+		return PTR_ERR(ov965x->regmap);
+	}
 
 	if (pdata) {
 		if (pdata->mclk_frequency == 0) {
@@ -1527,7 +1520,7 @@ static int ov965x_probe(struct i2c_client *client,
 		if (ret < 0)
 			return ret;
 	} else if (dev_fwnode(&client->dev)) {
-		ov965x->clk = devm_clk_get(&ov965x->client->dev, NULL);
+		ov965x->clk = devm_clk_get(&client->dev, NULL);
 		if (IS_ERR(ov965x->clk))
 			return PTR_ERR(ov965x->clk);
 		ov965x->mclk_frequency = clk_get_rate(ov965x->clk);
-- 
2.7.4

Re: [PATCH -next v4 1/3] regmap: add SCCB support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1125 bytes --]

On Tue, Jul 17, 2018 at 12:47:48AM +0900, Akinobu Mita wrote:
> This adds Serial Camera Control Bus (SCCB) support for regmap API that
> is intended to be used by some of Omnivision sensor drivers.

The following changes since commit ce397d215ccd07b8ae3f71db689aedb85d56ab40:

  Linux 4.18-rc1 (2018-06-17 08:04:49 +0900)

are available in the Git repository at:

  https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git tags/regmap-sccb

for you to fetch changes up to bcf7eac3d97f49d8400ba52c71bee5934bf20093:

  regmap: add SCCB support (2018-07-18 15:45:23 +0100)

----------------------------------------------------------------
regmap: Add support for SCCB

This is an I2C subset.

----------------------------------------------------------------
Akinobu Mita (1):
      regmap: add SCCB support

 drivers/base/regmap/Kconfig       |   4 ++
 drivers/base/regmap/Makefile      |   1 +
 drivers/base/regmap/regmap-sccb.c | 128 ++++++++++++++++++++++++++++++++++++++
 include/linux/regmap.h            |  35 +++++++++++
 4 files changed, 168 insertions(+)
 create mode 100644 drivers/base/regmap/regmap-sccb.c

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH -next v4 1/3] regmap: add SCCB support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2026 bytes --]

On Tue, Jul 17, 2018 at 12:47:48AM +0900, Akinobu Mita wrote:
> This adds Serial Camera Control Bus (SCCB) support for regmap API that
> is intended to be used by some of Omnivision sensor drivers.
> 
> The ov772x and ov9650 drivers are going to use this SCCB regmap API.
> 
> The ov772x driver 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 this SCCB regmap hides the implementation detail.
> 
> The ov9650 driver also issues two separated i2c messages to read the
> registers as the device doesn't support repeated start.  So it can
> make use of this SCCB regmap.

Cool, this series really gets better and better each time. Thank you for
keeping at it! And thanks to everyone for their suggestions. I really
like how we do not introduce a couple of i2c_sccb_* functions with a
need to export them. And given the nature of regmap, I'd think it should
be fairly easy to add support for ov2659 somewhen which has 16 bit
registers? Only minor nits:

> +#include <linux/regmap.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>

Sort these?

> +/**
> + * regmap_sccb_read - Read data from SCCB slave device
> + * @context: Device that will be interacted wit

"with"

> +	ret = __i2c_smbus_xfer(i2c->adapter, i2c->addr, i2c->flags,
> +			       I2C_SMBUS_WRITE, reg, I2C_SMBUS_BYTE, NULL);

Mark: __i2c_smbus_xfer is a dependency on i2c/for-next. Do you want an
immutable branch for that?

> +/**
> + * regmap_sccb_write - Write data to SCCB slave device
> + * @context: Device that will be interacted wit

"with"

But in general (especially for the I2C parts):

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH -next v4 1/3] regmap: add SCCB support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 601 bytes --]

On Wed, Jul 18, 2018 at 05:28:32PM +0200, Wolfram Sang wrote:
> On Tue, Jul 17, 2018 at 12:47:48AM +0900, Akinobu Mita wrote:

> > +	ret = __i2c_smbus_xfer(i2c->adapter, i2c->addr, i2c->flags,
> > +			       I2C_SMBUS_WRITE, reg, I2C_SMBUS_BYTE, NULL);

> Mark: __i2c_smbus_xfer is a dependency on i2c/for-next. Do you want an
> immutable branch for that?

Oh dear, that dependency wasn't mentioned anywhere I can see in the
submissions and I already applied this and created a signed tag :( .
I'll need a branch to pull in if I'm going to apply this (which I'd
prefer, it tends to make life easier).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH -next v4 2/3] media: ov772x: use SCCB regmap

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 785 bytes --]

On Tue, Jul 17, 2018 at 12:47:49AM +0900, Akinobu Mita wrote:
> Convert ov772x register access to use SCCB regmap.
> 
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Peter Rosin <peda@axentia.se>
> Cc: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
> 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>

From an I2C point of view, this makes a lot of sense:

Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH -next v4 3/3] media: ov9650: use SCCB regmap

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 757 bytes --]

On Tue, Jul 17, 2018 at 12:47:50AM +0900, Akinobu Mita wrote:
> Convert ov965x register access to use SCCB regmap.
> 
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Peter Rosin <peda@axentia.se>
> Cc: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
> 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>

Again, for the I2C parts:

Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH -next v4 2/3] media: ov772x: use SCCB regmap

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 15994 bytes --]

Hello Mita-San,
   thanks for keep pushing on this SCCB issue

Only one veryt minor nit, please see below..

On Tue, Jul 17, 2018 at 12:47:49AM +0900, Akinobu Mita wrote:
> Convert ov772x register access to use SCCB regmap.
>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Peter Rosin <peda@axentia.se>
> Cc: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
> 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>
> ---
>  drivers/media/i2c/Kconfig  |   1 +
>  drivers/media/i2c/ov772x.c | 192 +++++++++++++++++++--------------------------
>  2 files changed, 82 insertions(+), 111 deletions(-)
>
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 341452f..b923a51 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -715,6 +715,7 @@ config VIDEO_OV772X
>  	tristate "OmniVision OV772x sensor support"
>  	depends on I2C && VIDEO_V4L2
>  	depends on MEDIA_CAMERA_SUPPORT
> +	select REGMAP_SCCB
>  	---help---
>  	  This is a Video4Linux2 sensor-level driver for the OmniVision
>  	  OV772x camera.
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index 7158c31..0d3ed23 100644
> --- a/drivers/media/i2c/ov772x.c
> +++ b/drivers/media/i2c/ov772x.c
> @@ -21,6 +21,7 @@
>  #include <linux/init.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/regmap.h>
>  #include <linux/slab.h>
>  #include <linux/v4l2-mediabus.h>
>  #include <linux/videodev2.h>
> @@ -414,6 +415,7 @@ struct ov772x_priv {
>  	struct v4l2_subdev                subdev;
>  	struct v4l2_ctrl_handler	  hdl;
>  	struct clk			 *clk;
> +	struct regmap			 *regmap;
>  	struct ov772x_camera_info        *info;
>  	struct gpio_desc		 *pwdn_gpio;
>  	struct gpio_desc		 *rstb_gpio;
> @@ -549,51 +551,18 @@ static struct ov772x_priv *to_ov772x(struct v4l2_subdev *sd)
>  	return container_of(sd, struct ov772x_priv, subdev);
>  }
>
> -static int ov772x_read(struct i2c_client *client, u8 addr)
> -{
> -	int ret;
> -	u8 val;
> -
> -	ret = i2c_master_send(client, &addr, 1);
> -	if (ret < 0)
> -		return ret;
> -	ret = i2c_master_recv(client, &val, 1);
> -	if (ret < 0)
> -		return ret;
> -
> -	return val;
> -}
> -
> -static inline int ov772x_write(struct i2c_client *client, u8 addr, u8 value)
> -{
> -	return i2c_smbus_write_byte_data(client, addr, value);
> -}
> -
> -static int ov772x_mask_set(struct i2c_client *client, u8  command, u8  mask,
> -			   u8  set)
> -{
> -	s32 val = ov772x_read(client, command);
> -
> -	if (val < 0)
> -		return val;
> -
> -	val &= ~mask;
> -	val |= set & mask;
> -
> -	return ov772x_write(client, command, val);
> -}
> -

If I were you I would have kept these functions and wrapped the regmap
operations there. This is not an issue though if you prefer it this
way :)

> -static int ov772x_reset(struct i2c_client *client)
> +static int ov772x_reset(struct ov772x_priv *priv)
>  {
>  	int ret;
>
> -	ret = ov772x_write(client, COM7, SCCB_RESET);
> +	ret = regmap_write(priv->regmap, COM7, SCCB_RESET);
>  	if (ret < 0)
>  		return ret;
>
>  	usleep_range(1000, 5000);
>
> -	return ov772x_mask_set(client, COM2, SOFT_SLEEP_MODE, SOFT_SLEEP_MODE);
> +	return regmap_update_bits(priv->regmap, COM2, SOFT_SLEEP_MODE,
> +				  SOFT_SLEEP_MODE);
>  }
>
>  /*
> @@ -611,8 +580,8 @@ static int ov772x_s_stream(struct v4l2_subdev *sd, int enable)
>  	if (priv->streaming == enable)
>  		goto done;
>
> -	ret = ov772x_mask_set(client, COM2, SOFT_SLEEP_MODE,
> -			      enable ? 0 : SOFT_SLEEP_MODE);
> +	ret = regmap_update_bits(priv->regmap, COM2, SOFT_SLEEP_MODE,
> +				 enable ? 0 : SOFT_SLEEP_MODE);
>  	if (ret)
>  		goto done;
>
> @@ -657,7 +626,6 @@ static int ov772x_set_frame_rate(struct ov772x_priv *priv,
>  				 const struct ov772x_color_format *cfmt,
>  				 const struct ov772x_win_size *win)
>  {
> -	struct i2c_client *client = v4l2_get_subdevdata(&priv->subdev);
>  	unsigned long fin = clk_get_rate(priv->clk);
>  	unsigned int best_diff;
>  	unsigned int fsize;
> @@ -723,11 +691,11 @@ static int ov772x_set_frame_rate(struct ov772x_priv *priv,
>  		}
>  	}
>
> -	ret = ov772x_write(client, COM4, com4 | COM4_RESERVED);
> +	ret = regmap_write(priv->regmap, COM4, com4 | COM4_RESERVED);
>  	if (ret < 0)
>  		return ret;
>
> -	ret = ov772x_write(client, CLKRC, clkrc | CLKRC_RESERVED);
> +	ret = regmap_write(priv->regmap, CLKRC, clkrc | CLKRC_RESERVED);
>  	if (ret < 0)
>  		return ret;
>
> @@ -788,8 +756,7 @@ static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
>  {
>  	struct ov772x_priv *priv = container_of(ctrl->handler,
>  						struct ov772x_priv, hdl);
> -	struct v4l2_subdev *sd = &priv->subdev;
> -	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	struct regmap *regmap = priv->regmap;
>  	int ret = 0;
>  	u8 val;
>
> @@ -808,27 +775,27 @@ static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
>  		val = ctrl->val ? VFLIP_IMG : 0x00;
>  		if (priv->info && (priv->info->flags & OV772X_FLAG_VFLIP))
>  			val ^= VFLIP_IMG;
> -		return ov772x_mask_set(client, COM3, VFLIP_IMG, val);
> +		return regmap_update_bits(regmap, COM3, VFLIP_IMG, val);
>  	case V4L2_CID_HFLIP:
>  		val = ctrl->val ? HFLIP_IMG : 0x00;
>  		if (priv->info && (priv->info->flags & OV772X_FLAG_HFLIP))
>  			val ^= HFLIP_IMG;
> -		return ov772x_mask_set(client, COM3, HFLIP_IMG, val);
> +		return regmap_update_bits(regmap, COM3, HFLIP_IMG, val);
>  	case V4L2_CID_BAND_STOP_FILTER:
>  		if (!ctrl->val) {
>  			/* Switch the filter off, it is on now */
> -			ret = ov772x_mask_set(client, BDBASE, 0xff, 0xff);
> +			ret = regmap_update_bits(regmap, BDBASE, 0xff, 0xff);
>  			if (!ret)
> -				ret = ov772x_mask_set(client, COM8,
> -						      BNDF_ON_OFF, 0);
> +				ret = regmap_update_bits(regmap, COM8,
> +							 BNDF_ON_OFF, 0);
>  		} else {
>  			/* Switch the filter on, set AEC low limit */
>  			val = 256 - ctrl->val;
> -			ret = ov772x_mask_set(client, COM8,
> -					      BNDF_ON_OFF, BNDF_ON_OFF);
> +			ret = regmap_update_bits(regmap, COM8,
> +						 BNDF_ON_OFF, BNDF_ON_OFF);
>  			if (!ret)
> -				ret = ov772x_mask_set(client, BDBASE,
> -						      0xff, val);
> +				ret = regmap_update_bits(regmap, BDBASE,
> +							 0xff, val);
>  		}
>
>  		return ret;
> @@ -841,18 +808,19 @@ static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
>  static int ov772x_g_register(struct v4l2_subdev *sd,
>  			     struct v4l2_dbg_register *reg)
>  {
> -	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	struct ov772x_priv *priv = to_ov772x(sd);
>  	int ret;
> +	unsigned int val;

Nit: please keep variables sorted (move 'val' declaration one line
up).

Apart from that, for the ov772x driver:
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

>
>  	reg->size = 1;
>  	if (reg->reg > 0xff)
>  		return -EINVAL;
>
> -	ret = ov772x_read(client, reg->reg);
> +	ret = regmap_read(priv->regmap, reg->reg, &val);
>  	if (ret < 0)
>  		return ret;
>
> -	reg->val = (__u64)ret;
> +	reg->val = (__u64)val;
>
>  	return 0;
>  }
> @@ -860,13 +828,13 @@ static int ov772x_g_register(struct v4l2_subdev *sd,
>  static int ov772x_s_register(struct v4l2_subdev *sd,
>  			     const struct v4l2_dbg_register *reg)
>  {
> -	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	struct ov772x_priv *priv = to_ov772x(sd);
>
>  	if (reg->reg > 0xff ||
>  	    reg->val > 0xff)
>  		return -EINVAL;
>
> -	return ov772x_write(client, reg->reg, reg->val);
> +	return regmap_write(priv->regmap, reg->reg, reg->val);
>  }
>  #endif
>
> @@ -1004,7 +972,7 @@ static void ov772x_select_params(const struct v4l2_mbus_framefmt *mf,
>
>  static int ov772x_edgectrl(struct ov772x_priv *priv)
>  {
> -	struct i2c_client *client = v4l2_get_subdevdata(&priv->subdev);
> +	struct regmap *regmap = priv->regmap;
>  	int ret;
>
>  	if (!priv->info)
> @@ -1018,19 +986,19 @@ static int ov772x_edgectrl(struct ov772x_priv *priv)
>  		 * Remove it when manual mode.
>  		 */
>
> -		ret = ov772x_mask_set(client, DSPAUTO, EDGE_ACTRL, 0x00);
> +		ret = regmap_update_bits(regmap, DSPAUTO, EDGE_ACTRL, 0x00);
>  		if (ret < 0)
>  			return ret;
>
> -		ret = ov772x_mask_set(client,
> -				      EDGE_TRSHLD, OV772X_EDGE_THRESHOLD_MASK,
> -				      priv->info->edgectrl.threshold);
> +		ret = regmap_update_bits(regmap, EDGE_TRSHLD,
> +					 OV772X_EDGE_THRESHOLD_MASK,
> +					 priv->info->edgectrl.threshold);
>  		if (ret < 0)
>  			return ret;
>
> -		ret = ov772x_mask_set(client,
> -				      EDGE_STRNGT, OV772X_EDGE_STRENGTH_MASK,
> -				      priv->info->edgectrl.strength);
> +		ret = regmap_update_bits(regmap, EDGE_STRNGT,
> +					 OV772X_EDGE_STRENGTH_MASK,
> +					 priv->info->edgectrl.strength);
>  		if (ret < 0)
>  			return ret;
>
> @@ -1040,15 +1008,15 @@ static int ov772x_edgectrl(struct ov772x_priv *priv)
>  		 *
>  		 * Set upper and lower limit.
>  		 */
> -		ret = ov772x_mask_set(client,
> -				      EDGE_UPPER, OV772X_EDGE_UPPER_MASK,
> -				      priv->info->edgectrl.upper);
> +		ret = regmap_update_bits(regmap, EDGE_UPPER,
> +					 OV772X_EDGE_UPPER_MASK,
> +					 priv->info->edgectrl.upper);
>  		if (ret < 0)
>  			return ret;
>
> -		ret = ov772x_mask_set(client,
> -				      EDGE_LOWER, OV772X_EDGE_LOWER_MASK,
> -				      priv->info->edgectrl.lower);
> +		ret = regmap_update_bits(regmap, EDGE_LOWER,
> +					 OV772X_EDGE_LOWER_MASK,
> +					 priv->info->edgectrl.lower);
>  		if (ret < 0)
>  			return ret;
>  	}
> @@ -1060,12 +1028,11 @@ static int ov772x_set_params(struct ov772x_priv *priv,
>  			     const struct ov772x_color_format *cfmt,
>  			     const struct ov772x_win_size *win)
>  {
> -	struct i2c_client *client = v4l2_get_subdevdata(&priv->subdev);
>  	int ret;
>  	u8  val;
>
>  	/* Reset hardware. */
> -	ov772x_reset(client);
> +	ov772x_reset(priv);
>
>  	/* Edge Ctrl. */
>  	ret = ov772x_edgectrl(priv);
> @@ -1073,32 +1040,32 @@ static int ov772x_set_params(struct ov772x_priv *priv,
>  		return ret;
>
>  	/* Format and window size. */
> -	ret = ov772x_write(client, HSTART, win->rect.left >> 2);
> +	ret = regmap_write(priv->regmap, HSTART, win->rect.left >> 2);
>  	if (ret < 0)
>  		goto ov772x_set_fmt_error;
> -	ret = ov772x_write(client, HSIZE, win->rect.width >> 2);
> +	ret = regmap_write(priv->regmap, HSIZE, win->rect.width >> 2);
>  	if (ret < 0)
>  		goto ov772x_set_fmt_error;
> -	ret = ov772x_write(client, VSTART, win->rect.top >> 1);
> +	ret = regmap_write(priv->regmap, VSTART, win->rect.top >> 1);
>  	if (ret < 0)
>  		goto ov772x_set_fmt_error;
> -	ret = ov772x_write(client, VSIZE, win->rect.height >> 1);
> +	ret = regmap_write(priv->regmap, VSIZE, win->rect.height >> 1);
>  	if (ret < 0)
>  		goto ov772x_set_fmt_error;
> -	ret = ov772x_write(client, HOUTSIZE, win->rect.width >> 2);
> +	ret = regmap_write(priv->regmap, HOUTSIZE, win->rect.width >> 2);
>  	if (ret < 0)
>  		goto ov772x_set_fmt_error;
> -	ret = ov772x_write(client, VOUTSIZE, win->rect.height >> 1);
> +	ret = regmap_write(priv->regmap, VOUTSIZE, win->rect.height >> 1);
>  	if (ret < 0)
>  		goto ov772x_set_fmt_error;
> -	ret = ov772x_write(client, HREF,
> +	ret = regmap_write(priv->regmap, HREF,
>  			   ((win->rect.top & 1) << HREF_VSTART_SHIFT) |
>  			   ((win->rect.left & 3) << HREF_HSTART_SHIFT) |
>  			   ((win->rect.height & 1) << HREF_VSIZE_SHIFT) |
>  			   ((win->rect.width & 3) << HREF_HSIZE_SHIFT));
>  	if (ret < 0)
>  		goto ov772x_set_fmt_error;
> -	ret = ov772x_write(client, EXHCH,
> +	ret = regmap_write(priv->regmap, EXHCH,
>  			   ((win->rect.height & 1) << EXHCH_VSIZE_SHIFT) |
>  			   ((win->rect.width & 3) << EXHCH_HSIZE_SHIFT));
>  	if (ret < 0)
> @@ -1107,15 +1074,14 @@ static int ov772x_set_params(struct ov772x_priv *priv,
>  	/* Set DSP_CTRL3. */
>  	val = cfmt->dsp3;
>  	if (val) {
> -		ret = ov772x_mask_set(client,
> -				      DSP_CTRL3, UV_MASK, val);
> +		ret = regmap_update_bits(priv->regmap, DSP_CTRL3, UV_MASK, val);
>  		if (ret < 0)
>  			goto ov772x_set_fmt_error;
>  	}
>
>  	/* DSP_CTRL4: AEC reference point and DSP output format. */
>  	if (cfmt->dsp4) {
> -		ret = ov772x_write(client, DSP_CTRL4, cfmt->dsp4);
> +		ret = regmap_write(priv->regmap, DSP_CTRL4, cfmt->dsp4);
>  		if (ret < 0)
>  			goto ov772x_set_fmt_error;
>  	}
> @@ -1131,13 +1097,12 @@ static int ov772x_set_params(struct ov772x_priv *priv,
>  	if (priv->hflip_ctrl->val)
>  		val ^= HFLIP_IMG;
>
> -	ret = ov772x_mask_set(client,
> -			      COM3, SWAP_MASK | IMG_MASK, val);
> +	ret = regmap_update_bits(priv->regmap, COM3, SWAP_MASK | IMG_MASK, val);
>  	if (ret < 0)
>  		goto ov772x_set_fmt_error;
>
>  	/* COM7: Sensor resolution and output format control. */
> -	ret = ov772x_write(client, COM7, win->com7_bit | cfmt->com7);
> +	ret = regmap_write(priv->regmap, COM7, win->com7_bit | cfmt->com7);
>  	if (ret < 0)
>  		goto ov772x_set_fmt_error;
>
> @@ -1150,10 +1115,11 @@ static int ov772x_set_params(struct ov772x_priv *priv,
>  	if (priv->band_filter_ctrl->val) {
>  		unsigned short band_filter = priv->band_filter_ctrl->val;
>
> -		ret = ov772x_mask_set(client, COM8, BNDF_ON_OFF, BNDF_ON_OFF);
> +		ret = regmap_update_bits(priv->regmap, COM8,
> +					 BNDF_ON_OFF, BNDF_ON_OFF);
>  		if (!ret)
> -			ret = ov772x_mask_set(client, BDBASE,
> -					      0xff, 256 - band_filter);
> +			ret = regmap_update_bits(priv->regmap, BDBASE,
> +						 0xff, 256 - band_filter);
>  		if (ret < 0)
>  			goto ov772x_set_fmt_error;
>  	}
> @@ -1162,7 +1128,7 @@ static int ov772x_set_params(struct ov772x_priv *priv,
>
>  ov772x_set_fmt_error:
>
> -	ov772x_reset(client);
> +	ov772x_reset(priv);
>
>  	return ret;
>  }
> @@ -1276,12 +1242,12 @@ static int ov772x_video_probe(struct ov772x_priv *priv)
>  		return ret;
>
>  	/* Check and show product ID and manufacturer ID. */
> -	pid = ov772x_read(client, PID);
> -	if (pid < 0)
> -		return pid;
> -	ver = ov772x_read(client, VER);
> -	if (ver < 0)
> -		return ver;
> +	ret = regmap_read(priv->regmap, PID, &pid);
> +	if (ret < 0)
> +		return ret;
> +	ret = regmap_read(priv->regmap, VER, &ver);
> +	if (ret < 0)
> +		return ret;
>
>  	switch (VERSION(pid, ver)) {
>  	case OV7720:
> @@ -1297,12 +1263,12 @@ static int ov772x_video_probe(struct ov772x_priv *priv)
>  		goto done;
>  	}
>
> -	midh = ov772x_read(client, MIDH);
> -	if (midh < 0)
> -		return midh;
> -	midl = ov772x_read(client, MIDL);
> -	if (midl < 0)
> -		return midl;
> +	ret = regmap_read(priv->regmap, MIDH, &midh);
> +	if (ret < 0)
> +		return ret;
> +	ret = regmap_read(priv->regmap, MIDL, &midl);
> +	if (ret < 0)
> +		return ret;
>
>  	dev_info(&client->dev,
>  		 "%s Product ID %0x:%0x Manufacturer ID %x:%x\n",
> @@ -1386,8 +1352,12 @@ static int ov772x_probe(struct i2c_client *client,
>  			const struct i2c_device_id *did)
>  {
>  	struct ov772x_priv	*priv;
> -	struct i2c_adapter	*adapter = client->adapter;
>  	int			ret;
> +	static const struct regmap_config ov772x_regmap_config = {
> +		.reg_bits = 8,
> +		.val_bits = 8,
> +		.max_register = DSPAUTO,
> +	};
>
>  	if (!client->dev.of_node && !client->dev.platform_data) {
>  		dev_err(&client->dev,
> @@ -1395,16 +1365,16 @@ static int ov772x_probe(struct i2c_client *client,
>  		return -EINVAL;
>  	}
>
> -	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
> -		dev_err(&adapter->dev,
> -			"I2C-Adapter doesn't support SMBUS_BYTE_DATA\n");
> -		return -EIO;
> -	}
> -
>  	priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
>  		return -ENOMEM;
>
> +	priv->regmap = devm_regmap_init_sccb(client, &ov772x_regmap_config);
> +	if (IS_ERR(priv->regmap)) {
> +		dev_err(&client->dev, "Failed to allocate register map\n");
> +		return PTR_ERR(priv->regmap);
> +	}
> +
>  	priv->info = client->dev.platform_data;
>  	mutex_init(&priv->lock);
>
> --
> 2.7.4
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH -next v4 2/3] media: ov772x: use SCCB regmap

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 596 bytes --]


> > -static int ov772x_mask_set(struct i2c_client *client, u8  command, u8  mask,
> > -			   u8  set)
> > -{
> > -	s32 val = ov772x_read(client, command);
> > -
> > -	if (val < 0)
> > -		return val;
> > -
> > -	val &= ~mask;
> > -	val |= set & mask;
> > -
> > -	return ov772x_write(client, command, val);
> > -}
> > -
> 
> If I were you I would have kept these functions and wrapped the regmap
> operations there. This is not an issue though if you prefer it this
> way :)

I have suggested this way. It is not a show stopper issue, but I still
like this version better.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH -next v4 2/3] media: ov772x: use SCCB regmap

[Laurent Pinchart]

On Thursday, 19 July 2018 11:42:08 EEST Wolfram Sang wrote:
> > > -static int ov772x_mask_set(struct i2c_client *client, u8  command, u8 
> > > mask,
> > > -			   u8  set)
> > > -{
> > > -	s32 val = ov772x_read(client, command);
> > > -
> > > -	if (val < 0)
> > > -		return val;
> > > -
> > > -	val &= ~mask;
> > > -	val |= set & mask;
> > > -
> > > -	return ov772x_write(client, command, val);
> > > -}
> > > -
> > 
> > If I were you I would have kept these functions and wrapped the regmap
> > operations there. This is not an issue though if you prefer it this
> > way :)
> 
> I have suggested this way. It is not a show stopper issue, but I still
> like this version better.

Wrapping the regmap functions minimizes the diff and makes it easier to 
backport the driver.

-- 
Regards,

Laurent Pinchart

Re: [PATCH -next v4 1/3] regmap: add SCCB support

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 9612 bytes --]

Hi,

On Tue, Jul 17, 2018 at 12:47:48AM +0900, Akinobu Mita wrote:
> This adds Serial Camera Control Bus (SCCB) support for regmap API that
> is intended to be used by some of Omnivision sensor drivers.
> 
> The ov772x and ov9650 drivers are going to use this SCCB regmap API.
> 
> The ov772x driver 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 this SCCB regmap hides the implementation detail.
> 
> The ov9650 driver also issues two separated i2c messages to read the
> registers as the device doesn't support repeated start.  So it can
> make use of this SCCB regmap.
> 
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Peter Rosin <peda@axentia.se>
> Cc: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
> 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

>  drivers/base/regmap/Kconfig       |   4 ++
>  drivers/base/regmap/Makefile      |   1 +
>  drivers/base/regmap/regmap-sccb.c | 128 ++++++++++++++++++++++++++++++++++++++
>  include/linux/regmap.h            |  35 +++++++++++
>  4 files changed, 168 insertions(+)
>  create mode 100644 drivers/base/regmap/regmap-sccb.c
> 
> diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig
> index aff34c0..6ad5ef4 100644
> --- a/drivers/base/regmap/Kconfig
> +++ b/drivers/base/regmap/Kconfig
> @@ -45,3 +45,7 @@ config REGMAP_IRQ
>  config REGMAP_SOUNDWIRE
>  	tristate
>  	depends on SOUNDWIRE_BUS
> +
> +config REGMAP_SCCB
> +	tristate
> +	depends on I2C
> diff --git a/drivers/base/regmap/Makefile b/drivers/base/regmap/Makefile
> index 5ed0023..f5b4e88 100644
> --- a/drivers/base/regmap/Makefile
> +++ b/drivers/base/regmap/Makefile
> @@ -15,3 +15,4 @@ obj-$(CONFIG_REGMAP_MMIO) += regmap-mmio.o
>  obj-$(CONFIG_REGMAP_IRQ) += regmap-irq.o
>  obj-$(CONFIG_REGMAP_W1) += regmap-w1.o
>  obj-$(CONFIG_REGMAP_SOUNDWIRE) += regmap-sdw.o
> +obj-$(CONFIG_REGMAP_SCCB) += regmap-sccb.o
> diff --git a/drivers/base/regmap/regmap-sccb.c b/drivers/base/regmap/regmap-sccb.c
> new file mode 100644
> index 0000000..b6eb876
> --- /dev/null
> +++ b/drivers/base/regmap/regmap-sccb.c
> @@ -0,0 +1,128 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Register map access API - SCCB support
> +
> +#include <linux/regmap.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +
> +#include "internal.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 bool sccb_is_available(struct i2c_adapter *adap)
> +{
> +	u32 needed_funcs = I2C_FUNC_SMBUS_BYTE | I2C_FUNC_SMBUS_WRITE_BYTE_DATA;
> +
> +	/*
> +	 * If we ever want support for hardware doing SCCB natively, we will
> +	 * introduce a sccb_xfer() callback to struct i2c_algorithm and check
> +	 * for it here.
> +	 */
> +
> +	return (i2c_get_functionality(adap) & needed_funcs) == needed_funcs;
> +}
> +
> +/**
> + * regmap_sccb_read - Read data from SCCB slave device
> + * @context: Device that will be interacted wit
> + * @reg: Register to be read from
> + * @val: Pointer to store read value
> + *
> + * This executes the 2-phase write transmission cycle that is followed by a
> + * 2-phase read transmission cycle, returning negative errno else zero on
> + * success.
> + */
> +static int regmap_sccb_read(void *context, unsigned int reg, unsigned int *val)
> +{
> +	struct device *dev = context;
> +	struct i2c_client *i2c = to_i2c_client(dev);
> +	int ret;
> +	union i2c_smbus_data data;
> +
> +	i2c_lock_bus(i2c->adapter, I2C_LOCK_SEGMENT);
> +
> +	ret = __i2c_smbus_xfer(i2c->adapter, i2c->addr, i2c->flags,
> +			       I2C_SMBUS_WRITE, reg, I2C_SMBUS_BYTE, NULL);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = __i2c_smbus_xfer(i2c->adapter, i2c->addr, i2c->flags,
> +			       I2C_SMBUS_READ, 0, I2C_SMBUS_BYTE, &data);
> +	if (ret < 0)
> +		goto out;
> +
> +	*val = data.byte;
> +out:
> +	i2c_unlock_bus(i2c->adapter, I2C_LOCK_SEGMENT);
> +
> +	return ret;
> +}
> +
> +/**
> + * regmap_sccb_write - Write data to SCCB slave device
> + * @context: Device that will be interacted wit
> + * @reg: Register to write to
> + * @val: Value to be written
> + *
> + * This executes the SCCB 3-phase write transmission cycle, returning negative
> + * errno else zero on success.
> + */
> +static int regmap_sccb_write(void *context, unsigned int reg, unsigned int val)
> +{
> +	struct device *dev = context;
> +	struct i2c_client *i2c = to_i2c_client(dev);
> +
> +	return i2c_smbus_write_byte_data(i2c, reg, val);
> +}
> +
> +static struct regmap_bus regmap_sccb_bus = {
> +	.reg_write = regmap_sccb_write,
> +	.reg_read = regmap_sccb_read,
> +};
> +
> +static const struct regmap_bus *regmap_get_sccb_bus(struct i2c_client *i2c,
> +					const struct regmap_config *config)
> +{
> +	if (config->val_bits == 8 && config->reg_bits == 8 &&
> +			sccb_is_available(i2c->adapter))
> +		return &regmap_sccb_bus;
> +
> +	return ERR_PTR(-ENOTSUPP);
> +}
> +
> +struct regmap *__regmap_init_sccb(struct i2c_client *i2c,
> +				  const struct regmap_config *config,
> +				  struct lock_class_key *lock_key,
> +				  const char *lock_name)
> +{
> +	const struct regmap_bus *bus = regmap_get_sccb_bus(i2c, config);
> +
> +	if (IS_ERR(bus))
> +		return ERR_CAST(bus);
> +
> +	return __regmap_init(&i2c->dev, bus, &i2c->dev, config,
> +			     lock_key, lock_name);
> +}
> +EXPORT_SYMBOL_GPL(__regmap_init_sccb);
> +
> +struct regmap *__devm_regmap_init_sccb(struct i2c_client *i2c,
> +				       const struct regmap_config *config,
> +				       struct lock_class_key *lock_key,
> +				       const char *lock_name)
> +{
> +	const struct regmap_bus *bus = regmap_get_sccb_bus(i2c, config);
> +
> +	if (IS_ERR(bus))
> +		return ERR_CAST(bus);
> +
> +	return __devm_regmap_init(&i2c->dev, bus, &i2c->dev, config,
> +				  lock_key, lock_name);
> +}
> +EXPORT_SYMBOL_GPL(__devm_regmap_init_sccb);
> +
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/regmap.h b/include/linux/regmap.h
> index 4f38068..52bf358 100644
> --- a/include/linux/regmap.h
> +++ b/include/linux/regmap.h
> @@ -514,6 +514,10 @@ struct regmap *__regmap_init_i2c(struct i2c_client *i2c,
>  				 const struct regmap_config *config,
>  				 struct lock_class_key *lock_key,
>  				 const char *lock_name);
> +struct regmap *__regmap_init_sccb(struct i2c_client *i2c,
> +				  const struct regmap_config *config,
> +				  struct lock_class_key *lock_key,
> +				  const char *lock_name);
>  struct regmap *__regmap_init_slimbus(struct slim_device *slimbus,
>  				 const struct regmap_config *config,
>  				 struct lock_class_key *lock_key,
> @@ -558,6 +562,10 @@ struct regmap *__devm_regmap_init_i2c(struct i2c_client *i2c,
>  				      const struct regmap_config *config,
>  				      struct lock_class_key *lock_key,
>  				      const char *lock_name);
> +struct regmap *__devm_regmap_init_sccb(struct i2c_client *i2c,
> +				       const struct regmap_config *config,
> +				       struct lock_class_key *lock_key,
> +				       const char *lock_name);
>  struct regmap *__devm_regmap_init_spi(struct spi_device *dev,
>  				      const struct regmap_config *config,
>  				      struct lock_class_key *lock_key,
> @@ -646,6 +654,19 @@ int regmap_attach_dev(struct device *dev, struct regmap *map,
>  				i2c, config)
>  
>  /**
> + * regmap_init_sccb() - Initialise register map
> + *
> + * @i2c: Device that will be interacted with
> + * @config: Configuration for register map
> + *
> + * The return value will be an ERR_PTR() on error or a valid pointer to
> + * a struct regmap.
> + */
> +#define regmap_init_sccb(i2c, config)					\
> +	__regmap_lockdep_wrapper(__regmap_init_sccb, #config,		\
> +				i2c, config)
> +
> +/**
>   * regmap_init_slimbus() - Initialise register map
>   *
>   * @slimbus: Device that will be interacted with
> @@ -798,6 +819,20 @@ bool regmap_ac97_default_volatile(struct device *dev, unsigned int reg);
>  				i2c, config)
>  
>  /**
> + * devm_regmap_init_sccb() - Initialise managed register map
> + *
> + * @i2c: Device that will be interacted with
> + * @config: Configuration for register map
> + *
> + * The return value will be an ERR_PTR() on error or a valid pointer
> + * to a struct regmap.  The regmap will be automatically freed by the
> + * device management code.
> + */
> +#define devm_regmap_init_sccb(i2c, config)				\
> +	__regmap_lockdep_wrapper(__devm_regmap_init_sccb, #config,	\
> +				i2c, config)
> +
> +/**
>   * devm_regmap_init_spi() - Initialise register map
>   *
>   * @dev: Device that will be interacted with
> -- 
> 2.7.4
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH -next v4 2/3] media: ov772x: use SCCB regmap

[sakari.ailus]

On Thu, Jul 19, 2018 at 03:14:06PM +0300, Laurent Pinchart wrote:
> On Thursday, 19 July 2018 11:42:08 EEST Wolfram Sang wrote:
> > > > -static int ov772x_mask_set(struct i2c_client *client, u8  command, u8 
> > > > mask,
> > > > -			   u8  set)
> > > > -{
> > > > -	s32 val = ov772x_read(client, command);
> > > > -
> > > > -	if (val < 0)
> > > > -		return val;
> > > > -
> > > > -	val &= ~mask;
> > > > -	val |= set & mask;
> > > > -
> > > > -	return ov772x_write(client, command, val);
> > > > -}
> > > > -
> > > 
> > > If I were you I would have kept these functions and wrapped the regmap
> > > operations there. This is not an issue though if you prefer it this
> > > way :)
> > 
> > I have suggested this way. It is not a show stopper issue, but I still
> > like this version better.
> 
> Wrapping the regmap functions minimizes the diff and makes it easier to 
> backport the driver.

May be, but using the regmap functions directly makes the driver cleaner.
Most drivers have some kind of wrappers around the I²C framework (or
regmap) functions; this one is one of the few to get rid of them.

The two could be done in a separate patch, too, albeit I think the current
one seems fine as such.

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCH -next v4 2/3] media: ov772x: use SCCB regmap

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 1549 bytes --]

Hi all,

On Thu, Jul 19, 2018 at 04:10:20PM +0300, sakari.ailus@iki.fi wrote:
> On Thu, Jul 19, 2018 at 03:14:06PM +0300, Laurent Pinchart wrote:
> > On Thursday, 19 July 2018 11:42:08 EEST Wolfram Sang wrote:
> > > > > -static int ov772x_mask_set(struct i2c_client *client, u8  command, u8
> > > > > mask,
> > > > > -			   u8  set)
> > > > > -{
> > > > > -	s32 val = ov772x_read(client, command);
> > > > > -
> > > > > -	if (val < 0)
> > > > > -		return val;
> > > > > -
> > > > > -	val &= ~mask;
> > > > > -	val |= set & mask;
> > > > > -
> > > > > -	return ov772x_write(client, command, val);
> > > > > -}
> > > > > -
> > > >
> > > > If I were you I would have kept these functions and wrapped the regmap
> > > > operations there. This is not an issue though if you prefer it this
> > > > way :)
> > >
> > > I have suggested this way. It is not a show stopper issue, but I still
> > > like this version better.
> >
> > Wrapping the regmap functions minimizes the diff and makes it easier to
> > backport the driver.

This was my reasoning too, but I'm happy with the current
implementation. Thanks Akinobu for handling this!

>
> May be, but using the regmap functions directly makes the driver cleaner.
> Most drivers have some kind of wrappers around the I²C framework (or
> regmap) functions; this one is one of the few to get rid of them.
>
> The two could be done in a separate patch, too, albeit I think the current
> one seems fine as such.
>
> --
> Sakari Ailus
> e-mail: sakari.ailus@iki.fi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH -next v4 1/3] regmap: add SCCB support

[Akinobu Mita]

2018年7月19日(木) 0:28 Wolfram Sang <wsa@the-dreams.de>:
>
> On Tue, Jul 17, 2018 at 12:47:48AM +0900, Akinobu Mita wrote:
> > This adds Serial Camera Control Bus (SCCB) support for regmap API that
> > is intended to be used by some of Omnivision sensor drivers.
> >
> > The ov772x and ov9650 drivers are going to use this SCCB regmap API.
> >
> > The ov772x driver 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 this SCCB regmap hides the implementation detail.
> >
> > The ov9650 driver also issues two separated i2c messages to read the
> > registers as the device doesn't support repeated start.  So it can
> > make use of this SCCB regmap.
>
> Cool, this series really gets better and better each time. Thank you for
> keeping at it! And thanks to everyone for their suggestions. I really
> like how we do not introduce a couple of i2c_sccb_* functions with a
> need to export them. And given the nature of regmap, I'd think it should
> be fairly easy to add support for ov2659 somewhen which has 16 bit
> registers? Only minor nits:

I have an ov2659 sensor.  The ov2659 driver works with i2c adapter that
doesn't know I2C_FUNC_PROTOCOL_MANGLING, so it actually supports repeated
start.

But ov5645, ov5647, ov7251 drivers may want to use SCCB regmap API for
16-bit register.  Because they use i2c_master_send + i2c_master_recv
for reading register value.

> > +#include <linux/regmap.h>
> > +#include <linux/i2c.h>
> > +#include <linux/module.h>
>
> Sort these?
>
> > +/**
> > + * regmap_sccb_read - Read data from SCCB slave device
> > + * @context: Device that will be interacted wit
>
> "with"
>
> > +     ret = __i2c_smbus_xfer(i2c->adapter, i2c->addr, i2c->flags,
> > +                            I2C_SMBUS_WRITE, reg, I2C_SMBUS_BYTE, NULL);
>
> Mark: __i2c_smbus_xfer is a dependency on i2c/for-next. Do you want an
> immutable branch for that?
>
> > +/**
> > + * regmap_sccb_write - Write data to SCCB slave device
> > + * @context: Device that will be interacted wit
>
> "with"
>
> But in general (especially for the I2C parts):
>
> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Thank you for your reviewing.

Re: [PATCH -next v4 1/3] regmap: add SCCB support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 813 bytes --]

On Wed, Jul 18, 2018 at 04:31:40PM +0100, Mark Brown wrote:
> On Wed, Jul 18, 2018 at 05:28:32PM +0200, Wolfram Sang wrote:
> > On Tue, Jul 17, 2018 at 12:47:48AM +0900, Akinobu Mita wrote:
> 
> > > +	ret = __i2c_smbus_xfer(i2c->adapter, i2c->addr, i2c->flags,
> > > +			       I2C_SMBUS_WRITE, reg, I2C_SMBUS_BYTE, NULL);
> 
> > Mark: __i2c_smbus_xfer is a dependency on i2c/for-next. Do you want an
> > immutable branch for that?
> 
> Oh dear, that dependency wasn't mentioned anywhere I can see in the
> submissions and I already applied this and created a signed tag :( .
> I'll need a branch to pull in if I'm going to apply this (which I'd
> prefer, it tends to make life easier).

Here is it:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/smbus_xfer_unlock-immutable


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH -next v4 2/3] media: ov772x: use SCCB regmap

[Sakari Ailus]

Hi Mita-san,

On Tue, Jul 17, 2018 at 12:47:49AM +0900, Akinobu Mita wrote:
> Convert ov772x register access to use SCCB regmap.
> 
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Peter Rosin <peda@axentia.se>
> Cc: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
> 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>
> ---
>  drivers/media/i2c/Kconfig  |   1 +
>  drivers/media/i2c/ov772x.c | 192 +++++++++++++++++++--------------------------
>  2 files changed, 82 insertions(+), 111 deletions(-)

I'll pick the two patches up when the SCCB regmap support has reached media
tree master. Thanks.

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCH -next v4 1/3] regmap: add SCCB support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 196 bytes --]

On Fri, Jul 20, 2018 at 11:53:44PM +0200, Wolfram Sang wrote:

> Here is it:

> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/smbus_xfer_unlock-immutable

Thanks, pulled in now.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]