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?
>
> 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
--
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
