All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] media: i2c: Add driver for DW9719 VCM
@ 2021-11-28 23:21 Daniel Scally
  2022-01-19 12:19 ` Sakari Ailus
  0 siblings, 1 reply; 4+ messages in thread
From: Daniel Scally @ 2021-11-28 23:21 UTC (permalink / raw)
  To: linux-media; +Cc: sakari.ailus, hdegoede, laurent.pinchart, kieran.bingham

Add a driver for the DW9719 VCM. The driver creates a v4l2 subdevice
and registers a control to set the desired focus.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
 MAINTAINERS                |   7 +
 drivers/media/i2c/Kconfig  |  11 +
 drivers/media/i2c/Makefile |   1 +
 drivers/media/i2c/dw9719.c | 427 +++++++++++++++++++++++++++++++++++++
 4 files changed, 446 insertions(+)
 create mode 100644 drivers/media/i2c/dw9719.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 6e0763ff55db..99d2bc961bc0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5789,6 +5789,13 @@ T:	git git://linuxtv.org/media_tree.git
 F:	Documentation/devicetree/bindings/media/i2c/dongwoon,dw9714.txt
 F:	drivers/media/i2c/dw9714.c
 
+DONGWOON DW9719 LENS VOICE COIL DRIVER
+M:	Daniel Scally <djrscally@gmail.com>
+L:	linux-media@vger.kernel.org
+S:	Maintained
+T:	git git://linuxtv.org/media_tree.git
+F:	drivers/media/i2c/dw9719.c
+
 DONGWOON DW9768 LENS VOICE COIL DRIVER
 M:	Dongchun Zhu <dongchun.zhu@mediatek.com>
 L:	linux-media@vger.kernel.org
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 8761a90a7a86..096c9be6c402 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -1465,6 +1465,17 @@ config VIDEO_DW9714
 	  capability. This is designed for linear control of
 	  voice coil motors, controlled via I2C serial interface.
 
+config VIDEO_DW9719
+	tristate "DW9719 lens voice coil support"
+	depends on I2C && VIDEO_V4L2
+	select MEDIA_CONTROLLER
+	select VIDEO_V4L2_SUBDEV_API
+	select V4L2_ASYNC
+	help
+	  This is a driver for the DW9719 camera lens voice coil.
+	  This is designed for linear control of  voice coil motors,
+	  controlled via I2C serial interface.
+
 config VIDEO_DW9768
 	tristate "DW9768 lens voice coil support"
 	depends on I2C && VIDEO_V4L2
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index b01f6cd05ee8..ba8b31d79222 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_VIDEO_SAA6752HS) += saa6752hs.o
 obj-$(CONFIG_VIDEO_AD5820)  += ad5820.o
 obj-$(CONFIG_VIDEO_AK7375)  += ak7375.o
 obj-$(CONFIG_VIDEO_DW9714)  += dw9714.o
+obj-$(CONFIG_VIDEO_DW9719)  += dw9719.o
 obj-$(CONFIG_VIDEO_DW9768)  += dw9768.o
 obj-$(CONFIG_VIDEO_DW9807_VCM)  += dw9807-vcm.o
 obj-$(CONFIG_VIDEO_ADV7170) += adv7170.o
diff --git a/drivers/media/i2c/dw9719.c b/drivers/media/i2c/dw9719.c
new file mode 100644
index 000000000000..8451c75b696b
--- /dev/null
+++ b/drivers/media/i2c/dw9719.c
@@ -0,0 +1,427 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2012 Intel Corporation
+
+/*
+ * Based on linux/modules/camera/drivers/media/i2c/imx/dw9719.c in this repo:
+ * https://github.com/ZenfoneArea/android_kernel_asus_zenfone5
+ */
+
+#include <asm/unaligned.h>
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
+#include <linux/types.h>
+
+#include <media/v4l2-common.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-subdev.h>
+
+#define DW9719_MAX_FOCUS_POS	1023
+#define DW9719_CTRL_STEPS	16
+#define DW9719_CTRL_DELAY_US	1000
+#define DELAY_MAX_PER_STEP_NS	(1000000 * 1023)
+
+#define DW9719_INFO			0
+#define DW9719_ID			0xF1
+#define DW9719_CONTROL			2
+#define DW9719_VCM_CURRENT		3
+
+#define DW9719_MODE			6
+#define DW9719_VCM_FREQ			7
+
+#define DW9719_MODE_SAC3		0x40
+#define DW9719_DEFAULT_VCM_FREQ		0x60
+#define DW9719_ENABLE_RINGING		0x02
+
+#define NUM_REGULATORS			2
+
+#define to_dw9719_device(x) container_of(x, struct dw9719_device, sd)
+
+struct dw9719_device {
+	struct device *dev;
+	struct i2c_client *client;
+	struct regulator_bulk_data regulators[NUM_REGULATORS];
+	struct v4l2_subdev sd;
+
+	struct dw9719_v4l2_ctrls {
+		struct v4l2_ctrl_handler handler;
+		struct v4l2_ctrl *focus;
+	} ctrls;
+};
+
+static int dw9719_i2c_rd8(struct i2c_client *client, u8 reg, u8 *val)
+{
+	struct i2c_msg msg[2];
+	u8 buf[2] = { reg };
+	int ret;
+
+	msg[0].addr = client->addr;
+	msg[0].flags = 0;
+	msg[0].len = 1;
+	msg[0].buf = buf;
+
+	msg[1].addr = client->addr;
+	msg[1].flags = I2C_M_RD;
+	msg[1].len = 1;
+	msg[1].buf = &buf[1];
+	*val = 0;
+
+	ret = i2c_transfer(client->adapter, msg, 2);
+	if (ret < 0)
+		return ret;
+
+	*val = buf[1];
+
+	return 0;
+}
+
+static int dw9719_i2c_wr8(struct i2c_client *client, u8 reg, u8 val)
+{
+	struct i2c_msg msg;
+	int ret;
+
+	u8 buf[2] = { reg, val };
+
+	msg.addr = client->addr;
+	msg.flags = 0;
+	msg.len = sizeof(buf);
+	msg.buf = buf;
+
+	ret = i2c_transfer(client->adapter, &msg, 1);
+
+	return ret < 0 ? ret : 0;
+}
+
+static int dw9719_i2c_wr16(struct i2c_client *client, u8 reg, u16 val)
+{
+	struct i2c_msg msg;
+	u8 buf[3] = { reg };
+	int ret;
+
+	put_unaligned_be16(val, buf + 1);
+
+	msg.addr = client->addr;
+	msg.flags = 0;
+	msg.len = sizeof(buf);
+	msg.buf = buf;
+
+	ret = i2c_transfer(client->adapter, &msg, 1);
+
+	return ret < 0 ? ret : 0;
+}
+
+static int dw9719_detect(struct dw9719_device *dw9719)
+{
+	int ret;
+	u8 val;
+
+	ret = dw9719_i2c_rd8(dw9719->client, DW9719_INFO, &val);
+	if (ret < 0)
+		return ret;
+
+	if (val != DW9719_ID) {
+		dev_err(dw9719->dev, "Failed to detect correct id\n");
+		ret = -ENXIO;
+	}
+
+	return 0;
+}
+
+static int dw9719_power_down(struct dw9719_device *dw9719)
+{
+	return regulator_bulk_disable(NUM_REGULATORS, dw9719->regulators);
+}
+
+static int dw9719_power_up(struct dw9719_device *dw9719)
+{
+	int ret;
+
+	ret = regulator_bulk_enable(NUM_REGULATORS, dw9719->regulators);
+	if (ret)
+		return ret;
+
+	/* Jiggle SCL pin to wake up device */
+	ret = dw9719_i2c_wr8(dw9719->client, DW9719_CONTROL, 1);
+
+	/* Need 100us to transit from SHUTDOWN to STANDBY*/
+	usleep_range(100, 1000);
+
+	ret = dw9719_i2c_wr8(dw9719->client, DW9719_CONTROL,
+			     DW9719_ENABLE_RINGING);
+	if (ret < 0)
+		goto fail_powerdown;
+
+	ret = dw9719_i2c_wr8(dw9719->client, DW9719_MODE, DW9719_MODE_SAC3);
+	if (ret < 0)
+		goto fail_powerdown;
+
+	ret = dw9719_i2c_wr8(dw9719->client, DW9719_VCM_FREQ,
+			     DW9719_DEFAULT_VCM_FREQ);
+	if (ret < 0)
+		goto fail_powerdown;
+
+	return 0;
+
+fail_powerdown:
+	dw9719_power_down(dw9719);
+	return ret;
+}
+
+static int dw9719_t_focus_abs(struct dw9719_device *dw9719, s32 value)
+{
+	int ret;
+
+	value = clamp(value, 0, DW9719_MAX_FOCUS_POS);
+	ret = dw9719_i2c_wr16(dw9719->client, DW9719_VCM_CURRENT, value);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int dw9719_set_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct dw9719_device *dw9719 = container_of(ctrl->handler,
+						    struct dw9719_device,
+						    ctrls.handler);
+	int ret;
+
+	/* Only apply changes to the controls if the device is powered up */
+	if (!pm_runtime_get_if_in_use(dw9719->dev))
+		return 0;
+
+	switch (ctrl->id) {
+	case V4L2_CID_FOCUS_ABSOLUTE:
+		ret = dw9719_t_focus_abs(dw9719, ctrl->val);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+	pm_runtime_put(dw9719->dev);
+
+	return ret;
+}
+
+static const struct v4l2_ctrl_ops dw9719_ctrl_ops = {
+	.s_ctrl = dw9719_set_ctrl,
+};
+
+static int __maybe_unused dw9719_suspend(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct dw9719_device *dw9719 = to_dw9719_device(sd);
+	int ret;
+	int val;
+
+	for (val = dw9719->ctrls.focus->val; val >= 0;
+	     val -= DW9719_CTRL_STEPS) {
+		ret = dw9719_t_focus_abs(dw9719, val);
+		if (ret)
+			return ret;
+
+		usleep_range(DW9719_CTRL_DELAY_US, DW9719_CTRL_DELAY_US + 10);
+	}
+
+	return dw9719_power_down(dw9719);
+}
+
+static int __maybe_unused dw9719_resume(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct dw9719_device *dw9719 = to_dw9719_device(sd);
+	int current_focus = dw9719->ctrls.focus->val;
+	int ret;
+	int val;
+
+	ret = dw9719_power_up(dw9719);
+	if (ret)
+		return ret;
+
+	for (val = current_focus % DW9719_CTRL_STEPS; val < current_focus;
+	     val += DW9719_CTRL_STEPS) {
+		ret = dw9719_t_focus_abs(dw9719, val);
+		if (ret)
+			goto err_power_down;
+
+		usleep_range(DW9719_CTRL_DELAY_US, DW9719_CTRL_DELAY_US + 10);
+	}
+
+	return 0;
+
+err_power_down:
+	dw9719_power_down(dw9719);
+	return ret;
+}
+
+static int dw9719_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+	return pm_runtime_resume_and_get(sd->dev);
+}
+
+static int dw9719_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+	pm_runtime_put(sd->dev);
+
+	return 0;
+}
+
+static const struct v4l2_subdev_internal_ops dw9719_internal_ops = {
+	.open = dw9719_open,
+	.close = dw9719_close,
+};
+
+static int dw9719_init_controls(struct dw9719_device *dw9719)
+{
+	const struct v4l2_ctrl_ops *ops = &dw9719_ctrl_ops;
+	int ret;
+
+	ret = v4l2_ctrl_handler_init(&dw9719->ctrls.handler, 1);
+	if (ret)
+		return ret;
+
+	dw9719->ctrls.focus = v4l2_ctrl_new_std(&dw9719->ctrls.handler, ops,
+						V4L2_CID_FOCUS_ABSOLUTE, 0,
+						DW9719_MAX_FOCUS_POS, 1, 0);
+
+	if (dw9719->ctrls.handler.error) {
+		dev_err(dw9719->dev, "Error initialising v4l2 ctrls\n");
+		ret = dw9719->ctrls.handler.error;
+		goto err_free_handler;
+	}
+
+	dw9719->sd.ctrl_handler = &dw9719->ctrls.handler;
+
+	return ret;
+
+err_free_handler:
+	v4l2_ctrl_handler_free(&dw9719->ctrls.handler);
+	return ret;
+}
+
+static const struct v4l2_subdev_ops dw9719_ops = { };
+
+static int dw9719_probe(struct i2c_client *client)
+{
+	struct dw9719_device *dw9719;
+	int ret;
+
+	dw9719 = devm_kzalloc(&client->dev, sizeof(*dw9719), GFP_KERNEL);
+	if (!dw9719)
+		return -ENOMEM;
+
+	dw9719->client = client;
+	dw9719->dev = &client->dev;
+
+	dw9719->regulators[0].supply = "vdd";
+	/*
+	 * The DW9719 has only the 1 VDD voltage input, but some PMICs such as
+	 * the TPS68470 PMIC have I2C passthrough capability, to disconnect the
+	 * sensor's I2C pins from the I2C bus when the sensors VSIO (Sensor-IO)
+	 * is off, because some sensors then short these pins to ground;
+	 * and the DW9719 might sit behind this passthrough, this it needs to
+	 * enable VSIO as that will also enable the I2C passthrough.
+	 */
+	dw9719->regulators[1].supply = "vsio";
+
+	ret = devm_regulator_bulk_get(&client->dev, NUM_REGULATORS,
+				      dw9719->regulators);
+	if (ret)
+		return dev_err_probe(&client->dev, ret, "getting regulators\n");
+
+	v4l2_i2c_subdev_init(&dw9719->sd, client, &dw9719_ops);
+	dw9719->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	dw9719->sd.internal_ops = &dw9719_internal_ops;
+
+	ret = dw9719_init_controls(dw9719);
+	if (ret)
+		return ret;
+
+	ret = media_entity_pads_init(&dw9719->sd.entity, 0, NULL);
+	if (ret < 0)
+		goto err_free_ctrl_handler;
+
+	dw9719->sd.entity.function = MEDIA_ENT_F_LENS;
+
+	/*
+	 * We need the driver to work in the event that pm runtime is disable in
+	 * the kernel, so power up and verify the chip now. In the event that
+	 * runtime pm is disabled this will leave the chip on, so that the lens
+	 * will work.
+	 */
+
+	ret = dw9719_power_up(dw9719);
+	if (ret)
+		goto err_cleanup_media;
+
+	ret = dw9719_detect(dw9719);
+	if (ret)
+		goto err_powerdown;
+
+	pm_runtime_set_active(&client->dev);
+	pm_runtime_get_noresume(&client->dev);
+	pm_runtime_enable(&client->dev);
+
+	ret = v4l2_async_register_subdev(&dw9719->sd);
+	if (ret < 0)
+		goto err_pm_runtime;
+
+	pm_runtime_set_autosuspend_delay(&client->dev, 1000);
+	pm_runtime_use_autosuspend(&client->dev);
+	pm_runtime_put_autosuspend(&client->dev);
+
+	return ret;
+
+err_pm_runtime:
+	pm_runtime_disable(&client->dev);
+	pm_runtime_put_noidle(&client->dev);
+err_powerdown:
+	dw9719_power_down(dw9719);
+err_cleanup_media:
+	media_entity_cleanup(&dw9719->sd.entity);
+err_free_ctrl_handler:
+	v4l2_ctrl_handler_free(&dw9719->ctrls.handler);
+
+	return ret;
+}
+
+static int dw9719_remove(struct i2c_client *client)
+{
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct dw9719_device *dw9719 = container_of(sd, struct dw9719_device,
+						    sd);
+
+	pm_runtime_disable(&client->dev);
+	v4l2_async_unregister_subdev(sd);
+	v4l2_ctrl_handler_free(&dw9719->ctrls.handler);
+	media_entity_cleanup(&dw9719->sd.entity);
+
+	return 0;
+}
+
+static const struct i2c_device_id dw9719_id_table[] = {
+	{ "dw9719" },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, dw9719_id_table);
+
+static const struct dev_pm_ops dw9719_pm_ops = {
+	SET_RUNTIME_PM_OPS(dw9719_suspend, dw9719_resume, NULL)
+};
+
+static struct i2c_driver dw9719_i2c_driver = {
+	.driver = {
+		.name = "dw9719",
+		.pm = &dw9719_pm_ops,
+	},
+	.probe_new = dw9719_probe,
+	.remove = dw9719_remove,
+	.id_table = dw9719_id_table,
+};
+module_i2c_driver(dw9719_i2c_driver);
+
+MODULE_AUTHOR("Daniel Scally <djrscally@gmail.com>");
+MODULE_DESCRIPTION("DW9719 VCM Driver");
+MODULE_LICENSE("GPL");
-- 
2.25.1


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

* Re: [PATCH] media: i2c: Add driver for DW9719 VCM
  2021-11-28 23:21 [PATCH] media: i2c: Add driver for DW9719 VCM Daniel Scally
@ 2022-01-19 12:19 ` Sakari Ailus
  2022-01-19 15:14   ` Daniel Scally
  0 siblings, 1 reply; 4+ messages in thread
From: Sakari Ailus @ 2022-01-19 12:19 UTC (permalink / raw)
  To: Daniel Scally; +Cc: linux-media, hdegoede, laurent.pinchart, kieran.bingham

Hi Daniel,

Apologies for the late review.

On Sun, Nov 28, 2021 at 11:21:15PM +0000, Daniel Scally wrote:
> Add a driver for the DW9719 VCM. The driver creates a v4l2 subdevice
> and registers a control to set the desired focus.
> 
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
>  MAINTAINERS                |   7 +
>  drivers/media/i2c/Kconfig  |  11 +
>  drivers/media/i2c/Makefile |   1 +
>  drivers/media/i2c/dw9719.c | 427 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 446 insertions(+)
>  create mode 100644 drivers/media/i2c/dw9719.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6e0763ff55db..99d2bc961bc0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5789,6 +5789,13 @@ T:	git git://linuxtv.org/media_tree.git
>  F:	Documentation/devicetree/bindings/media/i2c/dongwoon,dw9714.txt
>  F:	drivers/media/i2c/dw9714.c
>  
> +DONGWOON DW9719 LENS VOICE COIL DRIVER
> +M:	Daniel Scally <djrscally@gmail.com>
> +L:	linux-media@vger.kernel.org
> +S:	Maintained
> +T:	git git://linuxtv.org/media_tree.git
> +F:	drivers/media/i2c/dw9719.c
> +
>  DONGWOON DW9768 LENS VOICE COIL DRIVER
>  M:	Dongchun Zhu <dongchun.zhu@mediatek.com>
>  L:	linux-media@vger.kernel.org
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 8761a90a7a86..096c9be6c402 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -1465,6 +1465,17 @@ config VIDEO_DW9714
>  	  capability. This is designed for linear control of
>  	  voice coil motors, controlled via I2C serial interface.
>  
> +config VIDEO_DW9719
> +	tristate "DW9719 lens voice coil support"
> +	depends on I2C && VIDEO_V4L2
> +	select MEDIA_CONTROLLER
> +	select VIDEO_V4L2_SUBDEV_API
> +	select V4L2_ASYNC
> +	help
> +	  This is a driver for the DW9719 camera lens voice coil.
> +	  This is designed for linear control of  voice coil motors,
> +	  controlled via I2C serial interface.
> +
>  config VIDEO_DW9768
>  	tristate "DW9768 lens voice coil support"
>  	depends on I2C && VIDEO_V4L2
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index b01f6cd05ee8..ba8b31d79222 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -24,6 +24,7 @@ obj-$(CONFIG_VIDEO_SAA6752HS) += saa6752hs.o
>  obj-$(CONFIG_VIDEO_AD5820)  += ad5820.o
>  obj-$(CONFIG_VIDEO_AK7375)  += ak7375.o
>  obj-$(CONFIG_VIDEO_DW9714)  += dw9714.o
> +obj-$(CONFIG_VIDEO_DW9719)  += dw9719.o
>  obj-$(CONFIG_VIDEO_DW9768)  += dw9768.o
>  obj-$(CONFIG_VIDEO_DW9807_VCM)  += dw9807-vcm.o
>  obj-$(CONFIG_VIDEO_ADV7170) += adv7170.o
> diff --git a/drivers/media/i2c/dw9719.c b/drivers/media/i2c/dw9719.c
> new file mode 100644
> index 000000000000..8451c75b696b
> --- /dev/null
> +++ b/drivers/media/i2c/dw9719.c
> @@ -0,0 +1,427 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2012 Intel Corporation
> +
> +/*
> + * Based on linux/modules/camera/drivers/media/i2c/imx/dw9719.c in this repo:
> + * https://github.com/ZenfoneArea/android_kernel_asus_zenfone5
> + */
> +
> +#include <asm/unaligned.h>
> +
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/types.h>
> +
> +#include <media/v4l2-common.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-subdev.h>
> +
> +#define DW9719_MAX_FOCUS_POS	1023
> +#define DW9719_CTRL_STEPS	16
> +#define DW9719_CTRL_DELAY_US	1000
> +#define DELAY_MAX_PER_STEP_NS	(1000000 * 1023)
> +
> +#define DW9719_INFO			0
> +#define DW9719_ID			0xF1
> +#define DW9719_CONTROL			2
> +#define DW9719_VCM_CURRENT		3
> +
> +#define DW9719_MODE			6
> +#define DW9719_VCM_FREQ			7
> +
> +#define DW9719_MODE_SAC3		0x40
> +#define DW9719_DEFAULT_VCM_FREQ		0x60
> +#define DW9719_ENABLE_RINGING		0x02

I guess you could have defaults but these are probably specific to the
module (spring and lens) rather than to this chip (which is just a current
sink).

Still it'd be much better to have these bound to the platform. See e.g.

	Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml

> +
> +#define NUM_REGULATORS			2
> +
> +#define to_dw9719_device(x) container_of(x, struct dw9719_device, sd)
> +
> +struct dw9719_device {
> +	struct device *dev;
> +	struct i2c_client *client;
> +	struct regulator_bulk_data regulators[NUM_REGULATORS];
> +	struct v4l2_subdev sd;
> +
> +	struct dw9719_v4l2_ctrls {
> +		struct v4l2_ctrl_handler handler;
> +		struct v4l2_ctrl *focus;
> +	} ctrls;
> +};
> +
> +static int dw9719_i2c_rd8(struct i2c_client *client, u8 reg, u8 *val)
> +{
> +	struct i2c_msg msg[2];
> +	u8 buf[2] = { reg };
> +	int ret;
> +
> +	msg[0].addr = client->addr;
> +	msg[0].flags = 0;
> +	msg[0].len = 1;
> +	msg[0].buf = buf;
> +
> +	msg[1].addr = client->addr;
> +	msg[1].flags = I2C_M_RD;
> +	msg[1].len = 1;
> +	msg[1].buf = &buf[1];
> +	*val = 0;
> +
> +	ret = i2c_transfer(client->adapter, msg, 2);
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = buf[1];
> +
> +	return 0;
> +}
> +
> +static int dw9719_i2c_wr8(struct i2c_client *client, u8 reg, u8 val)
> +{
> +	struct i2c_msg msg;
> +	int ret;
> +
> +	u8 buf[2] = { reg, val };
> +
> +	msg.addr = client->addr;
> +	msg.flags = 0;
> +	msg.len = sizeof(buf);
> +	msg.buf = buf;
> +
> +	ret = i2c_transfer(client->adapter, &msg, 1);
> +
> +	return ret < 0 ? ret : 0;
> +}
> +
> +static int dw9719_i2c_wr16(struct i2c_client *client, u8 reg, u16 val)
> +{
> +	struct i2c_msg msg;
> +	u8 buf[3] = { reg };
> +	int ret;
> +
> +	put_unaligned_be16(val, buf + 1);
> +
> +	msg.addr = client->addr;
> +	msg.flags = 0;
> +	msg.len = sizeof(buf);
> +	msg.buf = buf;
> +
> +	ret = i2c_transfer(client->adapter, &msg, 1);
> +
> +	return ret < 0 ? ret : 0;
> +}
> +
> +static int dw9719_detect(struct dw9719_device *dw9719)
> +{
> +	int ret;
> +	u8 val;
> +
> +	ret = dw9719_i2c_rd8(dw9719->client, DW9719_INFO, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (val != DW9719_ID) {
> +		dev_err(dw9719->dev, "Failed to detect correct id\n");
> +		ret = -ENXIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static int dw9719_power_down(struct dw9719_device *dw9719)
> +{
> +	return regulator_bulk_disable(NUM_REGULATORS, dw9719->regulators);
> +}
> +
> +static int dw9719_power_up(struct dw9719_device *dw9719)
> +{
> +	int ret;
> +
> +	ret = regulator_bulk_enable(NUM_REGULATORS, dw9719->regulators);
> +	if (ret)
> +		return ret;
> +
> +	/* Jiggle SCL pin to wake up device */
> +	ret = dw9719_i2c_wr8(dw9719->client, DW9719_CONTROL, 1);
> +
> +	/* Need 100us to transit from SHUTDOWN to STANDBY*/
> +	usleep_range(100, 1000);
> +
> +	ret = dw9719_i2c_wr8(dw9719->client, DW9719_CONTROL,
> +			     DW9719_ENABLE_RINGING);
> +	if (ret < 0)
> +		goto fail_powerdown;
> +
> +	ret = dw9719_i2c_wr8(dw9719->client, DW9719_MODE, DW9719_MODE_SAC3);
> +	if (ret < 0)
> +		goto fail_powerdown;
> +
> +	ret = dw9719_i2c_wr8(dw9719->client, DW9719_VCM_FREQ,
> +			     DW9719_DEFAULT_VCM_FREQ);
> +	if (ret < 0)
> +		goto fail_powerdown;
> +
> +	return 0;
> +
> +fail_powerdown:
> +	dw9719_power_down(dw9719);
> +	return ret;
> +}
> +
> +static int dw9719_t_focus_abs(struct dw9719_device *dw9719, s32 value)
> +{
> +	int ret;
> +
> +	value = clamp(value, 0, DW9719_MAX_FOCUS_POS);
> +	ret = dw9719_i2c_wr16(dw9719->client, DW9719_VCM_CURRENT, value);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int dw9719_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct dw9719_device *dw9719 = container_of(ctrl->handler,
> +						    struct dw9719_device,
> +						    ctrls.handler);
> +	int ret;
> +
> +	/* Only apply changes to the controls if the device is powered up */
> +	if (!pm_runtime_get_if_in_use(dw9719->dev))
> +		return 0;
> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_FOCUS_ABSOLUTE:
> +		ret = dw9719_t_focus_abs(dw9719, ctrl->val);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +	pm_runtime_put(dw9719->dev);
> +
> +	return ret;
> +}
> +
> +static const struct v4l2_ctrl_ops dw9719_ctrl_ops = {
> +	.s_ctrl = dw9719_set_ctrl,
> +};
> +
> +static int __maybe_unused dw9719_suspend(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct dw9719_device *dw9719 = to_dw9719_device(sd);
> +	int ret;
> +	int val;
> +
> +	for (val = dw9719->ctrls.focus->val; val >= 0;
> +	     val -= DW9719_CTRL_STEPS) {
> +		ret = dw9719_t_focus_abs(dw9719, val);
> +		if (ret)
> +			return ret;
> +
> +		usleep_range(DW9719_CTRL_DELAY_US, DW9719_CTRL_DELAY_US + 10);
> +	}
> +
> +	return dw9719_power_down(dw9719);
> +}
> +
> +static int __maybe_unused dw9719_resume(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct dw9719_device *dw9719 = to_dw9719_device(sd);
> +	int current_focus = dw9719->ctrls.focus->val;
> +	int ret;
> +	int val;
> +
> +	ret = dw9719_power_up(dw9719);
> +	if (ret)
> +		return ret;
> +
> +	for (val = current_focus % DW9719_CTRL_STEPS; val < current_focus;
> +	     val += DW9719_CTRL_STEPS) {
> +		ret = dw9719_t_focus_abs(dw9719, val);
> +		if (ret)
> +			goto err_power_down;
> +
> +		usleep_range(DW9719_CTRL_DELAY_US, DW9719_CTRL_DELAY_US + 10);
> +	}
> +
> +	return 0;
> +
> +err_power_down:
> +	dw9719_power_down(dw9719);
> +	return ret;
> +}
> +
> +static int dw9719_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> +{
> +	return pm_runtime_resume_and_get(sd->dev);
> +}
> +
> +static int dw9719_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> +{
> +	pm_runtime_put(sd->dev);
> +
> +	return 0;
> +}
> +
> +static const struct v4l2_subdev_internal_ops dw9719_internal_ops = {
> +	.open = dw9719_open,
> +	.close = dw9719_close,
> +};
> +
> +static int dw9719_init_controls(struct dw9719_device *dw9719)
> +{
> +	const struct v4l2_ctrl_ops *ops = &dw9719_ctrl_ops;
> +	int ret;
> +
> +	ret = v4l2_ctrl_handler_init(&dw9719->ctrls.handler, 1);
> +	if (ret)
> +		return ret;
> +
> +	dw9719->ctrls.focus = v4l2_ctrl_new_std(&dw9719->ctrls.handler, ops,
> +						V4L2_CID_FOCUS_ABSOLUTE, 0,
> +						DW9719_MAX_FOCUS_POS, 1, 0);
> +
> +	if (dw9719->ctrls.handler.error) {
> +		dev_err(dw9719->dev, "Error initialising v4l2 ctrls\n");
> +		ret = dw9719->ctrls.handler.error;
> +		goto err_free_handler;
> +	}
> +
> +	dw9719->sd.ctrl_handler = &dw9719->ctrls.handler;
> +
> +	return ret;
> +
> +err_free_handler:
> +	v4l2_ctrl_handler_free(&dw9719->ctrls.handler);
> +	return ret;
> +}
> +
> +static const struct v4l2_subdev_ops dw9719_ops = { };
> +
> +static int dw9719_probe(struct i2c_client *client)
> +{
> +	struct dw9719_device *dw9719;
> +	int ret;
> +
> +	dw9719 = devm_kzalloc(&client->dev, sizeof(*dw9719), GFP_KERNEL);
> +	if (!dw9719)
> +		return -ENOMEM;
> +
> +	dw9719->client = client;
> +	dw9719->dev = &client->dev;
> +
> +	dw9719->regulators[0].supply = "vdd";
> +	/*
> +	 * The DW9719 has only the 1 VDD voltage input, but some PMICs such as
> +	 * the TPS68470 PMIC have I2C passthrough capability, to disconnect the
> +	 * sensor's I2C pins from the I2C bus when the sensors VSIO (Sensor-IO)
> +	 * is off, because some sensors then short these pins to ground;
> +	 * and the DW9719 might sit behind this passthrough, this it needs to
> +	 * enable VSIO as that will also enable the I2C passthrough.
> +	 */
> +	dw9719->regulators[1].supply = "vsio";

Urgh.

That should have been handled in ACPI.

Do you know if such a sensor actually exists or is this motivated by the
PMIC chip's feature?

> +
> +	ret = devm_regulator_bulk_get(&client->dev, NUM_REGULATORS,
> +				      dw9719->regulators);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret, "getting regulators\n");
> +
> +	v4l2_i2c_subdev_init(&dw9719->sd, client, &dw9719_ops);
> +	dw9719->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	dw9719->sd.internal_ops = &dw9719_internal_ops;
> +
> +	ret = dw9719_init_controls(dw9719);
> +	if (ret)
> +		return ret;
> +
> +	ret = media_entity_pads_init(&dw9719->sd.entity, 0, NULL);
> +	if (ret < 0)
> +		goto err_free_ctrl_handler;
> +
> +	dw9719->sd.entity.function = MEDIA_ENT_F_LENS;
> +
> +	/*
> +	 * We need the driver to work in the event that pm runtime is disable in
> +	 * the kernel, so power up and verify the chip now. In the event that
> +	 * runtime pm is disabled this will leave the chip on, so that the lens
> +	 * will work.
> +	 */
> +
> +	ret = dw9719_power_up(dw9719);
> +	if (ret)
> +		goto err_cleanup_media;
> +
> +	ret = dw9719_detect(dw9719);
> +	if (ret)
> +		goto err_powerdown;
> +
> +	pm_runtime_set_active(&client->dev);
> +	pm_runtime_get_noresume(&client->dev);
> +	pm_runtime_enable(&client->dev);
> +
> +	ret = v4l2_async_register_subdev(&dw9719->sd);
> +	if (ret < 0)
> +		goto err_pm_runtime;
> +
> +	pm_runtime_set_autosuspend_delay(&client->dev, 1000);
> +	pm_runtime_use_autosuspend(&client->dev);
> +	pm_runtime_put_autosuspend(&client->dev);
> +
> +	return ret;
> +
> +err_pm_runtime:
> +	pm_runtime_disable(&client->dev);
> +	pm_runtime_put_noidle(&client->dev);
> +err_powerdown:
> +	dw9719_power_down(dw9719);
> +err_cleanup_media:
> +	media_entity_cleanup(&dw9719->sd.entity);
> +err_free_ctrl_handler:
> +	v4l2_ctrl_handler_free(&dw9719->ctrls.handler);
> +
> +	return ret;
> +}
> +
> +static int dw9719_remove(struct i2c_client *client)
> +{
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct dw9719_device *dw9719 = container_of(sd, struct dw9719_device,
> +						    sd);
> +
> +	pm_runtime_disable(&client->dev);
> +	v4l2_async_unregister_subdev(sd);
> +	v4l2_ctrl_handler_free(&dw9719->ctrls.handler);
> +	media_entity_cleanup(&dw9719->sd.entity);
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id dw9719_id_table[] = {
> +	{ "dw9719" },

Hmm. Do you need the ID table?

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, dw9719_id_table);
> +
> +static const struct dev_pm_ops dw9719_pm_ops = {
> +	SET_RUNTIME_PM_OPS(dw9719_suspend, dw9719_resume, NULL)
> +};
> +
> +static struct i2c_driver dw9719_i2c_driver = {
> +	.driver = {
> +		.name = "dw9719",
> +		.pm = &dw9719_pm_ops,
> +	},
> +	.probe_new = dw9719_probe,
> +	.remove = dw9719_remove,
> +	.id_table = dw9719_id_table,
> +};
> +module_i2c_driver(dw9719_i2c_driver);
> +
> +MODULE_AUTHOR("Daniel Scally <djrscally@gmail.com>");
> +MODULE_DESCRIPTION("DW9719 VCM Driver");
> +MODULE_LICENSE("GPL");

-- 
Kind regards,

Sakari Ailus

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

* Re: [PATCH] media: i2c: Add driver for DW9719 VCM
  2022-01-19 12:19 ` Sakari Ailus
