[PATCH 0/2] media: v4l: Call s_stream() on VCM when it is called on the associated sensor

[PATCH 0/2] media: v4l: Call s_stream() on VCM when it is called on the associated sensor

[Hans de Goede]

Hi All,

The first patch in this series addresses a long standing issue with VCM
device power-management where merely opening the /dev node causes
the VCM to power on. This issue was recently brought up on the libcamera-
devel list and this is my attempt at fixing this.

The second patch is a new VCM driver which I have had in my personal tree
for quite a while now, but finishing it was blocked on the power-management
issue. This new driver shows how VCM runtime-pm can work after patch 1.

Regards,

Hans


Hans de Goede (2):
  media: v4l: Call s_stream() on VCM when it is called on the associated
    sensor
  media: i2c: Add driver for AD5823 VCM

 drivers/media/i2c/Kconfig             |   5 +
 drivers/media/i2c/Makefile            |   1 +
 drivers/media/i2c/ad5823.c            | 311 ++++++++++++++++++++++++++
 drivers/media/v4l2-core/v4l2-async.c  |  20 ++
 drivers/media/v4l2-core/v4l2-subdev.c |  28 ++-
 include/media/v4l2-subdev.h           |   2 +
 6 files changed, 361 insertions(+), 6 deletions(-)
 create mode 100644 drivers/media/i2c/ad5823.c

-- 
2.46.0

[PATCH 1/2] media: v4l: Call s_stream() on VCM when it is called on the associated sensor

[Hans de Goede]

Currently VCM drivers power-up the VCM as soon as the VCM's /dev node
is opened and through the runtime-pm device-link to the sensor this
also powers up the sensor.

Powering up the VCM and sensor when the /dev node is opened is undesirable,
without a VCM sensors delay powering up until s_stream(1) is called. This
allows querying / negotiating capabilities without powering things up.

Sometimes a long running daemon like pipewire may keep the /dev node open
all the time. The kernel should still be able to powerdown the VCM + sensor
in this scenario.

VCM drivers should be able to do the same as sensor drivers and only
power-up the VCM when s_stream(1) is called on the VCM subdev, but this
requires that s_stream() is actually called on the VCM when the sensor
starts / stops streaming.

The s_stream() call on sensor subdevs is done by CSI-receiver/ISP drivers.
To avoid needing to change all these call sites to also call s_stream()
on the VCM (if there is one) handle the VCM in the v4l2-core similar to how
the core takes care of turning on/off the privacy LED.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/v4l2-core/v4l2-async.c  | 20 +++++++++++++++++++
 drivers/media/v4l2-core/v4l2-subdev.c | 28 +++++++++++++++++++++------
 include/media/v4l2-subdev.h           |  2 ++
 3 files changed, 44 insertions(+), 6 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index ee884a8221fb..9b854f1d1051 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -330,6 +330,11 @@ static int v4l2_async_create_ancillary_links(struct v4l2_async_notifier *n,
 		return 0;
 	}
 
+	if (sd->entity.function == MEDIA_ENT_F_LENS) {
+		dev_dbg(n->sd->dev, "Using %s VCM\n", dev_name(sd->dev));
+		n->sd->vcm = sd;
+	}
+
 	link = media_create_ancillary_link(&n->sd->entity, &sd->entity);
 
 	return IS_ERR(link) ? PTR_ERR(link) : 0;
@@ -871,6 +876,21 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
 	if (!sd->async_list.next)
 		return;
 
+#if defined(CONFIG_MEDIA_CONTROLLER)
+	if (sd->entity.function == MEDIA_ENT_F_LENS && sd->v4l2_dev && sd->v4l2_dev->mdev) {
+		struct media_entity *entity;
+
+		media_device_for_each_entity(entity, sd->v4l2_dev->mdev) {
+			struct v4l2_subdev *it = media_entity_to_v4l2_subdev(entity);
+
+			if (it->vcm == sd) {
+				dev_dbg(it->dev, "Clearing VCM\n");
+				it->vcm = NULL;
+			}
+		}
+	}
+#endif
+
 	v4l2_subdev_put_privacy_led(sd);
 
 	mutex_lock(&list_lock);
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 7c5812d55315..24a68d90f686 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -148,17 +148,33 @@ static int subdev_close(struct file *file)
 }
 #endif /* CONFIG_VIDEO_V4L2_SUBDEV_API */
 
-static void v4l2_subdev_enable_privacy_led(struct v4l2_subdev *sd)
+static void v4l2_subdev_enable_privacy_led_and_vcm(struct v4l2_subdev *sd)
 {
 #if IS_REACHABLE(CONFIG_LEDS_CLASS)
 	if (!IS_ERR_OR_NULL(sd->privacy_led))
 		led_set_brightness(sd->privacy_led,
 				   sd->privacy_led->max_brightness);
 #endif
+
+	if (sd->vcm && !sd->vcm_enabled &&
+	    v4l2_subdev_has_op(sd->vcm, video, s_stream)) {
+		int ret;
+
+		ret = v4l2_subdev_call(sd->vcm, video, s_stream, 1);
+		if (ret)
+			dev_err(sd->vcm->dev, "Error powering on VCM: %d\n", ret);
+		else
+			sd->vcm_enabled = true;
+	}
 }
 
-static void v4l2_subdev_disable_privacy_led(struct v4l2_subdev *sd)
+static void v4l2_subdev_disable_privacy_led_and_vcm(struct v4l2_subdev *sd)
 {
+	if (sd->vcm && sd->vcm_enabled) {
+		v4l2_subdev_call(sd->vcm, video, s_stream, 0);
+		sd->vcm_enabled = false;
+	}
+
 #if IS_REACHABLE(CONFIG_LEDS_CLASS)
 	if (!IS_ERR_OR_NULL(sd->privacy_led))
 		led_set_brightness(sd->privacy_led, 0);
@@ -466,9 +482,9 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
 		sd->s_stream_enabled = enable;
 
 		if (enable)
-			v4l2_subdev_enable_privacy_led(sd);
+			v4l2_subdev_enable_privacy_led_and_vcm(sd);
 		else
-			v4l2_subdev_disable_privacy_led(sd);
+			v4l2_subdev_disable_privacy_led_and_vcm(sd);
 	}
 
 	return ret;
@@ -2289,7 +2305,7 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
 	 * for all cases.
 	 */
 	if (!use_s_stream && !already_streaming)
-		v4l2_subdev_enable_privacy_led(sd);
+		v4l2_subdev_enable_privacy_led_and_vcm(sd);
 
 done:
 	if (!use_s_stream)
