media: i2c: Add driver for AD5823 VCM

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

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/i2c/Kconfig  |   8 +
 drivers/media/i2c/Makefile |   1 +
 drivers/media/i2c/ad5823.c | 312 +++++++++++++++++++++++++++++++++++++
 3 files changed, 321 insertions(+)
 create mode 100644 drivers/media/i2c/ad5823.c

Hi Hans,

Thanks for the patch. Please see my comments below.

On Tue, Nov 05, 2024 at 09:42:46PM +0100, Hans de Goede wrote:
> Add a driver for the AD5823 VCM. The driver creates a v4l2 subdevice
> and registers a control to set the desired focus.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/media/i2c/Kconfig  |   8 +
>  drivers/media/i2c/Makefile |   1 +
>  drivers/media/i2c/ad5823.c | 312 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 321 insertions(+)
>  create mode 100644 drivers/media/i2c/ad5823.c
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 0e0c7cfdee26..dd81de88e1e2 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -763,6 +763,14 @@ config VIDEO_AD5820
>  	  This is a driver for the AD5820 camera lens voice coil.
>  	  It is used for example in Nokia N900 (RX-51).
>  
> +config VIDEO_AD5823
> +	tristate "AD5823 lens voice coil support"
> +	select V4L2_CCI_I2C
> +	help
> +	  This is a driver for the AD5823 camera lens voice coil / VCM.
> +	  This is designed for linear control of voice coil motors,
> +	  controlled via I2C serial interface.
> +
>  config VIDEO_AK7375
>  	tristate "AK7375 lens voice coil support"
>  	depends on I2C && VIDEO_DEV
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index 4ce92b8b0683..2518ac751117 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -4,6 +4,7 @@ msp3400-objs	:=	msp3400-driver.o msp3400-kthreads.o
>  
>  obj-$(CONFIG_SDR_MAX2175) += max2175.o
>  obj-$(CONFIG_VIDEO_AD5820) += ad5820.o
> +obj-$(CONFIG_VIDEO_AD5823) += ad5823.o
>  obj-$(CONFIG_VIDEO_ADP1653) += adp1653.o
>  obj-$(CONFIG_VIDEO_ADV7170) += adv7170.o
>  obj-$(CONFIG_VIDEO_ADV7175) += adv7175.o
> diff --git a/drivers/media/i2c/ad5823.c b/drivers/media/i2c/ad5823.c
> new file mode 100644
> index 000000000000..96e7ff3a8583
> --- /dev/null
> +++ b/drivers/media/i2c/ad5823.c
> @@ -0,0 +1,312 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Analog Devices AD5823 VCM driver
> + * Copyright 2023 - 2024 Hans de Goede <hansg@kernel.org>
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/types.h>
> +
> +#include <media/v4l2-cci.h>
> +#include <media/v4l2-common.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-subdev.h>
> +
> +#define AD5823_MAX_FOCUS_POS		1023
> +
> +#define AD5823_RESET			CCI_REG8(1)
> +#define AD5823_RESET_RESET		BIT(0)
> +
> +#define AD5823_MODE			CCI_REG8(2)
> +#define AD5823_ARC_RES1			0x01
> +
> +#define AD5823_VCM_MOVE_TIME		CCI_REG8(3)
> +#define AD5823_VCM_MOVE_TIME_DEFAULT	0x80
> +#define AD5823_RESONANCE_PERIOD		100000	/* in 0.1 us units */
> +#define AD5823_RESONANCE_COEF		512	/* in 0.1 us units */
> +
> +#define AD5823_RESONANCE_OFFSET		0x80	/* for reg 0x02 bit 5 == 0 */
> +
> +#define AD5823_VCM_CODE			CCI_REG16(4)
> +#define AD5823_VCM_CODE_RING_CTRL	BIT(10)
> +
> +#define AD5823_VCM_THRESHOLD		CCI_REG16(6)
> +#define AD5823_VCM_THRESHOLD_DEFAULT	0x10
> +
> +#define to_ad5823_device(x) container_of(x, struct ad5823_device, sd)

container_of_const()?

> +
> +struct ad5823_device {
> +	struct v4l2_subdev sd;
> +	struct regmap *regmap;
> +	struct regulator *regulator;
> +	u32 arc_mode;
> +	u32 resonance_period;	/* in 0.1 us units */
> +
> +	struct ad5823_v4l2_ctrls {
> +		struct v4l2_ctrl_handler handler;
> +		struct v4l2_ctrl *focus;
> +	} ctrls;
> +};
> +
> +static int ad5823_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct ad5823_device *ad5823 = container_of(ctrl->handler,
> +						    struct ad5823_device,
> +						    ctrls.handler);

Ditto.

> +	int ret;
> +
> +	/* Only apply changes to the controls if the device is powered up */
> +	if (!pm_runtime_get_if_in_use(ad5823->sd.dev))

I'd suggest to use pm_runtime_get_if_active() as it also proceeds when
there are no users but the device is still powered on. Otherwise setting
focus won't be updated to the device.

