[PATCH 0/2] media: i2c: Add support for GT97xx VCM

[PATCH 0/2] media: i2c: Add support for GT97xx VCM

[Zhi Mao]

This series add YAML DT binding and V4L2 sub-device driver for Giantec's GT9768&GT9769.
GT9768&GT9769 is a 10-bit DAC with 100mA output current sink capability, designed
for voice coil motor(VCM) with I2C control bus.

This driver supports:
 - support pm runtime function for suspend/resume
 - support camera lens focus position by V4L2_CID_FOCUS_ABSOLUTE CMD
 - used in camera features on ChromeOS application

This series is based on linux-next, tag: next-20240409

Thanks

Zhi Mao (2):
  media: dt-bindings: i2c: add Giantec GT97xx VCM driver
  media: i2c: Add GT97xx VCM driver

 .../bindings/media/i2c/giantec,gt97xx.yaml    |  91 +++
 drivers/media/i2c/Kconfig                     |  13 +
 drivers/media/i2c/Makefile                    |   1 +
 drivers/media/i2c/gt97xx.c                    | 640 ++++++++++++++++++
 4 files changed, 745 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml
 create mode 100644 drivers/media/i2c/gt97xx.c

-- 
2.25.1 

[PATCH 1/2] media: dt-bindings: i2c: add Giantec GT97xx VCM driver

[Zhi Mao]

Add YAML device tree binding for GT97xx VCM driver,
and the relevant MAINTAINERS entries.

Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
---
 .../bindings/media/i2c/giantec,gt97xx.yaml    | 91 +++++++++++++++++++
 1 file changed, 91 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml

diff --git a/Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml b/Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml
new file mode 100644
index 000000000000..8c9f1eb4dac8
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml
@@ -0,0 +1,91 @@
+# 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/giantec,gt97xx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Giantec Semiconductor, Crop. GT97xx Voice Coil Motor (VCM)
+
+maintainers:
+  - Zhi Mao <zhi.mao@mediatek.com>
+
+description: |-
+  The Giantec GT97xx is a 10-bit DAC with current sink capability.
+  The DAC is controlled via I2C bus that operates at clock rates up to 1MHz.
+  This chip integrates Advanced Actuator Control (AAC) technology
+  and is intended for driving voice coil lens in camera modules.
+
+properties:
+  compatible:
+    enum:
+      - giantec,gt9768 # for GT9768 VCM
+      - giantec,gt9769 # for GT9769 VCM
+
+  reg:
+    maxItems: 1
+
+  vin-supply: true
+
+  vdd-supply: true
+
+  giantec,aac-mode:
+    description:
+      Indication of AAC mode select.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum:
+      - 1    #  AAC2 mode(operation time# 0.48 x Tvib)
+      - 2    #  AAC3 mode(operation time# 0.70 x Tvib)
+      - 3    #  AAC4 mode(operation time# 0.75 x Tvib)
+      - 5    #  AAC8 mode(operation time# 1.13 x Tvib)
+    default: 2
+
+  giantec,aac-timing:
+    description:
+      Number of AAC Timing count that controlled by one 6-bit period of
+      vibration register AACT[5:0], the unit of which is 100 us.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    default: 0x20
+    minimum: 0x00
+    maximum: 0x3f
+
+  giantec,clock-presc:
+    description:
+      Indication of VCM internal clock dividing rate select, as one multiple
+      factor to calculate VCM ring periodic time Tvib.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum:
+      - 0    #  Dividing Rate -  2
+      - 1    #  Dividing Rate -  1
+      - 2    #  Dividing Rate -  1/2
+      - 3    #  Dividing Rate -  1/4
+      - 4    #  Dividing Rate -  8
+      - 5    #  Dividing Rate -  4
+    default: 1
+
+required:
+  - compatible
+  - reg
+  - vin-supply
+  - vdd-supply
+
+additionalProperties: false
+
+examples:
+  - |
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        vcm@c {
+            compatible = "giantec,gt9768";
+            reg = <0x0c>;
+
+            vin-supply = <&gt97xx_vin>;
+            vdd-supply = <&gt97xx_vdd>;
+            giantec,aac-timing = <0x20>;
+        };
+    };
+
+...
-- 
2.25.1

[PATCH 2/2] media: i2c: Add GT97xx VCM driver

[Zhi Mao]

Add a V4L2 sub-device driver for Giantec GT97xx VCM.

Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
---
 drivers/media/i2c/Kconfig  |  13 +
 drivers/media/i2c/Makefile |   1 +
 drivers/media/i2c/gt97xx.c | 640 +++++++++++++++++++++++++++++++++++++
 3 files changed, 654 insertions(+)
 create mode 100644 drivers/media/i2c/gt97xx.c

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 56f276b920ab..fcb330cebfe0 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -759,6 +759,19 @@ config VIDEO_DW9807_VCM
 	  capability. This is designed for linear control of
 	  voice coil motors, controlled via I2C serial interface.
 
+config VIDEO_GT97XX
+	tristate "GT97xx lens voice coil support"
+	depends on I2C && VIDEO_DEV
+	select MEDIA_CONTROLLER
+	select VIDEO_V4L2_SUBDEV_API
+	select V4L2_FWNODE
+	select V4L2_CCI_I2C
+	help
+	  This is a driver for the GT97xx camera lens voice coil.
+	  GT97xx is a 10 bit DAC with 100mA output current sink
+	  capability. It is designed for linear control of
+	  voice coil motors, controlled via I2C serial interface.
+
 endmenu
 
 menu "Flash devices"
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index dfbe6448b549..af36a7aa3d12 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -39,6 +39,7 @@ obj-$(CONFIG_VIDEO_DW9807_VCM) += dw9807-vcm.o
 obj-$(CONFIG_VIDEO_ET8EK8) += et8ek8/
 obj-$(CONFIG_VIDEO_GC0308) += gc0308.o
 obj-$(CONFIG_VIDEO_GC2145) += gc2145.o
+obj-$(CONFIG_VIDEO_GT97XX) += gt97xx.o
 obj-$(CONFIG_VIDEO_HI556) += hi556.o
 obj-$(CONFIG_VIDEO_HI846) += hi846.o
 obj-$(CONFIG_VIDEO_HI847) += hi847.o