@@ -2382,7 +2398,7 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
 done:
 	if (!use_s_stream) {
 		if (!v4l2_subdev_is_streaming(sd))
-			v4l2_subdev_disable_privacy_led(sd);
+			v4l2_subdev_disable_privacy_led_and_vcm(sd);
 
 		v4l2_subdev_unlock_state(state);
 	}
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index bd235d325ff9..6568a0cc070b 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -1094,6 +1094,7 @@ struct v4l2_subdev {
 	 */
 
 	struct led_classdev *privacy_led;
+	struct v4l2_subdev *vcm;
 
 	/*
 	 * TODO: active_state should most likely be changed from a pointer to an
@@ -1104,6 +1105,7 @@ struct v4l2_subdev {
 	struct v4l2_subdev_state *active_state;
 	u64 enabled_pads;
 	bool s_stream_enabled;
+	bool vcm_enabled;
 };
 
 
-- 
2.46.0

Re: [PATCH 1/2] media: v4l: Call s_stream() on VCM when it is called on the associated sensor

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Sun, Sep 01, 2024 at 11:18:33PM +0200, Hans de Goede wrote:
> Currently VCM drivers power-up the VCM as soon as the VCM's /dev node
> is opened and through the runtime-pm device-link to the sensor this
> also powers up the sensor.
> 
> Powering up the VCM and sensor when the /dev node is opened is undesirable,
> without a VCM sensors delay powering up until s_stream(1) is called. This
> allows querying / negotiating capabilities without powering things up.
> 
> Sometimes a long running daemon like pipewire may keep the /dev node open
> all the time. The kernel should still be able to powerdown the VCM + sensor
> in this scenario.
> 
> VCM drivers should be able to do the same as sensor drivers and only
> power-up the VCM when s_stream(1) is called on the VCM subdev, but this
> requires that s_stream() is actually called on the VCM when the sensor
> starts / stops streaming.

.s_stream() doesn't conceptually make sense for VCMs. Furthermore,
.s_stream() is being replaced with .enable_streams() and
.disable_streams(), which will make even less sense. We need a different
API.

> The s_stream() call on sensor subdevs is done by CSI-receiver/ISP drivers.
> To avoid needing to change all these call sites to also call s_stream()
> on the VCM (if there is one) handle the VCM in the v4l2-core similar to how
> the core takes care of turning on/off the privacy LED.

This needs to come with a design rationale, documented in
Documentation/. The design needs to explain the use cases. Lens motion
may take time, which I expect will influence how we will need to handle
power management.

I'm not very comfortable handling this in v4l2-subdev.c, it seems that
we'll hardcode use cases. Without a clear and detailed designed
rationale, this patch feels we'll paint ourselves in a corner. We have
enough badly designed (or not designed at all) APIs for cameras, it's
time to do better.

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/media/v4l2-core/v4l2-async.c  | 20 +++++++++++++++++++
>  drivers/media/v4l2-core/v4l2-subdev.c | 28 +++++++++++++++++++++------
>  include/media/v4l2-subdev.h           |  2 ++
>  3 files changed, 44 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index ee884a8221fb..9b854f1d1051 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -330,6 +330,11 @@ static int v4l2_async_create_ancillary_links(struct v4l2_async_notifier *n,
>  		return 0;
>  	}
>  
> +	if (sd->entity.function == MEDIA_ENT_F_LENS) {
> +		dev_dbg(n->sd->dev, "Using %s VCM\n", dev_name(sd->dev));
> +		n->sd->vcm = sd;
> +	}
> +
>  	link = media_create_ancillary_link(&n->sd->entity, &sd->entity);
>  
>  	return IS_ERR(link) ? PTR_ERR(link) : 0;
> @@ -871,6 +876,21 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
>  	if (!sd->async_list.next)
>  		return;
>  
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> +	if (sd->entity.function == MEDIA_ENT_F_LENS && sd->v4l2_dev && sd->v4l2_dev->mdev) {
> +		struct media_entity *entity;
> +
> +		media_device_for_each_entity(entity, sd->v4l2_dev->mdev) {
> +			struct v4l2_subdev *it = media_entity_to_v4l2_subdev(entity);
> +
> +			if (it->vcm == sd) {
> +				dev_dbg(it->dev, "Clearing VCM\n");
> +				it->vcm = NULL;
> +			}
> +		}
> +	}
> +#endif
> +
>  	v4l2_subdev_put_privacy_led(sd);
>  
>  	mutex_lock(&list_lock);
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 7c5812d55315..24a68d90f686 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -148,17 +148,33 @@ static int subdev_close(struct file *file)
>  }
>  #endif /* CONFIG_VIDEO_V4L2_SUBDEV_API */
>  
> -static void v4l2_subdev_enable_privacy_led(struct v4l2_subdev *sd)
> +static void v4l2_subdev_enable_privacy_led_and_vcm(struct v4l2_subdev *sd)
>  {
>  #if IS_REACHABLE(CONFIG_LEDS_CLASS)
>  	if (!IS_ERR_OR_NULL(sd->privacy_led))
>  		led_set_brightness(sd->privacy_led,
>  				   sd->privacy_led->max_brightness);
>  #endif
> +
> +	if (sd->vcm && !sd->vcm_enabled &&
> +	    v4l2_subdev_has_op(sd->vcm, video, s_stream)) {
> +		int ret;
> +
> +		ret = v4l2_subdev_call(sd->vcm, video, s_stream, 1);
> +		if (ret)
> +			dev_err(sd->vcm->dev, "Error powering on VCM: %d\n", ret);
> +		else
> +			sd->vcm_enabled = true;
> +	}
>  }
>  
> -static void v4l2_subdev_disable_privacy_led(struct v4l2_subdev *sd)
> +static void v4l2_subdev_disable_privacy_led_and_vcm(struct v4l2_subdev *sd)
>  {
> +	if (sd->vcm && sd->vcm_enabled) {
> +		v4l2_subdev_call(sd->vcm, video, s_stream, 0);
> +		sd->vcm_enabled = false;
> +	}
> +
>  #if IS_REACHABLE(CONFIG_LEDS_CLASS)
>  	if (!IS_ERR_OR_NULL(sd->privacy_led))
>  		led_set_brightness(sd->privacy_led, 0);
> @@ -466,9 +482,9 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
>  		sd->s_stream_enabled = enable;
>  
>  		if (enable)
> -			v4l2_subdev_enable_privacy_led(sd);
> +			v4l2_subdev_enable_privacy_led_and_vcm(sd);
>  		else
> -			v4l2_subdev_disable_privacy_led(sd);
> +			v4l2_subdev_disable_privacy_led_and_vcm(sd);
>  	}
>  
>  	return ret;
> @@ -2289,7 +2305,7 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
>  	 * for all cases.
>  	 */
>  	if (!use_s_stream && !already_streaming)
> -		v4l2_subdev_enable_privacy_led(sd);
> +		v4l2_subdev_enable_privacy_led_and_vcm(sd);
>  
>  done:
>  	if (!use_s_stream)
> @@ -2382,7 +2398,7 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
>  done:
>  	if (!use_s_stream) {
>  		if (!v4l2_subdev_is_streaming(sd))
> -			v4l2_subdev_disable_privacy_led(sd);
> +			v4l2_subdev_disable_privacy_led_and_vcm(sd);
>  
>  		v4l2_subdev_unlock_state(state);
>  	}
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index bd235d325ff9..6568a0cc070b 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -1094,6 +1094,7 @@ struct v4l2_subdev {
>  	 */
>  
>  	struct led_classdev *privacy_led;
> +	struct v4l2_subdev *vcm;
>  
>  	/*
>  	 * TODO: active_state should most likely be changed from a pointer to an
> @@ -1104,6 +1105,7 @@ struct v4l2_subdev {
>  	struct v4l2_subdev_state *active_state;
>  	u64 enabled_pads;
>  	bool s_stream_enabled;
> +	bool vcm_enabled;
>  };
>  
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/2] media: v4l: Call s_stream() on VCM when it is called on the associated sensor

[Hans de Goede]

Hi Laurent,

On 1-Sep-24 11:28 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Sun, Sep 01, 2024 at 11:18:33PM +0200, Hans de Goede wrote:
>> Currently VCM drivers power-up the VCM as soon as the VCM's /dev node
>> is opened and through the runtime-pm device-link to the sensor this
>> also powers up the sensor.
>>
>> Powering up the VCM and sensor when the /dev node is opened is undesirable,
>> without a VCM sensors delay powering up until s_stream(1) is called. This
>> allows querying / negotiating capabilities without powering things up.
>>
>> Sometimes a long running daemon like pipewire may keep the /dev node open
>> all the time. The kernel should still be able to powerdown the VCM + sensor
>> in this scenario.
>>
>> VCM drivers should be able to do the same as sensor drivers and only
>> power-up the VCM when s_stream(1) is called on the VCM subdev, but this
>> requires that s_stream() is actually called on the VCM when the sensor
>> starts / stops streaming.
> 
> .s_stream() doesn't conceptually make sense for VCMs. Furthermore,
> .s_stream() is being replaced with .enable_streams() and
> .disable_streams(), which will make even less sense. We need a different
> API.

We can discuss how to call the subdev ops for this once we know
what the overal design for this is going to be. So lets postpone
the discussion about naming the subdev ops for this till later.

>> The s_stream() call on sensor subdevs is done by CSI-receiver/ISP drivers.
>> To avoid needing to change all these call sites to also call s_stream()
>> on the VCM (if there is one) handle the VCM in the v4l2-core similar to how
>> the core takes care of turning on/off the privacy LED.
> 
> This needs to come with a design rationale, documented in
> Documentation/. The design needs to explain the use cases. Lens motion
> may take time, which I expect will influence how we will need to handle
> power management.
> 
> I'm not very comfortable handling this in v4l2-subdev.c, it seems that
> we'll hardcode use cases. Without a clear and detailed designed
> rationale, this patch feels we'll paint ourselves in a corner. We have
> enough badly designed (or not designed at all) APIs for cameras, it's
> time to do better.

Design wise I roughly see 3 options:

a. Stick with the current design of powering on on open of /dev/v4l-subdev

b. Add explicit calls userspace can make to power-on the device

c. My current proposal to automatically power-on when the sensor associated
with the VCM.

Since this patch series implements 3, I'm going to discuss that option
here now. But if that hits a dead end we can look at 2. too.

Talking about use-cases, lets simplify things by looking at use-cases
solely from a VCM power on/off point of view. Looking at it that way
I really only see 2 orthogonal groupings for all use-cases (so 4
possible combinations of groupings, but I think we can discuss each
of the 2 axis separately):

Grouping i: amount of sensor stream start/stops in a single "use"

1. The sensor starts streaming once and then the stream stops for
a significant period of time; vs

2. The stream is sometimes stopped to change settings and then
restarted within a single use-case.

For 1. automatically powering on the VCM on stream start is fine.
For 2. we do not want the VCM to turn off and then back on again
while changing settings. We can use auto-suspend with a long enough
auto-suspend delay (we could default to 1s) for this. There might
be some special cases where we need to change the auto-suspend delay.
We could add some V4L2 API (ctrl?) for this, or use the existing
sysfs API through e.g. a udev rule.

To me using autosuspend-delays here seems workable and having
the v4l2-subdev code manage the power has the advantage that it does
not break any userspace APIs. IOW things will just work while
saving power for existing users and things will also just work
for new use-cases without requiring userspace to have to do
extra work for this.

Grouping ii: Does the VCM need to be turned on before the stream
starts:

3. A small delay at startup for the power-on (on the first start
stream, not when switching modes) is fine /  having the first few
frames being out of focus while the lens moves is fine, they typically
will be anyways until the autofocus algorithm has locked; vs

4. A small delay at startup for the power-on is undesirable /
the focus value is known beforehand and the lens must be in position
before capturing the first frame.

4. will clearly not work well when the v4l2-subdev code manage
the power. But this seems like a rather corner case scenario; and
one which we could still make work by simply disabling runtime-pm
for the VCM through sysfs in this case.

In conclusion to me it seems that having the v4l2-subdev code
manage the power seems like it would work well. The only really
problematic scenario which I can come up with is 4. and that seems
a bit of a corner case. If this ever becomes a problem we could
can make it work by either disabling runtime-pm through sysfs or
by offering some API to prime the VCM which turns it on before
the first stream-start (and it will still get automatically turned
off after stream-stop + timeout).

We could even mix option b) and c) from the "Design wise I roughly
see 3 options" options above and offer explicit power-management
for apps which want that; while still automatically increasing
the power-on count on sensor streaming, with any power-on
count > 1 turning the VCM on.

This way apps/use-cases which want to make sure the VCM is on
early or stays on during mode switching can use the explicit APIs
while simpler use-cases (and existing use-cases since we cannot
break userspace API) can rely on the automatic powering on done
by the v4l2-subdev core.

The only downside which I can see of using a power-on counter
and combining explicit + automatic control is that it will be
impossible to turn the VCM off while streaming.

Regards,

Hans






> 
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/media/v4l2-core/v4l2-async.c  | 20 +++++++++++++++++++
>>  drivers/media/v4l2-core/v4l2-subdev.c | 28 +++++++++++++++++++++------
>>  include/media/v4l2-subdev.h           |  2 ++
>>  3 files changed, 44 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
>> index ee884a8221fb..9b854f1d1051 100644
>> --- a/drivers/media/v4l2-core/v4l2-async.c
>> +++ b/drivers/media/v4l2-core/v4l2-async.c
>> @@ -330,6 +330,11 @@ static int v4l2_async_create_ancillary_links(struct v4l2_async_notifier *n,
>>  		return 0;
>>  	}
>>  
>> +	if (sd->entity.function == MEDIA_ENT_F_LENS) {
>> +		dev_dbg(n->sd->dev, "Using %s VCM\n", dev_name(sd->dev));
>> +		n->sd->vcm = sd;
>> +	}
>> +
>>  	link = media_create_ancillary_link(&n->sd->entity, &sd->entity);
>>  
>>  	return IS_ERR(link) ? PTR_ERR(link) : 0;
>> @@ -871,6 +876,21 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
>>  	if (!sd->async_list.next)
>>  		return;
>>  
>> +#if defined(CONFIG_MEDIA_CONTROLLER)
>> +	if (sd->entity.function == MEDIA_ENT_F_LENS && sd->v4l2_dev && sd->v4l2_dev->mdev) {
>> +		struct media_entity *entity;
>> +
>> +		media_device_for_each_entity(entity, sd->v4l2_dev->mdev) {
>> +			struct v4l2_subdev *it = media_entity_to_v4l2_subdev(entity);
>> +
>> +			if (it->vcm == sd) {
>> +				dev_dbg(it->dev, "Clearing VCM\n");
>> +				it->vcm = NULL;
>> +			}
>> +		}
>> +	}
>> +#endif
>> +
>>  	v4l2_subdev_put_privacy_led(sd);
>>  
>>  	mutex_lock(&list_lock);
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>> index 7c5812d55315..24a68d90f686 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -148,17 +148,33 @@ static int subdev_close(struct file *file)
>>  }
>>  #endif /* CONFIG_VIDEO_V4L2_SUBDEV_API */
>>  
>> -static void v4l2_subdev_enable_privacy_led(struct v4l2_subdev *sd)
>> +static void v4l2_subdev_enable_privacy_led_and_vcm(struct v4l2_subdev *sd)
>>  {
>>  #if IS_REACHABLE(CONFIG_LEDS_CLASS)
>>  	if (!IS_ERR_OR_NULL(sd->privacy_led))
>>  		led_set_brightness(sd->privacy_led,
>>  				   sd->privacy_led->max_brightness);
>>  #endif
>> +
>> +	if (sd->vcm && !sd->vcm_enabled &&
>> +	    v4l2_subdev_has_op(sd->vcm, video, s_stream)) {
>> +		int ret;
>> +
>> +		ret = v4l2_subdev_call(sd->vcm, video, s_stream, 1);
>> +		if (ret)
>> +			dev_err(sd->vcm->dev, "Error powering on VCM: %d\n", ret);
>> +		else
>> +			sd->vcm_enabled = true;
>> +	}
>>  }
>>  
>> -static void v4l2_subdev_disable_privacy_led(struct v4l2_subdev *sd)
>> +static void v4l2_subdev_disable_privacy_led_and_vcm(struct v4l2_subdev *sd)
>>  {
>> +	if (sd->vcm && sd->vcm_enabled) {
>> +		v4l2_subdev_call(sd->vcm, video, s_stream, 0);
>> +		sd->vcm_enabled = false;
>> +	}
>> +
>>  #if IS_REACHABLE(CONFIG_LEDS_CLASS)
>>  	if (!IS_ERR_OR_NULL(sd->privacy_led))
>>  		led_set_brightness(sd->privacy_led, 0);
>> @@ -466,9 +482,9 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
>>  		sd->s_stream_enabled = enable;
>>  
>>  		if (enable)
>> -			v4l2_subdev_enable_privacy_led(sd);
>> +			v4l2_subdev_enable_privacy_led_and_vcm(sd);
>>  		else
>> -			v4l2_subdev_disable_privacy_led(sd);
>> +			v4l2_subdev_disable_privacy_led_and_vcm(sd);
>>  	}
>>  
>>  	return ret;
>> @@ -2289,7 +2305,7 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
>>  	 * for all cases.
>>  	 */
>>  	if (!use_s_stream && !already_streaming)
>> -		v4l2_subdev_enable_privacy_led(sd);
>> +		v4l2_subdev_enable_privacy_led_and_vcm(sd);
>>  
>>  done:
>>  	if (!use_s_stream)
>> @@ -2382,7 +2398,7 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
>>  done:
>>  	if (!use_s_stream) {
>>  		if (!v4l2_subdev_is_streaming(sd))
>> -			v4l2_subdev_disable_privacy_led(sd);
>> +			v4l2_subdev_disable_privacy_led_and_vcm(sd);
>>  
>>  		v4l2_subdev_unlock_state(state);
>>  	}
>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>> index bd235d325ff9..6568a0cc070b 100644
>> --- a/include/media/v4l2-subdev.h
>> +++ b/include/media/v4l2-subdev.h
>> @@ -1094,6 +1094,7 @@ struct v4l2_subdev {
>>  	 */
>>  
>>  	struct led_classdev *privacy_led;
>> +	struct v4l2_subdev *vcm;
>>  
>>  	/*
>>  	 * TODO: active_state should most likely be changed from a pointer to an
>> @@ -1104,6 +1105,7 @@ struct v4l2_subdev {
>>  	struct v4l2_subdev_state *active_state;
>>  	u64 enabled_pads;
>>  	bool s_stream_enabled;
>> +	bool vcm_enabled;
>>  };
>>  
>>  
> 

Re: [PATCH 1/2] media: v4l: Call s_stream() on VCM when it is called on the associated sensor

[Hans de Goede]

p.s.

On 23-Oct-24 2:48 PM, Hans de Goede wrote:
> Hi Laurent,
> 
> On 1-Sep-24 11:28 PM, Laurent Pinchart wrote:
>> Hi Hans,
>>
>> Thank you for the patch.
>>
>> On Sun, Sep 01, 2024 at 11:18:33PM +0200, Hans de Goede wrote:
>>> Currently VCM drivers power-up the VCM as soon as the VCM's /dev node
>>> is opened and through the runtime-pm device-link to the sensor this
>>> also powers up the sensor.
>>>
>>> Powering up the VCM and sensor when the /dev node is opened is undesirable,
>>> without a VCM sensors delay powering up until s_stream(1) is called. This
>>> allows querying / negotiating capabilities without powering things up.
>>>
>>> Sometimes a long running daemon like pipewire may keep the /dev node open
>>> all the time. The kernel should still be able to powerdown the VCM + sensor
>>> in this scenario.
>>>
>>> VCM drivers should be able to do the same as sensor drivers and only
>>> power-up the VCM when s_stream(1) is called on the VCM subdev, but this
>>> requires that s_stream() is actually called on the VCM when the sensor
>>> starts / stops streaming.
>>
>> .s_stream() doesn't conceptually make sense for VCMs. Furthermore,
>> .s_stream() is being replaced with .enable_streams() and
>> .disable_streams(), which will make even less sense. We need a different
>> API.
> 
> We can discuss how to call the subdev ops for this once we know
> what the overal design for this is going to be. So lets postpone
> the discussion about naming the subdev ops for this till later.
> 
>>> The s_stream() call on sensor subdevs is done by CSI-receiver/ISP drivers.
>>> To avoid needing to change all these call sites to also call s_stream()
>>> on the VCM (if there is one) handle the VCM in the v4l2-core similar to how
>>> the core takes care of turning on/off the privacy LED.
>>
>> This needs to come with a design rationale, documented in
>> Documentation/. The design needs to explain the use cases. Lens motion
>> may take time, which I expect will influence how we will need to handle
>> power management.
>>
>> I'm not very comfortable handling this in v4l2-subdev.c, it seems that
>> we'll hardcode use cases. Without a clear and detailed designed
>> rationale, this patch feels we'll paint ourselves in a corner. We have
>> enough badly designed (or not designed at all) APIs for cameras, it's
>> time to do better.
> 
> Design wise I roughly see 3 options:
> 
> a. Stick with the current design of powering on on open of /dev/v4l-subdev
> 
> b. Add explicit calls userspace can make to power-on the device
> 
> c. My current proposal to automatically power-on when the sensor associated
> with the VCM.
> 
> Since this patch series implements 3, I'm going to discuss that option
> here now. But if that hits a dead end we can look at 2. too.
> 
> Talking about use-cases, lets simplify things by looking at use-cases
> solely from a VCM power on/off point of view. Looking at it that way
> I really only see 2 orthogonal groupings for all use-cases (so 4
> possible combinations of groupings, but I think we can discuss each
> of the 2 axis separately):
> 
> Grouping i: amount of sensor stream start/stops in a single "use"
> 
> 1. The sensor starts streaming once and then the stream stops for
> a significant period of time; vs
> 
> 2. The stream is sometimes stopped to change settings and then
> restarted within a single use-case.
> 
> For 1. automatically powering on the VCM on stream start is fine.
> For 2. we do not want the VCM to turn off and then back on again
> while changing settings. We can use auto-suspend with a long enough
> auto-suspend delay (we could default to 1s) for this. There might
> be some special cases where we need to change the auto-suspend delay.
> We could add some V4L2 API (ctrl?) for this, or use the existing
> sysfs API through e.g. a udev rule.
> 
> To me using autosuspend-delays here seems workable and having
> the v4l2-subdev code manage the power has the advantage that it does
> not break any userspace APIs. IOW things will just work while
> saving power for existing users and things will also just work
> for new use-cases without requiring userspace to have to do
> extra work for this.

Note that apparently even with the existing power-on on open()
there are issues where the VCM powers-down when change modes
and people are already submitting patch to work around this, e.g. :

https://patchwork.linuxtv.org/project/linux-media/patch/20240831055328.22482-1-zhi.mao@mediatek.com/

> Grouping ii: Does the VCM need to be turned on before the stream
> starts:
> 
> 3. A small delay at startup for the power-on (on the first start
> stream, not when switching modes) is fine /  having the first few
> frames being out of focus while the lens moves is fine, they typically
> will be anyways until the autofocus algorithm has locked; vs
> 
> 4. A small delay at startup for the power-on is undesirable /
> the focus value is known beforehand and the lens must be in position
> before capturing the first frame.
> 
> 4. will clearly not work well when the v4l2-subdev code manage
> the power. But this seems like a rather corner case scenario; and
> one which we could still make work by simply disabling runtime-pm
> for the VCM through sysfs in this case.
> 
> In conclusion to me it seems that having the v4l2-subdev code
> manage the power seems like it would work well. The only really
> problematic scenario which I can come up with is 4. and that seems
> a bit of a corner case. If this ever becomes a problem we could
> can make it work by either disabling runtime-pm through sysfs or
> by offering some API to prime the VCM which turns it on before
> the first stream-start (and it will still get automatically turned
> off after stream-stop + timeout).
> 
> We could even mix option b) and c) from the "Design wise I roughly
> see 3 options" options above and offer explicit power-management
> for apps which want that; while still automatically increasing
> the power-on count on sensor streaming, with any power-on
> count > 1 turning the VCM on.
> 
> This way apps/use-cases which want to make sure the VCM is on
> early or stays on during mode switching can use the explicit APIs
> while simpler use-cases (and existing use-cases since we cannot
> break userspace API) can rely on the automatic powering on done
> by the v4l2-subdev core.
> 
> The only downside which I can see of using a power-on counter
> and combining explicit + automatic control is that it will be
> impossible to turn the VCM off while streaming.
> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> 
> 
>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>>  drivers/media/v4l2-core/v4l2-async.c  | 20 +++++++++++++++++++
>>>  drivers/media/v4l2-core/v4l2-subdev.c | 28 +++++++++++++++++++++------
>>>  include/media/v4l2-subdev.h           |  2 ++
>>>  3 files changed, 44 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
>>> index ee884a8221fb..9b854f1d1051 100644
>>> --- a/drivers/media/v4l2-core/v4l2-async.c
>>> +++ b/drivers/media/v4l2-core/v4l2-async.c
>>> @@ -330,6 +330,11 @@ static int v4l2_async_create_ancillary_links(struct v4l2_async_notifier *n,
>>>  		return 0;
>>>  	}
>>>  
>>> +	if (sd->entity.function == MEDIA_ENT_F_LENS) {
>>> +		dev_dbg(n->sd->dev, "Using %s VCM\n", dev_name(sd->dev));
>>> +		n->sd->vcm = sd;
>>> +	}
>>> +
>>>  	link = media_create_ancillary_link(&n->sd->entity, &sd->entity);
>>>  
>>>  	return IS_ERR(link) ? PTR_ERR(link) : 0;
>>> @@ -871,6 +876,21 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
>>>  	if (!sd->async_list.next)
>>>  		return;
>>>  
>>> +#if defined(CONFIG_MEDIA_CONTROLLER)
>>> +	if (sd->entity.function == MEDIA_ENT_F_LENS && sd->v4l2_dev && sd->v4l2_dev->mdev) {
>>> +		struct media_entity *entity;
>>> +
>>> +		media_device_for_each_entity(entity, sd->v4l2_dev->mdev) {
>>> +			struct v4l2_subdev *it = media_entity_to_v4l2_subdev(entity);
>>> +
>>> +			if (it->vcm == sd) {
>>> +				dev_dbg(it->dev, "Clearing VCM\n");
>>> +				it->vcm = NULL;
>>> +			}
>>> +		}
>>> +	}
>>> +#endif
>>> +
>>>  	v4l2_subdev_put_privacy_led(sd);
>>>  
>>>  	mutex_lock(&list_lock);
>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>>> index 7c5812d55315..24a68d90f686 100644
>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>>> @@ -148,17 +148,33 @@ static int subdev_close(struct file *file)
>>>  }
>>>  #endif /* CONFIG_VIDEO_V4L2_SUBDEV_API */
>>>  
>>> -static void v4l2_subdev_enable_privacy_led(struct v4l2_subdev *sd)
>>> +static void v4l2_subdev_enable_privacy_led_and_vcm(struct v4l2_subdev *sd)
>>>  {
>>>  #if IS_REACHABLE(CONFIG_LEDS_CLASS)
>>>  	if (!IS_ERR_OR_NULL(sd->privacy_led))
>>>  		led_set_brightness(sd->privacy_led,
>>>  				   sd->privacy_led->max_brightness);
>>>  #endif
>>> +
>>> +	if (sd->vcm && !sd->vcm_enabled &&
>>> +	    v4l2_subdev_has_op(sd->vcm, video, s_stream)) {
>>> +		int ret;
>>> +
>>> +		ret = v4l2_subdev_call(sd->vcm, video, s_stream, 1);
>>> +		if (ret)
>>> +			dev_err(sd->vcm->dev, "Error powering on VCM: %d\n", ret);
>>> +		else
>>> +			sd->vcm_enabled = true;
>>> +	}
>>>  }
>>>  
>>> -static void v4l2_subdev_disable_privacy_led(struct v4l2_subdev *sd)
>>> +static void v4l2_subdev_disable_privacy_led_and_vcm(struct v4l2_subdev *sd)
>>>  {
>>> +	if (sd->vcm && sd->vcm_enabled) {
>>> +		v4l2_subdev_call(sd->vcm, video, s_stream, 0);
>>> +		sd->vcm_enabled = false;
>>> +	}
>>> +
>>>  #if IS_REACHABLE(CONFIG_LEDS_CLASS)
>>>  	if (!IS_ERR_OR_NULL(sd->privacy_led))
>>>  		led_set_brightness(sd->privacy_led, 0);
>>> @@ -466,9 +482,9 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
>>>  		sd->s_stream_enabled = enable;
>>>  
>>>  		if (enable)
>>> -			v4l2_subdev_enable_privacy_led(sd);
>>> +			v4l2_subdev_enable_privacy_led_and_vcm(sd);
>>>  		else
>>> -			v4l2_subdev_disable_privacy_led(sd);
>>> +			v4l2_subdev_disable_privacy_led_and_vcm(sd);
>>>  	}
>>>  
>>>  	return ret;
>>> @@ -2289,7 +2305,7 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
>>>  	 * for all cases.
>>>  	 */
>>>  	if (!use_s_stream && !already_streaming)
>>> -		v4l2_subdev_enable_privacy_led(sd);
>>> +		v4l2_subdev_enable_privacy_led_and_vcm(sd);
>>>  
>>>  done:
>>>  	if (!use_s_stream)
>>> @@ -2382,7 +2398,7 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
>>>  done:
>>>  	if (!use_s_stream) {
>>>  		if (!v4l2_subdev_is_streaming(sd))
>>> -			v4l2_subdev_disable_privacy_led(sd);
>>> +			v4l2_subdev_disable_privacy_led_and_vcm(sd);
>>>  
>>>  		v4l2_subdev_unlock_state(state);
>>>  	}
>>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>>> index bd235d325ff9..6568a0cc070b 100644
>>> --- a/include/media/v4l2-subdev.h
>>> +++ b/include/media/v4l2-subdev.h
>>> @@ -1094,6 +1094,7 @@ struct v4l2_subdev {
>>>  	 */
>>>  
>>>  	struct led_classdev *privacy_led;
>>> +	struct v4l2_subdev *vcm;
>>>  
>>>  	/*
>>>  	 * TODO: active_state should most likely be changed from a pointer to an
>>> @@ -1104,6 +1105,7 @@ struct v4l2_subdev {
>>>  	struct v4l2_subdev_state *active_state;
>>>  	u64 enabled_pads;
>>>  	bool s_stream_enabled;
>>> +	bool vcm_enabled;
>>>  };
>>>  
>>>  
>>
> 

[PATCH 2/2] media: i2c: Add driver for AD5823 VCM

[Hans de Goede]

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  |   5 +
 drivers/media/i2c/Makefile |   1 +
 drivers/media/i2c/ad5823.c | 311 +++++++++++++++++++++++++++++++++++++
 3 files changed, 317 insertions(+)
 create mode 100644 drivers/media/i2c/ad5823.c

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 70254f0cca67..4c0b198b7c31 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -742,6 +742,11 @@ 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"
+	help
+	  This is a driver for the AD5823 camera lens voice coil / VCM.
+
 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 babc1bc2393e..34d3e7a2abbb 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..83de4ab38399
--- /dev/null
+++ b/drivers/media/i2c/ad5823.c
@@ -0,0 +1,311 @@
+/*
+ * Analog Devices AD5823 VCM driver
+ * Copyright 2023 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)
+
+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);
+	int ret;
+
+	/* Only apply changes to the controls if the device is powered up */
+	if (!pm_runtime_get_if_in_use(ad5823->sd.dev))
+		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;
+
+	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);
+
+	return ad5823_power_up(ad5823, false);
+}
+
+static int ad5823_s_stream(struct v4l2_subdev *sd, int enable)
+{
+	struct ad5823_device *ad5823 = to_ad5823_device(sd);
+	int ret;
+
+	if (enable) {
+		ret = pm_runtime_resume_and_get(sd->dev);
+		if (ret < 0)
+			return ret;
+
+		/* Restore value of ctrls */
+		ret = v4l2_ctrl_handler_setup(&ad5823->ctrls.handler);
+		if (ret < 0)
+			pm_runtime_put(sd->dev);
+
+		return ret;
+	} else {
+		pm_runtime_put(sd->dev);
+		return 0;
+	}
+}
+
+static const struct v4l2_subdev_video_ops ad5823_video_ops = {
+	.s_stream = ad5823_s_stream,
+};
+
+static const struct v4l2_subdev_ops ad5823_ops = {
+	.video	= &ad5823_video_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;
+
+	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);
+	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);
+
+	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" },
+	{ }
+};
+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");
-- 
2.46.0

