[V6, 0/2] media: i2c: Add support for DW9768 VCM driver

[V6, 0/2] media: i2c: Add support for DW9768 VCM driver

[Dongchun Zhu]

Hello,

This series adds DT bindings in YAML and V4L2 sub-device driver for DW9768
lens voice coil motor(VCM), which is a 10-bit DAC with 100mA output current
sink capability from Dongwoon.

The driver is designed for linear control of voice coil motor,
and controlled via IIC serial interface to set the desired focus.
It controls the position with 10-bit DAC data D[9:0] and seperates
two 8-bit regs to control the VCM position as belows.
DAC_MSB: D[9:8](ADDR: 0x03):
     +---+---+---+---+---+---+---+---+
     |---|---|---|---|---|---|D09|D08|
     +---+---+---+---+---+---+---+---+
DAC_LSB: D[7:0](ADDR: 0x04):
     +---+---+---+---+---+---+---+---+
     |D07|D06|D05|D04|D03|D02|D01|D00|
     +---+---+---+---+---+---+---+---+

This driver supports:
 - set DW9768 to standby mode once suspend and turn it back to active if resume
 - set the desired focus via V4L2_CID_FOCUS_ABSOLUTE ctrl

Previous versions of this patch-set can be found here:
v5: https://lore.kernel.org/linux-media/20200502161727.30463-1-dongchun.zhu@mediatek.com/
v4: https://lore.kernel.org/linux-media/20200330123634.363-1-dongchun.zhu@mediatek.com/
v3: https://lore.kernel.org/linux-media/20200228155958.20657-1-dongchun.zhu@mediatek.com/
v2: https://lore.kernel.org/linux-media/20190905072142.14606-1-dongchun.zhu@mediatek.com/
v1: https://lore.kernel.org/linux-media/20190708100641.2702-1-dongchun.zhu@mediatek.com/

Mainly changes of v6 are addressing comments from Rob, Sakari, Tomasz.
Compared to v5:
 - Add a second compatible string for the Giantec device
 - Document optional properties: "dongwoon,aac-mode", "dongwoon,aac-timing" and
   "dongwoon,clock-dividing-rate" for lens specific reg settings
 - Adjust Kconfig to match the current media tree master branch
 - Use container_of() directly to replace of defining macro function

Please help review.
Thanks.

Dongchun Zhu (2):
  media: dt-bindings: media: i2c: Document DW9768 bindings
  media: i2c: dw9768: Add DW9768 VCM driver

 .../bindings/media/i2c/dongwoon,dw9768.yaml        | 105 +++++
 MAINTAINERS                                        |   8 +
 drivers/media/i2c/Kconfig                          |  13 +
 drivers/media/i2c/Makefile                         |   1 +
 drivers/media/i2c/dw9768.c                         | 515 +++++++++++++++++++++
 5 files changed, 642 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
 create mode 100644 drivers/media/i2c/dw9768.c

-- 
2.9.2

[V6, 1/2] media: dt-bindings: media: i2c: Document DW9768 bindings

[Dongchun Zhu]

Add DeviceTree binding documentation for Dongwoon Anatech DW9768 voice
coil actuator.

Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../bindings/media/i2c/dongwoon,dw9768.yaml        | 105 +++++++++++++++++++++
 MAINTAINERS                                        |   7 ++
 2 files changed, 112 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml

diff --git a/Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml b/Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
new file mode 100644
index 0000000..b909e83
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
@@ -0,0 +1,105 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright (c) 2020 MediaTek Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/dongwoon,dw9768.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Dongwoon Anatech DW9768 Voice Coil Motor (VCM) Lens Device Tree Bindings
+
+maintainers:
+  - Dongchun Zhu <dongchun.zhu@mediatek.com>
+
+description: |-
+  The Dongwoon DW9768 is a single 10-bit digital-to-analog (DAC) converter
+  with 100 mA output current sink capability. VCM current is controlled with
+  a linear mode driver. The DAC is controlled via a 2-wire (I2C-compatible)
+  serial interface that operates at clock rates up to 1MHz. This chip
+  integrates Advanced Actuator Control (AAC) technology and is intended for
+  driving voice coil lenses in camera modules.
+
+properties:
+  compatible:
+    enum:
+      # for DW9768 VCM
+      - dongwoon,dw9768
+      # for GT9769 VCM
+      - giantec,gt9769
+
+  reg:
+    maxItems: 1
+
+  vin-supply:
+    description:
+      Definition of the regulator used as I2C I/O interface power supply.
+
+  vdd-supply:
+    description:
+      Definition of the regulator used as VCM chip power supply.
+
+  dongwoon,aac-mode:
+    description:
+      Indication of AAC mode select.
+    allOf:
+      - $ref: "/schemas/types.yaml#/definitions/uint32"
+      - enum:
+          - 0    #  Direct (default)
+          - 1    #  AAC2 (operation time# 0.48 x Tvib)
+          - 2    #  AAC3 (operation time# 0.70 x Tvib)
+          - 3    #  AAC4 (operation time# 0.75 x Tvib)
+          - 4    #  Reserved
+          - 5    #  AAC8 (operation time# 1.13 x Tvib)
+          - 6    #  Reserved
+          - 7    #  Reserved
+
+  dongwoon,aac-timing:
+    description:
+      Indication of AAC Timing count, unit of 0.1 milliseconds.
+      Valid values vary from 0 to 63 (default 32).
+    allOf:
+      - $ref: "/schemas/types.yaml#/definitions/uint32"
+
+  dongwoon,clock-dividing-rate:
+    description:
+      Indication of VCM internal clock dividing rate select, as one multiple
+      factor to calculate VCM ring periodic time Tvib.
+    allOf:
+      - $ref: "/schemas/types.yaml#/definitions/uint32"
+      - enum:
+          - 0    #  Dividing Rate -  2
+          - 1    #  Dividing Rate -  1 (default)
+          - 2    #  Dividing Rate -  1/2
+          - 3    #  Dividing Rate -  1/4
+          - 4    #  Dividing Rate -  8
+          - 5    #  Dividing Rate -  4
+          - 6    #  Dividing Rate -  Reserved
+          - 7    #  Dividing Rate -  Reserved
+
+required:
+  - compatible
+  - reg
+  - vin-supply
+  - vdd-supply
+
+additionalProperties: false
+
+examples:
+  - |
+
+    i2c {
+        clock-frequency = <400000>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        dw9768: camera-lens@c {
+            compatible = "dongwoon,dw9768";
+            reg = <0x0c>;
+
+            vin-supply = <&mt6358_vcamio_reg>;
+            vdd-supply = <&mt6358_vcama2_reg>;
+            dongwoon,aac-mode = <2>;
+            dongwoon,aac-timing = <57>;
+        };
+    };
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index e64e5db..8d72c41 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5151,6 +5151,13 @@ T:	git git://linuxtv.org/media_tree.git
 F:	Documentation/devicetree/bindings/media/i2c/dongwoon,dw9714.txt
 F:	drivers/media/i2c/dw9714.c
 
+DONGWOON DW9768 LENS VOICE COIL DRIVER
+M:	Dongchun Zhu <dongchun.zhu@mediatek.com>
+L:	linux-media@vger.kernel.org
+S:	Maintained
+T:	git git://linuxtv.org/media_tree.git
+F:	Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
+
 DONGWOON DW9807 LENS VOICE COIL DRIVER
 M:	Sakari Ailus <sakari.ailus@linux.intel.com>
 L:	linux-media@vger.kernel.org
-- 
2.9.2

[V6, 2/2] media: i2c: dw9768: Add DW9768 VCM driver

[Dongchun Zhu]

Add a V4L2 sub-device driver for DW9768 voice coil motor, providing
control to set the desired focus via IIC serial interface.

Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
---
 MAINTAINERS                |   1 +
 drivers/media/i2c/Kconfig  |  13 ++
 drivers/media/i2c/Makefile |   1 +
 drivers/media/i2c/dw9768.c | 515 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 530 insertions(+)
 create mode 100644 drivers/media/i2c/dw9768.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 8d72c41..c92dc99 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5157,6 +5157,7 @@ L:	linux-media@vger.kernel.org
 S:	Maintained
 T:	git git://linuxtv.org/media_tree.git
 F:	Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
+F:	drivers/media/i2c/dw9768.c
 
 DONGWOON DW9807 LENS VOICE COIL DRIVER
 M:	Sakari Ailus <sakari.ailus@linux.intel.com>
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 125d596..afdf994 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -1040,6 +1040,19 @@ config VIDEO_DW9714
 	  capability. This is designed for linear control of
 	  voice coil motors, controlled via I2C serial interface.
 
+config VIDEO_DW9768
+	tristate "DW9768 lens voice coil support"
+	depends on I2C && VIDEO_V4L2
+	depends on PM
+	select MEDIA_CONTROLLER
+	select VIDEO_V4L2_SUBDEV_API
+	select V4L2_FWNODE
+	help
+	  This is a driver for the DW9768 camera lens voice coil.
+	  DW9768 is a 10 bit DAC with 100mA output current sink
+	  capability. This is designed for linear control of
+	  voice coil motors, controlled via I2C serial interface.
+
 config VIDEO_DW9807_VCM
 	tristate "DW9807 lens voice coil support"
 	depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 77bf7d0..4057476 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_VIDEO_SAA6752HS) += saa6752hs.o
 obj-$(CONFIG_VIDEO_AD5820)  += ad5820.o
 obj-$(CONFIG_VIDEO_AK7375)  += ak7375.o
 obj-$(CONFIG_VIDEO_DW9714)  += dw9714.o
