[PATCH v3] ARM: i.MX27 pcm970: Add camera support

[g.liakhovetski@gmx.de (Guennadi Liakhovetski)]

On Wed, 18 Aug 2010, Mark Brown wrote:

> On Wed, Aug 18, 2010 at 10:05:25PM +0200, Guennadi Liakhovetski wrote:
> 
> > 1. unbalanced regulator enable. IMHO, this is no exception to the general 
> > enable / disable, alloc / free, lock / unlock etc. call balancing rule. If 
> > your regulator has to stay enabled for the whole system run-time, you 
> > don't have to (even try to) enable it multiple times. Please, either make 
> > enabling / disabling symmetric, or leave only one enable in the 
> > initialisation call.
> 
> If the regulator needs to be on all the time on a given board then
> forcing this with always_on in the constraints is much more idiomatic
> and straightforward.
> 
> > > +static int pcm970_camera_power_bus(struct device *dev, int toggle)
> > > +{
> > > +	struct regulator *regulator;
> > > +
> > > +	regulator = regulator_get(NULL, "imx_cam_vcc");
> > > +	if (IS_ERR(regulator)) {
> > > +		pr_err("unable to get regulator: %ld\n", PTR_ERR(regulator));
> > > +		return -ENODEV;
> > > +	} else {
> > > +		if (!regulator_is_enabled(regulator) && toggle)
> > > +			regulator_enable(regulator);
> > > +	}
> > > +	return 0;
> > > +}
> 
> This is all very suspicious - your regulator_get() should be using the
> struct device it was passed to look up the regulator by supply name
> (probably "cam_vcc").  The regulator_is_enabled() call is going to break
> if the supply is shared, too.
> 
> It all looks like something that I'd expect the camera driver to be
> taking care of for itself, there's nothing board specific in here.
> 
> > > +	regulator = regulator_get(NULL, "imx_cam_vcc");
> > > +	if (IS_ERR(regulator)) {
> > > +		pr_err("unable to get regulator: %ld\n", PTR_ERR(regulator));
> > > +		return -ENODEV;
> > > +	} else if (!regulator_is_enabled(regulator)) {
> > > +		regulator_enable(regulator);
> > > +	}
> 
> Again, this looks like you need to add regulator support to the camera
> driver.

No, don't think so. This is a generic camera sensor, that can be attached 
to any video-capable SoC or built into a USB webcam. It knows nothing 
about where it's getting its power from.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/