> +		return 0;
> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_FOCUS_ABSOLUTE:
> +		ret = cci_write(ad5823->regmap, AD5823_VCM_CODE,
> +				AD5823_VCM_CODE_RING_CTRL | ctrl->val, NULL);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	pm_runtime_put(ad5823->sd.dev);
> +	return ret;
> +}
> +
> +static const struct v4l2_ctrl_ops ad5823_ctrl_ops = {
> +	.s_ctrl = ad5823_set_ctrl,
> +};
> +
> +static int ad5823_power_down(struct ad5823_device *ad5823)
> +{
> +	return regulator_disable(ad5823->regulator);
> +}
> +
> +static int ad5823_power_up(struct ad5823_device *ad5823, bool detect)
> +{
> +	u64 vcm_move_time, vcm_threshold;
> +	int ret;
> +
> +	ret = regulator_enable(ad5823->regulator);
> +	if (ret)
> +		return ret;
> +

Is there no power-up delay at all? Even the regulator supplied voltage may
take some time to rise.

> +	cci_write(ad5823->regmap, AD5823_RESET, BIT(0), &ret);
> +
> +	if (detect) {
> +		/* There is no id register, check for default reg values. */
> +		cci_read(ad5823->regmap, AD5823_VCM_MOVE_TIME, &vcm_move_time, &ret);
> +		cci_read(ad5823->regmap, AD5823_VCM_THRESHOLD, &vcm_threshold, &ret);
> +
> +		if (!ret && (vcm_move_time != AD5823_VCM_MOVE_TIME_DEFAULT ||
> +			     vcm_threshold != AD5823_VCM_THRESHOLD_DEFAULT)) {
> +			dev_err(ad5823->sd.dev, "Failed to detect AD5823 got move-time 0x%02llx vcm-threshold 0x%02llx\n",
> +				vcm_move_time, vcm_threshold);
> +			ret = -ENXIO;
> +		}
> +	}
> +
> +	vcm_move_time = ad5823->resonance_period / AD5823_RESONANCE_COEF -
> +			AD5823_RESONANCE_OFFSET;
> +
> +	dev_dbg(ad5823->sd.dev, "mode 0x%02x move-time 0x%02llx\n",
> +		ad5823->arc_mode, vcm_move_time);
> +
> +	cci_write(ad5823->regmap, AD5823_MODE, ad5823->arc_mode, &ret);
> +	cci_write(ad5823->regmap, AD5823_VCM_MOVE_TIME, vcm_move_time, &ret);
> +	if (ret)
> +		ad5823_power_down(ad5823);
> +
> +	return ret;
> +}
> +
> +static int ad5823_suspend(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct ad5823_device *ad5823 = to_ad5823_device(sd);
> +
> +	return ad5823_power_down(ad5823);
> +}
> +
> +static int ad5823_resume(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct ad5823_device *ad5823 = to_ad5823_device(sd);
> +	int ret;
> +
> +	ret = ad5823_power_up(ad5823, false);
> +	if (ret)
> +		return ret;
> +
> +	/* Restore value of ctrls */
> +	ret = v4l2_ctrl_handler_setup(&ad5823->ctrls.handler);
> +	if (ret < 0)
> +		dev_warn(dev, "Failed to restore focus ctrl value: %d\n", ret);

Shouldn't resume fail in this case? The register writes in power_up()
function will have that effect.

> +
> +	return 0;
> +}

See comment in probe(), too.

I'd try to with a single set of power on/off functions. Using the device as
an argument is mandatory of course.

