* [PATCH 1/2] Documentation: dt-bindings: media: i2c: Add ROHM BU64754 bindings
[not found] <20230915165946.4183839-1-kieran.bingham@ideasonboard.com>
@ 2023-09-15 16:59 ` Kieran Bingham
2023-09-15 21:13 ` Conor Dooley
2023-09-17 9:37 ` Krzysztof Kozlowski
2023-09-15 16:59 ` [PATCH 2/2] media: i2c: Add ROHM BU64754 Camera Autofocus Actuator Kieran Bingham
1 sibling, 2 replies; 13+ messages in thread
From: Kieran Bingham @ 2023-09-15 16:59 UTC (permalink / raw)
To: linux-media
Cc: Kieran Bingham, Mauro Carvalho Chehab, Rob Herring,
Krzysztof Kozlowski, Conor Dooley,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/2] media: i2c: Add ROHM BU64754 Camera Autofocus Actuator
[not found] <20230915165946.4183839-1-kieran.bingham@ideasonboard.com>
2023-09-15 16:59 ` [PATCH 1/2] Documentation: dt-bindings: media: i2c: Add ROHM BU64754 bindings Kieran Bingham
@ 2023-09-15 16:59 ` Kieran Bingham
2023-09-15 17:44 ` Dave Stevenson
1 sibling, 1 reply; 13+ messages in thread
From: Kieran Bingham @ 2023-09-15 16:59 UTC (permalink / raw)
To: linux-media
Cc: Kieran Bingham, Mauro Carvalho Chehab, Liam Girdwood, Mark Brown,
Sakari Ailus, Laurent Pinchart, Hans de Goede, Andy Shevchenko,
Tomi Valkeinen, Marco Felsch, Gerald Loacker, Bingbu Cao,
Daniel Scally, Linus Walleij, open list
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] media: i2c: Add ROHM BU64754 Camera Autofocus Actuator
2023-09-15 16:59 ` [PATCH 2/2] media: i2c: Add ROHM BU64754 Camera Autofocus Actuator Kieran Bingham
@ 2023-09-15 17:44 ` Dave Stevenson
2023-09-15 19:19 ` Kieran Bingham
0 siblings, 1 reply; 13+ messages in thread
From: Dave Stevenson @ 2023-09-15 17:44 UTC (permalink / raw)
To: Kieran Bingham
Cc: linux-media, Mauro Carvalho Chehab, Liam Girdwood, Mark Brown,
Sakari Ailus, Laurent Pinchart, Hans de Goede, Andy Shevchenko,
Tomi Valkeinen, Marco Felsch, Gerald Loacker, Bingbu Cao,
Daniel Scally, Linus Walleij, open list
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
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] media: i2c: Add ROHM BU64754 Camera Autofocus Actuator
2023-09-15 17:44 ` Dave Stevenson
@ 2023-09-15 19:19 ` Kieran Bingham
2023-09-18 10:16 ` Dave Stevenson
2023-09-18 12:01 ` Sakari Ailus
0 siblings, 2 replies; 13+ messages in thread
From: Kieran Bingham @ 2023-09-15 19:19 UTC (permalink / raw)
To: Dave Stevenson
Cc: linux-media, Mauro Carvalho Chehab, Liam Girdwood, Mark Brown,
Sakari Ailus, Laurent Pinchart, Hans de Goede, Andy Shevchenko,
Tomi Valkeinen, Marco Felsch, Gerald Loacker, Bingbu Cao,
Daniel Scally, Linus Walleij, open list
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
> >
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] Documentation: dt-bindings: media: i2c: Add ROHM BU64754 bindings
2023-09-15 16:59 ` [PATCH 1/2] Documentation: dt-bindings: media: i2c: Add ROHM BU64754 bindings Kieran Bingham
@ 2023-09-15 21:13 ` Conor Dooley
2023-09-17 10:26 ` Kieran Bingham
2023-09-17 9:37 ` Krzysztof Kozlowski
1 sibling, 1 reply; 13+ messages in thread
From: Conor Dooley @ 2023-09-15 21:13 UTC (permalink / raw)
To: Kieran Bingham
Cc: linux-media, Mauro Carvalho Chehab, Rob Herring,
Krzysztof Kozlowski, Conor Dooley,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
[-- 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 --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] Documentation: dt-bindings: media: i2c: Add ROHM BU64754 bindings
2023-09-15 16:59 ` [PATCH 1/2] Documentation: dt-bindings: media: i2c: Add ROHM BU64754 bindings Kieran Bingham
2023-09-15 21:13 ` Conor Dooley
@ 2023-09-17 9:37 ` Krzysztof Kozlowski
2023-09-17 10:33 ` Kieran Bingham
1 sibling, 1 reply; 13+ messages in thread
From: Krzysztof Kozlowski @ 2023-09-17 9:37 UTC (permalink / raw)
To: Kieran Bingham, linux-media
Cc: Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
Conor Dooley,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] Documentation: dt-bindings: media: i2c: Add ROHM BU64754 bindings
2023-09-15 21:13 ` Conor Dooley
@ 2023-09-17 10:26 ` Kieran Bingham
0 siblings, 0 replies; 13+ messages in thread
From: Kieran Bingham @ 2023-09-17 10:26 UTC (permalink / raw)
To: Conor Dooley
Cc: linux-media, Mauro Carvalho Chehab, Rob Herring,
Krzysztof Kozlowski, Conor Dooley,
OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list,
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] Documentation: dt-bindings: media: i2c: Add ROHM BU64754 bindings
2023-09-17 9:37 ` Krzysztof Kozlowski
@ 2023-09-17 10:33 ` Kieran Bingham
2023-09-17 17:27 ` Krzysztof Kozlowski
0 siblings, 1 reply; 13+ messages in thread
From: Kieran Bingham @ 2023-09-17 10:33 UTC (permalink / raw)
To: Krzysztof Kozlowski, linux-media
Cc: Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list,
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
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] Documentation: dt-bindings: media: i2c: Add ROHM BU64754 bindings
2023-09-17 10:33 ` Kieran Bingham
@ 2023-09-17 17:27 ` Krzysztof Kozlowski
0 siblings, 0 replies; 13+ messages in thread
From: Krzysztof Kozlowski @ 2023-09-17 17:27 UTC (permalink / raw)
To: Kieran Bingham, linux-media
Cc: Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] media: i2c: Add ROHM BU64754 Camera Autofocus Actuator
2023-09-15 19:19 ` Kieran Bingham
@ 2023-09-18 10:16 ` Dave Stevenson
2023-09-18 10:35 ` Kieran Bingham
2023-09-18 12:01 ` Sakari Ailus
1 sibling, 1 reply; 13+ messages in thread
From: Dave Stevenson @ 2023-09-18 10:16 UTC (permalink / raw)
To: Kieran Bingham
Cc: linux-media, Mauro Carvalho Chehab, Liam Girdwood, Mark Brown,
Sakari Ailus, Laurent Pinchart, Hans de Goede, Andy Shevchenko,
Tomi Valkeinen, Marco Felsch, Gerald Loacker, Bingbu Cao,
Daniel Scally, Linus Walleij, open list
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
> > >
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] media: i2c: Add ROHM BU64754 Camera Autofocus Actuator
2023-09-18 10:16 ` Dave Stevenson
@ 2023-09-18 10:35 ` Kieran Bingham
0 siblings, 0 replies; 13+ messages in thread
From: Kieran Bingham @ 2023-09-18 10:35 UTC (permalink / raw)
To: Dave Stevenson
Cc: linux-media, Mauro Carvalho Chehab, Liam Girdwood, Mark Brown,
Sakari Ailus, Laurent Pinchart, Hans de Goede, Andy Shevchenko,
Tomi Valkeinen, Marco Felsch, Gerald Loacker, Bingbu Cao,
Daniel Scally, Linus Walleij, open list
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
> > > >
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] media: i2c: Add ROHM BU64754 Camera Autofocus Actuator
2023-09-15 19:19 ` Kieran Bingham
2023-09-18 10:16 ` Dave Stevenson
@ 2023-09-18 12:01 ` Sakari Ailus
2023-09-18 12:13 ` Hans de Goede
1 sibling, 1 reply; 13+ messages in thread
From: Sakari Ailus @ 2023-09-18 12:01 UTC (permalink / raw)
To: Kieran Bingham
Cc: Dave Stevenson, linux-media, Mauro Carvalho Chehab,
Liam Girdwood, Mark Brown, Laurent Pinchart, Hans de Goede,
Andy Shevchenko, Tomi Valkeinen, Marco Felsch, Gerald Loacker,
Bingbu Cao, Daniel Scally, Linus Walleij, open list
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] media: i2c: Add ROHM BU64754 Camera Autofocus Actuator
2023-09-18 12:01 ` Sakari Ailus
@ 2023-09-18 12:13 ` Hans de Goede
0 siblings, 0 replies; 13+ messages in thread
From: Hans de Goede @ 2023-09-18 12:13 UTC (permalink / raw)
To: Sakari Ailus, Kieran Bingham
Cc: Dave Stevenson, linux-media, Mauro Carvalho Chehab,
Liam Girdwood, Mark Brown, Laurent Pinchart, Andy Shevchenko,
Tomi Valkeinen, Marco Felsch, Gerald Loacker, Bingbu Cao,
Daniel Scally, Linus Walleij, open list
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.
>
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2023-09-18 12:15 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20230915165946.4183839-1-kieran.bingham@ideasonboard.com>
2023-09-15 16:59 ` [PATCH 1/2] Documentation: dt-bindings: media: i2c: Add ROHM BU64754 bindings Kieran Bingham
2023-09-15 21:13 ` Conor Dooley
2023-09-17 10:26 ` Kieran Bingham
2023-09-17 9:37 ` Krzysztof Kozlowski
2023-09-17 10:33 ` Kieran Bingham
2023-09-17 17:27 ` Krzysztof Kozlowski
2023-09-15 16:59 ` [PATCH 2/2] media: i2c: Add ROHM BU64754 Camera Autofocus Actuator Kieran Bingham
2023-09-15 17:44 ` Dave Stevenson
2023-09-15 19:19 ` Kieran Bingham
2023-09-18 10:16 ` Dave Stevenson
2023-09-18 10:35 ` Kieran Bingham
2023-09-18 12:01 ` Sakari Ailus
2023-09-18 12:13 ` Hans de Goede
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®