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

[Hans de Goede <hdegoede@redhat.com>]

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).

Regards,

Hans