Hi,
On 11/2/21 00:21, Daniel Scally wrote:
> From: Hans de Goede <hdegoede@xxxxxxxxxx>
>
> ov5693_sw_standby() starts/stops streaming rename it so that it is actually
> named after what it does.
>
> This also removes the weird enable inverting going on before.
>
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
> drivers/media/i2c/ov5693.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
> index 8ad51f8f88cf..2613bad49f78 100644
> --- a/drivers/media/i2c/ov5693.c
> +++ b/drivers/media/i2c/ov5693.c
> @@ -742,13 +742,13 @@ static int ov5693_mode_configure(struct ov5693_device *ov5693)
> return ret;
> }
>
> -static int ov5693_sw_standby(struct ov5693_device *ov5693, bool standby)
> +static int ov5693_enable_streaming(struct ov5693_device *ov5693, bool enable)
> {
> int ret = 0;
>
> ov5693_write_reg(ov5693, OV5693_SW_STREAM_REG,
> - standby ? OV5693_STOP_STREAMING :
> - OV5693_START_STREAMING, &ret);
> + enable ? OV5693_STOP_STREAMING :
> + OV5693_START_STREAMING, &ret);
In this version of this patch the function still behaves as if it is
setting the sensor in a standby mode, my original version had this:
ov5693_write_reg(ov5693, OV5693_SW_STREAM_REG,
enable ? OV5693_START_STREAMING : OV5693_STOP_STREAMING,
&ret);
Which makes the function behave more logical.
>
> return ret;
> }
> @@ -784,9 +784,9 @@ static int ov5693_sensor_init(struct ov5693_device *ov5693)
> return ret;
> }
>
> - ret = ov5693_sw_standby(ov5693, true);
> + ret = ov5693_enable_streaming(ov5693, true);
And my original version changes the true to false here, stopping
streaming on init (as before when setting standby to true,
including the error messages being different:
ret = ov5693_enable_streaming(ov5693, false);
if (ret)
dev_err(ov5693->dev, "%s stop streaming error\n", __func__);
> if (ret)
> - dev_err(ov5693->dev, "software standby error\n");
> + dev_err(ov5693->dev, "enable streaming error\n");
>
> return ret;
> }
> @@ -1119,7 +1119,7 @@ static int ov5693_s_stream(struct v4l2_subdev *sd, int enable)
> }
>
> mutex_lock(&ov5693->lock);
> - ret = ov5693_sw_standby(ov5693, enable);
> + ret = ov5693_enable_streaming(ov5693, enable);
And this used to be a change from !enable -> enable.
Note that the current version cannot work, you pass enable
(instead of !enable) but ov5693_enable_streaming() still
sends OV5693_STOP_STREAMING if enable is true, so you are
now stopping streaming when called to enable it ?
> mutex_unlock(&ov5693->lock);
>
> if (ret)
>
Besides this needing some work, it is fine with me, and
I believe that it is best, to just squash this and
patch 5/5 into patch 2 (since your introducing this
driver in this series it is a bit to then apply fixes
to it in the same series).
Regards,
Hans
