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

Hi Sakari,

Thanks for the timely review.

On Fri, 2020-06-05 at 15:14 +0300, Sakari Ailus wrote:

Hi Dongchun,

Thank you for the update.

On Fri, Jun 05, 2020 at 06:54:12PM +0800, Dongchun Zhu wrote:

quoted

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

Signed-off-by: Dongchun Zhu <redacted>
---
 MAINTAINERS                |   1 +
 drivers/media/i2c/Kconfig  |  13 ++
 drivers/media/i2c/Makefile |   1 +
 drivers/media/i2c/dw9768.c | 566 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 581 insertions(+)
 create mode 100644 drivers/media/i2c/dw9768.c

[snip]...

quoted

+
+static int dw9768_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct dw9768 *dw9768;
+	u32 aac_mode_select;
+	u32 aac_timing_select;
+	u32 clock_presc_select;
+	unsigned int i;
+	int ret;
+
+	dw9768 = devm_kzalloc(dev, sizeof(*dw9768), GFP_KERNEL);
+	if (!dw9768)
+		return -ENOMEM;
+
+	/* Initialize subdev */
+	v4l2_i2c_subdev_init(&dw9768->sd, client, &dw9768_ops);
+
+	dw9768->aac_mode = DW9768_AAC_MODE_DEFAULT;
+	dw9768->aac_timing = DW9768_AAC_TIME_DEFAULT;
+	dw9768->clock_presc = DW9768_CLOCK_PRE_SCALE_DEFAULT;
+
+	/* Optional indication of AAC mode select */
+	ret = fwnode_property_read_u32(dev_fwnode(dev), "dongwoon,aac-mode",
+				       &aac_mode_select);
+
+	if (!ret)
+		dw9768->aac_mode = aac_mode_select;
+
+	/* Optional indication of clock pre-scale select */
+	ret = fwnode_property_read_u32(dev_fwnode(dev), "dongwoon,clock-presc",
+				       &clock_presc_select);
+
+	if (!ret)
+		dw9768->clock_presc = clock_presc_select;
+
+	/* Optional indication of AAC Timing */
+	ret = fwnode_property_read_u32(dev_fwnode(dev), "dongwoon,aac-timing",
+				       &aac_timing_select);
+
+	if (!ret)
+		dw9768->aac_timing = aac_timing_select;

You can assign the defaults to the dw9768 struct and use the fwnode
property API to read the properties into the same fields. No return values
need to be checked.

Good idea :-)

quoted

+
+	for (i = 0; i < ARRAY_SIZE(dw9768_supply_names); i++)
+		dw9768->supplies[i].supply = dw9768_supply_names[i];
+
+	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(dw9768_supply_names),
+				      dw9768->supplies);
+	if (ret) {
+		dev_err(dev, "failed to get regulators\n");
+		return ret;
+	}
+
+	/* Initialize controls */
+	ret = dw9768_init_controls(dw9768);
+	if (ret)
+		goto err_free_handler;
+
+	/* Initialize subdev */
+	dw9768->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	dw9768->sd.internal_ops = &dw9768_int_ops;
+
+	ret = media_entity_pads_init(&dw9768->sd.entity, 0, NULL);
+	if (ret < 0)
+		goto err_free_handler;
+
+	dw9768->sd.entity.function = MEDIA_ENT_F_LENS;
+
+	pm_runtime_enable(dev);
+	if (!pm_runtime_enabled(dev)) {
+		ret = dw9768_runtime_resume(dev);
+		if (ret < 0) {
+			dev_err(dev, "failed to power on: %d\n", ret);
+			goto err_clean_entity;
+		}
+	}
+
+	ret = v4l2_async_register_subdev(&dw9768->sd);
+	if (ret < 0) {
+		dev_err(dev, "failed to register V4L2 subdev: %d", ret);
+		goto error_async_register;
+	}
+
+	return 0;
+
+error_async_register:
+	if (!pm_runtime_enabled(dev))
+		dw9768_runtime_suspend(dev);
+err_clean_entity:
+	media_entity_cleanup(&dw9768->sd.entity);
+err_free_handler:
+	v4l2_ctrl_handler_free(&dw9768->ctrls);
+
+	return ret;
+}
+

[snip]...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel