Hi Hans
On 02/11/2021 09:24, Hans de Goede wrote:
> 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).
OK; I'll squash them then. Sorry to have messed up the application too!
The patch wouldn't apply cleanly due to other changes I'd made, and
apparently I wasn't nearly as careful sorting it as I thought I had been.
>
> Regards,
>
> Hans
>
>