diff --git a/drivers/media/i2c/gt97xx.c b/drivers/media/i2c/gt97xx.c
new file mode 100644
index 000000000000..d91314b872fa
--- /dev/null
+++ b/drivers/media/i2c/gt97xx.c
@@ -0,0 +1,640 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for Giantec gt97xx VCM lens device
+ *
+ * Copyright 2024 MediaTek
+ *
+ * Zhi Mao <zhi.mao@mediatek.com>
+ */
+#include <linux/array_size.h>
+#include <linux/bits.h>
+#include <linux/clk.h>
+#include <linux/container_of.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/gpio/consumer.h>
+#include <linux/mod_devicetable.h>
+#include <linux/pm_runtime.h>
+#include <linux/property.h>
+#include <linux/regulator/consumer.h>
+#include <linux/types.h>
+#include <linux/units.h>
+
+#include <media/v4l2-cci.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-event.h>
+#include <media/v4l2-fwnode.h>
+#include <media/v4l2-subdev.h>
+
+/* gt97xx chip info register and name */
+#define GT97XX_IC_INFO_REG CCI_REG8(0x00)
+#define GT9768_ID 0xE9
+#define GT9769_ID 0xE1
+#define GT97XX_NAME "gt97xx"
+
+/*
+ * 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
+ * gt97xx requires waiting time of Topr after PD reset takes place.
+ */
+#define GT97XX_RING_PD_CONTROL_REG CCI_REG8(0x02)
+#define GT97XX_PD_MODE_OFF 0x00
+#define GT97XX_PD_MODE_EN BIT(0)
+#define GT97XX_AAC_MODE_EN BIT(1)
+
+/*
+ * gt97xx 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 GT97XX_MSB_ADDR_REG CCI_REG16(0x03)
+
+/*
+ * AAC mode control & prescale register
+ * Bit[7:5] Namely AC[2:0], decide the VCM mode and operation time.
+ * 001 AAC2 0.48 x Tvib
+ * 010 AAC3 0.70 x Tvib
+ * 011 AAC4 0.75 x Tvib
+ * 101 AAC8 1.13 x Tvib
+ * Bit[2:0] Namely PRESC[2:0], set the internal clock dividing rate as follow.
+ * 000 2
+ * 001 1
+ * 010 1/2
+ * 011 1/4
+ * 100 8
+ * 101 4
+ */
+#define GT97XX_AAC_PRESC_REG CCI_REG8(0x06)
+#define GT97XX_AAC_MODE_SEL_MASK GENMASK(7, 5)
+#define GT97XX_CLOCK_PRE_SCALE_SEL_MASK GENMASK(2, 0)
+
+/*
+ * VCM period of vibration register
+ * Bit[5:0] Defined as VCM rising periodic time (Tvib) together with PRESC[2:0]
+ * Tvib = (6.3ms + AACT[5:0] * 0.1ms) * Dividing Rate
+ * Dividing Rate is the internal clock dividing rate that is defined at
+ * PRESCALE register (ADD: 0x06)
+ */
+#define GT97XX_AAC_TIME_REG CCI_REG8(0x07)
+
+/*
+ * gt97xx requires waiting time (delay time) of t_OPR after power-up,
+ * or in the case of PD reset taking place.
+ */
+#define GT97XX_T_OPR_US (1 * USEC_PER_MSEC)
+#define GT97XX_TVIB_MS_BASE10 (64 - 1)
+#define GT97XX_AAC_MODE_DEFAULT 2
+#define GT97XX_AAC_TIME_DEFAULT 0x20
+#define GT97XX_CLOCK_PRE_SCALE_DEFAULT 1
+
+/*
+ * 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 GT97XX_MOVE_STEPS 16
+#define GT97XX_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 GT97XX_FOCUS_STEPS 1
+
+enum vcm_giantec_reg_desc {
+	GT_IC_INFO_REG,
+	GT_RING_PD_CONTROL_REG,
+	GT_MSB_ADDR_REG,
+	GT_AAC_PRESC_REG,
+	GT_AAC_TIME_REG,
+	GT_MAX_REG,
+};
+
+struct vcm_giantec_of_data {
+	unsigned int id;
+	unsigned int regs[GT_MAX_REG];
+};
+
+static const char *const gt97xx_supply_names[] = {
+	"vin", /* Digital I/O power */
+	"vdd", /* Digital core power */
+};
+
+/* gt97xx device structure */
+struct gt97xx {
+	struct v4l2_subdev sd;
+
+	struct regulator_bulk_data supplies[ARRAY_SIZE(gt97xx_supply_names)];
+
+	struct v4l2_ctrl_handler ctrls;
+	struct v4l2_ctrl *focus;
+
+	u32 aac_mode;
+	u32 aac_timing;
+	u32 clock_presc;
+	u32 move_delay_us;
+
+	struct regmap *regmap;
+
+	const struct vcm_giantec_of_data *chip;
+};
+
+static inline struct gt97xx *sd_to_gt97xx(struct v4l2_subdev *subdev)
+{
+	return container_of(subdev, struct gt97xx, sd);
+}
+
+struct regval_list {
+	u8 reg_num;
+	u8 value;
+};
+
+struct gt97xx_aac_mode_ot_multi {
+	u32 aac_mode_enum;
+	u32 ot_multi_base100;
+};
+
+struct gt97xx_clk_presc_dividing_rate {
+	u32 clk_presc_enum;
+	u32 dividing_rate_base100;
+};
+
+static const struct gt97xx_aac_mode_ot_multi aac_mode_ot_multi[] = {
+	{ 1, 48 },
+	{ 2, 70 },
+	{ 3, 75 },
+	{ 5, 113 },
+};
+
+static const struct gt97xx_clk_presc_dividing_rate presc_dividing_rate[] = {
+	{ 0, 200 }, { 1, 100 }, { 2, 50 }, { 3, 25 }, { 4, 800 }, { 5, 400 },
+};
+
+static u32 gt97xx_find_ot_multi(u32 aac_mode_param)
+{
+	u32 cur_ot_multi_base100 = 70;
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(aac_mode_ot_multi); i++) {
+		if (aac_mode_ot_multi[i].aac_mode_enum == aac_mode_param) {
+			cur_ot_multi_base100 =
+				aac_mode_ot_multi[i].ot_multi_base100;
+		}
+	}
+
+	return cur_ot_multi_base100;
+}
+
+static u32 gt97xx_find_dividing_rate(u32 presc_param)
+{
+	u32 cur_clk_dividing_rate_base100 = 100;
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(presc_dividing_rate); i++) {
+		if (presc_dividing_rate[i].clk_presc_enum == presc_param) {
+			cur_clk_dividing_rate_base100 =
+				presc_dividing_rate[i].dividing_rate_base100;
+		}
+	}
+
+	return cur_clk_dividing_rate_base100;
+}
+
+/*
+ * GT97xx_AAC_PRESC_REG & GT97xx_AAC_TIME_REG determine VCM operation time.
+ * For current VCM mode: AAC3, Operation Time would be 0.70 x Tvib.
+ * Tvib = (6.3ms + AACT[5:0] * 0.1MS) * Dividing Rate.
+ * Below is calculation of the operation delay for each step.
+ */
+static inline u32 gt97xx_cal_move_delay(u32 aac_mode_param, u32 presc_param,
+					u32 aac_timing_param)
+{
+	u32 tvib_us;
+	u32 ot_multi_base100;
+	u32 clk_dividing_rate_base100;
+
+	ot_multi_base100 = gt97xx_find_ot_multi(aac_mode_param);
+
+	clk_dividing_rate_base100 = gt97xx_find_dividing_rate(presc_param);
+
+	tvib_us = (GT97XX_TVIB_MS_BASE10 + aac_timing_param) *
+		  clk_dividing_rate_base100;
+
+	return tvib_us * ot_multi_base100 / 100;
+}
+
+static int gt97xx_mod_reg(struct gt97xx *gt97xx, u32 reg, u8 mask, u8 val)
+{
+	u64 read_val;
+	int ret;
+
+	ret = cci_read(gt97xx->regmap, reg, &read_val, NULL);
+	if (ret < 0)
+		return ret;
+
+	val = ((unsigned char)read_val & ~mask) | (val & mask);
+
+	return cci_write(gt97xx->regmap, reg, val, NULL);
+}
+
+static int gt97xx_set_dac(struct gt97xx *gt97xx, u16 val)
+{
+	/* Write VCM position to registers */
+	return cci_write(gt97xx->regmap,
+			 gt97xx->chip->regs[GT_MSB_ADDR_REG], val, NULL);
+}
+
+static int gt97xx_identify_module(struct gt97xx *gt97xx)
+{
+	int ret;
+	u64 ic_id;
+	struct i2c_client *client = v4l2_get_subdevdata(&gt97xx->sd);
+
+	ret = cci_read(gt97xx->regmap, gt97xx->chip->regs[GT_IC_INFO_REG],
+		       &ic_id, NULL);
+	if (ret < 0)
+		return ret;
+
+	if (ic_id != gt97xx->chip->id) {
+		dev_err(&client->dev, "chip id mismatch: 0x%x!=0x%llx",
+			gt97xx->chip->id, ic_id);
+		return -1;
+	}
+
+	return 0;
+}
+
+static int gt97xx_init(struct gt97xx *gt97xx)
+{
+	int ret, val;
+
+	ret = gt97xx_identify_module(gt97xx);
+	if (ret < 0)
+		return ret;
+
+	/* Reset GT97xx_RING_PD_CONTROL_REG to default status 0x00 */
+	ret = cci_write(gt97xx->regmap,
+			gt97xx->chip->regs[GT_RING_PD_CONTROL_REG],
+			GT97XX_PD_MODE_OFF, NULL);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * GT97xx requires waiting delay time of t_OPR
+	 * after PD reset takes place.
+	 */
+	fsleep(GT97XX_T_OPR_US);
+
+	/* Set GT97xx_RING_PD_CONTROL_REG to GT97xx_AAC_MODE_EN(0x01) */
+	ret = cci_write(gt97xx->regmap,
+			gt97xx->chip->regs[GT_RING_PD_CONTROL_REG],
+			GT97XX_AAC_MODE_EN, NULL);
+	if (ret < 0)
+		return ret;
+
+	/* Set AAC mode */
+	ret = gt97xx_mod_reg(gt97xx, gt97xx->chip->regs[GT_AAC_PRESC_REG],
+			     GT97XX_AAC_MODE_SEL_MASK, gt97xx->aac_mode << 5);
+	if (ret < 0)
+		return ret;
+
+	/* Set clock presc */
+	if (gt97xx->clock_presc != GT97XX_CLOCK_PRE_SCALE_DEFAULT) {
+		ret = gt97xx_mod_reg(gt97xx,
+				     gt97xx->chip->regs[GT_AAC_PRESC_REG],
+				     GT97XX_CLOCK_PRE_SCALE_SEL_MASK,
+				     gt97xx->clock_presc);
+		if (ret < 0)
+			return ret;
+	}
+
+	/* Set AAC Timing */
+	if (gt97xx->aac_timing != GT97XX_AAC_TIME_DEFAULT) {
+		ret = cci_write(gt97xx->regmap,
+				gt97xx->chip->regs[GT_AAC_TIME_REG],
+				gt97xx->aac_timing, NULL);
+		if (ret < 0)
+			return ret;
+	}
+
+	for (val = gt97xx->focus->val % GT97XX_MOVE_STEPS;
+	     val <= gt97xx->focus->val; val += GT97XX_MOVE_STEPS) {
+		ret = gt97xx_set_dac(gt97xx, val);
+		if (ret)
+			return ret;
+
+		fsleep(gt97xx->move_delay_us);
+	}
+
+	return 0;
+}
+
+static int gt97xx_release(struct gt97xx *gt97xx)
+{
+	int ret, val;
+
+	val = round_down(gt97xx->focus->val, GT97XX_MOVE_STEPS);
+	for (; val >= 0; val -= GT97XX_MOVE_STEPS) {
+		ret = gt97xx_set_dac(gt97xx, val);
+		if (ret)
+			return ret;
+
+		fsleep(gt97xx->move_delay_us);
+	}
+
+	return 0;
+}
+
+static int gt97xx_power_on(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct gt97xx *gt97xx = sd_to_gt97xx(sd);
+	int ret;
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(gt97xx_supply_names),
+				    gt97xx->supplies);
+	if (ret < 0) {
+		dev_err(dev, "failed to enable regulators\n");
+		return ret;
+	}
+
+	return ret;
+}
+
+static int gt97xx_power_off(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct gt97xx *gt97xx = sd_to_gt97xx(sd);
+	int ret;
+
+	ret = regulator_bulk_disable(ARRAY_SIZE(gt97xx_supply_names),
+				     gt97xx->supplies);
+	if (ret < 0) {
+		dev_err(dev, "failed to disable regulators\n");
+		return ret;
+	}
+
+	return ret;
+}
+
+static int gt97xx_runtime_suspend(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct gt97xx *gt97xx = sd_to_gt97xx(sd);
+
+	gt97xx_release(gt97xx);
+	gt97xx_power_off(dev);
+
+	return 0;
+}
+
+static int gt97xx_runtime_resume(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct gt97xx *gt97xx = sd_to_gt97xx(sd);
+	int ret;
+
+	ret = gt97xx_power_on(dev);
+	if (ret < 0) {
+		dev_err(dev, "failed to power_on\n");
+		return ret;
+	}
+
+	/*
+	 * The datasheet refers to t_OPR that needs to be waited before sending
+	 * I2C commands after power-up.
+	 */
+	fsleep(GT97XX_T_OPR_US);
+
+	ret = gt97xx_init(gt97xx);
+	if (ret < 0)
+		goto disable_power;
+
+	return 0;
+
+disable_power:
+	gt97xx_power_off(dev);
+
+	return ret;
+}
+
+static int gt97xx_set_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct gt97xx *gt97xx =
+		container_of(ctrl->handler, struct gt97xx, ctrls);
+
+	if (ctrl->id == V4L2_CID_FOCUS_ABSOLUTE)
+		return gt97xx_set_dac(gt97xx, ctrl->val);
+
+	return 0;
+}
+
+static const struct v4l2_ctrl_ops gt97xx_ctrl_ops = {
+	.s_ctrl = gt97xx_set_ctrl,
+};
+
+static int gt97xx_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+	return pm_runtime_resume_and_get(sd->dev);
+}
+
+static int gt97xx_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+	return pm_runtime_put(sd->dev);
+}
+
+static const struct v4l2_subdev_internal_ops gt97xx_int_ops = {
+	.open = gt97xx_open,
+	.close = gt97xx_close,
+};
+
+static const struct v4l2_subdev_core_ops gt97xx_core_ops = {
+	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
+	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
+};
+
+static const struct v4l2_subdev_ops gt97xx_ops = {
+	.core = &gt97xx_core_ops,
+};
+
+static int gt97xx_init_controls(struct gt97xx *gt97xx)
+{
+	struct v4l2_ctrl_handler *hdl = &gt97xx->ctrls;
+	const struct v4l2_ctrl_ops *ops = &gt97xx_ctrl_ops;
+
+	v4l2_ctrl_handler_init(hdl, 1);
+
+	gt97xx->focus = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_FOCUS_ABSOLUTE, 0,
+					  GT97XX_MAX_FOCUS_POS,
+					  GT97XX_FOCUS_STEPS, 0);
+
+	if (hdl->error)
+		return hdl->error;
+
+	gt97xx->sd.ctrl_handler = hdl;
+
+	return 0;
+}
+
+static int gt97xx_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct gt97xx *gt97xx;
+	unsigned int i;
+	int ret;
+
+	gt97xx = devm_kzalloc(dev, sizeof(*gt97xx), GFP_KERNEL);
+	if (!gt97xx)
+		return -ENOMEM;
+
+	gt97xx->regmap = devm_cci_regmap_init_i2c(client, 8);
+	if (IS_ERR(gt97xx->regmap))
+		return dev_err_probe(dev, PTR_ERR(gt97xx->regmap),
+				     "failed to init CCI\n");
+
+	/* Initialize subdev */
+	v4l2_i2c_subdev_init(&gt97xx->sd, client, &gt97xx_ops);
+
+	gt97xx->chip = of_device_get_match_data(dev);
+
+	gt97xx->aac_mode = GT97XX_AAC_MODE_DEFAULT;
+	gt97xx->aac_timing = GT97XX_AAC_TIME_DEFAULT;
+	gt97xx->clock_presc = GT97XX_CLOCK_PRE_SCALE_DEFAULT;
+
+	/* Optional indication of AAC mode select */
+	fwnode_property_read_u32(dev_fwnode(dev), "giantec,aac-mode",
+				 &gt97xx->aac_mode);
+
+	/* Optional indication of clock pre-scale select */
+	fwnode_property_read_u32(dev_fwnode(dev), "giantec,clock-presc",
+				 &gt97xx->clock_presc);
+
+	/* Optional indication of AAC Timing */
+	fwnode_property_read_u32(dev_fwnode(dev), "giantec,aac-timing",
+				 &gt97xx->aac_timing);
+
+	gt97xx->move_delay_us = gt97xx_cal_move_delay(gt97xx->aac_mode,
+						      gt97xx->clock_presc,
+						      gt97xx->aac_timing);
+
+	for (i = 0; i < ARRAY_SIZE(gt97xx_supply_names); i++)
+		gt97xx->supplies[i].supply = gt97xx_supply_names[i];
+
+	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(gt97xx_supply_names),
+				      gt97xx->supplies);
+	if (ret < 0)
+		return dev_err_probe(dev, ret,
+				     "failed to get regulators\n");
+
+	/* Initialize controls */
+	ret = gt97xx_init_controls(gt97xx);
+	if (ret)
+		goto err_free_handler;
+
+	/* Initialize subdev */
+	gt97xx->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	gt97xx->sd.internal_ops = &gt97xx_int_ops;
+	gt97xx->sd.entity.function = MEDIA_ENT_F_LENS;
+
+	ret = media_entity_pads_init(&gt97xx->sd.entity, 0, NULL);
+	if (ret < 0)
+		goto err_free_handler;
+
+	/*power on and Initialize hw*/
+	ret = gt97xx_runtime_resume(dev);
+	if (ret < 0) {
+		dev_err(dev, "failed to power on: %d\n", ret);
+		goto err_clean_entity;
+	}
+
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+	pm_runtime_set_autosuspend_delay(dev, 1000);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_idle(dev);
+
+	ret = v4l2_async_register_subdev(&gt97xx->sd);
+	if (ret < 0) {
+		dev_err(dev, "failed to register V4L2 subdev: %d", ret);
+		goto err_power_off;
+	}
+
+	return 0;
+
+err_power_off:
+	pm_runtime_disable(dev);
+err_clean_entity:
+	media_entity_cleanup(&gt97xx->sd.entity);
+err_free_handler:
+	v4l2_ctrl_handler_free(&gt97xx->ctrls);
+
+	return ret;
+}
+
+static void gt97xx_remove(struct i2c_client *client)
+{
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct gt97xx *gt97xx = sd_to_gt97xx(sd);
+
+	v4l2_async_unregister_subdev(&gt97xx->sd);
+	v4l2_ctrl_handler_free(&gt97xx->ctrls);
+	media_entity_cleanup(&gt97xx->sd.entity);
+	pm_runtime_disable(&client->dev);
+	if (!pm_runtime_status_suspended(&client->dev))
+		gt97xx_runtime_suspend(&client->dev);
+	pm_runtime_set_suspended(&client->dev);
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(gt97xx_pm_ops,
+				 gt97xx_runtime_suspend,
+				 gt97xx_runtime_resume,
+				 NULL);
+
+static const struct vcm_giantec_of_data gt9768_data = {
+	.id = GT9768_ID,
+	.regs[GT_IC_INFO_REG] = GT97XX_IC_INFO_REG,
+	.regs[GT_RING_PD_CONTROL_REG] = GT97XX_RING_PD_CONTROL_REG,
+	.regs[GT_MSB_ADDR_REG] = GT97XX_MSB_ADDR_REG,
+	.regs[GT_AAC_PRESC_REG] = GT97XX_AAC_PRESC_REG,
+	.regs[GT_AAC_TIME_REG] = GT97XX_AAC_TIME_REG,
+};
+
+static const struct vcm_giantec_of_data gt9769_data = {
+	.id = GT9769_ID,
+	.regs[GT_IC_INFO_REG] = GT97XX_IC_INFO_REG,
+	.regs[GT_RING_PD_CONTROL_REG] = GT97XX_RING_PD_CONTROL_REG,
+	.regs[GT_MSB_ADDR_REG] = GT97XX_MSB_ADDR_REG,
+	.regs[GT_AAC_PRESC_REG] = GT97XX_AAC_PRESC_REG,
+	.regs[GT_AAC_TIME_REG] = GT97XX_AAC_TIME_REG,
+};
+
+static const struct of_device_id gt97xx_of_table[] = {
+	{ .compatible = "giantec,gt9768", .data = &gt9768_data },
+	{ .compatible = "giantec,gt9769", .data = &gt9769_data },
+	{}
+};
+MODULE_DEVICE_TABLE(of, gt97xx_of_table);
+
+static struct i2c_driver gt97xx_i2c_driver = {
+	.driver = {
+		.name = GT97XX_NAME,
+		.pm = pm_ptr(&gt97xx_pm_ops),
+		.of_match_table = gt97xx_of_table,
+	},
+	.probe = gt97xx_probe,
+	.remove = gt97xx_remove,
+};
+module_i2c_driver(gt97xx_i2c_driver);
+
+MODULE_AUTHOR("Zhi Mao <zhi.mao@mediatek.com>");
+MODULE_DESCRIPTION("GT97xx VCM driver");
+MODULE_LICENSE("GPL");
-- 
2.25.1

Re: [PATCH 1/2] media: dt-bindings: i2c: add Giantec GT97xx VCM driver

[Conor Dooley]

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

Hey,

On Wed, Apr 10, 2024 at 06:40:01PM +0800, Zhi Mao wrote:
> Add YAML device tree binding for GT97xx VCM driver,

Please don't mention drivers here, bindings are for hardware.

> and the relevant MAINTAINERS entries.
> 
> Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
> ---
>  .../bindings/media/i2c/giantec,gt97xx.yaml    | 91 +++++++++++++++++++
>  1 file changed, 91 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml b/Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml
> new file mode 100644
> index 000000000000..8c9f1eb4dac8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml
> @@ -0,0 +1,91 @@
> +# 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/giantec,gt97xx.yaml#

Filename patching compatible please.

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Giantec Semiconductor, Crop. GT97xx Voice Coil Motor (VCM)
> +
> +maintainers:
> +  - Zhi Mao <zhi.mao@mediatek.com>
> +
> +description: |-
> +  The Giantec GT97xx is a 10-bit DAC with current sink capability.
> +  The DAC is controlled via I2C bus that operates at clock rates up to 1MHz.
> +  This chip integrates Advanced Actuator Control (AAC) technology
> +  and is intended for driving voice coil lens in camera modules.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - giantec,gt9768 # for GT9768 VCM
> +      - giantec,gt9769 # for GT9769 VCM

I don't think these comments are needed, they should be clear from the
compatibles, no?

> +
> +  reg:
> +    maxItems: 1
> +
> +  vin-supply: true
> +
> +  vdd-supply: true
> +
> +  giantec,aac-mode:
> +    description:
> +      Indication of AAC mode select.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum:
> +      - 1    #  AAC2 mode(operation time# 0.48 x Tvib)
> +      - 2    #  AAC3 mode(operation time# 0.70 x Tvib)
> +      - 3    #  AAC4 mode(operation time# 0.75 x Tvib)
> +      - 5    #  AAC8 mode(operation time# 1.13 x Tvib)

I dislike these enum based properties and I would rather this either be
the values themselves (0.48, 0.70 etc).

> +    default: 2
> +
> +  giantec,aac-timing:
> +    description:
> +      Number of AAC Timing count that controlled by one 6-bit period of
> +      vibration register AACT[5:0], the unit of which is 100 us.

Then the property should be in a standard unit of time, not "random" hex
numbers that correspond to register values.

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    default: 0x20
> +    minimum: 0x00
> +    maximum: 0x3f
> +
> +  giantec,clock-presc:
> +    description:
> +      Indication of VCM internal clock dividing rate select, as one multiple
> +      factor to calculate VCM ring periodic time Tvib.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum:
> +      - 0    #  Dividing Rate -  2
> +      - 1    #  Dividing Rate -  1
> +      - 2    #  Dividing Rate -  1/2
> +      - 3    #  Dividing Rate -  1/4
> +      - 4    #  Dividing Rate -  8
> +      - 5    #  Dividing Rate -  4

Same here, you should not need these comments explaining the values, use
an enum with meaningful values please. 

> +    default: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - vin-supply
> +  - vdd-supply
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        vcm@c {

"vcm" is not a generic node-name, can you use one please?
Look around similar bindings or at the dt spec for generic node-names.

Thanks,
Conor.

> +            compatible = "giantec,gt9768";
> +            reg = <0x0c>;
> +
> +            vin-supply = <&gt97xx_vin>;
> +            vdd-supply = <&gt97xx_vdd>;
> +            giantec,aac-timing = <0x20>;
> +        };
> +    };
> +
> +...
> -- 
> 2.25.1
> 

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

Re: [PATCH 1/2] media: dt-bindings: i2c: add Giantec GT97xx VCM driver

[Sakari Ailus]

Hi Conor, Zhi,

On Wed, Apr 10, 2024 at 12:27:07PM +0100, Conor Dooley wrote:
> Hey,
> 
> On Wed, Apr 10, 2024 at 06:40:01PM +0800, Zhi Mao wrote:
> > Add YAML device tree binding for GT97xx VCM driver,
> 
> Please don't mention drivers here, bindings are for hardware.
> 
> > and the relevant MAINTAINERS entries.
> > 
> > Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
> > ---
> >  .../bindings/media/i2c/giantec,gt97xx.yaml    | 91 +++++++++++++++++++
> >  1 file changed, 91 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml b/Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml
> > new file mode 100644
> > index 000000000000..8c9f1eb4dac8
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml
> > @@ -0,0 +1,91 @@
> > +# 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/giantec,gt97xx.yaml#
> 
> Filename patching compatible please.
> 
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Giantec Semiconductor, Crop. GT97xx Voice Coil Motor (VCM)
> > +
> > +maintainers:
> > +  - Zhi Mao <zhi.mao@mediatek.com>
> > +
> > +description: |-
> > +  The Giantec GT97xx is a 10-bit DAC with current sink capability.
> > +  The DAC is controlled via I2C bus that operates at clock rates up to 1MHz.
> > +  This chip integrates Advanced Actuator Control (AAC) technology
> > +  and is intended for driving voice coil lens in camera modules.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - giantec,gt9768 # for GT9768 VCM
> > +      - giantec,gt9769 # for GT9769 VCM
> 
> I don't think these comments are needed, they should be clear from the
> compatibles, no?
> 
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  vin-supply: true
> > +
> > +  vdd-supply: true
> > +
> > +  giantec,aac-mode:
> > +    description:
> > +      Indication of AAC mode select.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum:
> > +      - 1    #  AAC2 mode(operation time# 0.48 x Tvib)
> > +      - 2    #  AAC3 mode(operation time# 0.70 x Tvib)
> > +      - 3    #  AAC4 mode(operation time# 0.75 x Tvib)
> > +      - 5    #  AAC8 mode(operation time# 1.13 x Tvib)
> 
> I dislike these enum based properties and I would rather this either be
> the values themselves (0.48, 0.70 etc).
> 
> > +    default: 2
> > +
> > +  giantec,aac-timing:
> > +    description:
> > +      Number of AAC Timing count that controlled by one 6-bit period of
> > +      vibration register AACT[5:0], the unit of which is 100 us.
> 
> Then the property should be in a standard unit of time, not "random" hex
> numbers that correspond to register values.
> 
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    default: 0x20
> > +    minimum: 0x00
> > +    maximum: 0x3f
> > +
> > +  giantec,clock-presc:
> > +    description:
> > +      Indication of VCM internal clock dividing rate select, as one multiple
> > +      factor to calculate VCM ring periodic time Tvib.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum:
> > +      - 0    #  Dividing Rate -  2
> > +      - 1    #  Dividing Rate -  1
> > +      - 2    #  Dividing Rate -  1/2
> > +      - 3    #  Dividing Rate -  1/4
> > +      - 4    #  Dividing Rate -  8
> > +      - 5    #  Dividing Rate -  4
> 
> Same here, you should not need these comments explaining the values, use
> an enum with meaningful values please. 
> 
> > +    default: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - vin-supply
> > +  - vdd-supply
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        vcm@c {
> 
> "vcm" is not a generic node-name, can you use one please?
> Look around similar bindings or at the dt spec for generic node-names.

"camera-lens" would seem to be widely used. I can post patches to change
some of the rest out there that aren't aligned.

> 
> Thanks,
> Conor.
> 
> > +            compatible = "giantec,gt9768";
> > +            reg = <0x0c>;
> > +
> > +            vin-supply = <&gt97xx_vin>;
> > +            vdd-supply = <&gt97xx_vdd>;
> > +            giantec,aac-timing = <0x20>;
> > +        };
> > +    };
> > +
> > +...

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 2/2] media: i2c: Add GT97xx VCM driver

