Hi,
On 10/26/2016 07:48 PM, Ian Arkver wrote:
> On 26/10/16 15:07, Todor Tomov wrote:
>> Hi,
>>
>> On 10/26/2016 03:48 PM, Ian Arkver wrote:
>>> [snip]
>>>>>>>>> +static int ov5645_regulators_enable(struct ov5645 *ov5645)
>>>>>>>>> +{
>>>>>>>>> + int ret;
>>>>>>>>> +
>>>>>>>>> + ret = regulator_enable(ov5645->io_regulator);
>>>>>>>>> + if (ret < 0) {
>>>>>>>>> + dev_err(ov5645->dev, "set io voltage failed\n");
>>>>>>>>> + return ret;
>>>>>>>>> + }
>>>>>>>>> +
>>>>>>>>> + ret = regulator_enable(ov5645->core_regulator);
>>>>>>>>> + if (ret) {
>>>>>>>>> + dev_err(ov5645->dev, "set core voltage failed\n");
>>>>>>>>> + goto err_disable_io;
>>>>>>>>> + }
>>>>>>>>> +
>>>>>>>>> + ret = regulator_enable(ov5645->analog_regulator);
>>>>>>>>> + if (ret) {
>>>>>>>>> + dev_err(ov5645->dev, "set analog voltage failed\n");
>>>>>>>>> + goto err_disable_core;
>>>>>>>>> + }
>>>>>>>> How about using the regulator bulk API ? This would simplify the enable
>>>>>>>> and disable functions.
>>>>>>> The driver must enable the regulators in this order. I can see in the
>>>>>>> implementation of the bulk api that they are enabled again in order
>>>>>>> but I don't see stated anywhere that it is guaranteed to follow the
>>>>>>> same order in future. I'd prefer to keep it explicit as it is now.
>>>>>> I believe it should be an API guarantee, otherwise many drivers using
>>>>>> the bulk
>>>>>> API would break. Mark, could you please comment on that ?
>>>>> Ok, let's wait for a response from Mark.
>>> I don't have the OV5645 datasheet, but I do have the OV5640 and OV5647
>>> datasheets. Both of these show that AVDD should come up before DVDD where
>>> DVDD is externally supplied, although the minimum delay between them is
>>> 0ms. Both datasheets also imply that this requirement is only to allow host
>>> SCCB access on a shared I2C bus while the device is powering up. They imply
>>> that if one waits 20ms after power on then SCCB will be fine without this
>>> sequencing. Your code includes an msleep(20);
>> There is also requirement that DOVDD should become stable before AVDD (in
>> both cases -
>> external or internal DVDD).
>>
>> Aren't these requirements needed to allow I2C access to another device on
>> the same I2C bus even during these 20ms?
> Well, it's a really obscure corner case where these rails are actually
> switched and the rise times are all available to the regulator framework (so
> that there's a difference between three distinct calls to regulator_enable
> and one call to the ASYNC_DOMAIN driven bulk enable) and the I2C bus is
> shared with another device that is being accessed at the same time as the
> sensor is enabled and the sensor breaks that access.
>
> Having said that, really obscure corner cases are what lurk around and catch
> you unawares years in the future, so maybe three calls is necessary here.
> However, I think analog should be enabled before core - check with your
> datasheet if you have the correct one.
Yes, analog (AVDD) should be enabled before core (DVDD);
and I/O (DOVDD) should be enabled before analog.
I also don't think that the benefit of using the bulk API (like shorter and
simpler code) is worth against the possibility of introducing a potential
problem (even if it is that rare).
But in any case, thank you for reviewing my code!
>
> Regards,
> Ian
>
>>
>>> Further, the reference schematic for the OV5647 shows three separate LDOs
>>> with no sequencing between them.
>>>
>>> I've no comment on whether the bulk regulator API needs a "keep sequencing"
>>> flag or somesuch, but I don't think this device will drive that requirement.
>>>
>>> Regards,
>>> Ian
>>>
>> Best regards,
>> Todor
>>
>
--
Best regards,
Todor Tomov
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
