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

[PATCH v3 0/3] 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

Previous versions of this patch-set can be found here:
v2:https://lore.kernel.org/all/20240427012613.6621-1-zhi.mao@mediatek.com/
v1:https://lore.kernel.org/all/20240420011840.23148-1-zhi.mao@mediatek.com/
v0:https://lore.kernel.org/all/20240410104002.1197-1-zhi.mao@mediatek.com/

This series is based on linux-next, tag: next-20240611
Changes in v3:
- Add maintainer entry for GT97xx VCM driver

Thanks

Zhi Mao (3):
  media: dt-bindings: i2c: add Giantec GT97xx VCM
  media: i2c: Add GT97xx VCM driver
  MAINTAINERS: Add entry for GT97xx VCM driver

 .../bindings/media/i2c/giantec,gt9769.yaml    |  55 +++
 MAINTAINERS                                   |   8 +
 drivers/media/i2c/Kconfig                     |  13 +
 drivers/media/i2c/Makefile                    |   1 +
 drivers/media/i2c/gt97xx.c                    | 436 ++++++++++++++++++
 5 files changed, 513 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/giantec,gt9769.yaml
 create mode 100644 drivers/media/i2c/gt97xx.c

-- 
2.25.1

[PATCH v3 1/3] media: dt-bindings: i2c: add Giantec GT97xx VCM

[Zhi Mao]

Add YAML device tree binding for GT9768 & GT9769 VCM,
and the relevant MAINTAINERS entries.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
---
 .../bindings/media/i2c/giantec,gt9769.yaml    | 55 +++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/giantec,gt9769.yaml

diff --git a/Documentation/devicetree/bindings/media/i2c/giantec,gt9769.yaml b/Documentation/devicetree/bindings/media/i2c/giantec,gt9769.yaml
new file mode 100644
index 000000000000..92d603acc53c
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/giantec,gt9769.yaml
@@ -0,0 +1,55 @@
+# 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,gt9769.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Giantec Semiconductor, Crop. GT9768 & GT9769 Voice Coil Motor (VCM)
+
+maintainers:
+  - Zhi Mao <zhi.mao@mediatek.com>
+
+description:
+  The Giantec GT9768 & GT9769 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
+      - giantec,gt9769
+
+  reg:
+    maxItems: 1
+
+  vin-supply: true
+
+  vdd-supply: true
+
+required:
+  - compatible
+  - reg
+  - vin-supply
+  - vdd-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        camera-lens@c {
+            compatible = "giantec,gt9769";
+            reg = <0x0c>;
+
+            vin-supply = <&gt97xx_vin>;
+            vdd-supply = <&gt97xx_vdd>;
+        };
+    };
+
+...
-- 
2.25.1

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

[AngeloGioacchino Del Regno]

Il 12/06/24 03:20, Zhi Mao ha scritto:
> Add YAML device tree binding for GT9768 & GT9769 VCM,
> and the relevant MAINTAINERS entries.
> 
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
> ---
>   .../bindings/media/i2c/giantec,gt9769.yaml    | 55 +++++++++++++++++++
>   1 file changed, 55 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/media/i2c/giantec,gt9769.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/giantec,gt9769.yaml b/Documentation/devicetree/bindings/media/i2c/giantec,gt9769.yaml
> new file mode 100644
> index 000000000000..92d603acc53c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/giantec,gt9769.yaml
> @@ -0,0 +1,55 @@
> +# 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,gt9769.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Giantec Semiconductor, Crop. GT9768 & GT9769 Voice Coil Motor (VCM)
> +
> +maintainers:
> +  - Zhi Mao <zhi.mao@mediatek.com>
> +
> +description:
> +  The Giantec GT9768 & GT9769 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

Can't you simply add the "giantec,gt9768" compatible to dongwoon,dw9768.yaml?

Regards,
Angelo