[Andy Shevchenko]

On Wed, Apr 10, 2024 at 1:40 PM Zhi Mao <zhi.mao@mediatek.com> wrote:
>
> Add a V4L2 sub-device driver for Giantec GT97xx VCM.

...

> +/*
> + * 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
> + * gt97xx requires waiting time of Topr after PD reset takes place.
> + */
> +#define GT97XX_RING_PD_CONTROL_REG CCI_REG8(0x02)

> +#define GT97XX_PD_MODE_OFF 0x00

Okay, this seems to be bitmapped, but do you really need to have this
separate definition?

> +#define GT97XX_PD_MODE_EN BIT(0)
> +#define GT97XX_AAC_MODE_EN BIT(1)

...

> +#define GT97XX_TVIB_MS_BASE10 (64 - 1)

Should it be (BIT(6) - 1) ?

...

> +#define GT97XX_AAC_MODE_DEFAULT 2
> +#define GT97XX_AAC_TIME_DEFAULT 0x20

Some are decimal, some are hexadecimal, but look to me like bitfields.

...

> +#define GT97XX_MAX_FOCUS_POS (1024 - 1)

(BIT(10) - 1) ?

...

> +enum vcm_giantec_reg_desc {
> +       GT_IC_INFO_REG,
> +       GT_RING_PD_CONTROL_REG,
> +       GT_MSB_ADDR_REG,
> +       GT_AAC_PRESC_REG,
> +       GT_AAC_TIME_REG,

> +       GT_MAX_REG,

No comma for the terminator.

> +};