@ 2022-01-19 15:14   ` Daniel Scally
  0 siblings, 0 replies; 4+ messages in thread
From: Daniel Scally @ 2022-01-19 15:14 UTC (permalink / raw)
  To: Sakari Ailus; +Cc: linux-media, hdegoede, laurent.pinchart, kieran.bingham

Hi Sakari

On 19/01/2022 12:19, Sakari Ailus wrote:
> Hi Daniel,
>
> Apologies for the late review.


No problem at all - thanks for taking a look

> On Sun, Nov 28, 2021 at 11:21:15PM +0000, Daniel Scally wrote:
>> Add a driver for the DW9719 VCM. The driver creates a v4l2 subdevice
>> and registers a control to set the desired focus.
>>
>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>> ---
>>  MAINTAINERS                |   7 +
>>  drivers/media/i2c/Kconfig  |  11 +
>>  drivers/media/i2c/Makefile |   1 +
>>  drivers/media/i2c/dw9719.c | 427 +++++++++++++++++++++++++++++++++++++
>>  4 files changed, 446 insertions(+)
>>  create mode 100644 drivers/media/i2c/dw9719.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 6e0763ff55db..99d2bc961bc0 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -5789,6 +5789,13 @@ T:	git git://linuxtv.org/media_tree.git
>>  F:	Documentation/devicetree/bindings/media/i2c/dongwoon,dw9714.txt
>>  F:	drivers/media/i2c/dw9714.c
>>  
>> +DONGWOON DW9719 LENS VOICE COIL DRIVER
>> +M:	Daniel Scally <djrscally@gmail.com>
>> +L:	linux-media@vger.kernel.org
>> +S:	Maintained
>> +T:	git git://linuxtv.org/media_tree.git
>> +F:	drivers/media/i2c/dw9719.c
>> +
>>  DONGWOON DW9768 LENS VOICE COIL DRIVER
>>  M:	Dongchun Zhu <dongchun.zhu@mediatek.com>
>>  L:	linux-media@vger.kernel.org
>> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
>> index 8761a90a7a86..096c9be6c402 100644
>> --- a/drivers/media/i2c/Kconfig
>> +++ b/drivers/media/i2c/Kconfig
>> @@ -1465,6 +1465,17 @@ config VIDEO_DW9714
>>  	  capability. This is designed for linear control of
>>  	  voice coil motors, controlled via I2C serial interface.
>>  
>> +config VIDEO_DW9719
>> +	tristate "DW9719 lens voice coil support"
>> +	depends on I2C && VIDEO_V4L2
>> +	select MEDIA_CONTROLLER
>> +	select VIDEO_V4L2_SUBDEV_API
>> +	select V4L2_ASYNC
>> +	help
>> +	  This is a driver for the DW9719 camera lens voice coil.
>> +	  This is designed for linear control of  voice coil motors,
>> +	  controlled via I2C serial interface.
>> +
>>  config VIDEO_DW9768
>>  	tristate "DW9768 lens voice coil support"
>>  	depends on I2C && VIDEO_V4L2
>> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
>> index b01f6cd05ee8..ba8b31d79222 100644
>> --- a/drivers/media/i2c/Makefile
>> +++ b/drivers/media/i2c/Makefile
>> @@ -24,6 +24,7 @@ obj-$(CONFIG_VIDEO_SAA6752HS) += saa6752hs.o
>>  obj-$(CONFIG_VIDEO_AD5820)  += ad5820.o
>>  obj-$(CONFIG_VIDEO_AK7375)  += ak7375.o
>>  obj-$(CONFIG_VIDEO_DW9714)  += dw9714.o
>> +obj-$(CONFIG_VIDEO_DW9719)  += dw9719.o
>>  obj-$(CONFIG_VIDEO_DW9768)  += dw9768.o
>>  obj-$(CONFIG_VIDEO_DW9807_VCM)  += dw9807-vcm.o
>>  obj-$(CONFIG_VIDEO_ADV7170) += adv7170.o
>> diff --git a/drivers/media/i2c/dw9719.c b/drivers/media/i2c/dw9719.c
>> new file mode 100644
>> index 000000000000..8451c75b696b
>> --- /dev/null
>> +++ b/drivers/media/i2c/dw9719.c
>> @@ -0,0 +1,427 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2012 Intel Corporation
>> +
>> +/*
>> + * Based on linux/modules/camera/drivers/media/i2c/imx/dw9719.c in this repo:
>> + * https://github.com/ZenfoneArea/android_kernel_asus_zenfone5
>> + */
>> +
>> +#include <asm/unaligned.h>
>> +
>> +#include <linux/delay.h>
>> +#include <linux/i2c.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/types.h>
>> +
>> +#include <media/v4l2-common.h>
>> +#include <media/v4l2-ctrls.h>
>> +#include <media/v4l2-subdev.h>
>> +
>> +#define DW9719_MAX_FOCUS_POS	1023
>> +#define DW9719_CTRL_STEPS	16
>> +#define DW9719_CTRL_DELAY_US	1000
>> +#define DELAY_MAX_PER_STEP_NS	(1000000 * 1023)
>> +
>> +#define DW9719_INFO			0
>> +#define DW9719_ID			0xF1
>> +#define DW9719_CONTROL			2
>> +#define DW9719_VCM_CURRENT		3
>> +
>> +#define DW9719_MODE			6
>> +#define DW9719_VCM_FREQ			7
>> +
>> +#define DW9719_MODE_SAC3		0x40
>> +#define DW9719_DEFAULT_VCM_FREQ		0x60
>> +#define DW9719_ENABLE_RINGING		0x02
> I guess you could have defaults but these are probably specific to the
> module (spring and lens) rather than to this chip (which is just a current
> sink).


Ah ok, that would make sense.

>
> Still it'd be much better to have these bound to the platform. See e.g.
>
> 	Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml


In this case it's an ACPI platform; those devices are currently being
created by the cio2-bridge code (because the less-than-optimal DSDT
tables for these devices only describe them as a second I2cSerialBusV2
against the camera sensor...). I can create fwnode properties against
the device for those settings in the cio2-bridge and optionally read
(using those existing macros as defaults) them from fwnode in the driver
the same as the dw9768 driver does...how's that sound? Out of curiosity,
is there a list of established property names somewhere?



> +
>> +#define NUM_REGULATORS			2
>> +
>> +#define to_dw9719_device(x) container_of(x, struct dw9719_device, sd)
>> +
>> +struct dw9719_device {
>> +	struct device *dev;
>> +	struct i2c_client *client;
>> +	struct regulator_bulk_data regulators[NUM_REGULATORS];
>> +	struct v4l2_subdev sd;
>> +
>> +	struct dw9719_v4l2_ctrls {
>> +		struct v4l2_ctrl_handler handler;
>> +		struct v4l2_ctrl *focus;
>> +	} ctrls;
>> +};
>> +
>> +static int dw9719_i2c_rd8(struct i2c_client *client, u8 reg, u8 *val)
>> +{
>> +	struct i2c_msg msg[2];
>> +	u8 buf[2] = { reg };
>> +	int ret;
>> +
>> +	msg[0].addr = client->addr;
>> +	msg[0].flags = 0;
>> +	msg[0].len = 1;
>> +	msg[0].buf = buf;
>> +
>> +	msg[1].addr = client->addr;
>> +	msg[1].flags = I2C_M_RD;
>> +	msg[1].len = 1;
>> +	msg[1].buf = &buf[1];
>> +	*val = 0;
>> +
>> +	ret = i2c_transfer(client->adapter, msg, 2);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	*val = buf[1];
>> +
>> +	return 0;
>> +}
>> +
>> +static int dw9719_i2c_wr8(struct i2c_client *client, u8 reg, u8 val)
>> +{
>> +	struct i2c_msg msg;
>> +	int ret;
>> +
>> +	u8 buf[2] = { reg, val };
>> +
>> +	msg.addr = client->addr;
>> +	msg.flags = 0;
>> +	msg.len = sizeof(buf);
>> +	msg.buf = buf;
>> +
>> +	ret = i2c_transfer(client->adapter, &msg, 1);
>> +
>> +	return ret < 0 ? ret : 0;
>> +}
>> +
>> +static int dw9719_i2c_wr16(struct i2c_client *client, u8 reg, u16 val)
>> +{
>> +	struct i2c_msg msg;
>> +	u8 buf[3] = { reg };
>> +	int ret;
>> +
>> +	put_unaligned_be16(val, buf + 1);
>> +
>> +	msg.addr = client->addr;
>> +	msg.flags = 0;
>> +	msg.len = sizeof(buf);
>> +	msg.buf = buf;
>> +
>> +	ret = i2c_transfer(client->adapter, &msg, 1);
>> +
>> +	return ret < 0 ? ret : 0;
>> +}
>> +
>> +static int dw9719_detect(struct dw9719_device *dw9719)
>> +{
>> +	int ret;
>> +	u8 val;
>> +
>> +	ret = dw9719_i2c_rd8(dw9719->client, DW9719_INFO, &val);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	if (val != DW9719_ID) {
>> +		dev_err(dw9719->dev, "Failed to detect correct id\n");
>> +		ret = -ENXIO;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int dw9719_power_down(struct dw9719_device *dw9719)
>> +{
>> +	return regulator_bulk_disable(NUM_REGULATORS, dw9719->regulators);
>> +}
>> +
>> +static int dw9719_power_up(struct dw9719_device *dw9719)
>> +{
>> +	int ret;
>> +
>> +	ret = regulator_bulk_enable(NUM_REGULATORS, dw9719->regulators);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Jiggle SCL pin to wake up device */
>> +	ret = dw9719_i2c_wr8(dw9719->client, DW9719_CONTROL, 1);
>> +
>> +	/* Need 100us to transit from SHUTDOWN to STANDBY*/
>> +	usleep_range(100, 1000);
>> +
>> +	ret = dw9719_i2c_wr8(dw9719->client, DW9719_CONTROL,
>> +			     DW9719_ENABLE_RINGING);
>> +	if (ret < 0)
>> +		goto fail_powerdown;
>> +
>> +	ret = dw9719_i2c_wr8(dw9719->client, DW9719_MODE, DW9719_MODE_SAC3);
>> +	if (ret < 0)
>> +		goto fail_powerdown;
>> +
>> +	ret = dw9719_i2c_wr8(dw9719->client, DW9719_VCM_FREQ,
>> +			     DW9719_DEFAULT_VCM_FREQ);
>> +	if (ret < 0)
>> +		goto fail_powerdown;
>> +
>> +	return 0;
>> +
>> +fail_powerdown:
>> +	dw9719_power_down(dw9719);
>> +	return ret;
>> +}
>> +
>> +static int dw9719_t_focus_abs(struct dw9719_device *dw9719, s32 value)
>> +{
>> +	int ret;
>> +
>> +	value = clamp(value, 0, DW9719_MAX_FOCUS_POS);
>> +	ret = dw9719_i2c_wr16(dw9719->client, DW9719_VCM_CURRENT, value);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +
>> +static int dw9719_set_ctrl(struct v4l2_ctrl *ctrl)
>> +{
>> +	struct dw9719_device *dw9719 = container_of(ctrl->handler,
>> +						    struct dw9719_device,
>> +						    ctrls.handler);
>> +	int ret;
>> +
>> +	/* Only apply changes to the controls if the device is powered up */
>> +	if (!pm_runtime_get_if_in_use(dw9719->dev))
>> +		return 0;
>> +
>> +	switch (ctrl->id) {
>> +	case V4L2_CID_FOCUS_ABSOLUTE:
>> +		ret = dw9719_t_focus_abs(dw9719, ctrl->val);
>> +		break;
>> +	default:
>> +		ret = -EINVAL;
>> +	}
>> +
>> +	pm_runtime_put(dw9719->dev);
>> +
>> +	return ret;
>> +}
>> +
>> +static const struct v4l2_ctrl_ops dw9719_ctrl_ops = {
>> +	.s_ctrl = dw9719_set_ctrl,
>> +};
>> +
>> +static int __maybe_unused dw9719_suspend(struct device *dev)
>> +{
>> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>> +	struct dw9719_device *dw9719 = to_dw9719_device(sd);
>> +	int ret;
>> +	int val;
>> +
>> +	for (val = dw9719->ctrls.focus->val; val >= 0;
>> +	     val -= DW9719_CTRL_STEPS) {
>> +		ret = dw9719_t_focus_abs(dw9719, val);
>> +		if (ret)
>> +			return ret;
>> +
>> +		usleep_range(DW9719_CTRL_DELAY_US, DW9719_CTRL_DELAY_US + 10);
>> +	}
>> +
>> +	return dw9719_power_down(dw9719);
>> +}
>> +
>> +static int __maybe_unused dw9719_resume(struct device *dev)
>> +{
>> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>> +	struct dw9719_device *dw9719 = to_dw9719_device(sd);
>> +	int current_focus = dw9719->ctrls.focus->val;
>> +	int ret;
>> +	int val;
>> +
>> +	ret = dw9719_power_up(dw9719);
>> +	if (ret)
>> +		return ret;
>> +
>> +	for (val = current_focus % DW9719_CTRL_STEPS; val < current_focus;
>> +	     val += DW9719_CTRL_STEPS) {
>> +		ret = dw9719_t_focus_abs(dw9719, val);
>> +		if (ret)
>> +			goto err_power_down;
>> +
>> +		usleep_range(DW9719_CTRL_DELAY_US, DW9719_CTRL_DELAY_US + 10);
>> +	}
>> +
>> +	return 0;
>> +
>> +err_power_down:
>> +	dw9719_power_down(dw9719);
>> +	return ret;
>> +}
>> +
>> +static int dw9719_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>> +{
>> +	return pm_runtime_resume_and_get(sd->dev);
>> +}
>> +
>> +static int dw9719_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>> +{
>> +	pm_runtime_put(sd->dev);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct v4l2_subdev_internal_ops dw9719_internal_ops = {
>> +	.open = dw9719_open,
>> +	.close = dw9719_close,
>> +};
>> +
>> +static int dw9719_init_controls(struct dw9719_device *dw9719)
>> +{
>> +	const struct v4l2_ctrl_ops *ops = &dw9719_ctrl_ops;
>> +	int ret;
>> +
>> +	ret = v4l2_ctrl_handler_init(&dw9719->ctrls.handler, 1);
>> +	if (ret)
>> +		return ret;
>> +
>> +	dw9719->ctrls.focus = v4l2_ctrl_new_std(&dw9719->ctrls.handler, ops,
>> +						V4L2_CID_FOCUS_ABSOLUTE, 0,
>> +						DW9719_MAX_FOCUS_POS, 1, 0);
>> +
>> +	if (dw9719->ctrls.handler.error) {
>> +		dev_err(dw9719->dev, "Error initialising v4l2 ctrls\n");
>> +		ret = dw9719->ctrls.handler.error;
>> +		goto err_free_handler;
>> +	}
>> +
>> +	dw9719->sd.ctrl_handler = &dw9719->ctrls.handler;
>> +
>> +	return ret;
>> +
>> +err_free_handler:
>> +	v4l2_ctrl_handler_free(&dw9719->ctrls.handler);
>> +	return ret;
>> +}
>> +
>> +static const struct v4l2_subdev_ops dw9719_ops = { };
>> +
>> +static int dw9719_probe(struct i2c_client *client)
>> +{
>> +	struct dw9719_device *dw9719;
>> +	int ret;
>> +
>> +	dw9719 = devm_kzalloc(&client->dev, sizeof(*dw9719), GFP_KERNEL);
>> +	if (!dw9719)
>> +		return -ENOMEM;
>> +
>> +	dw9719->client = client;
>> +	dw9719->dev = &client->dev;
>> +
>> +	dw9719->regulators[0].supply = "vdd";
>> +	/*
>> +	 * The DW9719 has only the 1 VDD voltage input, but some PMICs such as
>> +	 * the TPS68470 PMIC have I2C passthrough capability, to disconnect the
>> +	 * sensor's I2C pins from the I2C bus when the sensors VSIO (Sensor-IO)
>> +	 * is off, because some sensors then short these pins to ground;
>> +	 * and the DW9719 might sit behind this passthrough, this it needs to
>> +	 * enable VSIO as that will also enable the I2C passthrough.
>> +	 */
>> +	dw9719->regulators[1].supply = "vsio";
> Urgh.


Yup

> That should have been handled in ACPI.


Yup

> Do you know if such a sensor actually exists or is this motivated by the
> PMIC chip's feature?


Yup - the ov8865 on the Surface Go 1/2/3 models. Without setting it up
this way the driver won't probe unless you're already streaming with the
camera sensor.

>
>> +
>> +	ret = devm_regulator_bulk_get(&client->dev, NUM_REGULATORS,
>> +				      dw9719->regulators);
>> +	if (ret)
>> +		return dev_err_probe(&client->dev, ret, "getting regulators\n");
>> +
>> +	v4l2_i2c_subdev_init(&dw9719->sd, client, &dw9719_ops);
>> +	dw9719->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>> +	dw9719->sd.internal_ops = &dw9719_internal_ops;
>> +
>> +	ret = dw9719_init_controls(dw9719);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = media_entity_pads_init(&dw9719->sd.entity, 0, NULL);
>> +	if (ret < 0)
>> +		goto err_free_ctrl_handler;
>> +
>> +	dw9719->sd.entity.function = MEDIA_ENT_F_LENS;
>> +
>> +	/*
>> +	 * We need the driver to work in the event that pm runtime is disable in
>> +	 * the kernel, so power up and verify the chip now. In the event that
>> +	 * runtime pm is disabled this will leave the chip on, so that the lens
>> +	 * will work.
>> +	 */
>> +
>> +	ret = dw9719_power_up(dw9719);
>> +	if (ret)
>> +		goto err_cleanup_media;
>> +
>> +	ret = dw9719_detect(dw9719);
>> +	if (ret)
>> +		goto err_powerdown;
>> +
>> +	pm_runtime_set_active(&client->dev);
>> +	pm_runtime_get_noresume(&client->dev);
>> +	pm_runtime_enable(&client->dev);
>> +
>> +	ret = v4l2_async_register_subdev(&dw9719->sd);
>> +	if (ret < 0)
>> +		goto err_pm_runtime;
>> +
>> +	pm_runtime_set_autosuspend_delay(&client->dev, 1000);
>> +	pm_runtime_use_autosuspend(&client->dev);
>> +	pm_runtime_put_autosuspend(&client->dev);
>> +
>> +	return ret;
>> +
>> +err_pm_runtime:
>> +	pm_runtime_disable(&client->dev);
>> +	pm_runtime_put_noidle(&client->dev);
>> +err_powerdown:
>> +	dw9719_power_down(dw9719);
>> +err_cleanup_media:
>> +	media_entity_cleanup(&dw9719->sd.entity);
>> +err_free_ctrl_handler:
>> +	v4l2_ctrl_handler_free(&dw9719->ctrls.handler);
>> +
>> +	return ret;
>> +}
>> +
>> +static int dw9719_remove(struct i2c_client *client)
>> +{
>> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
>> +	struct dw9719_device *dw9719 = container_of(sd, struct dw9719_device,
>> +						    sd);
>> +
>> +	pm_runtime_disable(&client->dev);
>> +	v4l2_async_unregister_subdev(sd);
>> +	v4l2_ctrl_handler_free(&dw9719->ctrls.handler);
>> +	media_entity_cleanup(&dw9719->sd.entity);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct i2c_device_id dw9719_id_table[] = {
>> +	{ "dw9719" },
> Hmm. Do you need the ID table?


Err, I think so? There's no dt / acpi matching, how would the driver and
device bind without the i2c ID? Is there a 4th method I've not noticed?

>
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, dw9719_id_table);
>> +
>> +static const struct dev_pm_ops dw9719_pm_ops = {
>> +	SET_RUNTIME_PM_OPS(dw9719_suspend, dw9719_resume, NULL)
>> +};
>> +
>> +static struct i2c_driver dw9719_i2c_driver = {
>> +	.driver = {
>> +		.name = "dw9719",
>> +		.pm = &dw9719_pm_ops,
>> +	},
>> +	.probe_new = dw9719_probe,
>> +	.remove = dw9719_remove,
>> +	.id_table = dw9719_id_table,
>> +};
>> +module_i2c_driver(dw9719_i2c_driver);
>> +
>> +MODULE_AUTHOR("Daniel Scally <djrscally@gmail.com>");
>> +MODULE_DESCRIPTION("DW9719 VCM Driver");
>> +MODULE_LICENSE("GPL");

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

* Re: [PATCH] media: i2c: Add driver for DW9719 VCM
@ 2021-12-04 19:34 kernel test robot
  0 siblings, 0 replies; 4+ messages in thread
From: kernel test robot @ 2021-12-04 19:34 UTC (permalink / raw)
  To: kbuild

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

CC: llvm(a)lists.linux.dev
CC: kbuild-all(a)lists.01.org
In-Reply-To: <20211128232115.38833-1-djrscally@gmail.com>
References: <20211128232115.38833-1-djrscally@gmail.com>
TO: Daniel Scally <djrscally@gmail.com>
TO: linux-media(a)vger.kernel.org
CC: sakari.ailus(a)linux.intel.com
CC: hdegoede(a)redhat.com
CC: laurent.pinchart(a)ideasonboard.com
CC: kieran.bingham(a)ideasonboard.com

Hi Daniel,

I love your patch! Perhaps something to improve:

[auto build test WARNING on media-tree/master]
[also build test WARNING on linus/master v5.16-rc3 next-20211203]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Daniel-Scally/media-i2c-Add-driver-for-DW9719-VCM/20211129-072436
base:   git://linuxtv.org/media_tree.git master
:::::: branch date: 6 days ago
:::::: commit date: 6 days ago
config: i386-randconfig-c001-20211130 (https://download.01.org/0day-ci/archive/20211205/202112050308.lEaCEX4U-lkp(a)intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 25eb7fa01d7ebbe67648ea03841cda55b4239ab2)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/8bbbe049053a268d99463f6fda429d0af571a27a
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Daniel-Scally/media-i2c-Add-driver-for-DW9719-VCM/20211129-072436
        git checkout 8bbbe049053a268d99463f6fda429d0af571a27a
        # save the config file to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=i386 clang-analyzer 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


clang-analyzer warnings: (new ones prefixed by >>)
               ^~~~~
   sound/drivers/vx/vx_pcm.c:217:2: note: Taking false branch
           if (! err)
           ^
   sound/drivers/vx/vx_pcm.c:219:2: note: Returning without writing to '*state'
           return err;
           ^
   sound/drivers/vx/vx_pcm.c:323:6: note: Returning from 'vx_get_pipe_state'
           if (vx_get_pipe_state(chip, pipe, &cur_state) < 0)
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   sound/drivers/vx/vx_pcm.c:323:6: note: Assuming the condition is false
           if (vx_get_pipe_state(chip, pipe, &cur_state) < 0)
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   sound/drivers/vx/vx_pcm.c:323:2: note: Taking false branch
           if (vx_get_pipe_state(chip, pipe, &cur_state) < 0)
           ^
   sound/drivers/vx/vx_pcm.c:325:12: note: The right operand of '==' is a garbage value
           if (state == cur_state)
                     ^  ~~~~~~~~~
   sound/drivers/vx/vx_pcm.c:1225:3: warning: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119 [clang-analyzer-security.insecureAPI.strcpy]
                   strcpy(pcm->name, chip->card->shortname);
                   ^~~~~~
   sound/drivers/vx/vx_pcm.c:1225:3: note: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119
                   strcpy(pcm->name, chip->card->shortname);
                   ^~~~~~
   Suppressed 4 warnings (4 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   8 warnings generated.
   sound/drivers/vx/vx_mixer.c:836:55: warning: The left operand of '>>' is a garbage value [clang-analyzer-core.UndefinedBinaryOperatorResult]
           ucontrol->value.integer.value[0] = meter[0].vu_level >> METER_SHIFT;
                                              ~~~~~~~~~~~~~~~~~ ^
   sound/drivers/vx/vx_mixer.c:835:2: note: Calling 'vx_get_audio_vu_meter'
           vx_get_audio_vu_meter(chip, audio, capture, meter);
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   sound/drivers/vx/vx_mixer.c:367:6: note: Assuming the condition is true
           if (chip->chip_status & VX_STAT_IS_STALE)
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   sound/drivers/vx/vx_mixer.c:367:2: note: Taking true branch
           if (chip->chip_status & VX_STAT_IS_STALE)
           ^
   sound/drivers/vx/vx_mixer.c:368:3: note: Returning without writing to 'info->vu_level'
                   return -EBUSY;
                   ^
   sound/drivers/vx/vx_mixer.c:835:2: note: Returning from 'vx_get_audio_vu_meter'
           vx_get_audio_vu_meter(chip, audio, capture, meter);
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   sound/drivers/vx/vx_mixer.c:836:55: note: The left operand of '>>' is a garbage value
           ucontrol->value.integer.value[0] = meter[0].vu_level >> METER_SHIFT;
                                              ~~~~~~~~~~~~~~~~~ ^
   sound/drivers/vx/vx_mixer.c:849:57: warning: The left operand of '>>' is a garbage value [clang-analyzer-core.UndefinedBinaryOperatorResult]
           ucontrol->value.integer.value[0] = meter[0].peak_level >> METER_SHIFT;
                                              ~~~~~~~~~~~~~~~~~~~ ^
   sound/drivers/vx/vx_mixer.c:848:2: note: Calling 'vx_get_audio_vu_meter'
           vx_get_audio_vu_meter(chip, audio, capture, meter);
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   sound/drivers/vx/vx_mixer.c:367:6: note: Assuming the condition is true
           if (chip->chip_status & VX_STAT_IS_STALE)
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   sound/drivers/vx/vx_mixer.c:367:2: note: Taking true branch
           if (chip->chip_status & VX_STAT_IS_STALE)
           ^
   sound/drivers/vx/vx_mixer.c:368:3: note: Returning without writing to 'info->peak_level'
                   return -EBUSY;
                   ^
   sound/drivers/vx/vx_mixer.c:848:2: note: Returning from 'vx_get_audio_vu_meter'
           vx_get_audio_vu_meter(chip, audio, capture, meter);
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   sound/drivers/vx/vx_mixer.c:849:57: note: The left operand of '>>' is a garbage value
           ucontrol->value.integer.value[0] = meter[0].peak_level >> METER_SHIFT;
                                              ~~~~~~~~~~~~~~~~~~~ ^
   sound/drivers/vx/vx_mixer.c:863:35: warning: Assigned value is garbage or undefined [clang-analyzer-core.uninitialized.Assign]
           ucontrol->value.integer.value[0] = meter[0].saturated;
                                            ^ ~~~~~~~~~~~~~~~~~~
   sound/drivers/vx/vx_mixer.c:862:2: note: Calling 'vx_get_audio_vu_meter'
           vx_get_audio_vu_meter(chip, audio, 1, meter); /* capture only */
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   sound/drivers/vx/vx_mixer.c:367:6: note: Assuming the condition is true
           if (chip->chip_status & VX_STAT_IS_STALE)
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   sound/drivers/vx/vx_mixer.c:367:2: note: Taking true branch
           if (chip->chip_status & VX_STAT_IS_STALE)
           ^
   sound/drivers/vx/vx_mixer.c:368:3: note: Returning without writing to 'info->saturated'
                   return -EBUSY;
                   ^
   sound/drivers/vx/vx_mixer.c:862:2: note: Returning from 'vx_get_audio_vu_meter'
           vx_get_audio_vu_meter(chip, audio, 1, meter); /* capture only */
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   sound/drivers/vx/vx_mixer.c:863:35: note: Assigned value is garbage or undefined
           ucontrol->value.integer.value[0] = meter[0].saturated;
                                            ^ ~~~~~~~~~~~~~~~~~~
   sound/drivers/vx/vx_mixer.c:906:2: warning: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119 [clang-analyzer-security.insecureAPI.strcpy]
           strcpy(card->mixername, card->driver);
           ^~~~~~
   sound/drivers/vx/vx_mixer.c:906:2: note: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119
           strcpy(card->mixername, card->driver);
           ^~~~~~
   Suppressed 4 warnings (4 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   9 warnings generated.
>> drivers/media/i2c/dw9719.c:126:3: warning: Value stored to 'ret' is never read [clang-analyzer-deadcode.DeadStores]
                   ret = -ENXIO;
                   ^     ~~~~~~
   drivers/media/i2c/dw9719.c:126:3: note: Value stored to 'ret' is never read
                   ret = -ENXIO;
                   ^     ~~~~~~
   drivers/media/i2c/dw9719.c:146:2: warning: Value stored to 'ret' is never read [clang-analyzer-deadcode.DeadStores]
           ret = dw9719_i2c_wr8(dw9719->client, DW9719_CONTROL, 1);
           ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/media/i2c/dw9719.c:146:2: note: Value stored to 'ret' is never read
           ret = dw9719_i2c_wr8(dw9719->client, DW9719_CONTROL, 1);
           ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   Suppressed 7 warnings (7 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   7 warnings generated.
   Suppressed 7 warnings (7 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   7 warnings generated.
   Suppressed 7 warnings (7 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   9 warnings generated.
   drivers/media/i2c/adv7175.c:416:3: warning: Value stored to 'i' is never read [clang-analyzer-deadcode.DeadStores]
                   i = adv7175_write(sd, 0x07, TR0MODE | TR0RST);
                   ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/media/i2c/adv7175.c:416:3: note: Value stored to 'i' is never read
                   i = adv7175_write(sd, 0x07, TR0MODE | TR0RST);
                   ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/media/i2c/adv7175.c:417:3: warning: Value stored to 'i' is never read [clang-analyzer-deadcode.DeadStores]
                   i = adv7175_write(sd, 0x07, TR0MODE);
                   ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/media/i2c/adv7175.c:417:3: note: Value stored to 'i' is never read
                   i = adv7175_write(sd, 0x07, TR0MODE);
                   ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   Suppressed 7 warnings (7 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   7 warnings generated.
   Suppressed 7 warnings (7 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   7 warnings generated.
   Suppressed 7 warnings (7 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   7 warnings generated.
   Suppressed 7 warnings (7 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   7 warnings generated.
   Suppressed 7 warnings (7 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   18 warnings generated.
   Suppressed 18 warnings (8 in non-user code, 10 with check filters).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   11 warnings generated.
   drivers/media/i2c/adv7842.c:3499:60: warning: The result of the left shift is undefined because the left operand is negative [clang-analyzer-core.UndefinedBinaryOperatorResult]
           rev = adv_smbus_read_byte_data_check(client, 0xea, false) << 8 |
                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
   drivers/media/i2c/adv7842.c:3469:7: note: Calling 'i2c_check_functionality'
           if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/i2c.h:913:9: note: Assuming the condition is true
           return (func & i2c_get_functionality(adap)) == func;
                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/i2c.h:913:2: note: Returning the value 1, which participates in a condition later
           return (func & i2c_get_functionality(adap)) == func;
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/media/i2c/adv7842.c:3469:7: note: Returning from 'i2c_check_functionality'
           if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/media/i2c/adv7842.c:3469:2: note: Taking false branch
           if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
           ^
   drivers/media/i2c/adv7842.c:3472:13: note: Assuming 'debug' is < 1
           v4l_dbg(1, debug, client, "detecting adv7842 client on address 0x%x\n",
                      ^
   include/media/v4l2-common.h:43:7: note: expanded from macro 'v4l_dbg'
                   if (debug >= (level))                                        \
                       ^~~~~~~~~~~~~~~~
   drivers/media/i2c/adv7842.c:3472:2: note: Taking false branch
           v4l_dbg(1, debug, client, "detecting adv7842 client on address 0x%x\n",
           ^
   include/media/v4l2-common.h:43:3: note: expanded from macro 'v4l_dbg'
                   if (debug >= (level))                                        \
                   ^
   drivers/media/i2c/adv7842.c:3472:2: note: Loop condition is false.  Exiting loop
           v4l_dbg(1, debug, client, "detecting adv7842 client on address 0x%x\n",
           ^
   include/media/v4l2-common.h:42:2: note: expanded from macro 'v4l_dbg'
           do {                                                                 \
           ^
   drivers/media/i2c/adv7842.c:3475:6: note: Assuming 'pdata' is non-null
           if (!pdata) {
               ^~~~~~
   drivers/media/i2c/adv7842.c:3475:2: note: Taking false branch
           if (!pdata) {
           ^
   drivers/media/i2c/adv7842.c:3481:6: note: Assuming 'state' is non-null
           if (!state)
               ^~~~~~
   drivers/media/i2c/adv7842.c:3481:2: note: Taking false branch
           if (!state)
           ^
   drivers/media/i2c/adv7842.c:3495:23: note: Assuming field 'input' is not equal to ADV7842_SELECT_HDMI_PORT_A
           state->hdmi_port_a = pdata->input == ADV7842_SELECT_HDMI_PORT_A;

vim +/ret +126 drivers/media/i2c/dw9719.c

8bbbe049053a26 Daniel Scally 2021-11-28  114  
8bbbe049053a26 Daniel Scally 2021-11-28  115  static int dw9719_detect(struct dw9719_device *dw9719)
8bbbe049053a26 Daniel Scally 2021-11-28  116  {
8bbbe049053a26 Daniel Scally 2021-11-28  117  	int ret;
8bbbe049053a26 Daniel Scally 2021-11-28  118  	u8 val;
8bbbe049053a26 Daniel Scally 2021-11-28  119  
8bbbe049053a26 Daniel Scally 2021-11-28  120  	ret = dw9719_i2c_rd8(dw9719->client, DW9719_INFO, &val);
8bbbe049053a26 Daniel Scally 2021-11-28  121  	if (ret < 0)
8bbbe049053a26 Daniel Scally 2021-11-28  122  		return ret;
8bbbe049053a26 Daniel Scally 2021-11-28  123  
8bbbe049053a26 Daniel Scally 2021-11-28  124  	if (val != DW9719_ID) {
8bbbe049053a26 Daniel Scally 2021-11-28  125  		dev_err(dw9719->dev, "Failed to detect correct id\n");
8bbbe049053a26 Daniel Scally 2021-11-28 @126  		ret = -ENXIO;
8bbbe049053a26 Daniel Scally 2021-11-28  127  	}
8bbbe049053a26 Daniel Scally 2021-11-28  128  
8bbbe049053a26 Daniel Scally 2021-11-28  129  	return 0;
8bbbe049053a26 Daniel Scally 2021-11-28  130  }
8bbbe049053a26 Daniel Scally 2021-11-28  131  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

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

end of thread, other threads:[~2022-01-19 15:14 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-28 23:21 [PATCH] media: i2c: Add driver for DW9719 VCM Daniel Scally
2022-01-19 12:19 ` Sakari Ailus
2022-01-19 15:14   ` Daniel Scally
2021-12-04 19:34 kernel test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.