+obj-$(CONFIG_VIDEO_DW9768)  += dw9768.o
 obj-$(CONFIG_VIDEO_DW9807_VCM)  += dw9807-vcm.o
 obj-$(CONFIG_VIDEO_ADV7170) += adv7170.o
 obj-$(CONFIG_VIDEO_ADV7175) += adv7175.o
diff --git a/drivers/media/i2c/dw9768.c b/drivers/media/i2c/dw9768.c
new file mode 100644
index 0000000..a0cc9f3
--- /dev/null
+++ b/drivers/media/i2c/dw9768.c
@@ -0,0 +1,515 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2020 MediaTek Inc.
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
+#include <media/v4l2-async.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-fwnode.h>
+#include <media/v4l2-subdev.h>
+
+#define DW9768_NAME				"dw9768"
+#define DW9768_MAX_FOCUS_POS			(1024 - 1)
+/*
+ * This sets the minimum granularity for the focus positions.
+ * A value of 1 gives maximum accuracy for a desired focus position
+ */
+#define DW9768_FOCUS_STEPS			1
+
+/*
+ * Ring control and Power control register
+ * Bit[1] RING_EN
+ * 0: Direct mode
+ * 1: AAC mode (ringing control mode)
+ * Bit[0] PD
+ * 0: Normal operation mode
+ * 1: Power down mode
+ * DW9768 requires waiting time of Topr after PD reset takes place.
+ */
+#define DW9768_RING_PD_CONTROL_REG		0x02
+#define DW9768_PD_MODE_OFF			0x00
+#define DW9768_PD_MODE_EN			BIT(0)
+#define DW9768_AAC_MODE_EN			BIT(1)
+
+/*
+ * DW9768 separates two registers to control the VCM position.
+ * One for MSB value, another is LSB value.
+ * DAC_MSB: D[9:8] (ADD: 0x03)
+ * DAC_LSB: D[7:0] (ADD: 0x04)
+ * D[9:0] DAC data input: positive output current = D[9:0] / 1023 * 100[mA]
+ */
+#define DW9768_MSB_ADDR				0x03
+#define DW9768_LSB_ADDR				0x04
+#define DW9768_STATUS_ADDR			0x05
+
+/*
+ * AAC mode control & prescale register
+ * Bit[7:5] Namely AC[2:0], decide the VCM mode and operation time.
+ * 000 Direct(default)
+ * 001 AAC2 0.48xTvib
+ * 010 AAC3 0.70xTvib
+ * 011 AAC4 0.75xTvib
+ * 100 Reserved
+ * 101 AAC8 1.13xTvib
+ * 110 Reserved
+ * 111 Reserved
+ * Bit[2:0] Namely PRESC[2:0], set the internal clock dividing rate as follow.
+ * 000 2
+ * 001 1(default)
+ * 010 1/2
+ * 011 1/4
+ * 100 8
+ * 101 4
+ * 110 Reserved
+ * 111 Reserved
+ */
+#define DW9768_AAC_PRESC_REG			0x06
+#define DW9768_AAC_MODE_SEL_MASK		GENMASK(7, 5)
+#define DW9768_CLOCK_PRE_SCALE_SEL_MASK		GENMASK(2, 0)
+
+/*
+ * VCM period of vibration register
+ * Bit[5:0] Defined as VCM rising periodic time (Tvib) together with PRESC[2:0]
+ * Tvib = (6.3ms + AACT[5:0] * 0.1ms) * Dividing Rate
+ * Dividing Rate is the internal clock dividing rate that is defined at
+ * PRESCALE register (ADD: 0x06)
+ */
+#define DW9768_AAC_TIME_REG			0x07
+
+/*
+ * DW9768 requires waiting time (delay time) of t_OPR after power-up,
+ * or in the case of PD reset taking place.
+ */
+#define DW9768_T_OPR_US				1000
+
+/*
+ * This acts as the minimum granularity of lens movement.
+ * Keep this value power of 2, so the control steps can be
+ * uniformly adjusted for gradual lens movement, with desired
+ * number of control steps.
+ */
+#define DW9768_MOVE_STEPS			16
+
+/*
+ * DW9768_AAC_PRESC_REG & DW9768_AAC_TIME_REG determine VCM operation time.
+ * If DW9768_AAC_PRESC_REG is 0x41, and DW9768_AAC_TIME_REG is 0x39, VCM mode
+ * would be AAC3, Operation Time would be 0.70xTvib, that is 8.40ms.
+ */
+#define DW9768_MOVE_DELAY_US			8400
+#define DW9768_STABLE_TIME_US			20000
+
+static const char * const dw9768_supply_names[] = {
+	"vin",	/* I2C I/O interface power */
+	"vdd",	/* VCM power */
+};
+
+/* dw9768 device structure */
+struct dw9768 {
+	struct regulator_bulk_data supplies[ARRAY_SIZE(dw9768_supply_names)];
+	struct v4l2_ctrl_handler ctrls;
+	struct v4l2_ctrl	*focus;
+	struct v4l2_subdev	sd;
+
+	u32			aac_mode;
+	u32			aac_timing;
+	u32			clock_dividing_rate;
+	bool			aac_mode_control_enable;
+	bool			aact_cnt_select_enable;
+	bool			clock_dividing_rate_select_enable;
+};
+
+static inline struct dw9768 *sd_to_dw9768(struct v4l2_subdev *subdev)
+{
+	return container_of(subdev, struct dw9768, sd);
+}
+
+struct regval_list {
+	u8 reg_num;
+	u8 value;
+};
+
+static int dw9768_read_smbus(struct dw9768 *dw9768, unsigned char reg,
+			     unsigned char *val)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(client, reg);
+
+	if (ret < 0)
+		return ret;
+
+	*val = (unsigned char)ret;
+
+	return 0;
+}
+
+static int dw9768_mod_reg(struct dw9768 *dw9768, u8 reg, u8 mask, u8 val)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
+	u8 readval;
+	int ret;
+
+	ret = dw9768_read_smbus(dw9768, reg, &readval);
+	if (ret)
+		return ret;
+
+	val = (readval & ~mask) | (val & mask);
+
+	return i2c_smbus_write_byte_data(client, reg, val);
+}
+
+static int dw9768_set_dac(struct dw9768 *dw9768, u16 val)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
+
+	/* Write VCM position to registers */
+	return i2c_smbus_write_word_swapped(client, DW9768_MSB_ADDR, val);
+}
+
+static int dw9768_init(struct dw9768 *dw9768)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
+	int ret, val;
+
+	/* Reset DW9768_RING_PD_CONTROL_REG to default status 0x00 */
+	ret = i2c_smbus_write_byte_data(client, DW9768_RING_PD_CONTROL_REG,
+					DW9768_PD_MODE_OFF);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * DW9769 requires waiting delay time of t_OPR
+	 * after PD reset takes place.
+	 */
+	usleep_range(DW9768_T_OPR_US, DW9768_T_OPR_US + 100);
+
+	/* Set DW9768_RING_PD_CONTROL_REG to DW9768_AAC_MODE_EN(0x01) */
+	ret = i2c_smbus_write_byte_data(client, DW9768_RING_PD_CONTROL_REG,
+					DW9768_AAC_MODE_EN);
+	if (ret < 0)
+		return ret;
+
+	/* Set AAC mode according to DT property */
+	if (dw9768->aac_mode_control_enable) {
+		ret = dw9768_mod_reg(dw9768, DW9768_AAC_PRESC_REG,
+				     DW9768_AAC_MODE_SEL_MASK,
+				     dw9768->aac_mode << 5);
+		if (ret < 0)
+			return ret;
+	}
+
+	/* Set Clock Dividing Rate factor according to DT property */
+	if (dw9768->clock_dividing_rate_select_enable) {
+		ret = dw9768_mod_reg(dw9768, DW9768_AAC_PRESC_REG,
+				     DW9768_CLOCK_PRE_SCALE_SEL_MASK,
+				     dw9768->clock_dividing_rate);
+		if (ret < 0)
+			return ret;
+	}
+
+	/* Set AAC Timing according to DT property */
+	if (dw9768->aact_cnt_select_enable) {
+		ret = i2c_smbus_write_byte_data(client, DW9768_AAC_TIME_REG,
+						dw9768->aac_timing);
+		if (ret < 0)
+			return ret;
+	}
+
+	for (val = dw9768->focus->val % DW9768_MOVE_STEPS;
+	     val <= dw9768->focus->val;
+	     val += DW9768_MOVE_STEPS) {
+		ret = dw9768_set_dac(dw9768, val);
+		if (ret) {
+			dev_err(&client->dev, "%s I2C failure: %d",
+				__func__, ret);
+			return ret;
+		}
+		usleep_range(DW9768_MOVE_DELAY_US,
+			     DW9768_MOVE_DELAY_US + 1000);
+	}
+
+	return 0;
+}
+
+static int dw9768_release(struct dw9768 *dw9768)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
+	int ret, val;
+
+	val = round_down(dw9768->focus->val, DW9768_MOVE_STEPS);
+	for ( ; val >= 0; val -= DW9768_MOVE_STEPS) {
+		ret = dw9768_set_dac(dw9768, val);
+		if (ret) {
+			dev_err(&client->dev, "I2C write fail: %d", ret);
+			return ret;
+		}
+		usleep_range(DW9768_MOVE_DELAY_US, DW9768_MOVE_DELAY_US + 1000);
+	}
+
+	/*
+	 * Wait for the motor to stabilize after the last movement
+	 * to prevent the motor from shaking.
+	 */
+	usleep_range(DW9768_STABLE_TIME_US - DW9768_MOVE_DELAY_US,
+		     DW9768_STABLE_TIME_US - DW9768_MOVE_DELAY_US + 1000);
+
+	ret = i2c_smbus_write_byte_data(client, DW9768_RING_PD_CONTROL_REG,
+					DW9768_PD_MODE_EN);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * DW9769 requires waiting delay time of t_OPR
+	 * after PD reset takes place.
+	 */
+	usleep_range(DW9768_T_OPR_US, DW9768_T_OPR_US + 100);
+
+	return 0;
+}
+
+static int dw9768_runtime_suspend(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct dw9768 *dw9768 = sd_to_dw9768(sd);
+
+	dw9768_release(dw9768);
+	regulator_bulk_disable(ARRAY_SIZE(dw9768_supply_names),
+			       dw9768->supplies);
+
+	return 0;
+}
+
+static int dw9768_runtime_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct dw9768 *dw9768 = sd_to_dw9768(sd);
+	int ret;
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(dw9768_supply_names),
+				    dw9768->supplies);
+	if (ret < 0) {
+		dev_err(dev, "failed to enable regulators\n");
+		return ret;
+	}
+
+	/*
+	 * The datasheet refers to t_OPR that needs to be waited before sending
+	 * I2C commands after power-up.
+	 */
+	usleep_range(DW9768_T_OPR_US, DW9768_T_OPR_US + 100);
+
+	ret = dw9768_init(dw9768);
+	if (ret < 0)
+		goto disable_regulator;
+
+	return 0;
+
+disable_regulator:
+	regulator_bulk_disable(ARRAY_SIZE(dw9768_supply_names),
+			       dw9768->supplies);
+
+	return ret;
+}
+
+static int dw9768_set_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct dw9768 *dw9768 = container_of(ctrl->handler,
+					     struct dw9768, ctrls);
+
+	if (ctrl->id == V4L2_CID_FOCUS_ABSOLUTE)
+		return dw9768_set_dac(dw9768, ctrl->val);
+
+	return 0;
+}
+
+static const struct v4l2_ctrl_ops dw9768_ctrl_ops = {
+	.s_ctrl = dw9768_set_ctrl,
+};
+
+static int dw9768_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+	int ret;
+
+	ret = pm_runtime_get_sync(sd->dev);
+	if (ret < 0) {
+		pm_runtime_put_noidle(sd->dev);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int dw9768_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+	pm_runtime_put(sd->dev);
+
+	return 0;
+}
+
+static const struct v4l2_subdev_internal_ops dw9768_int_ops = {
+	.open = dw9768_open,
+	.close = dw9768_close,
+};
+
+static const struct v4l2_subdev_ops dw9768_ops = { };
+
+static int dw9768_init_controls(struct dw9768 *dw9768)
+{
+	struct v4l2_ctrl_handler *hdl = &dw9768->ctrls;
+	const struct v4l2_ctrl_ops *ops = &dw9768_ctrl_ops;
+
+	v4l2_ctrl_handler_init(hdl, 1);
+
+	dw9768->focus = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_FOCUS_ABSOLUTE, 0,
+					  DW9768_MAX_FOCUS_POS,
+					  DW9768_FOCUS_STEPS, 0);
+
+	if (hdl->error)
+		return hdl->error;
+
+	dw9768->sd.ctrl_handler = hdl;
+
+	return 0;
+}
+
+static int dw9768_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct dw9768 *dw9768;
+	unsigned int aac_mode_select;
+	unsigned int aac_timing_select;
+	unsigned int clock_dividing_rate_select;
+	unsigned int i;
+	int ret;
+
+	dw9768 = devm_kzalloc(dev, sizeof(*dw9768), GFP_KERNEL);
+	if (!dw9768)
+		return -ENOMEM;
+
+	v4l2_i2c_subdev_init(&dw9768->sd, client, &dw9768_ops);
+	dw9768->aac_mode_control_enable = false;
+	dw9768->aact_cnt_select_enable = false;
+	dw9768->clock_dividing_rate_select_enable = false;
+
+	/* Optional indication of AAC mode select */
+	ret = fwnode_property_read_u32(dev_fwnode(dev), "dongwoon,aac-mode",
+				       &aac_mode_select);
+
+	if (!ret) {
+		dw9768->aac_mode_control_enable = true;
+		dw9768->aac_mode = aac_mode_select;
+	}
+
+	/* Optional indication of VCM internal clock dividing rate select */
+	ret = fwnode_property_read_u32(dev_fwnode(dev),
+				       "dongwoon,clock-dividing-rate",
+				       &clock_dividing_rate_select);
+
+	if (!ret) {
+		dw9768->clock_dividing_rate_select_enable = true;
+		dw9768->clock_dividing_rate = clock_dividing_rate_select;
+	}
+
+	/* Optional indication of AAC Timing */
+	ret = fwnode_property_read_u32(dev_fwnode(dev), "dongwoon,aac-timing",
+				       &aac_timing_select);
+
+	if (!ret) {
+		dw9768->aact_cnt_select_enable = true;
+		dw9768->aac_timing = aac_timing_select;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(dw9768_supply_names); i++)
+		dw9768->supplies[i].supply = dw9768_supply_names[i];
+
+	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(dw9768_supply_names),
+				      dw9768->supplies);
+	if (ret) {
+		dev_err(dev, "failed to get regulators\n");
+		return ret;
+	}
+
+	ret = dw9768_init_controls(dw9768);
+	if (ret)
+		goto entity_cleanup;
+
+	dw9768->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	dw9768->sd.internal_ops = &dw9768_int_ops;
+
+	ret = media_entity_pads_init(&dw9768->sd.entity, 0, NULL);
+	if (ret < 0)
+		goto entity_cleanup;
+
+	dw9768->sd.entity.function = MEDIA_ENT_F_LENS;
+
+	pm_runtime_enable(dev);
+	if (!pm_runtime_enabled(dev)) {
+		ret = dw9768_runtime_resume(dev);
+		if (ret < 0) {
+			dev_err(dev, "failed to power on: %d\n", ret);
+			goto entity_cleanup;
+		}
+	}
+
+	ret = v4l2_async_register_subdev(&dw9768->sd);
+	if (ret < 0)
+		goto entity_cleanup;
+
+	return 0;
+
+entity_cleanup:
+	v4l2_ctrl_handler_free(&dw9768->ctrls);
+	media_entity_cleanup(&dw9768->sd.entity);
+	return ret;
+}
+
+static int dw9768_remove(struct i2c_client *client)
+{
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct dw9768 *dw9768 = sd_to_dw9768(sd);
+
+	pm_runtime_disable(&client->dev);
+	v4l2_async_unregister_subdev(&dw9768->sd);
+	v4l2_ctrl_handler_free(&dw9768->ctrls);
+	media_entity_cleanup(&dw9768->sd.entity);
+	if (!pm_runtime_status_suspended(&client->dev))
+		dw9768_runtime_suspend(&client->dev);
+	pm_runtime_set_suspended(&client->dev);
+
+	return 0;
+}
+
+static const struct of_device_id dw9768_of_table[] = {
+	{ .compatible = "dongwoon,dw9768" },
+	{ .compatible = "giantec,gt9769" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, dw9768_of_table);
+
+static const struct dev_pm_ops dw9768_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				pm_runtime_force_resume)
+	SET_RUNTIME_PM_OPS(dw9768_runtime_suspend, dw9768_runtime_resume, NULL)
+};
+
+static struct i2c_driver dw9768_i2c_driver = {
+	.driver = {
+		.name = DW9768_NAME,
+		.pm = &dw9768_pm_ops,
+		.of_match_table = dw9768_of_table,
+	},
+	.probe_new  = dw9768_probe,
+	.remove = dw9768_remove,
+};
+module_i2c_driver(dw9768_i2c_driver);
+
+MODULE_AUTHOR("Dongchun Zhu <dongchun.zhu@mediatek.com>");
+MODULE_DESCRIPTION("DW9768 VCM driver");
+MODULE_LICENSE("GPL v2");
-- 
2.9.2

