[PATCH 1/2] Documentation: dt-bindings: media: i2c: Add ROHM BU64754 bindings

[PATCH 1/2] Documentation: dt-bindings: media: i2c: Add ROHM BU64754 bindings

[Kieran Bingham]

Add YAML device tree bindings for the ROHM BU64754 VCM Motor Driver for
Camera Autofocus.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 .../bindings/media/i2c/rohm,bu64754.yaml      | 48 +++++++++++++++++++
 MAINTAINERS                                   |  7 +++
 2 files changed, 55 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/rohm,bu64754.yaml

diff --git a/Documentation/devicetree/bindings/media/i2c/rohm,bu64754.yaml b/Documentation/devicetree/bindings/media/i2c/rohm,bu64754.yaml
new file mode 100644
index 000000000000..22da4a46bb0c
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/rohm,bu64754.yaml
@@ -0,0 +1,48 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) 2023 Ideas on Board Oy.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/rohm,bu64754.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ROHM BU64754 Actuator Driver for Camera Autofocus
+
+maintainers:
+  - Kieran Bingham <kieran.bingham@ideasonboard.com>
+
+description: |
+  The BU64754 is an actuator driver IC which can control the actuator
+  position precisely using an internal Hall Sensor.
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - rohm,bu64754
+
+  reg:
+    maxItems: 1
+
+  vdd-supply:
+    description:
+      Definition of the regulator used as VDD power supply to the driver.
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        lens@76 {
+            compatible = "rohm,bu64754";
+            reg = <0x76>;
+            vdd-supply = <&cam1_reg>;
+        };
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 90f13281d297..f43e0ffcaf56 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18570,6 +18570,13 @@ S:	Supported
 F:	drivers/iio/light/rohm-bu27008.c
 F:	drivers/iio/light/rohm-bu27034.c
 
+ROHM BU64754 MOTOR DRIVER FOR CAMERA AUTOFOCUS
+M:	Kieran Bingham <kieran.bingham@ideasonboard.com>
+L:	linux-media@vger.kernel.org
+S:	Maintained
+T:	git git://linuxtv.org/media_tree.git
+F:	Documentation/devicetree/bindings/media/i2c/rohm,bu64754.yaml
+
 ROHM MULTIFUNCTION BD9571MWV-M PMIC DEVICE DRIVERS
 M:	Marek Vasut <marek.vasut+renesas@gmail.com>
 L:	linux-kernel@vger.kernel.org
-- 
2.34.1

Re: [PATCH 1/2] Documentation: dt-bindings: media: i2c: Add ROHM BU64754 bindings

[Conor Dooley]

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

Yo,

On Fri, Sep 15, 2023 at 05:59:45PM +0100, Kieran Bingham wrote:
> Add YAML device tree bindings for the ROHM BU64754 VCM Motor Driver for
> Camera Autofocus.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  .../bindings/media/i2c/rohm,bu64754.yaml      | 48 +++++++++++++++++++
>  MAINTAINERS                                   |  7 +++
>  2 files changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/rohm,bu64754.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/rohm,bu64754.yaml b/Documentation/devicetree/bindings/media/i2c/rohm,bu64754.yaml
> new file mode 100644
> index 000000000000..22da4a46bb0c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/rohm,bu64754.yaml
> @@ -0,0 +1,48 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) 2023 Ideas on Board Oy.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/rohm,bu64754.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ROHM BU64754 Actuator Driver for Camera Autofocus
> +
> +maintainers:
> +  - Kieran Bingham <kieran.bingham@ideasonboard.com>
> +
> +description: |

That | is surplus to requirements since you have no formatting to
preserve.

> +  The BU64754 is an actuator driver IC which can control the actuator
> +  position precisely using an internal Hall Sensor.
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - rohm,bu64754

Why such a complicated pattern when you only have one item here?
Why not just const: rohm,bu64754?

Otherwise, looks fine to me.

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

Re: [PATCH 1/2] Documentation: dt-bindings: media: i2c: Add ROHM BU64754 bindings

[Kieran Bingham]

Quoting Conor Dooley (2023-09-15 22:13:40)
> Yo,
> 
> On Fri, Sep 15, 2023 at 05:59:45PM +0100, Kieran Bingham wrote:
> > Add YAML device tree bindings for the ROHM BU64754 VCM Motor Driver for
> > Camera Autofocus.
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  .../bindings/media/i2c/rohm,bu64754.yaml      | 48 +++++++++++++++++++
> >  MAINTAINERS                                   |  7 +++
> >  2 files changed, 55 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/i2c/rohm,bu64754.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/rohm,bu64754.yaml b/Documentation/devicetree/bindings/media/i2c/rohm,bu64754.yaml
> > new file mode 100644
> > index 000000000000..22da4a46bb0c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/rohm,bu64754.yaml
> > @@ -0,0 +1,48 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +# Copyright (C) 2023 Ideas on Board Oy.
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/i2c/rohm,bu64754.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: ROHM BU64754 Actuator Driver for Camera Autofocus
> > +
> > +maintainers:
> > +  - Kieran Bingham <kieran.bingham@ideasonboard.com>
> > +
> > +description: |
> 
> That | is surplus to requirements since you have no formatting to
> preserve.
> 
> > +  The BU64754 is an actuator driver IC which can control the actuator
> > +  position precisely using an internal Hall Sensor.
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - rohm,bu64754
> 
> Why such a complicated pattern when you only have one item here?
> Why not just const: rohm,bu64754?

These devices are really simple. I'd expect other versions/variants to
be able to be covered by the same driver in the future, but I can remove
this for now and simplify.

> 
> Otherwise, looks fine to me.


Thanks for the review.

--
Kieran

Re: [PATCH 1/2] Documentation: dt-bindings: media: i2c: Add ROHM BU64754 bindings

[Krzysztof Kozlowski]

On 15/09/2023 18:59, Kieran Bingham wrote:
> Add YAML device tree bindings for the ROHM BU64754 VCM Motor Driver for
> Camera Autofocus.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---

...

> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - rohm,bu64754
> +
> +  reg:
> +    maxItems: 1
> +
> +  vdd-supply:

If the supply is not required, how the driver gets any power?

> +    description:
> +      Definition of the regulator used as VDD power supply to the driver.
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        lens@76 {
> +            compatible = "rohm,bu64754";
> +            reg = <0x76>;
> +            vdd-supply = <&cam1_reg>;
> +        };
> +    };
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 90f13281d297..f43e0ffcaf56 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18570,6 +18570,13 @@ S:	Supported
>  F:	drivers/iio/light/rohm-bu27008.c
>  F:	drivers/iio/light/rohm-bu27034.c
>  
> +ROHM BU64754 MOTOR DRIVER FOR CAMERA AUTOFOCUS
> +M:	Kieran Bingham <kieran.bingham@ideasonboard.com>
> +L:	linux-media@vger.kernel.org
> +S:	Maintained
> +T:	git git://linuxtv.org/media_tree.git

Do you have push rights there? The git entry is for entries where you
maintain, which means: you accept and apply patches. This does not look
like the case, so copying subsystem Git entry is pointless. Subsystem
maintainer defines the git tree, not you.

Best regards,
Krzysztof

Re: [PATCH 1/2] Documentation: dt-bindings: media: i2c: Add ROHM BU64754 bindings

[Kieran Bingham]

Quoting Krzysztof Kozlowski (2023-09-17 10:37:07)
> On 15/09/2023 18:59, Kieran Bingham wrote:
> > Add YAML device tree bindings for the ROHM BU64754 VCM Motor Driver for
> > Camera Autofocus.
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> 
> ...
> 
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - rohm,bu64754
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  vdd-supply:
> 
> If the supply is not required, how the driver gets any power?

It may not be controllable. It could be wired to be always on.

The device has a low power mode which is controllable by software, so it
could be always powered on and controlled via software only if desired.
The driver handles that already as if the regulator isn't specified the
power down register bits will be set to put it in low power mode.


> 
> > +    description:
> > +      Definition of the regulator used as VDD power supply to the driver.
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        lens@76 {
> > +            compatible = "rohm,bu64754";
> > +            reg = <0x76>;
> > +            vdd-supply = <&cam1_reg>;
> > +        };
> > +    };
> > +...
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 90f13281d297..f43e0ffcaf56 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -18570,6 +18570,13 @@ S:   Supported
> >  F:   drivers/iio/light/rohm-bu27008.c
> >  F:   drivers/iio/light/rohm-bu27034.c
> >  
> > +ROHM BU64754 MOTOR DRIVER FOR CAMERA AUTOFOCUS
> > +M:   Kieran Bingham <kieran.bingham@ideasonboard.com>
> > +L:   linux-media@vger.kernel.org
> > +S:   Maintained
> > +T:   git git://linuxtv.org/media_tree.git
> 
> Do you have push rights there? The git entry is for entries where you
> maintain, which means: you accept and apply patches. This does not look
> like the case, so copying subsystem Git entry is pointless. Subsystem
> maintainer defines the git tree, not you.

I'll drop the line.

> 
> Best regards,
> Krzysztof
>

Re: [PATCH 1/2] Documentation: dt-bindings: media: i2c: Add ROHM BU64754 bindings

[Krzysztof Kozlowski]

On 17/09/2023 12:33, Kieran Bingham wrote:
> Quoting Krzysztof Kozlowski (2023-09-17 10:37:07)
>> On 15/09/2023 18:59, Kieran Bingham wrote:
>>> Add YAML device tree bindings for the ROHM BU64754 VCM Motor Driver for
>>> Camera Autofocus.
>>>
>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>> ---
>>
>> ...
>>
>>> +properties:
>>> +  compatible:
>>> +    items:
>>> +      - enum:
>>> +          - rohm,bu64754
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  vdd-supply:
>>
>> If the supply is not required, how the driver gets any power?
> 
> It may not be controllable. It could be wired to be always on.

That's different. You talk now about board, I am talking about device.
Is the supply required for the device? If yes, then it should be
required by the bindings.

> 
> The device has a low power mode which is controllable by software, so it
> could be always powered on and controlled via software only if desired.
> The driver handles that already as if the regulator isn't specified the
> power down register bits will be set to put it in low power mode.

Best regards,
Krzysztof

[PATCH 2/2] media: i2c: Add ROHM BU64754 Camera Autofocus Actuator

[Kieran Bingham]

Add support for the ROHM BU64754 Motor Driver for Camera Autofocus. A
V4L2 Subdevice is registered and provides a single
V4L2_CID_FOCUS_ABSOLUTE control.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 MAINTAINERS                 |   1 +
 drivers/media/i2c/Kconfig   |  13 ++
 drivers/media/i2c/Makefile  |   1 +
 drivers/media/i2c/bu64754.c | 308 ++++++++++++++++++++++++++++++++++++
 4 files changed, 323 insertions(+)
 create mode 100644 drivers/media/i2c/bu64754.c

diff --git a/MAINTAINERS b/MAINTAINERS
index f43e0ffcaf56..fd244560c317 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18576,6 +18576,7 @@ L:	linux-media@vger.kernel.org
 S:	Maintained
 T:	git git://linuxtv.org/media_tree.git
 F:	Documentation/devicetree/bindings/media/i2c/rohm,bu64754.yaml
+F:	drivers/media/i2c/bu64754.c
 
 ROHM MULTIFUNCTION BD9571MWV-M PMIC DEVICE DRIVERS
 M:	Marek Vasut <marek.vasut+renesas@gmail.com>
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 74ff833ff48c..b7b8004816ed 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -641,6 +641,19 @@ config VIDEO_AK7375
 	  capability. This is designed for linear control of
 	  voice coil motors, controlled via I2C serial interface.
 
+config VIDEO_BU64754
+	tristate "BU64754 Motor Driver for Camera Autofocus"
+	depends on I2C && VIDEO_DEV
+	select MEDIA_CONTROLLER
+	select VIDEO_V4L2_SUBDEV_API
+	select V4L2_ASYNC
+	select V4L2_CCI_I2C
+	help
+	  This is a driver for the BU64754 Motor Driver for Camera
+	  Autofocus. The BU64754 is an actuator driver IC which can
+	  control the actuator position precisely using an internal
+	  Hall Sensor.
+
 config VIDEO_DW9714
 	tristate "DW9714 lens voice coil support"
 	depends on I2C && VIDEO_DEV
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 80b00d39b48f..e62aa0df7b1a 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_VIDEO_AR0521) += ar0521.o
 obj-$(CONFIG_VIDEO_BT819) += bt819.o
 obj-$(CONFIG_VIDEO_BT856) += bt856.o
 obj-$(CONFIG_VIDEO_BT866) += bt866.o
+obj-$(CONFIG_VIDEO_BU64754) += bu64754.o
 obj-$(CONFIG_VIDEO_CCS) += ccs/
 obj-$(CONFIG_VIDEO_CCS_PLL) += ccs-pll.o
 obj-$(CONFIG_VIDEO_CS3308) += cs3308.o
