[PATCH v3 0/2] Pinefeat cef168 lens control board driver

[PATCH v3 0/2] Pinefeat cef168 lens control board driver

[Aliaksandr Smirnou]

This patch series adds support for the Pinefeat adapter, which interfaces
Canon EF and EF-S lenses to non-Canon camera bodies. The cef168 circuit
control board provides an I2C interface for electronic focus and aperture
control. The driver integrates with the V4L2 sub-device API.

For more information about the product, see:
https://github.com/pinefeat/cef168

Changes in v3:
 - removed vcc-supply property and example
 - fixed incorrect type in assignment
 - fixed cast to restricted
 - removed unreachable code
 - changed comparison to NULL
 - fixed indent in commit message

Link to v2: https://lore.kernel.org/all/20250811213102.15703-1-aliaksandr.smirnou@gmail.com/

Patches:
  dt-bindings: Pinefeat cef168 lens control board
  media: i2c: Pinefeat cef168 lens control board driver

 .../bindings/media/i2c/pinefeat,cef168.yaml   |  48 +++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 MAINTAINERS                                   |   8 +
 drivers/media/i2c/Kconfig                     |   8 +
 drivers/media/i2c/Makefile                    |   1 +
 drivers/media/i2c/cef168.c                    | 335 ++++++++++++++++++
 drivers/media/i2c/cef168.h                    |  51 +++
 7 files changed, 453 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/pinefeat,cef168.yaml
 create mode 100644 drivers/media/i2c/cef168.c
 create mode 100644 drivers/media/i2c/cef168.h


base-commit: 2b38afce25c4e1b8f943ff4f0a2b51d6c40f2ed2
-- 
2.34.1

[PATCH v3 1/2] dt-bindings: Pinefeat cef168 lens control board

[Aliaksandr Smirnou]

Add the Device Tree schema and examples for the Pinefeat cef168 lens
control board. This board interfaces Canon EF & EF-S lenses with
non-Canon camera bodies, enabling electronic control of focus and
aperture via V4L2.

Power supply is derived from fixed supplies via connector or GPIO
header. Therefore, the driver does not manage any regulator, so
representing any supply in the binding is redundant.

Signed-off-by: Aliaksandr Smirnou <support@pinefeat.co.uk>
---
 .../bindings/media/i2c/pinefeat,cef168.yaml   | 48 +++++++++++++++++++
 .../devicetree/bindings/vendor-prefixes.yaml  |  2 +
 MAINTAINERS                                   |  6 +++
 3 files changed, 56 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/pinefeat,cef168.yaml

diff --git a/Documentation/devicetree/bindings/media/i2c/pinefeat,cef168.yaml b/Documentation/devicetree/bindings/media/i2c/pinefeat,cef168.yaml
new file mode 100644
index 000000000000..80cdff40d175
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/pinefeat,cef168.yaml
@@ -0,0 +1,48 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (c) 2025 Pinefeat LLP
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/pinefeat,cef168.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Pinefeat cef168 lens driver
+
+maintainers:
+  - Aliaksandr Smirnou <support@pinefeat.co.uk>
+
+description: |
+  Pinefeat produces an adapter designed to interface between
+  Canon EF & EF-S lenses and non-Canon camera bodies, incorporating
+  features for electronic focus and aperture adjustment. The cef168
+  circuit board, included with the adapter, provides a software
+  programming interface that allows control of lens focus and
+  aperture positions. This driver enables controlling the lens
+  focus and aperture via the V4L2 (Video4Linux2) API.
+
+properties:
+  compatible:
+    enum:
+      - pinefeat,cef168
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        camera-lens@d {
+            compatible = "pinefeat,cef168";
+            reg = <0x0d>;
+        };
+    };
+
+...
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 77160cd47f54..dab27f769b0a 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -1195,6 +1195,8 @@ patternProperties:
     description: Picochip Ltd
   "^pine64,.*":
     description: Pine64
+  "^pinefeat,.*":
+    description: Pinefeat LLP
   "^pineriver,.*":
     description: Shenzhen PineRiver Designs Co., Ltd.
   "^pixcir,.*":
diff --git a/MAINTAINERS b/MAINTAINERS
index fe168477caa4..811c6a150029 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19985,6 +19985,12 @@ S:	Supported
 F:	Documentation/devicetree/bindings/input/pine64,pinephone-keyboard.yaml
 F:	drivers/input/keyboard/pinephone-keyboard.c
 
+PINEFEAT CEF168 LENS DRIVER
+M:	Aliaksandr Smirnou <support@pinefeat.co.uk>
+L:	linux-media@vger.kernel.org
+S:	Supported
+F:	Documentation/devicetree/bindings/media/i2c/pinefeat,cef168.yaml
+
 PLANTOWER PMS7003 AIR POLLUTION SENSOR DRIVER
 M:	Tomasz Duszynski <tduszyns@gmail.com>
 S:	Maintained
-- 
2.34.1

Re: [PATCH v3 1/2] dt-bindings: Pinefeat cef168 lens control board

[Conor Dooley]

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