Re: [V6, 1/2] media: dt-bindings: media: i2c: Document DW9768 bindings

[Tomasz Figa]

Hi Dongchun,

On Mon, May 18, 2020 at 3:29 PM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
>
> Add DeviceTree binding documentation for Dongwoon Anatech DW9768 voice
> coil actuator.

Thanks for the patch. Please see my comments below.

>
> Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> Reviewed-by: Rob Herring <robh@kernel.org>

This version includes significant changes, so the reviewed-by tag
shouldn't have been carried out.

> ---
>  .../bindings/media/i2c/dongwoon,dw9768.yaml        | 105 +++++++++++++++++++++
>  MAINTAINERS                                        |   7 ++
>  2 files changed, 112 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml b/Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
> new file mode 100644
> index 0000000..b909e83
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
> @@ -0,0 +1,105 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright (c) 2020 MediaTek Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/dongwoon,dw9768.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Dongwoon Anatech DW9768 Voice Coil Motor (VCM) Lens Device Tree Bindings
> +
> +maintainers:
> +  - Dongchun Zhu <dongchun.zhu@mediatek.com>
> +
> +description: |-
> +  The Dongwoon DW9768 is a single 10-bit digital-to-analog (DAC) converter
> +  with 100 mA output current sink capability. VCM current is controlled with
> +  a linear mode driver. The DAC is controlled via a 2-wire (I2C-compatible)
> +  serial interface that operates at clock rates up to 1MHz. This chip
> +  integrates Advanced Actuator Control (AAC) technology and is intended for
> +  driving voice coil lenses in camera modules.
> +
> +properties:
> +  compatible:
> +    enum:
> +      # for DW9768 VCM
> +      - dongwoon,dw9768
> +      # for GT9769 VCM
> +      - giantec,gt9769
> +
> +  reg:
> +    maxItems: 1
> +
> +  vin-supply:
> +    description:
> +      Definition of the regulator used as I2C I/O interface power supply.
> +
> +  vdd-supply:
> +    description:
> +      Definition of the regulator used as VCM chip power supply.
> +
> +  dongwoon,aac-mode:
> +    description:
> +      Indication of AAC mode select.
> +    allOf:
> +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> +      - enum:
> +          - 0    #  Direct (default)
> +          - 1    #  AAC2 (operation time# 0.48 x Tvib)
> +          - 2    #  AAC3 (operation time# 0.70 x Tvib)
> +          - 3    #  AAC4 (operation time# 0.75 x Tvib)
> +          - 4    #  Reserved
> +          - 5    #  AAC8 (operation time# 1.13 x Tvib)
> +          - 6    #  Reserved
> +          - 7    #  Reserved