...

> +static u32 gt97xx_find_ot_multi(u32 aac_mode_param)
> +{
> +       u32 cur_ot_multi_base100 = 70;
> +       unsigned int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(aac_mode_ot_multi); i++) {
> +               if (aac_mode_ot_multi[i].aac_mode_enum == aac_mode_param) {
> +                       cur_ot_multi_base100 =
> +                               aac_mode_ot_multi[i].ot_multi_base100;
> +               }

No break / return here?

> +       }
> +
> +       return cur_ot_multi_base100;
> +}
> +
> +static u32 gt97xx_find_dividing_rate(u32 presc_param)

Same comments as per above function.

...

> +static inline u32 gt97xx_cal_move_delay(u32 aac_mode_param, u32 presc_param,
> +                                       u32 aac_timing_param)

Why inline?

...

> +       return tvib_us * ot_multi_base100 / 100;

HECTO ?

...

> +       val = ((unsigned char)read_val & ~mask) | (val & mask);

Why casting?

...

> +static int gt97xx_power_on(struct device *dev)
> +{
> +       struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +       struct gt97xx *gt97xx = sd_to_gt97xx(sd);
> +       int ret;
> +
> +       ret = regulator_bulk_enable(ARRAY_SIZE(gt97xx_supply_names),
> +                                   gt97xx->supplies);
> +       if (ret < 0) {
> +               dev_err(dev, "failed to enable regulators\n");

> +               return ret;

Dup.

> +       }
> +
> +       return ret;
> +}
> +
> +static int gt97xx_power_off(struct device *dev)
> +{
> +       struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +       struct gt97xx *gt97xx = sd_to_gt97xx(sd);
> +       int ret;
> +
> +       ret = regulator_bulk_disable(ARRAY_SIZE(gt97xx_supply_names),
> +                                    gt97xx->supplies);
> +       if (ret < 0) {
> +               dev_err(dev, "failed to disable regulators\n");

> +               return ret;

Ditto.

> +       }
> +
> +       return ret;
> +}

