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