Re: [PATCH v2] soc_camera: Add the ability to bind regulators to soc_camedra devices

[Guennadi Liakhovetski <g.liakhovetski@gmx.de>]

On Tue, 30 Nov 2010, Alberto Panizzo wrote:

> In certain machines, camera devices are supplied directly
> by a number of regulators. This patch add the ability to drive
> these regulators directly by the soc_camera driver.
> 
> Signed-off-by: Alberto Panizzo <maramaopercheseimorto@gmail.com>
> ---
> 
> v2 changes:
> It is used the more standard regulator_bulk API, thanks to
> Mark Brown for pointing this.
> 
>  drivers/media/video/soc_camera.c |   73 +++++++++++++++++++++++++++-----------
>  include/media/soc_camera.h       |    5 +++
>  2 files changed, 57 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c
> index 43848a7..f1c2094 100644
> --- a/drivers/media/video/soc_camera.c
> +++ b/drivers/media/video/soc_camera.c

Have to

#include <linux/regulator/consumer.h>

here

> @@ -43,6 +43,41 @@ static LIST_HEAD(hosts);
>  static LIST_HEAD(devices);
>  static DEFINE_MUTEX(list_lock);		/* Protects the list of hosts */
>  
> +static int soc_camera_power_set(struct soc_camera_device *icd,
> +				struct soc_camera_link *icl,
> +				int power_on)
> +{
> +	int ret = 0;

"= 0" unneeded.

> +
> +	if (power_on) {
> +		ret = regulator_bulk_enable(icl->num_regulators,
> +					    icl->regulators);
> +	} else {
> +		ret = regulator_bulk_disable(icl->num_regulators,
> +					     icl->regulators);
> +	}

superfluous braces

> +	if (ret < 0) {
> +		dev_err(icd->pdev, "Cannot %s regulators",
> +			power_on ? "ENABLE" : "DISABLE");

why capitals?

> +		goto err;

just return ret

> +	}
> +
> +	if (icl->power) {
> +		ret = icl->power(icd->pdev, power_on);
> +		if (ret < 0) {
> +			dev_err(icd->pdev,
> +				"Platform failed to power %s the camera.\n",
> +				power_on ? "ON" : "OFF");

why capitals?

> +			goto err;

just return ret, although, if switching on failed, and the platform us 
also using regulators, don't you want to turn them off? Still I would do 
this here inline without a goto, but that's already a matter of taste, so, 
if you prefer, in this case a goto would be justified.

> +		}
> +	}
> +
> +	return 0;
> +
> +err:
> +	return ret;

with the above, this might go.

> +}
> +
>  const struct soc_camera_format_xlate *soc_camera_xlate_by_fourcc(
>  	struct soc_camera_device *icd, unsigned int fourcc)
>  {
> @@ -375,11 +410,9 @@ static int soc_camera_open(struct file *file)
>  			},
>  		};
>  
> -		if (icl->power) {
> -			ret = icl->power(icd->pdev, 1);
> -			if (ret < 0)
> -				goto epower;
> -		}
> +		ret = soc_camera_power_set(icd, icl, 1);
> +		if (ret < 0)
> +			goto epower;
>  
>  		/* The camera could have been already on, try to reset */
>  		if (icl->reset)
> @@ -425,8 +458,7 @@ esfmt:
>  eresume:
>  	ici->ops->remove(icd);
>  eiciadd:
> -	if (icl->power)
> -		icl->power(icd->pdev, 0);
> +	soc_camera_power_set(icd, icl, 0);
>  epower:
>  	icd->use_count--;
>  	mutex_unlock(&icd->video_lock);
> @@ -450,8 +482,7 @@ static int soc_camera_close(struct file *file)
>  
>  		ici->ops->remove(icd);
>  
> -		if (icl->power)
> -			icl->power(icd->pdev, 0);
> +		soc_camera_power_set(icd, icl, 0);
>  	}
>  
>  	if (icd->streamer == file)
> @@ -941,14 +972,14 @@ static int soc_camera_probe(struct device *dev)
>  
>  	dev_info(dev, "Probing %s\n", dev_name(dev));
>  
> -	if (icl->power) {
> -		ret = icl->power(icd->pdev, 1);
> -		if (ret < 0) {
> -			dev_err(dev,
> -				"Platform failed to power-on the camera.\n");
> -			goto epower;
> -		}
> -	}
> +	ret = regulator_bulk_get(icd->pdev, icl->num_regulators,
> +				 icl->regulators);
> +	if (ret)

"if (ret < 0)" for consistency, please

> +		goto epower;
> +
> +	ret = soc_camera_power_set(icd, icl, 1);
> +	if (ret < 0)
> +		goto epower;

You need a new label for this error - you also have to free regulators, if 
this fails.

>  
>  	/* The camera could have been already on, try to reset */
>  	if (icl->reset)
> @@ -1021,8 +1052,7 @@ static int soc_camera_probe(struct device *dev)
>  
>  	ici->ops->remove(icd);
>  
> -	if (icl->power)
> -		icl->power(icd->pdev, 0);
> +	soc_camera_power_set(icd, icl, 0);
>  
>  	mutex_unlock(&icd->video_lock);
>  
> @@ -1044,8 +1074,7 @@ eadddev:
>  evdc:
>  	ici->ops->remove(icd);
>  eadd:
> -	if (icl->power)
> -		icl->power(icd->pdev, 0);
> +	soc_camera_power_set(icd, icl, 0);
>  epower:
>  	return ret;
>  }
> @@ -1081,6 +1110,8 @@ static int soc_camera_remove(struct device *dev)
>  	}
>  	soc_camera_free_user_formats(icd);
>  
> +	regulator_bulk_free(icl->num_regulators, icl->regulators);
> +
>  	return 0;
>  }
>  
> diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
> index 86e3631..3e6b903 100644
> --- a/include/media/soc_camera.h
> +++ b/include/media/soc_camera.h
> @@ -15,6 +15,7 @@
>  #include <linux/device.h>
>  #include <linux/mutex.h>
>  #include <linux/pm.h>
> +#include <linux/regulator/consumer.h>

No need to include the header here, just declare

struct regulator_bulk_data;

>  #include <linux/videodev2.h>
>  #include <media/videobuf-core.h>
>  #include <media/v4l2-device.h>
> @@ -108,6 +109,10 @@ struct soc_camera_link {
>  	const char *module_name;
>  	void *priv;
>  
> +	/* Optional regulators that have to be managed on power on/off events */
> +	struct regulator_bulk_data *regulators;
> +	int num_regulators;
> +
>  	/*
>  	 * For non-I2C devices platform platform has to provide methods to
>  	 * add a device to the system and to remove
> -- 
> 1.6.3.3

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