Re: [PATCH 2/2] media: i2c: Add driver for AD5823 VCM

[kernel test robot]

Hi Hans,

kernel test robot noticed the following build errors:

[auto build test ERROR on media-tree/master]
[also build test ERROR on linuxtv-media-stage/master sailus-media-tree/master linus/master v6.11-rc6 next-20240903]
[cannot apply to sailus-media-tree/streams]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Hans-de-Goede/media-v4l-Call-s_stream-on-VCM-when-it-is-called-on-the-associated-sensor/20240902-052000
base:   git://linuxtv.org/media_tree.git master
patch link:    https://lore.kernel.org/r/20240901211834.145186-3-hdegoede%40redhat.com
patch subject: [PATCH 2/2] media: i2c: Add driver for AD5823 VCM
config: x86_64-randconfig-121-20240904 (https://download.01.org/0day-ci/archive/20240904/202409040751.A1Dm1pgG-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240904/202409040751.A1Dm1pgG-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409040751.A1Dm1pgG-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

   drivers/media/i2c/ad5823.c: In function 'ad5823_probe':
>> drivers/media/i2c/ad5823.c:203:26: error: implicit declaration of function 'devm_cci_regmap_init_i2c' [-Werror=implicit-function-declaration]
     203 |         ad5823->regmap = devm_cci_regmap_init_i2c(client, 8);
         |                          ^~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/media/i2c/ad5823.c:203:24: warning: assignment to 'struct regmap *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
     203 |         ad5823->regmap = devm_cci_regmap_init_i2c(client, 8);
         |                        ^
   cc1: some warnings being treated as errors


vim +/devm_cci_regmap_init_i2c +203 drivers/media/i2c/ad5823.c

   193	
   194	static int ad5823_probe(struct i2c_client *client)
   195	{
   196		struct ad5823_device *ad5823;
   197		int ret;
   198	
   199		ad5823 = devm_kzalloc(&client->dev, sizeof(*ad5823), GFP_KERNEL);
   200		if (!ad5823)
   201			return -ENOMEM;
   202	
 > 203		ad5823->regmap = devm_cci_regmap_init_i2c(client, 8);
   204		if (IS_ERR(ad5823->regmap))
   205			return PTR_ERR(ad5823->regmap);
   206	
   207		ad5823->arc_mode = AD5823_ARC_RES1;
   208		ad5823->resonance_period = AD5823_RESONANCE_PERIOD;
   209	
   210		/* Optional indication of ARC mode select */
   211		device_property_read_u32(&client->dev, "adi,arc-mode",
   212					 &ad5823->arc_mode);
   213	
   214		/* Optional indication of VCM resonance period */
   215		device_property_read_u32(&client->dev, "adi,resonance-period",
   216					 &ad5823->resonance_period);
   217	
   218		ad5823->regulator = devm_regulator_get(&client->dev, "vdd");
   219		if (IS_ERR(ad5823->regulator))
   220			return dev_err_probe(&client->dev, PTR_ERR(ad5823->regulator),
   221					     "getting regulator\n");
   222	
   223		v4l2_i2c_subdev_init(&ad5823->sd, client, &ad5823_ops);
   224		ad5823->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
   225	
   226		ret = ad5823_init_controls(ad5823);
   227		if (ret)
   228			return ret;
   229	
   230		ret = media_entity_pads_init(&ad5823->sd.entity, 0, NULL);
   231		if (ret < 0)
   232			goto err_free_ctrl_handler;
   233	
   234		ad5823->sd.entity.function = MEDIA_ENT_F_LENS;
   235	
   236		/*
   237		 * We need the driver to work in the event that pm runtime is disable in
   238		 * the kernel, so power up and verify the chip now. In the event that
   239		 * runtime pm is disabled this will leave the chip on, so that the lens
   240		 * will work.
   241		 */
   242	
   243		ret = ad5823_power_up(ad5823, true);
   244		if (ret)
   245			goto err_cleanup_media;
   246	
   247		pm_runtime_set_active(&client->dev);
   248		pm_runtime_get_noresume(&client->dev);
   249		pm_runtime_enable(&client->dev);
   250	
   251		ret = v4l2_async_register_subdev(&ad5823->sd);
   252		if (ret < 0)
   253			goto err_pm_runtime;
   254	
   255		pm_runtime_set_autosuspend_delay(&client->dev, 1000);
   256		pm_runtime_use_autosuspend(&client->dev);
   257		pm_runtime_put_autosuspend(&client->dev);
   258	
   259		return ret;
   260	
   261	err_pm_runtime:
   262		pm_runtime_disable(&client->dev);
   263		pm_runtime_put_noidle(&client->dev);
   264		ad5823_power_down(ad5823);
   265	err_cleanup_media:
   266		media_entity_cleanup(&ad5823->sd.entity);
   267	err_free_ctrl_handler:
   268		v4l2_ctrl_handler_free(&ad5823->ctrls.handler);
   269	
   270		return ret;
   271	}
   272	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/2] media: i2c: Add driver for AD5823 VCM