[PATCH v3 2/3] 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 | 436 +++++++++++++++++++++++++++++++++++++
 3 files changed, 450 insertions(+)
 create mode 100644 drivers/media/i2c/gt97xx.c

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 5498128773c7..9976f02575a3 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -772,6 +772,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 d1403e3273ca..0009d3d6d6af 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..ccae190ffba6
--- /dev/null
+++ b/drivers/media/i2c/gt97xx.c
@@ -0,0 +1,436 @@
+// 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
+ * requires waiting time 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)
+
+/*
+ * DAC is a 10bit address to control the VCM position.
+ * DAC_MSB: D[9:8] (ADD: 0x03)
+ * DAC_LSB: D[7:0] (ADD: 0x04)
+ */
+#define GT97XX_DAC_ADDR_REG CCI_REG16(0x03)
+
+#define GT97XX_MOVE_STEPS 16
+#define GT97XX_MAX_FOCUS_POS (BIT(10) - 1)
+
+#define GT97XX_SLEEP_US (1 * USEC_PER_MSEC)
+
+enum vcm_giantec_reg_desc {
+	GT_IC_INFO_REG,
+	GT_RING_PD_CONTROL_REG,
+	GT_DAC_ADDR_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",
+	"vdd",
+};
+
+/* 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;
+
+	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;
+};
+
+static int gt97xx_set_dac(struct gt97xx *gt97xx, u16 val)
+{
+	/* Write VCM position to registers */
+	return cci_write(gt97xx->regmap,
+			 gt97xx->chip->regs[GT_DAC_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 PD_CONTROL */
+	ret = cci_write(gt97xx->regmap,
+			gt97xx->chip->regs[GT_RING_PD_CONTROL_REG],
+			GT97XX_PD_MODE_OFF, NULL);
+	if (ret < 0)
+		return ret;
+
+	/* Need waiting delay time after PD reset */
+	fsleep(GT97XX_SLEEP_US);
+
+	/* Enable ACC mode */
+	ret = cci_write(gt97xx->regmap,
+			gt97xx->chip->regs[GT_RING_PD_CONTROL_REG],
+			GT97XX_AAC_MODE_EN, 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_SLEEP_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_SLEEP_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;
+}
+
+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;
+}
+
+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;
+	}
+
+	/* Need waited before sending I2C commands after power-up */
+	fsleep(GT97XX_SLEEP_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,
+					  1, 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 = device_get_match_data(dev);
+
+	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 hardware */
+	ret = gt97xx_runtime_resume(dev);
+	if (ret < 0) {
+		dev_err_probe(dev, ret, "failed to power on\n");
+		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_probe(dev, ret, "failed to register V4L2 subdev\n");
+		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_DAC_ADDR_REG] = GT97XX_DAC_ADDR_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_DAC_ADDR_REG] = GT97XX_DAC_ADDR_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 v3 2/3] media: i2c: Add GT97xx VCM driver

[AngeloGioacchino Del Regno]

Il 12/06/24 03:20, Zhi Mao ha scritto:
> Add a V4L2 sub-device driver for Giantec GT97xx VCM.
> 
> Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>

Hello Zhi,

I fail to see why would you need to upstream this new driver instead of
simply adding the IC_INFO_REG to the already existing (and more featureful)
dw9768 driver, which also seems to support the Giantec GT9769 VCM.

Cheers,
Angelo

> ---
>   drivers/media/i2c/Kconfig  |  13 ++
>   drivers/media/i2c/Makefile |   1 +
>   drivers/media/i2c/gt97xx.c | 436 +++++++++++++++++++++++++++++++++++++
>   3 files changed, 450 insertions(+)
>   create mode 100644 drivers/media/i2c/gt97xx.c
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 5498128773c7..9976f02575a3 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -772,6 +772,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 d1403e3273ca..0009d3d6d6af 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..ccae190ffba6
> --- /dev/null
> +++ b/drivers/media/i2c/gt97xx.c
> @@ -0,0 +1,436 @@
> +// 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
> + * requires waiting time 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)
> +
> +/*
> + * DAC is a 10bit address to control the VCM position.
> + * DAC_MSB: D[9:8] (ADD: 0x03)
> + * DAC_LSB: D[7:0] (ADD: 0x04)
> + */
> +#define GT97XX_DAC_ADDR_REG CCI_REG16(0x03)
> +
> +#define GT97XX_MOVE_STEPS 16
> +#define GT97XX_MAX_FOCUS_POS (BIT(10) - 1)
> +
> +#define GT97XX_SLEEP_US (1 * USEC_PER_MSEC)
> +
> +enum vcm_giantec_reg_desc {
> +	GT_IC_INFO_REG,
> +	GT_RING_PD_CONTROL_REG,
> +	GT_DAC_ADDR_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",
> +	"vdd",
> +};
> +
> +/* 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;
> +
> +	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;
> +};
> +
> +static int gt97xx_set_dac(struct gt97xx *gt97xx, u16 val)
> +{
> +	/* Write VCM position to registers */
> +	return cci_write(gt97xx->regmap,
> +			 gt97xx->chip->regs[GT_DAC_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 PD_CONTROL */
> +	ret = cci_write(gt97xx->regmap,
> +			gt97xx->chip->regs[GT_RING_PD_CONTROL_REG],
> +			GT97XX_PD_MODE_OFF, NULL);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Need waiting delay time after PD reset */
> +	fsleep(GT97XX_SLEEP_US);
> +
> +	/* Enable ACC mode */
> +	ret = cci_write(gt97xx->regmap,
> +			gt97xx->chip->regs[GT_RING_PD_CONTROL_REG],
> +			GT97XX_AAC_MODE_EN, 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_SLEEP_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_SLEEP_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;
> +}
> +
> +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;
> +}
> +
> +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;
> +	}
> +
> +	/* Need waited before sending I2C commands after power-up */
> +	fsleep(GT97XX_SLEEP_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,
> +					  1, 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 = device_get_match_data(dev);
> +
> +	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 hardware */
> +	ret = gt97xx_runtime_resume(dev);
> +	if (ret < 0) {
> +		dev_err_probe(dev, ret, "failed to power on\n");
> +		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_probe(dev, ret, "failed to register V4L2 subdev\n");
> +		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_DAC_ADDR_REG] = GT97XX_DAC_ADDR_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_DAC_ADDR_REG] = GT97XX_DAC_ADDR_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");

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

