[PATCH 0/5] media: i2c: dw9719: add DT compatible and DW9718S support

[PATCH 0/5] media: i2c: dw9719: add DT compatible and DW9718S support

[Val Packett]

The DW9718S voice coil motor is found on various smartphones like motorola-nora
that are currently being worked on in the postmarketOS community. Since the way
it operates is very similar to DW9719, this patch series adds support for it to
the existing dw9719 driver. Because that driver did not yet support DT, we also
add DT bindings and the dongwoon,dw9719 ofw compatible first. With DW9718S, the
driver was tested fully, including runtime PM.

Val Packett (5):
  media: dt-bindings: i2c: add DW9719/DW9718S VCM binding
  media: i2c: dw9719: add an of_match_table
  media: i2c: dw9719: increase tOPR wait time
  media: i2c: dw9719: update PM last busy time upon close
  media: i2c: dw9719: add support for dw9718s

 .../bindings/media/i2c/dongwoon,dw9719.yaml   | 110 ++++++++++++++++
 MAINTAINERS                                   |   1 +
 drivers/media/i2c/dw9719.c                    | 118 ++++++++++++++++--
 3 files changed, 220 insertions(+), 9 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/dongwoon,dw9719.yaml

-- 
2.48.1

[PATCH 1/5] media: dt-bindings: i2c: add DW9719/DW9718S VCM binding

[Val Packett]

Add DT bindings for the dw9719 voice coil motor driver, which is getting
devicetree compatibles added along with DW9718S support.

Also mention the binding file in the corresponding MAINTAINERS entry.

Signed-off-by: Val Packett <val@packett.cool>
---
 .../bindings/media/i2c/dongwoon,dw9719.yaml   | 110 ++++++++++++++++++
 MAINTAINERS                                   |   1 +
 2 files changed, 111 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/dongwoon,dw9719.yaml

diff --git a/Documentation/devicetree/bindings/media/i2c/dongwoon,dw9719.yaml b/Documentation/devicetree/bindings/media/i2c/dongwoon,dw9719.yaml
new file mode 100644
index 000000000000..88161038223f
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/dongwoon,dw9719.yaml
@@ -0,0 +1,110 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/dongwoon,dw9719.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Dongwoon Anatech DW9719 Voice Coil Motor (VCM) DAC
+
+maintainers:
+  - Daniel Scally <djrscally@gmail.com>
+
+description: |-
+  The Dongwoon DW9719/DW9718S is a single 10-bit digital-to-analog converter
+  with 100 mA output current sink capability, designed for linear control of
+  voice coil motors (VCM) in camera lenses. This chip provides a Smart Actuator
+  Control (SAC) mode intended for driving voice coil lenses in camera modules.
+
+properties:
+  compatible:
+    enum:
+      - dongwoon,dw9719
+      - dongwoon,dw9718s
+
+  reg:
+    maxItems: 1
+
+  vdd-supply:
+    description: VDD power supply
+
+  dongwoon,sac-mode:
+    description: |
+      Slew Rate Control mode to use: direct, LSC (Linear Slope Control) or
+      SAC1-SAC6 (Smart Actuator Control).
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum:
+      - 0    # Direct mode
+      - 1    # LSC mode
+      - 2    # SAC1 mode (operation time# 0.32 x Tvib)
+      - 3    # SAC2 mode (operation time# 0.48 x Tvib)
+      - 4    # SAC3 mode (operation time# 0.72 x Tvib)
+      - 5    # SAC4 mode (operation time# 1.20 x Tvib)
+      - 6    # SAC5 mode (operation time# 1.64 x Tvib)
+      - 7    # SAC6 mode (operation time# 1.88 x Tvib)
+    default: 4
+
+  dongwoon,vcm-freq:
+    description:
+      The switching frequency for the voice coil motor.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: dongwoon,dw9718s
+    then:
+      properties:
+        dongwoon,vcm-freq:
+          default: 0
+          enum:
+            - 0    # 5.00 MHz
+            - 1    # 3.33 MHz
+            - 2    # 2.50 MHz
+            - 3    # 2.00 MHz
+            - 4    # 1.67 MHz
+            - 5    # 1.43 MHz
+            - 6    # 1.25 MHz
+            - 7    # 1.11 MHz
+            - 8    # 1.00 MHz
+            - 9    # 0.91 MHz
+            - 10   # 0.83 MHz
+            - 11   # 0.77 MHz
+            - 12   # 0.71 MHz
+            - 13   # 0.67 MHz
+            - 14   # 0.63 MHz
+            - 15   # 0.59 MHz
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: dongwoon,dw9719
+    then:
+      properties:
+        dongwoon,vcm-freq:
+          default: 0x60
+
+required:
+  - compatible
+  - reg
+  - vdd-supply
+
+additionalProperties: false
+
+examples:
+  - |
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        vcm_rear: camera-lens@c {
+            compatible = "dongwoon,dw9718s";
+            reg = <0x0c>;
+
+            vdd-supply = <&pm8937_l17>;
+        };
+    };
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 603b11222d67..42dd86f5d5c8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6932,6 +6932,7 @@ M:	Daniel Scally <djrscally@gmail.com>
 L:	linux-media@vger.kernel.org
 S:	Maintained
 T:	git git://linuxtv.org/media.git