diff --git a/drivers/media/i2c/bu64754.c b/drivers/media/i2c/bu64754.c
new file mode 100644
index 000000000000..3367b6f17660
--- /dev/null
+++ b/drivers/media/i2c/bu64754.c
@@ -0,0 +1,308 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * The BU64754 is an actuator driver IC which can control the
+ * actuator position precisely using an internal Hall Sensor.
+ */
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
+
+#include <media/v4l2-cci.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-device.h>
+
+#define BU64754_REG_ACTIVE	CCI_REG16(0x07)
+#define BU64754_ACTIVE_MODE	0x8080
+
+#define BU64754_REG_SERVE	CCI_REG16(0xd9)
+#define BU64754_SERVE_ON	0x0404
+
+#define BU64754_REG_POSITION	CCI_REG16(0x45)
+#define BU64753_POSITION_MAX	1023 /* 0x3ff */
+
+#define BU64754_POWER_ON_DELAY	800 /* uS : t1, t3 */
+
+struct bu64754 {
+	struct device *dev;
+
+	struct v4l2_ctrl_handler ctrls_vcm;
+	struct v4l2_subdev sd;
+	struct regmap *cci;
+
+	u16 current_val;
+	struct regulator *vdd;
+	struct notifier_block notifier;
+};
+
+static inline struct bu64754 *sd_to_bu64754(struct v4l2_subdev *subdev)
+{
+	return container_of(subdev, struct bu64754, sd);
+}
+
+static int bu64754_set(struct bu64754 *bu64754, u16 position)
+{
+	int ret;
+
+	ret = cci_write(bu64754->cci, BU64754_REG_POSITION, position, NULL);
+	if (ret) {
+		dev_err(bu64754->dev, "Set position failed ret=%d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int bu64754_active(struct bu64754 *bu64754)
+{
+	int ret;
+
+	/* Power on */
+	ret = cci_write(bu64754->cci, BU64754_REG_ACTIVE, BU64754_ACTIVE_MODE,
+			NULL);
+	if (ret < 0) {
+		dev_err(bu64754->dev, "Failed to set active mode ret = %d\n",
+			ret);
+		return ret;
+	}
+
+	/* Serve on */
+	ret = cci_write(bu64754->cci, BU64754_REG_SERVE, BU64754_SERVE_ON,
+			NULL);
+	if (ret < 0) {
+		dev_err(bu64754->dev, "Failed to enable serve ret = %d\n",
+			ret);
+		return ret;
+	}
+
+	return bu64754_set(bu64754, bu64754->current_val);
+}
+
+static int bu64754_standby(struct bu64754 *bu64754)
+{
+	int ret;
+
+	cci_write(bu64754->cci, BU64754_REG_ACTIVE, 0, &ret);
+	if (ret < 0)
+		dev_err(bu64754->dev, "Failed to set active mode ret = %d\n",
+			ret);
+
+	return ret;
+}
+
+static int bu64754_regulator_event(struct notifier_block *nb,
+				   unsigned long action, void *data)
+{
+	struct bu64754 *bu64754 = container_of(nb, struct bu64754, notifier);
+
+	if (action & REGULATOR_EVENT_ENABLE) {
+		/*
+		 * Initialisation delay between VDD low->high and availability
+		 * i2c operation.
+		 */
+		usleep_range(BU64754_POWER_ON_DELAY,
+			     BU64754_POWER_ON_DELAY + 100);
+
+		bu64754_active(bu64754);
+	} else if (action & REGULATOR_EVENT_PRE_DISABLE) {
+		bu64754_standby(bu64754);
+	}
+
+	return 0;
+}
+
+static int bu64754_set_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct bu64754 *bu64754 = container_of(ctrl->handler,
+		struct bu64754, ctrls_vcm);
+
+	if (ctrl->id == V4L2_CID_FOCUS_ABSOLUTE) {
+		bu64754->current_val = ctrl->val;
+		return bu64754_set(bu64754, ctrl->val);
+	}
+
+	return -EINVAL;
+}
+
+static const struct v4l2_ctrl_ops bu64754_vcm_ctrl_ops = {
+	.s_ctrl = bu64754_set_ctrl,
+};
+
+static int bu64754_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+	return pm_runtime_resume_and_get(sd->dev);
+}
+
+static int bu64754_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+	pm_runtime_put(sd->dev);
+	return 0;
+}
+
+static const struct v4l2_subdev_internal_ops bu64754_int_ops = {
+	.open = bu64754_open,
+	.close = bu64754_close,
+};
+
+static const struct v4l2_subdev_ops bu64754_ops = { };
+
+static void bu64754_subdev_cleanup(struct bu64754 *bu64754)
+{
+	v4l2_async_unregister_subdev(&bu64754->sd);
+	v4l2_ctrl_handler_free(&bu64754->ctrls_vcm);
+	media_entity_cleanup(&bu64754->sd.entity);
+}
+
+static int bu64754_init_controls(struct bu64754 *bu64754)
+{
+	struct v4l2_ctrl_handler *hdl = &bu64754->ctrls_vcm;
+	const struct v4l2_ctrl_ops *ops = &bu64754_vcm_ctrl_ops;
+
+	v4l2_ctrl_handler_init(hdl, 1);
+
+	v4l2_ctrl_new_std(hdl, ops, V4L2_CID_FOCUS_ABSOLUTE,
+			  0, BU64753_POSITION_MAX, 1, 0);
+
+	bu64754->current_val = 0;
+
+	bu64754->sd.ctrl_handler = hdl;
+	if (hdl->error) {
+		dev_err(bu64754->dev, "%s fail error: 0x%x\n",
+			__func__, hdl->error);
+		return hdl->error;
+	}
+
+	return 0;
+}
+
+static int bu64754_probe(struct i2c_client *client)
+{
+	struct bu64754 *bu64754;
+	int ret;
+
+	bu64754 = devm_kzalloc(&client->dev, sizeof(*bu64754), GFP_KERNEL);
+	if (!bu64754)
+		return -ENOMEM;
+
+	bu64754->dev = &client->dev;
+
+	bu64754->cci = devm_cci_regmap_init_i2c(client, 8);
+	if (IS_ERR(bu64754->cci)) {
+		dev_err(bu64754->dev, "Failed to initialize CCI\n");
+		return PTR_ERR(bu64754->cci);
+	}
+
+	bu64754->vdd = devm_regulator_get_optional(&client->dev, "vdd");
+	if (IS_ERR(bu64754->vdd)) {
+		if (PTR_ERR(bu64754->vdd) != -ENODEV)
+			return PTR_ERR(bu64754->vdd);
+
+		bu64754->vdd = NULL;
+	} else {
+		bu64754->notifier.notifier_call = bu64754_regulator_event;
+
+		ret = regulator_register_notifier(bu64754->vdd,
+						  &bu64754->notifier);
+		if (ret) {
+			dev_err(bu64754->dev,
+				"could not register regulator notifier\n");
+			return ret;
+		}
+	}
+
+	v4l2_i2c_subdev_init(&bu64754->sd, client, &bu64754_ops);
+	bu64754->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	bu64754->sd.internal_ops = &bu64754_int_ops;
+	bu64754->sd.entity.function = MEDIA_ENT_F_LENS;
+
+	ret = bu64754_init_controls(bu64754);
+	if (ret)
+		goto err_cleanup;
+
+	ret = media_entity_pads_init(&bu64754->sd.entity, 0, NULL);
+	if (ret < 0)
+		goto err_cleanup;
+
+	ret = v4l2_async_register_subdev(&bu64754->sd);
+	if (ret < 0)
+		goto err_cleanup;
+
+	if (!bu64754->vdd)
+		pm_runtime_set_active(&client->dev);
+
+	pm_runtime_enable(&client->dev);
+	pm_runtime_idle(&client->dev);
+
+	return 0;
+
+err_cleanup:
+	v4l2_ctrl_handler_free(&bu64754->ctrls_vcm);
+	media_entity_cleanup(&bu64754->sd.entity);
+
+	return ret;
+}
+
+static void bu64754_remove(struct i2c_client *client)
+{
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct bu64754 *bu64754 = sd_to_bu64754(sd);
+
+	if (bu64754->vdd)
+		regulator_unregister_notifier(bu64754->vdd,
+					      &bu64754->notifier);
+
+	pm_runtime_disable(&client->dev);
+
+	bu64754_subdev_cleanup(bu64754);
+}
+
+static int __maybe_unused bu64754_vcm_suspend(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct bu64754 *bu64754 = sd_to_bu64754(sd);
+
+	if (bu64754->vdd)
+		return regulator_disable(bu64754->vdd);
+
+	return bu64754_standby(bu64754);
+}
+
+static int  __maybe_unused bu64754_vcm_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct bu64754 *bu64754 = sd_to_bu64754(sd);
+
+	if (bu64754->vdd)
+		return regulator_enable(bu64754->vdd);
+
+	return bu64754_active(bu64754);
+}
+
+static const struct of_device_id bu64754_of_table[] = {
+	{ .compatible = "rohm,bu64754", },
+	{ /* sentinel */ }
+};
+
+MODULE_DEVICE_TABLE(of, bu64754_of_table);
+
+static SIMPLE_DEV_PM_OPS(bu64754_pm, bu64754_vcm_suspend, bu64754_vcm_resume);
+
+static struct i2c_driver bu64754_i2c_driver = {
+	.driver = {
+		.name = "bu64754",
+		.pm = &bu64754_pm,
+		.of_match_table = bu64754_of_table,
+	},
+	.probe = bu64754_probe,
+	.remove = bu64754_remove,
+};
+
+module_i2c_driver(bu64754_i2c_driver);
+
+MODULE_AUTHOR("Kieran Bingham <kieran.bingham@ideasonboard.com>");
+MODULE_DESCRIPTION("ROHM BU64754 VCM driver");
+MODULE_LICENSE("GPL");
-- 
2.34.1

Re: [PATCH 2/2] media: i2c: Add ROHM BU64754 Camera Autofocus Actuator

[Dave Stevenson]

Hi Kieran

On Fri, 15 Sept 2023 at 18:02, Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Add support for the ROHM BU64754 Motor Driver for Camera Autofocus. A
> V4L2 Subdevice is registered and provides a single
> V4L2_CID_FOCUS_ABSOLUTE control.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  MAINTAINERS                 |   1 +
>  drivers/media/i2c/Kconfig   |  13 ++
>  drivers/media/i2c/Makefile  |   1 +
>  drivers/media/i2c/bu64754.c | 308 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 323 insertions(+)
>  create mode 100644 drivers/media/i2c/bu64754.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f43e0ffcaf56..fd244560c317 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18576,6 +18576,7 @@ L:      linux-media@vger.kernel.org
>  S:     Maintained
>  T:     git git://linuxtv.org/media_tree.git
>  F:     Documentation/devicetree/bindings/media/i2c/rohm,bu64754.yaml
> +F:     drivers/media/i2c/bu64754.c
>
>  ROHM MULTIFUNCTION BD9571MWV-M PMIC DEVICE DRIVERS
>  M:     Marek Vasut <marek.vasut+renesas@gmail.com>
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 74ff833ff48c..b7b8004816ed 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -641,6 +641,19 @@ config VIDEO_AK7375
>           capability. This is designed for linear control of
>           voice coil motors, controlled via I2C serial interface.
>
> +config VIDEO_BU64754
> +       tristate "BU64754 Motor Driver for Camera Autofocus"
> +       depends on I2C && VIDEO_DEV
> +       select MEDIA_CONTROLLER
> +       select VIDEO_V4L2_SUBDEV_API
> +       select V4L2_ASYNC
> +       select V4L2_CCI_I2C
> +       help
> +         This is a driver for the BU64754 Motor Driver for Camera
> +         Autofocus. The BU64754 is an actuator driver IC which can
> +         control the actuator position precisely using an internal
> +         Hall Sensor.

I can't find any data on this driver.
Is it still expecting a VCM and hence near instantaneous movement? I
was noting your comment on the hall sensor and thinking you
potentially needed to be able to report whether the target position
had been reached or not. Michael's series at [1] was trying to address
that.

[1] https://patchwork.linuxtv.org/project/linux-media/cover/20230406-feature-controls-lens-v2-0-faa8ad2bc404@wolfvision.net/

