Add support for the Pinefeat cef168 lens control board that provides
electronic focus and aperture control for Canon EF & EF-S lenses on
non-Canon camera bodies.

Signed-off-by: Aliaksandr Smirnou <support@pinefeat.co.uk>
---
 MAINTAINERS                |   2 +
 drivers/media/i2c/Kconfig  |   8 +
 drivers/media/i2c/Makefile |   1 +
 drivers/media/i2c/cef168.c | 335 +++++++++++++++++++++++++++++++++++++
 drivers/media/i2c/cef168.h |  51 ++++++
 5 files changed, 397 insertions(+)
 create mode 100644 drivers/media/i2c/cef168.c
 create mode 100644 drivers/media/i2c/cef168.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 811c6a150029..922efc000722 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19990,6 +19990,8 @@ M:	Aliaksandr Smirnou <support@pinefeat.co.uk>
 L:	linux-media@vger.kernel.org
 S:	Supported
 F:	Documentation/devicetree/bindings/media/i2c/pinefeat,cef168.yaml
+F:	drivers/media/i2c/cef168.c
+F:	drivers/media/i2c/cef168.h
 
 PLANTOWER PMS7003 AIR POLLUTION SENSOR DRIVER
 M:	Tomasz Duszynski <tduszyns@gmail.com>
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 6237fe804a5c..c4c3b03a0b98 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -791,6 +791,14 @@ config VIDEO_AK7375
 	  capability. This is designed for linear control of
 	  voice coil motors, controlled via I2C serial interface.
 
+config VIDEO_CEF168
+	tristate "CEF168 lens control support"
+	help
+	  This is a driver for the CEF168 lens control board.
+	  The board provides an I2C interface for electronic focus
+	  and aperture control of EF and EF-S lenses. The driver
+	  integrates with the V4L2 sub-device API.
+
 config VIDEO_DW9714
 	tristate "DW9714 lens voice coil support"
 	depends on GPIOLIB
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 5873d29433ee..75a95f850f18 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -25,6 +25,7 @@ obj-$(CONFIG_VIDEO_BT856) += bt856.o
 obj-$(CONFIG_VIDEO_BT866) += bt866.o
 obj-$(CONFIG_VIDEO_CCS) += ccs/
 obj-$(CONFIG_VIDEO_CCS_PLL) += ccs-pll.o
+obj-$(CONFIG_VIDEO_CEF168) += cef168.o
 obj-$(CONFIG_VIDEO_CS3308) += cs3308.o
 obj-$(CONFIG_VIDEO_CS5345) += cs5345.o
 obj-$(CONFIG_VIDEO_CS53L32A) += cs53l32a.o