I'll ultimately leave it to DT maintainers, but is there any reason to
define the reserved values?

> +
> +  dongwoon,aac-timing:
> +    description:
> +      Indication of AAC Timing count, unit of 0.1 milliseconds.
> +      Valid values vary from 0 to 63 (default 32).
> +    allOf:
> +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> +
> +  dongwoon,clock-dividing-rate:
> +    description:
> +      Indication of VCM internal clock dividing rate select, as one multiple
> +      factor to calculate VCM ring periodic time Tvib.
> +    allOf:
> +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> +      - enum:
> +          - 0    #  Dividing Rate -  2
> +          - 1    #  Dividing Rate -  1 (default)
> +          - 2    #  Dividing Rate -  1/2
> +          - 3    #  Dividing Rate -  1/4
> +          - 4    #  Dividing Rate -  8
> +          - 5    #  Dividing Rate -  4
> +          - 6    #  Dividing Rate -  Reserved
> +          - 7    #  Dividing Rate -  Reserved

Ditto.

Best regards,
Tomasz

Re: [V6, 1/2] media: dt-bindings: media: i2c: Document DW9768 bindings

[Rob Herring]

On Mon, May 18, 2020 at 04:12:28PM +0200, Tomasz Figa wrote:
> Hi Dongchun,
> 
> On Mon, May 18, 2020 at 3:29 PM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> >
> > Add DeviceTree binding documentation for Dongwoon Anatech DW9768 voice
> > coil actuator.
> 
> Thanks for the patch. Please see my comments below.
> 
> >
> > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > Reviewed-by: Rob Herring <robh@kernel.org>
> 
> This version includes significant changes, so the reviewed-by tag
> shouldn't have been carried out.
> 
> > ---
> >  .../bindings/media/i2c/dongwoon,dw9768.yaml        | 105 +++++++++++++++++++++
> >  MAINTAINERS                                        |   7 ++
> >  2 files changed, 112 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml b/Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
> > new file mode 100644
> > index 0000000..b909e83
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
> > @@ -0,0 +1,105 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +# Copyright (c) 2020 MediaTek Inc.
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/i2c/dongwoon,dw9768.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Dongwoon Anatech DW9768 Voice Coil Motor (VCM) Lens Device Tree Bindings
> > +
> > +maintainers:
> > +  - Dongchun Zhu <dongchun.zhu@mediatek.com>
> > +
> > +description: |-
> > +  The Dongwoon DW9768 is a single 10-bit digital-to-analog (DAC) converter
> > +  with 100 mA output current sink capability. VCM current is controlled with
> > +  a linear mode driver. The DAC is controlled via a 2-wire (I2C-compatible)
> > +  serial interface that operates at clock rates up to 1MHz. This chip
> > +  integrates Advanced Actuator Control (AAC) technology and is intended for
> > +  driving voice coil lenses in camera modules.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      # for DW9768 VCM
> > +      - dongwoon,dw9768
> > +      # for GT9769 VCM
> > +      - giantec,gt9769
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  vin-supply:
> > +    description:
> > +      Definition of the regulator used as I2C I/O interface power supply.
> > +
> > +  vdd-supply:
> > +    description:
> > +      Definition of the regulator used as VCM chip power supply.
> > +
> > +  dongwoon,aac-mode:
> > +    description:
> > +      Indication of AAC mode select.
> > +    allOf:
> > +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> > +      - enum:
> > +          - 0    #  Direct (default)

Default can be expressed as 'default: 0'.

> > +          - 1    #  AAC2 (operation time# 0.48 x Tvib)
> > +          - 2    #  AAC3 (operation time# 0.70 x Tvib)
> > +          - 3    #  AAC4 (operation time# 0.75 x Tvib)
> > +          - 4    #  Reserved
> > +          - 5    #  AAC8 (operation time# 1.13 x Tvib)
> > +          - 6    #  Reserved
> > +          - 7    #  Reserved
> 
> I'll ultimately leave it to DT maintainers, but is there any reason to
> define the reserved values?

No.

> 
> > +
> > +  dongwoon,aac-timing:
> > +    description:
> > +      Indication of AAC Timing count, unit of 0.1 milliseconds.

Why not just use standard units (-us)?

> > +      Valid values vary from 0 to 63 (default 32).

Looks like constraints.

> > +    allOf:
> > +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> > +
> > +  dongwoon,clock-dividing-rate:
> > +    description:
> > +      Indication of VCM internal clock dividing rate select, as one multiple
> > +      factor to calculate VCM ring periodic time Tvib.
> > +    allOf:
> > +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> > +      - enum:
> > +          - 0    #  Dividing Rate -  2
> > +          - 1    #  Dividing Rate -  1 (default)
> > +          - 2    #  Dividing Rate -  1/2
> > +          - 3    #  Dividing Rate -  1/4
> > +          - 4    #  Dividing Rate -  8
> > +          - 5    #  Dividing Rate -  4
> > +          - 6    #  Dividing Rate -  Reserved
> > +          - 7    #  Dividing Rate -  Reserved
> 
> Ditto.
> 
> Best regards,
> Tomasz

Re: [V6, 1/2] media: dt-bindings: media: i2c: Document DW9768 bindings

[Dongchun Zhu]

Hi Tomasz, Rob,

Thanks for the timely review and good suggestions.

On Mon, 2020-05-18 at 08:31 -0600, Rob Herring wrote:
> On Mon, May 18, 2020 at 04:12:28PM +0200, Tomasz Figa wrote:
> > Hi Dongchun,
> > 
> > On Mon, May 18, 2020 at 3:29 PM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> > >
> > > Add DeviceTree binding documentation for Dongwoon Anatech DW9768 voice
> > > coil actuator.
> > 
> > Thanks for the patch. Please see my comments below.
> > 
> > >
> > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > > Reviewed-by: Rob Herring <robh@kernel.org>
> > 
> > This version includes significant changes, so the reviewed-by tag
> > shouldn't have been carried out.
> > 

Sorry, this is my fault.
In fact, I've struggled with it at the beginning.
Yes, you are right.
Now the new version of patch-set includes huge changes relative to the
last edition.
It would be removed in next release.

> > > ---
> > >  .../bindings/media/i2c/dongwoon,dw9768.yaml        | 105 +++++++++++++++++++++
> > >  MAINTAINERS                                        |   7 ++
> > >  2 files changed, 112 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml b/Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
> > > new file mode 100644
> > > index 0000000..b909e83
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
> > > @@ -0,0 +1,105 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +# Copyright (c) 2020 MediaTek Inc.
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/media/i2c/dongwoon,dw9768.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Dongwoon Anatech DW9768 Voice Coil Motor (VCM) Lens Device Tree Bindings
> > > +
> > > +maintainers:
> > > +  - Dongchun Zhu <dongchun.zhu@mediatek.com>
> > > +
> > > +description: |-
> > > +  The Dongwoon DW9768 is a single 10-bit digital-to-analog (DAC) converter
> > > +  with 100 mA output current sink capability. VCM current is controlled with
> > > +  a linear mode driver. The DAC is controlled via a 2-wire (I2C-compatible)
> > > +  serial interface that operates at clock rates up to 1MHz. This chip
> > > +  integrates Advanced Actuator Control (AAC) technology and is intended for
> > > +  driving voice coil lenses in camera modules.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      # for DW9768 VCM
> > > +      - dongwoon,dw9768
> > > +      # for GT9769 VCM
> > > +      - giantec,gt9769
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  vin-supply:
> > > +    description:
> > > +      Definition of the regulator used as I2C I/O interface power supply.
> > > +
> > > +  vdd-supply:
> > > +    description:
> > > +      Definition of the regulator used as VCM chip power supply.
> > > +
> > > +  dongwoon,aac-mode:
> > > +    description:
> > > +      Indication of AAC mode select.
> > > +    allOf:
> > > +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> > > +      - enum:
> > > +          - 0    #  Direct (default)
> 
> Default can be expressed as 'default: 0'.
> 