On Sun, Aug 17, 2025 at 02:05:48PM +0100, Aliaksandr Smirnou wrote:
> Add the Device Tree schema and examples for the Pinefeat cef168 lens
> control board. This board interfaces Canon EF & EF-S lenses with
> non-Canon camera bodies, enabling electronic control of focus and
> aperture via V4L2.
> 

> Power supply is derived from fixed supplies via connector or GPIO
> header. Therefore, the driver does not manage any regulator, so
> representing any supply in the binding is redundant.

Wut? This doesn't make sense, you have supplies so they should be
documented. The fact that they're shared with a bunch of other things on
the SBC you're aiming the product at doesn't matter. What if someone
doesn't use this sensor with an RPi and there is a dedicated regulator?

> Signed-off-by: Aliaksandr Smirnou <support@pinefeat.co.uk>

Is that actually your email address?

> ---
>  .../bindings/media/i2c/pinefeat,cef168.yaml   | 48 +++++++++++++++++++
>  .../devicetree/bindings/vendor-prefixes.yaml  |  2 +
>  MAINTAINERS                                   |  6 +++
>  3 files changed, 56 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/pinefeat,cef168.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/pinefeat,cef168.yaml b/Documentation/devicetree/bindings/media/i2c/pinefeat,cef168.yaml
> new file mode 100644
> index 000000000000..80cdff40d175
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/pinefeat,cef168.yaml
> @@ -0,0 +1,48 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (c) 2025 Pinefeat LLP
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/pinefeat,cef168.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Pinefeat cef168 lens driver
> +
> +maintainers:
> +  - Aliaksandr Smirnou <support@pinefeat.co.uk>
> +
> +description: |
> +  Pinefeat produces an adapter designed to interface between
> +  Canon EF & EF-S lenses and non-Canon camera bodies, incorporating
> +  features for electronic focus and aperture adjustment. The cef168
> +  circuit board, included with the adapter, provides a software
> +  programming interface that allows control of lens focus and
> +  aperture positions.

> This driver enables controlling the lens
> +  focus and aperture via the V4L2 (Video4Linux2) API.

Don't mention drivers in bindings, how linux handles things is not
relevant to a hardware description.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 1/2] dt-bindings: Pinefeat cef168 lens control board

[Aliaksandr Smirnou]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=yes, Size: 871 bytes --]

On Mon, 18 Aug 2025 18:36:50 +0100, Conor Dooley wrote:
> Wut? This doesn't make sense, you have supplies so they should be
> documented. The fact that they're shared with a bunch of other things on
> the SBC you're aiming the product at doesn't matter. What if someone
> doesn't use this sensor with an RPi and there is a dedicated regulator?

The supply was introduced in the second patch version but later removed
following feedback from another maintainer. I’m fine with adding it back
if needed - I just want to make sure we’re consistent.

https://lore.kernel.org/all/c1b848b9-b1da-4976-9838-d474394a0992@kernel.org/

Would you prefer it described as before?

> Is that actually your email address?

will change to a named one

> Don't mention drivers in bindings, how linux handles things is not
relevant to a hardware description.

will remove driver mention

Re: [PATCH v3 1/2] dt-bindings: Pinefeat cef168 lens control board

[Krzysztof Kozlowski]

On Mon, Aug 18, 2025 at 06:36:50PM +0100, Conor Dooley wrote:
> On Sun, Aug 17, 2025 at 02:05:48PM +0100, Aliaksandr Smirnou wrote:
> > Add the Device Tree schema and examples for the Pinefeat cef168 lens
> > control board. This board interfaces Canon EF & EF-S lenses with
> > non-Canon camera bodies, enabling electronic control of focus and
> > aperture via V4L2.
> > 
> 
> > Power supply is derived from fixed supplies via connector or GPIO
> > header. Therefore, the driver does not manage any regulator, so
> > representing any supply in the binding is redundant.
> 
> Wut? This doesn't make sense, you have supplies so they should be
> documented. The fact that they're shared with a bunch of other things on
> the SBC you're aiming the product at doesn't matter. What if someone

There is also some explanation at v2 discussion. I asked for that
because there is no known design (neither RPi or other boards having
compatible hat/connectors) which would have these supplies controllable.
Adding them now would mean you should make them also required (because
in fact they are), but since these are non-controllable there would be
just regulator-fixed with voltage and that's it. It's just bloat.

> doesn't use this sensor with an RPi and there is a dedicated regulator?

Unlikely but if ever such hardware appears, we can always add
regulators.

Best regards,
Krzysztof

[PATCH v3 2/2] media: i2c: Pinefeat cef168 lens control board driver

[Aliaksandr Smirnou]

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

Re: [PATCH v3 2/2] media: i2c: Pinefeat cef168 lens control board driver

[Jacopo Mondi]

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
>
>

Re: [PATCH v3 2/2] media: i2c: Pinefeat cef168 lens control board driver

[Aliaksandr Smirnou]

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

Re: [PATCH v3 2/2] media: i2c: Pinefeat cef168 lens control board driver

[Jacopo Mondi]

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

Re: [PATCH v3 2/2] media: i2c: Pinefeat cef168 lens control board driver

[Aliaksandr Smirnou]

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