Hi Steve,

On Mon, May 29, 2017 at 02:50:34PM -0700, Steve Longerbeam wrote:

quoted

<snip>

quoted

+
+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.

quoted

<snip>

quoted

+
+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.

quoted

<snip>

quoted

+
+free_ctrls:
+	v4l2_ctrl_handler_free(&sensor->ctrls.handler);
+entity_cleanup:
+	mutex_destroy(&sensor->lock);
+	media_entity_cleanup(&sensor->sd.entity);
+	regulator_bulk_disable(OV5640_NUM_SUPPLIES, sensor->supplies);

Should this still be here?

quoted

+	return ret;
+}
+
+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).

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus at iki.fi	XMPP: sailus at retiisi.org.uk