Thanks for the reminder.
Next release I'd try to write DT property "dongwoon,aac-mode" like this:
dongwoon,aac-mode:
  description:
    Indication of AAC mode select.
  allOf:
    - $ref: "/schemas/types.yaml#/definitions/uint32"
    - enum:
        - 0  # Direct
        - 1  # AAC2 (operation time# 0.48 x Tvib)
        - 2  # AAC3 (operation time# 0.70 x Tvib)
        - 3  # AAC4 (operation time# 0.75 x Tvib)
        - 5  # AAC8 (operation time# 1.13 x Tvib)
      default: 0

> > > +          - 1    #  AAC2 (operation time# 0.48 x Tvib)
> > > +          - 2    #  AAC3 (operation time# 0.70 x Tvib)
> > > +          - 3    #  AAC4 (operation time# 0.75 x Tvib)
> > > +          - 4    #  Reserved
> > > +          - 5    #  AAC8 (operation time# 1.13 x Tvib)
> > > +          - 6    #  Reserved
> > > +          - 7    #  Reserved
> > 
> > I'll ultimately leave it to DT maintainers, but is there any reason to
> > define the reserved values?
> 
> No.
> 
> > 
> > > +
> > > +  dongwoon,aac-timing:
> > > +    description:
> > > +      Indication of AAC Timing count, unit of 0.1 milliseconds.
> 
> Why not just use standard units (-us)?
> 

That sounds nice.
I'd re-write the description like this in next release:
dongwoon,aac-timing:
  description:
    Number of AAC Timing count that controlled by one 6-bit period
    of vibration register AACT[5:0], the unit of which is 100us.
  allOf:
    - $ref: "/schemas/types.yaml#/definitions/uint32"
    - minimum: 0x00
    - maximum: 0x3F
    - default: 0x20

> > > +      Valid values vary from 0 to 63 (default 32).
> 
> Looks like constraints.
> 

Yes. This property is controlled by one 6-bit reg.
So here we need to constrain the data set to a narrow range.

> > > +    allOf:
> > > +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> > > +
> > > +  dongwoon,clock-dividing-rate:
> > > +    description:
> > > +      Indication of VCM internal clock dividing rate select, as one multiple
> > > +      factor to calculate VCM ring periodic time Tvib.
> > > +    allOf:
> > > +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> > > +      - enum:
> > > +          - 0    #  Dividing Rate -  2
> > > +          - 1    #  Dividing Rate -  1 (default)
> > > +          - 2    #  Dividing Rate -  1/2
> > > +          - 3    #  Dividing Rate -  1/4
> > > +          - 4    #  Dividing Rate -  8
> > > +          - 5    #  Dividing Rate -  4
> > > +          - 6    #  Dividing Rate -  Reserved
> > > +          - 7    #  Dividing Rate -  Reserved
> > 
> > Ditto.
> > 

Thanks for kindly reminder.
Reserved values would be removed in next release.
Like this:
dongwoon,clock-dividing-rate:
  description:
    Indication of VCM internal clock dividing rate select, as one    
    multiplier to calculate VCM ring periodic time Tvib.
  allOf:
    - $ref: "/schemas/types.yaml#/definitions/uint32"
    - enum:
        - 0   #  Dividing Rate -  2
        - 1   #  Dividing Rate -  1
        - 2    #  Dividing Rate -  1/2
        - 3    #  Dividing Rate -  1/4
        - 4    #  Dividing Rate -  8
        - 5    #  Dividing Rate -  4
      default: 1

> > Best regards,
> > Tomasz

Re: [V6, 2/2] media: i2c: dw9768: Add DW9768 VCM driver

[Tomasz Figa]

Hi Dongchun, Sakari,

On Mon, May 18, 2020 at 09:27:31PM +0800, Dongchun Zhu wrote:
> Add a V4L2 sub-device driver for DW9768 voice coil motor, providing
> control to set the desired focus via IIC serial interface.
> 
> Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> ---
>  MAINTAINERS                |   1 +
>  drivers/media/i2c/Kconfig  |  13 ++
>  drivers/media/i2c/Makefile |   1 +
>  drivers/media/i2c/dw9768.c | 515 +++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 530 insertions(+)
>  create mode 100644 drivers/media/i2c/dw9768.c
[snip]
> +/*
> + * DW9768_AAC_PRESC_REG & DW9768_AAC_TIME_REG determine VCM operation time.
> + * If DW9768_AAC_PRESC_REG is 0x41, and DW9768_AAC_TIME_REG is 0x39, VCM mode
> + * would be AAC3, Operation Time would be 0.70xTvib, that is 8.40ms.
> + */
> +#define DW9768_MOVE_DELAY_US			8400
> +#define DW9768_STABLE_TIME_US			20000

These times are only valid with the specific settings mentioned in the
comment. If one sets different settings in DT, the driver would apply
incorrect delays. Rather than hardcoded, they should be computed based
on the configured values.

That said, I wonder if we're not digging too deep now. Sakari, do you
think we could take a step back, remove the optional DT properties and
just support the fixed values for now, so that we can get a basic driver
upstream first without doubling the effort?

> +
> +static const char * const dw9768_supply_names[] = {
> +	"vin",	/* I2C I/O interface power */
> +	"vdd",	/* VCM power */
> +};
> +
> +/* dw9768 device structure */
> +struct dw9768 {
> +	struct regulator_bulk_data supplies[ARRAY_SIZE(dw9768_supply_names)];
> +	struct v4l2_ctrl_handler ctrls;
> +	struct v4l2_ctrl	*focus;
> +	struct v4l2_subdev	sd;
> +
> +	u32			aac_mode;
> +	u32			aac_timing;
> +	u32			clock_dividing_rate;
> +	bool			aac_mode_control_enable;
> +	bool			aact_cnt_select_enable;
> +	bool			clock_dividing_rate_select_enable;

nit: Separate types from names with just 1 space.

> +};
> +
> +static inline struct dw9768 *sd_to_dw9768(struct v4l2_subdev *subdev)
> +{
> +	return container_of(subdev, struct dw9768, sd);
> +}
> +
> +struct regval_list {
> +	u8 reg_num;
> +	u8 value;
> +};
> +
> +static int dw9768_read_smbus(struct dw9768 *dw9768, unsigned char reg,
> +			     unsigned char *val)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(client, reg);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = (unsigned char)ret;
> +
> +	return 0;
> +}

Why do we need this function? Couldn't we just call
i2c_smbus_read_byte_data() directly?

[snip]
> +static int dw9768_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct dw9768 *dw9768;
> +	unsigned int aac_mode_select;
> +	unsigned int aac_timing_select;
> +	unsigned int clock_dividing_rate_select;
> +	unsigned int i;
> +	int ret;
> +
> +	dw9768 = devm_kzalloc(dev, sizeof(*dw9768), GFP_KERNEL);
> +	if (!dw9768)
> +		return -ENOMEM;
> +
> +	v4l2_i2c_subdev_init(&dw9768->sd, client, &dw9768_ops);
> +	dw9768->aac_mode_control_enable = false;
> +	dw9768->aact_cnt_select_enable = false;
> +	dw9768->clock_dividing_rate_select_enable = false;

devm_kzalloc() initializes the memory to zero, so no need to set anything
to false explicitly.

> +
> +	/* Optional indication of AAC mode select */
> +	ret = fwnode_property_read_u32(dev_fwnode(dev), "dongwoon,aac-mode",
> +				       &aac_mode_select);
> +
> +	if (!ret) {
> +		dw9768->aac_mode_control_enable = true;
> +		dw9768->aac_mode = aac_mode_select;

How about making aac_mode a signed int and assigning -1 by
default? Then we don't need two separate fields in the struct.

> +	}
> +
> +	/* Optional indication of VCM internal clock dividing rate select */
> +	ret = fwnode_property_read_u32(dev_fwnode(dev),
> +				       "dongwoon,clock-dividing-rate",
> +				       &clock_dividing_rate_select);
> +
> +	if (!ret) {
> +		dw9768->clock_dividing_rate_select_enable = true;
> +		dw9768->clock_dividing_rate = clock_dividing_rate_select;

Ditto.

> +	}
> +
> +	/* Optional indication of AAC Timing */
> +	ret = fwnode_property_read_u32(dev_fwnode(dev), "dongwoon,aac-timing",
> +				       &aac_timing_select);
> +
> +	if (!ret) {
> +		dw9768->aact_cnt_select_enable = true;
> +		dw9768->aac_timing = aac_timing_select;

Ditto.

> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(dw9768_supply_names); i++)
> +		dw9768->supplies[i].supply = dw9768_supply_names[i];
> +
> +	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(dw9768_supply_names),
> +				      dw9768->supplies);
> +	if (ret) {
> +		dev_err(dev, "failed to get regulators\n");
> +		return ret;
> +	}
> +
> +	ret = dw9768_init_controls(dw9768);
> +	if (ret)
> +		goto entity_cleanup;
> +
> +	dw9768->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	dw9768->sd.internal_ops = &dw9768_int_ops;
> +
> +	ret = media_entity_pads_init(&dw9768->sd.entity, 0, NULL);
> +	if (ret < 0)
> +		goto entity_cleanup;
> +
> +	dw9768->sd.entity.function = MEDIA_ENT_F_LENS;
> +
> +	pm_runtime_enable(dev);
> +	if (!pm_runtime_enabled(dev)) {
> +		ret = dw9768_runtime_resume(dev);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to power on: %d\n", ret);
> +			goto entity_cleanup;
> +		}
> +	}
> +
> +	ret = v4l2_async_register_subdev(&dw9768->sd);
> +	if (ret < 0)
> +		goto entity_cleanup;
> +
> +	return 0;
> +
> +entity_cleanup:

Need to power off if the code above powered on.

> +	v4l2_ctrl_handler_free(&dw9768->ctrls);
> +	media_entity_cleanup(&dw9768->sd.entity);
> +	return ret;
> +}
> +
> +static int dw9768_remove(struct i2c_client *client)
> +{
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct dw9768 *dw9768 = sd_to_dw9768(sd);
> +
> +	pm_runtime_disable(&client->dev);

First the device must be unregistered from the userspace. Otherwise there
is a race condition that risks the userspace accessing the device while the
deinitialization is happening.

Best regards,
Tomasz

Re: [V6, 2/2] media: i2c: dw9768: Add DW9768 VCM driver

[Dongchun Zhu]

Hi Tomasz,

Thanks for the review. My replies are as below.

On Thu, 2020-05-21 at 19:51 +0000, Tomasz Figa wrote:
> Hi Dongchun, Sakari,
> 
> On Mon, May 18, 2020 at 09:27:31PM +0800, Dongchun Zhu wrote:
> > Add a V4L2 sub-device driver for DW9768 voice coil motor, providing
> > control to set the desired focus via IIC serial interface.
> > 
> > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > ---
> >  MAINTAINERS                |   1 +
> >  drivers/media/i2c/Kconfig  |  13 ++
> >  drivers/media/i2c/Makefile |   1 +
> >  drivers/media/i2c/dw9768.c | 515 +++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 530 insertions(+)
> >  create mode 100644 drivers/media/i2c/dw9768.c
> [snip]
> > +/*
> > + * DW9768_AAC_PRESC_REG & DW9768_AAC_TIME_REG determine VCM operation time.
> > + * If DW9768_AAC_PRESC_REG is 0x41, and DW9768_AAC_TIME_REG is 0x39, VCM mode
> > + * would be AAC3, Operation Time would be 0.70xTvib, that is 8.40ms.
> > + */
> > +#define DW9768_MOVE_DELAY_US			8400
> > +#define DW9768_STABLE_TIME_US			20000
> 
> These times are only valid with the specific settings mentioned in the
> comment. If one sets different settings in DT, the driver would apply
> incorrect delays. Rather than hardcoded, they should be computed based
> on the configured values.
> 
> That said, I wonder if we're not digging too deep now. Sakari, do you
> think we could take a step back, remove the optional DT properties and
> just support the fixed values for now, so that we can get a basic driver
> upstream first without doubling the effort?
> 

Thanks for the reminder.
Yes, here DW9768_MOVE_DELAY_US actually represents Operation Time,
which is dependent upon board-specific settings that defined in DT.
For instance, for one given board, if aac-mode is 2, aac-timing is 0x39,
clock-presc is 1, then Operation Time would be 0.70*(6.3ms+57*0.1ms)*1 =
8.4ms.

> > +
> > +static const char * const dw9768_supply_names[] = {
> > +	"vin",	/* I2C I/O interface power */
> > +	"vdd",	/* VCM power */
> > +};
> > +
> > +/* dw9768 device structure */
> > +struct dw9768 {
> > +	struct regulator_bulk_data supplies[ARRAY_SIZE(dw9768_supply_names)];
> > +	struct v4l2_ctrl_handler ctrls;
> > +	struct v4l2_ctrl	*focus;
> > +	struct v4l2_subdev	sd;
> > +
> > +	u32			aac_mode;
> > +	u32			aac_timing;
> > +	u32			clock_dividing_rate;
> > +	bool			aac_mode_control_enable;
> > +	bool			aact_cnt_select_enable;
> > +	bool			clock_dividing_rate_select_enable;
> 
> nit: Separate types from names with just 1 space.
> 

Fixed in next release.

> > +};
> > +
> > +static inline struct dw9768 *sd_to_dw9768(struct v4l2_subdev *subdev)
> > +{
> > +	return container_of(subdev, struct dw9768, sd);
> > +}
> > +
> > +struct regval_list {
> > +	u8 reg_num;
> > +	u8 value;
> > +};
> > +
> > +static int dw9768_read_smbus(struct dw9768 *dw9768, unsigned char reg,
> > +			     unsigned char *val)
> > +{
> > +	struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
> > +	int ret;
> > +
> > +	ret = i2c_smbus_read_byte_data(client, reg);
> > +
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	*val = (unsigned char)ret;
> > +
> > +	return 0;
> > +}
> 
> Why do we need this function? Couldn't we just call
> i2c_smbus_read_byte_data() directly?
> 

Fixed in next release.

> [snip]
> > +static int dw9768_probe(struct i2c_client *client)
> > +{
> > +	struct device *dev = &client->dev;
> > +	struct dw9768 *dw9768;
> > +	unsigned int aac_mode_select;
> > +	unsigned int aac_timing_select;
> > +	unsigned int clock_dividing_rate_select;
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	dw9768 = devm_kzalloc(dev, sizeof(*dw9768), GFP_KERNEL);
> > +	if (!dw9768)
> > +		return -ENOMEM;
> > +
> > +	v4l2_i2c_subdev_init(&dw9768->sd, client, &dw9768_ops);
> > +	dw9768->aac_mode_control_enable = false;
> > +	dw9768->aact_cnt_select_enable = false;
> > +	dw9768->clock_dividing_rate_select_enable = false;
> 
> devm_kzalloc() initializes the memory to zero, so no need to set anything
> to false explicitly.
> 

Thanks for the reminder.
Yes, these parameters shall not be needed to initialized as zeros.

> > +
> > +	/* Optional indication of AAC mode select */
> > +	ret = fwnode_property_read_u32(dev_fwnode(dev), "dongwoon,aac-mode",
> > +				       &aac_mode_select);
> > +
> > +	if (!ret) {
> > +		dw9768->aac_mode_control_enable = true;
> > +		dw9768->aac_mode = aac_mode_select;
> 
> How about making aac_mode a signed int and assigning -1 by
> default? Then we don't need two separate fields in the struct.
> 

Good idea.

> > +	}
> > +
> > +	/* Optional indication of VCM internal clock dividing rate select */
> > +	ret = fwnode_property_read_u32(dev_fwnode(dev),
> > +				       "dongwoon,clock-dividing-rate",
> > +				       &clock_dividing_rate_select);
> > +
> > +	if (!ret) {
> > +		dw9768->clock_dividing_rate_select_enable = true;
> > +		dw9768->clock_dividing_rate = clock_dividing_rate_select;
> 
> Ditto.
> 

Got it.

> > +	}
> > +
> > +	/* Optional indication of AAC Timing */
> > +	ret = fwnode_property_read_u32(dev_fwnode(dev), "dongwoon,aac-timing",
> > +				       &aac_timing_select);
> > +
> > +	if (!ret) {
> > +		dw9768->aact_cnt_select_enable = true;
> > +		dw9768->aac_timing = aac_timing_select;
> 
> Ditto.
> 

Got it.

> > +	}
> > +
> > +	for (i = 0; i < ARRAY_SIZE(dw9768_supply_names); i++)
> > +		dw9768->supplies[i].supply = dw9768_supply_names[i];
> > +
> > +	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(dw9768_supply_names),
> > +				      dw9768->supplies);
> > +	if (ret) {
> > +		dev_err(dev, "failed to get regulators\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = dw9768_init_controls(dw9768);
> > +	if (ret)
> > +		goto entity_cleanup;
> > +
> > +	dw9768->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > +	dw9768->sd.internal_ops = &dw9768_int_ops;
> > +
> > +	ret = media_entity_pads_init(&dw9768->sd.entity, 0, NULL);
> > +	if (ret < 0)
> > +		goto entity_cleanup;
> > +
> > +	dw9768->sd.entity.function = MEDIA_ENT_F_LENS;
> > +
> > +	pm_runtime_enable(dev);
> > +	if (!pm_runtime_enabled(dev)) {
> > +		ret = dw9768_runtime_resume(dev);
> > +		if (ret < 0) {
> > +			dev_err(dev, "failed to power on: %d\n", ret);
> > +			goto entity_cleanup;
> > +		}
> > +	}
> > +
> > +	ret = v4l2_async_register_subdev(&dw9768->sd);
> > +	if (ret < 0)
> > +		goto entity_cleanup;
> > +
> > +	return 0;
> > +
> > +entity_cleanup:
> 
> Need to power off if the code above powered on.
> 

Thanks for the reminder.
If there is something wrong with runtime PM, actuator is to be powered
on via dw9768_runtime_resume() API.
When actuator sub-device is powered on completely and async registered
successfully, we shall power off it afterwards.

> > +	v4l2_ctrl_handler_free(&dw9768->ctrls);
> > +	media_entity_cleanup(&dw9768->sd.entity);
> > +	return ret;
> > +}
> > +
> > +static int dw9768_remove(struct i2c_client *client)
> > +{
> > +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > +	struct dw9768 *dw9768 = sd_to_dw9768(sd);
> > +
> > +	pm_runtime_disable(&client->dev);
> 
> First the device must be unregistered from the userspace. Otherwise there
> is a race condition that risks the userspace accessing the device while the
> deinitialization is happening.
> 

Fixed in next release by adjusting the sequence of unregistering and
runtime PM disable.

> Best regards,
> Tomasz

Re: [V6, 2/2] media: i2c: dw9768: Add DW9768 VCM driver

[Tomasz Figa]

On Fri, May 22, 2020 at 11:27 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
>
> Hi Tomasz,
>
> Thanks for the review. My replies are as below.
>
> On Thu, 2020-05-21 at 19:51 +0000, Tomasz Figa wrote:
> > Hi Dongchun, Sakari,
> >
> > On Mon, May 18, 2020 at 09:27:31PM +0800, Dongchun Zhu wrote:
[snip]
> > > +   pm_runtime_enable(dev);
> > > +   if (!pm_runtime_enabled(dev)) {
> > > +           ret = dw9768_runtime_resume(dev);
> > > +           if (ret < 0) {
> > > +                   dev_err(dev, "failed to power on: %d\n", ret);
> > > +                   goto entity_cleanup;
> > > +           }
> > > +   }
> > > +
> > > +   ret = v4l2_async_register_subdev(&dw9768->sd);
> > > +   if (ret < 0)
> > > +           goto entity_cleanup;
> > > +
> > > +   return 0;
> > > +
> > > +entity_cleanup:
> >
> > Need to power off if the code above powered on.
> >
>
> Thanks for the reminder.
> If there is something wrong with runtime PM, actuator is to be powered
> on via dw9768_runtime_resume() API.
> When actuator sub-device is powered on completely and async registered
> successfully, we shall power off it afterwards.
>

The code above calls dw9768_runtime_resume() if
!pm_runtime_enabled(dev), but the clean-up code below the
entity_cleanup label doesn't have the corresponding
dw9768_runtime_suspend() call.

Best regards,
Tomasz

Re: [V6, 2/2] media: i2c: dw9768: Add DW9768 VCM driver

[Dongchun Zhu]

Hi Tomasz,

On Mon, 2020-05-25 at 13:45 +0200, Tomasz Figa wrote:
> On Fri, May 22, 2020 at 11:27 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> >
> > Hi Tomasz,
> >
> > Thanks for the review. My replies are as below.
> >
> > On Thu, 2020-05-21 at 19:51 +0000, Tomasz Figa wrote:
> > > Hi Dongchun, Sakari,
> > >
> > > On Mon, May 18, 2020 at 09:27:31PM +0800, Dongchun Zhu wrote:
> [snip]
> > > > +   pm_runtime_enable(dev);
> > > > +   if (!pm_runtime_enabled(dev)) {
> > > > +           ret = dw9768_runtime_resume(dev);
> > > > +           if (ret < 0) {
> > > > +                   dev_err(dev, "failed to power on: %d\n", ret);
> > > > +                   goto entity_cleanup;
> > > > +           }
> > > > +   }
> > > > +
> > > > +   ret = v4l2_async_register_subdev(&dw9768->sd);
> > > > +   if (ret < 0)
> > > > +           goto entity_cleanup;
> > > > +
> > > > +   return 0;
> > > > +
> > > > +entity_cleanup:
> > >
> > > Need to power off if the code above powered on.
> > >
> >
> > Thanks for the reminder.
> > If there is something wrong with runtime PM, actuator is to be powered
> > on via dw9768_runtime_resume() API.
> > When actuator sub-device is powered on completely and async registered
> > successfully, we shall power off it afterwards.
> >
> 
> The code above calls dw9768_runtime_resume() if
> !pm_runtime_enabled(dev), but the clean-up code below the
> entity_cleanup label doesn't have the corresponding
> dw9768_runtime_suspend() call.
> 

Did you mean the 'entity_cleanup' after v4l2_async_register_subdev()?
Actually I made some changes for OV02A V9, according to this comment.
Could you help review that change? Thanks.

> Best regards,
> Tomasz

Re: [V6, 2/2] media: i2c: dw9768: Add DW9768 VCM driver

[Sakari Ailus]

On Mon, May 25, 2020 at 01:45:07PM +0200, Tomasz Figa wrote:
> On Fri, May 22, 2020 at 11:27 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> >
> > Hi Tomasz,
> >
> > Thanks for the review. My replies are as below.
> >
> > On Thu, 2020-05-21 at 19:51 +0000, Tomasz Figa wrote:
> > > Hi Dongchun, Sakari,
> > >
> > > On Mon, May 18, 2020 at 09:27:31PM +0800, Dongchun Zhu wrote:
> [snip]
> > > > +   pm_runtime_enable(dev);
> > > > +   if (!pm_runtime_enabled(dev)) {
> > > > +           ret = dw9768_runtime_resume(dev);
> > > > +           if (ret < 0) {
> > > > +                   dev_err(dev, "failed to power on: %d\n", ret);
> > > > +                   goto entity_cleanup;
> > > > +           }
> > > > +   }
> > > > +
> > > > +   ret = v4l2_async_register_subdev(&dw9768->sd);
> > > > +   if (ret < 0)
> > > > +           goto entity_cleanup;
> > > > +
> > > > +   return 0;
> > > > +
> > > > +entity_cleanup:
> > >
> > > Need to power off if the code above powered on.
> > >
> >
> > Thanks for the reminder.
> > If there is something wrong with runtime PM, actuator is to be powered
> > on via dw9768_runtime_resume() API.
> > When actuator sub-device is powered on completely and async registered
> > successfully, we shall power off it afterwards.
> >
> 
> The code above calls dw9768_runtime_resume() if
> !pm_runtime_enabled(dev), but the clean-up code below the
> entity_cleanup label doesn't have the corresponding
> dw9768_runtime_suspend() call.

Yes, an example on using runtime PM can be found in e.g. ov8856 driver.

The open / release callbacks seem fine though. Sensors just don't need
them as they have the streaming state (and s_stream).

-- 
Sakari Ailus

Re: [V6, 2/2] media: i2c: dw9768: Add DW9768 VCM driver

[Tomasz Figa]

On Wed, May 27, 2020 at 11:03 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
>
> Hi Tomasz,
>
> On Mon, 2020-05-25 at 13:45 +0200, Tomasz Figa wrote:
> > On Fri, May 22, 2020 at 11:27 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> > >
> > > Hi Tomasz,
> > >
> > > Thanks for the review. My replies are as below.
> > >
> > > On Thu, 2020-05-21 at 19:51 +0000, Tomasz Figa wrote:
> > > > Hi Dongchun, Sakari,
> > > >
> > > > On Mon, May 18, 2020 at 09:27:31PM +0800, Dongchun Zhu wrote:
> > [snip]
> > > > > +   pm_runtime_enable(dev);
> > > > > +   if (!pm_runtime_enabled(dev)) {
> > > > > +           ret = dw9768_runtime_resume(dev);
> > > > > +           if (ret < 0) {
> > > > > +                   dev_err(dev, "failed to power on: %d\n", ret);
> > > > > +                   goto entity_cleanup;
> > > > > +           }
> > > > > +   }
> > > > > +
> > > > > +   ret = v4l2_async_register_subdev(&dw9768->sd);
> > > > > +   if (ret < 0)
> > > > > +           goto entity_cleanup;
> > > > > +
> > > > > +   return 0;
> > > > > +
> > > > > +entity_cleanup:
> > > >
> > > > Need to power off if the code above powered on.
> > > >
> > >
> > > Thanks for the reminder.
> > > If there is something wrong with runtime PM, actuator is to be powered
> > > on via dw9768_runtime_resume() API.
> > > When actuator sub-device is powered on completely and async registered
> > > successfully, we shall power off it afterwards.
> > >
> >
> > The code above calls dw9768_runtime_resume() if
> > !pm_runtime_enabled(dev), but the clean-up code below the
> > entity_cleanup label doesn't have the corresponding
> > dw9768_runtime_suspend() call.
> >
>
> Did you mean the 'entity_cleanup' after v4l2_async_register_subdev()?

Yes.

> Actually I made some changes for OV02A V9, according to this comment.
> Could you help review that change? Thanks.

Sure, I will take a look.

Best regards,
Tomasz

Re: [V6, 2/2] media: i2c: dw9768: Add DW9768 VCM driver

[Dongchun Zhu]

Hi Tomasz,

On Mon, 2020-06-01 at 20:47 +0200, Tomasz Figa wrote:
> On Wed, May 27, 2020 at 11:03 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> >
> > Hi Tomasz,
> >
> > On Mon, 2020-05-25 at 13:45 +0200, Tomasz Figa wrote:
> > > On Fri, May 22, 2020 at 11:27 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> > > >
> > > > Hi Tomasz,
> > > >
> > > > Thanks for the review. My replies are as below.
> > > >
> > > > On Thu, 2020-05-21 at 19:51 +0000, Tomasz Figa wrote:
> > > > > Hi Dongchun, Sakari,
> > > > >
> > > > > On Mon, May 18, 2020 at 09:27:31PM +0800, Dongchun Zhu wrote:
> > > [snip]
> > > > > > +   pm_runtime_enable(dev);
> > > > > > +   if (!pm_runtime_enabled(dev)) {
> > > > > > +           ret = dw9768_runtime_resume(dev);
> > > > > > +           if (ret < 0) {
> > > > > > +                   dev_err(dev, "failed to power on: %d\n", ret);
> > > > > > +                   goto entity_cleanup;
> > > > > > +           }
> > > > > > +   }
> > > > > > +
> > > > > > +   ret = v4l2_async_register_subdev(&dw9768->sd);
> > > > > > +   if (ret < 0)
> > > > > > +           goto entity_cleanup;
> > > > > > +
> > > > > > +   return 0;
> > > > > > +
> > > > > > +entity_cleanup:
> > > > >
> > > > > Need to power off if the code above powered on.
> > > > >
> > > >
> > > > Thanks for the reminder.
> > > > If there is something wrong with runtime PM, actuator is to be powered
> > > > on via dw9768_runtime_resume() API.
> > > > When actuator sub-device is powered on completely and async registered
> > > > successfully, we shall power off it afterwards.
> > > >
> > >
> > > The code above calls dw9768_runtime_resume() if
> > > !pm_runtime_enabled(dev), but the clean-up code below the
> > > entity_cleanup label doesn't have the corresponding
> > > dw9768_runtime_suspend() call.
> > >
> >
> > Did you mean the 'entity_cleanup' after v4l2_async_register_subdev()?
> 
> Yes.
> 
> > Actually I made some changes for OV02A V9, according to this comment.
> > Could you help review that change? Thanks.
> 
> Sure, I will take a look.
> 

Thanks.
Sorry, I just wanna make sure the change is okay for next release.
May we use the check like OV02A V9 did?
ret = v4l2_async_register_subdev(&dw9768->sd);
if (ret < 0) {
	if (!pm_runtime_enabled(dev))
		dw9768_runtime_suspend(dev);
	goto entity_cleanup;
}

> Best regards,
> Tomasz

Re: [V6, 2/2] media: i2c: dw9768: Add DW9768 VCM driver

[Sakari Ailus]

On Thu, Jun 04, 2020 at 10:33:38AM +0800, Dongchun Zhu wrote:
> Hi Tomasz,
> 
> On Mon, 2020-06-01 at 20:47 +0200, Tomasz Figa wrote:
> > On Wed, May 27, 2020 at 11:03 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> > >
> > > Hi Tomasz,
> > >
> > > On Mon, 2020-05-25 at 13:45 +0200, Tomasz Figa wrote:
> > > > On Fri, May 22, 2020 at 11:27 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> > > > >
> > > > > Hi Tomasz,
> > > > >
> > > > > Thanks for the review. My replies are as below.
> > > > >
> > > > > On Thu, 2020-05-21 at 19:51 +0000, Tomasz Figa wrote:
> > > > > > Hi Dongchun, Sakari,
> > > > > >
> > > > > > On Mon, May 18, 2020 at 09:27:31PM +0800, Dongchun Zhu wrote:
> > > > [snip]
> > > > > > > +   pm_runtime_enable(dev);
> > > > > > > +   if (!pm_runtime_enabled(dev)) {
> > > > > > > +           ret = dw9768_runtime_resume(dev);
> > > > > > > +           if (ret < 0) {
> > > > > > > +                   dev_err(dev, "failed to power on: %d\n", ret);
> > > > > > > +                   goto entity_cleanup;
> > > > > > > +           }
> > > > > > > +   }
> > > > > > > +
> > > > > > > +   ret = v4l2_async_register_subdev(&dw9768->sd);
> > > > > > > +   if (ret < 0)
> > > > > > > +           goto entity_cleanup;
> > > > > > > +
> > > > > > > +   return 0;
> > > > > > > +
> > > > > > > +entity_cleanup:
> > > > > >
> > > > > > Need to power off if the code above powered on.
> > > > > >
> > > > >
> > > > > Thanks for the reminder.
> > > > > If there is something wrong with runtime PM, actuator is to be powered
> > > > > on via dw9768_runtime_resume() API.
> > > > > When actuator sub-device is powered on completely and async registered
> > > > > successfully, we shall power off it afterwards.
> > > > >
> > > >
> > > > The code above calls dw9768_runtime_resume() if
> > > > !pm_runtime_enabled(dev), but the clean-up code below the
> > > > entity_cleanup label doesn't have the corresponding
> > > > dw9768_runtime_suspend() call.
> > > >
> > >
> > > Did you mean the 'entity_cleanup' after v4l2_async_register_subdev()?
> > 
> > Yes.
> > 
> > > Actually I made some changes for OV02A V9, according to this comment.
> > > Could you help review that change? Thanks.
> > 
> > Sure, I will take a look.
> > 
> 
> Thanks.
> Sorry, I just wanna make sure the change is okay for next release.
> May we use the check like OV02A V9 did?
> ret = v4l2_async_register_subdev(&dw9768->sd);
> if (ret < 0) {
> 	if (!pm_runtime_enabled(dev))
> 		dw9768_runtime_suspend(dev);

Please do this part where you're jumping to, if possible.

> 	goto entity_cleanup;
> }
> 

-- 
Sakari Ailus

Re: [V6, 2/2] media: i2c: dw9768: Add DW9768 VCM driver

[Dongchun Zhu]

Hi Sakari,

On Thu, 2020-06-04 at 11:10 +0300, Sakari Ailus wrote:
> On Thu, Jun 04, 2020 at 10:33:38AM +0800, Dongchun Zhu wrote:
> > Hi Tomasz,
> > 
> > On Mon, 2020-06-01 at 20:47 +0200, Tomasz Figa wrote:
> > > On Wed, May 27, 2020 at 11:03 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> > > >
> > > > Hi Tomasz,
> > > >
> > > > On Mon, 2020-05-25 at 13:45 +0200, Tomasz Figa wrote:
> > > > > On Fri, May 22, 2020 at 11:27 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> > > > > >
> > > > > > Hi Tomasz,
> > > > > >
> > > > > > Thanks for the review. My replies are as below.
> > > > > >
> > > > > > On Thu, 2020-05-21 at 19:51 +0000, Tomasz Figa wrote:
> > > > > > > Hi Dongchun, Sakari,
> > > > > > >
> > > > > > > On Mon, May 18, 2020 at 09:27:31PM +0800, Dongchun Zhu wrote:
> > > > > [snip]
> > > > > > > > +   pm_runtime_enable(dev);
> > > > > > > > +   if (!pm_runtime_enabled(dev)) {
> > > > > > > > +           ret = dw9768_runtime_resume(dev);
> > > > > > > > +           if (ret < 0) {
> > > > > > > > +                   dev_err(dev, "failed to power on: %d\n", ret);
> > > > > > > > +                   goto entity_cleanup;
> > > > > > > > +           }
> > > > > > > > +   }
> > > > > > > > +
> > > > > > > > +   ret = v4l2_async_register_subdev(&dw9768->sd);
> > > > > > > > +   if (ret < 0)
> > > > > > > > +           goto entity_cleanup;
> > > > > > > > +
> > > > > > > > +   return 0;
> > > > > > > > +
> > > > > > > > +entity_cleanup:
> > > > > > >
> > > > > > > Need to power off if the code above powered on.
> > > > > > >
> > > > > >
> > > > > > Thanks for the reminder.
> > > > > > If there is something wrong with runtime PM, actuator is to be powered
> > > > > > on via dw9768_runtime_resume() API.
> > > > > > When actuator sub-device is powered on completely and async registered
> > > > > > successfully, we shall power off it afterwards.
> > > > > >
> > > > >
> > > > > The code above calls dw9768_runtime_resume() if
> > > > > !pm_runtime_enabled(dev), but the clean-up code below the
> > > > > entity_cleanup label doesn't have the corresponding
> > > > > dw9768_runtime_suspend() call.
> > > > >
> > > >
> > > > Did you mean the 'entity_cleanup' after v4l2_async_register_subdev()?
> > > 
> > > Yes.
> > > 
> > > > Actually I made some changes for OV02A V9, according to this comment.
> > > > Could you help review that change? Thanks.
> > > 
> > > Sure, I will take a look.
> > > 
> > 
> > Thanks.
> > Sorry, I just wanna make sure the change is okay for next release.
> > May we use the check like OV02A V9 did?
> > ret = v4l2_async_register_subdev(&dw9768->sd);
> > if (ret < 0) {
> > 	if (!pm_runtime_enabled(dev))
> > 		dw9768_runtime_suspend(dev);
> 
> Please do this part where you're jumping to, if possible.
> 

Fine.
Fixed in next release.

> > 	goto entity_cleanup;
> > }
> > 
>