diff --git a/drivers/media/i2c/cef168.c b/drivers/media/i2c/cef168.c
new file mode 100644
index 000000000000..563251a54835
--- /dev/null
+++ b/drivers/media/i2c/cef168.c
@@ -0,0 +1,335 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2025 Pinefeat LLP
+
+#include <linux/crc8.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-event.h>
+#include "cef168.h"
+
+/*
+ * cef168 device structure
+ */
+struct cef168_device {
+	struct v4l2_ctrl_handler ctrls;
+	struct v4l2_subdev sd;
+};
+
+static inline struct cef168_device *to_cef168(struct v4l2_ctrl *ctrl)
+{
+	return container_of(ctrl->handler, struct cef168_device, ctrls);
+}
+
+static inline struct cef168_device *sd_to_cef168(struct v4l2_subdev *subdev)
+{
+	return container_of(subdev, struct cef168_device, sd);
+}
+
+static int cef168_i2c_write(struct cef168_device *cef168_dev, u8 cmd, u16 val)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&cef168_dev->sd);
+	int retry, ret;
+
+	__le16 le_data = cpu_to_le16(val);
+	char tx_data[4] = { cmd, ((u8 *)&le_data)[0], ((u8 *)&le_data)[1] };
+
+	tx_data[3] = crc8(cef168_crc8_table, tx_data, 3, CRC8_INIT_VALUE);
+
+	for (retry = 0; retry < 3; retry++) {
+		ret = i2c_master_send(client, tx_data, sizeof(tx_data));
+		if (ret == sizeof(tx_data))
+			return 0;
+		else if (ret != -EIO && ret != -EREMOTEIO)
+			break;
+	}
+
+	dev_err(&client->dev, "I2C write fail after %d retries, ret=%d\n",
+		retry, ret);
+	return -EIO;
+}
+
+static int cef168_i2c_read(struct cef168_device *cef168_dev,
+			   struct cef168_data *rx_data)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&cef168_dev->sd);
+
+	int ret = i2c_master_recv(client, (char *)rx_data,
+				  sizeof(struct cef168_data));
+	if (ret != sizeof(struct cef168_data)) {
+		dev_err(&client->dev, "I2C read fail, ret=%d\n", ret);
+		return -EIO;
+	}
+
+	u8 computed_crc = crc8(cef168_crc8_table, (const u8 *)rx_data,
+			       sizeof(struct cef168_data) - 1, CRC8_INIT_VALUE);
+	if (computed_crc != rx_data->crc8) {
+		dev_err(&client->dev,
+			"CRC mismatch calculated=0x%02X read=0x%02X\n",
+			computed_crc, rx_data->crc8);
+		return -EIO;
+	}
+
+	rx_data->moving_time = le16_to_cpup((__le16 *)&rx_data->moving_time);
+	rx_data->focus_position_min = le16_to_cpup((__le16 *)&rx_data->focus_position_min);
+	rx_data->focus_position_max = le16_to_cpup((__le16 *)&rx_data->focus_position_max);
+	rx_data->focus_position_cur = le16_to_cpup((__le16 *)&rx_data->focus_position_cur);
+	rx_data->focus_distance_min = le16_to_cpup((__le16 *)&rx_data->focus_distance_min);
+	rx_data->focus_distance_max = le16_to_cpup((__le16 *)&rx_data->focus_distance_max);
+
+	return 0;
+}
+
+static int cef168_set_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct cef168_device *dev = to_cef168(ctrl);
+	u8 cmd;
+
+	switch (ctrl->id) {
+	case V4L2_CID_FOCUS_ABSOLUTE:
+		return cef168_i2c_write(dev, INP_SET_FOCUS, ctrl->val);
+	case V4L2_CID_FOCUS_RELATIVE:
+		cmd = ctrl->val < 0 ? INP_SET_FOCUS_N : INP_SET_FOCUS_P;
+		return cef168_i2c_write(dev, cmd, abs(ctrl->val));
+	case V4L2_CID_IRIS_ABSOLUTE:
+		return cef168_i2c_write(dev, INP_SET_APERTURE, ctrl->val);
+	case V4L2_CID_IRIS_RELATIVE:
+		cmd = ctrl->val < 0 ? INP_SET_APERTURE_N : INP_SET_APERTURE_P;
+		return cef168_i2c_write(dev, cmd, abs(ctrl->val));
+	case CEF168_V4L2_CID_CUSTOM(calibrate):
+		return cef168_i2c_write(dev, INP_CALIBRATE, 0);
+	}
+
+	return -EINVAL;
+}
+
+static int cef168_get_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct cef168_device *dev = to_cef168(ctrl);
+	int rval;
+
+	if (ctrl->id != V4L2_CID_FOCUS_ABSOLUTE &&
+	    ctrl->id != CEF168_V4L2_CID_CUSTOM(data) &&
+	    ctrl->id != CEF168_V4L2_CID_CUSTOM(focus_range) &&
+	    ctrl->id != CEF168_V4L2_CID_CUSTOM(lens_id))
+		return -EINVAL;
+
+	struct cef168_data data;
+
+	rval = cef168_i2c_read(dev, &data);
+	if (rval < 0)
+		return rval;
+
+	switch (ctrl->id) {
+	case V4L2_CID_FOCUS_ABSOLUTE:
+		ctrl->val = data.focus_position_cur;
+		return 0;
+	case CEF168_V4L2_CID_CUSTOM(focus_range):
+		ctrl->p_new.p_u32[0] = ((u32)data.focus_position_min << 16) |
+				       (u32)data.focus_position_max;
+		return 0;
+	case CEF168_V4L2_CID_CUSTOM(lens_id):
+		ctrl->p_new.p_u8[0] = data.lens_id;
+		return 0;
+	case CEF168_V4L2_CID_CUSTOM(data):
+		memcpy(ctrl->p_new.p_u8, &data, sizeof(data));
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static const struct v4l2_ctrl_ops cef168_ctrl_ops = {
+	.g_volatile_ctrl = cef168_get_ctrl,
+	.s_ctrl = cef168_set_ctrl,
+};
+
+static const struct v4l2_ctrl_config cef168_lens_id_ctrl = {
+	.ops = &cef168_ctrl_ops,
+	.id = CEF168_V4L2_CID_CUSTOM(lens_id),
+	.type = V4L2_CTRL_TYPE_U8,
+	.name = "Lens ID",
+	.min = 0,
+	.max = U8_MAX,
+	.step = 1,
+	.def = 0,
+	.flags = V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY,
+};
+
+static const struct v4l2_ctrl_config cef168_focus_range_ctrl = {
+	.ops = &cef168_ctrl_ops,
+	.id = CEF168_V4L2_CID_CUSTOM(focus_range),
+	.type = V4L2_CTRL_TYPE_U32,
+	.name = "Focus Range",
+	.min = 0,
+	.max = U32_MAX,
+	.step = 1,
+	.def = 0,
+	.flags = V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY,
+};
+
+static const struct v4l2_ctrl_config cef168_data_ctrl = {
+	.ops = &cef168_ctrl_ops,
+	.id = CEF168_V4L2_CID_CUSTOM(data),
+	.type = V4L2_CTRL_TYPE_U8,
+	.name = "Data",
+	.min = 0,
+	.max = U8_MAX,
+	.step = 1,
+	.def = 0,
+	.dims = { sizeof(struct cef168_data) / sizeof(u8) },
+	.elem_size = sizeof(u8),
+	.flags = V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY,
+};
+
+static const struct v4l2_ctrl_config cef168_calibrate_ctrl = {
+	.ops = &cef168_ctrl_ops,
+	.id = CEF168_V4L2_CID_CUSTOM(calibrate),
+	.type = V4L2_CTRL_TYPE_BUTTON,
+	.name = "Calibrate",
+};
+
+static int cef168_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+	return pm_runtime_resume_and_get(sd->dev);
+}
+
+static int cef168_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+	pm_runtime_put(sd->dev);
+	return 0;
+}
+
+static const struct v4l2_subdev_internal_ops cef168_int_ops = {
+	.open = cef168_open,
+	.close = cef168_close,
+};
+
+static const struct v4l2_subdev_core_ops cef168_core_ops = {
+	.log_status = v4l2_ctrl_subdev_log_status,
+	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
+	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
+};
+
+static const struct v4l2_subdev_ops cef168_ops = {
+	.core = &cef168_core_ops,
+};
+
+static void cef168_subdev_cleanup(struct cef168_device *cef168_dev)
+{
+	v4l2_async_unregister_subdev(&cef168_dev->sd);
+	v4l2_ctrl_handler_free(&cef168_dev->ctrls);
+	media_entity_cleanup(&cef168_dev->sd.entity);
+}
+
+static int cef168_init_controls(struct cef168_device *dev)
+{
+	struct v4l2_ctrl *ctrl;
+	struct v4l2_ctrl_handler *hdl = &dev->ctrls;
+	const struct v4l2_ctrl_ops *ops = &cef168_ctrl_ops;
+
+	v4l2_ctrl_handler_init(hdl, 8);
+
+	ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_FOCUS_ABSOLUTE, 0, S16_MAX,
+				 1, 0);
+	if (ctrl)
+		ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE |
+			       V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
+	v4l2_ctrl_new_std(hdl, ops, V4L2_CID_FOCUS_RELATIVE, S16_MIN, S16_MAX,
+			  1, 0);
+	ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_IRIS_ABSOLUTE, 0, S16_MAX,
+				 1, 0);
+	if (ctrl)
+		ctrl->flags |= V4L2_CTRL_FLAG_WRITE_ONLY |
+			       V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
+	v4l2_ctrl_new_std(hdl, ops, V4L2_CID_IRIS_RELATIVE, S16_MIN, S16_MAX, 1,
+			  0);
+	v4l2_ctrl_new_custom(hdl, &cef168_calibrate_ctrl, NULL);
+	v4l2_ctrl_new_custom(hdl, &cef168_focus_range_ctrl, NULL);
+	v4l2_ctrl_new_custom(hdl, &cef168_data_ctrl, NULL);
+	v4l2_ctrl_new_custom(hdl, &cef168_lens_id_ctrl, NULL);
+
+	if (hdl->error)
+		dev_err(dev->sd.dev, "%s fail error: 0x%x\n", __func__,
+			hdl->error);
+	dev->sd.ctrl_handler = hdl;
+	return hdl->error;
+}
+
+static int cef168_probe(struct i2c_client *client)
+{
+	struct cef168_device *cef168_dev;
+	int rval;
+
+	cef168_dev = devm_kzalloc(&client->dev, sizeof(*cef168_dev),
+				  GFP_KERNEL);
+	if (!cef168_dev)
+		return -ENOMEM;
+
+	v4l2_i2c_subdev_init(&cef168_dev->sd, client, &cef168_ops);
+	cef168_dev->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
+				V4L2_SUBDEV_FL_HAS_EVENTS;
+	cef168_dev->sd.internal_ops = &cef168_int_ops;
+
+	rval = cef168_init_controls(cef168_dev);
+	if (rval)
+		goto err_cleanup;
+
+	rval = media_entity_pads_init(&cef168_dev->sd.entity, 0, NULL);
+	if (rval < 0)
+		goto err_cleanup;
+
+	cef168_dev->sd.entity.function = MEDIA_ENT_F_LENS;
+
+	rval = v4l2_async_register_subdev(&cef168_dev->sd);
+	if (rval < 0)
+		goto err_cleanup;
+
+	crc8_populate_msb(cef168_crc8_table, CEF_CRC8_POLYNOMIAL);
+
+	pm_runtime_set_active(&client->dev);
+	pm_runtime_enable(&client->dev);
+	pm_runtime_idle(&client->dev);
+
+	return 0;
+
+err_cleanup:
+	v4l2_ctrl_handler_free(&cef168_dev->ctrls);
+	media_entity_cleanup(&cef168_dev->sd.entity);
+
+	return rval;
+}
+
+static void cef168_remove(struct i2c_client *client)
+{
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct cef168_device *cef168_dev = sd_to_cef168(sd);
+
+	pm_runtime_disable(&client->dev);
+	pm_runtime_set_suspended(&client->dev);
+	cef168_subdev_cleanup(cef168_dev);
+}
+
+static const struct of_device_id cef168_of_table[] = {
+	{ .compatible = "pinefeat,cef168" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, cef168_of_table);
+
+static struct i2c_driver cef168_i2c_driver = {
+	.driver = {
+		.name = CEF168_NAME,
+		.of_match_table = cef168_of_table,
+	},
+	.probe = cef168_probe,
+	.remove = cef168_remove,
+};
+
+module_i2c_driver(cef168_i2c_driver);
+
+MODULE_AUTHOR("support@pinefeat.co.uk>");
+MODULE_DESCRIPTION("CEF168 lens driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/media/i2c/cef168.h b/drivers/media/i2c/cef168.h
new file mode 100644
index 000000000000..cdce1a19bda0
--- /dev/null
+++ b/drivers/media/i2c/cef168.h
@@ -0,0 +1,51 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Pinefeat cef168 lens driver
+ *
+ * Copyright (c) 2025 Pinefeat LLP
+ */
+
+#ifndef CEF168_CEF168_H
+#define CEF168_CEF168_H
+
+#define CEF168_NAME "cef168"
+
+#define CEF168_V4L2_CID_CUSTOM(ctrl) \
+	((V4L2_CID_USER_BASE | 168) + custom_##ctrl)
+
+enum { custom_lens_id, custom_data, custom_focus_range, custom_calibrate };
+
+/**
+ * cef168 data structure
+ */
+struct cef168_data {
+	__u8 lens_id;
+	__u8 moving : 1;
+	__u8 calibrating : 2;
+	__u16 moving_time;
+	__u16 focus_position_min;
+	__u16 focus_position_max;
+	__u16 focus_position_cur;
+	__u16 focus_distance_min;
+	__u16 focus_distance_max;
+	__u8 crc8;
+} __packed;
+
+/*
+ * cef168 I2C protocol commands
+ */
+#define INP_CALIBRATE 0x22
+#define INP_SET_FOCUS 0x80
+#define INP_SET_FOCUS_P 0x81
+#define INP_SET_FOCUS_N 0x82
+#define INP_SET_APERTURE 0x7A
+#define INP_SET_APERTURE_P 0x7B
+#define INP_SET_APERTURE_N 0x7C
+
+#define CEF_CRC8_POLYNOMIAL 168
+
+#ifdef DECLARE_CRC8_TABLE
+DECLARE_CRC8_TABLE(cef168_crc8_table);
+#endif
+
+#endif //CEF168_CEF168_H
-- 
2.34.1

Hi Aliaksandr
  thanks for the patch

just a quick review, I'm not a v4l2-control expert so you might want
to wait for more reviews on that part..

On Sun, Aug 17, 2025 at 02:05:49PM +0100, Aliaksandr Smirnou wrote:
> Add support for the Pinefeat cef168 lens control board that provides
> electronic focus and aperture control for Canon EF & EF-S lenses on
> non-Canon camera bodies.
>
> Signed-off-by: Aliaksandr Smirnou <support@pinefeat.co.uk>
> ---
>  MAINTAINERS                |   2 +
>  drivers/media/i2c/Kconfig  |   8 +
>  drivers/media/i2c/Makefile |   1 +
>  drivers/media/i2c/cef168.c | 335 +++++++++++++++++++++++++++++++++++++
>  drivers/media/i2c/cef168.h |  51 ++++++
>  5 files changed, 397 insertions(+)
>  create mode 100644 drivers/media/i2c/cef168.c
>  create mode 100644 drivers/media/i2c/cef168.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 811c6a150029..922efc000722 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19990,6 +19990,8 @@ M:	Aliaksandr Smirnou <support@pinefeat.co.uk>
>  L:	linux-media@vger.kernel.org
>  S:	Supported
>  F:	Documentation/devicetree/bindings/media/i2c/pinefeat,cef168.yaml
> +F:	drivers/media/i2c/cef168.c
> +F:	drivers/media/i2c/cef168.h
>
>  PLANTOWER PMS7003 AIR POLLUTION SENSOR DRIVER
>  M:	Tomasz Duszynski <tduszyns@gmail.com>
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 6237fe804a5c..c4c3b03a0b98 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -791,6 +791,14 @@ config VIDEO_AK7375
>  	  capability. This is designed for linear control of
>  	  voice coil motors, controlled via I2C serial interface.
>
> +config VIDEO_CEF168
> +	tristate "CEF168 lens control support"
> +	help
> +	  This is a driver for the CEF168 lens control board.
> +	  The board provides an I2C interface for electronic focus
> +	  and aperture control of EF and EF-S lenses. The driver
> +	  integrates with the V4L2 sub-device API.
> +
>  config VIDEO_DW9714
>  	tristate "DW9714 lens voice coil support"
>  	depends on GPIOLIB
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index 5873d29433ee..75a95f850f18 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -25,6 +25,7 @@ obj-$(CONFIG_VIDEO_BT856) += bt856.o
>  obj-$(CONFIG_VIDEO_BT866) += bt866.o
>  obj-$(CONFIG_VIDEO_CCS) += ccs/
>  obj-$(CONFIG_VIDEO_CCS_PLL) += ccs-pll.o
> +obj-$(CONFIG_VIDEO_CEF168) += cef168.o
>  obj-$(CONFIG_VIDEO_CS3308) += cs3308.o
>  obj-$(CONFIG_VIDEO_CS5345) += cs5345.o
>  obj-$(CONFIG_VIDEO_CS53L32A) += cs53l32a.o
> diff --git a/drivers/media/i2c/cef168.c b/drivers/media/i2c/cef168.c
> new file mode 100644
> index 000000000000..563251a54835
> --- /dev/null
> +++ b/drivers/media/i2c/cef168.c
> @@ -0,0 +1,335 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2025 Pinefeat LLP
> +
> +#include <linux/crc8.h>

Do you need to

        select CRC8

in Kconfig then ?

> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-event.h>
> +#include "cef168.h"

Why an header file ? This isn't going to be included by any other
.c file, right ?

> +
> +/*
> + * cef168 device structure
> + */
> +struct cef168_device {
> +	struct v4l2_ctrl_handler ctrls;
> +	struct v4l2_subdev sd;
> +};
> +
> +static inline struct cef168_device *to_cef168(struct v4l2_ctrl *ctrl)
> +{
> +	return container_of(ctrl->handler, struct cef168_device, ctrls);
> +}
> +
> +static inline struct cef168_device *sd_to_cef168(struct v4l2_subdev *subdev)
> +{
> +	return container_of(subdev, struct cef168_device, sd);
> +}
> +
> +static int cef168_i2c_write(struct cef168_device *cef168_dev, u8 cmd, u16 val)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&cef168_dev->sd);
> +	int retry, ret;
> +
> +	__le16 le_data = cpu_to_le16(val);
> +	char tx_data[4] = { cmd, ((u8 *)&le_data)[0], ((u8 *)&le_data)[1] };
> +
> +	tx_data[3] = crc8(cef168_crc8_table, tx_data, 3, CRC8_INIT_VALUE);
> +
> +	for (retry = 0; retry < 3; retry++) {

This seems a bit random, why do you need to retry three times ?

> +		ret = i2c_master_send(client, tx_data, sizeof(tx_data));
> +		if (ret == sizeof(tx_data))
> +			return 0;
> +		else if (ret != -EIO && ret != -EREMOTEIO)
> +			break;
> +	}
> +
> +	dev_err(&client->dev, "I2C write fail after %d retries, ret=%d\n",
> +		retry, ret);
> +	return -EIO;
> +}
> +
> +static int cef168_i2c_read(struct cef168_device *cef168_dev,
> +			   struct cef168_data *rx_data)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&cef168_dev->sd);
> +
> +	int ret = i2c_master_recv(client, (char *)rx_data,
> +				  sizeof(struct cef168_data));
> +	if (ret != sizeof(struct cef168_data)) {
> +		dev_err(&client->dev, "I2C read fail, ret=%d\n", ret);
> +		return -EIO;
> +	}
> +
> +	u8 computed_crc = crc8(cef168_crc8_table, (const u8 *)rx_data,
> +			       sizeof(struct cef168_data) - 1, CRC8_INIT_VALUE);
> +	if (computed_crc != rx_data->crc8) {
> +		dev_err(&client->dev,
> +			"CRC mismatch calculated=0x%02X read=0x%02X\n",
> +			computed_crc, rx_data->crc8);
> +		return -EIO;
> +	}
> +
> +	rx_data->moving_time = le16_to_cpup((__le16 *)&rx_data->moving_time);
> +	rx_data->focus_position_min = le16_to_cpup((__le16 *)&rx_data->focus_position_min);
> +	rx_data->focus_position_max = le16_to_cpup((__le16 *)&rx_data->focus_position_max);
> +	rx_data->focus_position_cur = le16_to_cpup((__le16 *)&rx_data->focus_position_cur);
> +	rx_data->focus_distance_min = le16_to_cpup((__le16 *)&rx_data->focus_distance_min);
> +	rx_data->focus_distance_max = le16_to_cpup((__le16 *)&rx_data->focus_distance_max);
> +
> +	return 0;
> +}
> +
> +static int cef168_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct cef168_device *dev = to_cef168(ctrl);
> +	u8 cmd;
> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_FOCUS_ABSOLUTE:
> +		return cef168_i2c_write(dev, INP_SET_FOCUS, ctrl->val);
> +	case V4L2_CID_FOCUS_RELATIVE:
> +		cmd = ctrl->val < 0 ? INP_SET_FOCUS_N : INP_SET_FOCUS_P;
> +		return cef168_i2c_write(dev, cmd, abs(ctrl->val));
> +	case V4L2_CID_IRIS_ABSOLUTE:
> +		return cef168_i2c_write(dev, INP_SET_APERTURE, ctrl->val);
> +	case V4L2_CID_IRIS_RELATIVE:
> +		cmd = ctrl->val < 0 ? INP_SET_APERTURE_N : INP_SET_APERTURE_P;
> +		return cef168_i2c_write(dev, cmd, abs(ctrl->val));
> +	case CEF168_V4L2_CID_CUSTOM(calibrate):
> +		return cef168_i2c_write(dev, INP_CALIBRATE, 0);
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int cef168_get_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct cef168_device *dev = to_cef168(ctrl);
> +	int rval;
> +
> +	if (ctrl->id != V4L2_CID_FOCUS_ABSOLUTE &&
> +	    ctrl->id != CEF168_V4L2_CID_CUSTOM(data) &&
> +	    ctrl->id != CEF168_V4L2_CID_CUSTOM(focus_range) &&
> +	    ctrl->id != CEF168_V4L2_CID_CUSTOM(lens_id))
> +		return -EINVAL;