...

> +static int gt97xx_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> +{
> +       return pm_runtime_resume_and_get(sd->dev);
> +}
> +
> +static int gt97xx_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> +{
> +       return pm_runtime_put(sd->dev);
> +}

Hmm... Shouldn't v4l2 take care about these (PM calls)?

...

> +       gt97xx->chip = of_device_get_match_data(dev);

device_get_match_data()

...

> +       fwnode_property_read_u32(dev_fwnode(dev), "giantec,aac-mode",
> +                                &gt97xx->aac_mode);

No, use device_property_read_u32() directly.

...

> +       fwnode_property_read_u32(dev_fwnode(dev), "giantec,clock-presc",
> +                                &gt97xx->clock_presc);

Ditto.

...

> +       fwnode_property_read_u32(dev_fwnode(dev), "giantec,aac-timing",
> +                                &gt97xx->aac_timing);

Ditto.

...

> +       /*power on and Initialize hw*/

Missing spaces (and capitalisation?).

...

> +       ret = gt97xx_runtime_resume(dev);
> +       if (ret < 0) {
> +               dev_err(dev, "failed to power on: %d\n", ret);

Use dev_err_probe() to match the style of the messages.

> +               goto err_clean_entity;
> +       }

...

> +       ret = v4l2_async_register_subdev(&gt97xx->sd);
> +       if (ret < 0) {
> +               dev_err(dev, "failed to register V4L2 subdev: %d", ret);

Ditto.

> +               goto err_power_off;
> +       }

--
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] media: dt-bindings: i2c: add Giantec GT97xx VCM driver

[Rob Herring]

On Wed, Apr 10, 2024 at 12:27:07PM +0100, Conor Dooley wrote:
> Hey,
> 
> On Wed, Apr 10, 2024 at 06:40:01PM +0800, Zhi Mao wrote:
> > Add YAML device tree binding for GT97xx VCM driver,
> 
> Please don't mention drivers here, bindings are for hardware.
> 
> > and the relevant MAINTAINERS entries.
> > 
> > Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
> > ---
> >  .../bindings/media/i2c/giantec,gt97xx.yaml    | 91 +++++++++++++++++++
> >  1 file changed, 91 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml b/Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml
> > new file mode 100644
> > index 000000000000..8c9f1eb4dac8
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml
> > @@ -0,0 +1,91 @@
> > +# 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/giantec,gt97xx.yaml#
> 
> Filename patching compatible please.
> 
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Giantec Semiconductor, Crop. GT97xx Voice Coil Motor (VCM)
> > +
> > +maintainers:
> > +  - Zhi Mao <zhi.mao@mediatek.com>
> > +
> > +description: |-
> > +  The Giantec GT97xx is a 10-bit DAC with current sink capability.
> > +  The DAC is controlled via I2C bus that operates at clock rates up to 1MHz.
> > +  This chip integrates Advanced Actuator Control (AAC) technology
> > +  and is intended for driving voice coil lens in camera modules.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - giantec,gt9768 # for GT9768 VCM
> > +      - giantec,gt9769 # for GT9769 VCM
> 
> I don't think these comments are needed, they should be clear from the
> compatibles, no?
> 
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  vin-supply: true
> > +
> > +  vdd-supply: true
> > +
> > +  giantec,aac-mode:
> > +    description:
> > +      Indication of AAC mode select.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum:
> > +      - 1    #  AAC2 mode(operation time# 0.48 x Tvib)
> > +      - 2    #  AAC3 mode(operation time# 0.70 x Tvib)
> > +      - 3    #  AAC4 mode(operation time# 0.75 x Tvib)
> > +      - 5    #  AAC8 mode(operation time# 1.13 x Tvib)
> 
> I dislike these enum based properties and I would rather this either be
> the values themselves (0.48, 0.70 etc).

Except that those would have to be strings for floats or fractions. For 
properties which have little chance of being something common and aren't 
any form of standard unit, I think it is fine to just use the h/w 
specific values. 

The first question to ask whether these parameters are common to 
all/many voice coil motors?


> > +    default: 2
> > +
> > +  giantec,aac-timing:
> > +    description:
> > +      Number of AAC Timing count that controlled by one 6-bit period of
> > +      vibration register AACT[5:0], the unit of which is 100 us.
> 
> Then the property should be in a standard unit of time, not "random" hex
> numbers that correspond to register values.

Here, I agree.

Rob

Re: [PATCH 1/2] media: dt-bindings: i2c: add Giantec GT97xx VCM driver

[Zhi Mao (毛智)]

Hi Conor,

Thanks for your review.

