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