If you mark them WRITE_ONLY wouldn't the core take care of this ?

> +
> +	struct cef168_data data;

I thought the compiler would complain for variables declared not at
the beginning of the function

> +
> +	rval = cef168_i2c_read(dev, &data);
> +	if (rval < 0)
> +		return rval;
> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_FOCUS_ABSOLUTE:
> +		ctrl->val = data.focus_position_cur;
> +		return 0;
> +	case CEF168_V4L2_CID_CUSTOM(focus_range):
> +		ctrl->p_new.p_u32[0] = ((u32)data.focus_position_min << 16) |
> +				       (u32)data.focus_position_max;
> +		return 0;
> +	case CEF168_V4L2_CID_CUSTOM(lens_id):
> +		ctrl->p_new.p_u8[0] = data.lens_id;
> +		return 0;
> +	case CEF168_V4L2_CID_CUSTOM(data):
> +		memcpy(ctrl->p_new.p_u8, &data, sizeof(data));
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static const struct v4l2_ctrl_ops cef168_ctrl_ops = {
> +	.g_volatile_ctrl = cef168_get_ctrl,
> +	.s_ctrl = cef168_set_ctrl,
> +};
> +
> +static const struct v4l2_ctrl_config cef168_lens_id_ctrl = {
> +	.ops = &cef168_ctrl_ops,
> +	.id = CEF168_V4L2_CID_CUSTOM(lens_id),
> +	.type = V4L2_CTRL_TYPE_U8,
> +	.name = "Lens ID",
> +	.min = 0,
> +	.max = U8_MAX,
> +	.step = 1,
> +	.def = 0,
> +	.flags = V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY,
> +};
> +
> +static const struct v4l2_ctrl_config cef168_focus_range_ctrl = {
> +	.ops = &cef168_ctrl_ops,
> +	.id = CEF168_V4L2_CID_CUSTOM(focus_range),
> +	.type = V4L2_CTRL_TYPE_U32,
> +	.name = "Focus Range",
> +	.min = 0,
> +	.max = U32_MAX,
> +	.step = 1,
> +	.def = 0,
> +	.flags = V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY,
> +};
> +
> +static const struct v4l2_ctrl_config cef168_data_ctrl = {
> +	.ops = &cef168_ctrl_ops,
> +	.id = CEF168_V4L2_CID_CUSTOM(data),
> +	.type = V4L2_CTRL_TYPE_U8,
> +	.name = "Data",
> +	.min = 0,
> +	.max = U8_MAX,
> +	.step = 1,
> +	.def = 0,
> +	.dims = { sizeof(struct cef168_data) / sizeof(u8) },
> +	.elem_size = sizeof(u8),
> +	.flags = V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY,
> +};
> +
> +static const struct v4l2_ctrl_config cef168_calibrate_ctrl = {
> +	.ops = &cef168_ctrl_ops,
> +	.id = CEF168_V4L2_CID_CUSTOM(calibrate),
> +	.type = V4L2_CTRL_TYPE_BUTTON,
> +	.name = "Calibrate",
> +};
> +
> +static int cef168_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> +{
> +	return pm_runtime_resume_and_get(sd->dev);
> +}
> +
> +static int cef168_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> +{
> +	pm_runtime_put(sd->dev);
> +	return 0;
> +}
> +
> +static const struct v4l2_subdev_internal_ops cef168_int_ops = {
> +	.open = cef168_open,
> +	.close = cef168_close,
> +};
> +
> +static const struct v4l2_subdev_core_ops cef168_core_ops = {
> +	.log_status = v4l2_ctrl_subdev_log_status,
> +	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> +	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
> +};
> +
> +static const struct v4l2_subdev_ops cef168_ops = {
> +	.core = &cef168_core_ops,
> +};
> +
> +static void cef168_subdev_cleanup(struct cef168_device *cef168_dev)
> +{
> +	v4l2_async_unregister_subdev(&cef168_dev->sd);
> +	v4l2_ctrl_handler_free(&cef168_dev->ctrls);
> +	media_entity_cleanup(&cef168_dev->sd.entity);
> +}
> +
> +static int cef168_init_controls(struct cef168_device *dev)
> +{
> +	struct v4l2_ctrl *ctrl;
> +	struct v4l2_ctrl_handler *hdl = &dev->ctrls;
> +	const struct v4l2_ctrl_ops *ops = &cef168_ctrl_ops;
> +
> +	v4l2_ctrl_handler_init(hdl, 8);
> +
> +	ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_FOCUS_ABSOLUTE, 0, S16_MAX,
> +				 1, 0);
> +	if (ctrl)
> +		ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE |
> +			       V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
> +	v4l2_ctrl_new_std(hdl, ops, V4L2_CID_FOCUS_RELATIVE, S16_MIN, S16_MAX,
> +			  1, 0);
> +	ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_IRIS_ABSOLUTE, 0, S16_MAX,
> +				 1, 0);
> +	if (ctrl)
> +		ctrl->flags |= V4L2_CTRL_FLAG_WRITE_ONLY |
> +			       V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
> +	v4l2_ctrl_new_std(hdl, ops, V4L2_CID_IRIS_RELATIVE, S16_MIN, S16_MAX, 1,
> +			  0);
> +	v4l2_ctrl_new_custom(hdl, &cef168_calibrate_ctrl, NULL);
> +	v4l2_ctrl_new_custom(hdl, &cef168_focus_range_ctrl, NULL);
> +	v4l2_ctrl_new_custom(hdl, &cef168_data_ctrl, NULL);
> +	v4l2_ctrl_new_custom(hdl, &cef168_lens_id_ctrl, NULL);
> +
> +	if (hdl->error)
> +		dev_err(dev->sd.dev, "%s fail error: 0x%x\n", __func__,
> +			hdl->error);
> +	dev->sd.ctrl_handler = hdl;
> +	return hdl->error;
> +}
> +
> +static int cef168_probe(struct i2c_client *client)
> +{
> +	struct cef168_device *cef168_dev;
> +	int rval;
> +
> +	cef168_dev = devm_kzalloc(&client->dev, sizeof(*cef168_dev),
> +				  GFP_KERNEL);
> +	if (!cef168_dev)
> +		return -ENOMEM;
> +
> +	v4l2_i2c_subdev_init(&cef168_dev->sd, client, &cef168_ops);
> +	cef168_dev->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> +				V4L2_SUBDEV_FL_HAS_EVENTS;
> +	cef168_dev->sd.internal_ops = &cef168_int_ops;
> +
> +	rval = cef168_init_controls(cef168_dev);
> +	if (rval)
> +		goto err_cleanup;
> +
> +	rval = media_entity_pads_init(&cef168_dev->sd.entity, 0, NULL);
> +	if (rval < 0)
> +		goto err_cleanup;
> +
> +	cef168_dev->sd.entity.function = MEDIA_ENT_F_LENS;
> +
> +	rval = v4l2_async_register_subdev(&cef168_dev->sd);
> +	if (rval < 0)
> +		goto err_cleanup;
> +
> +	crc8_populate_msb(cef168_crc8_table, CEF_CRC8_POLYNOMIAL);
> +
> +	pm_runtime_set_active(&client->dev);

Is the device powered up at this point ?

> +	pm_runtime_enable(&client->dev);
> +	pm_runtime_idle(&client->dev);

If you depend on the pm_runtime_resume_and_get() call in open() to
power the device up, then you need to depend on PM in KConfig ?

> +
> +	return 0;
> +
> +err_cleanup:
> +	v4l2_ctrl_handler_free(&cef168_dev->ctrls);
> +	media_entity_cleanup(&cef168_dev->sd.entity);
> +
> +	return rval;
> +}
> +
> +static void cef168_remove(struct i2c_client *client)
> +{
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct cef168_device *cef168_dev = sd_to_cef168(sd);
> +
> +	pm_runtime_disable(&client->dev);
> +	pm_runtime_set_suspended(&client->dev);
> +	cef168_subdev_cleanup(cef168_dev);
> +}
> +
> +static const struct of_device_id cef168_of_table[] = {
> +	{ .compatible = "pinefeat,cef168" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, cef168_of_table);
> +
> +static struct i2c_driver cef168_i2c_driver = {
> +	.driver = {
> +		.name = CEF168_NAME,
> +		.of_match_table = cef168_of_table,
> +	},
> +	.probe = cef168_probe,
> +	.remove = cef168_remove,
> +};
> +
> +module_i2c_driver(cef168_i2c_driver);
> +
> +MODULE_AUTHOR("support@pinefeat.co.uk>");
> +MODULE_DESCRIPTION("CEF168 lens driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/media/i2c/cef168.h b/drivers/media/i2c/cef168.h
> new file mode 100644
> index 000000000000..cdce1a19bda0
> --- /dev/null
> +++ b/drivers/media/i2c/cef168.h
> @@ -0,0 +1,51 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Pinefeat cef168 lens driver
> + *
> + * Copyright (c) 2025 Pinefeat LLP
> + */
> +
> +#ifndef CEF168_CEF168_H
> +#define CEF168_CEF168_H
> +
> +#define CEF168_NAME "cef168"
> +
> +#define CEF168_V4L2_CID_CUSTOM(ctrl) \
> +	((V4L2_CID_USER_BASE | 168) + custom_##ctrl)

I think you need to reserve space for your controls in
include/uapi/linux/v4l2-controls.h

otherwise this will never be visible to applications ?

> +
> +enum { custom_lens_id, custom_data, custom_focus_range, custom_calibrate };
> +
> +/**

No need to kerneldoc unless you properly document all fields and
include the file in some of the Documentation/

> + * cef168 data structure
> + */
> +struct cef168_data {
> +	__u8 lens_id;
> +	__u8 moving : 1;
> +	__u8 calibrating : 2;
> +	__u16 moving_time;
> +	__u16 focus_position_min;
> +	__u16 focus_position_max;
> +	__u16 focus_position_cur;
> +	__u16 focus_distance_min;
> +	__u16 focus_distance_max;
> +	__u8 crc8;
> +} __packed;
> +
> +/*
> + * cef168 I2C protocol commands
> + */
> +#define INP_CALIBRATE 0x22
> +#define INP_SET_FOCUS 0x80
> +#define INP_SET_FOCUS_P 0x81
> +#define INP_SET_FOCUS_N 0x82
> +#define INP_SET_APERTURE 0x7A
> +#define INP_SET_APERTURE_P 0x7B
> +#define INP_SET_APERTURE_N 0x7C
> +
> +#define CEF_CRC8_POLYNOMIAL 168
> +
> +#ifdef DECLARE_CRC8_TABLE
> +DECLARE_CRC8_TABLE(cef168_crc8_table);
> +#endif
> +
> +#endif //CEF168_CEF168_H
> --
> 2.34.1
>
>

Hi Jacopo,

Thank you for the review. Your remarks are very helpful. While I'll apply
most of them, could you clarify the one regarding pm_runtime_ for me?

On Tue, 19 Aug 2025 15:47:54 +0200, Jacopo Mondi wrote:
> > +#include <linux/crc8.h>
>
> Do you need to "select CRC8" in Kconfig then ?

Yes, I'll include it.

> > +#include "cef168.h"
>
> Why an header file ?

Ok, I'll remove the header moving everying in the .c file.

> > +	for (retry = 0; retry < 3; retry++) {
>
> This seems a bit random, why do you need to retry three times ?

The driver retries writes to work around an issue in the Raspberry
Pi's I2C hardware, where the BCM2835 mishandles clock stretching.
When the slave stretches the clock, the Pi can misread the SCL line
or sample data too early, making it think the write failed. To
improve reliability, the kernel driver automatically retries the
write, effectively compensating for the hardware's timing bug.

> > +	    ctrl->id != CEF168_V4L2_CID_CUSTOM(data) &&
> > +	    ctrl->id != CEF168_V4L2_CID_CUSTOM(focus_range) &&
> > +	    ctrl->id != CEF168_V4L2_CID_CUSTOM(lens_id))
> > +		return -EINVAL;
>
> If you mark them WRITE_ONLY wouldn't the core take care of this ?

These controls are read-only. The data they return depens on the lens.

> > +	struct cef168_data data;
>
> I thought the compiler would complain for variables declared not at
> the beginning of the function

Ok, I'll move the variable at the beginning.

> > +	pm_runtime_set_active(&client->dev);
> 
> Is the device powered up at this point ?
> If you depend on the pm_runtime_resume_and_get() call in open() to
> power the device up, then you need to depend on PM in KConfig ?

Yes, the device powers from 3v3 rail of a SBC, which makes it powered
up as soon as the SBC is up. Given that, should I remove all code
around Power Management Runtime (pm_runtime_*) as redundant?

> > +#define CEF168_V4L2_CID_CUSTOM(ctrl) \
> > +	((V4L2_CID_USER_BASE | 168) + custom_##ctrl)
> 
> I think you need to reserve space for your controls in
> include/uapi/linux/v4l2-controls.h
>
> otherwise this will never be visible to applications ?

I found there is no need for that. Custom control become available
automatically by name via the v4l2-ctl utility. For example, the focus
range can be read directly in the terminal as follows:

v4l2-ctl -d $DEV_LENS -C focus_range

> > +/**
> > + * cef168 data structure
>
> No need to kerneldoc unless you properly document all fields and
> include the file in some of the Documentation/

OK, I'll remove the comment above the structure.

Kind regards,
  Aliaksandr

Hello
  +cc Hans

On Tue, Aug 19, 2025 at 10:32:34PM +0100, Aliaksandr Smirnou wrote:
> Hi Jacopo,
>
> Thank you for the review. Your remarks are very helpful. While I'll apply
> most of them, could you clarify the one regarding pm_runtime_ for me?
>
> On Tue, 19 Aug 2025 15:47:54 +0200, Jacopo Mondi wrote:
> > > +#include <linux/crc8.h>
> >
> > Do you need to "select CRC8" in Kconfig then ?
>
> Yes, I'll include it.
>
> > > +#include "cef168.h"
> >
> > Why an header file ?
>
> Ok, I'll remove the header moving everying in the .c file.
>
> > > +	for (retry = 0; retry < 3; retry++) {
> >
> > This seems a bit random, why do you need to retry three times ?
>
> The driver retries writes to work around an issue in the Raspberry
> Pi's I2C hardware, where the BCM2835 mishandles clock stretching.
> When the slave stretches the clock, the Pi can misread the SCL line
> or sample data too early, making it think the write failed. To
> improve reliability, the kernel driver automatically retries the
> write, effectively compensating for the hardware's timing bug.
>

mmm, usually a mainline driver is not the right place for introducing in
workarounds for a specific host/soc, as the assumption is that the
driver should work on all platforms equally.

Your case is certainly different as this product is pi-specific...

With a comment I wouldn't mind too much. Maybe others do :)

> > > +	    ctrl->id != CEF168_V4L2_CID_CUSTOM(data) &&
> > > +	    ctrl->id != CEF168_V4L2_CID_CUSTOM(focus_range) &&
> > > +	    ctrl->id != CEF168_V4L2_CID_CUSTOM(lens_id))
> > > +		return -EINVAL;
> >
> > If you mark them WRITE_ONLY wouldn't the core take care of this ?
>
> These controls are read-only. The data they return depens on the lens.
>

Sorry, I wasn't clear.

If you mark as WO the controls you don't accept here, will the core
handle this for you ?

> > > +	struct cef168_data data;
> >
> > I thought the compiler would complain for variables declared not at
> > the beginning of the function
>
> Ok, I'll move the variable at the beginning.
>
> > > +	pm_runtime_set_active(&client->dev);
> >
> > Is the device powered up at this point ?
> > If you depend on the pm_runtime_resume_and_get() call in open() to
> > power the device up, then you need to depend on PM in KConfig ?
>
> Yes, the device powers from 3v3 rail of a SBC, which makes it powered
> up as soon as the SBC is up. Given that, should I remove all code
> around Power Management Runtime (pm_runtime_*) as redundant?
>

Yes, I overlooked the fact you don't regiser any resume/suspend
routine, so your calls to pm_runtime_set_active/pm_runtime_put are
nops (they do at least provide usage counting, but I would anyway
remove them).

> > > +#define CEF168_V4L2_CID_CUSTOM(ctrl) \
> > > +	((V4L2_CID_USER_BASE | 168) + custom_##ctrl)
> >
> > I think you need to reserve space for your controls in
> > include/uapi/linux/v4l2-controls.h
> >
> > otherwise this will never be visible to applications ?
>
> I found there is no need for that. Custom control become available
> automatically by name via the v4l2-ctl utility. For example, the focus
> range can be read directly in the terminal as follows:
>
> v4l2-ctl -d $DEV_LENS -C focus_range
>

Yes the driver enuemrates them, but you need to add them to the main
header, otherwise USER_BASE | 168 will be take by someone else.

See in example 7c8c957ef12c41968adb66d785ce1dd5fb2f96e7 which is the
latest commit that adds a custom control space.

Hans, do we need a uapi header file with the definition of the custom
controls registered by this driver ?

Thanks
  j

> > > +/**
> > > + * cef168 data structure
> >
> > No need to kerneldoc unless you properly document all fields and
> > include the file in some of the Documentation/
>
> OK, I'll remove the comment above the structure.
>
> Kind regards,
>   Aliaksandr

On Wed, 20 Aug 2025 14:56:38 +0200, Jacopo Mondi wrote:
> > > > +	    ctrl->id != CEF168_V4L2_CID_CUSTOM(data) &&
> > > > +	    ctrl->id != CEF168_V4L2_CID_CUSTOM(focus_range) &&
> > > > +	    ctrl->id != CEF168_V4L2_CID_CUSTOM(lens_id))
> > > > +		return -EINVAL;
> > >
> > > If you mark them WRITE_ONLY wouldn't the core take care of this ?
> >
> > These controls are read-only. The data they return depens on the lens.
> >
>
> Sorry, I wasn't clear.
> 
> If you mark as WO the controls you don't accept here, will the core
> handle this for you ?

I see what you mean now. Indeed, the other controls are alredy WO, so the
core will not let them pass here. I'll remove this check as redundant.

> > > > +#define CEF168_V4L2_CID_CUSTOM(ctrl) \
> > > > +	((V4L2_CID_USER_BASE | 168) + custom_##ctrl)
> > >
> > > I think you need to reserve space for your controls in
> > > include/uapi/linux/v4l2-controls.h
> > >
> > > otherwise this will never be visible to applications ?
> >
> > I found there is no need for that. Custom control become available
> > automatically by name via the v4l2-ctl utility. For example, the focus
> > range can be read directly in the terminal as follows:
> >
> > v4l2-ctl -d $DEV_LENS -C focus_range
> >
> 
> Yes the driver enuemrates them, but you need to add them to the main
> header, otherwise USER_BASE | 168 will be take by someone else.
> 

I see, ok, I'll reserve 16 controls for this driver in v4l2-controls.h.

Thank you for your help.

Kind regards,
  Aliaksandr