+F:	Documentation/devicetree/bindings/media/i2c/dongwoon,dw9719.yaml
 F:	drivers/media/i2c/dw9719.c
 
 DONGWOON DW9768 LENS VOICE COIL DRIVER
-- 
2.48.1

[PATCH 2/5] media: i2c: dw9719: add an of_match_table

[Val Packett]

Allow this driver to be used from device trees using the
dongwoon,dw9719 OFW compatible string.

Signed-off-by: Val Packett <val@packett.cool>
---
 drivers/media/i2c/dw9719.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/media/i2c/dw9719.c b/drivers/media/i2c/dw9719.c
index c626ed845928..b6859cfd216c 100644
--- a/drivers/media/i2c/dw9719.c
+++ b/drivers/media/i2c/dw9719.c
@@ -331,6 +331,12 @@ static const struct i2c_device_id dw9719_id_table[] = {
 };
 MODULE_DEVICE_TABLE(i2c, dw9719_id_table);
 
+static const struct of_device_id dw9719_of_table[] = {
+	{ .compatible = "dongwoon,dw9719" },
+	{ { 0 } }
+};
+MODULE_DEVICE_TABLE(of, dw9719_of_table);
+
 static DEFINE_RUNTIME_DEV_PM_OPS(dw9719_pm_ops, dw9719_suspend, dw9719_resume,
 				 NULL);
 