[Hans de Goede]

Hi,

On 9/4/24 1:59 AM, kernel test robot wrote:
> Hi Hans,
> 
> kernel test robot noticed the following build errors:
> 
> [auto build test ERROR on media-tree/master]
> [also build test ERROR on linuxtv-media-stage/master sailus-media-tree/master linus/master v6.11-rc6 next-20240903]
> [cannot apply to sailus-media-tree/streams]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Hans-de-Goede/media-v4l-Call-s_stream-on-VCM-when-it-is-called-on-the-associated-sensor/20240902-052000
> base:   git://linuxtv.org/media_tree.git master
> patch link:    https://lore.kernel.org/r/20240901211834.145186-3-hdegoede%40redhat.com
> patch subject: [PATCH 2/2] media: i2c: Add driver for AD5823 VCM
> config: x86_64-randconfig-121-20240904 (https://download.01.org/0day-ci/archive/20240904/202409040751.A1Dm1pgG-lkp@intel.com/config)
> compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240904/202409040751.A1Dm1pgG-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202409040751.A1Dm1pgG-lkp@intel.com/
> 
> All error/warnings (new ones prefixed by >>):
> 
>    drivers/media/i2c/ad5823.c: In function 'ad5823_probe':
>>> drivers/media/i2c/ad5823.c:203:26: error: implicit declaration of function 'devm_cci_regmap_init_i2c' [-Werror=implicit-function-declaration]
>      203 |         ad5823->regmap = devm_cci_regmap_init_i2c(client, 8);
>          |                          ^~~~~~~~~~~~~~~~~~~~~~~~
>>> drivers/media/i2c/ad5823.c:203:24: warning: assignment to 'struct regmap *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
>      203 |         ad5823->regmap = devm_cci_regmap_init_i2c(client, 8);
>          |                        ^
>    cc1: some warnings being treated as errors


This is caused by a missing "select V4L2_CCI_I2C" I have fixed this
in my local tree and this fix will be included in v2 when I get
around to posting v2 (which also depends on the discussion
surrounding patch 1/2).

Regards,

Hans

Re: [PATCH 2/2] media: i2c: Add driver for AD5823 VCM

[kernel test robot]

Hi Hans,

kernel test robot noticed the following build errors:

[auto build test ERROR on media-tree/master]
[also build test ERROR on linuxtv-media-stage/master sailus-media-tree/master linus/master v6.11-rc6 next-20240903]
[cannot apply to sailus-media-tree/streams]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Hans-de-Goede/media-v4l-Call-s_stream-on-VCM-when-it-is-called-on-the-associated-sensor/20240902-052000
base:   git://linuxtv.org/media_tree.git master
patch link:    https://lore.kernel.org/r/20240901211834.145186-3-hdegoede%40redhat.com
patch subject: [PATCH 2/2] media: i2c: Add driver for AD5823 VCM
config: x86_64-randconfig-076-20240904 (https://download.01.org/0day-ci/archive/20240904/202409041123.VAFdMjn2-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240904/202409041123.VAFdMjn2-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409041123.VAFdMjn2-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/media/i2c/ad5823.c:203:19: error: call to undeclared function 'devm_cci_regmap_init_i2c'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
     203 |         ad5823->regmap = devm_cci_regmap_init_i2c(client, 8);
         |                          ^
>> drivers/media/i2c/ad5823.c:203:17: error: incompatible integer to pointer conversion assigning to 'struct regmap *' from 'int' [-Wint-conversion]
     203 |         ad5823->regmap = devm_cci_regmap_init_i2c(client, 8);
         |                        ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   2 errors generated.


vim +/devm_cci_regmap_init_i2c +203 drivers/media/i2c/ad5823.c

   193	
   194	static int ad5823_probe(struct i2c_client *client)
   195	{
   196		struct ad5823_device *ad5823;
   197		int ret;
   198	
   199		ad5823 = devm_kzalloc(&client->dev, sizeof(*ad5823), GFP_KERNEL);
   200		if (!ad5823)
   201			return -ENOMEM;
   202	
 > 203		ad5823->regmap = devm_cci_regmap_init_i2c(client, 8);
   204		if (IS_ERR(ad5823->regmap))
   205			return PTR_ERR(ad5823->regmap);
   206	
   207		ad5823->arc_mode = AD5823_ARC_RES1;
   208		ad5823->resonance_period = AD5823_RESONANCE_PERIOD;
   209	
   210		/* Optional indication of ARC mode select */
   211		device_property_read_u32(&client->dev, "adi,arc-mode",
   212					 &ad5823->arc_mode);
   213	
   214		/* Optional indication of VCM resonance period */
   215		device_property_read_u32(&client->dev, "adi,resonance-period",
   216					 &ad5823->resonance_period);
   217	
   218		ad5823->regulator = devm_regulator_get(&client->dev, "vdd");
   219		if (IS_ERR(ad5823->regulator))
   220			return dev_err_probe(&client->dev, PTR_ERR(ad5823->regulator),
   221					     "getting regulator\n");
   222	
   223		v4l2_i2c_subdev_init(&ad5823->sd, client, &ad5823_ops);
   224		ad5823->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
   225	
   226		ret = ad5823_init_controls(ad5823);
   227		if (ret)
   228			return ret;
   229	
   230		ret = media_entity_pads_init(&ad5823->sd.entity, 0, NULL);
   231		if (ret < 0)
   232			goto err_free_ctrl_handler;
   233	
   234		ad5823->sd.entity.function = MEDIA_ENT_F_LENS;
   235	
   236		/*
   237		 * We need the driver to work in the event that pm runtime is disable in
   238		 * the kernel, so power up and verify the chip now. In the event that
   239		 * runtime pm is disabled this will leave the chip on, so that the lens
   240		 * will work.
   241		 */
   242	
   243		ret = ad5823_power_up(ad5823, true);
   244		if (ret)
   245			goto err_cleanup_media;
   246	
   247		pm_runtime_set_active(&client->dev);
   248		pm_runtime_get_noresume(&client->dev);
   249		pm_runtime_enable(&client->dev);
   250	
   251		ret = v4l2_async_register_subdev(&ad5823->sd);
   252		if (ret < 0)
   253			goto err_pm_runtime;
   254	
   255		pm_runtime_set_autosuspend_delay(&client->dev, 1000);
   256		pm_runtime_use_autosuspend(&client->dev);
   257		pm_runtime_put_autosuspend(&client->dev);
   258	
   259		return ret;
   260	
   261	err_pm_runtime:
   262		pm_runtime_disable(&client->dev);
   263		pm_runtime_put_noidle(&client->dev);
   264		ad5823_power_down(ad5823);
   265	err_cleanup_media:
   266		media_entity_cleanup(&ad5823->sd.entity);
   267	err_free_ctrl_handler:
   268		v4l2_ctrl_handler_free(&ad5823->ctrls.handler);
   269	
   270		return ret;
   271	}
   272	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki