Re: [PATCH v6 2/2] media: Add a driver for the ov5645 camera sensor.

[Ian Arkver <ian.arkver.dev@gmail.com>]

[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);

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