Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
* [V5, 0/2] media: i2c: Add support for DW9768 VCM driver
@ 2020-05-02 16:17 Dongchun Zhu
  2020-05-02 16:17 ` [V5, 1/2] media: dt-bindings: media: i2c: Document DW9768 bindings Dongchun Zhu
  2020-05-02 16:17 ` [V5, 2/2] media: i2c: dw9768: Add DW9768 VCM driver Dongchun Zhu
  0 siblings, 2 replies; 17+ messages in thread
From: Dongchun Zhu @ 2020-05-02 16:17 UTC (permalink / raw)
  To: linus.walleij, bgolaszewski, mchehab, andriy.shevchenko, robh+dt,
	mark.rutland, sakari.ailus, drinkcat, tfiga, matthias.bgg,
	bingbu.cao
  Cc: srv_heupstream, linux-mediatek, linux-arm-kernel, sj.huang,
	linux-media, devicetree, louis.kuo, shengnan.wang, dongchun.zhu

Hello,

This series adds DT bindings in YAML and V4L2 sub-device driver for DW9768
lens voice coil motor, 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 registers 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:
 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/

Changes of v5 are addressing comments from Rob, Andy, Sakari, including:
 - Rebase onto 5.7-rc1
 - Refine DT bindings in YAML
 - Remove the condition of IS_ENABLED(CONFIG_PM) as the driver depends on PM
 - Reverse the order of enabling RPM and registering the async subdev

Please 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        |  60 +++
 MAINTAINERS                                        |   8 +
 drivers/media/i2c/Kconfig                          |  11 +
 drivers/media/i2c/Makefile                         |   1 +
 drivers/media/i2c/dw9768.c                         | 440 +++++++++++++++++++++
 5 files changed, 520 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
 create mode 100644 drivers/media/i2c/dw9768.c

-- 
2.9.2

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [V5, 1/2] media: dt-bindings: media: i2c: Document DW9768 bindings
  2020-05-02 16:17 [V5, 0/2] media: i2c: Add support for DW9768 VCM driver Dongchun Zhu
@ 2020-05-02 16:17 ` Dongchun Zhu
  2020-05-05 19:15   ` Rob Herring
  2020-05-02 16:17 ` [V5, 2/2] media: i2c: dw9768: Add DW9768 VCM driver Dongchun Zhu
  1 sibling, 1 reply; 17+ messages in thread
From: Dongchun Zhu @ 2020-05-02 16:17 UTC (permalink / raw)
  To: linus.walleij, bgolaszewski, mchehab, andriy.shevchenko, robh+dt,
	mark.rutland, sakari.ailus, drinkcat, tfiga, matthias.bgg,
	bingbu.cao
  Cc: srv_heupstream, linux-mediatek, linux-arm-kernel, sj.huang,
	linux-media, devicetree, louis.kuo, shengnan.wang, dongchun.zhu

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

Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
---
 .../bindings/media/i2c/dongwoon,dw9768.yaml        | 60 ++++++++++++++++++++++
 MAINTAINERS                                        |  7 +++
 2 files changed, 67 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..8dec22d
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
@@ -0,0 +1,60 @@
+# 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:
+    const: dongwoon,dw9768
+
+  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.
+
+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>;
+        };
+    };
+
+...
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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [V5, 2/2] media: i2c: dw9768: Add DW9768 VCM driver
  2020-05-02 16:17 [V5, 0/2] media: i2c: Add support for DW9768 VCM driver Dongchun Zhu
  2020-05-02 16:17 ` [V5, 1/2] media: dt-bindings: media: i2c: Document DW9768 bindings Dongchun Zhu
@ 2020-05-02 16:17 ` Dongchun Zhu
  2020-05-06 15:13   ` Sakari Ailus
  1 sibling, 1 reply; 17+ messages in thread
From: Dongchun Zhu @ 2020-05-02 16:17 UTC (permalink / raw)
  To: linus.walleij, bgolaszewski, mchehab, andriy.shevchenko, robh+dt,
	mark.rutland, sakari.ailus, drinkcat, tfiga, matthias.bgg,
	bingbu.cao
  Cc: srv_heupstream, linux-mediatek, linux-arm-kernel, sj.huang,
	linux-media, devicetree, louis.kuo, shengnan.wang, 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  |  11 ++
 drivers/media/i2c/Makefile |   1 +
 drivers/media/i2c/dw9768.c | 440 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 453 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..6a3f9da 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -1040,6 +1040,17 @@ 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 && MEDIA_CONTROLLER