> +
> +static int ad5823_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> +{
> +	return pm_runtime_resume_and_get(sd->dev);
> +}
> +
> +static int ad5823_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> +{
> +	pm_runtime_put(sd->dev);
> +	return 0;
> +}
> +
> +static const struct v4l2_subdev_internal_ops ad5823_internal_ops = {
> +	.open = ad5823_open,
> +	.close = ad5823_close,
> +};
> +
> +static const struct v4l2_subdev_ops ad5823_ops = { };
> +
> +static int ad5823_init_controls(struct ad5823_device *ad5823)
> +{
> +	const struct v4l2_ctrl_ops *ops = &ad5823_ctrl_ops;
> +	int ret;
> +
> +	v4l2_ctrl_handler_init(&ad5823->ctrls.handler, 1);
> +
> +	ad5823->ctrls.focus = v4l2_ctrl_new_std(&ad5823->ctrls.handler, ops,
> +						V4L2_CID_FOCUS_ABSOLUTE, 0,
> +						AD5823_MAX_FOCUS_POS, 1, 0);
> +
> +	if (ad5823->ctrls.handler.error) {
> +		dev_err(ad5823->sd.dev, "Error initialising v4l2 ctrls\n");
> +		ret = ad5823->ctrls.handler.error;
> +		goto err_free_handler;
> +	}
> +
> +	ad5823->sd.ctrl_handler = &ad5823->ctrls.handler;
> +	return 0;
> +
> +err_free_handler:
> +	v4l2_ctrl_handler_free(&ad5823->ctrls.handler);
> +	return ret;
> +}
> +
> +static int ad5823_probe(struct i2c_client *client)
> +{
> +	struct ad5823_device *ad5823;
> +	int ret;
> +
> +	ad5823 = devm_kzalloc(&client->dev, sizeof(*ad5823), GFP_KERNEL);
> +	if (!ad5823)
> +		return -ENOMEM;
> +
> +	ad5823->regmap = devm_cci_regmap_init_i2c(client, 8);
> +	if (IS_ERR(ad5823->regmap))
> +		return PTR_ERR(ad5823->regmap);
> +
> +	ad5823->arc_mode = AD5823_ARC_RES1;
> +	ad5823->resonance_period = AD5823_RESONANCE_PERIOD;
> +
> +	/* Optional indication of ARC mode select */
> +	device_property_read_u32(&client->dev, "adi,arc-mode",
> +				 &ad5823->arc_mode);
> +
> +	/* Optional indication of VCM resonance period */
> +	device_property_read_u32(&client->dev, "adi,resonance-period",
> +				 &ad5823->resonance_period);
> +
> +	ad5823->regulator = devm_regulator_get(&client->dev, "vdd");
> +	if (IS_ERR(ad5823->regulator))
> +		return dev_err_probe(&client->dev, PTR_ERR(ad5823->regulator),
> +				     "getting regulator\n");
> +
> +	v4l2_i2c_subdev_init(&ad5823->sd, client, &ad5823_ops);
> +	ad5823->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	ad5823->sd.internal_ops = &ad5823_internal_ops;
> +
> +	ret = ad5823_init_controls(ad5823);
> +	if (ret)
> +		return ret;
> +
> +	ret = media_entity_pads_init(&ad5823->sd.entity, 0, NULL);
> +	if (ret < 0)
> +		goto err_free_ctrl_handler;
> +
> +	ad5823->sd.entity.function = MEDIA_ENT_F_LENS;
> +
> +	/*
> +	 * We need the driver to work in the event that pm runtime is disable in
> +	 * the kernel, so power up and verify the chip now. In the event that
> +	 * runtime pm is disabled this will leave the chip on, so that the lens
> +	 * will work.
> +	 */
> +
> +	ret = ad5823_power_up(ad5823, true);

The control handler setup should be run here, too.

> +	if (ret)
> +		goto err_cleanup_media;
> +
> +	pm_runtime_set_active(&client->dev);
> +	pm_runtime_get_noresume(&client->dev);
> +	pm_runtime_enable(&client->dev);
> +
> +	ret = v4l2_async_register_subdev(&ad5823->sd);
> +	if (ret < 0)
> +		goto err_pm_runtime;
> +
> +	pm_runtime_set_autosuspend_delay(&client->dev, 1000);
> +	pm_runtime_use_autosuspend(&client->dev);
> +	pm_runtime_put_autosuspend(&client->dev);
> +
> +	return ret;
> +
> +err_pm_runtime:
> +	pm_runtime_disable(&client->dev);
> +	pm_runtime_put_noidle(&client->dev);
> +	ad5823_power_down(ad5823);
> +err_cleanup_media:
> +	media_entity_cleanup(&ad5823->sd.entity);
> +err_free_ctrl_handler:
> +	v4l2_ctrl_handler_free(&ad5823->ctrls.handler);
> +
> +	return ret;
> +}
> +
> +static void ad5823_remove(struct i2c_client *client)
> +{
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct ad5823_device *ad5823 =
> +		container_of(sd, struct ad5823_device, sd);

		to_ad5823_device(...);

> +
> +	v4l2_async_unregister_subdev(sd);
> +	v4l2_ctrl_handler_free(&ad5823->ctrls.handler);
> +	media_entity_cleanup(&ad5823->sd.entity);
> +
> +	pm_runtime_disable(&client->dev);
> +	if (!pm_runtime_status_suspended(&client->dev))
> +		ad5823_power_down(ad5823);
> +	pm_runtime_set_suspended(&client->dev);
> +}
> +
> +static const struct i2c_device_id ad5823_id_table[] = {
> +	{ "ad5823" },

Where is the device used, in a DT or ACPI based system? Or is this to be
used with the IPU bridge?

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, ad5823_id_table);
> +
> +static DEFINE_RUNTIME_DEV_PM_OPS(ad5823_pm_ops, ad5823_suspend, ad5823_resume,
> +				 NULL);
> +
> +static struct i2c_driver ad5823_i2c_driver = {
> +	.driver = {
> +		.name = "ad5823",
> +		.pm = pm_sleep_ptr(&ad5823_pm_ops),
> +	},
> +	.probe = ad5823_probe,
> +	.remove = ad5823_remove,
> +	.id_table = ad5823_id_table,
> +};
> +module_i2c_driver(ad5823_i2c_driver);
> +
> +MODULE_AUTHOR("Hans de Goede <hansg@kernel.org>");
> +MODULE_DESCRIPTION("AD5823 VCM Driver");
> +MODULE_LICENSE("GPL");