[Zhi Mao (毛智)]

Hi Angelo,

Thanks for your review.

On Wed, 2024-06-12 at 09:07 +0200, AngeloGioacchino Del Regno wrote:
> Il 12/06/24 03:20, Zhi Mao ha scritto:
> > Add a V4L2 sub-device driver for Giantec GT97xx VCM.
> > 
> > Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
> 
> Hello Zhi,
> 
> I fail to see why would you need to upstream this new driver instead
> of
> simply adding the IC_INFO_REG to the already existing (and more
> featureful)
> dw9768 driver, which also seems to support the Giantec GT9769 VCM.
> 

Our project uses Giantec VCM hardware. 
For detailed vendor information, please visit: (
https://en.giantec-semi.com/yqmd/164). 
The VCM datasheet we are referencing is provided by Giantec. 
Currently, the relationship between Giantec VCM and Dongwoon VCM is
unclear, but Dongwoon seems to be another manufacturer of VCM
hardware. 

From the perspective of software driver development and maintenance, it
makes sense for each vendor's hardware should have its own software
driver.
So, I upstream a new VCM driver for Giantec.

> Cheers,
> Angelo
> 
> > 
> 
> 

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

[Kieran Bingham]

Hi Zhi,

Also - Cc: Dongchun Zhu <dongchun.zhu@mediatek.com> who is listed as the
DW9768 VCM driver author...

Quoting Zhi Mao (毛智) (2024-06-12 12:13:40)
> Hi Angelo,
> 
> Thanks for your review.
> 
> On Wed, 2024-06-12 at 09:07 +0200, AngeloGioacchino Del Regno wrote:
> > Il 12/06/24 03:20, Zhi Mao ha scritto:
> > > Add a V4L2 sub-device driver for Giantec GT97xx VCM.
> > > 
> > > Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
> > 
> > Hello Zhi,
> > 
> > I fail to see why would you need to upstream this new driver instead
> > of
> > simply adding the IC_INFO_REG to the already existing (and more
> > featureful)
> > dw9768 driver, which also seems to support the Giantec GT9769 VCM.

Even more so especially as
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/i2c/dw9768.c
already directly supports the compatible strings added by this driver -
surely we don't want multiple (near identical) drivers matching the same
compatible string?

> > 
> 
> Our project uses Giantec VCM hardware. 
> For detailed vendor information, please visit: (
> https://en.giantec-semi.com/yqmd/164). 
> The VCM datasheet we are referencing is provided by Giantec. 
> Currently, the relationship between Giantec VCM and Dongwoon VCM is
> unclear, but Dongwoon seems to be another manufacturer of VCM
> hardware. 
> 
> From the perspective of software driver development and maintenance, it
> makes sense for each vendor's hardware should have its own software
> driver.

Personally, I don't think so. If two vendors make identical parts, we
shouldn't have two identical drivers.

I still have plans to refactor VCM drivers if I get some spare-time(tm)
as almost each driver does the same identical task. They're all just
copies of the boilerplate.  That seems like something we should reduce,
not increase.

--
Kieran


> So, I upstream a new VCM driver for Giantec.
> 
> > Cheers,
> > Angelo

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

[Andy Shevchenko]

On Thu, Jun 13, 2024 at 2:05 AM Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
> Also - Cc: Dongchun Zhu <dongchun.zhu@mediatek.com> who is listed as the
> DW9768 VCM driver author...
> Quoting Zhi Mao (毛智) (2024-06-12 12:13:40)
> > On Wed, 2024-06-12 at 09:07 +0200, AngeloGioacchino Del Regno wrote:

...

> > Our project uses Giantec VCM hardware.
> > For detailed vendor information, please visit: (
> > https://en.giantec-semi.com/yqmd/164).
> > The VCM datasheet we are referencing is provided by Giantec.
> > Currently, the relationship between Giantec VCM and Dongwoon VCM is
> > unclear, but Dongwoon seems to be another manufacturer of VCM
> > hardware.

There may be plenty of manufacturers of the same/similar IPs, but it's
not an excuse to have a duplication like this.

> > From the perspective of software driver development and maintenance, it
> > makes sense for each vendor's hardware should have its own software
> > driver.
>
> Personally, I don't think so. If two vendors make identical parts, we
> shouldn't have two identical drivers.

Exactly! That's why we have compatible strings or other means of
reusing the same code base as much as possible. This in particular
reduces maintenance costs (of all means!) _a lot_.

> I still have plans to refactor VCM drivers if I get some spare-time(tm)
> as almost each driver does the same identical task. They're all just
> copies of the boilerplate.  That seems like something we should reduce,
> not increase.


-- 
With Best Regards,
Andy Shevchenko

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

[sakari.ailus]

Hi Andy, others,

On Thu, Jun 13, 2024 at 09:51:03PM +0200, Andy Shevchenko wrote:
> On Thu, Jun 13, 2024 at 2:05 AM Kieran Bingham
> <kieran.bingham@ideasonboard.com> wrote:
> > Also - Cc: Dongchun Zhu <dongchun.zhu@mediatek.com> who is listed as the
> > DW9768 VCM driver author...
> > Quoting Zhi Mao (毛智) (2024-06-12 12:13:40)
> > > On Wed, 2024-06-12 at 09:07 +0200, AngeloGioacchino Del Regno wrote:
> 
> ...
> 
> > > Our project uses Giantec VCM hardware.
> > > For detailed vendor information, please visit: (
> > > https://en.giantec-semi.com/yqmd/164).
> > > The VCM datasheet we are referencing is provided by Giantec.
> > > Currently, the relationship between Giantec VCM and Dongwoon VCM is
> > > unclear, but Dongwoon seems to be another manufacturer of VCM
> > > hardware.
> 
> There may be plenty of manufacturers of the same/similar IPs, but it's
> not an excuse to have a duplication like this.
> 
> > > From the perspective of software driver development and maintenance, it
> > > makes sense for each vendor's hardware should have its own software
> > > driver.
> >
> > Personally, I don't think so. If two vendors make identical parts, we
> > shouldn't have two identical drivers.
> 
> Exactly! That's why we have compatible strings or other means of
> reusing the same code base as much as possible. This in particular
> reduces maintenance costs (of all means!) _a lot_.

Indeed. I'll mark these thus "rejected" in Patchwork.

If there's something that needs to be handled differently for the other
VCM, that can be taken into account in the driver.

-- 
Kind regards,

Sakari Ailus

[PATCH v3 3/3] MAINTAINERS: Add entry for GT97xx VCM driver

[Zhi Mao]

Add maintainer entry for GT97xx VCM driver

Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
---
 MAINTAINERS | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 670b8201973b..d12694c743ab 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9328,6 +9328,13 @@ F:	Documentation/filesystems/gfs2*
 F:	fs/gfs2/
 F:	include/uapi/linux/gfs2_ondisk.h
 
+GIANTEC GT9769 LENS VOICE COIL DRIVER
+M:	Zhi Mao <zhi.mao@mediatek.com>
+L:	linux-media@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/media/i2c/giantec,gt9769.yaml
+F:	drivers/media/i2c/gt97xx.c
+
 GIGABYTE WATERFORCE SENSOR DRIVER
 M:	Aleksa Savic <savicaleksa83@gmail.com>
 L:	linux-hwmon@vger.kernel.org
@@ -23590,6 +23597,7 @@ L:	linux-media@vger.kernel.org
 S:	Maintained
 F:	drivers/media/i2c/ak*
 F:	drivers/media/i2c/dw*
+F:	drivers/media/i2c/gt*
 F:	drivers/media/i2c/lm*
 
 V4L2 CAMERA SENSOR DRIVERS
-- 
2.25.1