@@ -338,6 +344,7 @@ static struct i2c_driver dw9719_i2c_driver = {
 	.driver = {
 		.name = "dw9719",
 		.pm = pm_sleep_ptr(&dw9719_pm_ops),
+		.of_match_table = dw9719_of_table,
 	},
 	.probe = dw9719_probe,
 	.remove = dw9719_remove,
-- 
2.48.1

[PATCH 3/5] media: i2c: dw9719: increase tOPR wait time

[Val Packett]

Use usleep_range and a named define, and increase from 100 to 200us
because in practice 100us turns out to be too low, particularly after
a suspend-resume cycle.

Signed-off-by: Val Packett <val@packett.cool>
---
 drivers/media/i2c/dw9719.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/dw9719.c b/drivers/media/i2c/dw9719.c
index b6859cfd216c..f2cf3bcd4dd3 100644
--- a/drivers/media/i2c/dw9719.c
+++ b/drivers/media/i2c/dw9719.c
@@ -20,6 +20,8 @@
 #define DW9719_MAX_FOCUS_POS	1023
 #define DW9719_CTRL_STEPS	16
 #define DW9719_CTRL_DELAY_US	1000
+/* 100 us is not enough on resume */
+#define DW9719_T_OPR_US				200
 
 #define DW9719_INFO			CCI_REG8(0)
 #define DW9719_ID			0xF1
@@ -85,8 +87,8 @@ static int dw9719_power_up(struct dw9719_device *dw9719)
 	/* Jiggle SCL pin to wake up device */
 	cci_write(dw9719->regmap, DW9719_CONTROL, 1, &ret);
 
-	/* Need 100us to transit from SHUTDOWN to STANDBY */
-	fsleep(100);
+	/* Need tOPR to transition from SHUTDOWN to STANDBY */
+	usleep_range(DW9719_T_OPR_US, DW9719_T_OPR_US + 10);
 
 	cci_write(dw9719->regmap, DW9719_CONTROL, DW9719_ENABLE_RINGING, &ret);
 	cci_write(dw9719->regmap, DW9719_MODE,
-- 
2.48.1

[PATCH 4/5] media: i2c: dw9719: update PM last busy time upon close

[Val Packett]

Update the close callback to match other similar drivers like dw9768.

Signed-off-by: Val Packett <val@packett.cool>
---
 drivers/media/i2c/dw9719.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/dw9719.c b/drivers/media/i2c/dw9719.c
index f2cf3bcd4dd3..74a57c2f59ae 100644
--- a/drivers/media/i2c/dw9719.c
+++ b/drivers/media/i2c/dw9719.c
@@ -188,7 +188,8 @@ static int dw9719_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 
 static int dw9719_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 {
-	pm_runtime_put(sd->dev);
+	pm_runtime_mark_last_busy(sd->dev);
+	__pm_runtime_put_autosuspend(sd->dev);
 
 	return 0;
 }
-- 
2.48.1

[PATCH 5/5] media: i2c: dw9719: add support for dw9718s

[Val Packett]

The DW9718S is a similar part that uses a different register set but
follows the same method of operation otherwise. Add support for it
to the existing dw9719 driver. While here, ensure suspend-resume works.

Tested on the Moto E5 (motorola-nora) smartphone.

Signed-off-by: Val Packett <val@packett.cool>
---
 drivers/media/i2c/dw9719.c | 104 ++++++++++++++++++++++++++++++++++---
 1 file changed, 97 insertions(+), 7 deletions(-)

diff --git a/drivers/media/i2c/dw9719.c b/drivers/media/i2c/dw9719.c
index 74a57c2f59ae..4a07684af52e 100644
--- a/drivers/media/i2c/dw9719.c
+++ b/drivers/media/i2c/dw9719.c
@@ -23,6 +23,22 @@
 /* 100 us is not enough on resume */
 #define DW9719_T_OPR_US				200
 
+#define DW9718_CONTROL			CCI_REG8(1)
+#define DW9718_CONTROL_SW_LINEAR	BIT(0)
+#define DW9718_CONTROL_SAC_SHIFT		1
+#define DW9718_CONTROL_SAC_MASK		0x7
+#define DW9718_CONTROL_OCP_DISABLE	BIT(4)
+#define DW9718_CONTROL_UVLO_DISABLE	BIT(5)
+
+#define DW9718_VCM_CURRENT		CCI_REG16(2)
+
+#define DW9718_SW			CCI_REG8(4)
+#define DW9718_SW_VCM_FREQ_MASK	0xF
+#define DW9718_DEFAULT_VCM_FREQ	0
+
+#define DW9718_SACT			CCI_REG8(5)
+#define DW9718_SACT_PERIOD_8_8MS	0x19
+
 #define DW9719_INFO			CCI_REG8(0)
 #define DW9719_ID			0xF1
 
@@ -48,12 +64,25 @@ struct dw9719_device {
 	u32 sac_mode;
 	u32 vcm_freq;
 
+	const struct dw9719_cfg {
+		int reg_current;
+		int default_vcm_freq;
+		int (*power_up)(struct dw9719_device *dw9719);
+		int (*detect)(struct dw9719_device *dw9719);
+	} *cfg;
+
 	struct dw9719_v4l2_ctrls {
 		struct v4l2_ctrl_handler handler;
 		struct v4l2_ctrl *focus;
 	} ctrls;
 };
 
+static int dw9718_detect(struct dw9719_device *dw9719)
+{
+	/* Unfortunately, there is no ID register */
+	return 0;
+}
+
 static int dw9719_detect(struct dw9719_device *dw9719)
 {
 	int ret;
@@ -73,9 +102,50 @@ static int dw9719_detect(struct dw9719_device *dw9719)
 
 static int dw9719_power_down(struct dw9719_device *dw9719)
 {
+	int ret;
+
+	cci_write(dw9719->regmap, DW9718_CONTROL, 1, &ret);
+	if (ret)
+		return ret;
+
+	/* Need tOPR to transition from STANDBY to SHUTDOWN */
+	usleep_range(DW9719_T_OPR_US, DW9719_T_OPR_US + 10);
+
 	return regulator_disable(dw9719->regulator);
 }
 
+static int dw9718_power_up(struct dw9719_device *dw9719)
+{
+	int ret;
+
+	ret = regulator_enable(dw9719->regulator);
+	if (ret)
+		return ret;
+
+	/* Need tOPR to transition from SHUTDOWN to STANDBY */
+	usleep_range(DW9719_T_OPR_US, DW9719_T_OPR_US + 10);
+
+	/* Datasheet says [OCP/UVLO] should be disabled below 2.5V */
+	cci_write(dw9719->regmap, DW9718_CONTROL,
+			     DW9718_CONTROL_SW_LINEAR |
+			     ((dw9719->sac_mode & DW9718_CONTROL_SAC_MASK)
+			      << DW9718_CONTROL_SAC_SHIFT) |
+			     DW9718_CONTROL_OCP_DISABLE |
+			     DW9718_CONTROL_UVLO_DISABLE,
+			     &ret);
+	cci_write(dw9719->regmap, DW9718_SACT,
+			     DW9718_SACT_PERIOD_8_8MS,
+			     &ret);
+	cci_write(dw9719->regmap, DW9718_SW,
+			     dw9719->vcm_freq & DW9718_SW_VCM_FREQ_MASK,
+			     &ret);
+
+	if (ret)
+		dw9719_power_down(dw9719);
+
+	return ret;
+}
+
 static int dw9719_power_up(struct dw9719_device *dw9719)
 {
 	int ret;
@@ -103,7 +173,7 @@ static int dw9719_power_up(struct dw9719_device *dw9719)
 
 static int dw9719_t_focus_abs(struct dw9719_device *dw9719, s32 value)
 {
-	return cci_write(dw9719->regmap, DW9719_VCM_CURRENT, value, NULL);
+	return cci_write(dw9719->regmap, dw9719->cfg->reg_current, value, NULL);
 }
 
 static int dw9719_set_ctrl(struct v4l2_ctrl *ctrl)
@@ -161,7 +231,7 @@ static int dw9719_resume(struct device *dev)
 	int ret;
 	int val;
 
-	ret = dw9719_power_up(dw9719);
+	ret = dw9719->cfg->power_up(dw9719);
 	if (ret)
 		return ret;
 
@@ -235,13 +305,17 @@ static int dw9719_probe(struct i2c_client *client)
 	if (!dw9719)
 		return -ENOMEM;
 
+	dw9719->cfg = i2c_get_match_data(client);
+	if (!dw9719->cfg)
+		return -ENODEV;
+
 	dw9719->regmap = devm_cci_regmap_init_i2c(client, 8);
 	if (IS_ERR(dw9719->regmap))
 		return PTR_ERR(dw9719->regmap);
 
 	dw9719->dev = &client->dev;
 	dw9719->sac_mode = DW9719_MODE_SAC3;
-	dw9719->vcm_freq = DW9719_DEFAULT_VCM_FREQ;
+	dw9719->vcm_freq = dw9719->cfg->default_vcm_freq;
 
 	/* Optional indication of SAC mode select */
 	device_property_read_u32(&client->dev, "dongwoon,sac-mode",
@@ -277,11 +351,11 @@ static int dw9719_probe(struct i2c_client *client)
 	 * will work.
 	 */
 
-	ret = dw9719_power_up(dw9719);
+	ret = dw9719->cfg->power_up(dw9719);
 	if (ret)
 		goto err_cleanup_media;
 
-	ret = dw9719_detect(dw9719);
+	ret = dw9719->cfg->detect(dw9719);
 	if (ret)
 		goto err_powerdown;
 
@@ -328,14 +402,30 @@ static void dw9719_remove(struct i2c_client *client)
 	pm_runtime_set_suspended(&client->dev);
 }
 
+static const struct dw9719_cfg dw9718_cfg = {
+	.reg_current = DW9718_VCM_CURRENT,
+	.default_vcm_freq = DW9718_DEFAULT_VCM_FREQ,
+	.power_up = dw9718_power_up,
+	.detect = dw9718_detect,
+};
+
+static const struct dw9719_cfg dw9719_cfg = {
+	.reg_current = DW9719_VCM_CURRENT,
+	.default_vcm_freq = DW9719_DEFAULT_VCM_FREQ,
+	.power_up = dw9719_power_up,
+	.detect = dw9719_detect,
+};
+
 static const struct i2c_device_id dw9719_id_table[] = {
-	{ "dw9719" },
+	{ "dw9718s", .driver_data = (kernel_ulong_t)&dw9718_cfg, },
+	{ "dw9719",  .driver_data = (kernel_ulong_t)&dw9719_cfg, },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, dw9719_id_table);
 
 static const struct of_device_id dw9719_of_table[] = {
-	{ .compatible = "dongwoon,dw9719" },
+	{ .compatible = "dongwoon,dw9718s", .data = &dw9718_cfg },
+	{ .compatible = "dongwoon,dw9719",  .data = &dw9719_cfg },
 	{ { 0 } }
 };
 MODULE_DEVICE_TABLE(of, dw9719_of_table);
-- 
2.48.1

Re: [PATCH 1/5] media: dt-bindings: i2c: add DW9719/DW9718S VCM binding

[Krzysztof Kozlowski]

On 10/02/2025 09:19, Val Packett wrote:
> Add DT bindings for the dw9719 voice coil motor driver, which is getting
> devicetree compatibles added along with DW9718S support.
> 
> Also mention the binding file in the corresponding MAINTAINERS entry.
> 
> Signed-off-by: Val Packett <val@packett.cool>
> ---
>  .../bindings/media/i2c/dongwoon,dw9719.yaml   | 110 ++++++++++++++++++
>  MAINTAINERS                                   |   1 +
>  2 files changed, 111 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/dongwoon,dw9719.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/dongwoon,dw9719.yaml b/Documentation/devicetree/bindings/media/i2c/dongwoon,dw9719.yaml
> new file mode 100644
> index 000000000000..88161038223f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/dongwoon,dw9719.yaml
> @@ -0,0 +1,110 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/dongwoon,dw9719.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Dongwoon Anatech DW9719 Voice Coil Motor (VCM) DAC
> +
> +maintainers:
> +  - Daniel Scally <djrscally@gmail.com>
> +
> +description: |-
> +  The Dongwoon DW9719/DW9718S is a single 10-bit digital-to-analog converter
> +  with 100 mA output current sink capability, designed for linear control of
> +  voice coil motors (VCM) in camera lenses. This chip provides a Smart Actuator
> +  Control (SAC) mode intended for driving voice coil lenses in camera modules.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - dongwoon,dw9719
> +      - dongwoon,dw9718s

Keep alphabetical order.

> +
> +  reg:
> +    maxItems: 1
> +
> +  vdd-supply:
> +    description: VDD power supply
> +
> +  dongwoon,sac-mode:
> +    description: |
> +      Slew Rate Control mode to use: direct, LSC (Linear Slope Control) or
> +      SAC1-SAC6 (Smart Actuator Control).
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum:
> +      - 0    # Direct mode
> +      - 1    # LSC mode
> +      - 2    # SAC1 mode (operation time# 0.32 x Tvib)
> +      - 3    # SAC2 mode (operation time# 0.48 x Tvib)
> +      - 4    # SAC3 mode (operation time# 0.72 x Tvib)
> +      - 5    # SAC4 mode (operation time# 1.20 x Tvib)
> +      - 6    # SAC5 mode (operation time# 1.64 x Tvib)
> +      - 7    # SAC6 mode (operation time# 1.88 x Tvib)
> +    default: 4
> +
> +  dongwoon,vcm-freq:
> +    description:
> +      The switching frequency for the voice coil motor.

Frequency is in Hertz, so use proper property unit suffix. BTW, you
cannot add incorrect properties post-factum based on already accepted
ACPI driver. This would be nice bypass of review, right?

> +    $ref: /schemas/types.yaml#/definitions/uint32

Drop.

minimum/maximum constraints

> +

No reset/powerdown gpios in the hardware?

Missing required block.

> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: dongwoon,dw9718s
> +    then:
> +      properties:
> +        dongwoon,vcm-freq:
> +          default: 0
> +          enum:
> +            - 0    # 5.00 MHz
> +            - 1    # 3.33 MHz
> +            - 2    # 2.50 MHz
> +            - 3    # 2.00 MHz
> +            - 4    # 1.67 MHz
> +            - 5    # 1.43 MHz
> +            - 6    # 1.25 MHz
> +            - 7    # 1.11 MHz
> +            - 8    # 1.00 MHz
> +            - 9    # 0.91 MHz
> +            - 10   # 0.83 MHz
> +            - 11   # 0.77 MHz
> +            - 12   # 0.71 MHz
> +            - 13   # 0.67 MHz
> +            - 14   # 0.63 MHz
> +            - 15   # 0.59 MHz
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: dongwoon,dw9719
> +    then:
> +      properties:
> +        dongwoon,vcm-freq:
> +          default: 0x60

Why no constraints? Why suddenly hex?
> +
> +required:
> +  - compatible
> +  - reg
> +  - vdd-supply

required always follows properties.

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +

Drop stray blank line

> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        vcm_rear: camera-lens@c {
> +            compatible = "dongwoon,dw9718s";
> +            reg = <0x0c>;
> +
> +            vdd-supply = <&pm8937_l17>;

Missing properties, make the example complete.

> +        };
> +    };
> +
> +...



Best regards,
Krzysztof

Re: [PATCH 1/5] media: dt-bindings: i2c: add DW9719/DW9718S VCM binding

[Sakari Ailus]

Hi Krzysztof,

On Mon, Feb 10, 2025 at 09:29:25AM +0100, Krzysztof Kozlowski wrote:
> > +  dongwoon,vcm-freq:
> > +    description:
> > +      The switching frequency for the voice coil motor.
> 
> Frequency is in Hertz, so use proper property unit suffix. BTW, you
> cannot add incorrect properties post-factum based on already accepted
> ACPI driver. This would be nice bypass of review, right?

What's actually configured here is the divisor (10 MHz clock, divisor seems
to be value + 2). It's similar to existing
Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml . I prefer
this as it's much easier to use that in a driver (think of having values
like 1428571 in DT, too).

> 
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> 
> Drop.
> 
> minimum/maximum constraints
> 
> > +
> 
> No reset/powerdown gpios in the hardware?
> 
> Missing required block.
> 
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: dongwoon,dw9718s
> > +    then:
> > +      properties:
> > +        dongwoon,vcm-freq:
> > +          default: 0
> > +          enum:
> > +            - 0    # 5.00 MHz
> > +            - 1    # 3.33 MHz
> > +            - 2    # 2.50 MHz
> > +            - 3    # 2.00 MHz
> > +            - 4    # 1.67 MHz
> > +            - 5    # 1.43 MHz
> > +            - 6    # 1.25 MHz
> > +            - 7    # 1.11 MHz
> > +            - 8    # 1.00 MHz
> > +            - 9    # 0.91 MHz
> > +            - 10   # 0.83 MHz
> > +            - 11   # 0.77 MHz
> > +            - 12   # 0.71 MHz
> > +            - 13   # 0.67 MHz
> > +            - 14   # 0.63 MHz
> > +            - 15   # 0.59 MHz
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: dongwoon,dw9719
> > +    then:
> > +      properties:
> > +        dongwoon,vcm-freq:
> > +          default: 0x60

-- 
Regards,

Sakari Ailus

Re: [PATCH 5/5] media: i2c: dw9719: add support for dw9718s

[Sakari Ailus]

Hi Val,

Thanks for the set!

I've cc'd André who's adding support for dw9761 in the same driver. Please
work together to get both changes in.

On Mon, Feb 10, 2025 at 05:19:20AM -0300, Val Packett wrote:
> The DW9718S is a similar part that uses a different register set but
> follows the same method of operation otherwise. Add support for it
> to the existing dw9719 driver. While here, ensure suspend-resume works.
> 
> Tested on the Moto E5 (motorola-nora) smartphone.
> 
> Signed-off-by: Val Packett <val@packett.cool>
> ---
>  drivers/media/i2c/dw9719.c | 104 ++++++++++++++++++++++++++++++++++---
>  1 file changed, 97 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/i2c/dw9719.c b/drivers/media/i2c/dw9719.c
> index 74a57c2f59ae..4a07684af52e 100644
> --- a/drivers/media/i2c/dw9719.c
> +++ b/drivers/media/i2c/dw9719.c
> @@ -23,6 +23,22 @@
>  /* 100 us is not enough on resume */
>  #define DW9719_T_OPR_US				200
>  
> +#define DW9718_CONTROL			CCI_REG8(1)
> +#define DW9718_CONTROL_SW_LINEAR	BIT(0)
> +#define DW9718_CONTROL_SAC_SHIFT		1
> +#define DW9718_CONTROL_SAC_MASK		0x7
> +#define DW9718_CONTROL_OCP_DISABLE	BIT(4)
> +#define DW9718_CONTROL_UVLO_DISABLE	BIT(5)
> +
> +#define DW9718_VCM_CURRENT		CCI_REG16(2)
> +
> +#define DW9718_SW			CCI_REG8(4)
> +#define DW9718_SW_VCM_FREQ_MASK	0xF
> +#define DW9718_DEFAULT_VCM_FREQ	0
> +
> +#define DW9718_SACT			CCI_REG8(5)
> +#define DW9718_SACT_PERIOD_8_8MS	0x19
> +
>  #define DW9719_INFO			CCI_REG8(0)
>  #define DW9719_ID			0xF1
>  
> @@ -48,12 +64,25 @@ struct dw9719_device {
>  	u32 sac_mode;
>  	u32 vcm_freq;
>  
> +	const struct dw9719_cfg {
> +		int reg_current;
> +		int default_vcm_freq;
> +		int (*power_up)(struct dw9719_device *dw9719);
> +		int (*detect)(struct dw9719_device *dw9719);

Could you instead use a pointer to the match data struct directly?

> +	} *cfg;
> +
>  	struct dw9719_v4l2_ctrls {
>  		struct v4l2_ctrl_handler handler;
>  		struct v4l2_ctrl *focus;
>  	} ctrls;
>  };
>  
> +static int dw9718_detect(struct dw9719_device *dw9719)
> +{
> +	/* Unfortunately, there is no ID register */
> +	return 0;
> +}
> +
>  static int dw9719_detect(struct dw9719_device *dw9719)
>  {
>  	int ret;
> @@ -73,9 +102,50 @@ static int dw9719_detect(struct dw9719_device *dw9719)
>  
>  static int dw9719_power_down(struct dw9719_device *dw9719)
>  {
> +	int ret;
> +
> +	cci_write(dw9719->regmap, DW9718_CONTROL, 1, &ret);
> +	if (ret)

Please just proceed and return 0 if power down fails. There's nothing the
caller can reasonably do about this. Feel free to complain though (e.g.
dev_err()).

> +		return ret;
> +
> +	/* Need tOPR to transition from STANDBY to SHUTDOWN */
> +	usleep_range(DW9719_T_OPR_US, DW9719_T_OPR_US + 10);
> +
>  	return regulator_disable(dw9719->regulator);
>  }
>  
> +static int dw9718_power_up(struct dw9719_device *dw9719)
> +{
> +	int ret;
> +
> +	ret = regulator_enable(dw9719->regulator);
> +	if (ret)
> +		return ret;
> +
> +	/* Need tOPR to transition from SHUTDOWN to STANDBY */
> +	usleep_range(DW9719_T_OPR_US, DW9719_T_OPR_US + 10);
> +
> +	/* Datasheet says [OCP/UVLO] should be disabled below 2.5V */
> +	cci_write(dw9719->regmap, DW9718_CONTROL,
> +			     DW9718_CONTROL_SW_LINEAR |
> +			     ((dw9719->sac_mode & DW9718_CONTROL_SAC_MASK)
> +			      << DW9718_CONTROL_SAC_SHIFT) |
> +			     DW9718_CONTROL_OCP_DISABLE |
> +			     DW9718_CONTROL_UVLO_DISABLE,
> +			     &ret);
> +	cci_write(dw9719->regmap, DW9718_SACT,
> +			     DW9718_SACT_PERIOD_8_8MS,
> +			     &ret);
> +	cci_write(dw9719->regmap, DW9718_SW,
> +			     dw9719->vcm_freq & DW9718_SW_VCM_FREQ_MASK,
> +			     &ret);
> +
> +	if (ret)
> +		dw9719_power_down(dw9719);
> +
> +	return ret;
> +}
> +
>  static int dw9719_power_up(struct dw9719_device *dw9719)
>  {
>  	int ret;
> @@ -103,7 +173,7 @@ static int dw9719_power_up(struct dw9719_device *dw9719)
>  
>  static int dw9719_t_focus_abs(struct dw9719_device *dw9719, s32 value)
>  {
> -	return cci_write(dw9719->regmap, DW9719_VCM_CURRENT, value, NULL);
> +	return cci_write(dw9719->regmap, dw9719->cfg->reg_current, value, NULL);
>  }
>  
>  static int dw9719_set_ctrl(struct v4l2_ctrl *ctrl)
> @@ -161,7 +231,7 @@ static int dw9719_resume(struct device *dev)
>  	int ret;
>  	int val;
>  
> -	ret = dw9719_power_up(dw9719);
> +	ret = dw9719->cfg->power_up(dw9719);
>  	if (ret)
>  		return ret;
>  
> @@ -235,13 +305,17 @@ static int dw9719_probe(struct i2c_client *client)
>  	if (!dw9719)
>  		return -ENOMEM;
>  
> +	dw9719->cfg = i2c_get_match_data(client);
> +	if (!dw9719->cfg)
> +		return -ENODEV;
> +
>  	dw9719->regmap = devm_cci_regmap_init_i2c(client, 8);
>  	if (IS_ERR(dw9719->regmap))
>  		return PTR_ERR(dw9719->regmap);
>  
>  	dw9719->dev = &client->dev;
>  	dw9719->sac_mode = DW9719_MODE_SAC3;
> -	dw9719->vcm_freq = DW9719_DEFAULT_VCM_FREQ;
> +	dw9719->vcm_freq = dw9719->cfg->default_vcm_freq;
>  
>  	/* Optional indication of SAC mode select */
>  	device_property_read_u32(&client->dev, "dongwoon,sac-mode",
> @@ -277,11 +351,11 @@ static int dw9719_probe(struct i2c_client *client)
>  	 * will work.
>  	 */
>  
> -	ret = dw9719_power_up(dw9719);
> +	ret = dw9719->cfg->power_up(dw9719);
>  	if (ret)
>  		goto err_cleanup_media;
>  
> -	ret = dw9719_detect(dw9719);
> +	ret = dw9719->cfg->detect(dw9719);
>  	if (ret)
>  		goto err_powerdown;
>  
> @@ -328,14 +402,30 @@ static void dw9719_remove(struct i2c_client *client)
>  	pm_runtime_set_suspended(&client->dev);
>  }
>  
> +static const struct dw9719_cfg dw9718_cfg = {
> +	.reg_current = DW9718_VCM_CURRENT,
> +	.default_vcm_freq = DW9718_DEFAULT_VCM_FREQ,
> +	.power_up = dw9718_power_up,
> +	.detect = dw9718_detect,
> +};
> +
> +static const struct dw9719_cfg dw9719_cfg = {
> +	.reg_current = DW9719_VCM_CURRENT,
> +	.default_vcm_freq = DW9719_DEFAULT_VCM_FREQ,
> +	.power_up = dw9719_power_up,
> +	.detect = dw9719_detect,
> +};
> +
>  static const struct i2c_device_id dw9719_id_table[] = {
> -	{ "dw9719" },
> +	{ "dw9718s", .driver_data = (kernel_ulong_t)&dw9718_cfg, },
> +	{ "dw9719",  .driver_data = (kernel_ulong_t)&dw9719_cfg, },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, dw9719_id_table);
>  
>  static const struct of_device_id dw9719_of_table[] = {
> -	{ .compatible = "dongwoon,dw9719" },
> +	{ .compatible = "dongwoon,dw9718s", .data = &dw9718_cfg },
> +	{ .compatible = "dongwoon,dw9719",  .data = &dw9719_cfg },
>  	{ { 0 } }
>  };
>  MODULE_DEVICE_TABLE(of, dw9719_of_table);

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 1/5] media: dt-bindings: i2c: add DW9719/DW9718S VCM binding

[Krzysztof Kozlowski]

On 10/02/2025 11:36, Sakari Ailus wrote:
> Hi Krzysztof,
> 
> On Mon, Feb 10, 2025 at 09:29:25AM +0100, Krzysztof Kozlowski wrote:
>>> +  dongwoon,vcm-freq:
>>> +    description:
>>> +      The switching frequency for the voice coil motor.
>>
>> Frequency is in Hertz, so use proper property unit suffix. BTW, you
>> cannot add incorrect properties post-factum based on already accepted
>> ACPI driver. This would be nice bypass of review, right?
> 
> What's actually configured here is the divisor (10 MHz clock, divisor seems
> to be value + 2). It's similar to existing
> Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml . I prefer
> this as it's much easier to use that in a driver (think of having values
> like 1428571 in DT, too).