> +
>  config VIDEO_DW9714
>         tristate "DW9714 lens voice coil support"
>         depends on I2C && VIDEO_DEV
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index 80b00d39b48f..e62aa0df7b1a 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -22,6 +22,7 @@ obj-$(CONFIG_VIDEO_AR0521) += ar0521.o
>  obj-$(CONFIG_VIDEO_BT819) += bt819.o
>  obj-$(CONFIG_VIDEO_BT856) += bt856.o
>  obj-$(CONFIG_VIDEO_BT866) += bt866.o
> +obj-$(CONFIG_VIDEO_BU64754) += bu64754.o
>  obj-$(CONFIG_VIDEO_CCS) += ccs/
>  obj-$(CONFIG_VIDEO_CCS_PLL) += ccs-pll.o
>  obj-$(CONFIG_VIDEO_CS3308) += cs3308.o
> diff --git a/drivers/media/i2c/bu64754.c b/drivers/media/i2c/bu64754.c
> new file mode 100644
> index 000000000000..3367b6f17660
> --- /dev/null
> +++ b/drivers/media/i2c/bu64754.c
> @@ -0,0 +1,308 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * The BU64754 is an actuator driver IC which can control the
> + * actuator position precisely using an internal Hall Sensor.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <media/v4l2-cci.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-device.h>
> +
> +#define BU64754_REG_ACTIVE     CCI_REG16(0x07)
> +#define BU64754_ACTIVE_MODE    0x8080
> +
> +#define BU64754_REG_SERVE      CCI_REG16(0xd9)
> +#define BU64754_SERVE_ON       0x0404
> +
> +#define BU64754_REG_POSITION   CCI_REG16(0x45)
> +#define BU64753_POSITION_MAX   1023 /* 0x3ff */
> +
> +#define BU64754_POWER_ON_DELAY 800 /* uS : t1, t3 */
> +
> +struct bu64754 {
> +       struct device *dev;
> +
> +       struct v4l2_ctrl_handler ctrls_vcm;
> +       struct v4l2_subdev sd;
> +       struct regmap *cci;
> +
> +       u16 current_val;
> +       struct regulator *vdd;
> +       struct notifier_block notifier;
> +};
> +
> +static inline struct bu64754 *sd_to_bu64754(struct v4l2_subdev *subdev)
> +{
> +       return container_of(subdev, struct bu64754, sd);
> +}
> +
> +static int bu64754_set(struct bu64754 *bu64754, u16 position)
> +{
> +       int ret;
> +
> +       ret = cci_write(bu64754->cci, BU64754_REG_POSITION, position, NULL);
> +       if (ret) {
> +               dev_err(bu64754->dev, "Set position failed ret=%d\n", ret);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static int bu64754_active(struct bu64754 *bu64754)
> +{
> +       int ret;
> +
> +       /* Power on */
> +       ret = cci_write(bu64754->cci, BU64754_REG_ACTIVE, BU64754_ACTIVE_MODE,
> +                       NULL);
> +       if (ret < 0) {
> +               dev_err(bu64754->dev, "Failed to set active mode ret = %d\n",
> +                       ret);
> +               return ret;
> +       }
> +
> +       /* Serve on */
> +       ret = cci_write(bu64754->cci, BU64754_REG_SERVE, BU64754_SERVE_ON,
> +                       NULL);
> +       if (ret < 0) {
> +               dev_err(bu64754->dev, "Failed to enable serve ret = %d\n",
> +                       ret);
> +               return ret;
> +       }
> +
> +       return bu64754_set(bu64754, bu64754->current_val);
> +}
> +
> +static int bu64754_standby(struct bu64754 *bu64754)
> +{
> +       int ret;
> +
> +       cci_write(bu64754->cci, BU64754_REG_ACTIVE, 0, &ret);
> +       if (ret < 0)
> +               dev_err(bu64754->dev, "Failed to set active mode ret = %d\n",
> +                       ret);
> +
> +       return ret;
> +}
> +
> +static int bu64754_regulator_event(struct notifier_block *nb,
> +                                  unsigned long action, void *data)
> +{
> +       struct bu64754 *bu64754 = container_of(nb, struct bu64754, notifier);
> +
> +       if (action & REGULATOR_EVENT_ENABLE) {
> +               /*
> +                * Initialisation delay between VDD low->high and availability
> +                * i2c operation.
> +                */
> +               usleep_range(BU64754_POWER_ON_DELAY,
> +                            BU64754_POWER_ON_DELAY + 100);
> +
> +               bu64754_active(bu64754);
> +       } else if (action & REGULATOR_EVENT_PRE_DISABLE) {
> +               bu64754_standby(bu64754);
> +       }

Presumably this is based on the assumption that the same regulator
controls sensor and lens, so when the sensor is powered up the lens
position gets restored.
I'm sure when I suggested doing the same previously it was shot down
in flames ... found it [2]

Personally I think it makes sense that the lens powers up
automagically, and have almost exactly the same code as this in a
couple of our VCM drivers, but others disagree.

  Dave

[2] https://lore.kernel.org/all/CAPY8ntBZpZjecHNCMf-eMefcp2EgmbqkXMt4p=UeOe0n-o8WrA@mail.gmail.com/

> +
> +       return 0;
> +}
> +
> +static int bu64754_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +       struct bu64754 *bu64754 = container_of(ctrl->handler,
> +               struct bu64754, ctrls_vcm);
> +
> +       if (ctrl->id == V4L2_CID_FOCUS_ABSOLUTE) {
> +               bu64754->current_val = ctrl->val;
> +               return bu64754_set(bu64754, ctrl->val);
> +       }
> +
> +       return -EINVAL;
> +}
> +
> +static const struct v4l2_ctrl_ops bu64754_vcm_ctrl_ops = {
> +       .s_ctrl = bu64754_set_ctrl,
> +};
> +
> +static int bu64754_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> +{
> +       return pm_runtime_resume_and_get(sd->dev);
> +}
> +
> +static int bu64754_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> +{
> +       pm_runtime_put(sd->dev);
> +       return 0;
> +}
> +
> +static const struct v4l2_subdev_internal_ops bu64754_int_ops = {
> +       .open = bu64754_open,
> +       .close = bu64754_close,
> +};
> +
> +static const struct v4l2_subdev_ops bu64754_ops = { };
> +
> +static void bu64754_subdev_cleanup(struct bu64754 *bu64754)
> +{
> +       v4l2_async_unregister_subdev(&bu64754->sd);
> +       v4l2_ctrl_handler_free(&bu64754->ctrls_vcm);
> +       media_entity_cleanup(&bu64754->sd.entity);
> +}
> +
> +static int bu64754_init_controls(struct bu64754 *bu64754)
> +{
> +       struct v4l2_ctrl_handler *hdl = &bu64754->ctrls_vcm;
> +       const struct v4l2_ctrl_ops *ops = &bu64754_vcm_ctrl_ops;
> +
> +       v4l2_ctrl_handler_init(hdl, 1);
> +
> +       v4l2_ctrl_new_std(hdl, ops, V4L2_CID_FOCUS_ABSOLUTE,
> +                         0, BU64753_POSITION_MAX, 1, 0);
> +
> +       bu64754->current_val = 0;
> +
> +       bu64754->sd.ctrl_handler = hdl;
> +       if (hdl->error) {
> +               dev_err(bu64754->dev, "%s fail error: 0x%x\n",
> +                       __func__, hdl->error);
> +               return hdl->error;
> +       }
> +
> +       return 0;
> +}
> +
> +static int bu64754_probe(struct i2c_client *client)
> +{
> +       struct bu64754 *bu64754;
> +       int ret;
> +
> +       bu64754 = devm_kzalloc(&client->dev, sizeof(*bu64754), GFP_KERNEL);
> +       if (!bu64754)
> +               return -ENOMEM;
> +
> +       bu64754->dev = &client->dev;
> +
> +       bu64754->cci = devm_cci_regmap_init_i2c(client, 8);
> +       if (IS_ERR(bu64754->cci)) {
> +               dev_err(bu64754->dev, "Failed to initialize CCI\n");
> +               return PTR_ERR(bu64754->cci);
> +       }
> +
> +       bu64754->vdd = devm_regulator_get_optional(&client->dev, "vdd");
> +       if (IS_ERR(bu64754->vdd)) {
> +               if (PTR_ERR(bu64754->vdd) != -ENODEV)
> +                       return PTR_ERR(bu64754->vdd);
> +
> +               bu64754->vdd = NULL;
> +       } else {
> +               bu64754->notifier.notifier_call = bu64754_regulator_event;
> +
> +               ret = regulator_register_notifier(bu64754->vdd,
> +                                                 &bu64754->notifier);
> +               if (ret) {
> +                       dev_err(bu64754->dev,
> +                               "could not register regulator notifier\n");
> +                       return ret;
> +               }
> +       }
> +
> +       v4l2_i2c_subdev_init(&bu64754->sd, client, &bu64754_ops);
> +       bu64754->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +       bu64754->sd.internal_ops = &bu64754_int_ops;
> +       bu64754->sd.entity.function = MEDIA_ENT_F_LENS;
> +
> +       ret = bu64754_init_controls(bu64754);
> +       if (ret)
> +               goto err_cleanup;
> +
> +       ret = media_entity_pads_init(&bu64754->sd.entity, 0, NULL);
> +       if (ret < 0)
> +               goto err_cleanup;
> +
> +       ret = v4l2_async_register_subdev(&bu64754->sd);
> +       if (ret < 0)
> +               goto err_cleanup;
> +
> +       if (!bu64754->vdd)
> +               pm_runtime_set_active(&client->dev);
> +
> +       pm_runtime_enable(&client->dev);
> +       pm_runtime_idle(&client->dev);
> +
> +       return 0;
> +
> +err_cleanup:
> +       v4l2_ctrl_handler_free(&bu64754->ctrls_vcm);
> +       media_entity_cleanup(&bu64754->sd.entity);
> +
> +       return ret;
> +}
> +
> +static void bu64754_remove(struct i2c_client *client)
> +{
> +       struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +       struct bu64754 *bu64754 = sd_to_bu64754(sd);
> +
> +       if (bu64754->vdd)
> +               regulator_unregister_notifier(bu64754->vdd,
> +                                             &bu64754->notifier);
> +
> +       pm_runtime_disable(&client->dev);
> +
> +       bu64754_subdev_cleanup(bu64754);
> +}
> +
> +static int __maybe_unused bu64754_vcm_suspend(struct device *dev)
> +{
> +       struct i2c_client *client = to_i2c_client(dev);
> +       struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +       struct bu64754 *bu64754 = sd_to_bu64754(sd);
> +
> +       if (bu64754->vdd)
> +               return regulator_disable(bu64754->vdd);
> +
> +       return bu64754_standby(bu64754);
> +}
> +
> +static int  __maybe_unused bu64754_vcm_resume(struct device *dev)
> +{
> +       struct i2c_client *client = to_i2c_client(dev);
> +       struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +       struct bu64754 *bu64754 = sd_to_bu64754(sd);
> +
> +       if (bu64754->vdd)
> +               return regulator_enable(bu64754->vdd);
> +
> +       return bu64754_active(bu64754);
> +}
> +
> +static const struct of_device_id bu64754_of_table[] = {
> +       { .compatible = "rohm,bu64754", },
> +       { /* sentinel */ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, bu64754_of_table);
> +
> +static SIMPLE_DEV_PM_OPS(bu64754_pm, bu64754_vcm_suspend, bu64754_vcm_resume);
> +
> +static struct i2c_driver bu64754_i2c_driver = {
> +       .driver = {
> +               .name = "bu64754",
> +               .pm = &bu64754_pm,
> +               .of_match_table = bu64754_of_table,
> +       },
> +       .probe = bu64754_probe,
> +       .remove = bu64754_remove,
> +};
> +
> +module_i2c_driver(bu64754_i2c_driver);
> +
> +MODULE_AUTHOR("Kieran Bingham <kieran.bingham@ideasonboard.com>");
> +MODULE_DESCRIPTION("ROHM BU64754 VCM driver");
> +MODULE_LICENSE("GPL");
> --
> 2.34.1
>

Re: [PATCH 2/2] media: i2c: Add ROHM BU64754 Camera Autofocus Actuator

[Kieran Bingham]

Hi Dave,

Quoting Dave Stevenson (2023-09-15 18:44:02)
> Hi Kieran
> 
> On Fri, 15 Sept 2023 at 18:02, Kieran Bingham
> <kieran.bingham@ideasonboard.com> wrote:
> >
> > Add support for the ROHM BU64754 Motor Driver for Camera Autofocus. A
> > V4L2 Subdevice is registered and provides a single
> > V4L2_CID_FOCUS_ABSOLUTE control.
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  MAINTAINERS                 |   1 +
> >  drivers/media/i2c/Kconfig   |  13 ++
> >  drivers/media/i2c/Makefile  |   1 +
> >  drivers/media/i2c/bu64754.c | 308 ++++++++++++++++++++++++++++++++++++
> >  4 files changed, 323 insertions(+)
> >  create mode 100644 drivers/media/i2c/bu64754.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index f43e0ffcaf56..fd244560c317 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -18576,6 +18576,7 @@ L:      linux-media@vger.kernel.org
> >  S:     Maintained
> >  T:     git git://linuxtv.org/media_tree.git
> >  F:     Documentation/devicetree/bindings/media/i2c/rohm,bu64754.yaml
> > +F:     drivers/media/i2c/bu64754.c
> >
> >  ROHM MULTIFUNCTION BD9571MWV-M PMIC DEVICE DRIVERS
> >  M:     Marek Vasut <marek.vasut+renesas@gmail.com>
> > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > index 74ff833ff48c..b7b8004816ed 100644
> > --- a/drivers/media/i2c/Kconfig
> > +++ b/drivers/media/i2c/Kconfig
> > @@ -641,6 +641,19 @@ config VIDEO_AK7375
> >           capability. This is designed for linear control of
> >           voice coil motors, controlled via I2C serial interface.
> >
> > +config VIDEO_BU64754
> > +       tristate "BU64754 Motor Driver for Camera Autofocus"
> > +       depends on I2C && VIDEO_DEV
> > +       select MEDIA_CONTROLLER
> > +       select VIDEO_V4L2_SUBDEV_API
> > +       select V4L2_ASYNC
> > +       select V4L2_CCI_I2C
> > +       help
> > +         This is a driver for the BU64754 Motor Driver for Camera
> > +         Autofocus. The BU64754 is an actuator driver IC which can
> > +         control the actuator position precisely using an internal
> > +         Hall Sensor.
> 
> I can't find any data on this driver.
> Is it still expecting a VCM and hence near instantaneous movement? I
> was noting your comment on the hall sensor and thinking you
> potentially needed to be able to report whether the target position
> had been reached or not. Michael's series at [1] was trying to address
> that.

The hall sensor is internal. The component description is just a
slightly fixed grammar of the product brief.

As I understand it, this is a VCM yes but I have so little visibility.

> 
> [1] https://patchwork.linuxtv.org/project/linux-media/cover/20230406-feature-controls-lens-v2-0-faa8ad2bc404@wolfvision.net/
> 
> > +
> >  config VIDEO_DW9714
> >         tristate "DW9714 lens voice coil support"
> >         depends on I2C && VIDEO_DEV
> > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> > index 80b00d39b48f..e62aa0df7b1a 100644
> > --- a/drivers/media/i2c/Makefile
> > +++ b/drivers/media/i2c/Makefile
> > @@ -22,6 +22,7 @@ obj-$(CONFIG_VIDEO_AR0521) += ar0521.o
> >  obj-$(CONFIG_VIDEO_BT819) += bt819.o
> >  obj-$(CONFIG_VIDEO_BT856) += bt856.o
> >  obj-$(CONFIG_VIDEO_BT866) += bt866.o
> > +obj-$(CONFIG_VIDEO_BU64754) += bu64754.o
> >  obj-$(CONFIG_VIDEO_CCS) += ccs/
> >  obj-$(CONFIG_VIDEO_CCS_PLL) += ccs-pll.o
> >  obj-$(CONFIG_VIDEO_CS3308) += cs3308.o
> > diff --git a/drivers/media/i2c/bu64754.c b/drivers/media/i2c/bu64754.c
> > new file mode 100644
> > index 000000000000..3367b6f17660
> > --- /dev/null
> > +++ b/drivers/media/i2c/bu64754.c
> > @@ -0,0 +1,308 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * The BU64754 is an actuator driver IC which can control the
> > + * actuator position precisely using an internal Hall Sensor.
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/i2c.h>
> > +#include <linux/module.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/regulator/consumer.h>
> > +
> > +#include <media/v4l2-cci.h>
> > +#include <media/v4l2-ctrls.h>
> > +#include <media/v4l2-device.h>
> > +
> > +#define BU64754_REG_ACTIVE     CCI_REG16(0x07)
> > +#define BU64754_ACTIVE_MODE    0x8080
> > +
> > +#define BU64754_REG_SERVE      CCI_REG16(0xd9)
> > +#define BU64754_SERVE_ON       0x0404
> > +
> > +#define BU64754_REG_POSITION   CCI_REG16(0x45)
> > +#define BU64753_POSITION_MAX   1023 /* 0x3ff */
> > +
> > +#define BU64754_POWER_ON_DELAY 800 /* uS : t1, t3 */
> > +
> > +struct bu64754 {
> > +       struct device *dev;
> > +
> > +       struct v4l2_ctrl_handler ctrls_vcm;
> > +       struct v4l2_subdev sd;
> > +       struct regmap *cci;
> > +
> > +       u16 current_val;
> > +       struct regulator *vdd;
> > +       struct notifier_block notifier;
> > +};
> > +
> > +static inline struct bu64754 *sd_to_bu64754(struct v4l2_subdev *subdev)
> > +{
> > +       return container_of(subdev, struct bu64754, sd);
> > +}
> > +
> > +static int bu64754_set(struct bu64754 *bu64754, u16 position)
> > +{
> > +       int ret;
> > +
> > +       ret = cci_write(bu64754->cci, BU64754_REG_POSITION, position, NULL);
> > +       if (ret) {
> > +               dev_err(bu64754->dev, "Set position failed ret=%d\n", ret);
> > +               return ret;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int bu64754_active(struct bu64754 *bu64754)
> > +{
> > +       int ret;
> > +
> > +       /* Power on */
> > +       ret = cci_write(bu64754->cci, BU64754_REG_ACTIVE, BU64754_ACTIVE_MODE,
> > +                       NULL);
> > +       if (ret < 0) {
> > +               dev_err(bu64754->dev, "Failed to set active mode ret = %d\n",
> > +                       ret);
> > +               return ret;
> > +       }
> > +
> > +       /* Serve on */
> > +       ret = cci_write(bu64754->cci, BU64754_REG_SERVE, BU64754_SERVE_ON,
> > +                       NULL);
> > +       if (ret < 0) {
> > +               dev_err(bu64754->dev, "Failed to enable serve ret = %d\n",
> > +                       ret);
> > +               return ret;
> > +       }
> > +
> > +       return bu64754_set(bu64754, bu64754->current_val);
> > +}
> > +
> > +static int bu64754_standby(struct bu64754 *bu64754)
> > +{
> > +       int ret;
> > +
> > +       cci_write(bu64754->cci, BU64754_REG_ACTIVE, 0, &ret);
> > +       if (ret < 0)
> > +               dev_err(bu64754->dev, "Failed to set active mode ret = %d\n",
> > +                       ret);
> > +
> > +       return ret;
> > +}
> > +
> > +static int bu64754_regulator_event(struct notifier_block *nb,
> > +                                  unsigned long action, void *data)
> > +{
> > +       struct bu64754 *bu64754 = container_of(nb, struct bu64754, notifier);
> > +
> > +       if (action & REGULATOR_EVENT_ENABLE) {
> > +               /*
> > +                * Initialisation delay between VDD low->high and availability
> > +                * i2c operation.
> > +                */
> > +               usleep_range(BU64754_POWER_ON_DELAY,
> > +                            BU64754_POWER_ON_DELAY + 100);
> > +
> > +               bu64754_active(bu64754);
> > +       } else if (action & REGULATOR_EVENT_PRE_DISABLE) {
> > +               bu64754_standby(bu64754);
> > +       }
> 
> Presumably this is based on the assumption that the same regulator
> controls sensor and lens, so when the sensor is powered up the lens
> position gets restored.
> I'm sure when I suggested doing the same previously it was shot down
> in flames ... found it [2]
> 
> Personally I think it makes sense that the lens powers up
> automagically, and have almost exactly the same code as this in a
> couple of our VCM drivers, but others disagree.

I've tested this on the Raspberry Pi, and I expect this design model
comes from you originally then. I thought it was a good way to handle
things, but I wasn't aware that others had already disagreed.

Being a module that will connect over the RPi interface, with a single
POWER_EN, indeed - the regulators are shared with the camera, and
synchronising how it is handled seemed reasonable. I guess I have some
more reading to do now then.

I'm already thinking about making a 'simple-vcm' module or parent-driver
that can reduce the boilerplate for the vcms though. There's very little
difference between them all ... and I think they could be abstracted out
substantially to simplify adding new devices.

--
Kieran



> 
>   Dave
> 
> [2] https://lore.kernel.org/all/CAPY8ntBZpZjecHNCMf-eMefcp2EgmbqkXMt4p=UeOe0n-o8WrA@mail.gmail.com/
> 
> > +
> > +       return 0;
> > +}
> > +
> > +static int bu64754_set_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > +       struct bu64754 *bu64754 = container_of(ctrl->handler,
> > +               struct bu64754, ctrls_vcm);
> > +
> > +       if (ctrl->id == V4L2_CID_FOCUS_ABSOLUTE) {
> > +               bu64754->current_val = ctrl->val;
> > +               return bu64754_set(bu64754, ctrl->val);
> > +       }
> > +
> > +       return -EINVAL;
> > +}
> > +
> > +static const struct v4l2_ctrl_ops bu64754_vcm_ctrl_ops = {
> > +       .s_ctrl = bu64754_set_ctrl,
> > +};
> > +
> > +static int bu64754_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> > +{
> > +       return pm_runtime_resume_and_get(sd->dev);
> > +}
> > +
> > +static int bu64754_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> > +{
> > +       pm_runtime_put(sd->dev);
> > +       return 0;
> > +}
> > +
> > +static const struct v4l2_subdev_internal_ops bu64754_int_ops = {
> > +       .open = bu64754_open,
> > +       .close = bu64754_close,
> > +};
> > +
> > +static const struct v4l2_subdev_ops bu64754_ops = { };
> > +
> > +static void bu64754_subdev_cleanup(struct bu64754 *bu64754)
> > +{
> > +       v4l2_async_unregister_subdev(&bu64754->sd);
> > +       v4l2_ctrl_handler_free(&bu64754->ctrls_vcm);
> > +       media_entity_cleanup(&bu64754->sd.entity);
> > +}
> > +
> > +static int bu64754_init_controls(struct bu64754 *bu64754)
> > +{
> > +       struct v4l2_ctrl_handler *hdl = &bu64754->ctrls_vcm;
> > +       const struct v4l2_ctrl_ops *ops = &bu64754_vcm_ctrl_ops;
> > +
> > +       v4l2_ctrl_handler_init(hdl, 1);
> > +
> > +       v4l2_ctrl_new_std(hdl, ops, V4L2_CID_FOCUS_ABSOLUTE,
> > +                         0, BU64753_POSITION_MAX, 1, 0);
> > +
> > +       bu64754->current_val = 0;
> > +
> > +       bu64754->sd.ctrl_handler = hdl;
> > +       if (hdl->error) {
> > +               dev_err(bu64754->dev, "%s fail error: 0x%x\n",
> > +                       __func__, hdl->error);
> > +               return hdl->error;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int bu64754_probe(struct i2c_client *client)
> > +{
> > +       struct bu64754 *bu64754;
> > +       int ret;
> > +
> > +       bu64754 = devm_kzalloc(&client->dev, sizeof(*bu64754), GFP_KERNEL);
> > +       if (!bu64754)
> > +               return -ENOMEM;
> > +
> > +       bu64754->dev = &client->dev;
> > +
> > +       bu64754->cci = devm_cci_regmap_init_i2c(client, 8);
> > +       if (IS_ERR(bu64754->cci)) {
> > +               dev_err(bu64754->dev, "Failed to initialize CCI\n");
> > +               return PTR_ERR(bu64754->cci);
> > +       }
> > +
> > +       bu64754->vdd = devm_regulator_get_optional(&client->dev, "vdd");
> > +       if (IS_ERR(bu64754->vdd)) {
> > +               if (PTR_ERR(bu64754->vdd) != -ENODEV)
> > +                       return PTR_ERR(bu64754->vdd);
> > +
> > +               bu64754->vdd = NULL;
> > +       } else {
> > +               bu64754->notifier.notifier_call = bu64754_regulator_event;
> > +
> > +               ret = regulator_register_notifier(bu64754->vdd,
> > +                                                 &bu64754->notifier);
> > +               if (ret) {
> > +                       dev_err(bu64754->dev,
> > +                               "could not register regulator notifier\n");
> > +                       return ret;
> > +               }
> > +       }
> > +
> > +       v4l2_i2c_subdev_init(&bu64754->sd, client, &bu64754_ops);
> > +       bu64754->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > +       bu64754->sd.internal_ops = &bu64754_int_ops;
> > +       bu64754->sd.entity.function = MEDIA_ENT_F_LENS;
> > +
> > +       ret = bu64754_init_controls(bu64754);
> > +       if (ret)
> > +               goto err_cleanup;
> > +
> > +       ret = media_entity_pads_init(&bu64754->sd.entity, 0, NULL);
> > +       if (ret < 0)
> > +               goto err_cleanup;
> > +
> > +       ret = v4l2_async_register_subdev(&bu64754->sd);
> > +       if (ret < 0)
> > +               goto err_cleanup;
> > +
> > +       if (!bu64754->vdd)
> > +               pm_runtime_set_active(&client->dev);
> > +
> > +       pm_runtime_enable(&client->dev);
> > +       pm_runtime_idle(&client->dev);
> > +
> > +       return 0;
> > +
> > +err_cleanup:
> > +       v4l2_ctrl_handler_free(&bu64754->ctrls_vcm);
> > +       media_entity_cleanup(&bu64754->sd.entity);
> > +
> > +       return ret;
> > +}
> > +
> > +static void bu64754_remove(struct i2c_client *client)
> > +{
> > +       struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > +       struct bu64754 *bu64754 = sd_to_bu64754(sd);
> > +
> > +       if (bu64754->vdd)
> > +               regulator_unregister_notifier(bu64754->vdd,
> > +                                             &bu64754->notifier);
> > +
> > +       pm_runtime_disable(&client->dev);
> > +
> > +       bu64754_subdev_cleanup(bu64754);
> > +}
> > +
> > +static int __maybe_unused bu64754_vcm_suspend(struct device *dev)
> > +{
> > +       struct i2c_client *client = to_i2c_client(dev);
> > +       struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > +       struct bu64754 *bu64754 = sd_to_bu64754(sd);
> > +
> > +       if (bu64754->vdd)
> > +               return regulator_disable(bu64754->vdd);
> > +
> > +       return bu64754_standby(bu64754);
> > +}
> > +
> > +static int  __maybe_unused bu64754_vcm_resume(struct device *dev)
> > +{
> > +       struct i2c_client *client = to_i2c_client(dev);
> > +       struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > +       struct bu64754 *bu64754 = sd_to_bu64754(sd);
> > +
> > +       if (bu64754->vdd)
> > +               return regulator_enable(bu64754->vdd);
> > +
> > +       return bu64754_active(bu64754);
> > +}
> > +
> > +static const struct of_device_id bu64754_of_table[] = {
> > +       { .compatible = "rohm,bu64754", },
> > +       { /* sentinel */ }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, bu64754_of_table);
> > +
> > +static SIMPLE_DEV_PM_OPS(bu64754_pm, bu64754_vcm_suspend, bu64754_vcm_resume);
> > +
> > +static struct i2c_driver bu64754_i2c_driver = {
> > +       .driver = {
> > +               .name = "bu64754",
> > +               .pm = &bu64754_pm,
> > +               .of_match_table = bu64754_of_table,
> > +       },
> > +       .probe = bu64754_probe,
> > +       .remove = bu64754_remove,
> > +};
> > +
> > +module_i2c_driver(bu64754_i2c_driver);
> > +
> > +MODULE_AUTHOR("Kieran Bingham <kieran.bingham@ideasonboard.com>");
> > +MODULE_DESCRIPTION("ROHM BU64754 VCM driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.34.1
> >

Re: [PATCH 2/2] media: i2c: Add ROHM BU64754 Camera Autofocus Actuator

[Dave Stevenson]

Hi Kieran

On Fri, 15 Sept 2023 at 20:19, Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Hi Dave,
>
> Quoting Dave Stevenson (2023-09-15 18:44:02)
> > Hi Kieran
> >
> > On Fri, 15 Sept 2023 at 18:02, Kieran Bingham
> > <kieran.bingham@ideasonboard.com> wrote:
> > >
> > > Add support for the ROHM BU64754 Motor Driver for Camera Autofocus. A
> > > V4L2 Subdevice is registered and provides a single
> > > V4L2_CID_FOCUS_ABSOLUTE control.
> > >
> > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > ---
> > >  MAINTAINERS                 |   1 +
> > >  drivers/media/i2c/Kconfig   |  13 ++
> > >  drivers/media/i2c/Makefile  |   1 +
> > >  drivers/media/i2c/bu64754.c | 308 ++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 323 insertions(+)
> > >  create mode 100644 drivers/media/i2c/bu64754.c
> > >
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index f43e0ffcaf56..fd244560c317 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -18576,6 +18576,7 @@ L:      linux-media@vger.kernel.org
> > >  S:     Maintained
> > >  T:     git git://linuxtv.org/media_tree.git
> > >  F:     Documentation/devicetree/bindings/media/i2c/rohm,bu64754.yaml
> > > +F:     drivers/media/i2c/bu64754.c
> > >
> > >  ROHM MULTIFUNCTION BD9571MWV-M PMIC DEVICE DRIVERS
> > >  M:     Marek Vasut <marek.vasut+renesas@gmail.com>
> > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > > index 74ff833ff48c..b7b8004816ed 100644
> > > --- a/drivers/media/i2c/Kconfig
> > > +++ b/drivers/media/i2c/Kconfig
> > > @@ -641,6 +641,19 @@ config VIDEO_AK7375
> > >           capability. This is designed for linear control of
> > >           voice coil motors, controlled via I2C serial interface.
> > >
> > > +config VIDEO_BU64754
> > > +       tristate "BU64754 Motor Driver for Camera Autofocus"
> > > +       depends on I2C && VIDEO_DEV
> > > +       select MEDIA_CONTROLLER
> > > +       select VIDEO_V4L2_SUBDEV_API
> > > +       select V4L2_ASYNC
> > > +       select V4L2_CCI_I2C
> > > +       help
> > > +         This is a driver for the BU64754 Motor Driver for Camera
> > > +         Autofocus. The BU64754 is an actuator driver IC which can
> > > +         control the actuator position precisely using an internal
> > > +         Hall Sensor.
> >
> > I can't find any data on this driver.
> > Is it still expecting a VCM and hence near instantaneous movement? I
> > was noting your comment on the hall sensor and thinking you
> > potentially needed to be able to report whether the target position
> > had been reached or not. Michael's series at [1] was trying to address
> > that.
>
> The hall sensor is internal. The component description is just a
> slightly fixed grammar of the product brief.
>
> As I understand it, this is a VCM yes but I have so little visibility.

Fair enough, should be quick enough not to make too much difference then.

> >
> > [1] https://patchwork.linuxtv.org/project/linux-media/cover/20230406-feature-controls-lens-v2-0-faa8ad2bc404@wolfvision.net/
> >
> > > +
> > >  config VIDEO_DW9714
> > >         tristate "DW9714 lens voice coil support"
> > >         depends on I2C && VIDEO_DEV
> > > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> > > index 80b00d39b48f..e62aa0df7b1a 100644
> > > --- a/drivers/media/i2c/Makefile
> > > +++ b/drivers/media/i2c/Makefile
> > > @@ -22,6 +22,7 @@ obj-$(CONFIG_VIDEO_AR0521) += ar0521.o
> > >  obj-$(CONFIG_VIDEO_BT819) += bt819.o
> > >  obj-$(CONFIG_VIDEO_BT856) += bt856.o
> > >  obj-$(CONFIG_VIDEO_BT866) += bt866.o
> > > +obj-$(CONFIG_VIDEO_BU64754) += bu64754.o
> > >  obj-$(CONFIG_VIDEO_CCS) += ccs/
> > >  obj-$(CONFIG_VIDEO_CCS_PLL) += ccs-pll.o
> > >  obj-$(CONFIG_VIDEO_CS3308) += cs3308.o
> > > diff --git a/drivers/media/i2c/bu64754.c b/drivers/media/i2c/bu64754.c
> > > new file mode 100644
> > > index 000000000000..3367b6f17660
> > > --- /dev/null
> > > +++ b/drivers/media/i2c/bu64754.c
> > > @@ -0,0 +1,308 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * The BU64754 is an actuator driver IC which can control the
> > > + * actuator position precisely using an internal Hall Sensor.
> > > + */
> > > +
> > > +#include <linux/delay.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/module.h>
> > > +#include <linux/pm_runtime.h>
> > > +#include <linux/regulator/consumer.h>
> > > +
> > > +#include <media/v4l2-cci.h>
> > > +#include <media/v4l2-ctrls.h>
> > > +#include <media/v4l2-device.h>
> > > +
> > > +#define BU64754_REG_ACTIVE     CCI_REG16(0x07)
> > > +#define BU64754_ACTIVE_MODE    0x8080
> > > +
> > > +#define BU64754_REG_SERVE      CCI_REG16(0xd9)
> > > +#define BU64754_SERVE_ON       0x0404
> > > +
> > > +#define BU64754_REG_POSITION   CCI_REG16(0x45)
> > > +#define BU64753_POSITION_MAX   1023 /* 0x3ff */
> > > +
> > > +#define BU64754_POWER_ON_DELAY 800 /* uS : t1, t3 */
> > > +
> > > +struct bu64754 {
> > > +       struct device *dev;
> > > +
> > > +       struct v4l2_ctrl_handler ctrls_vcm;
> > > +       struct v4l2_subdev sd;
> > > +       struct regmap *cci;
> > > +
> > > +       u16 current_val;
> > > +       struct regulator *vdd;
> > > +       struct notifier_block notifier;
> > > +};
> > > +
> > > +static inline struct bu64754 *sd_to_bu64754(struct v4l2_subdev *subdev)
> > > +{
> > > +       return container_of(subdev, struct bu64754, sd);
> > > +}
> > > +
> > > +static int bu64754_set(struct bu64754 *bu64754, u16 position)
> > > +{
> > > +       int ret;
> > > +
> > > +       ret = cci_write(bu64754->cci, BU64754_REG_POSITION, position, NULL);
> > > +       if (ret) {
> > > +               dev_err(bu64754->dev, "Set position failed ret=%d\n", ret);
> > > +               return ret;
> > > +       }
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static int bu64754_active(struct bu64754 *bu64754)
> > > +{
> > > +       int ret;
> > > +
> > > +       /* Power on */
> > > +       ret = cci_write(bu64754->cci, BU64754_REG_ACTIVE, BU64754_ACTIVE_MODE,
> > > +                       NULL);
> > > +       if (ret < 0) {
> > > +               dev_err(bu64754->dev, "Failed to set active mode ret = %d\n",
> > > +                       ret);
> > > +               return ret;
> > > +       }
> > > +
> > > +       /* Serve on */
> > > +       ret = cci_write(bu64754->cci, BU64754_REG_SERVE, BU64754_SERVE_ON,
> > > +                       NULL);
> > > +       if (ret < 0) {
> > > +               dev_err(bu64754->dev, "Failed to enable serve ret = %d\n",
> > > +                       ret);
> > > +               return ret;
> > > +       }
> > > +
> > > +       return bu64754_set(bu64754, bu64754->current_val);
> > > +}
> > > +
> > > +static int bu64754_standby(struct bu64754 *bu64754)
> > > +{
> > > +       int ret;
> > > +
> > > +       cci_write(bu64754->cci, BU64754_REG_ACTIVE, 0, &ret);
> > > +       if (ret < 0)
> > > +               dev_err(bu64754->dev, "Failed to set active mode ret = %d\n",
> > > +                       ret);
> > > +
> > > +       return ret;
> > > +}
> > > +
> > > +static int bu64754_regulator_event(struct notifier_block *nb,
> > > +                                  unsigned long action, void *data)
> > > +{
> > > +       struct bu64754 *bu64754 = container_of(nb, struct bu64754, notifier);
> > > +
> > > +       if (action & REGULATOR_EVENT_ENABLE) {
> > > +               /*
> > > +                * Initialisation delay between VDD low->high and availability
> > > +                * i2c operation.
> > > +                */
> > > +               usleep_range(BU64754_POWER_ON_DELAY,
> > > +                            BU64754_POWER_ON_DELAY + 100);
> > > +
> > > +               bu64754_active(bu64754);
> > > +       } else if (action & REGULATOR_EVENT_PRE_DISABLE) {
> > > +               bu64754_standby(bu64754);
> > > +       }
> >
> > Presumably this is based on the assumption that the same regulator
> > controls sensor and lens, so when the sensor is powered up the lens
> > position gets restored.
> > I'm sure when I suggested doing the same previously it was shot down
> > in flames ... found it [2]
> >
> > Personally I think it makes sense that the lens powers up
> > automagically, and have almost exactly the same code as this in a
> > couple of our VCM drivers, but others disagree.
>
> I've tested this on the Raspberry Pi, and I expect this design model
> comes from you originally then. I thought it was a good way to handle
> things, but I wasn't aware that others had already disagreed.
>
> Being a module that will connect over the RPi interface, with a single
> POWER_EN, indeed - the regulators are shared with the camera, and
> synchronising how it is handled seemed reasonable. I guess I have some
> more reading to do now then.

Personally I like it as it simplifies userspace for some simple cases,
and has no impact on the complex ones which always open the lens
driver subdev for themselves. Let's see what other people think.

> I'm already thinking about making a 'simple-vcm' module or parent-driver
> that can reduce the boilerplate for the vcms though. There's very little
> difference between them all ... and I think they could be abstracted out
> substantially to simplify adding new devices.

Reducing boilerplate is always nice!

  Dave

> --
> Kieran
>
>
>
> >
> >   Dave
> >
> > [2] https://lore.kernel.org/all/CAPY8ntBZpZjecHNCMf-eMefcp2EgmbqkXMt4p=UeOe0n-o8WrA@mail.gmail.com/
> >
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static int bu64754_set_ctrl(struct v4l2_ctrl *ctrl)
> > > +{
> > > +       struct bu64754 *bu64754 = container_of(ctrl->handler,
> > > +               struct bu64754, ctrls_vcm);
> > > +
> > > +       if (ctrl->id == V4L2_CID_FOCUS_ABSOLUTE) {
> > > +               bu64754->current_val = ctrl->val;
> > > +               return bu64754_set(bu64754, ctrl->val);
> > > +       }
> > > +
> > > +       return -EINVAL;
> > > +}
> > > +
> > > +static const struct v4l2_ctrl_ops bu64754_vcm_ctrl_ops = {
> > > +       .s_ctrl = bu64754_set_ctrl,
> > > +};
> > > +
> > > +static int bu64754_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> > > +{
> > > +       return pm_runtime_resume_and_get(sd->dev);
> > > +}
> > > +
> > > +static int bu64754_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> > > +{
> > > +       pm_runtime_put(sd->dev);
> > > +       return 0;
> > > +}
> > > +
> > > +static const struct v4l2_subdev_internal_ops bu64754_int_ops = {
> > > +       .open = bu64754_open,
> > > +       .close = bu64754_close,
> > > +};
> > > +
> > > +static const struct v4l2_subdev_ops bu64754_ops = { };
> > > +
> > > +static void bu64754_subdev_cleanup(struct bu64754 *bu64754)
> > > +{
> > > +       v4l2_async_unregister_subdev(&bu64754->sd);
> > > +       v4l2_ctrl_handler_free(&bu64754->ctrls_vcm);
> > > +       media_entity_cleanup(&bu64754->sd.entity);
> > > +}
> > > +
> > > +static int bu64754_init_controls(struct bu64754 *bu64754)
> > > +{
> > > +       struct v4l2_ctrl_handler *hdl = &bu64754->ctrls_vcm;
> > > +       const struct v4l2_ctrl_ops *ops = &bu64754_vcm_ctrl_ops;
> > > +
> > > +       v4l2_ctrl_handler_init(hdl, 1);
> > > +
> > > +       v4l2_ctrl_new_std(hdl, ops, V4L2_CID_FOCUS_ABSOLUTE,
> > > +                         0, BU64753_POSITION_MAX, 1, 0);
> > > +
> > > +       bu64754->current_val = 0;
> > > +
> > > +       bu64754->sd.ctrl_handler = hdl;
> > > +       if (hdl->error) {
> > > +               dev_err(bu64754->dev, "%s fail error: 0x%x\n",
> > > +                       __func__, hdl->error);
> > > +               return hdl->error;
> > > +       }
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static int bu64754_probe(struct i2c_client *client)
> > > +{
> > > +       struct bu64754 *bu64754;
> > > +       int ret;
> > > +
> > > +       bu64754 = devm_kzalloc(&client->dev, sizeof(*bu64754), GFP_KERNEL);
> > > +       if (!bu64754)
> > > +               return -ENOMEM;
> > > +
> > > +       bu64754->dev = &client->dev;
> > > +
> > > +       bu64754->cci = devm_cci_regmap_init_i2c(client, 8);
> > > +       if (IS_ERR(bu64754->cci)) {
> > > +               dev_err(bu64754->dev, "Failed to initialize CCI\n");
> > > +               return PTR_ERR(bu64754->cci);
> > > +       }
> > > +
> > > +       bu64754->vdd = devm_regulator_get_optional(&client->dev, "vdd");
> > > +       if (IS_ERR(bu64754->vdd)) {
> > > +               if (PTR_ERR(bu64754->vdd) != -ENODEV)
> > > +                       return PTR_ERR(bu64754->vdd);
> > > +
> > > +               bu64754->vdd = NULL;
> > > +       } else {
> > > +               bu64754->notifier.notifier_call = bu64754_regulator_event;
> > > +
> > > +               ret = regulator_register_notifier(bu64754->vdd,
> > > +                                                 &bu64754->notifier);
> > > +               if (ret) {
> > > +                       dev_err(bu64754->dev,
> > > +                               "could not register regulator notifier\n");
> > > +                       return ret;
> > > +               }
> > > +       }
> > > +
> > > +       v4l2_i2c_subdev_init(&bu64754->sd, client, &bu64754_ops);
> > > +       bu64754->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > > +       bu64754->sd.internal_ops = &bu64754_int_ops;
> > > +       bu64754->sd.entity.function = MEDIA_ENT_F_LENS;
> > > +
> > > +       ret = bu64754_init_controls(bu64754);
> > > +       if (ret)
> > > +               goto err_cleanup;
> > > +
> > > +       ret = media_entity_pads_init(&bu64754->sd.entity, 0, NULL);
> > > +       if (ret < 0)
> > > +               goto err_cleanup;
> > > +
> > > +       ret = v4l2_async_register_subdev(&bu64754->sd);
> > > +       if (ret < 0)
> > > +               goto err_cleanup;
> > > +
> > > +       if (!bu64754->vdd)
> > > +               pm_runtime_set_active(&client->dev);
> > > +
> > > +       pm_runtime_enable(&client->dev);
> > > +       pm_runtime_idle(&client->dev);
> > > +
> > > +       return 0;
> > > +
> > > +err_cleanup:
> > > +       v4l2_ctrl_handler_free(&bu64754->ctrls_vcm);
> > > +       media_entity_cleanup(&bu64754->sd.entity);
> > > +
> > > +       return ret;
> > > +}
> > > +
> > > +static void bu64754_remove(struct i2c_client *client)
> > > +{
> > > +       struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > > +       struct bu64754 *bu64754 = sd_to_bu64754(sd);
> > > +
> > > +       if (bu64754->vdd)
> > > +               regulator_unregister_notifier(bu64754->vdd,
> > > +                                             &bu64754->notifier);
> > > +
> > > +       pm_runtime_disable(&client->dev);
> > > +
> > > +       bu64754_subdev_cleanup(bu64754);
> > > +}
> > > +
> > > +static int __maybe_unused bu64754_vcm_suspend(struct device *dev)
> > > +{
> > > +       struct i2c_client *client = to_i2c_client(dev);
> > > +       struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > > +       struct bu64754 *bu64754 = sd_to_bu64754(sd);
> > > +
> > > +       if (bu64754->vdd)
> > > +               return regulator_disable(bu64754->vdd);
> > > +
> > > +       return bu64754_standby(bu64754);
> > > +}
> > > +
> > > +static int  __maybe_unused bu64754_vcm_resume(struct device *dev)
> > > +{
> > > +       struct i2c_client *client = to_i2c_client(dev);
> > > +       struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > > +       struct bu64754 *bu64754 = sd_to_bu64754(sd);
> > > +
> > > +       if (bu64754->vdd)
> > > +               return regulator_enable(bu64754->vdd);
> > > +
> > > +       return bu64754_active(bu64754);
> > > +}
> > > +
> > > +static const struct of_device_id bu64754_of_table[] = {
> > > +       { .compatible = "rohm,bu64754", },
> > > +       { /* sentinel */ }
> > > +};
> > > +
> > > +MODULE_DEVICE_TABLE(of, bu64754_of_table);
> > > +
> > > +static SIMPLE_DEV_PM_OPS(bu64754_pm, bu64754_vcm_suspend, bu64754_vcm_resume);
> > > +
> > > +static struct i2c_driver bu64754_i2c_driver = {
> > > +       .driver = {
> > > +               .name = "bu64754",
> > > +               .pm = &bu64754_pm,
> > > +               .of_match_table = bu64754_of_table,
> > > +       },
> > > +       .probe = bu64754_probe,
> > > +       .remove = bu64754_remove,
> > > +};
> > > +
> > > +module_i2c_driver(bu64754_i2c_driver);
> > > +
> > > +MODULE_AUTHOR("Kieran Bingham <kieran.bingham@ideasonboard.com>");
> > > +MODULE_DESCRIPTION("ROHM BU64754 VCM driver");
> > > +MODULE_LICENSE("GPL");
> > > --
> > > 2.34.1
> > >

Re: [PATCH 2/2] media: i2c: Add ROHM BU64754 Camera Autofocus Actuator

[Kieran Bingham]

Quoting Dave Stevenson (2023-09-18 11:16:47)
> Hi Kieran
> 
> On Fri, 15 Sept 2023 at 20:19, Kieran Bingham
> <kieran.bingham@ideasonboard.com> wrote:
> >
> > Hi Dave,
> >
> > Quoting Dave Stevenson (2023-09-15 18:44:02)
> > > Hi Kieran
> > >
> > > On Fri, 15 Sept 2023 at 18:02, Kieran Bingham
> > > <kieran.bingham@ideasonboard.com> wrote:
> > > >
> > > > Add support for the ROHM BU64754 Motor Driver for Camera Autofocus. A
> > > > V4L2 Subdevice is registered and provides a single
> > > > V4L2_CID_FOCUS_ABSOLUTE control.
> > > >
> > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > ---
> > > >  MAINTAINERS                 |   1 +
> > > >  drivers/media/i2c/Kconfig   |  13 ++
> > > >  drivers/media/i2c/Makefile  |   1 +
> > > >  drivers/media/i2c/bu64754.c | 308 ++++++++++++++++++++++++++++++++++++
> > > >  4 files changed, 323 insertions(+)
> > > >  create mode 100644 drivers/media/i2c/bu64754.c
> > > >
> > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > index f43e0ffcaf56..fd244560c317 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -18576,6 +18576,7 @@ L:      linux-media@vger.kernel.org
> > > >  S:     Maintained
> > > >  T:     git git://linuxtv.org/media_tree.git
> > > >  F:     Documentation/devicetree/bindings/media/i2c/rohm,bu64754.yaml
> > > > +F:     drivers/media/i2c/bu64754.c
> > > >
> > > >  ROHM MULTIFUNCTION BD9571MWV-M PMIC DEVICE DRIVERS
> > > >  M:     Marek Vasut <marek.vasut+renesas@gmail.com>
> > > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > > > index 74ff833ff48c..b7b8004816ed 100644
> > > > --- a/drivers/media/i2c/Kconfig
> > > > +++ b/drivers/media/i2c/Kconfig
> > > > @@ -641,6 +641,19 @@ config VIDEO_AK7375
> > > >           capability. This is designed for linear control of
> > > >           voice coil motors, controlled via I2C serial interface.
> > > >
> > > > +config VIDEO_BU64754
> > > > +       tristate "BU64754 Motor Driver for Camera Autofocus"
> > > > +       depends on I2C && VIDEO_DEV
> > > > +       select MEDIA_CONTROLLER
> > > > +       select VIDEO_V4L2_SUBDEV_API
> > > > +       select V4L2_ASYNC
> > > > +       select V4L2_CCI_I2C
> > > > +       help
> > > > +         This is a driver for the BU64754 Motor Driver for Camera
> > > > +         Autofocus. The BU64754 is an actuator driver IC which can
> > > > +         control the actuator position precisely using an internal
> > > > +         Hall Sensor.
> > >
> > > I can't find any data on this driver.
> > > Is it still expecting a VCM and hence near instantaneous movement? I
> > > was noting your comment on the hall sensor and thinking you
> > > potentially needed to be able to report whether the target position
> > > had been reached or not. Michael's series at [1] was trying to address
> > > that.
> >
> > The hall sensor is internal. The component description is just a
> > slightly fixed grammar of the product brief.
> >
> > As I understand it, this is a VCM yes but I have so little visibility.
> 
> Fair enough, should be quick enough not to make too much difference then.

I think so yes. The world would be fantastic if we could ever actually
get the right documentation but ... ho hum ;-)


> 
> > >
> > > [1] https://patchwork.linuxtv.org/project/linux-media/cover/20230406-feature-controls-lens-v2-0-faa8ad2bc404@wolfvision.net/
> > >
> > > > +
> > > >  config VIDEO_DW9714
> > > >         tristate "DW9714 lens voice coil support"
> > > >         depends on I2C && VIDEO_DEV
> > > > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> > > > index 80b00d39b48f..e62aa0df7b1a 100644
> > > > --- a/drivers/media/i2c/Makefile
> > > > +++ b/drivers/media/i2c/Makefile
> > > > @@ -22,6 +22,7 @@ obj-$(CONFIG_VIDEO_AR0521) += ar0521.o
> > > >  obj-$(CONFIG_VIDEO_BT819) += bt819.o
> > > >  obj-$(CONFIG_VIDEO_BT856) += bt856.o
> > > >  obj-$(CONFIG_VIDEO_BT866) += bt866.o
> > > > +obj-$(CONFIG_VIDEO_BU64754) += bu64754.o
> > > >  obj-$(CONFIG_VIDEO_CCS) += ccs/
> > > >  obj-$(CONFIG_VIDEO_CCS_PLL) += ccs-pll.o
> > > >  obj-$(CONFIG_VIDEO_CS3308) += cs3308.o
> > > > diff --git a/drivers/media/i2c/bu64754.c b/drivers/media/i2c/bu64754.c
> > > > new file mode 100644
> > > > index 000000000000..3367b6f17660
> > > > --- /dev/null
> > > > +++ b/drivers/media/i2c/bu64754.c
> > > > @@ -0,0 +1,308 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * The BU64754 is an actuator driver IC which can control the
> > > > + * actuator position precisely using an internal Hall Sensor.
> > > > + */
> > > > +
> > > > +#include <linux/delay.h>
> > > > +#include <linux/i2c.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/pm_runtime.h>
> > > > +#include <linux/regulator/consumer.h>
> > > > +
> > > > +#include <media/v4l2-cci.h>
> > > > +#include <media/v4l2-ctrls.h>
> > > > +#include <media/v4l2-device.h>
> > > > +
> > > > +#define BU64754_REG_ACTIVE     CCI_REG16(0x07)
> > > > +#define BU64754_ACTIVE_MODE    0x8080
> > > > +
> > > > +#define BU64754_REG_SERVE      CCI_REG16(0xd9)
> > > > +#define BU64754_SERVE_ON       0x0404
> > > > +
> > > > +#define BU64754_REG_POSITION   CCI_REG16(0x45)
> > > > +#define BU64753_POSITION_MAX   1023 /* 0x3ff */
> > > > +
> > > > +#define BU64754_POWER_ON_DELAY 800 /* uS : t1, t3 */
> > > > +
> > > > +struct bu64754 {
> > > > +       struct device *dev;
> > > > +
> > > > +       struct v4l2_ctrl_handler ctrls_vcm;
> > > > +       struct v4l2_subdev sd;
> > > > +       struct regmap *cci;
> > > > +
> > > > +       u16 current_val;
> > > > +       struct regulator *vdd;
> > > > +       struct notifier_block notifier;
> > > > +};
> > > > +
> > > > +static inline struct bu64754 *sd_to_bu64754(struct v4l2_subdev *subdev)
> > > > +{
> > > > +       return container_of(subdev, struct bu64754, sd);
> > > > +}
> > > > +
> > > > +static int bu64754_set(struct bu64754 *bu64754, u16 position)
> > > > +{
> > > > +       int ret;
> > > > +
> > > > +       ret = cci_write(bu64754->cci, BU64754_REG_POSITION, position, NULL);
> > > > +       if (ret) {
> > > > +               dev_err(bu64754->dev, "Set position failed ret=%d\n", ret);
> > > > +               return ret;
> > > > +       }
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +static int bu64754_active(struct bu64754 *bu64754)
> > > > +{
> > > > +       int ret;
> > > > +
> > > > +       /* Power on */
> > > > +       ret = cci_write(bu64754->cci, BU64754_REG_ACTIVE, BU64754_ACTIVE_MODE,
> > > > +                       NULL);
> > > > +       if (ret < 0) {
> > > > +               dev_err(bu64754->dev, "Failed to set active mode ret = %d\n",
> > > > +                       ret);
> > > > +               return ret;
> > > > +       }
> > > > +
> > > > +       /* Serve on */
> > > > +       ret = cci_write(bu64754->cci, BU64754_REG_SERVE, BU64754_SERVE_ON,
> > > > +                       NULL);
> > > > +       if (ret < 0) {
> > > > +               dev_err(bu64754->dev, "Failed to enable serve ret = %d\n",
> > > > +                       ret);
> > > > +               return ret;
> > > > +       }
> > > > +
> > > > +       return bu64754_set(bu64754, bu64754->current_val);
> > > > +}
> > > > +
> > > > +static int bu64754_standby(struct bu64754 *bu64754)
> > > > +{
> > > > +       int ret;
> > > > +
> > > > +       cci_write(bu64754->cci, BU64754_REG_ACTIVE, 0, &ret);
> > > > +       if (ret < 0)
> > > > +               dev_err(bu64754->dev, "Failed to set active mode ret = %d\n",
> > > > +                       ret);
> > > > +
> > > > +       return ret;
> > > > +}
> > > > +
> > > > +static int bu64754_regulator_event(struct notifier_block *nb,
> > > > +                                  unsigned long action, void *data)
> > > > +{
> > > > +       struct bu64754 *bu64754 = container_of(nb, struct bu64754, notifier);
> > > > +
> > > > +       if (action & REGULATOR_EVENT_ENABLE) {
> > > > +               /*
> > > > +                * Initialisation delay between VDD low->high and availability
> > > > +                * i2c operation.
> > > > +                */
> > > > +               usleep_range(BU64754_POWER_ON_DELAY,
> > > > +                            BU64754_POWER_ON_DELAY + 100);
> > > > +
> > > > +               bu64754_active(bu64754);
> > > > +       } else if (action & REGULATOR_EVENT_PRE_DISABLE) {
> > > > +               bu64754_standby(bu64754);
> > > > +       }
> > >
> > > Presumably this is based on the assumption that the same regulator
> > > controls sensor and lens, so when the sensor is powered up the lens
> > > position gets restored.
> > > I'm sure when I suggested doing the same previously it was shot down
> > > in flames ... found it [2]
> > >
> > > Personally I think it makes sense that the lens powers up
> > > automagically, and have almost exactly the same code as this in a
> > > couple of our VCM drivers, but others disagree.
> >
> > I've tested this on the Raspberry Pi, and I expect this design model
> > comes from you originally then. I thought it was a good way to handle
> > things, but I wasn't aware that others had already disagreed.
> >
> > Being a module that will connect over the RPi interface, with a single
> > POWER_EN, indeed - the regulators are shared with the camera, and
> > synchronising how it is handled seemed reasonable. I guess I have some
> > more reading to do now then.
> 
> Personally I like it as it simplifies userspace for some simple cases,
> and has no impact on the complex ones which always open the lens
> driver subdev for themselves. Let's see what other people think.

Yes, and ensures that if someone opens the camera (powers up all the
shared regulators) the driver gets a notification and can 'do the right
thing' ... like either move to a known position or a low power position,
or the last configured position. Anything that's expected...

> > I'm already thinking about making a 'simple-vcm' module or parent-driver
> > that can reduce the boilerplate for the vcms though. There's very little
> > difference between them all ... and I think they could be abstracted out
> > substantially to simplify adding new devices.
> 
> Reducing boilerplate is always nice!

Having looked around the VCMs in both mainline and the RPi kernel -
there really is a lot duplicated 'for a device with a single control' -
so i've sketched out a simple-vcm at the weekend, but I won't be able to
continue that until next weekend now. I hope it will make things nicer.

I'll probably see if we can move all lens drivers to media/i2c/lens/*
too - media/i2c is getting crowded with lots of hard to identify 'part
numbers'. I think we could do better organising there.

I'd probably also move sensor drivers to media/i2c/sensor/ or even as
far as media/i2c/sensor/{sony,omnivision,$vendor}/, .... particularly if
we're going to get larger number of drivers for all these parts now
there's more framework to support them.

And then I'd love to see if we could reduce sensor boilerplate too and
have a v4l2-sensor 'class'. I'll start with the 'easy' vcm first though
;-)

--
Kieran


> 
>   Dave
> 
> > --
> > Kieran
> >
> >
> >
> > >
> > >   Dave
> > >
> > > [2] https://lore.kernel.org/all/CAPY8ntBZpZjecHNCMf-eMefcp2EgmbqkXMt4p=UeOe0n-o8WrA@mail.gmail.com/
> > >
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +static int bu64754_set_ctrl(struct v4l2_ctrl *ctrl)
> > > > +{
> > > > +       struct bu64754 *bu64754 = container_of(ctrl->handler,
> > > > +               struct bu64754, ctrls_vcm);
> > > > +
> > > > +       if (ctrl->id == V4L2_CID_FOCUS_ABSOLUTE) {
> > > > +               bu64754->current_val = ctrl->val;
> > > > +               return bu64754_set(bu64754, ctrl->val);
> > > > +       }
> > > > +
> > > > +       return -EINVAL;
> > > > +}
> > > > +
> > > > +static const struct v4l2_ctrl_ops bu64754_vcm_ctrl_ops = {
> > > > +       .s_ctrl = bu64754_set_ctrl,
> > > > +};
> > > > +
> > > > +static int bu64754_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> > > > +{
> > > > +       return pm_runtime_resume_and_get(sd->dev);
> > > > +}
> > > > +
> > > > +static int bu64754_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> > > > +{
> > > > +       pm_runtime_put(sd->dev);
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +static const struct v4l2_subdev_internal_ops bu64754_int_ops = {
> > > > +       .open = bu64754_open,
> > > > +       .close = bu64754_close,
> > > > +};
> > > > +
> > > > +static const struct v4l2_subdev_ops bu64754_ops = { };
> > > > +
> > > > +static void bu64754_subdev_cleanup(struct bu64754 *bu64754)
> > > > +{
> > > > +       v4l2_async_unregister_subdev(&bu64754->sd);
> > > > +       v4l2_ctrl_handler_free(&bu64754->ctrls_vcm);
> > > > +       media_entity_cleanup(&bu64754->sd.entity);
> > > > +}
> > > > +
> > > > +static int bu64754_init_controls(struct bu64754 *bu64754)
> > > > +{
> > > > +       struct v4l2_ctrl_handler *hdl = &bu64754->ctrls_vcm;
> > > > +       const struct v4l2_ctrl_ops *ops = &bu64754_vcm_ctrl_ops;
> > > > +
> > > > +       v4l2_ctrl_handler_init(hdl, 1);
> > > > +
> > > > +       v4l2_ctrl_new_std(hdl, ops, V4L2_CID_FOCUS_ABSOLUTE,
> > > > +                         0, BU64753_POSITION_MAX, 1, 0);
> > > > +
> > > > +       bu64754->current_val = 0;
> > > > +
> > > > +       bu64754->sd.ctrl_handler = hdl;
> > > > +       if (hdl->error) {
> > > > +               dev_err(bu64754->dev, "%s fail error: 0x%x\n",
> > > > +                       __func__, hdl->error);
> > > > +               return hdl->error;
> > > > +       }
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +static int bu64754_probe(struct i2c_client *client)
> > > > +{
> > > > +       struct bu64754 *bu64754;
> > > > +       int ret;
> > > > +
> > > > +       bu64754 = devm_kzalloc(&client->dev, sizeof(*bu64754), GFP_KERNEL);
> > > > +       if (!bu64754)
> > > > +               return -ENOMEM;
> > > > +
> > > > +       bu64754->dev = &client->dev;
> > > > +
> > > > +       bu64754->cci = devm_cci_regmap_init_i2c(client, 8);
> > > > +       if (IS_ERR(bu64754->cci)) {
> > > > +               dev_err(bu64754->dev, "Failed to initialize CCI\n");
> > > > +               return PTR_ERR(bu64754->cci);
> > > > +       }
> > > > +
> > > > +       bu64754->vdd = devm_regulator_get_optional(&client->dev, "vdd");
> > > > +       if (IS_ERR(bu64754->vdd)) {
> > > > +               if (PTR_ERR(bu64754->vdd) != -ENODEV)
> > > > +                       return PTR_ERR(bu64754->vdd);
> > > > +
> > > > +               bu64754->vdd = NULL;
> > > > +       } else {
> > > > +               bu64754->notifier.notifier_call = bu64754_regulator_event;
> > > > +
> > > > +               ret = regulator_register_notifier(bu64754->vdd,
> > > > +                                                 &bu64754->notifier);
> > > > +               if (ret) {
> > > > +                       dev_err(bu64754->dev,
> > > > +                               "could not register regulator notifier\n");
> > > > +                       return ret;
> > > > +               }
> > > > +       }
> > > > +
> > > > +       v4l2_i2c_subdev_init(&bu64754->sd, client, &bu64754_ops);
> > > > +       bu64754->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > > > +       bu64754->sd.internal_ops = &bu64754_int_ops;
> > > > +       bu64754->sd.entity.function = MEDIA_ENT_F_LENS;
> > > > +
> > > > +       ret = bu64754_init_controls(bu64754);
> > > > +       if (ret)
> > > > +               goto err_cleanup;
> > > > +
> > > > +       ret = media_entity_pads_init(&bu64754->sd.entity, 0, NULL);
> > > > +       if (ret < 0)
> > > > +               goto err_cleanup;
> > > > +
> > > > +       ret = v4l2_async_register_subdev(&bu64754->sd);
> > > > +       if (ret < 0)
> > > > +               goto err_cleanup;
> > > > +
> > > > +       if (!bu64754->vdd)
> > > > +               pm_runtime_set_active(&client->dev);
> > > > +
> > > > +       pm_runtime_enable(&client->dev);
> > > > +       pm_runtime_idle(&client->dev);
> > > > +
> > > > +       return 0;
> > > > +
> > > > +err_cleanup:
> > > > +       v4l2_ctrl_handler_free(&bu64754->ctrls_vcm);
> > > > +       media_entity_cleanup(&bu64754->sd.entity);
> > > > +
> > > > +       return ret;
> > > > +}
> > > > +
> > > > +static void bu64754_remove(struct i2c_client *client)
> > > > +{
> > > > +       struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > > > +       struct bu64754 *bu64754 = sd_to_bu64754(sd);
> > > > +
> > > > +       if (bu64754->vdd)
> > > > +               regulator_unregister_notifier(bu64754->vdd,
> > > > +                                             &bu64754->notifier);
> > > > +
> > > > +       pm_runtime_disable(&client->dev);
> > > > +
> > > > +       bu64754_subdev_cleanup(bu64754);
> > > > +}
> > > > +
> > > > +static int __maybe_unused bu64754_vcm_suspend(struct device *dev)
> > > > +{
> > > > +       struct i2c_client *client = to_i2c_client(dev);
> > > > +       struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > > > +       struct bu64754 *bu64754 = sd_to_bu64754(sd);
> > > > +
> > > > +       if (bu64754->vdd)
> > > > +               return regulator_disable(bu64754->vdd);
> > > > +
> > > > +       return bu64754_standby(bu64754);
> > > > +}
> > > > +
> > > > +static int  __maybe_unused bu64754_vcm_resume(struct device *dev)
> > > > +{
> > > > +       struct i2c_client *client = to_i2c_client(dev);
> > > > +       struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > > > +       struct bu64754 *bu64754 = sd_to_bu64754(sd);
> > > > +
> > > > +       if (bu64754->vdd)
> > > > +               return regulator_enable(bu64754->vdd);
> > > > +
> > > > +       return bu64754_active(bu64754);
> > > > +}
> > > > +
> > > > +static const struct of_device_id bu64754_of_table[] = {
> > > > +       { .compatible = "rohm,bu64754", },
> > > > +       { /* sentinel */ }
> > > > +};
> > > > +
> > > > +MODULE_DEVICE_TABLE(of, bu64754_of_table);
> > > > +
> > > > +static SIMPLE_DEV_PM_OPS(bu64754_pm, bu64754_vcm_suspend, bu64754_vcm_resume);
> > > > +
> > > > +static struct i2c_driver bu64754_i2c_driver = {
> > > > +       .driver = {
> > > > +               .name = "bu64754",
> > > > +               .pm = &bu64754_pm,
> > > > +               .of_match_table = bu64754_of_table,
> > > > +       },
> > > > +       .probe = bu64754_probe,
> > > > +       .remove = bu64754_remove,
> > > > +};
> > > > +
> > > > +module_i2c_driver(bu64754_i2c_driver);
> > > > +
> > > > +MODULE_AUTHOR("Kieran Bingham <kieran.bingham@ideasonboard.com>");
> > > > +MODULE_DESCRIPTION("ROHM BU64754 VCM driver");
> > > > +MODULE_LICENSE("GPL");
> > > > --
> > > > 2.34.1
> > > >

Re: [PATCH 2/2] media: i2c: Add ROHM BU64754 Camera Autofocus Actuator

[Sakari Ailus]

Hi Kieran, Dave,

On Fri, Sep 15, 2023 at 08:19:32PM +0100, Kieran Bingham wrote:
> Hi Dave,
> 
> Quoting Dave Stevenson (2023-09-15 18:44:02)
> > Hi Kieran
> > 
> > On Fri, 15 Sept 2023 at 18:02, Kieran Bingham
> > <kieran.bingham@ideasonboard.com> wrote:
> > >
> > > Add support for the ROHM BU64754 Motor Driver for Camera Autofocus. A
> > > V4L2 Subdevice is registered and provides a single
> > > V4L2_CID_FOCUS_ABSOLUTE control.
> > >
> > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > ---
> > >  MAINTAINERS                 |   1 +
> > >  drivers/media/i2c/Kconfig   |  13 ++
> > >  drivers/media/i2c/Makefile  |   1 +
> > >  drivers/media/i2c/bu64754.c | 308 ++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 323 insertions(+)
> > >  create mode 100644 drivers/media/i2c/bu64754.c
> > >
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index f43e0ffcaf56..fd244560c317 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -18576,6 +18576,7 @@ L:      linux-media@vger.kernel.org
> > >  S:     Maintained
> > >  T:     git git://linuxtv.org/media_tree.git
> > >  F:     Documentation/devicetree/bindings/media/i2c/rohm,bu64754.yaml
> > > +F:     drivers/media/i2c/bu64754.c
> > >
> > >  ROHM MULTIFUNCTION BD9571MWV-M PMIC DEVICE DRIVERS
> > >  M:     Marek Vasut <marek.vasut+renesas@gmail.com>
> > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > > index 74ff833ff48c..b7b8004816ed 100644
> > > --- a/drivers/media/i2c/Kconfig
> > > +++ b/drivers/media/i2c/Kconfig
> > > @@ -641,6 +641,19 @@ config VIDEO_AK7375
> > >           capability. This is designed for linear control of
> > >           voice coil motors, controlled via I2C serial interface.
> > >
> > > +config VIDEO_BU64754
> > > +       tristate "BU64754 Motor Driver for Camera Autofocus"
> > > +       depends on I2C && VIDEO_DEV
> > > +       select MEDIA_CONTROLLER
> > > +       select VIDEO_V4L2_SUBDEV_API
> > > +       select V4L2_ASYNC
> > > +       select V4L2_CCI_I2C
> > > +       help
> > > +         This is a driver for the BU64754 Motor Driver for Camera
> > > +         Autofocus. The BU64754 is an actuator driver IC which can
> > > +         control the actuator position precisely using an internal
> > > +         Hall Sensor.
> > 
> > I can't find any data on this driver.
> > Is it still expecting a VCM and hence near instantaneous movement? I
> > was noting your comment on the hall sensor and thinking you
> > potentially needed to be able to report whether the target position
> > had been reached or not. Michael's series at [1] was trying to address
> > that.
> 
> The hall sensor is internal. The component description is just a
> slightly fixed grammar of the product brief.
> 
> As I understand it, this is a VCM yes but I have so little visibility.
> 
> > 
> > [1] https://patchwork.linuxtv.org/project/linux-media/cover/20230406-feature-controls-lens-v2-0-faa8ad2bc404@wolfvision.net/
> > 
> > > +
> > >  config VIDEO_DW9714
> > >         tristate "DW9714 lens voice coil support"
> > >         depends on I2C && VIDEO_DEV
> > > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> > > index 80b00d39b48f..e62aa0df7b1a 100644
> > > --- a/drivers/media/i2c/Makefile
> > > +++ b/drivers/media/i2c/Makefile
> > > @@ -22,6 +22,7 @@ obj-$(CONFIG_VIDEO_AR0521) += ar0521.o
> > >  obj-$(CONFIG_VIDEO_BT819) += bt819.o
> > >  obj-$(CONFIG_VIDEO_BT856) += bt856.o
> > >  obj-$(CONFIG_VIDEO_BT866) += bt866.o
> > > +obj-$(CONFIG_VIDEO_BU64754) += bu64754.o
> > >  obj-$(CONFIG_VIDEO_CCS) += ccs/
> > >  obj-$(CONFIG_VIDEO_CCS_PLL) += ccs-pll.o
> > >  obj-$(CONFIG_VIDEO_CS3308) += cs3308.o
> > > diff --git a/drivers/media/i2c/bu64754.c b/drivers/media/i2c/bu64754.c
> > > new file mode 100644
> > > index 000000000000..3367b6f17660
> > > --- /dev/null
> > > +++ b/drivers/media/i2c/bu64754.c
> > > @@ -0,0 +1,308 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * The BU64754 is an actuator driver IC which can control the
> > > + * actuator position precisely using an internal Hall Sensor.
> > > + */
> > > +
> > > +#include <linux/delay.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/module.h>
> > > +#include <linux/pm_runtime.h>
> > > +#include <linux/regulator/consumer.h>
> > > +
> > > +#include <media/v4l2-cci.h>
> > > +#include <media/v4l2-ctrls.h>
> > > +#include <media/v4l2-device.h>
> > > +
> > > +#define BU64754_REG_ACTIVE     CCI_REG16(0x07)
> > > +#define BU64754_ACTIVE_MODE    0x8080
> > > +
> > > +#define BU64754_REG_SERVE      CCI_REG16(0xd9)
> > > +#define BU64754_SERVE_ON       0x0404
> > > +
> > > +#define BU64754_REG_POSITION   CCI_REG16(0x45)
> > > +#define BU64753_POSITION_MAX   1023 /* 0x3ff */
> > > +
> > > +#define BU64754_POWER_ON_DELAY 800 /* uS : t1, t3 */
> > > +
> > > +struct bu64754 {
> > > +       struct device *dev;
> > > +
> > > +       struct v4l2_ctrl_handler ctrls_vcm;
> > > +       struct v4l2_subdev sd;
> > > +       struct regmap *cci;
> > > +
> > > +       u16 current_val;
> > > +       struct regulator *vdd;
> > > +       struct notifier_block notifier;
> > > +};
> > > +
> > > +static inline struct bu64754 *sd_to_bu64754(struct v4l2_subdev *subdev)
> > > +{
> > > +       return container_of(subdev, struct bu64754, sd);
> > > +}
> > > +
> > > +static int bu64754_set(struct bu64754 *bu64754, u16 position)
> > > +{
> > > +       int ret;
> > > +
> > > +       ret = cci_write(bu64754->cci, BU64754_REG_POSITION, position, NULL);
> > > +       if (ret) {
> > > +               dev_err(bu64754->dev, "Set position failed ret=%d\n", ret);
> > > +               return ret;
> > > +       }
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static int bu64754_active(struct bu64754 *bu64754)
> > > +{
> > > +       int ret;
> > > +
> > > +       /* Power on */
> > > +       ret = cci_write(bu64754->cci, BU64754_REG_ACTIVE, BU64754_ACTIVE_MODE,
> > > +                       NULL);
> > > +       if (ret < 0) {
> > > +               dev_err(bu64754->dev, "Failed to set active mode ret = %d\n",
> > > +                       ret);
> > > +               return ret;
> > > +       }
> > > +
> > > +       /* Serve on */
> > > +       ret = cci_write(bu64754->cci, BU64754_REG_SERVE, BU64754_SERVE_ON,
> > > +                       NULL);
> > > +       if (ret < 0) {
> > > +               dev_err(bu64754->dev, "Failed to enable serve ret = %d\n",
> > > +                       ret);
> > > +               return ret;
> > > +       }
> > > +
> > > +       return bu64754_set(bu64754, bu64754->current_val);
> > > +}
> > > +
> > > +static int bu64754_standby(struct bu64754 *bu64754)
> > > +{
> > > +       int ret;
> > > +
> > > +       cci_write(bu64754->cci, BU64754_REG_ACTIVE, 0, &ret);
> > > +       if (ret < 0)
> > > +               dev_err(bu64754->dev, "Failed to set active mode ret = %d\n",
> > > +                       ret);
> > > +
> > > +       return ret;
> > > +}
> > > +
> > > +static int bu64754_regulator_event(struct notifier_block *nb,
> > > +                                  unsigned long action, void *data)
> > > +{
> > > +       struct bu64754 *bu64754 = container_of(nb, struct bu64754, notifier);
> > > +
> > > +       if (action & REGULATOR_EVENT_ENABLE) {
> > > +               /*
> > > +                * Initialisation delay between VDD low->high and availability
> > > +                * i2c operation.
> > > +                */
> > > +               usleep_range(BU64754_POWER_ON_DELAY,
> > > +                            BU64754_POWER_ON_DELAY + 100);
> > > +
> > > +               bu64754_active(bu64754);
> > > +       } else if (action & REGULATOR_EVENT_PRE_DISABLE) {
> > > +               bu64754_standby(bu64754);
> > > +       }
> > 
> > Presumably this is based on the assumption that the same regulator
> > controls sensor and lens, so when the sensor is powered up the lens
> > position gets restored.
> > I'm sure when I suggested doing the same previously it was shot down
> > in flames ... found it [2]
> > 
> > Personally I think it makes sense that the lens powers up
> > automagically, and have almost exactly the same code as this in a
> > couple of our VCM drivers, but others disagree.
> 
> I've tested this on the Raspberry Pi, and I expect this design model
> comes from you originally then. I thought it was a good way to handle
> things, but I wasn't aware that others had already disagreed.
> 
> Being a module that will connect over the RPi interface, with a single
> POWER_EN, indeed - the regulators are shared with the camera, and
> synchronising how it is handled seemed reasonable. I guess I have some
> more reading to do now then.

Consider this is a driver for a VCM that may well be used in other sensors
and so wired differently.

Is there no reset GPIO? Or another regulator? Add these, and you will start
having circular dependencies.

In general case, the power up sequence is specific to the camera module.
We don't have a device for the modules but I think we could for cases where
you can't correctly execute the power on and off sequences separately for
the sensor and the lens.

What comes to powering on the lens based on the sensor --- could you use a
device link for that purpose?

> 
> I'm already thinking about making a 'simple-vcm' module or parent-driver
> that can reduce the boilerplate for the vcms though. There's very little
> difference between them all ... and I think they could be abstracted out
> substantially to simplify adding new devices.

Either that, or a library to have the helpers modular. That approach could
produce lots of very small drivers though.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 2/2] media: i2c: Add ROHM BU64754 Camera Autofocus Actuator

[Hans de Goede]

Hi,

On 9/18/23 14:01, Sakari Ailus wrote:
> Hi Kieran, Dave,
> 
> On Fri, Sep 15, 2023 at 08:19:32PM +0100, Kieran Bingham wrote:
>> Hi Dave,
>>
>> Quoting Dave Stevenson (2023-09-15 18:44:02)
>>> Hi Kieran
>>>
>>> On Fri, 15 Sept 2023 at 18:02, Kieran Bingham
>>> <kieran.bingham@ideasonboard.com> wrote:
>>>>
>>>> Add support for the ROHM BU64754 Motor Driver for Camera Autofocus. A
>>>> V4L2 Subdevice is registered and provides a single
>>>> V4L2_CID_FOCUS_ABSOLUTE control.
>>>>
>>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>>> ---
>>>>  MAINTAINERS                 |   1 +
>>>>  drivers/media/i2c/Kconfig   |  13 ++
>>>>  drivers/media/i2c/Makefile  |   1 +
>>>>  drivers/media/i2c/bu64754.c | 308 ++++++++++++++++++++++++++++++++++++
>>>>  4 files changed, 323 insertions(+)
>>>>  create mode 100644 drivers/media/i2c/bu64754.c
>>>>
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index f43e0ffcaf56..fd244560c317 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -18576,6 +18576,7 @@ L:      linux-media@vger.kernel.org
>>>>  S:     Maintained
>>>>  T:     git git://linuxtv.org/media_tree.git
>>>>  F:     Documentation/devicetree/bindings/media/i2c/rohm,bu64754.yaml
>>>> +F:     drivers/media/i2c/bu64754.c
>>>>
>>>>  ROHM MULTIFUNCTION BD9571MWV-M PMIC DEVICE DRIVERS
>>>>  M:     Marek Vasut <marek.vasut+renesas@gmail.com>
>>>> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
>>>> index 74ff833ff48c..b7b8004816ed 100644
>>>> --- a/drivers/media/i2c/Kconfig
>>>> +++ b/drivers/media/i2c/Kconfig
>>>> @@ -641,6 +641,19 @@ config VIDEO_AK7375
>>>>           capability. This is designed for linear control of
>>>>           voice coil motors, controlled via I2C serial interface.
>>>>
>>>> +config VIDEO_BU64754
>>>> +       tristate "BU64754 Motor Driver for Camera Autofocus"
>>>> +       depends on I2C && VIDEO_DEV
>>>> +       select MEDIA_CONTROLLER
>>>> +       select VIDEO_V4L2_SUBDEV_API
>>>> +       select V4L2_ASYNC
>>>> +       select V4L2_CCI_I2C
>>>> +       help
>>>> +         This is a driver for the BU64754 Motor Driver for Camera
>>>> +         Autofocus. The BU64754 is an actuator driver IC which can
>>>> +         control the actuator position precisely using an internal
>>>> +         Hall Sensor.
>>>
>>> I can't find any data on this driver.
>>> Is it still expecting a VCM and hence near instantaneous movement? I
>>> was noting your comment on the hall sensor and thinking you
>>> potentially needed to be able to report whether the target position
>>> had been reached or not. Michael's series at [1] was trying to address
>>> that.
>>
>> The hall sensor is internal. The component description is just a
>> slightly fixed grammar of the product brief.
>>
>> As I understand it, this is a VCM yes but I have so little visibility.
>>
>>>
>>> [1] https://patchwork.linuxtv.org/project/linux-media/cover/20230406-feature-controls-lens-v2-0-faa8ad2bc404@wolfvision.net/
>>>
>>>> +
>>>>  config VIDEO_DW9714
>>>>         tristate "DW9714 lens voice coil support"
>>>>         depends on I2C && VIDEO_DEV
>>>> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
>>>> index 80b00d39b48f..e62aa0df7b1a 100644
>>>> --- a/drivers/media/i2c/Makefile
>>>> +++ b/drivers/media/i2c/Makefile
>>>> @@ -22,6 +22,7 @@ obj-$(CONFIG_VIDEO_AR0521) += ar0521.o
>>>>  obj-$(CONFIG_VIDEO_BT819) += bt819.o
>>>>  obj-$(CONFIG_VIDEO_BT856) += bt856.o
>>>>  obj-$(CONFIG_VIDEO_BT866) += bt866.o
>>>> +obj-$(CONFIG_VIDEO_BU64754) += bu64754.o
>>>>  obj-$(CONFIG_VIDEO_CCS) += ccs/
>>>>  obj-$(CONFIG_VIDEO_CCS_PLL) += ccs-pll.o
>>>>  obj-$(CONFIG_VIDEO_CS3308) += cs3308.o
>>>> diff --git a/drivers/media/i2c/bu64754.c b/drivers/media/i2c/bu64754.c
>>>> new file mode 100644
>>>> index 000000000000..3367b6f17660
>>>> --- /dev/null
>>>> +++ b/drivers/media/i2c/bu64754.c
>>>> @@ -0,0 +1,308 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * The BU64754 is an actuator driver IC which can control the
>>>> + * actuator position precisely using an internal Hall Sensor.
>>>> + */
>>>> +
>>>> +#include <linux/delay.h>
>>>> +#include <linux/i2c.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/pm_runtime.h>
>>>> +#include <linux/regulator/consumer.h>
>>>> +
>>>> +#include <media/v4l2-cci.h>
>>>> +#include <media/v4l2-ctrls.h>
>>>> +#include <media/v4l2-device.h>
>>>> +
>>>> +#define BU64754_REG_ACTIVE     CCI_REG16(0x07)
>>>> +#define BU64754_ACTIVE_MODE    0x8080
>>>> +
>>>> +#define BU64754_REG_SERVE      CCI_REG16(0xd9)
>>>> +#define BU64754_SERVE_ON       0x0404
>>>> +
>>>> +#define BU64754_REG_POSITION   CCI_REG16(0x45)
>>>> +#define BU64753_POSITION_MAX   1023 /* 0x3ff */
>>>> +
>>>> +#define BU64754_POWER_ON_DELAY 800 /* uS : t1, t3 */
>>>> +
>>>> +struct bu64754 {
>>>> +       struct device *dev;
>>>> +
>>>> +       struct v4l2_ctrl_handler ctrls_vcm;
>>>> +       struct v4l2_subdev sd;
>>>> +       struct regmap *cci;
>>>> +
>>>> +       u16 current_val;
>>>> +       struct regulator *vdd;
>>>> +       struct notifier_block notifier;
>>>> +};
>>>> +
>>>> +static inline struct bu64754 *sd_to_bu64754(struct v4l2_subdev *subdev)
>>>> +{
>>>> +       return container_of(subdev, struct bu64754, sd);
>>>> +}
>>>> +
>>>> +static int bu64754_set(struct bu64754 *bu64754, u16 position)
>>>> +{
>>>> +       int ret;
>>>> +
>>>> +       ret = cci_write(bu64754->cci, BU64754_REG_POSITION, position, NULL);
>>>> +       if (ret) {
>>>> +               dev_err(bu64754->dev, "Set position failed ret=%d\n", ret);
>>>> +               return ret;
>>>> +       }
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static int bu64754_active(struct bu64754 *bu64754)
>>>> +{
>>>> +       int ret;
>>>> +
>>>> +       /* Power on */
>>>> +       ret = cci_write(bu64754->cci, BU64754_REG_ACTIVE, BU64754_ACTIVE_MODE,
>>>> +                       NULL);
>>>> +       if (ret < 0) {
>>>> +               dev_err(bu64754->dev, "Failed to set active mode ret = %d\n",
>>>> +                       ret);
>>>> +               return ret;
>>>> +       }
>>>> +
>>>> +       /* Serve on */
>>>> +       ret = cci_write(bu64754->cci, BU64754_REG_SERVE, BU64754_SERVE_ON,
>>>> +                       NULL);
>>>> +       if (ret < 0) {
>>>> +               dev_err(bu64754->dev, "Failed to enable serve ret = %d\n",
>>>> +                       ret);
>>>> +               return ret;
>>>> +       }
>>>> +
>>>> +       return bu64754_set(bu64754, bu64754->current_val);
>>>> +}
>>>> +
>>>> +static int bu64754_standby(struct bu64754 *bu64754)
>>>> +{
>>>> +       int ret;
>>>> +
>>>> +       cci_write(bu64754->cci, BU64754_REG_ACTIVE, 0, &ret);
>>>> +       if (ret < 0)
>>>> +               dev_err(bu64754->dev, "Failed to set active mode ret = %d\n",
>>>> +                       ret);
>>>> +
>>>> +       return ret;
>>>> +}
>>>> +
>>>> +static int bu64754_regulator_event(struct notifier_block *nb,
>>>> +                                  unsigned long action, void *data)
>>>> +{
>>>> +       struct bu64754 *bu64754 = container_of(nb, struct bu64754, notifier);
>>>> +
>>>> +       if (action & REGULATOR_EVENT_ENABLE) {
>>>> +               /*
>>>> +                * Initialisation delay between VDD low->high and availability
>>>> +                * i2c operation.
>>>> +                */
>>>> +               usleep_range(BU64754_POWER_ON_DELAY,
>>>> +                            BU64754_POWER_ON_DELAY + 100);
>>>> +
>>>> +               bu64754_active(bu64754);
>>>> +       } else if (action & REGULATOR_EVENT_PRE_DISABLE) {
>>>> +               bu64754_standby(bu64754);
>>>> +       }
>>>
>>> Presumably this is based on the assumption that the same regulator
>>> controls sensor and lens, so when the sensor is powered up the lens
>>> position gets restored.
>>> I'm sure when I suggested doing the same previously it was shot down
>>> in flames ... found it [2]
>>>
>>> Personally I think it makes sense that the lens powers up
>>> automagically, and have almost exactly the same code as this in a
>>> couple of our VCM drivers, but others disagree.
>>
>> I've tested this on the Raspberry Pi, and I expect this design model
>> comes from you originally then. I thought it was a good way to handle
>> things, but I wasn't aware that others had already disagreed.
>>
>> Being a module that will connect over the RPi interface, with a single
>> POWER_EN, indeed - the regulators are shared with the camera, and
>> synchronising how it is handled seemed reasonable. I guess I have some
>> more reading to do now then.
> 
> Consider this is a driver for a VCM that may well be used in other sensors
> and so wired differently.
> 
> Is there no reset GPIO? Or another regulator? Add these, and you will start
> having circular dependencies.
> 
> In general case, the power up sequence is specific to the camera module.
> We don't have a device for the modules but I think we could for cases where
> you can't correctly execute the power on and off sequences separately for
> the sensor and the lens.
> 
> What comes to powering on the lens based on the sensor --- could you use a
> device link for that purpose?

I don't think we should try to auto-enable the VCM. Instead userspace
should open it when it needs it.

The VCM relying on the sensor to be powered up for its own power
is somewhat common.

ATM the ipu-bridge code contains a device-link in the *other* direction,
powering up the sensor when the VCM is runtime-resumed (so the model here
is that the VCM relies on the sensor to be powered up not the other way
around).

This is done so that probing the VCM, which may involve reading some
id register will work.

Under normal post-probe use I would expect userspace to always first
activate the sensor and then start to control the VCM to adjust
the focus, so then the device-link is a no-op since the sensor
is already on.

As for linking the 2 together using regulator events, please don't
do that. On e.g. ACPI platform the regulator might be hidden /
abstracted  away in _PS0 / _PS3 ACPI methods so there will be no
regulator to listen to.

The correct model here to indicate the power-dependency is
using a device-link with DL_FLAG_PM_RUNTIME set.

Regards,

Hans





> 
>>
>> I'm already thinking about making a 'simple-vcm' module or parent-driver
>> that can reduce the boilerplate for the vcms though. There's very little
>> difference between them all ... and I think they could be abstracted out
>> substantially to simplify adding new devices.
> 
> Either that, or a library to have the helpers modular. That approach could
> produce lots of very small drivers though.
>