Re: [PATCH v5 4/5] media: i2c: ov5693: Rename ov5693_sw_standby() to ov5693_enable_streaming()

[Daniel Scally <djrscally@gmail.com>]

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@redhat.com>
>>
>> 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@redhat.com>
>> ---
>>  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
>
>