On Wed, 2024-04-10 at 12:27 +0100, Conor Dooley wrote:
> > 
> > 
> Hey,
> 
> On Wed, Apr 10, 2024 at 06:40:01PM +0800, Zhi Mao wrote:
> > b/Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml
> > @@ -0,0 +1,91 @@
> > +# 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/giantec,gt97xx.yaml#
> 
> Filename patching compatible please.
> 
> 
Sorry, I don't catch this point. 
Can you explain more details? 
> > 
> > 
> > +
> > +  giantec,aac-mode:
> > +    description:
> > +      Indication of AAC mode select.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum:
> > +      - 1    #  AAC2 mode(operation time# 0.48 x Tvib)
> > +      - 2    #  AAC3 mode(operation time# 0.70 x Tvib)
> > +      - 3    #  AAC4 mode(operation time# 0.75 x Tvib)
> > +      - 5    #  AAC8 mode(operation time# 1.13 x Tvib)
> 
> I dislike these enum based properties and I would rather this either
> be
> the values themselves (0.48, 0.70 etc).
> 
> > +
> > +  giantec,aac-timing:
> > +    description:
> > +      Number of AAC Timing count that controlled by one 6-bit
> > period of
> > +      vibration register AACT[5:0], the unit of which is 100 us.
> 
> Then the property should be in a standard unit of time, not "random"
> hex
> numbers that correspond to register values.
> 
> > 
> > +  giantec,clock-presc:
> > +    description:
> > +      Indication of VCM internal clock dividing rate select, as
> > one multiple
> > +      factor to calculate VCM ring periodic time Tvib.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum:
> > +      - 0    #  Dividing Rate -  2
> > +      - 1    #  Dividing Rate -  1
> > +      - 2    #  Dividing Rate -  1/2
> > +      - 3    #  Dividing Rate -  1/4
> > +      - 4    #  Dividing Rate -  8
> > +      - 5    #  Dividing Rate -  4
> 
> Same here, you should not need these comments explaining the values,
> use
> an enum with meaningful values please. 
> 
About "aac-mode/aac-timing/clock-presc", we test this driver with
default settings accroding to SPEC and VCM works well, so I will not
export these property in YMAL and let driver use default settings.
How do you think about it?
  

> Thanks,
> Conor.
> 
> > 
> > 

Re: [PATCH 1/2] media: dt-bindings: i2c: add Giantec GT97xx VCM driver

[Krzysztof Kozlowski]

On 11/04/2024 04:04, Zhi Mao (毛智) wrote:
> Hi Conor,
> 
> Thanks for your review.
> 
> On Wed, 2024-04-10 at 12:27 +0100, Conor Dooley wrote:
>>>
>>>
>> Hey,
>>
>> On Wed, Apr 10, 2024 at 06:40:01PM +0800, Zhi Mao wrote:
>>> b/Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml
>>> @@ -0,0 +1,91 @@
>>> +# 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/giantec,gt97xx.yaml#
>>
>> Filename patching compatible please.
>>
>>
> Sorry, I don't catch this point. 
> Can you explain more details? 

s/patching/matching/
Use compatible as filename.


>>>
>>>
>>> +
>>> +  giantec,aac-mode:
>>> +    description:
>>> +      Indication of AAC mode select.
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    enum:
>>> +      - 1    #  AAC2 mode(operation time# 0.48 x Tvib)
>>> +      - 2    #  AAC3 mode(operation time# 0.70 x Tvib)
>>> +      - 3    #  AAC4 mode(operation time# 0.75 x Tvib)
>>> +      - 5    #  AAC8 mode(operation time# 1.13 x Tvib)
>>
>> I dislike these enum based properties and I would rather this either
>> be
>> the values themselves (0.48, 0.70 etc).
>>
>>> +
>>> +  giantec,aac-timing:
>>> +    description:
>>> +      Number of AAC Timing count that controlled by one 6-bit
>>> period of
>>> +      vibration register AACT[5:0], the unit of which is 100 us.
>>
>> Then the property should be in a standard unit of time, not "random"
>> hex
>> numbers that correspond to register values.
>>
>>>
>>> +  giantec,clock-presc:
>>> +    description:
>>> +      Indication of VCM internal clock dividing rate select, as
>>> one multiple
>>> +      factor to calculate VCM ring periodic time Tvib.
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    enum:
>>> +      - 0    #  Dividing Rate -  2
>>> +      - 1    #  Dividing Rate -  1
>>> +      - 2    #  Dividing Rate -  1/2
>>> +      - 3    #  Dividing Rate -  1/4
>>> +      - 4    #  Dividing Rate -  8
>>> +      - 5    #  Dividing Rate -  4
>>
>> Same here, you should not need these comments explaining the values,
>> use
>> an enum with meaningful values please. 
>>
> About "aac-mode/aac-timing/clock-presc", we test this driver with
> default settings accroding to SPEC and VCM works well, so I will not
> export these property in YMAL and let driver use default settings.
> How do you think about it?

You must remove them from the driver code in such case.

Best regards,
Krzysztof

Re: [PATCH 2/2] media: i2c: Add GT97xx VCM driver

[Sakari Ailus]

Hi Andy, Zhi,

On Wed, Apr 10, 2024 at 07:00:02PM +0300, Andy Shevchenko wrote:
> > +static int gt97xx_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> > +{
> > +       return pm_runtime_resume_and_get(sd->dev);
> > +}
> > +
> > +static int gt97xx_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> > +{
> > +       return pm_runtime_put(sd->dev);
> > +}
> 
> Hmm... Shouldn't v4l2 take care about these (PM calls)?

Ideally yes. We don't have a good mechanism for this at the moment as the
lens isn't part of the image pipeline. Non-data links may be used for this
in the future but that's not implemented yet.

-- 
Regards,

Sakari Ailus

Re: [PATCH 2/2] media: i2c: Add GT97xx VCM driver

[Andy Shevchenko]

On Fri, Apr 12, 2024 at 12:39 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
> On Wed, Apr 10, 2024 at 07:00:02PM +0300, Andy Shevchenko wrote:
> > > +static int gt97xx_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> > > +{
> > > +       return pm_runtime_resume_and_get(sd->dev);
> > > +}
> > > +
> > > +static int gt97xx_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> > > +{
> > > +       return pm_runtime_put(sd->dev);
> > > +}
> >
> > Hmm... Shouldn't v4l2 take care about these (PM calls)?
>
> Ideally yes. We don't have a good mechanism for this at the moment as the
> lens isn't part of the image pipeline. Non-data links may be used for this
> in the future but that's not implemented yet.

Aren't you using devlinks? It was designed exactly to make sure that
the PM chain of calls goes in the correct order.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] media: i2c: Add GT97xx VCM driver

[Sakari Ailus]

Hi Andy,

On Fri, Apr 12, 2024 at 04:43:43PM +0300, Andy Shevchenko wrote:
> On Fri, Apr 12, 2024 at 12:39 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> > On Wed, Apr 10, 2024 at 07:00:02PM +0300, Andy Shevchenko wrote:
> > > > +static int gt97xx_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> > > > +{
> > > > +       return pm_runtime_resume_and_get(sd->dev);
> > > > +}
> > > > +
> > > > +static int gt97xx_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> > > > +{
> > > > +       return pm_runtime_put(sd->dev);
> > > > +}
> > >
> > > Hmm... Shouldn't v4l2 take care about these (PM calls)?
> >
> > Ideally yes. We don't have a good mechanism for this at the moment as the
> > lens isn't part of the image pipeline. Non-data links may be used for this
> > in the future but that's not implemented yet.
> 
> Aren't you using devlinks? It was designed exactly to make sure that
> the PM chain of calls goes in the correct order.

