Re: [PATCH 1/3] soc_camera: Add the ability to bind regulators to soc_camedra devices
On lun, 2010-11-29 at 15:51 +, Mark Brown wrote:
On Mon, Nov 29, 2010 at 10:34:57AM +0100, Alberto Panizzo wrote:
On dom, 2010-11-28 at 20:05 +0100, Guennadi Liakhovetski wrote:
(2) would anyone really want to
use several regulators for a camera sensor? If so, can it be the case,
that, for example, the regulators have to be switched off in the reverse
order to switching on? Or something else?
Well, I'm working on the i.MX31 3 Stack board support and there are 2
regulators that powers the camera and if you consider the digital output
that enable another supplier needed, the total regulators are three..
So, yes a list of regulators is needed in this case, and Yes I did not
considered the order of enabling and disabling operations. Just because
even the freescale drivers didn't.
A practical general rule is to turn off switchers in the reverse order
than the turning on one. And this can be easily implemented here.
But as you rose the question, we can add priorities of turning on and
off.
If you use the regulator bulk API it'll reverse the ordering when doing
the power down (or should if it doesn't already).
Great API regulator_bulk, let's get it a try! ..I was reinventing the
hot water..
+static int soc_camera_power_set(struct soc_camera_device *icd,
+ struct soc_camera_link *icl,
+ int power_on)
+{
+ int ret, i;
+
+ for (i = 0; i icd-num_soc_regulators; i++) {
+ if (power_on) {
+ ret =
regulator_set_voltage(icd-soc_regulators[i],
+
icl-soc_regulator_descs[i].value_on_min,
+
icl-soc_regulator_descs[i].value_on_max);
Unless you're actively varying the voltages at runtime (as Guennadi
mentioned) I'd really expect the voltages to be handled by the regulator
constraints.
This is sane thinking at a static board configuration. What if a single
board have different deploying schemas where two different cameras can
be placed on the same connector and these cameras have different voltage
levels for core supply? This scenario will require two different
constraints chosen at compile time - two different kernel binaries.
Otherwise constraints will always pick the minimum level and maybe this
will not be enough.
Best regards,
--
Alberto!
Be Persistent!
- Greg Kroah-Hartman (FOSDEM 2010)
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] soc_camera: Add the ability to bind regulators to soc_camedra devices
On Tue, Nov 30, 2010 at 11:45:23AM +0100, Alberto Panizzo wrote: On lun, 2010-11-29 at 15:51 +, Mark Brown wrote: Unless you're actively varying the voltages at runtime (as Guennadi mentioned) I'd really expect the voltages to be handled by the regulator constraints. This is sane thinking at a static board configuration. What if a single board have different deploying schemas where two different cameras can be placed on the same connector and these cameras have different voltage levels for core supply? This scenario will require two different constraints chosen at compile time - two different kernel binaries. Otherwise constraints will always pick the minimum level and maybe this will not be enough. The way I'd expect to see that handled is that the constraints would be chosen when the module is detected, possibly by having a platform device representing the assembly as a whole. There was someone working with plugin modules doing something along those lines. I certainly wouldn't expect the device itself to worry about this unless it was actively doing something to manage the voltage. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] soc_camera: Add the ability to bind regulators to soc_camedra devices
Hi Guennadi,
On dom, 2010-11-28 at 20:05 +0100, Guennadi Liakhovetski wrote:
On Sun, 28 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.
IIRC, there has been a discussion a while ago, how to supply power to
cameras by regulators. Someone has tried to provide a .power() hook in the
platform code, but the problem was the order of driver loading / probing.
So, how about doing the following:
1. in your platform code you register a notifier like
bus_register_notifier(soc_camera_bus_type, cam_notifier);
2. in your notifier you verify, that the driver is already available, or
request_module(my-regulator-driver), and use another notifier to wait,
until the driver is probed.
3. if the above has been successful, in your .power() method you just use
the regulator as usual.
Notice, that your current patch doesn't load regulator driver modules, so,
it also relies on them being already available at the time of camera
probing. If you can live with that restriction, you can skip loading the
module and waiting for its probe above too.
The reason I propose this implementation is because in a different
subsystem (mmc) the regulator management is similar and accepted.
In that case, platform code that drive regulators directly is not
considered successful and it is maintained a platform .power() function
that can manage other operations than regulators driving.
The power of binding regulators to devices, I think, is given by the
ability to fine manage these objects in terms of power management.
But of course you are right in some objections:
The reasons why I do not want to add this to the core are: (1) I do not
want to have two methods for turning power on and off: a platform provided
.power() hook and and a set of regulators,
Why? The implementation I gave, abstract these power management actions
in a single layer. Of course I had to manage the way of passing
preferred voltage values and the results are not perfect.
(2) would anyone really want to
use several regulators for a camera sensor? If so, can it be the case,
that, for example, the regulators have to be switched off in the reverse
order to switching on? Or something else?
Well, I'm working on the i.MX31 3 Stack board support and there are 2
regulators that powers the camera and if you consider the digital output
that enable another supplier needed, the total regulators are three..
So, yes a list of regulators is needed in this case, and Yes I did not
considered the order of enabling and disabling operations. Just because
even the freescale drivers didn't.
A practical general rule is to turn off switchers in the reverse order
than the turning on one. And this can be easily implemented here.
But as you rose the question, we can add priorities of turning on and
off.
(3) regulators can often do
more, than just set one of two power levels - for on and off. What if a
need arises to use other voltages?
Well, you are thinking at the possibility to have one camera that have
different voltage levels needs during the ON time (between open and a
close).
IMHO I think that this could be considered only if a power management
event happen and we decide that, in some way, the camera chip can be
maintained powered with a different voltage level instead of turning
it off and reinitializing it on resume.
Is this really the situation you are trying to address?
Is there any really good reason, why we _have_ to do this in soc-camera
core?
There are three options in regulators management:
1 Pure platform code.
2 Pure driver code.
3 subsystem code.
The first is the one you are suggesting me, but other subsystems
rejected it.
The third IMHO is better than the first but solve the needs of the
single driver.
The second is the one I proposed and I think is the best way because
add a general framework to manage regulators in this subsystem.
Thanks you!
Alberto!
What the machine code have to do to use this functionality is to:
1- Define a number of useful regulator supply descriptions such as:
static struct regulator_consumer_supply camera_reg1_consumers[] = {
...
REGULATOR_SUPPLY(camera_reg1, soc-camera-pdrv.0),
...
};
(Pay attention at the .N suffix of soc-camera-pdrv in case of
a system with multiple cameras)
2- Define the list of regulators to bind to a specific instance of
soc-camera-pdrv with their voltages:
static struct soc_camera_regulator_desc soc_camera_regs[] = {
SOCAM_REG_DESC(camera_reg1, 130, 130),
SOCAM_REG_DESC(camera_reg2, 280, 280),
...
};
3- Add the list to the corresponding soc_camera_link description:
static struct soc_camera_link iclink_my_camera = {
...
.soc_regulator_descs= soc_camera_regs,
Re: [PATCH 1/3] soc_camera: Add the ability to bind regulators to soc_camedra devices
On Sun, Nov 28, 2010 at 08:05:06PM +0100, Guennadi Liakhovetski wrote: On Sun, 28 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. IIRC, there has been a discussion a while ago, how to supply power to cameras by regulators. Someone has tried to provide a .power() hook in the platform code, but the problem was the order of driver loading / probing. So, how about doing the following: 1. in your platform code you register a notifier like bus_register_notifier(soc_camera_bus_type, cam_notifier); FWIW I'm looking at implementing a standard regulator API feature along these lines in the background. This should hopefully mean we don't need driver support for most simple power control applications. No ETA yet. The reasons why I do not want to add this to the core are: (1) I do not want to have two methods for turning power on and off: a platform provided .power() hook and and a set of regulators, (2) would anyone really want to use several regulators for a camera sensor? If so, can it be the case, that, for example, the regulators have to be switched off in the reverse order to switching on? Or something else? (3) regulators can often do more, than just set one of two power levels - for on and off. What if a need arises to use other voltages? The way MMC handled this was to provide a standard version of the hook in the core which could be used by platforms with regulators supplying the device - they just assign the appropriate function as their power() operation AIUI. That seems a fairly clean way of keeping stuff in the core without giving up the flexibility. Is there any really good reason, why we _have_ to do this in soc-camera core? It does save everyone open coding stuff. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] soc_camera: Add the ability to bind regulators to soc_camedra devices
On Mon, Nov 29, 2010 at 10:34:57AM +0100, Alberto Panizzo wrote:
On dom, 2010-11-28 at 20:05 +0100, Guennadi Liakhovetski wrote:
(2) would anyone really want to
use several regulators for a camera sensor? If so, can it be the case,
that, for example, the regulators have to be switched off in the reverse
order to switching on? Or something else?
Well, I'm working on the i.MX31 3 Stack board support and there are 2
regulators that powers the camera and if you consider the digital output
that enable another supplier needed, the total regulators are three..
So, yes a list of regulators is needed in this case, and Yes I did not
considered the order of enabling and disabling operations. Just because
even the freescale drivers didn't.
A practical general rule is to turn off switchers in the reverse order
than the turning on one. And this can be easily implemented here.
But as you rose the question, we can add priorities of turning on and
off.
If you use the regulator bulk API it'll reverse the ordering when doing
the power down (or should if it doesn't already).
+static int soc_camera_power_set(struct soc_camera_device *icd,
+ struct soc_camera_link *icl,
+ int power_on)
+{
+ int ret, i;
+
+ for (i = 0; i icd-num_soc_regulators; i++) {
+ if (power_on) {
+ ret = regulator_set_voltage(icd-soc_regulators[i],
+ icl-soc_regulator_descs[i].value_on_min,
+ icl-soc_regulator_descs[i].value_on_max);
Unless you're actively varying the voltages at runtime (as Guennadi
mentioned) I'd really expect the voltages to be handled by the regulator
constraints.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] soc_camera: Add the ability to bind regulators to soc_camedra devices
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.
What the machine code have to do to use this functionality is to:
1- Define a number of useful regulator supply descriptions such as:
static struct regulator_consumer_supply camera_reg1_consumers[] = {
...
REGULATOR_SUPPLY(camera_reg1, soc-camera-pdrv.0),
...
};
(Pay attention at the .N suffix of soc-camera-pdrv in case of
a system with multiple cameras)
2- Define the list of regulators to bind to a specific instance of
soc-camera-pdrv with their voltages:
static struct soc_camera_regulator_desc soc_camera_regs[] = {
SOCAM_REG_DESC(camera_reg1, 130, 130),
SOCAM_REG_DESC(camera_reg2, 280, 280),
...
};
3- Add the list to the corresponding soc_camera_link description:
static struct soc_camera_link iclink_my_camera = {
...
.soc_regulator_descs= soc_camera_regs,
.num_soc_regulator_descs= ARRAY_SIZE(soc_camera_regs),
};
4- And register it as usual with the platform device description:
static struct platform_device machine_my_camera = {
.name = soc-camera-pdrv,
.id = 0,
.dev= {
.platform_data = iclink_my_camera,
},
};
Signed-off-by: Alberto Panizzo maramaopercheseimo...@gmail.com
---
drivers/media/video/soc_camera.c | 135 +++--
include/media/soc_camera.h | 16 +
2 files changed, 129 insertions(+), 22 deletions(-)
diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c
index 43848a7..8fc5831 100644
--- a/drivers/media/video/soc_camera.c
+++ b/drivers/media/video/soc_camera.c
@@ -43,6 +43,96 @@ static LIST_HEAD(hosts);
static LIST_HEAD(devices);
static DEFINE_MUTEX(list_lock);/* Protects the list of hosts */
+static int soc_camera_setup_regulators(struct soc_camera_device *icd,
+ struct soc_camera_link *icl)
+{
+ int i, ret;
+
+ icd-soc_regulators = kzalloc(icl-num_soc_regulator_descs *
+ sizeof(struct regulator *), GFP_KERNEL);
+ if (!icd-soc_regulators) {
+ dev_err(icd-pdev, Not enough memory.\n);
+ ret = -ENOMEM;
+ goto err;
+ }
+
+ for (i = 0; i icl-num_soc_regulator_descs; i++) {
+ dev_dbg(icd-pdev, Looking for reg:'%s' bound to dev:'%s',
+ icl-soc_regulator_descs[i].supply,
+ dev_name(icd-pdev));
+ icd-soc_regulators[i] = regulator_get(icd-pdev,
+ icl-soc_regulator_descs[i].supply);
+ if (IS_ERR(icd-soc_regulators[i])) {
+ icd-soc_regulators[i] = NULL;
+ dev_err(icd-pdev, Unable to get regulator: \%s\.\n,
+ icl-soc_regulator_descs[i].supply);
+ ret = -ENODEV;
+ goto free_regs;
+ }
+ }
+
+ icd-num_soc_regulators = icl-num_soc_regulator_descs;
+
+ return 0;
+
+free_regs:
+ for (i--; i = 0; i--)
+ regulator_put(icd-soc_regulators[i]);
+err:
+ return ret;
+}
+
+static int soc_camera_power_set(struct soc_camera_device *icd,
+ struct soc_camera_link *icl,
+ int power_on)
+{
+ int ret, i;
+
+ for (i = 0; i icd-num_soc_regulators; i++) {
+ if (power_on) {
+ ret = regulator_set_voltage(icd-soc_regulators[i],
+ icl-soc_regulator_descs[i].value_on_min,
+ icl-soc_regulator_descs[i].value_on_max);
+ if (ret) {
+ dev_err(icd-pdev, Cannot set '%s' to %d:%d,
+ icl-soc_regulator_descs[i].supply,
+ icl-soc_regulator_descs[i].value_on_min,
+ icl-soc_regulator_descs[i].value_on_max);
+ goto err;
+ }
+
+ ret = regulator_enable(icd-soc_regulators[i]);
+ if (ret 0) {
+ dev_err(icd-pdev, Cannot enable reg '%s',
+ icl-soc_regulator_descs[i].supply);
+ goto err;
+ }
+ } else {
+ ret = regulator_disable(icd-soc_regulators[i]);
+ if (ret) {
+ dev_err(icd-pdev, Cannot disable reg '%s',
+ icl-soc_regulator_descs[i].supply);
+ goto err;
+ }
+ }
+ }
+
+ if
Re: [PATCH 1/3] soc_camera: Add the ability to bind regulators to soc_camedra devices
On Sun, 28 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.
IIRC, there has been a discussion a while ago, how to supply power to
cameras by regulators. Someone has tried to provide a .power() hook in the
platform code, but the problem was the order of driver loading / probing.
So, how about doing the following:
1. in your platform code you register a notifier like
bus_register_notifier(soc_camera_bus_type, cam_notifier);
2. in your notifier you verify, that the driver is already available, or
request_module(my-regulator-driver), and use another notifier to wait,
until the driver is probed.
3. if the above has been successful, in your .power() method you just use
the regulator as usual.
Notice, that your current patch doesn't load regulator driver modules, so,
it also relies on them being already available at the time of camera
probing. If you can live with that restriction, you can skip loading the
module and waiting for its probe above too.
The reasons why I do not want to add this to the core are: (1) I do not
want to have two methods for turning power on and off: a platform provided
.power() hook and and a set of regulators, (2) would anyone really want to
use several regulators for a camera sensor? If so, can it be the case,
that, for example, the regulators have to be switched off in the reverse
order to switching on? Or something else? (3) regulators can often do
more, than just set one of two power levels - for on and off. What if a
need arises to use other voltages?
Is there any really good reason, why we _have_ to do this in soc-camera
core?
Thanks
Guennadi
What the machine code have to do to use this functionality is to:
1- Define a number of useful regulator supply descriptions such as:
static struct regulator_consumer_supply camera_reg1_consumers[] = {
...
REGULATOR_SUPPLY(camera_reg1, soc-camera-pdrv.0),
...
};
(Pay attention at the .N suffix of soc-camera-pdrv in case of
a system with multiple cameras)
2- Define the list of regulators to bind to a specific instance of
soc-camera-pdrv with their voltages:
static struct soc_camera_regulator_desc soc_camera_regs[] = {
SOCAM_REG_DESC(camera_reg1, 130, 130),
SOCAM_REG_DESC(camera_reg2, 280, 280),
...
};
3- Add the list to the corresponding soc_camera_link description:
static struct soc_camera_link iclink_my_camera = {
...
.soc_regulator_descs= soc_camera_regs,
.num_soc_regulator_descs= ARRAY_SIZE(soc_camera_regs),
};
4- And register it as usual with the platform device description:
static struct platform_device machine_my_camera = {
.name = soc-camera-pdrv,
.id = 0,
.dev= {
.platform_data = iclink_my_camera,
},
};
Signed-off-by: Alberto Panizzo maramaopercheseimo...@gmail.com
---
drivers/media/video/soc_camera.c | 135 +++--
include/media/soc_camera.h | 16 +
2 files changed, 129 insertions(+), 22 deletions(-)
diff --git a/drivers/media/video/soc_camera.c
b/drivers/media/video/soc_camera.c
index 43848a7..8fc5831 100644
--- a/drivers/media/video/soc_camera.c
+++ b/drivers/media/video/soc_camera.c
@@ -43,6 +43,96 @@ static LIST_HEAD(hosts);
static LIST_HEAD(devices);
static DEFINE_MUTEX(list_lock); /* Protects the list of hosts */
+static int soc_camera_setup_regulators(struct soc_camera_device *icd,
+ struct soc_camera_link *icl)
+{
+ int i, ret;
+
+ icd-soc_regulators = kzalloc(icl-num_soc_regulator_descs *
+ sizeof(struct regulator *), GFP_KERNEL);
+ if (!icd-soc_regulators) {
+ dev_err(icd-pdev, Not enough memory.\n);
+ ret = -ENOMEM;
+ goto err;
+ }
+
+ for (i = 0; i icl-num_soc_regulator_descs; i++) {
+ dev_dbg(icd-pdev, Looking for reg:'%s' bound to dev:'%s',
+ icl-soc_regulator_descs[i].supply,
+ dev_name(icd-pdev));
+ icd-soc_regulators[i] = regulator_get(icd-pdev,
+ icl-soc_regulator_descs[i].supply);
+ if (IS_ERR(icd-soc_regulators[i])) {
+ icd-soc_regulators[i] = NULL;
+ dev_err(icd-pdev, Unable to get regulator: \%s\.\n,
+ icl-soc_regulator_descs[i].supply);
+ ret = -ENODEV;
+ goto free_regs;
+ }
+ }
+
+ icd-num_soc_regulators = icl-num_soc_regulator_descs;
+
+ return 0;
+
+free_regs:
+ for (i--; i = 0; i--)
+ regulator_put(icd-soc_regulators[i]);
+err:
+ return ret;
+}
+
