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. > > > ><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. > > > > ><snip> > > > >>+ > >>+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? > > > >>+ 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@xxxxxx XMPP: sailus@xxxxxxxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html