Hi Sakari,
On 17.04.2018 23:10, Sakari Ailus wrote:
> Hi Todor,
>
> On Tue, Apr 17, 2018 at 06:32:07PM +0300, Todor Tomov wrote:
> ...
>>>> +static int ov7251_regulators_enable(struct ov7251 *ov7251)
>>>> +{
>>>> + int ret;
>>>> +
>>>> + ret = regulator_enable(ov7251->io_regulator);
>>>
>>> How about regulator_bulk_enable() here, and bulk_disable below?
>>
>> I'm not using the bulk API because usually there is a power up
>> sequence and intervals that must be followed. For this sensor
>> the only constraint is that core regulator must be enabled
>> after io regulator. But the bulk API doesn't guarantee the
>> order.
>
> Could you add a comment explaining this? Otherwise it won't take long until
> someone "fixes" the code.
Sure :D
I'm adding a comment.
>
> ...
>
>>>> +static int ov7251_read_reg(struct ov7251 *ov7251, u16 reg, u8 *val)
>>>> +{
>>>> + u8 regbuf[2];
>>>> + int ret;
>>>> +
>>>> + regbuf[0] = reg >> 8;
>>>> + regbuf[1] = reg & 0xff;
>>>> +
>>>> + ret = i2c_master_send(ov7251->i2c_client, regbuf, 2);
>>>> + if (ret < 0) {
>>>> + dev_err(ov7251->dev, "%s: write reg error %d: reg=%x\n",
>>>> + __func__, ret, reg);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + ret = i2c_master_recv(ov7251->i2c_client, val, 1);
>>>> + if (ret < 0) {
>>>> + dev_err(ov7251->dev, "%s: read reg error %d: reg=%x\n",
>>>> + __func__, ret, reg);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int ov7251_set_exposure(struct ov7251 *ov7251, s32 exposure)
>>>> +{
>>>> + int ret;
>>>> +
>>>> + ret = ov7251_write_reg(ov7251, OV7251_AEC_EXPO_0,
>>>> + (exposure & 0xf000) >> 12);
>>>> + if (ret < 0)
>>>> + return ret;
>>>> +
>>>> + ret = ov7251_write_reg(ov7251, OV7251_AEC_EXPO_1,
>>>> + (exposure & 0x0ff0) >> 4);
>>>> + if (ret < 0)
>>>> + return ret;
>>>> +
>>>> + return ov7251_write_reg(ov7251, OV7251_AEC_EXPO_2,
>>>> + (exposure & 0x000f) << 4);
>>>
>>> It's not a good idea to access multi-octet registers separately. Depending
>>> on the hardware implementation, the hardware could latch the value in the
>>> middle of an update. This is only an issue during streaming in practice
>>> though.
>>
>> Good point. The sensor has a group write functionality which can be used
>> to solve this but in general is intended
>> to apply a group of exposure and gain settings in the same frame. However
>> it seems to me that is not possible to use this functionality efficiently
>> with the currently available user controls. The group write is configured
>> using an id for a group of commands. So if we configure exposure and gain
>> separately (a group for each):
>> - if the driver uses same group id for exposure and gain, if both controls
>> are received in one frame the second will overwrite the first (the
>> first will not be applied);
>> - if the driver uses different group id for exposure and gain, it will not
>> be possible for the user to change exposure and gain in the same frame
>> (as some exposure algorithms do) and it will lead again to frames with
>> "incorrect" brightness.
>>
>> To do this correctly we will have to extend the API to be able to apply
>> exposure and gain "atomically":
>> - a single user control which will set both exposure and gain and it will
>> guarantee that they will be applied in the same frame;
>> - some kind of: begin, set exposure, set gain, end, launch -API
>>
>> What do you think?
>>
>> Actually, I'm a little bit surprised that I didn't find anything
>> like this already. And there are already a number of sensor drivers
>> which update more than one register to set exposure.
>
> The latter of the two would be preferred as it isn't limited to exposure
> and gain only. Still, you could address the problem for this driver by
> simply writing the register in a single transaction.
Thanks for suggestion. I've tried the single transaction, I will send the
next version of the driver shortly.
--
Best regards,
Todor Tomov
