[46/57] media: atomisp: ov2680: Delay power-on till streaming is started
Commit Message
Move the setting of the mode to stream on, this also allows
delaying power-on till streaming is started.
And drop the deprecated s_power callback since this now no long
is necessary.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
.../media/atomisp/i2c/atomisp-ov2680.c | 101 +++++++-----------
1 file changed, 41 insertions(+), 60 deletions(-)
Comments
On Mon, Jan 23, 2023 at 01:51:54PM +0100, Hans de Goede wrote:
> Move the setting of the mode to stream on, this also allows
> delaying power-on till streaming is started.
>
> And drop the deprecated s_power callback since this now no long
> is necessary.
Reviewed-by: Andy Shevchenko <andy@kernel.org>
See below.
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> .../media/atomisp/i2c/atomisp-ov2680.c | 101 +++++++-----------
> 1 file changed, 41 insertions(+), 60 deletions(-)
>
> diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> index 1dc821ca4e68..2a8c4508cc66 100644
> --- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> +++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> @@ -327,24 +327,6 @@ static int power_down(struct v4l2_subdev *sd)
> return 0;
> }
>
> -static int ov2680_s_power(struct v4l2_subdev *sd, int on)
> -{
> - struct ov2680_device *dev = to_ov2680_sensor(sd);
> - int ret;
> -
> - mutex_lock(&dev->input_lock);
> -
> - if (on == 0) {
> - ret = power_down(sd);
> - } else {
> - ret = power_up(sd);
> - }
> -
> - mutex_unlock(&dev->input_lock);
> -
> - return ret;
> -}
> -
> static struct v4l2_mbus_framefmt *
> __ov2680_get_pad_format(struct ov2680_device *sensor,
> struct v4l2_subdev_state *state,
> @@ -393,14 +375,12 @@ static void ov2680_calc_mode(struct ov2680_device *sensor, int width, int height
> sensor->mode.vts = OV2680_LINES_PER_FRAME;
> }
>
> -static int ov2680_set_mode(struct ov2680_device *sensor, int width, int height)
> +static int ov2680_set_mode(struct ov2680_device *sensor)
> {
> struct i2c_client *client = sensor->client;
> u8 pll_div, unknown, inc, fmt1, fmt2;
> int ret;
>
> - ov2680_calc_mode(sensor, width, height);
> -
> if (sensor->mode.binning) {
> pll_div = 1;
> unknown = 0x23;
> @@ -500,7 +480,6 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
> struct i2c_client *client = v4l2_get_subdevdata(sd);
> struct v4l2_mbus_framefmt *fmt;
> unsigned int width, height;
> - int ret = 0;
>
> dev_dbg(&client->dev, "%s: %s: pad: %d, fmt: %p\n",
> __func__,
> @@ -518,23 +497,10 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
> if (format->which == V4L2_SUBDEV_FORMAT_TRY)
> return 0;
>
> - dev_dbg(&client->dev, "%s: %dx%d\n",
> - __func__, fmt->width, fmt->height);
> -
> mutex_lock(&dev->input_lock);
> -
> - /* s_power has not been called yet for std v4l2 clients (camorama) */
> - power_up(sd);
> -
> - ret = ov2680_set_mode(dev, fmt->width, fmt->height);
> - if (ret < 0)
> - goto err;
> -
> - /* Restore value of all ctrls */
> - ret = __v4l2_ctrl_handler_setup(&dev->ctrls.handler);
> -err:
> + ov2680_calc_mode(dev, fmt->width, fmt->height);
> mutex_unlock(&dev->input_lock);
> - return ret;
> + return 0;
> }
>
> static int ov2680_get_fmt(struct v4l2_subdev *sd,
> @@ -584,30 +550,50 @@ static int ov2680_detect(struct i2c_client *client)
>
> static int ov2680_s_stream(struct v4l2_subdev *sd, int enable)
> {
> - struct ov2680_device *dev = to_ov2680_sensor(sd);
> + struct ov2680_device *sensor = to_ov2680_sensor(sd);
> struct i2c_client *client = v4l2_get_subdevdata(sd);
> - int ret;
> + int ret = 0;
>
> - mutex_lock(&dev->input_lock);
> - if (enable)
> - dev_dbg(&client->dev, "ov2680_s_stream one\n");
> - else
> - dev_dbg(&client->dev, "ov2680_s_stream off\n");
> -
> - ret = ovxxxx_write_reg8(client, OV2680_SW_STREAM,
> - enable ? OV2680_START_STREAMING : OV2680_STOP_STREAMING);
> - if (ret == 0) {
> - dev->is_streaming = enable;
> - v4l2_ctrl_activate(dev->ctrls.vflip, !enable);
> - v4l2_ctrl_activate(dev->ctrls.hflip, !enable);
> + mutex_lock(&sensor->input_lock);
> +
> + if (sensor->is_streaming == enable) {
> + dev_warn(&client->dev, "stream already %sed\n", enable ? "start" : "stopp");
stopP ?!
> + goto error_unlock;
> }
>
> - //otp valid at stream on state
> - //if(!dev->otp_data)
> - // dev->otp_data = ov2680_otp_read(sd);
> + if (enable) {
> + ret = power_up(sd);
> + if (ret)
> + goto error_unlock;
>
> - mutex_unlock(&dev->input_lock);
> + ret = ov2680_set_mode(sensor);
> + if (ret)
> + goto error_power_down;
>
> + /* Restore value of all ctrls */
> + ret = __v4l2_ctrl_handler_setup(&sensor->ctrls.handler);
> + if (ret)
> + goto error_power_down;
> +
> + ret = ovxxxx_write_reg8(client, OV2680_SW_STREAM, OV2680_START_STREAMING);
> + if (ret)
> + goto error_power_down;
> + } else {
> + ovxxxx_write_reg8(client, OV2680_SW_STREAM, OV2680_STOP_STREAMING);
> + power_down(sd);
> + }
> +
> + sensor->is_streaming = enable;
> + v4l2_ctrl_activate(sensor->ctrls.vflip, !enable);
> + v4l2_ctrl_activate(sensor->ctrls.hflip, !enable);
> +
> + mutex_unlock(&sensor->input_lock);
> + return 0;
> +
> +error_power_down:
> + power_down(sd);
> +error_unlock:
> + mutex_unlock(&sensor->input_lock);
> return ret;
> }
>
> @@ -736,10 +722,6 @@ static const struct v4l2_subdev_sensor_ops ov2680_sensor_ops = {
> .g_skip_frames = ov2680_g_skip_frames,
> };
>
> -static const struct v4l2_subdev_core_ops ov2680_core_ops = {
> - .s_power = ov2680_s_power,
> -};
> -
> static const struct v4l2_subdev_pad_ops ov2680_pad_ops = {
> .enum_mbus_code = ov2680_enum_mbus_code,
> .enum_frame_size = ov2680_enum_frame_size,
> @@ -749,7 +731,6 @@ static const struct v4l2_subdev_pad_ops ov2680_pad_ops = {
> };
>
> static const struct v4l2_subdev_ops ov2680_ops = {
> - .core = &ov2680_core_ops,
> .video = &ov2680_video_ops,
> .pad = &ov2680_pad_ops,
> .sensor = &ov2680_sensor_ops,
> --
> 2.39.0
>
Hi,
On 1/24/23 11:51, Andy Shevchenko wrote:
> On Mon, Jan 23, 2023 at 01:51:54PM +0100, Hans de Goede wrote:
>> Move the setting of the mode to stream on, this also allows
>> delaying power-on till streaming is started.
>>
>> And drop the deprecated s_power callback since this now no long
>> is necessary.
>
> Reviewed-by: Andy Shevchenko <andy@kernel.org>
>
> See below.
>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> .../media/atomisp/i2c/atomisp-ov2680.c | 101 +++++++-----------
>> 1 file changed, 41 insertions(+), 60 deletions(-)
>>
>> diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
>> index 1dc821ca4e68..2a8c4508cc66 100644
>> --- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
>> +++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
<snip>
>> + if (sensor->is_streaming == enable) {
>> + dev_warn(&client->dev, "stream already %sed\n", enable ? "start" : "stopp");
>
> stopP ?!
Yes the format string is "%sed" so "stopp" gives us "stopped".
Regards,
Hans
On Tue, Jan 24, 2023 at 12:31:14PM +0100, Hans de Goede wrote:
> On 1/24/23 11:51, Andy Shevchenko wrote:
> > On Mon, Jan 23, 2023 at 01:51:54PM +0100, Hans de Goede wrote:
...
> >> + if (sensor->is_streaming == enable) {
> >> + dev_warn(&client->dev, "stream already %sed\n", enable ? "start" : "stopp");
> >
> > stopP ?!
>
> Yes the format string is "%sed" so "stopp" gives us "stopped".
Can we, please, use full words, so it will be easier to find a candidate for
including into string_choices.h or so?
Hi,
On 1/24/23 13:52, Andy Shevchenko wrote:
> On Tue, Jan 24, 2023 at 12:31:14PM +0100, Hans de Goede wrote:
>> On 1/24/23 11:51, Andy Shevchenko wrote:
>>> On Mon, Jan 23, 2023 at 01:51:54PM +0100, Hans de Goede wrote:
>
> ...
>
>>>> + if (sensor->is_streaming == enable) {
>>>> + dev_warn(&client->dev, "stream already %sed\n", enable ? "start" : "stopp");
>>>
>>> stopP ?!
>>
>> Yes the format string is "%sed" so "stopp" gives us "stopped".
>
> Can we, please, use full words, so it will be easier to find a candidate for
> including into string_choices.h or so?
Sure I'll update this before adding it to the PR.
Regards,
Hans
@@ -327,24 +327,6 @@ static int power_down(struct v4l2_subdev *sd)
return 0;
}
-static int ov2680_s_power(struct v4l2_subdev *sd, int on)
-{
- struct ov2680_device *dev = to_ov2680_sensor(sd);
- int ret;
-
- mutex_lock(&dev->input_lock);
-
- if (on == 0) {
- ret = power_down(sd);
- } else {
- ret = power_up(sd);
- }
-
- mutex_unlock(&dev->input_lock);
-
- return ret;
-}
-
static struct v4l2_mbus_framefmt *
__ov2680_get_pad_format(struct ov2680_device *sensor,
struct v4l2_subdev_state *state,
@@ -393,14 +375,12 @@ static void ov2680_calc_mode(struct ov2680_device *sensor, int width, int height
sensor->mode.vts = OV2680_LINES_PER_FRAME;
}
-static int ov2680_set_mode(struct ov2680_device *sensor, int width, int height)
+static int ov2680_set_mode(struct ov2680_device *sensor)
{
struct i2c_client *client = sensor->client;
u8 pll_div, unknown, inc, fmt1, fmt2;
int ret;
- ov2680_calc_mode(sensor, width, height);
-
if (sensor->mode.binning) {
pll_div = 1;
unknown = 0x23;
@@ -500,7 +480,6 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
struct i2c_client *client = v4l2_get_subdevdata(sd);
struct v4l2_mbus_framefmt *fmt;
unsigned int width, height;
- int ret = 0;
dev_dbg(&client->dev, "%s: %s: pad: %d, fmt: %p\n",
__func__,
@@ -518,23 +497,10 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
if (format->which == V4L2_SUBDEV_FORMAT_TRY)
return 0;
- dev_dbg(&client->dev, "%s: %dx%d\n",
- __func__, fmt->width, fmt->height);
-
mutex_lock(&dev->input_lock);
-
- /* s_power has not been called yet for std v4l2 clients (camorama) */
- power_up(sd);
-
- ret = ov2680_set_mode(dev, fmt->width, fmt->height);
- if (ret < 0)
- goto err;
-
- /* Restore value of all ctrls */
- ret = __v4l2_ctrl_handler_setup(&dev->ctrls.handler);
-err:
+ ov2680_calc_mode(dev, fmt->width, fmt->height);
mutex_unlock(&dev->input_lock);
- return ret;
+ return 0;
}
static int ov2680_get_fmt(struct v4l2_subdev *sd,
@@ -584,30 +550,50 @@ static int ov2680_detect(struct i2c_client *client)
static int ov2680_s_stream(struct v4l2_subdev *sd, int enable)
{
- struct ov2680_device *dev = to_ov2680_sensor(sd);
+ struct ov2680_device *sensor = to_ov2680_sensor(sd);
struct i2c_client *client = v4l2_get_subdevdata(sd);
- int ret;
+ int ret = 0;
- mutex_lock(&dev->input_lock);
- if (enable)
- dev_dbg(&client->dev, "ov2680_s_stream one\n");
- else
- dev_dbg(&client->dev, "ov2680_s_stream off\n");
-
- ret = ovxxxx_write_reg8(client, OV2680_SW_STREAM,
- enable ? OV2680_START_STREAMING : OV2680_STOP_STREAMING);
- if (ret == 0) {
- dev->is_streaming = enable;
- v4l2_ctrl_activate(dev->ctrls.vflip, !enable);
- v4l2_ctrl_activate(dev->ctrls.hflip, !enable);
+ mutex_lock(&sensor->input_lock);
+
+ if (sensor->is_streaming == enable) {
+ dev_warn(&client->dev, "stream already %sed\n", enable ? "start" : "stopp");
+ goto error_unlock;
}
- //otp valid at stream on state
- //if(!dev->otp_data)
- // dev->otp_data = ov2680_otp_read(sd);
+ if (enable) {
+ ret = power_up(sd);
+ if (ret)
+ goto error_unlock;
- mutex_unlock(&dev->input_lock);
+ ret = ov2680_set_mode(sensor);
+ if (ret)
+ goto error_power_down;
+ /* Restore value of all ctrls */
+ ret = __v4l2_ctrl_handler_setup(&sensor->ctrls.handler);
+ if (ret)
+ goto error_power_down;
+
+ ret = ovxxxx_write_reg8(client, OV2680_SW_STREAM, OV2680_START_STREAMING);
+ if (ret)
+ goto error_power_down;
+ } else {
+ ovxxxx_write_reg8(client, OV2680_SW_STREAM, OV2680_STOP_STREAMING);
+ power_down(sd);
+ }
+
+ sensor->is_streaming = enable;
+ v4l2_ctrl_activate(sensor->ctrls.vflip, !enable);
+ v4l2_ctrl_activate(sensor->ctrls.hflip, !enable);
+
+ mutex_unlock(&sensor->input_lock);
+ return 0;
+
+error_power_down:
+ power_down(sd);
+error_unlock:
+ mutex_unlock(&sensor->input_lock);
return ret;
}
@@ -736,10 +722,6 @@ static const struct v4l2_subdev_sensor_ops ov2680_sensor_ops = {
.g_skip_frames = ov2680_g_skip_frames,
};
-static const struct v4l2_subdev_core_ops ov2680_core_ops = {
- .s_power = ov2680_s_power,
-};
-
static const struct v4l2_subdev_pad_ops ov2680_pad_ops = {
.enum_mbus_code = ov2680_enum_mbus_code,
.enum_frame_size = ov2680_enum_frame_size,
@@ -749,7 +731,6 @@ static const struct v4l2_subdev_pad_ops ov2680_pad_ops = {
};
static const struct v4l2_subdev_ops ov2680_ops = {
- .core = &ov2680_core_ops,
.video = &ov2680_video_ops,
.pad = &ov2680_pad_ops,
.sensor = &ov2680_sensor_ops,