Device links are already used by the IPU bridge, but in the other
direction: the VCM requires the sensor to be powered up in this case.

-- 
Regards,

Sakari Ailus

Re: [PATCH 2/2] media: i2c: Add GT97xx VCM driver

[Zhi Mao (毛智)]

Hi Andy,

Thanks for your review.

On Wed, 2024-04-10 at 19:00 +0300, Andy Shevchenko wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On Wed, Apr 10, 2024 at 1:40 PM Zhi Mao <zhi.mao@mediatek.com>
> wrote:
> >
> > Add a V4L2 sub-device driver for Giantec GT97xx VCM.
> 
> ...
> 
> > +/*
> > + * 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
> > + * gt97xx requires waiting time of Topr after PD reset takes
> place.
> > + */
> > +#define GT97XX_RING_PD_CONTROL_REG CCI_REG8(0x02)
> 
> > +#define GT97XX_PD_MODE_OFF 0x00
> 
> Okay, this seems to be bitmapped, but do you really need to have this
> separate definition?
> 
> > +#define GT97XX_PD_MODE_EN BIT(0)
> > +#define GT97XX_AAC_MODE_EN BIT(1)
> 
> ...
> 
> > +#define GT97XX_TVIB_MS_BASE10 (64 - 1)
> 
> Should it be (BIT(6) - 1) ?
> 
> ...
> 
> > +#define GT97XX_AAC_MODE_DEFAULT 2
> > +#define GT97XX_AAC_TIME_DEFAULT 0x20
> 
> Some are decimal, some are hexadecimal, but look to me like
> bitfields.
> 
Some "aac-mode/aac-timing/clock-presc" control function were removed,
so these related defines were also removed.

> ...
> 
> > +#define GT97XX_MAX_FOCUS_POS (1024 - 1)
> 
> (BIT(10) - 1) ?
> 
fixed in patch:v1.
> ...
> 
> > +enum vcm_giantec_reg_desc {
> > +       GT_IC_INFO_REG,
> > +       GT_RING_PD_CONTROL_REG,
> > +       GT_MSB_ADDR_REG,
> > +       GT_AAC_PRESC_REG,
> > +       GT_AAC_TIME_REG,
> 
> > +       GT_MAX_REG,
> 
> No comma for the terminator.
> 
fixed in patch:v1.
> > +};
> 
> ...
> 
> > +static u32 gt97xx_find_ot_multi(u32 aac_mode_param)
> > +{
> > +       u32 cur_ot_multi_base100 = 70;
> > +       unsigned int i;
> > +
> > +       for (i = 0; i < ARRAY_SIZE(aac_mode_ot_multi); i++) {
> > +               if (aac_mode_ot_multi[i].aac_mode_enum ==
> aac_mode_param) {
> > +                       cur_ot_multi_base100 =
> >
> +                               aac_mode_ot_multi[i].ot_multi_base100
> ;
> > +               }
> 
> No break / return here?
fixed in patch:v1.
> 
> > +       }
> > +
> > +       return cur_ot_multi_base100;
> > +}
> > +
> > +static u32 gt97xx_find_dividing_rate(u32 presc_param)
> 
> Same comments as per above function.
> 
> ...
> 
> > +static inline u32 gt97xx_cal_move_delay(u32 aac_mode_param, u32
> presc_param,
> > +                                       u32 aac_timing_param)
> 
> Why inline?
> 
> ...
> 
> > +       return tvib_us * ot_multi_base100 / 100;
> 
> HECTO ?
> 
> ...
> 
> > +       val = ((unsigned char)read_val & ~mask) | (val & mask);
> 
> Why casting?
> 
Some "aac-mode/aac-timing/clock-presc" related control function were
removed.
> ...
> 
> > +static int gt97xx_power_on(struct device *dev)
> > +{
> > +       struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > +       struct gt97xx *gt97xx = sd_to_gt97xx(sd);
> > +       int ret;
> > +
> > +       ret =
> regulator_bulk_enable(ARRAY_SIZE(gt97xx_supply_names),
> > +                                   gt97xx->supplies);
> > +       if (ret < 0) {
> > +               dev_err(dev, "failed to enable regulators\n");
> 
> > +               return ret;
> 
> Dup.
fixed in patch:v1.
> 
> > +       }
> > +
> > +       return ret;
> > +}
> > +
> > +static int gt97xx_power_off(struct device *dev)
> > +{
> > +       struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > +       struct gt97xx *gt97xx = sd_to_gt97xx(sd);
> > +       int ret;
> > +
> > +       ret =
> regulator_bulk_disable(ARRAY_SIZE(gt97xx_supply_names),
> > +                                    gt97xx->supplies);
> > +       if (ret < 0) {
> > +               dev_err(dev, "failed to disable regulators\n");
> 
> > +               return ret;
> 
> Ditto.
fixed in patch:v1.
> 
> > +       }
> > +
> > +       return ret;
> > +}
> 
> ...
> 
> > +static int gt97xx_open(struct v4l2_subdev *sd, struct
> v4l2_subdev_fh *fh)
> > +{
> > +       return pm_runtime_resume_and_get(sd->dev);
> > +}
> > +
> > +static int gt97xx_close(struct v4l2_subdev *sd, struct
> v4l2_subdev_fh *fh)
> > +{
> > +       return pm_runtime_put(sd->dev);
> > +}
> 
> Hmm... Shouldn't v4l2 take care about these (PM calls)?
> 
Accoring to disscussion in another thread, there is not a good
mechanism at present, so I keep this method. 
> ...
> 
> > +       gt97xx->chip = of_device_get_match_data(dev);
> 
> device_get_match_data()
> 
> ...
> 
> > +       fwnode_property_read_u32(dev_fwnode(dev), "giantec,aac-
> mode",
> > +                                &gt97xx->aac_mode);
> 
> No, use device_property_read_u32() directly.
> 
> ...
> 
> > +       fwnode_property_read_u32(dev_fwnode(dev), "giantec,clock-
> presc",
> > +                                &gt97xx->clock_presc);
> 
> Ditto.
> 
> ...
> 
> > +       fwnode_property_read_u32(dev_fwnode(dev), "giantec,aac-
> timing",
> > +                                &gt97xx->aac_timing);
> 
> Ditto.
> 
Some "aac-mode/aac-timing/clock-presc" related control function were
removed.
> ...
> 
> > +       /*power on and Initialize hw*/
> 
> Missing spaces (and capitalisation?).
> 
fixed in patch:v1.
> ...
> 
> > +       ret = gt97xx_runtime_resume(dev);
> > +       if (ret < 0) {
> > +               dev_err(dev, "failed to power on: %d\n", ret);
> 
> Use dev_err_probe() to match the style of the messages.
> 
fixed in patch:v1.
> > +               goto err_clean_entity;
> > +       }
> 
> ...
> 
> > +       ret = v4l2_async_register_subdev(&gt97xx->sd);
> > +       if (ret < 0) {
> > +               dev_err(dev, "failed to register V4L2 subdev: %d",
> ret);
> 
> Ditto.
fixed in patch:v1.
> 
> > +               goto err_power_off;
> > +       }
> 
> --
> With Best Regards,
> Andy Shevchenko