Sure, but then this should be renamed to match purpose and description
rephrased.

> 
>>
>>> +    $ref: /schemas/types.yaml#/definitions/uint32


And this would stay + constraints.


Best regards,
Krzysztof

Re: [PATCH 1/5] media: dt-bindings: i2c: add DW9719/DW9718S VCM binding

[André Apitzsch]

Hi Krzysztof, hi Sakari,

Am Montag, dem 10.02.2025 um 12:04 +0100 schrieb Krzysztof Kozlowski:
> On 10/02/2025 11:36, Sakari Ailus wrote:
> > Hi Krzysztof,
> > 
> > On Mon, Feb 10, 2025 at 09:29:25AM +0100, Krzysztof Kozlowski
> > wrote:
> > > > +  dongwoon,vcm-freq:
> > > > +    description:
> > > > +      The switching frequency for the voice coil motor.
> > > 
> > > Frequency is in Hertz, so use proper property unit suffix. BTW,
> > > you cannot add incorrect properties post-factum based on already
> > > accepted ACPI driver. This would be nice bypass of review, right?
> > 
> > What's actually configured here is the divisor (10 MHz clock,
> > divisor seems to be value + 2). It's similar to existing
> > Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml .
> > I prefer this as it's much easier to use that in a driver (think of
> > having values like 1428571 in DT, too).
> 
> 
> Sure, but then this should be renamed to match purpose and
> description rephrased.
> 

What would be a better name for the property?

Best regards,
André

> > 
> > > 
> > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> 
> 
> And this would stay + constraints.
> 
> 
> Best regards,
> Krzysztof