Re: [PATCH v3 2/2] media: i2c: Add support for ov5693 sensor

[Daniel Scally <djrscally@gmail.com>]

Hi Sakari

On 21/09/2021 13:48, Sakari Ailus wrote:
> Hi Daniel,
>
> On Tue, Sep 21, 2021 at 12:47:56PM +0100, Daniel Scally wrote:
>>> +
>>> +static int ov5693_get_exposure(struct ov5693_device *ov5693, s32 *value)
>>> +{
>>> +	u8 exposure_hh = 0, exposure_h = 0, exposure_l = 0;
>>> +	int ret;
>>> +
>>> +	ret = ov5693_read_reg(ov5693, OV5693_EXPOSURE_L_CTRL_HH_REG, &exposure_hh);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	ret = ov5693_read_reg(ov5693, OV5693_EXPOSURE_L_CTRL_H_REG, &exposure_h);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	ret = ov5693_read_reg(ov5693, OV5693_EXPOSURE_L_CTRL_L_REG, &exposure_l);
>>> +	if (ret)
>>> +		return ret;
>>> Does the sensor not allow reading this register as a single operation?
>>>
>>> Just a question. Some sensors from the vendor do not. Same for the writes.
>>
>> It does; if I'm honest I just preferred the individual read/writes. I
>> find it's easier to see exactly what's going on. Happy to change it if
>> you prefer though - it's less important now that the work is mostly done.
> It's certainly not wrong to do that but it takes a longer time. So you're
> much, much more likely to miss the frame you intended the settings to take
> effect. Also note the device could have a specific order in which to write
> them for the update to be atomic. Missing this could cause wildly
> misexposed frames. I don't know if this one does.
>
The datasheet doesn't mention a specific ordering, but I take your point
about the timings. I'll look at switching it to a single write operation.