+	depends on VIDEO_V4L2_SUBDEV_API
+	depends on PM
+	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..dd68534
--- /dev/null
+++ b/drivers/media/i2c/dw9768.c
@@ -0,0 +1,440 @@
+// 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-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_AAC3_SELECT_DIVIDING_RATE_1	0x41
+
+/*
+ * 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
+#define DW9768_AACT_CNT				0x39
+
+/*
+ * 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;
+};
+
+static inline struct dw9768 *to_dw9768(struct v4l2_ctrl *ctrl)
+{
+	return container_of(ctrl->handler, struct dw9768, ctrls);
+}
+
+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 const struct regval_list dw9768_init_regs[] = {
+	{DW9768_RING_PD_CONTROL_REG, DW9768_AAC_MODE_EN},
+	{DW9768_AAC_PRESC_REG, DW9768_AAC3_SELECT_DIVIDING_RATE_1},
+	{DW9768_AAC_TIME_REG, DW9768_AACT_CNT},
+};
+
+static int dw9768_write_array(struct dw9768 *dw9768,
+			      const struct regval_list *vals, size_t len)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
+	unsigned int i;
+	int ret;
+
+	for (i = 0; i < len; i++) {
+		ret = i2c_smbus_write_byte_data(client, vals[i].reg_num,
+						vals[i].value);
+		if (ret < 0)
+			return ret;
+	}
+	return 0;
+}
+
+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);
+
+	ret = dw9768_write_array(dw9768, dw9768_init_regs,
+				 ARRAY_SIZE(dw9768_init_regs));
+	if (ret)
+		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 __maybe_unused 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 __maybe_unused 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 = to_dw9768(ctrl);
+
+	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 i;
+	int ret;
+
+	dw9768 = devm_kzalloc(dev, sizeof(*dw9768), GFP_KERNEL);
+	if (!dw9768)
+		return -ENOMEM;
+
+	v4l2_i2c_subdev_init(&dw9768->sd, client, &dw9768_ops);
+
+	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" },
+	{}
+};
+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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [V5, 1/2] media: dt-bindings: media: i2c: Document DW9768 bindings
  2020-05-02 16:17 ` [V5, 1/2] media: dt-bindings: media: i2c: Document DW9768 bindings Dongchun Zhu
@ 2020-05-05 19:15   ` Rob Herring
  0 siblings, 0 replies; 17+ messages in thread
From: Rob Herring @ 2020-05-05 19:15 UTC (permalink / raw)
  To: Dongchun Zhu
  Cc: linus.walleij, bgolaszewski, mchehab, andriy.shevchenko, robh+dt,
	mark.rutland, sakari.ailus, drinkcat, tfiga, matthias.bgg,
	bingbu.cao, devicetree, srv_heupstream, shengnan.wang, sj.huang,
	linux-mediatek, dongchun.zhu, louis.kuo, linux-arm-kernel,
	linux-media

On Sun, 3 May 2020 00:17:26 +0800, Dongchun Zhu wrote:
> Add DeviceTree binding documentation for Dongwoon Anatech DW9768 voice
> coil actuator.
> 
> Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> ---
>  .../bindings/media/i2c/dongwoon,dw9768.yaml        | 60 ++++++++++++++++++++++
>  MAINTAINERS                                        |  7 +++
>  2 files changed, 67 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [V5, 2/2] media: i2c: dw9768: Add DW9768 VCM driver
  2020-05-02 16:17 ` [V5, 2/2] media: i2c: dw9768: Add DW9768 VCM driver Dongchun Zhu
@ 2020-05-06 15:13   ` Sakari Ailus
  2020-05-07 12:45     ` Dongchun Zhu
  0 siblings, 1 reply; 17+ messages in thread
From: Sakari Ailus @ 2020-05-06 15:13 UTC (permalink / raw)
  To: Dongchun Zhu
  Cc: linus.walleij, bgolaszewski, mchehab, andriy.shevchenko, robh+dt,
	mark.rutland, drinkcat, tfiga, matthias.bgg, bingbu.cao,
	srv_heupstream, linux-mediatek, linux-arm-kernel, sj.huang,
	linux-media, devicetree, louis.kuo, shengnan.wang

Hi Dongchun,

On Sun, May 03, 2020 at 12:17:27AM +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  |  11 ++
>  drivers/media/i2c/Makefile |   1 +
>  drivers/media/i2c/dw9768.c | 440 +++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 453 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..6a3f9da 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -1040,6 +1040,17 @@ 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 && MEDIA_CONTROLLER
> +	depends on VIDEO_V4L2_SUBDEV_API

Please check how this works in the media tree master branch now --- it's
largely select based.

In general the patch seems fine to me, but please see the other comments
below, too.

> +	depends on PM
> +	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..dd68534
> --- /dev/null
> +++ b/drivers/media/i2c/dw9768.c
> @@ -0,0 +1,440 @@
> +// 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-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_AAC3_SELECT_DIVIDING_RATE_1	0x41

I guess we can start with these values. But I can't think of another option
than putting them into DT if there are differences between what hardware
platforms require.

> +
> +/*
> + * 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
> +#define DW9768_AACT_CNT				0x39
> +
> +/*
> + * 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;
> +};
> +
> +static inline struct dw9768 *to_dw9768(struct v4l2_ctrl *ctrl)
> +{
> +	return container_of(ctrl->handler, struct dw9768, ctrls);
> +}

This is used in a single place. I'd just use container_of() directly there.

> +
> +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 const struct regval_list dw9768_init_regs[] = {
> +	{DW9768_RING_PD_CONTROL_REG, DW9768_AAC_MODE_EN},
> +	{DW9768_AAC_PRESC_REG, DW9768_AAC3_SELECT_DIVIDING_RATE_1},
> +	{DW9768_AAC_TIME_REG, DW9768_AACT_CNT},
> +};
> +
> +static int dw9768_write_array(struct dw9768 *dw9768,
> +			      const struct regval_list *vals, size_t len)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
> +	unsigned int i;
> +	int ret;
> +
> +	for (i = 0; i < len; i++) {
> +		ret = i2c_smbus_write_byte_data(client, vals[i].reg_num,
> +						vals[i].value);
> +		if (ret < 0)
> +			return ret;
> +	}
> +	return 0;
> +}
> +
> +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);
> +
> +	ret = dw9768_write_array(dw9768, dw9768_init_regs,
> +				 ARRAY_SIZE(dw9768_init_regs));
> +	if (ret)
> +		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 __maybe_unused 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 __maybe_unused 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 = to_dw9768(ctrl);
> +
> +	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 i;
> +	int ret;
> +
> +	dw9768 = devm_kzalloc(dev, sizeof(*dw9768), GFP_KERNEL);
> +	if (!dw9768)
> +		return -ENOMEM;
> +
> +	v4l2_i2c_subdev_init(&dw9768->sd, client, &dw9768_ops);
> +
> +	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" },
> +	{}
> +};
> +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");

-- 
Kind regards,

Sakari Ailus

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [V5, 2/2] media: i2c: dw9768: Add DW9768 VCM driver
  2020-05-06 15:13   ` Sakari Ailus
@ 2020-05-07 12:45     ` Dongchun Zhu
  2020-05-07 13:12       ` Sakari Ailus
  0 siblings, 1 reply; 17+ messages in thread
From: Dongchun Zhu @ 2020-05-07 12:45 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: linus.walleij, bgolaszewski, mchehab, andriy.shevchenko, robh+dt,
	mark.rutland, drinkcat, tfiga, matthias.bgg, bingbu.cao,
	srv_heupstream, linux-mediatek, linux-arm-kernel, sj.huang,
	linux-media, devicetree, louis.kuo, shengnan.wang

Hi Sakari,

Thanks for the review.

On Wed, 2020-05-06 at 18:13 +0300, Sakari Ailus wrote:
> Hi Dongchun,
> 
> On Sun, May 03, 2020 at 12:17:27AM +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  |  11 ++
> >  drivers/media/i2c/Makefile |   1 +
> >  drivers/media/i2c/dw9768.c | 440 +++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 453 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..6a3f9da 100644
> > --- a/drivers/media/i2c/Kconfig
> > +++ b/drivers/media/i2c/Kconfig
> > @@ -1040,6 +1040,17 @@ 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 && MEDIA_CONTROLLER
> > +	depends on VIDEO_V4L2_SUBDEV_API
> 
> Please check how this works in the media tree master branch now --- it's
> largely select based.
> 

The actuator driver uses some structures that require the
VIDEO_V4L2_SUBDEV_API code, so here we add VIDEO_V4L2_SUBDEV_API
dependency to avoid possible build error when it's not enabled.

> In general the patch seems fine to me, but please see the other comments
> below, too.
> 
> > +	depends on PM
> > +	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..dd68534
> > --- /dev/null
> > +++ b/drivers/media/i2c/dw9768.c
> > @@ -0,0 +1,440 @@
> > +// 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-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_AAC3_SELECT_DIVIDING_RATE_1	0x41
> 
> I guess we can start with these values. But I can't think of another option
> than putting them into DT if there are differences between what hardware
> platforms require.
> 

Let's have a discussion about this.
Now these non-default register settings represent one AAC operation
mode, this is one option and works for a given lens or a module.
If sometime in the future hardware platforms require another different
settings, then DT properties may need to be created.

> > +
> > +/*
> > + * 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
> > +#define DW9768_AACT_CNT				0x39
> > +
> > +/*
> > + * 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;
> > +};
> > +
> > +static inline struct dw9768 *to_dw9768(struct v4l2_ctrl *ctrl)
> > +{
> > +	return container_of(ctrl->handler, struct dw9768, ctrls);
> > +}
> 
> This is used in a single place. I'd just use container_of() directly there.
> 

Thanks for the reminder.
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 const struct regval_list dw9768_init_regs[] = {
> > +	{DW9768_RING_PD_CONTROL_REG, DW9768_AAC_MODE_EN},
> > +	{DW9768_AAC_PRESC_REG, DW9768_AAC3_SELECT_DIVIDING_RATE_1},
> > +	{DW9768_AAC_TIME_REG, DW9768_AACT_CNT},
> > +};
> > +
> > +static int dw9768_write_array(struct dw9768 *dw9768,
> > +			      const struct regval_list *vals, size_t len)
> > +{
> > +	struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	for (i = 0; i < len; i++) {
> > +		ret = i2c_smbus_write_byte_data(client, vals[i].reg_num,
> > +						vals[i].value);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +	return 0;
> > +}
> > +
> > +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);
> > +
> > +	ret = dw9768_write_array(dw9768, dw9768_init_regs,
> > +				 ARRAY_SIZE(dw9768_init_regs));
> > +	if (ret)
> > +		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 __maybe_unused 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 __maybe_unused 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 = to_dw9768(ctrl);
> > +
> > +	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 i;
> > +	int ret;
> > +
> > +	dw9768 = devm_kzalloc(dev, sizeof(*dw9768), GFP_KERNEL);
> > +	if (!dw9768)
> > +		return -ENOMEM;
> > +
> > +	v4l2_i2c_subdev_init(&dw9768->sd, client, &dw9768_ops);
> > +
> > +	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" },
> > +	{}
> > +};
> > +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");
> 


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [V5, 2/2] media: i2c: dw9768: Add DW9768 VCM driver
  2020-05-07 12:45     ` Dongchun Zhu
@ 2020-05-07 13:12       ` Sakari Ailus
  2020-05-07 13:46         ` Tomasz Figa
  0 siblings, 1 reply; 17+ messages in thread
From: Sakari Ailus @ 2020-05-07 13:12 UTC (permalink / raw)
  To: Dongchun Zhu
  Cc: linus.walleij, bgolaszewski, mchehab, andriy.shevchenko, robh+dt,
	mark.rutland, drinkcat, tfiga, matthias.bgg, bingbu.cao,
	srv_heupstream, linux-mediatek, linux-arm-kernel, sj.huang,
	linux-media, devicetree, louis.kuo, shengnan.wang

HI Dongchun,

On Thu, May 07, 2020 at 08:45:24PM +0800, Dongchun Zhu wrote:
> Hi Sakari,
> 
> Thanks for the review.
> 
> On Wed, 2020-05-06 at 18:13 +0300, Sakari Ailus wrote:
> > Hi Dongchun,
> > 
> > On Sun, May 03, 2020 at 12:17:27AM +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  |  11 ++
> > >  drivers/media/i2c/Makefile |   1 +
> > >  drivers/media/i2c/dw9768.c | 440 +++++++++++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 453 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..6a3f9da 100644
> > > --- a/drivers/media/i2c/Kconfig
> > > +++ b/drivers/media/i2c/Kconfig
> > > @@ -1040,6 +1040,17 @@ 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 && MEDIA_CONTROLLER
> > > +	depends on VIDEO_V4L2_SUBDEV_API
> > 
> > Please check how this works in the media tree master branch now --- it's
> > largely select based.
> > 
> 
> The actuator driver uses some structures that require the
> VIDEO_V4L2_SUBDEV_API code, so here we add VIDEO_V4L2_SUBDEV_API
> dependency to avoid possible build error when it's not enabled.

Please make sure this works with current media tree master. Right now it
does not.

> 
> > In general the patch seems fine to me, but please see the other comments
> > below, too.
> > 
> > > +	depends on PM
> > > +	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..dd68534
> > > --- /dev/null
> > > +++ b/drivers/media/i2c/dw9768.c
> > > @@ -0,0 +1,440 @@
> > > +// 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-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_AAC3_SELECT_DIVIDING_RATE_1	0x41
> > 
> > I guess we can start with these values. But I can't think of another option
> > than putting them into DT if there are differences between what hardware
> > platforms require.
> > 
> 
> Let's have a discussion about this.
> Now these non-default register settings represent one AAC operation
> mode, this is one option and works for a given lens or a module.
> If sometime in the future hardware platforms require another different
> settings, then DT properties may need to be created.

If these values indeed are specific to a given lens (and presumably also a
spring), then I'd put them to DT right now --- we don't have drivers for
these components the drivers of which could hold this information, nor it
makes sense to add them just for that.

> 
> > > +
> > > +/*
> > > + * 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
> > > +#define DW9768_AACT_CNT				0x39
> > > +
> > > +/*
> > > + * 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;
> > > +};
> > > +
> > > +static inline struct dw9768 *to_dw9768(struct v4l2_ctrl *ctrl)
> > > +{
> > > +	return container_of(ctrl->handler, struct dw9768, ctrls);
> > > +}
> > 
> > This is used in a single place. I'd just use container_of() directly there.
> > 
> 
> Thanks for the reminder.
> 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 const struct regval_list dw9768_init_regs[] = {
> > > +	{DW9768_RING_PD_CONTROL_REG, DW9768_AAC_MODE_EN},
> > > +	{DW9768_AAC_PRESC_REG, DW9768_AAC3_SELECT_DIVIDING_RATE_1},
> > > +	{DW9768_AAC_TIME_REG, DW9768_AACT_CNT},
> > > +};
> > > +
> > > +static int dw9768_write_array(struct dw9768 *dw9768,
> > > +			      const struct regval_list *vals, size_t len)
> > > +{
> > > +	struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
> > > +	unsigned int i;
> > > +	int ret;
> > > +
> > > +	for (i = 0; i < len; i++) {
> > > +		ret = i2c_smbus_write_byte_data(client, vals[i].reg_num,
> > > +						vals[i].value);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +	}
> > > +	return 0;
> > > +}
> > > +
> > > +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);
> > > +
> > > +	ret = dw9768_write_array(dw9768, dw9768_init_regs,
> > > +				 ARRAY_SIZE(dw9768_init_regs));
> > > +	if (ret)
> > > +		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 __maybe_unused 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 __maybe_unused 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 = to_dw9768(ctrl);
> > > +
> > > +	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 i;
> > > +	int ret;
> > > +
> > > +	dw9768 = devm_kzalloc(dev, sizeof(*dw9768), GFP_KERNEL);
> > > +	if (!dw9768)
> > > +		return -ENOMEM;
> > > +
> > > +	v4l2_i2c_subdev_init(&dw9768->sd, client, &dw9768_ops);
> > > +
> > > +	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" },
> > > +	{}
> > > +};
> > > +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");
> > 
> 

-- 
Sakari Ailus

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [V5, 2/2] media: i2c: dw9768: Add DW9768 VCM driver
  2020-05-07 13:12       ` Sakari Ailus
@ 2020-05-07 13:46         ` Tomasz Figa
  2020-05-07 14:00           ` Sakari Ailus
  2020-05-08  3:08           ` Dongchun Zhu
  0 siblings, 2 replies; 17+ messages in thread
From: Tomasz Figa @ 2020-05-07 13:46 UTC (permalink / raw)
  To: Sakari Ailus, Dongchun Zhu
  Cc: Linus Walleij, Bartosz Golaszewski, Mauro Carvalho Chehab,
	Andy Shevchenko, Rob Herring, Mark Rutland, Nicolas Boichat,
	Matthias Brugger, Cao Bing Bu, srv_heupstream,
	moderated list:ARM/Mediatek SoC support,
	list@263.net:IOMMU DRIVERS
	<iommu@lists.linux-foundation.org>,
	Joerg Roedel <joro@8bytes.org>,,
	Sj Huang, Linux Media Mailing List, linux-devicetree, Louis Kuo,
	Shengnan Wang (王圣男)

Hi Sakari, Dongchun,

On Thu, May 7, 2020 at 3:12 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> HI Dongchun,
>
> On Thu, May 07, 2020 at 08:45:24PM +0800, Dongchun Zhu wrote:
> > Hi Sakari,
> >
> > Thanks for the review.
> >
> > On Wed, 2020-05-06 at 18:13 +0300, Sakari Ailus wrote:
> > > Hi Dongchun,
> > >
> > > On Sun, May 03, 2020 at 12:17:27AM +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  |  11 ++
> > > >  drivers/media/i2c/Makefile |   1 +
> > > >  drivers/media/i2c/dw9768.c | 440 +++++++++++++++++++++++++++++++++++++++++++++
> > > >  4 files changed, 453 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..6a3f9da 100644
> > > > --- a/drivers/media/i2c/Kconfig
> > > > +++ b/drivers/media/i2c/Kconfig
> > > > @@ -1040,6 +1040,17 @@ 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 && MEDIA_CONTROLLER
> > > > + depends on VIDEO_V4L2_SUBDEV_API
> > >
> > > Please check how this works in the media tree master branch now --- it's
> > > largely select based.
> > >
> >
> > The actuator driver uses some structures that require the
> > VIDEO_V4L2_SUBDEV_API code, so here we add VIDEO_V4L2_SUBDEV_API
> > dependency to avoid possible build error when it's not enabled.
>
> Please make sure this works with current media tree master. Right now it
> does not.
>

Dongchun, as Sakari said, please make sure to base the patches on the
master branch of the media tree.
(https://git.linuxtv.org/media_tree.git/). The approach for Kconfig
dependency selection there seems to have changed recently.

> >
> > > In general the patch seems fine to me, but please see the other comments
> > > below, too.
> > >
> > > > + depends on PM
> > > > + 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..dd68534
> > > > --- /dev/null
> > > > +++ b/drivers/media/i2c/dw9768.c
> > > > @@ -0,0 +1,440 @@
> > > > +// 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-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_AAC3_SELECT_DIVIDING_RATE_1       0x41
> > >
> > > I guess we can start with these values. But I can't think of another option
> > > than putting them into DT if there are differences between what hardware
> > > platforms require.
> > >
> >
> > Let's have a discussion about this.
> > Now these non-default register settings represent one AAC operation
> > mode, this is one option and works for a given lens or a module.
> > If sometime in the future hardware platforms require another different
> > settings, then DT properties may need to be created.
>
> If these values indeed are specific to a given lens (and presumably also a
> spring), then I'd put them to DT right now --- we don't have drivers for
> these components the drivers of which could hold this information, nor it
> makes sense to add them just for that.
>

I tend to stay on the conservative side and only add DT properties
once there is really a need to do so. Right now we haven't seen any
system which would use different values of these parameters.

Best regards,
Tomasz

> >
> > > > +
> > > > +/*
> > > > + * 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
> > > > +#define DW9768_AACT_CNT                          0x39
> > > > +
> > > > +/*
> > > > + * 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;
> > > > +};
> > > > +
> > > > +static inline struct dw9768 *to_dw9768(struct v4l2_ctrl *ctrl)
> > > > +{
> > > > + return container_of(ctrl->handler, struct dw9768, ctrls);
> > > > +}
> > >
> > > This is used in a single place. I'd just use container_of() directly there.
> > >
> >
> > Thanks for the reminder.
> > 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 const struct regval_list dw9768_init_regs[] = {
> > > > + {DW9768_RING_PD_CONTROL_REG, DW9768_AAC_MODE_EN},
> > > > + {DW9768_AAC_PRESC_REG, DW9768_AAC3_SELECT_DIVIDING_RATE_1},
> > > > + {DW9768_AAC_TIME_REG, DW9768_AACT_CNT},
> > > > +};
> > > > +
> > > > +static int dw9768_write_array(struct dw9768 *dw9768,
> > > > +                       const struct regval_list *vals, size_t len)
> > > > +{
> > > > + struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
> > > > + unsigned int i;
> > > > + int ret;
> > > > +
> > > > + for (i = 0; i < len; i++) {
> > > > +         ret = i2c_smbus_write_byte_data(client, vals[i].reg_num,
> > > > +                                         vals[i].value);
> > > > +         if (ret < 0)
> > > > +                 return ret;
> > > > + }
> > > > + return 0;
> > > > +}
> > > > +
> > > > +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);
> > > > +
> > > > + ret = dw9768_write_array(dw9768, dw9768_init_regs,
> > > > +                          ARRAY_SIZE(dw9768_init_regs));
> > > > + if (ret)
> > > > +         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 __maybe_unused 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 __maybe_unused 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 = to_dw9768(ctrl);
> > > > +
> > > > + 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 i;
> > > > + int ret;
> > > > +
> > > > + dw9768 = devm_kzalloc(dev, sizeof(*dw9768), GFP_KERNEL);
> > > > + if (!dw9768)
> > > > +         return -ENOMEM;
> > > > +
> > > > + v4l2_i2c_subdev_init(&dw9768->sd, client, &dw9768_ops);
> > > > +
> > > > + 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" },
> > > > + {}
> > > > +};
> > > > +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");
> > >
> >
>
> --
> Sakari Ailus

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [V5, 2/2] media: i2c: dw9768: Add DW9768 VCM driver
  2020-05-07 13:46         ` Tomasz Figa
@ 2020-05-07 14:00           ` Sakari Ailus
  2020-05-08  3:27             ` Dongchun Zhu
  2020-05-08  3:08           ` Dongchun Zhu
  1 sibling, 1 reply; 17+ messages in thread
From: Sakari Ailus @ 2020-05-07 14:00 UTC (permalink / raw)
  To: Tomasz Figa
  Cc: Dongchun Zhu, Linus Walleij, Bartosz Golaszewski,
	Mauro Carvalho Chehab, Andy Shevchenko, Rob Herring,
	Mark Rutland, Nicolas Boichat, Matthias Brugger, Cao Bing Bu,
	srv_heupstream, moderated list:ARM/Mediatek SoC support,
	list@263.net:IOMMU DRIVERS
	<iommu@lists.linux-foundation.org>,
	Joerg Roedel <joro@8bytes.org>,,
	Sj Huang, Linux Media Mailing List, linux-devicetree, Louis Kuo,
	Shengnan Wang (王圣男)

Hi Tomasz,

On Thu, May 07, 2020 at 03:46:31PM +0200, Tomasz Figa wrote:
> Hi Sakari, Dongchun,
> 
> On Thu, May 7, 2020 at 3:12 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > HI Dongchun,
> >
> > On Thu, May 07, 2020 at 08:45:24PM +0800, Dongchun Zhu wrote:
> > > Hi Sakari,
> > >
> > > Thanks for the review.
> > >
> > > On Wed, 2020-05-06 at 18:13 +0300, Sakari Ailus wrote:
> > > > Hi Dongchun,
> > > >
> > > > On Sun, May 03, 2020 at 12:17:27AM +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  |  11 ++
> > > > >  drivers/media/i2c/Makefile |   1 +
> > > > >  drivers/media/i2c/dw9768.c | 440 +++++++++++++++++++++++++++++++++++++++++++++
> > > > >  4 files changed, 453 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..6a3f9da 100644
> > > > > --- a/drivers/media/i2c/Kconfig
> > > > > +++ b/drivers/media/i2c/Kconfig
> > > > > @@ -1040,6 +1040,17 @@ 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 && MEDIA_CONTROLLER
> > > > > + depends on VIDEO_V4L2_SUBDEV_API
> > > >
> > > > Please check how this works in the media tree master branch now --- it's
> > > > largely select based.
> > > >
> > >
> > > The actuator driver uses some structures that require the
> > > VIDEO_V4L2_SUBDEV_API code, so here we add VIDEO_V4L2_SUBDEV_API
> > > dependency to avoid possible build error when it's not enabled.
> >
> > Please make sure this works with current media tree master. Right now it
> > does not.
> >
> 
> Dongchun, as Sakari said, please make sure to base the patches on the
> master branch of the media tree.
> (https://git.linuxtv.org/media_tree.git/). The approach for Kconfig
> dependency selection there seems to have changed recently.
> 
> > >
> > > > In general the patch seems fine to me, but please see the other comments
> > > > below, too.
> > > >
> > > > > + depends on PM
> > > > > + 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..dd68534
> > > > > --- /dev/null
> > > > > +++ b/drivers/media/i2c/dw9768.c
> > > > > @@ -0,0 +1,440 @@
> > > > > +// 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-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_AAC3_SELECT_DIVIDING_RATE_1       0x41
> > > >
> > > > I guess we can start with these values. But I can't think of another option
> > > > than putting them into DT if there are differences between what hardware
> > > > platforms require.
> > > >
> > >
> > > Let's have a discussion about this.
> > > Now these non-default register settings represent one AAC operation
> > > mode, this is one option and works for a given lens or a module.
> > > If sometime in the future hardware platforms require another different
> > > settings, then DT properties may need to be created.
> >
> > If these values indeed are specific to a given lens (and presumably also a
> > spring), then I'd put them to DT right now --- we don't have drivers for
> > these components the drivers of which could hold this information, nor it
> > makes sense to add them just for that.
> >
> 
> I tend to stay on the conservative side and only add DT properties
> once there is really a need to do so. Right now we haven't seen any
> system which would use different values of these parameters.

I think it's also conservative to put things that are system specific to
DT. :-)

In practice we haven't put lens specific parameters to DT before, but
that's because 1) devices that need them are old, and so are the drivers
and the matter was not recognised at the time they were merged and 2) a lot
of devices these days don't have such configuration registers.

That said, I don't have a strong opinion on this one, but I think putting
this to DT would be a safer bet in the long run as this is specific to the
board, not the device itself.

-- 
Regards,

Sakari Ailus

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [V5, 2/2] media: i2c: dw9768: Add DW9768 VCM driver
  2020-05-07 13:46         ` Tomasz Figa
  2020-05-07 14:00           ` Sakari Ailus
@ 2020-05-08  3:08           ` Dongchun Zhu
  2020-05-08 21:13             ` Sakari Ailus
  1 sibling, 1 reply; 17+ messages in thread
From: Dongchun Zhu @ 2020-05-08  3:08 UTC (permalink / raw)
  To: Tomasz Figa
  Cc: Sakari Ailus, Linus Walleij, Bartosz Golaszewski,
	Mauro Carvalho Chehab, Andy Shevchenko, Rob Herring,
	Mark Rutland, Nicolas Boichat, Matthias Brugger, Cao Bing Bu,
	srv_heupstream, moderated list:ARM/Mediatek SoC support,
	list@263.net:IOMMU DRIVERS
	<iommu@lists.linux-foundation.org>,
	Joerg  Roedel <joro@8bytes.org>,,
	Sj Huang, Linux Media Mailing List, linux-devicetree, Louis Kuo,
	Shengnan Wang (王圣男)

Hi Sakari, Tomasz,

Thanks for the review.

On Thu, 2020-05-07 at 15:46 +0200, Tomasz Figa wrote:
> Hi Sakari, Dongchun,
> 
> On Thu, May 7, 2020 at 3:12 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > HI Dongchun,
> >
> > On Thu, May 07, 2020 at 08:45:24PM +0800, Dongchun Zhu wrote:
> > > Hi Sakari,
> > >
> > > Thanks for the review.
> > >
> > > On Wed, 2020-05-06 at 18:13 +0300, Sakari Ailus wrote:
> > > > Hi Dongchun,
> > > >
> > > > On Sun, May 03, 2020 at 12:17:27AM +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  |  11 ++
> > > > >  drivers/media/i2c/Makefile |   1 +
> > > > >  drivers/media/i2c/dw9768.c | 440 +++++++++++++++++++++++++++++++++++++++++++++
> > > > >  4 files changed, 453 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..6a3f9da 100644
> > > > > --- a/drivers/media/i2c/Kconfig
> > > > > +++ b/drivers/media/i2c/Kconfig
> > > > > @@ -1040,6 +1040,17 @@ 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 && MEDIA_CONTROLLER
> > > > > + depends on VIDEO_V4L2_SUBDEV_API
> > > >
> > > > Please check how this works in the media tree master branch now --- it's
> > > > largely select based.
> > > >
> > >
> > > The actuator driver uses some structures that require the
> > > VIDEO_V4L2_SUBDEV_API code, so here we add VIDEO_V4L2_SUBDEV_API
> > > dependency to avoid possible build error when it's not enabled.
> >
> > Please make sure this works with current media tree master. Right now it
> > does not.
> >
> 
> Dongchun, as Sakari said, please make sure to base the patches on the
> master branch of the media tree.
> (https://git.linuxtv.org/media_tree.git/). The approach for Kconfig
> dependency selection there seems to have changed recently.
> 

I searched the patches on the media tree master branch.
It seems that we need to remove the VIDEO_V4L2_SUBDEV_API dependency in
Kconfig, and add #ifdef CONFIG_VIDEO_V4L2_SUBDEV_API to include
v4l2-subdev code.
The change mainly is to make build pass, and don't return ENOTTY if
SUBDEV_API is not set.
Am I right?

> > >
> > > > In general the patch seems fine to me, but please see the other comments
> > > > below, too.
> > > >
> > > > > + depends on PM
> > > > > + 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..dd68534
> > > > > --- /dev/null
> > > > > +++ b/drivers/media/i2c/dw9768.c
> > > > > @@ -0,0 +1,440 @@
> > > > > +// 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-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_AAC3_SELECT_DIVIDING_RATE_1       0x41
> > > >
> > > > I guess we can start with these values. But I can't think of another option
> > > > than putting them into DT if there are differences between what hardware
> > > > platforms require.
> > > >
> > >
> > > Let's have a discussion about this.
> > > Now these non-default register settings represent one AAC operation
> > > mode, this is one option and works for a given lens or a module.
> > > If sometime in the future hardware platforms require another different
> > > settings, then DT properties may need to be created.
> >
> > If these values indeed are specific to a given lens (and presumably also a
> > spring), then I'd put them to DT right now --- we don't have drivers for
> > these components the drivers of which could hold this information, nor it
> > makes sense to add them just for that.
> >
> 
> I tend to stay on the conservative side and only add DT properties
> once there is really a need to do so. Right now we haven't seen any
> system which would use different values of these parameters.
> 
> Best regards,
> Tomasz
> 
> > >
> > > > > +
> > > > > +/*
> > > > > + * 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
> > > > > +#define DW9768_AACT_CNT                          0x39
> > > > > +
> > > > > +/*
> > > > > + * 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;
> > > > > +};
> > > > > +
> > > > > +static inline struct dw9768 *to_dw9768(struct v4l2_ctrl *ctrl)
> > > > > +{
> > > > > + return container_of(ctrl->handler, struct dw9768, ctrls);
> > > > > +}
> > > >
> > > > This is used in a single place. I'd just use container_of() directly there.
> > > >
> > >
> > > Thanks for the reminder.
> > > 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 const struct regval_list dw9768_init_regs[] = {
> > > > > + {DW9768_RING_PD_CONTROL_REG, DW9768_AAC_MODE_EN},
> > > > > + {DW9768_AAC_PRESC_REG, DW9768_AAC3_SELECT_DIVIDING_RATE_1},
> > > > > + {DW9768_AAC_TIME_REG, DW9768_AACT_CNT},
> > > > > +};
> > > > > +
> > > > > +static int dw9768_write_array(struct dw9768 *dw9768,
> > > > > +                       const struct regval_list *vals, size_t len)
> > > > > +{
> > > > > + struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
> > > > > + unsigned int i;
> > > > > + int ret;
> > > > > +
> > > > > + for (i = 0; i < len; i++) {
> > > > > +         ret = i2c_smbus_write_byte_data(client, vals[i].reg_num,
> > > > > +                                         vals[i].value);
> > > > > +         if (ret < 0)
> > > > > +                 return ret;
> > > > > + }
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +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);
> > > > > +
> > > > > + ret = dw9768_write_array(dw9768, dw9768_init_regs,
> > > > > +                          ARRAY_SIZE(dw9768_init_regs));
> > > > > + if (ret)
> > > > > +         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 __maybe_unused 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 __maybe_unused 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 = to_dw9768(ctrl);
> > > > > +
> > > > > + 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 i;
> > > > > + int ret;
> > > > > +
> > > > > + dw9768 = devm_kzalloc(dev, sizeof(*dw9768), GFP_KERNEL);
> > > > > + if (!dw9768)
> > > > > +         return -ENOMEM;
> > > > > +
> > > > > + v4l2_i2c_subdev_init(&dw9768->sd, client, &dw9768_ops);
> > > > > +
> > > > > + 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" },
> > > > > + {}
> > > > > +};
> > > > > +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");
> > > >
> > >
> >
> > --
> > Sakari Ailus


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [V5, 2/2] media: i2c: dw9768: Add DW9768 VCM driver
  2020-05-07 14:00           ` Sakari Ailus
@ 2020-05-08  3:27             ` Dongchun Zhu
  0 siblings, 0 replies; 17+ messages in thread
From: Dongchun Zhu @ 2020-05-08  3:27 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Tomasz Figa, Linus Walleij, Bartosz Golaszewski,
	Mauro Carvalho Chehab, dongchun.zhu, Andy Shevchenko,
	Rob Herring, Mark Rutland, Nicolas Boichat, Matthias Brugger,
	Cao Bing Bu, srv_heupstream,
	moderated list:ARM/Mediatek SoC support,
	list@263.net:IOMMU DRIVERS
	<iommu@lists.linux-foundation.org>,
	Joerg  Roedel <joro@8bytes.org>,,
	Sj Huang, Linux Media Mailing List, linux-devicetree, Louis Kuo,
	Shengnan Wang (王圣男)

Hi Sakari,

Thanks for the review.

On Thu, 2020-05-07 at 17:00 +0300, Sakari Ailus wrote:
> Hi Tomasz,
> 
> On Thu, May 07, 2020 at 03:46:31PM +0200, Tomasz Figa wrote:
> > Hi Sakari, Dongchun,
> > 
> > On Thu, May 7, 2020 at 3:12 PM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > >
> > > HI Dongchun,
> > >
> > > On Thu, May 07, 2020 at 08:45:24PM +0800, Dongchun Zhu wrote:
> > > > Hi Sakari,
> > > >
> > > > Thanks for the review.
> > > >
> > > > On Wed, 2020-05-06 at 18:13 +0300, Sakari Ailus wrote:
> > > > > Hi Dongchun,
> > > > >
> > > > > On Sun, May 03, 2020 at 12:17:27AM +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  |  11 ++
> > > > > >  drivers/media/i2c/Makefile |   1 +
> > > > > >  drivers/media/i2c/dw9768.c | 440 +++++++++++++++++++++++++++++++++++++++++++++
> > > > > >  4 files changed, 453 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..6a3f9da 100644
> > > > > > --- a/drivers/media/i2c/Kconfig
> > > > > > +++ b/drivers/media/i2c/Kconfig
> > > > > > @@ -1040,6 +1040,17 @@ 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 && MEDIA_CONTROLLER
> > > > > > + depends on VIDEO_V4L2_SUBDEV_API
> > > > >
> > > > > Please check how this works in the media tree master branch now --- it's
> > > > > largely select based.
> > > > >
> > > >
> > > > The actuator driver uses some structures that require the
> > > > VIDEO_V4L2_SUBDEV_API code, so here we add VIDEO_V4L2_SUBDEV_API
> > > > dependency to avoid possible build error when it's not enabled.
> > >
> > > Please make sure this works with current media tree master. Right now it
> > > does not.
> > >
> > 
> > Dongchun, as Sakari said, please make sure to base the patches on the
> > master branch of the media tree.
> > (https://git.linuxtv.org/media_tree.git/). The approach for Kconfig
> > dependency selection there seems to have changed recently.
> > 
> > > >
> > > > > In general the patch seems fine to me, but please see the other comments
> > > > > below, too.
> > > > >
> > > > > > + depends on PM
> > > > > > + 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..dd68534
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/media/i2c/dw9768.c
> > > > > > @@ -0,0 +1,440 @@
> > > > > > +// 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-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_AAC3_SELECT_DIVIDING_RATE_1       0x41
> > > > >
> > > > > I guess we can start with these values. But I can't think of another option
> > > > > than putting them into DT if there are differences between what hardware
> > > > > platforms require.
> > > > >
> > > >
> > > > Let's have a discussion about this.
> > > > Now these non-default register settings represent one AAC operation
> > > > mode, this is one option and works for a given lens or a module.
> > > > If sometime in the future hardware platforms require another different
> > > > settings, then DT properties may need to be created.
> > >
> > > If these values indeed are specific to a given lens (and presumably also a
> > > spring), then I'd put them to DT right now --- we don't have drivers for
> > > these components the drivers of which could hold this information, nor it
> > > makes sense to add them just for that.
> > >
> > 
> > I tend to stay on the conservative side and only add DT properties
> > once there is really a need to do so. Right now we haven't seen any
> > system which would use different values of these parameters.
> 
> I think it's also conservative to put things that are system specific to
> DT. :-)
> 
> In practice we haven't put lens specific parameters to DT before, but
> that's because 1) devices that need them are old, and so are the drivers
> and the matter was not recognised at the time they were merged and 2) a lot
> of devices these days don't have such configuration registers.
> 
> That said, I don't have a strong opinion on this one, but I think putting
> this to DT would be a safer bet in the long run as this is specific to the
> board, not the device itself.
> 

In theory, lens parameter settings tend to be similar as sensor mode.
Current settings should be common and it is not board specific,
at least for our 2 projects/boards in hand.

In practice, for one set of lens parameters, it would be some
complicated to create DT properties as the driver indeed doesn't use
various values of these parameters.


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [V5, 2/2] media: i2c: dw9768: Add DW9768 VCM driver
  2020-05-08  3:08           ` Dongchun Zhu
@ 2020-05-08 21:13             ` Sakari Ailus
  2020-05-09  2:23               ` Dongchun Zhu
  0 siblings, 1 reply; 17+ messages in thread
From: Sakari Ailus @ 2020-05-08 21:13 UTC (permalink / raw)
  To: Dongchun Zhu
  Cc: Tomasz Figa, Linus Walleij, Bartosz Golaszewski,
	Mauro Carvalho Chehab, Andy Shevchenko, Rob Herring,
	Mark Rutland, Nicolas Boichat, Matthias Brugger, Cao Bing Bu,
	srv_heupstream, moderated list:ARM/Mediatek SoC support,
	list@263.net:IOMMU DRIVERS
	<iommu@lists.linux-foundation.org>,
	Joerg  Roedel <joro@8bytes.org>,,
	Sj Huang, Linux Media Mailing List, linux-devicetree, Louis Kuo,
	Shengnan Wang (王圣男)

Hi Dongchun,

On Fri, May 08, 2020 at 11:08:08AM +0800, Dongchun Zhu wrote:
> Hi Sakari, Tomasz,
> 
> Thanks for the review.
> 
> On Thu, 2020-05-07 at 15:46 +0200, Tomasz Figa wrote:
> > Hi Sakari, Dongchun,
> > 
> > On Thu, May 7, 2020 at 3:12 PM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > >
> > > HI Dongchun,
> > >
> > > On Thu, May 07, 2020 at 08:45:24PM +0800, Dongchun Zhu wrote:
> > > > Hi Sakari,
> > > >
> > > > Thanks for the review.
> > > >
> > > > On Wed, 2020-05-06 at 18:13 +0300, Sakari Ailus wrote:
> > > > > Hi Dongchun,
> > > > >
> > > > > On Sun, May 03, 2020 at 12:17:27AM +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  |  11 ++
> > > > > >  drivers/media/i2c/Makefile |   1 +
> > > > > >  drivers/media/i2c/dw9768.c | 440 +++++++++++++++++++++++++++++++++++++++++++++
> > > > > >  4 files changed, 453 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..6a3f9da 100644
> > > > > > --- a/drivers/media/i2c/Kconfig
> > > > > > +++ b/drivers/media/i2c/Kconfig
> > > > > > @@ -1040,6 +1040,17 @@ 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 && MEDIA_CONTROLLER
> > > > > > + depends on VIDEO_V4L2_SUBDEV_API
> > > > >
> > > > > Please check how this works in the media tree master branch now --- it's
> > > > > largely select based.
> > > > >
> > > >
> > > > The actuator driver uses some structures that require the
> > > > VIDEO_V4L2_SUBDEV_API code, so here we add VIDEO_V4L2_SUBDEV_API
> > > > dependency to avoid possible build error when it's not enabled.
> > >
> > > Please make sure this works with current media tree master. Right now it
> > > does not.
> > >
> > 
> > Dongchun, as Sakari said, please make sure to base the patches on the
> > master branch of the media tree.
> > (https://git.linuxtv.org/media_tree.git/). The approach for Kconfig
> > dependency selection there seems to have changed recently.
> > 
> 
> I searched the patches on the media tree master branch.
> It seems that we need to remove the VIDEO_V4L2_SUBDEV_API dependency in
> Kconfig, and add #ifdef CONFIG_VIDEO_V4L2_SUBDEV_API to include
> v4l2-subdev code.
> The change mainly is to make build pass, and don't return ENOTTY if
> SUBDEV_API is not set.
> Am I right?

Please see Kconfig entries for other similar drivers from Dongwoon.

> 
> > > >
> > > > > In general the patch seems fine to me, but please see the other comments
> > > > > below, too.
> > > > >
> > > > > > + depends on PM
> > > > > > + 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..dd68534
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/media/i2c/dw9768.c
> > > > > > @@ -0,0 +1,440 @@
> > > > > > +// 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-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_AAC3_SELECT_DIVIDING_RATE_1       0x41
> > > > >
> > > > > I guess we can start with these values. But I can't think of another option
> > > > > than putting them into DT if there are differences between what hardware
> > > > > platforms require.
> > > > >
> > > >
> > > > Let's have a discussion about this.
> > > > Now these non-default register settings represent one AAC operation
> > > > mode, this is one option and works for a given lens or a module.
> > > > If sometime in the future hardware platforms require another different
> > > > settings, then DT properties may need to be created.
> > >
> > > If these values indeed are specific to a given lens (and presumably also a
> > > spring), then I'd put them to DT right now --- we don't have drivers for
> > > these components the drivers of which could hold this information, nor it
> > > makes sense to add them just for that.
> > >
> > 
> > I tend to stay on the conservative side and only add DT properties
> > once there is really a need to do so. Right now we haven't seen any
> > system which would use different values of these parameters.
> > 
> > Best regards,
> > Tomasz
> > 
> > > >
> > > > > > +
> > > > > > +/*
> > > > > > + * 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
> > > > > > +#define DW9768_AACT_CNT                          0x39
> > > > > > +
> > > > > > +/*
> > > > > > + * 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;
> > > > > > +};
> > > > > > +
> > > > > > +static inline struct dw9768 *to_dw9768(struct v4l2_ctrl *ctrl)
> > > > > > +{
> > > > > > + return container_of(ctrl->handler, struct dw9768, ctrls);
> > > > > > +}
> > > > >
> > > > > This is used in a single place. I'd just use container_of() directly there.
> > > > >
> > > >
> > > > Thanks for the reminder.
> > > > 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 const struct regval_list dw9768_init_regs[] = {
> > > > > > + {DW9768_RING_PD_CONTROL_REG, DW9768_AAC_MODE_EN},
> > > > > > + {DW9768_AAC_PRESC_REG, DW9768_AAC3_SELECT_DIVIDING_RATE_1},
> > > > > > + {DW9768_AAC_TIME_REG, DW9768_AACT_CNT},
> > > > > > +};
> > > > > > +
> > > > > > +static int dw9768_write_array(struct dw9768 *dw9768,
> > > > > > +                       const struct regval_list *vals, size_t len)
> > > > > > +{
> > > > > > + struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
> > > > > > + unsigned int i;
> > > > > > + int ret;
> > > > > > +
> > > > > > + for (i = 0; i < len; i++) {
> > > > > > +         ret = i2c_smbus_write_byte_data(client, vals[i].reg_num,
> > > > > > +                                         vals[i].value);
> > > > > > +         if (ret < 0)
> > > > > > +                 return ret;
> > > > > > + }
> > > > > > + return 0;
> > > > > > +}
> > > > > > +
> > > > > > +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);
> > > > > > +
> > > > > > + ret = dw9768_write_array(dw9768, dw9768_init_regs,
> > > > > > +                          ARRAY_SIZE(dw9768_init_regs));
> > > > > > + if (ret)
> > > > > > +         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 __maybe_unused 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 __maybe_unused 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 = to_dw9768(ctrl);
> > > > > > +
> > > > > > + 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 i;
> > > > > > + int ret;
> > > > > > +
> > > > > > + dw9768 = devm_kzalloc(dev, sizeof(*dw9768), GFP_KERNEL);
> > > > > > + if (!dw9768)
> > > > > > +         return -ENOMEM;
> > > > > > +
> > > > > > + v4l2_i2c_subdev_init(&dw9768->sd, client, &dw9768_ops);
> > > > > > +
> > > > > > + 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" },
> > > > > > + {}
> > > > > > +};
> > > > > > +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");
> > > > >
> > > >
> > >
> > > --
> > > Sakari Ailus
> 

-- 
Sakari Ailus

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [V5, 2/2] media: i2c: dw9768: Add DW9768 VCM driver
  2020-05-08 21:13             ` Sakari Ailus
@ 2020-05-09  2:23               ` Dongchun Zhu
  2020-05-11 18:20                 ` Tomasz Figa
  0 siblings, 1 reply; 17+ messages in thread
From: Dongchun Zhu @ 2020-05-09  2:23 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Tomasz Figa, Linus Walleij, Bartosz Golaszewski,
	Mauro Carvalho Chehab, Andy Shevchenko, Rob Herring,
	Mark Rutland, Nicolas Boichat, Matthias Brugger, Cao Bing Bu,
	srv_heupstream, moderated list:ARM/Mediatek SoC support,
	list@263.net:IOMMU DRIVERS
	<iommu@lists.linux-foundation.org>,
	Joerg  Roedel <joro@8bytes.org>,,
	Sj Huang, Linux Media Mailing List, linux-devicetree, Louis Kuo,
	Shengnan Wang (王圣男)

Hi Sakari,

On Sat, 2020-05-09 at 00:13 +0300, Sakari Ailus wrote:
> Hi Dongchun,
> 
> On Fri, May 08, 2020 at 11:08:08AM +0800, Dongchun Zhu wrote:
> > Hi Sakari, Tomasz,
> > 
> > Thanks for the review.
> > 
> > On Thu, 2020-05-07 at 15:46 +0200, Tomasz Figa wrote:
> > > Hi Sakari, Dongchun,
> > > 
> > > On Thu, May 7, 2020 at 3:12 PM Sakari Ailus
> > > <sakari.ailus@linux.intel.com> wrote:
> > > >
> > > > HI Dongchun,
> > > >
> > > > On Thu, May 07, 2020 at 08:45:24PM +0800, Dongchun Zhu wrote:
> > > > > Hi Sakari,
> > > > >
> > > > > Thanks for the review.
> > > > >
> > > > > On Wed, 2020-05-06 at 18:13 +0300, Sakari Ailus wrote:
> > > > > > Hi Dongchun,
> > > > > >
> > > > > > On Sun, May 03, 2020 at 12:17:27AM +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  |  11 ++
> > > > > > >  drivers/media/i2c/Makefile |   1 +
> > > > > > >  drivers/media/i2c/dw9768.c | 440 +++++++++++++++++++++++++++++++++++++++++++++
> > > > > > >  4 files changed, 453 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..6a3f9da 100644
> > > > > > > --- a/drivers/media/i2c/Kconfig
> > > > > > > +++ b/drivers/media/i2c/Kconfig
> > > > > > > @@ -1040,6 +1040,17 @@ 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 && MEDIA_CONTROLLER
> > > > > > > + depends on VIDEO_V4L2_SUBDEV_API
> > > > > >
> > > > > > Please check how this works in the media tree master branch now --- it's
> > > > > > largely select based.
> > > > > >
> > > > >
> > > > > The actuator driver uses some structures that require the
> > > > > VIDEO_V4L2_SUBDEV_API code, so here we add VIDEO_V4L2_SUBDEV_API
> > > > > dependency to avoid possible build error when it's not enabled.
> > > >
> > > > Please make sure this works with current media tree master. Right now it
> > > > does not.
> > > >
> > > 
> > > Dongchun, as Sakari said, please make sure to base the patches on the
> > > master branch of the media tree.
> > > (https://git.linuxtv.org/media_tree.git/). The approach for Kconfig
> > > dependency selection there seems to have changed recently.
> > > 
> > 
> > I searched the patches on the media tree master branch.
> > It seems that we need to remove the VIDEO_V4L2_SUBDEV_API dependency in
> > Kconfig, and add #ifdef CONFIG_VIDEO_V4L2_SUBDEV_API to include
> > v4l2-subdev code.
> > The change mainly is to make build pass, and don't return ENOTTY if
> > SUBDEV_API is not set.
> > Am I right?
> 
> Please see Kconfig entries for other similar drivers from Dongwoon.
> 

Sorry for the mistake :-)
Just found the current media tree master branch code...
I would update Kconfig entries in next release by referring to:
https://git.linuxtv.org/media_tree.git/tree/drivers/media/i2c/Kconfig

...


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [V5, 2/2] media: i2c: dw9768: Add DW9768 VCM driver
  2020-05-09  2:23               ` Dongchun Zhu
@ 2020-05-11 18:20                 ` Tomasz Figa
  2020-05-12  3:33                   ` Dongchun Zhu
  0 siblings, 1 reply; 17+ messages in thread
From: Tomasz Figa @ 2020-05-11 18:20 UTC (permalink / raw)
  To: Dongchun Zhu
  Cc: Sakari Ailus, Linus Walleij, Bartosz Golaszewski,
	Mauro Carvalho Chehab, Andy Shevchenko, Rob Herring,
	Mark Rutland, Nicolas Boichat, Matthias Brugger, Cao Bing Bu,
	srv_heupstream, moderated list:ARM/Mediatek SoC support,
	list@263.net:IOMMU DRIVERS
	<iommu@lists.linux-foundation.org>,
	Joerg Roedel <joro@8bytes.org>,,
	Sj Huang, Linux Media Mailing List, linux-devicetree, Louis Kuo,
	Shengnan Wang (王圣男)

Hi Dongchun,

On Sat, May 9, 2020 at 4:25 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
>
> Hi Sakari,
>
> On Sat, 2020-05-09 at 00:13 +0300, Sakari Ailus wrote:
> > Hi Dongchun,
> >
> > On Fri, May 08, 2020 at 11:08:08AM +0800, Dongchun Zhu wrote:
> > > Hi Sakari, Tomasz,
> > >
> > > Thanks for the review.
> > >
> > > On Thu, 2020-05-07 at 15:46 +0200, Tomasz Figa wrote:
> > > > Hi Sakari, Dongchun,
> > > >
> > > > On Thu, May 7, 2020 at 3:12 PM Sakari Ailus
> > > > <sakari.ailus@linux.intel.com> wrote:
> > > > >
> > > > > HI Dongchun,
> > > > >
> > > > > On Thu, May 07, 2020 at 08:45:24PM +0800, Dongchun Zhu wrote:
> > > > > > Hi Sakari,
> > > > > >
> > > > > > Thanks for the review.
> > > > > >
> > > > > > On Wed, 2020-05-06 at 18:13 +0300, Sakari Ailus wrote:
> > > > > > > Hi Dongchun,
> > > > > > >
> > > > > > > On Sun, May 03, 2020 at 12:17:27AM +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  |  11 ++
> > > > > > > >  drivers/media/i2c/Makefile |   1 +
> > > > > > > >  drivers/media/i2c/dw9768.c | 440 +++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > >  4 files changed, 453 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..6a3f9da 100644
> > > > > > > > --- a/drivers/media/i2c/Kconfig
> > > > > > > > +++ b/drivers/media/i2c/Kconfig
> > > > > > > > @@ -1040,6 +1040,17 @@ 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 && MEDIA_CONTROLLER
> > > > > > > > + depends on VIDEO_V4L2_SUBDEV_API
> > > > > > >
> > > > > > > Please check how this works in the media tree master branch now --- it's
> > > > > > > largely select based.
> > > > > > >
> > > > > >
> > > > > > The actuator driver uses some structures that require the
> > > > > > VIDEO_V4L2_SUBDEV_API code, so here we add VIDEO_V4L2_SUBDEV_API
> > > > > > dependency to avoid possible build error when it's not enabled.
> > > > >
> > > > > Please make sure this works with current media tree master. Right now it
> > > > > does not.
> > > > >
> > > >
> > > > Dongchun, as Sakari said, please make sure to base the patches on the
> > > > master branch of the media tree.
> > > > (https://git.linuxtv.org/media_tree.git/). The approach for Kconfig
> > > > dependency selection there seems to have changed recently.
> > > >
> > >
> > > I searched the patches on the media tree master branch.
> > > It seems that we need to remove the VIDEO_V4L2_SUBDEV_API dependency in
> > > Kconfig, and add #ifdef CONFIG_VIDEO_V4L2_SUBDEV_API to include
> > > v4l2-subdev code.
> > > The change mainly is to make build pass, and don't return ENOTTY if
> > > SUBDEV_API is not set.
> > > Am I right?
> >
> > Please see Kconfig entries for other similar drivers from Dongwoon.
> >
>
> Sorry for the mistake :-)
> Just found the current media tree master branch code...
> I would update Kconfig entries in next release by referring to:
> https://git.linuxtv.org/media_tree.git/tree/drivers/media/i2c/Kconfig

Sorry for last minute comments again. We had a short discussion
offline with Sakari and we think there are some changes needed to this
driver, namely:

1) The hardware being driven in our case is a gt9769, which could be
compatible with dw9768, but it's still a different implementation and
could have slightly different characteristics. Thus we think the
driver name and compatible strings should be renamed from
dongwoon,dw9768 to giantec,gt9769. In the future, if there is a device
with exactly a dw9768 chip, the same driver could be reused by adding
a dongwoon,dw9768 compatible string.

2) The chip has some default configuration, which is lost because the
driver overrides it with its own values. For use cases where one would
want to keep the default values, we should make it possible to prevent
the driver from overriding them. We could achieve this by adding
optional DT properties for the custom parameters and if they are not
present, defaults would be used.

Do you think that is doable? Thanks!

Best regards,
Tomasz

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [V5, 2/2] media: i2c: dw9768: Add DW9768 VCM driver
  2020-05-11 18:20                 ` Tomasz Figa
@ 2020-05-12  3:33                   ` Dongchun Zhu
  2020-05-12  8:58                     ` Sakari Ailus
  0 siblings, 1 reply; 17+ messages in thread
From: Dongchun Zhu @ 2020-05-12  3:33 UTC (permalink / raw)
  To: Tomasz Figa
  Cc: Sakari Ailus, Linus Walleij, Bartosz Golaszewski,
	Mauro Carvalho Chehab, Andy Shevchenko, Rob Herring,
	Mark Rutland, Nicolas Boichat, Matthias Brugger, Cao Bing Bu,
	srv_heupstream, moderated list:ARM/Mediatek SoC support,
	list@263.net:IOMMU DRIVERS
	<iommu@lists.linux-foundation.org>,
	Joerg  Roedel <joro@8bytes.org>,,
	Sj Huang, Linux Media Mailing List, linux-devicetree, Louis Kuo,
	Shengnan Wang (王圣男)

Hi Tomasz,

On Mon, 2020-05-11 at 20:20 +0200, Tomasz Figa wrote:
> Hi Dongchun,
> 
> On Sat, May 9, 2020 at 4:25 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> >
> > Hi Sakari,
> >
> > On Sat, 2020-05-09 at 00:13 +0300, Sakari Ailus wrote:
> > > Hi Dongchun,
> > >
> > > On Fri, May 08, 2020 at 11:08:08AM +0800, Dongchun Zhu wrote:
> > > > Hi Sakari, Tomasz,
> > > >
> > > > Thanks for the review.
> > > >
> > > > On Thu, 2020-05-07 at 15:46 +0200, Tomasz Figa wrote:
> > > > > Hi Sakari, Dongchun,
> > > > >
> > > > > On Thu, May 7, 2020 at 3:12 PM Sakari Ailus
> > > > > <sakari.ailus@linux.intel.com> wrote:
> > > > > >
> > > > > > HI Dongchun,
> > > > > >
> > > > > > On Thu, May 07, 2020 at 08:45:24PM +0800, Dongchun Zhu wrote:
> > > > > > > Hi Sakari,
> > > > > > >
> > > > > > > Thanks for the review.
> > > > > > >
> > > > > > > On Wed, 2020-05-06 at 18:13 +0300, Sakari Ailus wrote:
> > > > > > > > Hi Dongchun,
> > > > > > > >
> > > > > > > > On Sun, May 03, 2020 at 12:17:27AM +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  |  11 ++
> > > > > > > > >  drivers/media/i2c/Makefile |   1 +
> > > > > > > > >  drivers/media/i2c/dw9768.c | 440 +++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > > >  4 files changed, 453 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..6a3f9da 100644
> > > > > > > > > --- a/drivers/media/i2c/Kconfig
> > > > > > > > > +++ b/drivers/media/i2c/Kconfig
> > > > > > > > > @@ -1040,6 +1040,17 @@ 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 && MEDIA_CONTROLLER
> > > > > > > > > + depends on VIDEO_V4L2_SUBDEV_API
> > > > > > > >
> > > > > > > > Please check how this works in the media tree master branch now --- it's
> > > > > > > > largely select based.
> > > > > > > >
> > > > > > >
> > > > > > > The actuator driver uses some structures that require the
> > > > > > > VIDEO_V4L2_SUBDEV_API code, so here we add VIDEO_V4L2_SUBDEV_API
> > > > > > > dependency to avoid possible build error when it's not enabled.
> > > > > >
> > > > > > Please make sure this works with current media tree master. Right now it
> > > > > > does not.
> > > > > >
> > > > >
> > > > > Dongchun, as Sakari said, please make sure to base the patches on the
> > > > > master branch of the media tree.
> > > > > (https://git.linuxtv.org/media_tree.git/). The approach for Kconfig
> > > > > dependency selection there seems to have changed recently.
> > > > >
> > > >
> > > > I searched the patches on the media tree master branch.
> > > > It seems that we need to remove the VIDEO_V4L2_SUBDEV_API dependency in
> > > > Kconfig, and add #ifdef CONFIG_VIDEO_V4L2_SUBDEV_API to include
> > > > v4l2-subdev code.
> > > > The change mainly is to make build pass, and don't return ENOTTY if
> > > > SUBDEV_API is not set.
> > > > Am I right?
> > >
> > > Please see Kconfig entries for other similar drivers from Dongwoon.
> > >
> >
> > Sorry for the mistake :-)
> > Just found the current media tree master branch code...
> > I would update Kconfig entries in next release by referring to:
> > https://git.linuxtv.org/media_tree.git/tree/drivers/media/i2c/Kconfig
> 
> Sorry for last minute comments again. We had a short discussion
> offline with Sakari and we think there are some changes needed to this
> driver, namely:
> 
> 1) The hardware being driven in our case is a gt9769, which could be
> compatible with dw9768, but it's still a different implementation and
> could have slightly different characteristics. Thus we think the
> driver name and compatible strings should be renamed from
> dongwoon,dw9768 to giantec,gt9769. In the future, if there is a device
> with exactly a dw9768 chip, the same driver could be reused by adding
> a dongwoon,dw9768 compatible string.
> 
> 2) The chip has some default configuration, which is lost because the
> driver overrides it with its own values. For use cases where one would
> want to keep the default values, we should make it possible to prevent
> the driver from overriding them. We could achieve this by adding
> optional DT properties for the custom parameters and if they are not
> present, defaults would be used.
> 
> Do you think that is doable? Thanks!
> 
> Best regards,
> Tomasz

For the renaming, I cannot agree with you more.
I would try to adjust it in next release.

For actuator driver, it seems our initial idea is that driver registers
should always use the default or reserved values.
Indeed, nowadays lens drivers on the community don't have such
configuration registers.
Albeit I don't quite sure whether it is reasonable for this summary.

As you discussed, for the current kernel, it would be nice to keep the
same with the traditional style.


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [V5, 2/2] media: i2c: dw9768: Add DW9768 VCM driver
  2020-05-12  3:33                   ` Dongchun Zhu
@ 2020-05-12  8:58                     ` Sakari Ailus
  2020-05-12 11:32                       ` Dongchun Zhu
  0 siblings, 1 reply; 17+ messages in thread
From: Sakari Ailus @ 2020-05-12  8:58 UTC (permalink / raw)
  To: Dongchun Zhu
  Cc: Tomasz Figa, Linus Walleij, Bartosz Golaszewski,
	Mauro Carvalho Chehab, Andy Shevchenko, Rob Herring,
	Mark Rutland, Nicolas Boichat, Matthias Brugger, Cao Bing Bu,
	srv_heupstream, moderated list:ARM/Mediatek SoC support,
	list@263.net:IOMMU DRIVERS
	<iommu@lists.linux-foundation.org>,
	Joerg  Roedel <joro@8bytes.org>,,
	Sj Huang, Linux Media Mailing List, linux-devicetree, Louis Kuo,
	Shengnan Wang (王圣男)

Hi Dongchun,

On Tue, May 12, 2020 at 11:33:23AM +0800, Dongchun Zhu wrote:
> Hi Tomasz,
> 
> On Mon, 2020-05-11 at 20:20 +0200, Tomasz Figa wrote:
> > Hi Dongchun,
> > 
> > On Sat, May 9, 2020 at 4:25 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> > >
> > > Hi Sakari,
> > >
> > > On Sat, 2020-05-09 at 00:13 +0300, Sakari Ailus wrote:
> > > > Hi Dongchun,
> > > >
> > > > On Fri, May 08, 2020 at 11:08:08AM +0800, Dongchun Zhu wrote:
> > > > > Hi Sakari, Tomasz,
> > > > >
> > > > > Thanks for the review.
> > > > >
> > > > > On Thu, 2020-05-07 at 15:46 +0200, Tomasz Figa wrote:
> > > > > > Hi Sakari, Dongchun,
> > > > > >
> > > > > > On Thu, May 7, 2020 at 3:12 PM Sakari Ailus
> > > > > > <sakari.ailus@linux.intel.com> wrote:
> > > > > > >
> > > > > > > HI Dongchun,
> > > > > > >
> > > > > > > On Thu, May 07, 2020 at 08:45:24PM +0800, Dongchun Zhu wrote:
> > > > > > > > Hi Sakari,
> > > > > > > >
> > > > > > > > Thanks for the review.
> > > > > > > >
> > > > > > > > On Wed, 2020-05-06 at 18:13 +0300, Sakari Ailus wrote:
> > > > > > > > > Hi Dongchun,
> > > > > > > > >
> > > > > > > > > On Sun, May 03, 2020 at 12:17:27AM +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  |  11 ++
> > > > > > > > > >  drivers/media/i2c/Makefile |   1 +
> > > > > > > > > >  drivers/media/i2c/dw9768.c | 440 +++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > > > >  4 files changed, 453 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..6a3f9da 100644
> > > > > > > > > > --- a/drivers/media/i2c/Kconfig
> > > > > > > > > > +++ b/drivers/media/i2c/Kconfig
> > > > > > > > > > @@ -1040,6 +1040,17 @@ 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 && MEDIA_CONTROLLER
> > > > > > > > > > + depends on VIDEO_V4L2_SUBDEV_API
> > > > > > > > >
> > > > > > > > > Please check how this works in the media tree master branch now --- it's
> > > > > > > > > largely select based.
> > > > > > > > >
> > > > > > > >
> > > > > > > > The actuator driver uses some structures that require the
> > > > > > > > VIDEO_V4L2_SUBDEV_API code, so here we add VIDEO_V4L2_SUBDEV_API
> > > > > > > > dependency to avoid possible build error when it's not enabled.
> > > > > > >
> > > > > > > Please make sure this works with current media tree master. Right now it
> > > > > > > does not.
> > > > > > >
> > > > > >
> > > > > > Dongchun, as Sakari said, please make sure to base the patches on the
> > > > > > master branch of the media tree.
> > > > > > (https://git.linuxtv.org/media_tree.git/). The approach for Kconfig
> > > > > > dependency selection there seems to have changed recently.
> > > > > >
> > > > >
> > > > > I searched the patches on the media tree master branch.
> > > > > It seems that we need to remove the VIDEO_V4L2_SUBDEV_API dependency in
> > > > > Kconfig, and add #ifdef CONFIG_VIDEO_V4L2_SUBDEV_API to include
> > > > > v4l2-subdev code.
> > > > > The change mainly is to make build pass, and don't return ENOTTY if
> > > > > SUBDEV_API is not set.
> > > > > Am I right?
> > > >
> > > > Please see Kconfig entries for other similar drivers from Dongwoon.
> > > >
> > >
> > > Sorry for the mistake :-)
> > > Just found the current media tree master branch code...
> > > I would update Kconfig entries in next release by referring to:
> > > https://git.linuxtv.org/media_tree.git/tree/drivers/media/i2c/Kconfig
> > 
> > Sorry for last minute comments again. We had a short discussion
> > offline with Sakari and we think there are some changes needed to this
> > driver, namely:
> > 
> > 1) The hardware being driven in our case is a gt9769, which could be
> > compatible with dw9768, but it's still a different implementation and
> > could have slightly different characteristics. Thus we think the
> > driver name and compatible strings should be renamed from
> > dongwoon,dw9768 to giantec,gt9769. In the future, if there is a device

Sorry, I actually meant just the compatible string --- Dongwoon is likely
the original manufacturer. Therefore I'd name the driver according to that,
and just add a second compatible string for the Giantec device.

Either works for me though.

-- 
Regards,

Sakari Ailus

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [V5, 2/2] media: i2c: dw9768: Add DW9768 VCM driver
  2020-05-12  8:58                     ` Sakari Ailus
@ 2020-05-12 11:32                       ` Dongchun Zhu
  0 siblings, 0 replies; 17+ messages in thread
From: Dongchun Zhu @ 2020-05-12 11:32 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Tomasz Figa, Linus Walleij, Bartosz Golaszewski,
	Mauro Carvalho Chehab, Andy Shevchenko, Rob Herring,
	Mark Rutland, Nicolas Boichat, Matthias Brugger, Cao Bing Bu,
	srv_heupstream, moderated list:ARM/Mediatek SoC support,
	list@263.net:IOMMU DRIVERS
	<iommu@lists.linux-foundation.org>,
	Joerg  Roedel <joro@8bytes.org>,,
	Sj Huang, Linux Media Mailing List, linux-devicetree, Louis Kuo,
	Shengnan Wang (王圣男),
	dongchun.zhu

Hi Sakari,

On Tue, 2020-05-12 at 11:58 +0300, Sakari Ailus wrote:
> Hi Dongchun,
> 
> On Tue, May 12, 2020 at 11:33:23AM +0800, Dongchun Zhu wrote:
> > Hi Tomasz,
> > 
> > On Mon, 2020-05-11 at 20:20 +0200, Tomasz Figa wrote:
> > > Hi Dongchun,
> > > 
> > > On Sat, May 9, 2020 at 4:25 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> > > >
> > > > Hi Sakari,
> > > >
> > > > On Sat, 2020-05-09 at 00:13 +0300, Sakari Ailus wrote:
> > > > > Hi Dongchun,
> > > > >
> > > > > On Fri, May 08, 2020 at 11:08:08AM +0800, Dongchun Zhu wrote:
> > > > > > Hi Sakari, Tomasz,
> > > > > >
> > > > > > Thanks for the review.
> > > > > >
> > > > > > On Thu, 2020-05-07 at 15:46 +0200, Tomasz Figa wrote:
> > > > > > > Hi Sakari, Dongchun,
> > > > > > >
> > > > > > > On Thu, May 7, 2020 at 3:12 PM Sakari Ailus
> > > > > > > <sakari.ailus@linux.intel.com> wrote:
> > > > > > > >
> > > > > > > > HI Dongchun,
> > > > > > > >
> > > > > > > > On Thu, May 07, 2020 at 08:45:24PM +0800, Dongchun Zhu wrote:
> > > > > > > > > Hi Sakari,
> > > > > > > > >
> > > > > > > > > Thanks for the review.
> > > > > > > > >
> > > > > > > > > On Wed, 2020-05-06 at 18:13 +0300, Sakari Ailus wrote:
> > > > > > > > > > Hi Dongchun,
> > > > > > > > > >
> > > > > > > > > > On Sun, May 03, 2020 at 12:17:27AM +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  |  11 ++
> > > > > > > > > > >  drivers/media/i2c/Makefile |   1 +
> > > > > > > > > > >  drivers/media/i2c/dw9768.c | 440 +++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > > > > >  4 files changed, 453 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..6a3f9da 100644
> > > > > > > > > > > --- a/drivers/media/i2c/Kconfig
> > > > > > > > > > > +++ b/drivers/media/i2c/Kconfig
> > > > > > > > > > > @@ -1040,6 +1040,17 @@ 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 && MEDIA_CONTROLLER
> > > > > > > > > > > + depends on VIDEO_V4L2_SUBDEV_API
> > > > > > > > > >
> > > > > > > > > > Please check how this works in the media tree master branch now --- it's
> > > > > > > > > > largely select based.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > The actuator driver uses some structures that require the
> > > > > > > > > VIDEO_V4L2_SUBDEV_API code, so here we add VIDEO_V4L2_SUBDEV_API
> > > > > > > > > dependency to avoid possible build error when it's not enabled.
> > > > > > > >
> > > > > > > > Please make sure this works with current media tree master. Right now it
> > > > > > > > does not.
> > > > > > > >
> > > > > > >
> > > > > > > Dongchun, as Sakari said, please make sure to base the patches on the
> > > > > > > master branch of the media tree.
> > > > > > > (https://git.linuxtv.org/media_tree.git/). The approach for Kconfig
> > > > > > > dependency selection there seems to have changed recently.
> > > > > > >
> > > > > >
> > > > > > I searched the patches on the media tree master branch.
> > > > > > It seems that we need to remove the VIDEO_V4L2_SUBDEV_API dependency in
> > > > > > Kconfig, and add #ifdef CONFIG_VIDEO_V4L2_SUBDEV_API to include
> > > > > > v4l2-subdev code.
> > > > > > The change mainly is to make build pass, and don't return ENOTTY if
> > > > > > SUBDEV_API is not set.
> > > > > > Am I right?
> > > > >
> > > > > Please see Kconfig entries for other similar drivers from Dongwoon.
> > > > >
> > > >
> > > > Sorry for the mistake :-)
> > > > Just found the current media tree master branch code...
> > > > I would update Kconfig entries in next release by referring to:
> > > > https://git.linuxtv.org/media_tree.git/tree/drivers/media/i2c/Kconfig
> > > 
> > > Sorry for last minute comments again. We had a short discussion
> > > offline with Sakari and we think there are some changes needed to this
> > > driver, namely:
> > > 
> > > 1) The hardware being driven in our case is a gt9769, which could be
> > > compatible with dw9768, but it's still a different implementation and
> > > could have slightly different characteristics. Thus we think the
> > > driver name and compatible strings should be renamed from
> > > dongwoon,dw9768 to giantec,gt9769. In the future, if there is a device
> 
> Sorry, I actually meant just the compatible string --- Dongwoon is likely
> the original manufacturer. Therefore I'd name the driver according to that,
> and just add a second compatible string for the Giantec device.
> 
> Either works for me though.
> 

Just checked the legacy lens driver based on Mediatek architecture.
I found that both DW9718 and DW9719 have been using some configuration
registers to initialize VCM.
Unlickily, I cannot see any upstream patches about these two lens
drivers on the community.

For your suggestion, I think it is okay.
In fact, I just synced with Giantec FAE.
Dongwoon Anatech and Giantec are two different driver companies.
Their most VCM driver products are compatible with each other.
In fact, they have a mapping table of DW & GT.
For user, the actuator driver is common, such as DW9768 and GT9769.
However, algorithms inside the chip from different Manufactures differs.

For GT9769/DW9768, there is one read-only register 0x00 that could be
read out to distinguish VCM IC Manufacture ID.
The default value 0xE1 represents Giantec.

Finally I would add one more compatible for Giantec in next release.
Like this:
static const struct of_device_id dw9768_of_table[] = {
   { .compatible = "dongwoon,dw9768" },
   { .compatible = "giantec,gt9769" },
   {}
};
MODULE_DEVICE_TABLE(of, dw9768_of_table);


^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, back to index

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-02 16:17 [V5, 0/2] media: i2c: Add support for DW9768 VCM driver Dongchun Zhu
2020-05-02 16:17 ` [V5, 1/2] media: dt-bindings: media: i2c: Document DW9768 bindings Dongchun Zhu
2020-05-05 19:15   ` Rob Herring
2020-05-02 16:17 ` [V5, 2/2] media: i2c: dw9768: Add DW9768 VCM driver Dongchun Zhu
2020-05-06 15:13   ` Sakari Ailus
2020-05-07 12:45     ` Dongchun Zhu
2020-05-07 13:12       ` Sakari Ailus
2020-05-07 13:46         ` Tomasz Figa
2020-05-07 14:00           ` Sakari Ailus
2020-05-08  3:27             ` Dongchun Zhu
2020-05-08  3:08           ` Dongchun Zhu
2020-05-08 21:13             ` Sakari Ailus
2020-05-09  2:23               ` Dongchun Zhu
2020-05-11 18:20                 ` Tomasz Figa
2020-05-12  3:33                   ` Dongchun Zhu
2020-05-12  8:58                     ` Sakari Ailus
2020-05-12 11:32                       ` Dongchun Zhu

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org
	public-inbox-index linux-media

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git