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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
