Hi Sakari,
On 05/29/2017 11:56 PM, Sakari Ailus wrote:
Hi Steve,
On Mon, May 29, 2017 at 02:50:34PM -0700, Steve Longerbeam wrote:
<snip>
+
+static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+ struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
+ struct ov5640_dev *sensor = to_ov5640_dev(sd);
+ int ret = 0;
+
+ mutex_lock(&sensor->lock);
Could you use the same lock for the controls as you use for the rest? Just
setting handler->lock after handler init does the trick.
Can you please rephrase, I don't follow. "same lock for the controls as
you use for the rest" - there's only one device lock owned by this driver
and I am already using that same lock.
There's another in the control handler. You could use your own lock for the
control handler as well.
I still don't understand.
<snip>
+
+static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
+{
+ struct ov5640_dev *sensor = to_ov5640_dev(sd);
+ int ret = 0;
+
+ mutex_lock(&sensor->lock);
+
+#if defined(CONFIG_MEDIA_CONTROLLER)
+ if (sd->entity.stream_count > 1)
The entity stream_count isn't connected to the number of times s_stream(sd,
true) is called. Please remove the check.
It's incremented by media_pipeline_start(), even if the entity is already
a member of the given pipeline.
I added this check because in imx-media, the ov5640 can be streaming
concurrently to multiple video capture devices, and each capture device
calls
media_pipeline_start() at stream on, which increments the entity stream
count.
So if one capture device issues a stream off while others are still
streaming,
ov5640 should remain at stream on. So the entity stream count is being
used as a streaming usage counter. Is there a better way to do this? Should
I use a private stream use counter instead?
Different drivers may use media_pipeline_start() in different ways. Stream
control shouldn't depend on that count. This could cause issues in using the
driver with other ISP / receiver drivers.
I think it should be enough to move the check to the imx driver in this
case.
I will remove this check.
+
+static int ov5640_remove(struct i2c_client *client)
+{
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+ struct ov5640_dev *sensor = to_ov5640_dev(sd);
+
+ regulator_bulk_disable(OV5640_NUM_SUPPLIES, sensor->supplies);
Ditto.
I don't understand. regulator_bulk_disable() is still needed, am I missing
something?
You still need to enable it first. I don't see that being done in probe. As
the driver implements the s_power() op, I don't see a need for powering the
device on at probe time (and conversely off at remove time).
Oh you're right, it must have been left over from a previous revision
I guess. Yes, regulator_bulk_enable|disable() is only called in
ov5640_set_power(). I'll remove regulator_bulk_disable() from
probe/remove.
Steve
